Skip to content
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

Vmgenid on aarch64 #4687

Merged
merged 9 commits into from
Jul 22, 2024
Merged

Conversation

bchalios
Copy link
Contributor

@bchalios bchalios commented Jul 17, 2024

Changes

Add support for VMGenID device on ARM systems. We make the changes required to expose VMGenID to the guest via DeviceTree.

Upstream Linux adds DeviceTree support for VMGenID with Linux 6.10. We do not support Linux 6.10, so in order to test VMGenID on ARM we backport the relevant commits from 6.10 to 6.1. We add the patches in the repo, so that when we recreate CI artifacts, the script will automatically patch 6.1 guest kernel for ARM hosts with them.

Reason

Support VMGenID on ARM hosts.

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

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this
    PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet
    contribution quality standards.

  • This functionality cannot be added in rust-vmm.

Copy link

codecov bot commented Jul 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.81%. Comparing base (9b6f067) to head (e04b7ad).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4687      +/-   ##
==========================================
+ Coverage   82.80%   82.81%   +0.01%     
==========================================
  Files         255      255              
  Lines       30838    30867      +29     
==========================================
+ Hits        25534    25563      +29     
  Misses       5304     5304              
Flag Coverage Δ
5.10-c5n.metal 82.90% <100.00%> (-0.01%) ⬇️
5.10-m5n.metal 82.89% <100.00%> (-0.01%) ⬇️
5.10-m6a.metal 82.21% <100.00%> (-0.01%) ⬇️
5.10-m6g.metal 80.02% <100.00%> (-0.06%) ⬇️
5.10-m6i.metal 82.89% <100.00%> (-0.01%) ⬇️
5.10-m7g.metal 80.02% <100.00%> (-0.06%) ⬇️
6.1-c5n.metal 82.90% <100.00%> (-0.01%) ⬇️
6.1-m5n.metal 82.89% <100.00%> (-0.01%) ⬇️
6.1-m6a.metal 82.22% <100.00%> (-0.01%) ⬇️
6.1-m6g.metal 80.01% <100.00%> (-0.06%) ⬇️
6.1-m6i.metal 82.89% <100.00%> (-0.01%) ⬇️
6.1-m7g.metal 80.01% <100.00%> (-0.06%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bchalios bchalios force-pushed the vmgenid_on_aarch64 branch 9 times, most recently from 8539059 to 9a6dd56 Compare July 18, 2024 15:30
@bchalios bchalios marked this pull request as ready for review July 18, 2024 16:21
@bchalios bchalios added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Jul 18, 2024
src/vmm/src/arch/aarch64/fdt.rs Outdated Show resolved Hide resolved
src/vmm/src/arch/aarch64/fdt.rs Show resolved Hide resolved
docs/snapshotting/random-for-clones.md Outdated Show resolved Hide resolved
kalyazin
kalyazin previously approved these changes Jul 19, 2024
ShadowCurse
ShadowCurse previously approved these changes Jul 19, 2024
Copy link
Contributor

@ShadowCurse ShadowCurse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM.

src/vmm/src/arch/aarch64/fdt.rs Outdated Show resolved Hide resolved
src/vmm/src/arch/aarch64/fdt.rs Outdated Show resolved Hide resolved
Following the QEMU implementation, we were allocating a full page for
holding the memory of the VMGenID device, even though the device itself
is only using 16 bytes for the generation ID.

We actually don't have any reason to do that. The Linux kernel itself is
just remapping 16 bytes from the address we pass over via
ACPI/DeviceTree.

Revert to allocating 16 bytes for the VMGenID device and expose the
value via a public constant for other systems to consume.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Sudan Landge and others added 6 commits July 22, 2024 12:54
Expose the VMGenID device via Device Tree bindings. We add a node in the
Device Tree that passes to the guest the address of the generation ID
and the interrupt we are using to notify it about generation changes,
aka snapshot events.

Signed-off-by: Sudan Landge <sudanl@amazon.com>
Signed-off-by: Babis Chalios <bchalios@amazon.es>
Our unit tests for creating FDT depends on DTB blobs that we create once
and then we use as a reference to ensure that we don't make a change
that breaks them.

Now that we pass VMGenID via DeviceTree we need to recreate these blobs,
because we modify the layout of the guest memory, so the memory node is
suddenly different.

Also, fix the code that we use to create these blobs. Since, we
re-organized crates in the repo, it was trying to write to non-existent
paths.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Linux support for VMGenID on ARM systems landed in kernel 6.10. We only
support up to kernel 6.1. So in order to be able to test VMGenID on ARM
systems we need to patch our 6.1 kernel to backport the 6.10 commits.

This commit adds the patches in our repo under
`resources/patches/vmgenid_dt` and changes rebuild.sh to patch 6.1
kernel on ARM before building it. Also, it enables `CONFIG_VMGENID` for
ARM 6.1 guest kernel.

Eventually we will switch to using guest kernels from Amazon Linux trees
hosted on Github, which they will eventually backport the relevant
patches. Once this is done we can drop the patches from our tree and we
won't need to manually patch them.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Now we have support for the device on ARM hosts as well.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Mention that Linux support VMGenID for systems with DeviceTree support
since version 6.10. Also, mention that we only support up to Linux 6.1,
so users that want to make use of VMGenID on Linux 6.1 they need to
backport DeviceTree support from 6.10.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
Also, mention that users need to backport DeviceTree support for VMGenID
from mainline 6.10 down to 6.1 guest kernels.

Signed-off-by: Babis Chalios <bchalios@amazon.es>
src/vmm/src/arch/aarch64/fdt.rs Show resolved Hide resolved
@bchalios bchalios merged commit 26b9326 into firecracker-microvm:main Jul 22, 2024
6 of 7 checks passed
@bchalios bchalios deleted the vmgenid_on_aarch64 branch July 24, 2024 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting review Indicates that a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants