Re: [edk2-devel] [PATCH 1/1] OvmfPkg/Sec: Setup MTRR early in the boot process.

["Laszlo Ersek" <lersek@redhat.com>]

On 1/24/24 16:15, Gerd Hoffmann wrote:
> Specifically before running lzma uncompress of the main firmware volume.
> This is needed to make sure caching is enabled, otherwise the uncompress
> can be extremely slow.
> 
> Adapt the ASSERTs in PlatformInitLib to the changes.
> 
> Background:  In some virtual machine configurations with assigned
> devices kvm uses EPT memory types to apply guest MTRR settings.
> In case MTRRs are disabled kvm will use the uncachable memory type
> for all mappings.

I suggest mentioning mdev and noncoherent DMA here (the linux code you
quoted elsewhere would be welcome too).

> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Library/PlatformInitLib/MemDetect.c |  8 ++++--
>  OvmfPkg/Sec/SecMain.c                       | 32 +++++++++++++++++++++
>  2 files changed, 37 insertions(+), 3 deletions(-)

The original report <https://edk2.groups.io/g/devel/message/113517>
claims that commit e8aa4c6546ad ("UefiCpuPkg/ResetVector: Cache Disable
should not be set by default in CR0", 2023-08-30) triggered the symptom.

I still don't understand how that is possible. Assuming we have
noncoherent DMA due to mdev assignment, KVM will honor "Guest
CD/MTRR/PAT". Let's consider each in turn:

- CD (cache disable): set to zero in recent commit e8aa4c6546ad.

- MTRR: we set DefType to WB, in this patch, but not enabled upstream,
at present

- PAT: IIUC, we don't deal with that at all in edk2

- PCD: not set, minimally through edk2 commit 98f378a7be12
("OvmfPkg/ResetVector: enable caching in initial page tables", 2013-09-24)

I don't understand how *clearing* CD in CR0, could make guest memory
*less* cacheable. I understand the point about MTRR, but exactly the
MTRR's currently upstream state should have masked any changes to CD.

(1) Is the initial report wrong?

> 
> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> index f042517bb64a..cb2ae0f3d79d 100644
> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> @@ -1082,11 +1082,13 @@ PlatformQemuInitializeRam (
>      MtrrGetAllMtrrs (&MtrrSettings);
>  
>      //
> -    // MTRRs disabled, fixed MTRRs disabled, default type is uncached
> +    // Fixed MTRRs disabled, default type is uncached or write back (see SecMtrrSetup())
>      //
> -    ASSERT ((MtrrSettings.MtrrDefType & BIT11) == 0);
>      ASSERT ((MtrrSettings.MtrrDefType & BIT10) == 0);
> -    ASSERT ((MtrrSettings.MtrrDefType & 0xFF) == 0);
> +    ASSERT (
> +      (MtrrSettings.MtrrDefType & 0xFF) == 0x0 ||
> +      (MtrrSettings.MtrrDefType & 0xFF) == 0x6
> +      );
>  
>      //
>      // flip default type to writeback

This code comes (originally) from commit 79d274b8b6b1 ("OvmfPkg:
PlatformPei: invert MTRR setup in QemuInitializeRam()", 2015-06-26).

The purpose(s) of that commit include "setting the default to writeback"
-- even in the context quoted above, we see the comment "flip default
type to writeback". A wider context is:

    //
    // flip default type to writeback
    //
    SetMem (&MtrrSettings.Fixed, sizeof MtrrSettings.Fixed, 0x06);
    ZeroMem (&MtrrSettings.Variables, sizeof MtrrSettings.Variables);
    MtrrSettings.MtrrDefType |= BIT11 | BIT10 | 6;
    MtrrSetAllMtrrs (&MtrrSettings);

So:

(2) We already assume (minimally since 2015) that MTRRs are supported by
the processor. This means that (a) the CPUID check in SecMtrrSetup() is
superfluous, and that (b) we need not / should not permit

  (MtrrSettings.MtrrDefType & 0xFF) == 0x0

in the ASSERT(), going forward.

(3) The rest of the code and *comments* here -- in
PlatformQemuInitializeRam() -- should be updated such that we only
enable the fixed MTRRs (BIT10) on top of what SEC does earlier (which
is: BIT11 (general enable) and DefType=6).

Something like:

  ASSERT ((MtrrSettings.MtrrDefType & BIT11) != 0);
  ASSERT ((MtrrSettings.MtrrDefType & BIT10) == 0);
  ASSERT ((MtrrSettings.MtrrDefType & 0xFF) == 6);

  ...

  MtrrSettings.MtrrDefType |= BIT10;

> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 31da5d0ace51..a672751b046a 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -30,6 +30,8 @@
>  #include <Ppi/MpInitLibDep.h>
>  #include <Library/TdxHelperLib.h>
>  #include <Library/CcProbeLib.h>
> +#include <Register/Intel/ArchitecturalMsr.h>
> +#include <Register/Intel/Cpuid.h>

(4) with the CPUID gone, the second header should not be needed

>  #include "AmdSev.h"
>  
>  #define SEC_IDT_ENTRY_COUNT  34
> @@ -744,6 +746,31 @@ FindAndReportEntryPoints (
>    return;
>  }
>  
> +//
> +// Enable MTRR early, set default type to write back.
> +// Needed to make sure caching is enabled,
> +// without this lzma decompress can be very slow.
> +//
> +STATIC
> +VOID
> +SecMtrrSetup (
> +  VOID
> +  )
> +{
> +  CPUID_VERSION_INFO_EDX           Edx;
> +  MSR_IA32_MTRR_DEF_TYPE_REGISTER  DefType;
> +
> +  AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &Edx.Uint32);
> +  if (!Edx.Bits.MTRR) {
> +    return;
> +  }
> +
> +  DefType.Uint64    = 0;

(5) Can we first read the MSR, instead of explicit zeroing?

> +  DefType.Bits.Type = 6; /* write back */

(6) I've noticed an independent (preexistent!) wart: I dislike the naked
constant "6" for WriteBack.

Turns out we have had MTRR_CACHE_WRITE_BACK in

  UefiCpuPkg/Include/Library/MtrrLib.h

since historical commit e50466da2437 ("Add MTRR library for IA32 & X64
processor architectures.", 2009-05-27).

This means that my original commit 79d274b8b6b1 ("OvmfPkg: PlatformPei:
invert MTRR setup in QemuInitializeRam()", 2015-06-26) could have been
better: that commit already consumed MtrrLib, so it could have used
MTRR_CACHE_WRITE_BACK, in place of "6", from the lib class header.

OVMF SEC however does not include <Library/MtrrLib.h> -- and it
shouldn't. That means it cannot use MTRR_CACHE_WRITE_BACK. That makes me
somewhat unhappy.

Proposal:

- Copy and rename the MTRR_CACHE_* macros from
"UefiCpuPkg/Include/Library/MtrrLib.h" to
"MdePkg/Include/Register/Intel/ArchitecturalMsr.h"

- Redefine MTRR_CACHE_* in MtrrLib.h in terms of the new macros from
"ArchitecturalMsr.h".

(in a separate patch, of course).

This will preserve compatibility, but also expose the (new) macro names
to modules that don't consume MtrrLib -- for example, OVMF SEC.

Probably consult the MtrrLib and ArchitecturalMsr.h maintainers first,
though -- so I would propose three patch in the end:

- this patch, using naked constant "6"

- macro reorg

- adopting the new macro name in OVMF SEC, in place of 6

If the MtrrLib and ArchitecturalMsr.h maintainers disagree with patch
#2, we can simply drop #2 and #3, and just merge #1.


> +  DefType.Bits.E    = 1; /* enable */
> +  AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
> +}
> +
>  VOID
>  EFIAPI
>  SecCoreStartupWithStack (
> @@ -942,6 +969,11 @@ SecCoreStartupWithStack (
>    InitializeApicTimer (0, MAX_UINT32, TRUE, 5);
>    DisableApicTimerInterrupt ();
>  
> +  //
> +  // Initialize MTRR
> +  //
> +  SecMtrrSetup ();
> +
>    //
>    // Initialize Debug Agent to support source level debug in SEC/PEI phases before memory ready.
>    //

Ugh, I can't really comment where to place this call. FWIW, the problem
path is:

SecCoreStartupWithStack()
  InitializeDebugAgent()
    SecStartupPhase2()
      FindAndReportEntryPoints()
        FindPeiCoreImageBase()
          DecompressMemFvs()
            ExtractGuidedSectionDecode()

That's where the LZMA decompression happens.

The patch places the call to SecMtrrSetup() "early enough", but is it
the "best" location from all possible, "early enough" locations? I can't
say.

(7) If you have a particular reason for doing it here, please capture
that in the comment.


(8) Just for symmetry -- I'm noticing there are two
SecCoreStartupWithStack() functions; the other being in
"OvmfPkg/IntelTdx/Sec/SecMain.c".

Also, Min's original QEMU command line included TDVF references.

Does that mean this patch should be reflected to the TDX platform's modules?

Thanks,
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114315): https://edk2.groups.io/g/devel/message/114315
Mute This Topic: https://groups.io/mt/103933443/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-