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

OvmfPkg/Sec: Skip setup MTRR early in TD-Guest #5874

Merged
merged 2 commits into from
Jul 11, 2024

Conversation

sunceping
Copy link
Contributor

@sunceping sunceping commented Jul 5, 2024

With the commit ce4c76e (“OvmfPkg/Sec: Setup MTRR early in the boot process.”), we find an unexpected #VE is triggered in TD-Guest.

The background of importing the above commit is that: Before running lzma uncompress of the main firmware volume, if not correctly set MTRR, that would make the uncompress be extremely slow.
Detailed discussion info can refer to below links:
https://edk2.groups.io/g/devel/message/114202
https://edk2.groups.io/g/devel/message/114977

Refer to [intel-tdx-module-1.5-base-spec] Section 11.3 and section11.6.1, CR0.CD is enforced to 0 in TD-Guest. And refer to section 18.2.1.4, TDX module MTRR emulation enforces WB in VMM.

Currently the initial MTRR are:

  • Td-Guest : MTRR disabled, Type is WB.
  • Non-Td-Guest : MTRR disabled, Type is UC.

In DXE phase, OVMF/TDVF would check the MTRR Type for MMIO (in CpuSetMemoryAttributes -> MtrrGetMemoryAttribute -> MtrrGetMemoryAttributeworker:
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MtrrLib/MtrrLib.c#L929 ).
If MTRR is disabled, it always returns UC. Otherwise, it returns the actual value.

If it checks that the type is not UC then the MTRR is programmed. It is required to disable cache by setting CR0.CD to 1. That will trigger an unexpected #VE in TD-Guest.

Based on above analysis we propose to skip "Setup MTRR early" in TD-Guest because of:

  • TD-Guest doesn’t have the issue that lzma uncompress extremely slow.
  • This patch will trigger an unexpected #VE in TD-Guest.

intel-tdx-module-1.5-base-spec:
https://cdrdv2.intel.com/v1/dl/getContent/733575

Cc: Erdem Aktas [email protected]
Cc: Jiewen Yao [email protected]
Cc: Min Xu [email protected]
Cc: Gerd Hoffmann [email protected]
Cc: Elena Reshetova [email protected]

Description

<Include a description of the change and why this change was made.>

<For each item, place an "x" in between [ and ] if true. Example: [x] (you can also check items in GitHub UI)>

<Create the PR as a Draft PR if it is only created to run CI checks.>

<Delete lines in <> tags before creating the PR.>

  • Breaking change?
    • Breaking change - Will this cause a break in build or boot behavior?
    • Examples: Add a new library class or move a module to a different repo.
  • Impacts security?
    • Security - Does the change have a direct security impact?
    • Examples: Crypto algorithm change or buffer overflow fix.
  • Includes tests?
    • Tests - Does the change include any explicit test code?
    • Examples: Unit tests or integration tests.

How This Was Tested

<Describe the test(s) that were run to verify the changes.>

Integration Instructions

<Describe how these changes should be integrated. Use N/A if nothing is required.>

@sunceping sunceping marked this pull request as ready for review July 5, 2024 10:04
@sunceping sunceping force-pushed the disablemtrr.v1 branch 3 times, most recently from 325196b to 5ebec60 Compare July 8, 2024 00:33
@sunceping sunceping requested a review from jyao1 July 8, 2024 01:03
With the commit ce4c76e (“OvmfPkg/Sec: Setup MTRR early
in the boot process.”), we find an unexpected #VE is triggered
in TD-Guest.

The background of importing the above commit is that:
Before running lzma uncompress of the main firmware volume,
if not correctly set MTRR, that would make the uncompress be
extremely slow.
Detailed discussion info can refer to below links:
https://edk2.groups.io/g/devel/message/114202
https://edk2.groups.io/g/devel/message/114977

Refer to [intel-tdx-module-1.5-base-spec] Section 11.3 and
section11.6.1, CR0.CD is enforced to 0 in TD-Guest.
And refer to section 18.2.1.4,  TDX module MTRR emulation
enforces WB in VMM.

Currently the initial MTRR are:
- Td-Guest     :  MTRR disabled, Type is WB.
- Non-Td-Guest :  MTRR disabled, Type is UC.

In DXE phase, OVMF/TDVF would check the MTRR Type for MMIO
(in CpuSetMemoryAttributes -> MtrrGetMemoryAttribute ->
MtrrGetMemoryAttributeworker:
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/MtrrLib/MtrrLib.c#L929
).
If MTRR is disabled, it always returns UC. Otherwise, it returns
the actual value.

If it checks that the type is not UC then the MTRR is programmed.
It is required to disable cache by setting CR0.CD to 1. That will trigger
an unexpected #VE in TD-Guest.

Based on above analysis we propose to skip "Setup MTRR early" in
TD-Guest because of:
- TD-Guest doesn’t have the issue that lzma uncompress extremely slow.
- This patch will trigger an unexpected #VE in TD-Guest.

intel-tdx-module-1.5-base-spec:
https://cdrdv2.intel.com/v1/dl/getContent/733575

Cc: Erdem Aktas <[email protected]>
Cc: Jiewen Yao <[email protected]>
Cc: Min Xu <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Elena Reshetova <[email protected]>
Signed-off-by: Ceping Sun <[email protected]>
@jyao1 jyao1 added the push Auto push patch series in PR if all checks pass label Jul 9, 2024
@mergify mergify bot merged commit 071d2cf into tianocore:master Jul 11, 2024
127 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
push Auto push patch series in PR if all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants