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.

Sign up for GitHub

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

Jump to bottom

fix: make aarch64_with_sve_and_pac template example work with >=2 vcpus #4925

Merged
merged 3 commits into from
Nov 25, 2024
Merged

fix: make aarch64_with_sve_and_pac template example work with >=2 vcpus #4925

merged 3 commits into from
Nov 25, 2024

Conversation

roypat
Copy link
Contributor

@roypat roypat commented Nov 25, 2024

Setting the least significant 4 bits to zero overwrites some
configuration that Firecracker sets on secondary CPUs that is needed for
them to be able to be booted. With these 4 bits set to 0, the CPUs will
never show up as online inside the guest, and KVM will just infinitely
spin inside KVM_RUN for them. Fix by using 'x' in the template, which
preserves the defaults set by Firecracker.

Signed-off-by: Patrick Roy [email protected]

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

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • If a specific issue led to this PR, this PR closes the issue.
  • When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • I have linked an issue to every new TODO.
  • This functionality cannot be added in rust-vmm.

@roypat
replace set([...]) with set literal
14c18ed 
My IDE was complaining about this

Signed-off-by: Patrick Roy <[email protected]>
@roypat
chore: make sure hexadecimal numbers are prefixed by '0x'
265729e 
I got an error message saying 'Start microvm error: Internal error while
starting microVM: Vm error: Missing KVM capabilities: aa', which just
looked like KVM was screaming.

Thus, look through where we print hex numbers, and make sure they are
prefixed using '0x', and do so uniformly by using Rust's `:#x` format
modifier [1]

[1]: https://doc.rust-lang.org/std/fmt/#sign0

Signed-off-by: Patrick Roy <[email protected]>
@roypat
fix: make aarch64_with_sve_and_pac template work with >=2 vcpus
22e1870 
Setting the least significant 4 bits to zero overwrites some
configuration that Firecracker sets on secondary CPUs that is needed for
them to be able to be booted. With these 4 bits set to 0, the CPUs will
never show up as online inside the guest, and KVM will just infinitely
spin inside KVM_RUN for them. Fix by using 'x' in the template, which
preserves the defaults set by Firecracker.

Signed-off-by: Patrick Roy <[email protected]>
@roypat roypat added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Nov 25, 2024
@pb8o pb8o added Type: Bug Indicates an unexpected problem or unintended behavior Type: Fix Indicates a fix to existing code labels Nov 25, 2024
@codecov Codecov
Copy link

codecov bot commented Nov 25, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 60.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.09%. Comparing base (4352366) to head (22e1870).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/vmm/src/devices/virtio/mmio.rs 57.14% 3 Missing ⚠️
src/vmm/src/devices/virtio/vsock/device.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4925      +/-   ##
==========================================
+ Coverage   84.07%   84.09%   +0.01%     
==========================================
  Files         251      251              
  Lines       28067    28061       -6     
==========================================
  Hits        23597    23597              
+ Misses       4470     4464       -6
Flag Coverage Δ
5.10-c5n.metal 84.66% <60.00%> (+0.02%) ⬆️
5.10-m5n.metal 84.64% <60.00%> (+0.02%) ⬆️
5.10-m6a.metal 83.95% <60.00%> (+0.02%) ⬆️
5.10-m6g.metal 80.77% <55.55%> (+0.01%) ⬆️
5.10-m6i.metal 84.64% <60.00%> (+0.02%) ⬆️
5.10-m7g.metal 80.77% <55.55%> (+0.01%) ⬆️
6.1-c5n.metal 84.66% <60.00%> (+0.02%) ⬆️
6.1-m5n.metal 84.64% <60.00%> (+0.02%) ⬆️
6.1-m6a.metal 83.95% <60.00%> (+0.02%) ⬆️
6.1-m6g.metal 80.76% <55.55%> (+0.01%) ⬆️
6.1-m6i.metal 84.64% <60.00%> (+0.02%) ⬆️
6.1-m7g.metal 80.77% <55.55%> (+0.01%) ⬆️

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.

🚨 Try these New Features:

@roypat roypat merged commit 1320786 into firecracker-microvm:main Nov 25, 2024
9 of 10 checks passed
@roypat roypat deleted the cpu-template-fix branch November 25, 2024 15:07
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 4, 2024
@zulinx86
doc: Use a valid bitmap for vcpu_features
c181b69 
As seen in firecracker-microvm#4925, setting the least significant bits of vcpu_features
field to 0 make secondary CPUs not work. There is no reason to use such
an invalid example in doc. The sample in doc intended to enable PAC
(Pointer Authentication) since it checks capabilities 171 and 172 [2].
Thus, not set the other bits than bits 5 and 6.

[1]: https://elixir.bootlin.com/linux/v6.12.1/source/include/uapi/linux/kvm.h#L868-L869
[2]: https://elixir.bootlin.com/linux/v6.12.1/source/arch/arm64/include/uapi/asm/kvm.h#L108-L109

Signed-off-by: Takahiro Itazuri <[email protected]>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 4, 2024
@zulinx86
test: Check all vCPUs are oneline
57a163e 
As seen in firecracker-microvm#4925, a bad CPU template made secondary CPUs not come up. To
catch similar bugs of CPU templates, check all the vCPUs are online from
guests' perspective.

Signed-off-by: Takahiro Itazuri <[email protected]>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 4, 2024
@zulinx86
test: Check all vCPUs are oneline
fa07e9f 
As seen in firecracker-microvm#4925, a bad CPU template made secondary CPUs not come up. To
catch similar bugs of CPU templates, check all the vCPUs are online from
guests' perspective.

Signed-off-by: Takahiro Itazuri <[email protected]>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 4, 2024
@zulinx86
test: Check all vCPUs are oneline
76d1db1 
As seen in firecracker-microvm#4925, a bad CPU template made secondary CPUs not come up. To
catch similar bugs of CPU templates, check all the vCPUs are online from
guests' perspective.

Signed-off-by: Takahiro Itazuri <[email protected]>
@zulinx86 zulinx86 mentioned this pull request Dec 4, 2024
Merged
5 tasks
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 5, 2024
@zulinx86
doc: Use a valid bitmap for vcpu_features
4d2467c 
As seen in firecracker-microvm#4925, setting the least significant bits of vcpu_features
field to 0 make secondary CPUs not work. There is no reason to use such
an invalid example in doc. The sample in doc intended to enable PAC
(Pointer Authentication) since it checks capabilities 171 and 172 [2].
Thus, not set the other bits than bits 5 and 6.

[1]: https://elixir.bootlin.com/linux/v6.12.1/source/include/uapi/linux/kvm.h#L868-L869
[2]: https://elixir.bootlin.com/linux/v6.12.1/source/arch/arm64/include/uapi/asm/kvm.h#L108-L109

Signed-off-by: Takahiro Itazuri <[email protected]>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 5, 2024
@zulinx86
test: Check all vCPUs are oneline
1348fe9 
As seen in firecracker-microvm#4925, a bad CPU template made secondary CPUs not come up. To
catch similar bugs of CPU templates, check all the vCPUs are online from
guests' perspective.

Signed-off-by: Takahiro Itazuri <[email protected]>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 6, 2024
@zulinx86
doc: Use a valid bitmap for vcpu_features
3f32e6b 
As seen in firecracker-microvm#4925, setting the least significant bits of vcpu_features
field to 0 make secondary CPUs not work. There is no reason to use such
an invalid example in doc. The sample in doc intended to enable PAC
(Pointer Authentication) since it checks capabilities 171 and 172 [2].
Thus, not set the other bits than bits 5 and 6.

[1]: https://elixir.bootlin.com/linux/v6.12.1/source/include/uapi/linux/kvm.h#L868-L869
[2]: https://elixir.bootlin.com/linux/v6.12.1/source/arch/arm64/include/uapi/asm/kvm.h#L108-L109

Signed-off-by: Takahiro Itazuri <[email protected]>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 6, 2024
@zulinx86
test: Check all vCPUs are oneline
cb831ec 
As seen in firecracker-microvm#4925, a bad CPU template made secondary CPUs not come up. To
catch similar bugs of CPU templates, check all the vCPUs are online from
guests' perspective.

Signed-off-by: Takahiro Itazuri <[email protected]>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 6, 2024
@zulinx86
test: Check all vCPUs are oneline
3f70728 
As seen in firecracker-microvm#4925, a bad CPU template made secondary CPUs not come up. To
catch similar bugs of CPU templates, check all the vCPUs are online from
guests' perspective.

Signed-off-by: Takahiro Itazuri <[email protected]>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 9, 2024
@zulinx86
doc: Use a valid bitmap for vcpu_features
fb62df3 
As seen in firecracker-microvm#4925, setting the least significant bits of vcpu_features
field to 0 make secondary CPUs not work. There is no reason to use such
an invalid example in doc. The sample in doc intended to enable PAC
(Pointer Authentication) since it checks capabilities 171 and 172 [2].
Thus, not set the other bits than bits 5 and 6.

[1]: https://elixir.bootlin.com/linux/v6.12.1/source/include/uapi/linux/kvm.h#L868-L869
[2]: https://elixir.bootlin.com/linux/v6.12.1/source/arch/arm64/include/uapi/asm/kvm.h#L108-L109

Signed-off-by: Takahiro Itazuri <[email protected]>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 9, 2024
@zulinx86
test: Check all vCPUs are oneline
a484c1b 
As seen in firecracker-microvm#4925, a bad CPU template made secondary CPUs not come up. To
catch similar bugs of CPU templates, check all the vCPUs are online from
guests' perspective.

Signed-off-by: Takahiro Itazuri <[email protected]>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 9, 2024
@zulinx86
test: Check all vCPUs are oneline
db7720b 
As seen in firecracker-microvm#4925, a bad CPU template made secondary CPUs not come up. To
catch similar bugs of CPU templates, check all the vCPUs are online from
guests' perspective.

Signed-off-by: Takahiro Itazuri <[email protected]>
zulinx86 added a commit to zulinx86/firecracker that referenced this pull request Dec 10, 2024
@zulinx86
test: Check all vCPUs are online
a7c78bf 
As seen in firecracker-microvm#4925, a bad CPU template made secondary CPUs not come up. To
catch similar bugs of CPU templates, check all the vCPUs are online from
guests' perspective.

Signed-off-by: Takahiro Itazuri <[email protected]>
roypat pushed a commit that referenced this pull request Dec 10, 2024
@zulinx86 @roypat
doc: Use a valid bitmap for vcpu_features
f59717c 
As seen in #4925, setting the least significant bits of vcpu_features
field to 0 make secondary CPUs not work. There is no reason to use such
an invalid example in doc. The sample in doc intended to enable PAC
(Pointer Authentication) since it checks capabilities 171 and 172 [2].
Thus, not set the other bits than bits 5 and 6.

[1]: https://elixir.bootlin.com/linux/v6.12.1/source/include/uapi/linux/kvm.h#L868-L869
[2]: https://elixir.bootlin.com/linux/v6.12.1/source/arch/arm64/include/uapi/asm/kvm.h#L108-L109

Signed-off-by: Takahiro Itazuri <[email protected]>
roypat pushed a commit that referenced this pull request Dec 10, 2024
@zulinx86 @roypat
test: Check all vCPUs are online
da1c3b3 
As seen in #4925, a bad CPU template made secondary CPUs not come up. To
catch similar bugs of CPU templates, check all the vCPUs are online from
guests' perspective.

Signed-off-by: Takahiro Itazuri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pb8o pb8o pb8o approved these changes

@Manciukic Manciukic Manciukic approved these changes

Assignees
No one assigned
Labels
Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Bug Indicates an unexpected problem or unintended behavior Type: Fix Indicates a fix to existing code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@roypat @Manciukic @pb8o