CPUID (2/3): Bit fields

[JonathanWoollett-Light]

#3104 must be merged first

Reason for This PR

Working with the large number of values smaller than u8 required for CPUID quickly becomes untenable. With thousands of constants denoting offsets.

bitflags does not support fields which are required for CPUID. To use bitflags here would require high hundreds (possibly 1000+) of additional loosely associated constants to reference these fields within the bitflag structures. This would form a large and difficult to read implementation.

bitfield does not support exotically sized bit ranges (e.g. bits 2-7). To use this would require heavy adaption.

Description of Changes

Introduces bit-fields and bit-fields-macros crates to support handling these values. These function as one crate¹ inacting a similar role to bitflags except with additional support for fields within bytes used for unsigned integer values.

Linting

bit-fields and bit-fields-macros raise the bar in safety adhering to much stricter linting. This is defined with:

#![warn(clippy::pedantic, clippy::restriction)]
#![allow(
    clippy::blanket_clippy_restriction_lints,
    clippy::implicit_return,
    clippy::pattern_type_mismatch,
    clippy::std_instead_of_alloc,
    clippy::std_instead_of_core,
    clippy::pub_use,
    clippy::non_ascii_literal,
    clippy::single_char_lifetime_names,
    clippy::exhaustive_enums,
    clippy::exhaustive_structs,
    clippy::unseparated_literal_suffix,
    clippy::mod_module_files
)]

Beginning from the strictest possible linting #![warn(clippy::pedantic, clippy::restriction)] then disabling some of the lints within clippy::restriction to make it reasonable.

Testing & Coverage

Coverage = ~97% or ~75% with grcov or ~93% with cargo-llvm-cov

Both crates feature extensive testing. Due to #3206 when running tests within the container and in CI coverage is greatly reduced to ~40% (recent-coverage.zip).

When running with doc tests:

[ec2-user@ip-172-31-15-148 bit-fields]$ env RUSTFLAGS="-C instrument-coverage" RUSTDOCFLAGS="-C instrument-coverage -Z unstable-options --persist-doctests ../../target/debug/doctestbins" LLVM_PROFILE_FILE="coverage-%p-%m.profraw" cargo +nightly test
warning: patch for `kvm-bindings` uses the features mechanism. default-features and features will not take effect because the patch dependency does not support this mechanism
   Compiling proc-macro2 v1.0.43
   Compiling unicode-ident v1.0.3
   Compiling quote v1.0.21
   Compiling syn v1.0.99
   Compiling libc v0.2.117
   Compiling serde_derive v1.0.144
   Compiling cfg-if v1.0.0
   Compiling serde v1.0.144
   Compiling serde_json v1.0.85
   Compiling ppv-lite86 v0.2.16
   Compiling ryu v1.0.9
   Compiling itoa v1.0.1
   Compiling getrandom v0.2.4
   Compiling rand_core v0.6.3
   Compiling rand_chacha v0.3.1
   Compiling rand v0.8.5
   Compiling thiserror-impl v1.0.32
   Compiling thiserror v1.0.32
   Compiling bit-fields-macros v0.1.0 (/home/ec2-user/firecracker/src/bit-fields-macros)
   Compiling bit-fields v0.1.0 (/home/ec2-user/firecracker/src/bit-fields)
    Finished test [unoptimized + debuginfo] target(s) in 10.14s
     Running unittests src/lib.rs (/home/ec2-user/firecracker/target/debug/deps/bit_fields-0c4aac3aa3031440)

running 15 tests
test tests::access ... ok
test bit_range::tests::mask ... ok
test tests::checked_add_assign ... ok
test tests::checked_sub_assign ... ok
test tests::display ... ok
test tests::on_off ... ok
test tests::set ... ok
test tests::size ... ok
test tests::flip ... ok
test tests::conversion ... ok
test tests::type_max ... ok
test tests::value_max ... ok
test tests::serialize ... ok
test tests::checked_assign ... ok
test tests::add_sub ... ok

test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.49s

   Doc-tests bit-fields

running 157 tests
test src/bit.rs - bit::BitMut::off (line 290) ... ok
test src/bit.rs - bit::BitMut::flip (line 287) ... ok
test src/bit.rs - bit::Bit::is_on (line 123) ... ok
test src/bit.rs - bit::Bit::is_off (line 121) ... ok
...
test src/bit_range.rs - bit_range::BitRangeMut::fmt (line 60) ... ok
test src/bit_range.rs - bit_range::BitRangeMut::checked_sub_assign (line 371) ... ok
test src/bit_range.rs - bit_range::BitRangeMut::checked_sub_assign (line 368) ... ok
test src/bit_range.rs - bit_range::BitRangeMut::checked_add_assign (line 367) ... ok

test result: ok. 155 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 0.58s

[ec2-user@ip-172-31-15-148 bit-fields]$ grcov . -s . --binary-path ../../target/debug/ --excl-start "mod tests" -t html --branch --ignore-not-existing -o coverage
[ec2-user@ip-172-31-15-148 bit-fields]$ 

The coverage is reported as ~75%. Although there does appear to be an anomaly in the html coverage report, this is that index.html reports a coverage of 62.37% in bit_range.rs while bit_range.rs.html reports 97.22%.

The current grcov coverage does not cover proc-macros.

@pb8o informed me of https://crates.io/crates/cargo-llvm-cov:

[ec2-user@ip-172-31-15-148 bit-fields]$ cargo +nightly llvm-cov --html --doctests
warning: --doctests option is unstable
I will run `rustup component add llvm-tools-preview --toolchain nightly-x86_64-unknown-linux-gnu` to install the `llvm-tools-preview` component for the selected toolchain.
Proceed? [Y/n] Y
info: downloading component 'llvm-tools-preview'
info: installing component 'llvm-tools-preview'
 32.7 MiB /  32.7 MiB (100 %)  15.8 MiB/s in  2s ETA:  0s
warning: patch for `kvm-bindings` uses the features mechanism. default-features and features will not take effect because the patch dependency does not support this mechanism
   Compiling proc-macro2 v1.0.43
   Compiling unicode-ident v1.0.3
   Compiling quote v1.0.21
   Compiling syn v1.0.99
   Compiling libc v0.2.117
   Compiling serde_derive v1.0.144
   Compiling serde v1.0.144
   Compiling cfg-if v1.0.0
   Compiling serde_json v1.0.85
   Compiling ppv-lite86 v0.2.16
   Compiling ryu v1.0.9
   Compiling itoa v1.0.1
   Compiling getrandom v0.2.4
   Compiling rand_core v0.6.3
   Compiling rand_chacha v0.3.1
   Compiling rand v0.8.5
   Compiling thiserror-impl v1.0.32
   Compiling thiserror v1.0.32
   Compiling bit-fields-macros v0.1.0 (/home/ec2-user/firecracker/src/bit-fields-macros)
   Compiling bit-fields v0.1.0 (/home/ec2-user/firecracker/src/bit-fields)
    Finished test [unoptimized + debuginfo] target(s) in 9.88s
     Running unittests src/lib.rs (/home/ec2-user/firecracker/target/llvm-cov-target/debug/deps/bit_fields-a7f187ca2c22e13d)

running 15 tests
test bit_range::tests::mask ... ok
test tests::access ... ok
test tests::add_sub ... ok
test tests::checked_add_assign ... ok
test tests::checked_assign ... ok
test tests::checked_sub_assign ... ok
test tests::conversion ... ok
test tests::display ... ok
test tests::flip ... ok
test tests::on_off ... ok
test tests::serialize ... ok
test tests::set ... ok
test tests::size ... ok
test tests::type_max ... ok
test tests::value_max ... ok

test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.34s

   Doc-tests bit-fields

running 157 tests
test src/bit-fields/src/bit.rs - bit::Bit::eq (line 397) ... ok
test src/bit-fields/src/bit.rs - bit::Bit::eq (line 397) ... ok
test src/bit-fields/src/bit.rs - bit::Bit::eq (line 398) ... ok
test src/bit-fields/src/bit.rs - bit::Bit::eq (line 398) ... ok
...
test src/bit-fields/src/bit_range.rs - bit_range::BitRangeMut::write (line 368) ... ok
test src/bit-fields/src/bit_range.rs - bit_range::BitRangeMut::write (line 369) ... ok
test src/bit-fields/src/lib.rs - BitIndex (line 70) ... ignored
test src/bit-fields/src/lib.rs - BitIndexMut (line 92) ... ignored

test result: ok. 155 passed; 0 failed; 2 ignored; 0 measured; 0 filtered out; finished in 20.55s


    Finished report saved to /home/ec2-user/firecracker/target/llvm-cov/html
[ec2-user@ip-172-31-15-148 bit-fields]$ 

Produces a report which notes ~93% line coverage.

On bit-fields:

Filename                             Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
bit-fields-macros/src/builder.rs          12                 1    91.67%           8                 1    87.50%         641                 1    99.84%           0                 0         -
bit-fields-macros/src/lib.rs              53                20    62.26%           5                 3    40.00%          66                22    66.67%           0                 0         -
bit-fields-macros/src/parser.rs           83                28    66.27%           5                 1    80.00%         204                89    56.37%           0                 0         -
bit-fields-macros/src/utils.rs            51                16    68.63%          23                12    47.83%          81                21    74.07%           0                 0         -
bit-fields/src/bit.rs                    186                 4    97.85%          63                 3    95.24%         418                 4    99.04%           0                 0         -
bit-fields/src/bit_range.rs              285                17    94.04%          59                14    76.27%         491                48    90.22%           0                 0         -
bit-fields/src/errors.rs                  66                 1    98.48%          36                 1    97.22%         143                 3    97.90%           0                 0         -
bit-fields/src/lib.rs                    392                56    85.71%          48                 1    97.92%         569                 4    99.30%           0                 0         -
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                   1128               143    87.32%         247                36    85.43%        2613               192    92.65%           0                 0         -

On bit-fields-macros:

Filename                      Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
builder.rs                         39                 5    87.18%          25                 2    92.00%         711                 6    99.16%           0                 0         -
lib.rs                             54                53     1.85%           6                 5    16.67%          67                66     1.49%           0                 0         -
parser.rs                         384                24    93.75%          56                 0   100.00%         452                26    94.25%           0                 0         -
utils.rs                          139                 3    97.84%          43                 2    95.35%         210                 2    99.05%           0                 0         -
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                             616                85    86.20%         130                 9    93.08%        1440               100    93.06%           0                 0         -

This path may warrant further discussion, although may well be outside the scope of this PR.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

• All commits in this PR are signed (git commit -s).
• The issue which led to this PR has a clear conclusion.
• This PR follows the solution outlined in the related issue.
• The description of changes is clear and encompassing.
• Any required documentation changes (code and docs) are included in this PR.
• Any newly added unsafe code is properly documented.
• Any API changes follow the Runbook for Firecracker API changes.
• Any user-facing changes are mentioned in CHANGELOG.md.
• All added/changed functionality is tested.

Footnotes

1. A crates cannot export both procedural-macros and non-procedural-macros, so it requires 2 crates acting as 1, like serde and serde-derive ↩

[alsrdn]

This PR needs a better motivation on why these new crates are needed. We already use the bitflags crate. We should outline why this is different and what differences does this bring.

It also seems that the commitmsg is empty. We should add these details in the commit message as well.

Since there are 2 separate crates being added, we can split the commit in 2 and have 2 commits, each one for each crate.

[JonathanWoollett-Light]

This PR needs a better motivation on why these new crates are needed. We already use the bitflags crate. We should outline why this is different and what differences does this bring.

I've added a short justification.

It also seems that the commitmsg is empty. We should add these details in the commit message as well.

Added commit message.

Since there are 2 separate crates being added, we can split the commit in 2 and have 2 commits, each one for each crate.

Added a point about this, that these should be used as 1 crate.

[JonathanWoollett-Light]

The current failures in CI are a result of the currently not explicitly allowed license Unicode-DSF-2016 being present in a transitive dependency. I am looking into this.

[roypat]

I've gone through just bit-fields-macros and highlighted all the allow directives that are outdated/can easily have the underlying lint fixed.

I still think that #[warn(pedantic, restriction)] just generates too much noise, resulting in out of date and excessive allows, and desensitizes us to actually fixing lints.

I feel we should instead have a look at the list of clippy lints that are actually useful without high false-positives and explicitly enable that subset.

[roypat]

These also apply to the non-zero impls.

[roypat]

Alright, I think from a functionality point of view, there are no more issues. The main things I looked at during my review were:

• undefined behavior caused by unsafe code
• being able to construct Bit(Range)s using nonsense const generic parameters (e.g. a Bit<10>(u8)).

I am approving based on this PR now correctly providing all the required functionality. However, there are some design points I would like to see revisited in follow up PRs (or justified by the usage of the functionality in the final product):

• Currently, the proc-macro generates Bit/BitMut implementations on each BitField (a few hundred lines of generated code for bitfield). These can instead be provided once per type by just implementing a HasBit<const N: u8> marker-trait on u8, u16,... (since Bit/BitMut are only used as marker traits for the function that allows arbitrary bit access to a bit-fields underlying value.
• The HashSet/HashMap (de)serialization functions generated are not currently used by any implementation. If this stays this way, they should be removed (again, multiple hundred lines of code generated with each macro invocation). If they are needed, I would like them to be as specialized as possible (e.g. all the T: Display and K: From<u64> bounds should be either justified through usage, or specialized to the types for which the implementations are actually needed).
• The previous point regarding "remove that is not used" extends to a lot of other functions too (e.g. the multiple aliases for reading a bit(range), etc)
• The parser code can be cleaned up significantly. The grammar here is LL(1), and we should be using an LL(1) parser to process it. Potentially, We can use the syn crate (on which we depend already anyway) for simplifiying a lot of code, as it provides methods to, for example, parse attributes and other bits of valid rust syntax that show up in the bitfields grammar.

And lastly, I personally do not see the advantage of the Bit/BitRange structs over simply having the proc-macro generate getters and setters for each bit field bit/range. This is the approach taken by crosvm's bitfields crate. Using the complexity of that crate (which apart from not allow "holes" in defined Bitfields is at feature parity with this bit-fields crate) as a baseline for complexity, we should be able to cut down roughly 4000 lines of code by doing this.

[dianpopa]

For the time being, I think the functionality can be merged like it is in the feature branch. did not find any major functional bugs.
However, I do think some refinements need to happen for the functionality to be merged in main:

• One of my biggest concern is the size of the code that gets generated right now for each bitfield-enabled structure. The size is right now approximately 28K. This is worrying since this is only for one structure (i.e one Leaf and the CPUID ends up having a big number of those). So, what I would like to see is the generated code getting trimmed down to exactly the minimum we require for CPUID. I left some comments regarding some of the things that I saw we can get rid of (NonZero types for e.g)
• I am in line with Patrick on modifying the current approach of using non-mut/mut functionality to one that uses getters/setters cause in the end this is what we need for CPUID; This will make the code more straightforward and will again rid us of some duplication (like implementing read methods on both non-mut and mut code).
• I think we should move this functionality all in one crate because it is weird for bit-fields-macros to generate code that calls into bit-fields when the crate dependency is actually the other way around. I am talkign about this: https://github.com/firecracker-microvm/firecracker/pull/3105/files#diff-db7421964bc6c93ef265a89cff62f1c7ea9e95748ee3c90755b492ed516878bbR142.

I will not approve for the moment since I also want to wait for @bchalios's input.

[JonathanWoollett-Light]

• I think we should move this functionality all in one crate because it is weird for bit-fields-macros to generate code that calls into bit-fields when the crate dependency is actually the other way around. I am talkign about this: https://github.com/firecracker-microvm/firecracker/pull/3105/files#diff-db7421964bc6c93ef265a89cff62f1c7ea9e95748ee3c90755b492ed516878bbR142.

This functionality cannot be in 1 crate, crates which export proc-macros cannot export any other items.

[dianpopa]

• I think we should move this functionality all in one crate because it is weird for bit-fields-macros to generate code that calls into bit-fields when the crate dependency is actually the other way around. I am talkign about this: https://github.com/firecracker-microvm/firecracker/pull/3105/files#diff-db7421964bc6c93ef265a89cff62f1c7ea9e95748ee3c90755b492ed516878bbR142.

This functionality cannot be in 1 crate, crates which export proc-macros cannot export any other items.

I saw crosvm has all in 1 crate, maybe it s worth taking a look since they also used proc-macros.

[JonathanWoollett-Light]

• I think we should move this functionality all in one crate because it is weird for bit-fields-macros to generate code that calls into bit-fields when the crate dependency is actually the other way around. I am talkign about this: https://github.com/firecracker-microvm/firecracker/pull/3105/files#diff-db7421964bc6c93ef265a89cff62f1c7ea9e95748ee3c90755b492ed516878bbR142.

This functionality cannot be in 1 crate, crates which export proc-macros cannot export any other items.

I saw crosvm has all in 1 crate, maybe it s worth taking a look since they also used proc-macros.

They are using 2 nested crates, the 1st is at https://github.com/dgreid/crosvm/tree/main/bit_field and the 2nd is at https://github.com/dgreid/crosvm/tree/main/bit_field/bit_field_derive. We could also do this, it shouldn't make any difference, I think its just a question of style.

[roypat]

reapproving after force push