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

["Min Xu" <min.m.xu@intel.com>]

On Wednesday, January 31, 2024 8:06 PM, Laszlo Ersek wrote:
> 
> Hi Min,
> 
> On 1/30/24 20:22, Laszlo Ersek wrote:
> > On 1/30/24 14:04, 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 and MTRR setup in PlatformInitLib to the changes.
> >>
> >> Background:  Depending on virtual machine configuration kvm may uses
> >> EPT memory types to apply guest MTRR settings.  In case MTRRs are
> >> disabled kvm will use the uncachable memory type for all mappings.
> >> The
> >> vmx_get_mt_mask() function in the linux kernel handles this and can
> >> be found here:
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree
> >> /arch/x86/kvm/vmx/vmx.c?h=v6.7.1#n7580
> >>
> >> In most VM configurations kvm uses MTRR_TYPE_WRBACK
> unconditionally.
> >> In case the VM has a mdev device assigned that is not the case though.
> >>
> >> Before commit e8aa4c6546ad ("UefiCpuPkg/ResetVector: Cache Disable
> >> should not be set by default in CR0") kvm also ended up using
> >> MTRR_TYPE_WRBACK due to KVM_X86_QUIRK_CD_NW_CLEARED.  After
> that
> >> commit kvm evaluates guest mtrr settings, which why setting up MTRRs
> >> early is important now.
> >>
> >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >> ---
> >>  OvmfPkg/IntelTdx/Sec/SecMain.c              | 32 +++++++++++++++++++++
> >>  OvmfPkg/Library/PlatformInitLib/MemDetect.c | 10 +++----
> >>  OvmfPkg/Sec/SecMain.c                       | 32 +++++++++++++++++++++
> >>  3 files changed, 69 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/OvmfPkg/IntelTdx/Sec/SecMain.c
> >> b/OvmfPkg/IntelTdx/Sec/SecMain.c index 42a587adfa57..a218ca17a01a
> >> 100644
> >> --- a/OvmfPkg/IntelTdx/Sec/SecMain.c
> >> +++ b/OvmfPkg/IntelTdx/Sec/SecMain.c
> >> @@ -27,6 +27,8 @@
> >>  #include <Library/TdxHelperLib.h>
> >>  #include <Library/CcProbeLib.h>
> >>  #include <Library/PeilessStartupLib.h>
> >> +#include <Register/Intel/ArchitecturalMsr.h>
> >> +#include <Register/Intel/Cpuid.h>
> >>
> >>  #define SEC_IDT_ENTRY_COUNT  34
> >>
> >> @@ -48,6 +50,31 @@ IA32_IDT_GATE_DESCRIPTOR  mIdtEntryTemplate =
> {
> >>    }
> >>  };
> >>
> >> +//
> >> +// 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    = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
> >> +  DefType.Bits.Type = 6; /* write back */
> >> +  DefType.Bits.E    = 1; /* enable */
> >> +  AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64); }
> >> +
> >>  VOID
> >>  EFIAPI
> >>  SecCoreStartupWithStack (
> >> @@ -204,6 +231,11 @@ SecCoreStartupWithStack (
> >>    InitializeApicTimer (0, MAX_UINT32, TRUE, 5);
> >>    DisableApicTimerInterrupt ();
> >>
> >> +  //
> >> +  // Initialize MTRR
> >> +  //
> >> +  SecMtrrSetup ();
> >> +
> >>    PeilessStartup (&SecCoreData);
> >>
> >>    ASSERT (FALSE);
> >> diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> >> b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> >> index f042517bb64a..e89f63eee054 100644
> >> --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> >> +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c
> >> @@ -1082,18 +1082,18 @@ PlatformQemuInitializeRam (
> >>      MtrrGetAllMtrrs (&MtrrSettings);
> >>
> >>      //
> >> -    // MTRRs disabled, fixed MTRRs disabled, default type is uncached
> >> +    // See SecMtrrSetup(), default type should be write back
> >>      //
> >> -    ASSERT ((MtrrSettings.MtrrDefType & BIT11) == 0);
> >> +    ASSERT ((MtrrSettings.MtrrDefType & BIT11) != 0);
> >>      ASSERT ((MtrrSettings.MtrrDefType & BIT10) == 0);
> >> -    ASSERT ((MtrrSettings.MtrrDefType & 0xFF) == 0);
> >> +    ASSERT ((MtrrSettings.MtrrDefType & 0xFF) ==
> >> + MTRR_CACHE_WRITE_BACK);
> >>
> >>      //
> >>      // flip default type to writeback
> >>      //
> >> -    SetMem (&MtrrSettings.Fixed, sizeof MtrrSettings.Fixed, 0x06);
> >> +    SetMem (&MtrrSettings.Fixed, sizeof MtrrSettings.Fixed,
> >> + MTRR_CACHE_WRITE_BACK);
> >>      ZeroMem (&MtrrSettings.Variables, sizeof MtrrSettings.Variables);
> >> -    MtrrSettings.MtrrDefType |= BIT11 | BIT10 | 6;
> >> +    MtrrSettings.MtrrDefType |= BIT10;
> >>      MtrrSetAllMtrrs (&MtrrSettings);
> >>
> >>      //
> >> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c index
> >> 31da5d0ace51..46c54f2984ff 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>
> >>  #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    = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
> >> +  DefType.Bits.Type = 6; /* write back */
> >> +  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.
> >>    //
> >
> > Cannot comment on the "OvmfPkg/IntelTdx/Sec/SecMain.c" source file
> > changes, but for the rest:
> 
> Can you confirm (a) this patch is OK for "OvmfPkg/IntelTdx/Sec/SecMain.c",
> and (b) this series fixes the slowdown you had encountered?
> 
> (that's what's left before we can merge this series)
> 
We test the patch in TDX and find EXIT_REASON_CR_ACCESS is triggered in DXE phase. It cause an assert because it is not handled by CcExitHandleVe.

Thanks
Min


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