-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support FDT setup for CPU caches with high number of sets #4869
Support FDT setup for CPU caches with high number of sets #4869
Conversation
Change the doc comment (which also affects the implementation of `std::fmt::Display`) for `ConfigurationError::SetupFDT` to include the underlying `fdt::FtdError`. This improves both the reporting of failures during FDT setup on the logs, as well as the message in API server's corresponding HTTP response. Signed-off-by: Christos Katsakioris <[email protected]> Signed-off-by: Filippos Tofalos <[email protected]>
7b6e3c8
to
1c49679
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4869 +/- ##
=======================================
Coverage 84.07% 84.07%
=======================================
Files 251 251
Lines 28052 28052
=======================================
Hits 23586 23586
Misses 4466 4466
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hey guys, thanks a lot for your contribution. It generally LGTM. Could you fix the style issue? It's not in your changes, but your changes did make redundant a |
Change the type of `CacheEntry`'s `number_of_sets` field from `u16` to `u32`, to allow correctly parsing CPU cache information from sysfs and successfully setting up the FDT on hosts with CPU caches with a number of sets that is higher than `u16::MAX`. This also makes a couple of `u16`->`u32` conversions redundant, which we therefore remove. Signed-off-by: Christos Katsakioris <[email protected]> Signed-off-by: Filippos Tofalos <[email protected]>
1c49679
to
9b48dee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the contribution
firecracker-microvm#4869 changed the type that holds the number of sets of a CPU cache from `u16` to `u32`. This allows us to support ARM systems with CPU caches with sets more than `u16::MAX`. Add a CHANGELOG entry for this change Signed-off-by: Babis Chalios <[email protected]>
#4869 changed the type that holds the number of sets of a CPU cache from `u16` to `u32`. This allows us to support ARM systems with CPU caches with sets more than `u16::MAX`. Add a CHANGELOG entry for this change Signed-off-by: Babis Chalios <[email protected]>
firecracker-microvm#4869 changed the type that holds the number of sets of a CPU cache from `u16` to `u32`. This allows us to support ARM systems with CPU caches with sets more than `u16::MAX`. Add a CHANGELOG entry for this change Signed-off-by: Babis Chalios <[email protected]>
firecracker-microvm#4869 changed the type that holds the number of sets of a CPU cache from `u16` to `u32`. This allows us to support ARM systems with CPU caches with sets more than `u16::MAX`. Add a CHANGELOG entry for this change Signed-off-by: Babis Chalios <[email protected]>
firecracker-microvm#4869 changed the type that holds the number of sets of a CPU cache from `u16` to `u32`. This allows us to support ARM systems with CPU caches with sets more than `u16::MAX`. Add a CHANGELOG entry for this change Signed-off-by: Babis Chalios <[email protected]>
firecracker-microvm#4869 changed the type that holds the number of sets of a CPU cache from `u16` to `u32`. This allows us to support ARM systems with CPU caches with sets more than `u16::MAX`. Add a CHANGELOG entry for this change Signed-off-by: Babis Chalios <[email protected]>
Changes
The first commit improves the logging/reporting of errors during FDT creation (e.g., failures while parsing CPU cache information from
sysfs
).The second commit changes the type of field
number_of_sets
in structvmm::arch::aarch64::cache_info::CacheEntry
fromu16
tou32
, thus allowing support for hosts with CPU caches with a number of sets higher thanu16::MAX
(e.g., NVIDIA GH200 Grace Hopper).Reason
When we attempted to boot a microVM on a GH200 host following the Getting Started guide (with a minor modification, to download the
aarch64
kernel binary), building the microVM failed, and the following was logged:while this was the response of the API server:
The first commit improves the reporting of failures while setting up FDT, by expanding the doc comments of the
vmm::arch::aarch64::ConfigurationError::SetupFDT
variant (thus also its implementation ofstd::fmt::Display
, thanks to cratesthiserror
anddisplaydoc
) to include the underlyingvmm::arch::aarch64::fdt::FdtError
.As a result, the improved logs end up being as follows:
API server's HTTP response becomes more descriptive as well:
The second commit fixes the actual bug for the GH200 host (unveiled by and shown in the improved logs above). In this case, host CPU's number of sets for the unified L3 cache is higher than
u16::MAX
:Changing the type of the
number_of_sets
field in structvmm::arch::aarch64::cache_info::CacheEntry
fromu16
tou32
suffices to fit a high number of CPU cache sets, thus allowing the microVM to be built and booted without any problem.License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md
.PR Checklist
PR.
CHANGELOG.md
.TODO
s link to an issue.contribution quality standards.
rust-vmm
.