feat: reset KVM_REG_ARM_PTIMER_CNT on VM boot #4987

ShadowCurse · 2025-01-10T13:00:37Z

Reset KVM_REG_ARM_PTIMER_CNT physical counter register on VM boot to avoid passing through host physical counter. Note that resetting the register on VM boot does not guarantee that VM will see the counter value 0 at startup because there is a delta in time between register reset and VM boot during which counter continues to advance.

Reason

Prevent guest from reading host performance counter.

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.

codecov · 2025-01-10T13:15:37Z

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 83.95%. Comparing base (a5ffb7a) to head (b5be991).

Files with missing lines	Patch %	Lines
src/vmm/src/arch/aarch64/vcpu.rs	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4987      +/-   ##
==========================================
- Coverage   83.95%   83.95%   -0.01%     
==========================================
  Files         248      248              
  Lines       27839    27845       +6     
==========================================
+ Hits        23371    23376       +5     
- Misses       4468     4469       +1

Flag	Coverage Δ
5.10-c5n.metal	`84.53% <ø> (+<0.01%)`	⬆️
5.10-m5n.metal	`84.51% <ø> (ø)`
5.10-m6a.metal	`83.79% <ø> (+<0.01%)`	⬆️
5.10-m6g.metal	`80.63% <83.33%> (+<0.01%)`	⬆️
5.10-m6i.metal	`84.50% <ø> (+<0.01%)`	⬆️
5.10-m7g.metal	`80.63% <83.33%> (+<0.01%)`	⬆️
6.1-c5n.metal	`84.53% <ø> (+<0.01%)`	⬆️
6.1-m5n.metal	`84.51% <ø> (ø)`
6.1-m6a.metal	`83.79% <ø> (ø)`
6.1-m6g.metal	`80.63% <83.33%> (+<0.01%)`	⬆️
6.1-m6i.metal	`84.50% <ø> (ø)`
6.1-m7g.metal	`80.63% <83.33%> (+<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.

roypat · 2025-01-10T15:23:37Z

src/vmm/src/arch/aarch64/vcpu.rs

+            .set_one_reg(KVM_REG_ARM_PTIMER_CNT, &zero.to_le_bytes())
+            .is_err()
+        {
+            warn!("Unable to reset performance counter. VM will use host value instead.");


I really don't think we should have warnings that the user cannot silence. Maybe we can only print this if running on 6.4 or newer? Rest lgtm

I don't think this is needed to be silenced. 6.4 is not super new and as time goes on, the less users will use kernel which will print the warning. If we add option to silence this, we will have to have it until we deprecate it and this is more work to do.

I'm not inclined to print a warning either. It is not quite expected that users running Firecracker on a supported kernel get a warning they cannot fix.

Well, this is a warning and not an error. They are not meant to be "fixed" I think.
I can add a link to the patch series that added the needed fixes to the changelog and tell users to use kernel with this patches if they want to remove the warning.

feels like we could update https://github.com/firecracker-microvm/firecracker/blob/main/docs/prod-host-setup.md#arm-only-physical-counter-directly-passed-through-to-the-guest in order to explain that

I am ok with warning, but if that's too unpalatable, can we keep it as info rather than dropping it?

I was also thinking about making it info, but then only people who enable info log level will see it.

how about we only warn if the KVM_CAP_COUNTER_OFFSET KVM capability is supported (meaning we are on a kernel where setting the registers should be possible)?

src/vmm/src/arch/aarch64/vcpu.rs

kalyazin · 2025-01-10T16:01:34Z

src/vmm/src/arch/aarch64/vcpu.rs

+            .set_one_reg(KVM_REG_ARM_PTIMER_CNT, &zero.to_le_bytes())
+            .is_err()
+        {
+            warn!("Unable to reset performance counter. VM will use host value instead.");


I'm not inclined to print a warning either. It is not quite expected that users running Firecracker on a supported kernel get a warning they cannot fix.

src/vmm/src/arch/aarch64/regs.rs

CHANGELOG.md

Reset KVM_REG_ARM_PTIMER_CNT physical counter register on VM boot to avoid passing through host physical counter. Note that resetting the register on VM boot does not guarantee that VM will see the counter value 0 at startup because there is a delta in time between register reset and VM boot during which counter continues to advance. Signed-off-by: Egor Lazarchuk <[email protected]>

Add an entry about physical counter reset to the CHANGELOG. Signed-off-by: Egor Lazarchuk <[email protected]>

pb8o · 2025-01-10T16:47:20Z

src/vmm/src/arch/aarch64/vcpu.rs

@@ -238,6 +257,15 @@ mod tests {
        vcpu.vcpu_init(&kvi).unwrap();

        setup_boot_regs(&vcpu, 0, 0x0, &mem).unwrap();
+
+        // Check that the register is reset on compatible kernels.


Can we also add an integration test in tests/integration_tests/security?

Integration test will be quite difficult as we will need kernel access to read the physical counter.

Update a note about physical counter on ARM being reset instead of directly passed through. Signed-off-by: Egor Lazarchuk <[email protected]>

ShadowCurse force-pushed the aarch64_counter_reset branch from b34d97d to b459ae3 Compare January 10, 2025 13:11

ShadowCurse force-pushed the aarch64_counter_reset branch 2 times, most recently from 1bd3f94 to 5ea2f4a Compare January 10, 2025 14:24

ShadowCurse changed the title ~~feat: reset SYS_CNTPCT_EL0 on VM boot~~ feat: reset KVM_REG_ARM_PTIMER_CNT on VM boot Jan 10, 2025

ShadowCurse force-pushed the aarch64_counter_reset branch 4 times, most recently from 75a7ec1 to 053f265 Compare January 10, 2025 15:19

ShadowCurse marked this pull request as ready for review January 10, 2025 15:20

ShadowCurse requested review from xmarcalx, kalyazin, pb8o and Manciukic as code owners January 10, 2025 15:20

ShadowCurse self-assigned this Jan 10, 2025

ShadowCurse added Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Fix Indicates a fix to existing code labels Jan 10, 2025

roypat reviewed Jan 10, 2025

View reviewed changes

kalyazin reviewed Jan 10, 2025

View reviewed changes

ShadowCurse force-pushed the aarch64_counter_reset branch from 053f265 to d3abf67 Compare January 10, 2025 16:26

chore: add an entry to the CHANGELOG

57b458b

Add an entry about physical counter reset to the CHANGELOG. Signed-off-by: Egor Lazarchuk <[email protected]>

ShadowCurse force-pushed the aarch64_counter_reset branch from d3abf67 to 57b458b Compare January 10, 2025 16:31

pb8o reviewed Jan 10, 2025

View reviewed changes

chore: update prod-host-setup.md with arm physical counter info

b5be991

Update a note about physical counter on ARM being reset instead of directly passed through. Signed-off-by: Egor Lazarchuk <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: reset KVM_REG_ARM_PTIMER_CNT on VM boot #4987

feat: reset KVM_REG_ARM_PTIMER_CNT on VM boot #4987

ShadowCurse commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading

roypat Jan 10, 2025

ShadowCurse Jan 10, 2025

kalyazin Jan 10, 2025

ShadowCurse Jan 10, 2025

kalyazin Jan 10, 2025

pb8o Jan 10, 2025

ShadowCurse Jan 10, 2025

roypat Jan 10, 2025

kalyazin Jan 10, 2025

pb8o Jan 10, 2025

ShadowCurse Jan 10, 2025

feat: reset KVM_REG_ARM_PTIMER_CNT on VM boot #4987

Are you sure you want to change the base?

feat: reset KVM_REG_ARM_PTIMER_CNT on VM boot #4987

Conversation

ShadowCurse commented Jan 10, 2025 • edited Loading

Reason

License Acceptance

PR Checklist

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShadowCurse commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 10, 2025 •

edited

Loading