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

["Sun, Yi Y" <yi.y.sun@intel.com>]

Hi, all,

Per our test, this issue only happens when the mdev/vdev is assigned to VM. But PF(physical function)/VF(virtual function) assignment is good. Per Jiaqing's investigation, it is because that the mdev/vdev passthrough flow is different with PF/VF. For vdev/mdev passthrough to VM, the enforce_cache_coherency was enabled after device bound to iommufd. But it doesn't update the kvm coherency state after that. This makes the memory type be UNCACHABLE which causes the reading is very slow.

So, what is your opinion to fix it? Which side to fix it is better, ovmf or vfio? Thanks!

BTW, we have cooked a patch to fix it. But it bases on Intel internal repo. If it is necessary, we can rebase it to upstream.

BRs,
Yi Sun

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Thursday, February 1, 2024 1:21 PM
> To: devel@edk2.groups.io; lersek@redhat.com
> Cc: Michael Roth <michael.roth@amd.com>; Oliver Steffen
> <osteffen@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom
> Lendacky <thomas.lendacky@amd.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Aktas, Erdem <erdemaktas@google.com>;
> Gerd Hoffmann <kraxel@redhat.com>; Sun, Yi Y <yi.y.sun@intel.com>;
> Huang, Jiaqing <jiaqing.huang@intel.com>
> Subject: RE: [edk2-devel] [PATCH v3 1/4] OvmfPkg/Sec: Setup MTRR early in
> the boot process.
> 
> 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/tr
> > >> ee
> > >> /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 (#114977): https://edk2.groups.io/g/devel/message/114977
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]
-=-=-=-=-=-=-=-=-=-=-=-