Skip to content

Conversation

@ShadowCurse
Copy link
Contributor

Changes

The IA32_MTRRdefType MSR describes the default memory type
used for physical addresses which are outside of any configured
memory ranges, as well as whether MTRRs and fixed ranges
are enabled.

Without enabling this MSR, guest kernel is not able to change
memory types for memory ranges outside main memory. This was
causing pmem region to be marked as uncached-minus which
instructed the cpu to skip all caches (L1, L2, L3) when
reading/writing to that memory region.

To fix this set IA32_MTRRdefType set to 0x806:

  • bit 11 enables MTRR
  • bits 0..7 specify the memory type used by default
    • value 6 represents write-back type

Now guest can correctly set pmem memory region to
write-back memory type.

Reason

Fixes incorrect memory type assigment for pmem memory on x86_64.

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 checkbuild --all to verify that the PR passes
    build checks on all supported architectures.
  • 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.

@ShadowCurse ShadowCurse self-assigned this Nov 21, 2025
@ShadowCurse ShadowCurse added Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Fix Indicates a fix to existing code labels Nov 21, 2025
@codecov
Copy link

codecov bot commented Nov 21, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.88%. Comparing base (fa2627a) to head (f3f65c8).
⚠️ Report is 4 commits behind head on main.

⚠️ Current head f3f65c8 differs from pull request most recent head dd12568

Please upload reports for the commit dd12568 to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5526   +/-   ##
=======================================
  Coverage   82.87%   82.88%           
=======================================
  Files         270      270           
  Lines       27780    27784    +4     
=======================================
+ Hits        23024    23028    +4     
  Misses       4756     4756           
Flag Coverage Δ
5.10-m5n.metal 83.06% <100.00%> (+<0.01%) ⬆️
5.10-m6a.metal 82.34% <100.00%> (+0.01%) ⬆️
5.10-m6g.metal 79.64% <ø> (ø)
5.10-m6i.metal 83.05% <100.00%> (-0.01%) ⬇️
5.10-m7a.metal-48xl 82.32% <100.00%> (+<0.01%) ⬆️
5.10-m7g.metal 79.64% <ø> (ø)
5.10-m7i.metal-24xl 83.03% <100.00%> (+<0.01%) ⬆️
5.10-m7i.metal-48xl 83.03% <100.00%> (+<0.01%) ⬆️
5.10-m8g.metal-24xl 79.63% <ø> (ø)
5.10-m8g.metal-48xl 79.63% <ø> (ø)
6.1-m5n.metal 83.09% <100.00%> (+<0.01%) ⬆️
6.1-m6a.metal 82.37% <100.00%> (+<0.01%) ⬆️
6.1-m6g.metal 79.63% <ø> (ø)
6.1-m6i.metal 83.08% <100.00%> (+<0.01%) ⬆️
6.1-m7a.metal-48xl 82.35% <100.00%> (+<0.01%) ⬆️
6.1-m7g.metal 79.63% <ø> (-0.01%) ⬇️
6.1-m7i.metal-24xl 83.10% <100.00%> (+0.01%) ⬆️
6.1-m7i.metal-48xl 83.11% <100.00%> (+0.01%) ⬆️
6.1-m8g.metal-24xl 79.63% <ø> (-0.01%) ⬇️
6.1-m8g.metal-48xl 79.63% <ø> (+<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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zulinx86
zulinx86 previously approved these changes Nov 21, 2025
The IA32_MTRRdefType MSR describes the default memory type
used for physical addresses which are outside of any configured
memory ranges, as well as whether MTRRs and fixed ranges
are enabled.

Without enabling this MSR, guest kernel is not able to change
memory types for memory ranges outside main memory. This was
causing pmem region to be marked as `uncached-minus` which
instructed the cpu to skip all caches (L1, L2, L3) when
reading/writing to that memory region.

To fix this set IA32_MTRRdefType set to 0x806:
- bit 11 enables MTRR
- bits 0..7 specify the memory type used by default
  - value 6 represents `write-back` type

Now guest can correctly set pmem memory region to
`write-back` memory type.

Signed-off-by: Egor Lazarchuk <[email protected]>
Replace hand coded MTRR MSRs with auto generated ones.

Signed-off-by: Egor Lazarchuk <[email protected]>
IA32_PAT_MSR can be modified by the guest to change
page attributes for memory regions.

Signed-off-by: Egor Lazarchuk <[email protected]>
Copy link
Contributor

@Manciukic Manciukic left a comment

Choose a reason for hiding this comment

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

LGTM, but I think we should have a changelog item as "Changed", just in case.

Do we need to change anything in the CPU templates or is this applied after, so there is no issue?

@ShadowCurse
Copy link
Contributor Author

Added a note to the CHANGELOG.
This is set after normalization is happening in the configure function.

Add note about IA32_MTRRdefType MSR to CHANGELOG.

Signed-off-by: Egor Lazarchuk <[email protected]>
Manciukic
Manciukic previously approved these changes Nov 21, 2025
@zulinx86
Copy link
Contributor

Added a note to the CHANGELOG. This is set after normalization is happening in the configure function.

@ShadowCurse In that case, we should update the doc for boot protocol, I think: https://github.com/firecracker-microvm/firecracker/blob/main/docs/cpu_templates/boot-protocol.md

@zulinx86
Copy link
Contributor

Added a note to the CHANGELOG. This is set after normalization is happening in the configure function.

@ShadowCurse In that case, we should update the doc for boot protocol, I think: https://github.com/firecracker-microvm/firecracker/blob/main/docs/cpu_templates/boot-protocol.md

Since MTTR-related MSRs are not necessary to boot, having them in the boot protocol doc looks a bit weird. We do it exactly where we apply modification for the boot protocol. It would be better to do it in the CPUID normalization process, but we need to do some refactoring for it and we should rename it since it would modify not only CPUID but also MSRs. But we should have the description somewhere in our doc any way.

zulinx86
zulinx86 previously approved these changes Nov 21, 2025
@ShadowCurse ShadowCurse enabled auto-merge (rebase) November 21, 2025 17:01
@ShadowCurse ShadowCurse merged commit 2b05435 into firecracker-microvm:main Nov 21, 2025
9 checks passed
@ShadowCurse ShadowCurse deleted the mtrr branch November 21, 2025 17:16
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 Type: Fix Indicates a fix to existing code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants