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

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

[Gerd Hoffmann]

Also add #defines for MTRR cache types to ArchitecturalMsr.h
and followup patches to use these #defines.

v2:
 - Use constants for the mtrr enum too.
 - Don't include linux kernel source code in the commit message,
   use a link instead.  Also reword the description.

Gerd Hoffmann (4):
  OvmfPkg/Sec: Setup MTRR early in the boot process.
  MdePkg/ArchitecturalMsr.h: add #defines for MTRR cache types
  UefiCpuPkg/MtrrLib.h: use cache type #defines from ArchitecturalMsr.h
  OvmfPkg/Sec: use cache type #defines from ArchitecturalMsr.h

 .../Include/Register/Intel/ArchitecturalMsr.h |  7 ++++
 UefiCpuPkg/Include/Library/MtrrLib.h          | 26 ++++++++-------
 OvmfPkg/IntelTdx/Sec/SecMain.c                | 32 +++++++++++++++++++
 OvmfPkg/Library/PlatformInitLib/MemDetect.c   | 10 +++---
 OvmfPkg/Sec/SecMain.c                         | 32 +++++++++++++++++++
 5 files changed, 90 insertions(+), 17 deletions(-)

-- 
2.43.0



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

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

[Gerd Hoffmann]

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.
   //
-- 
2.43.0



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114782): https://edk2.groups.io/g/devel/message/114782
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]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 2/4] MdePkg/ArchitecturalMsr.h: add #defines for MTRR cache types

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 MdePkg/Include/Register/Intel/ArchitecturalMsr.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MdePkg/Include/Register/Intel/ArchitecturalMsr.h b/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
index 756e7c86ec31..2b2e34b08c8b 100644
--- a/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
+++ b/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
@@ -2103,6 +2103,13 @@ typedef union {
 #define MSR_IA32_MTRR_PHYSBASE9  0x00000212
 /// @}
 
+#define MSR_IA32_MTRR_CACHE_UNCACHEABLE      0
+#define MSR_IA32_MTRR_CACHE_WRITE_COMBINING  1
+#define MSR_IA32_MTRR_CACHE_WRITE_THROUGH    4
+#define MSR_IA32_MTRR_CACHE_WRITE_PROTECTED  5
+#define MSR_IA32_MTRR_CACHE_WRITE_BACK       6
+#define MSR_IA32_MTRR_CACHE_INVALID_TYPE     7
+
 /**
   MSR information returned for MSR indexes #MSR_IA32_MTRR_PHYSBASE0 to
   #MSR_IA32_MTRR_PHYSBASE9
-- 
2.43.0



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

[edk2-devel] [PATCH v3 3/4] UefiCpuPkg/MtrrLib.h: use cache type #defines from ArchitecturalMsr.h

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Include/Library/MtrrLib.h | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/UefiCpuPkg/Include/Library/MtrrLib.h b/UefiCpuPkg/Include/Library/MtrrLib.h
index 86cc1aab3b8e..9d715d50dfb0 100644
--- a/UefiCpuPkg/Include/Library/MtrrLib.h
+++ b/UefiCpuPkg/Include/Library/MtrrLib.h
@@ -9,6 +9,8 @@
 #ifndef  _MTRR_LIB_H_
 #define  _MTRR_LIB_H_
 
+#include <Register/Intel/ArchitecturalMsr.h>
+
 //
 // According to IA32 SDM, MTRRs number and MSR offset are always consistent
 // for IA32 processor family
@@ -82,20 +84,20 @@ typedef struct _MTRR_SETTINGS_ {
 // Memory cache types
 //
 typedef enum {
-  CacheUncacheable    = 0,
-  CacheWriteCombining = 1,
-  CacheWriteThrough   = 4,
-  CacheWriteProtected = 5,
-  CacheWriteBack      = 6,
-  CacheInvalid        = 7
+  CacheUncacheable    = MSR_IA32_MTRR_CACHE_UNCACHEABLE,
+  CacheWriteCombining = MSR_IA32_MTRR_CACHE_WRITE_COMBINING,
+  CacheWriteThrough   = MSR_IA32_MTRR_CACHE_WRITE_THROUGH,
+  CacheWriteProtected = MSR_IA32_MTRR_CACHE_WRITE_PROTECTED,
+  CacheWriteBack      = MSR_IA32_MTRR_CACHE_WRITE_BACK,
+  CacheInvalid        = MSR_IA32_MTRR_CACHE_INVALID_TYPE,
 } MTRR_MEMORY_CACHE_TYPE;
 
-#define  MTRR_CACHE_UNCACHEABLE      0
-#define  MTRR_CACHE_WRITE_COMBINING  1
-#define  MTRR_CACHE_WRITE_THROUGH    4
-#define  MTRR_CACHE_WRITE_PROTECTED  5
-#define  MTRR_CACHE_WRITE_BACK       6
-#define  MTRR_CACHE_INVALID_TYPE     7
+#define  MTRR_CACHE_UNCACHEABLE      MSR_IA32_MTRR_CACHE_UNCACHEABLE
+#define  MTRR_CACHE_WRITE_COMBINING  MSR_IA32_MTRR_CACHE_WRITE_COMBINING
+#define  MTRR_CACHE_WRITE_THROUGH    MSR_IA32_MTRR_CACHE_WRITE_THROUGH
+#define  MTRR_CACHE_WRITE_PROTECTED  MSR_IA32_MTRR_CACHE_WRITE_PROTECTED
+#define  MTRR_CACHE_WRITE_BACK       MSR_IA32_MTRR_CACHE_WRITE_BACK
+#define  MTRR_CACHE_INVALID_TYPE     MSR_IA32_MTRR_CACHE_INVALID_TYPE
 
 typedef struct {
   UINT64                    BaseAddress;
-- 
2.43.0



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

[edk2-devel] [PATCH v3 4/4] OvmfPkg/Sec: use cache type #defines from ArchitecturalMsr.h

[Gerd Hoffmann]

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/IntelTdx/Sec/SecMain.c | 2 +-
 OvmfPkg/Sec/SecMain.c          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/IntelTdx/Sec/SecMain.c b/OvmfPkg/IntelTdx/Sec/SecMain.c
index a218ca17a01a..9006c7e6a1b3 100644
--- a/OvmfPkg/IntelTdx/Sec/SecMain.c
+++ b/OvmfPkg/IntelTdx/Sec/SecMain.c
@@ -70,7 +70,7 @@ SecMtrrSetup (
   }
 
   DefType.Uint64    = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
-  DefType.Bits.Type = 6; /* write back */
+  DefType.Bits.Type = MSR_IA32_MTRR_CACHE_WRITE_BACK;
   DefType.Bits.E    = 1; /* enable */
   AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
 }
diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index 46c54f2984ff..4be371eff330 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -766,7 +766,7 @@ SecMtrrSetup (
   }
 
   DefType.Uint64    = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
-  DefType.Bits.Type = 6; /* write back */
+  DefType.Bits.Type = MSR_IA32_MTRR_CACHE_WRITE_BACK;
   DefType.Bits.E    = 1; /* enable */
   AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
 }
-- 
2.43.0



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

Re: [edk2-devel] [PATCH v3 3/4] UefiCpuPkg/MtrrLib.h: use cache type #defines from ArchitecturalMsr.h

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Tuesday, January 30, 2024 5:05 AM
> To: devel@edk2.groups.io
> Cc: Michael Roth <michael.roth@amd.com>; Oliver Steffen
> <osteffen@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Tom Lendacky <thomas.lendacky@amd.com>; Laszlo
> Ersek <lersek@redhat.com>; Xu, Min M <min.m.xu@intel.com>; Ard
> Biesheuvel <ardb+tianocore@kernel.org>; Aktas, Erdem
> <erdemaktas@google.com>
> Subject: [edk2-devel] [PATCH v3 3/4] UefiCpuPkg/MtrrLib.h: use cache
> type #defines from ArchitecturalMsr.h
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Include/Library/MtrrLib.h | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/UefiCpuPkg/Include/Library/MtrrLib.h
> b/UefiCpuPkg/Include/Library/MtrrLib.h
> index 86cc1aab3b8e..9d715d50dfb0 100644
> --- a/UefiCpuPkg/Include/Library/MtrrLib.h
> +++ b/UefiCpuPkg/Include/Library/MtrrLib.h
> @@ -9,6 +9,8 @@
>  #ifndef  _MTRR_LIB_H_
>  #define  _MTRR_LIB_H_
> 
> +#include <Register/Intel/ArchitecturalMsr.h>
> +
>  //
>  // According to IA32 SDM, MTRRs number and MSR offset are always
> consistent
>  // for IA32 processor family
> @@ -82,20 +84,20 @@ typedef struct _MTRR_SETTINGS_ {
>  // Memory cache types
>  //
>  typedef enum {
> -  CacheUncacheable    = 0,
> -  CacheWriteCombining = 1,
> -  CacheWriteThrough   = 4,
> -  CacheWriteProtected = 5,
> -  CacheWriteBack      = 6,
> -  CacheInvalid        = 7
> +  CacheUncacheable    = MSR_IA32_MTRR_CACHE_UNCACHEABLE,
> +  CacheWriteCombining = MSR_IA32_MTRR_CACHE_WRITE_COMBINING,
> +  CacheWriteThrough   = MSR_IA32_MTRR_CACHE_WRITE_THROUGH,
> +  CacheWriteProtected = MSR_IA32_MTRR_CACHE_WRITE_PROTECTED,
> +  CacheWriteBack      = MSR_IA32_MTRR_CACHE_WRITE_BACK,
> +  CacheInvalid        = MSR_IA32_MTRR_CACHE_INVALID_TYPE,
>  } MTRR_MEMORY_CACHE_TYPE;
> 
> -#define  MTRR_CACHE_UNCACHEABLE      0
> -#define  MTRR_CACHE_WRITE_COMBINING  1
> -#define  MTRR_CACHE_WRITE_THROUGH    4
> -#define  MTRR_CACHE_WRITE_PROTECTED  5
> -#define  MTRR_CACHE_WRITE_BACK       6
> -#define  MTRR_CACHE_INVALID_TYPE     7
> +#define  MTRR_CACHE_UNCACHEABLE      MSR_IA32_MTRR_CACHE_UNCACHEABLE
> +#define  MTRR_CACHE_WRITE_COMBINING
> MSR_IA32_MTRR_CACHE_WRITE_COMBINING
> +#define  MTRR_CACHE_WRITE_THROUGH    MSR_IA32_MTRR_CACHE_WRITE_THROUGH
> +#define  MTRR_CACHE_WRITE_PROTECTED
> MSR_IA32_MTRR_CACHE_WRITE_PROTECTED
> +#define  MTRR_CACHE_WRITE_BACK       MSR_IA32_MTRR_CACHE_WRITE_BACK
> +#define  MTRR_CACHE_INVALID_TYPE     MSR_IA32_MTRR_CACHE_INVALID_TYPE
> 
>  typedef struct {
>    UINT64                    BaseAddress;
> --
> 2.43.0
> 
> 
> 
> 
> 



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

Re: [edk2-devel] [PATCH v3 2/4] MdePkg/ArchitecturalMsr.h: add #defines for MTRR cache types

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Tuesday, January 30, 2024 5:05 AM
> To: devel@edk2.groups.io
> Cc: Michael Roth <michael.roth@amd.com>; Oliver Steffen
> <osteffen@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Tom Lendacky <thomas.lendacky@amd.com>; Laszlo
> Ersek <lersek@redhat.com>; Xu, Min M <min.m.xu@intel.com>; Ard
> Biesheuvel <ardb+tianocore@kernel.org>; Aktas, Erdem
> <erdemaktas@google.com>
> Subject: [edk2-devel] [PATCH v3 2/4] MdePkg/ArchitecturalMsr.h: add
> #defines for MTRR cache types
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  MdePkg/Include/Register/Intel/ArchitecturalMsr.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
> b/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
> index 756e7c86ec31..2b2e34b08c8b 100644
> --- a/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
> +++ b/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
> @@ -2103,6 +2103,13 @@ typedef union {
>  #define MSR_IA32_MTRR_PHYSBASE9  0x00000212
>  /// @}
> 
> +#define MSR_IA32_MTRR_CACHE_UNCACHEABLE      0
> +#define MSR_IA32_MTRR_CACHE_WRITE_COMBINING  1
> +#define MSR_IA32_MTRR_CACHE_WRITE_THROUGH    4
> +#define MSR_IA32_MTRR_CACHE_WRITE_PROTECTED  5
> +#define MSR_IA32_MTRR_CACHE_WRITE_BACK       6
> +#define MSR_IA32_MTRR_CACHE_INVALID_TYPE     7
> +
>  /**
>    MSR information returned for MSR indexes #MSR_IA32_MTRR_PHYSBASE0 to
>    #MSR_IA32_MTRR_PHYSBASE9
> --
> 2.43.0
> 
> 
> 
> 
> 



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

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

[Laszlo Ersek]

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:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

Re: [edk2-devel] [PATCH v3 2/4] MdePkg/ArchitecturalMsr.h: add #defines for MTRR cache types

[Laszlo Ersek]

On 1/30/24 14:04, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  MdePkg/Include/Register/Intel/ArchitecturalMsr.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MdePkg/Include/Register/Intel/ArchitecturalMsr.h b/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
> index 756e7c86ec31..2b2e34b08c8b 100644
> --- a/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
> +++ b/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
> @@ -2103,6 +2103,13 @@ typedef union {
>  #define MSR_IA32_MTRR_PHYSBASE9  0x00000212
>  /// @}
>  
> +#define MSR_IA32_MTRR_CACHE_UNCACHEABLE      0
> +#define MSR_IA32_MTRR_CACHE_WRITE_COMBINING  1
> +#define MSR_IA32_MTRR_CACHE_WRITE_THROUGH    4
> +#define MSR_IA32_MTRR_CACHE_WRITE_PROTECTED  5
> +#define MSR_IA32_MTRR_CACHE_WRITE_BACK       6
> +#define MSR_IA32_MTRR_CACHE_INVALID_TYPE     7
> +
>  /**
>    MSR information returned for MSR indexes #MSR_IA32_MTRR_PHYSBASE0 to
>    #MSR_IA32_MTRR_PHYSBASE9

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

Re: [edk2-devel] [PATCH v3 3/4] UefiCpuPkg/MtrrLib.h: use cache type #defines from ArchitecturalMsr.h

[Laszlo Ersek]

On 1/30/24 14:04, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Include/Library/MtrrLib.h | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/UefiCpuPkg/Include/Library/MtrrLib.h b/UefiCpuPkg/Include/Library/MtrrLib.h
> index 86cc1aab3b8e..9d715d50dfb0 100644
> --- a/UefiCpuPkg/Include/Library/MtrrLib.h
> +++ b/UefiCpuPkg/Include/Library/MtrrLib.h
> @@ -9,6 +9,8 @@
>  #ifndef  _MTRR_LIB_H_
>  #define  _MTRR_LIB_H_
>  
> +#include <Register/Intel/ArchitecturalMsr.h>
> +
>  //
>  // According to IA32 SDM, MTRRs number and MSR offset are always consistent
>  // for IA32 processor family
> @@ -82,20 +84,20 @@ typedef struct _MTRR_SETTINGS_ {
>  // Memory cache types
>  //
>  typedef enum {
> -  CacheUncacheable    = 0,
> -  CacheWriteCombining = 1,
> -  CacheWriteThrough   = 4,
> -  CacheWriteProtected = 5,
> -  CacheWriteBack      = 6,
> -  CacheInvalid        = 7
> +  CacheUncacheable    = MSR_IA32_MTRR_CACHE_UNCACHEABLE,
> +  CacheWriteCombining = MSR_IA32_MTRR_CACHE_WRITE_COMBINING,
> +  CacheWriteThrough   = MSR_IA32_MTRR_CACHE_WRITE_THROUGH,
> +  CacheWriteProtected = MSR_IA32_MTRR_CACHE_WRITE_PROTECTED,
> +  CacheWriteBack      = MSR_IA32_MTRR_CACHE_WRITE_BACK,
> +  CacheInvalid        = MSR_IA32_MTRR_CACHE_INVALID_TYPE,
>  } MTRR_MEMORY_CACHE_TYPE;
>  
> -#define  MTRR_CACHE_UNCACHEABLE      0
> -#define  MTRR_CACHE_WRITE_COMBINING  1
> -#define  MTRR_CACHE_WRITE_THROUGH    4
> -#define  MTRR_CACHE_WRITE_PROTECTED  5
> -#define  MTRR_CACHE_WRITE_BACK       6
> -#define  MTRR_CACHE_INVALID_TYPE     7
> +#define  MTRR_CACHE_UNCACHEABLE      MSR_IA32_MTRR_CACHE_UNCACHEABLE
> +#define  MTRR_CACHE_WRITE_COMBINING  MSR_IA32_MTRR_CACHE_WRITE_COMBINING
> +#define  MTRR_CACHE_WRITE_THROUGH    MSR_IA32_MTRR_CACHE_WRITE_THROUGH
> +#define  MTRR_CACHE_WRITE_PROTECTED  MSR_IA32_MTRR_CACHE_WRITE_PROTECTED
> +#define  MTRR_CACHE_WRITE_BACK       MSR_IA32_MTRR_CACHE_WRITE_BACK
> +#define  MTRR_CACHE_INVALID_TYPE     MSR_IA32_MTRR_CACHE_INVALID_TYPE
>  
>  typedef struct {
>    UINT64                    BaseAddress;

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

Re: [edk2-devel] [PATCH v3 4/4] OvmfPkg/Sec: use cache type #defines from ArchitecturalMsr.h

[Laszlo Ersek]

On 1/30/24 14:04, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/IntelTdx/Sec/SecMain.c | 2 +-
>  OvmfPkg/Sec/SecMain.c          | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/IntelTdx/Sec/SecMain.c b/OvmfPkg/IntelTdx/Sec/SecMain.c
> index a218ca17a01a..9006c7e6a1b3 100644
> --- a/OvmfPkg/IntelTdx/Sec/SecMain.c
> +++ b/OvmfPkg/IntelTdx/Sec/SecMain.c
> @@ -70,7 +70,7 @@ SecMtrrSetup (
>    }
>  
>    DefType.Uint64    = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
> -  DefType.Bits.Type = 6; /* write back */
> +  DefType.Bits.Type = MSR_IA32_MTRR_CACHE_WRITE_BACK;
>    DefType.Bits.E    = 1; /* enable */
>    AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
>  }
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 46c54f2984ff..4be371eff330 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -766,7 +766,7 @@ SecMtrrSetup (
>    }
>  
>    DefType.Uint64    = AsmReadMsr64 (MSR_IA32_MTRR_DEF_TYPE);
> -  DefType.Bits.Type = 6; /* write back */
> +  DefType.Bits.Type = MSR_IA32_MTRR_CACHE_WRITE_BACK;
>    DefType.Bits.E    = 1; /* enable */
>    AsmWriteMsr64 (MSR_IA32_MTRR_DEF_TYPE, DefType.Uint64);
>  }

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



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

Re: [edk2-devel] [PATCH v3 3/4] UefiCpuPkg/MtrrLib.h: use cache type #defines from ArchitecturalMsr.h

[Laszlo Ersek]

CC'ing Mike too

On 1/30/24 14:04, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Include/Library/MtrrLib.h | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/UefiCpuPkg/Include/Library/MtrrLib.h b/UefiCpuPkg/Include/Library/MtrrLib.h
> index 86cc1aab3b8e..9d715d50dfb0 100644
> --- a/UefiCpuPkg/Include/Library/MtrrLib.h
> +++ b/UefiCpuPkg/Include/Library/MtrrLib.h
> @@ -9,6 +9,8 @@
>  #ifndef  _MTRR_LIB_H_
>  #define  _MTRR_LIB_H_
>  
> +#include <Register/Intel/ArchitecturalMsr.h>
> +
>  //
>  // According to IA32 SDM, MTRRs number and MSR offset are always consistent
>  // for IA32 processor family
> @@ -82,20 +84,20 @@ typedef struct _MTRR_SETTINGS_ {
>  // Memory cache types
>  //
>  typedef enum {
> -  CacheUncacheable    = 0,
> -  CacheWriteCombining = 1,
> -  CacheWriteThrough   = 4,
> -  CacheWriteProtected = 5,
> -  CacheWriteBack      = 6,
> -  CacheInvalid        = 7
> +  CacheUncacheable    = MSR_IA32_MTRR_CACHE_UNCACHEABLE,
> +  CacheWriteCombining = MSR_IA32_MTRR_CACHE_WRITE_COMBINING,
> +  CacheWriteThrough   = MSR_IA32_MTRR_CACHE_WRITE_THROUGH,
> +  CacheWriteProtected = MSR_IA32_MTRR_CACHE_WRITE_PROTECTED,
> +  CacheWriteBack      = MSR_IA32_MTRR_CACHE_WRITE_BACK,
> +  CacheInvalid        = MSR_IA32_MTRR_CACHE_INVALID_TYPE,
>  } MTRR_MEMORY_CACHE_TYPE;
>  
> -#define  MTRR_CACHE_UNCACHEABLE      0
> -#define  MTRR_CACHE_WRITE_COMBINING  1
> -#define  MTRR_CACHE_WRITE_THROUGH    4
> -#define  MTRR_CACHE_WRITE_PROTECTED  5
> -#define  MTRR_CACHE_WRITE_BACK       6
> -#define  MTRR_CACHE_INVALID_TYPE     7
> +#define  MTRR_CACHE_UNCACHEABLE      MSR_IA32_MTRR_CACHE_UNCACHEABLE
> +#define  MTRR_CACHE_WRITE_COMBINING  MSR_IA32_MTRR_CACHE_WRITE_COMBINING
> +#define  MTRR_CACHE_WRITE_THROUGH    MSR_IA32_MTRR_CACHE_WRITE_THROUGH
> +#define  MTRR_CACHE_WRITE_PROTECTED  MSR_IA32_MTRR_CACHE_WRITE_PROTECTED
> +#define  MTRR_CACHE_WRITE_BACK       MSR_IA32_MTRR_CACHE_WRITE_BACK
> +#define  MTRR_CACHE_INVALID_TYPE     MSR_IA32_MTRR_CACHE_INVALID_TYPE
>  
>  typedef struct {
>    UINT64                    BaseAddress;




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

Re: [edk2-devel] [PATCH v3 2/4] MdePkg/ArchitecturalMsr.h: add #defines for MTRR cache types

[Laszlo Ersek]

CC'ing MdePkg owners

On 1/30/24 14:04, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  MdePkg/Include/Register/Intel/ArchitecturalMsr.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/MdePkg/Include/Register/Intel/ArchitecturalMsr.h b/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
> index 756e7c86ec31..2b2e34b08c8b 100644
> --- a/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
> +++ b/MdePkg/Include/Register/Intel/ArchitecturalMsr.h
> @@ -2103,6 +2103,13 @@ typedef union {
>  #define MSR_IA32_MTRR_PHYSBASE9  0x00000212
>  /// @}
>  
> +#define MSR_IA32_MTRR_CACHE_UNCACHEABLE      0
> +#define MSR_IA32_MTRR_CACHE_WRITE_COMBINING  1
> +#define MSR_IA32_MTRR_CACHE_WRITE_THROUGH    4
> +#define MSR_IA32_MTRR_CACHE_WRITE_PROTECTED  5
> +#define MSR_IA32_MTRR_CACHE_WRITE_BACK       6
> +#define MSR_IA32_MTRR_CACHE_INVALID_TYPE     7
> +
>  /**
>    MSR information returned for MSR indexes #MSR_IA32_MTRR_PHYSBASE0 to
>    #MSR_IA32_MTRR_PHYSBASE9



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

Re: [edk2-devel] [PATCH v3 3/4] UefiCpuPkg/MtrrLib.h: use cache type #defines from ArchitecturalMsr.h

[Laszlo Ersek]

CC'ing Ray and Rahul

On 1/30/24 14:04, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Include/Library/MtrrLib.h | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/UefiCpuPkg/Include/Library/MtrrLib.h b/UefiCpuPkg/Include/Library/MtrrLib.h
> index 86cc1aab3b8e..9d715d50dfb0 100644
> --- a/UefiCpuPkg/Include/Library/MtrrLib.h
> +++ b/UefiCpuPkg/Include/Library/MtrrLib.h
> @@ -9,6 +9,8 @@
>  #ifndef  _MTRR_LIB_H_
>  #define  _MTRR_LIB_H_
>  
> +#include <Register/Intel/ArchitecturalMsr.h>
> +
>  //
>  // According to IA32 SDM, MTRRs number and MSR offset are always consistent
>  // for IA32 processor family
> @@ -82,20 +84,20 @@ typedef struct _MTRR_SETTINGS_ {
>  // Memory cache types
>  //
>  typedef enum {
> -  CacheUncacheable    = 0,
> -  CacheWriteCombining = 1,
> -  CacheWriteThrough   = 4,
> -  CacheWriteProtected = 5,
> -  CacheWriteBack      = 6,
> -  CacheInvalid        = 7
> +  CacheUncacheable    = MSR_IA32_MTRR_CACHE_UNCACHEABLE,
> +  CacheWriteCombining = MSR_IA32_MTRR_CACHE_WRITE_COMBINING,
> +  CacheWriteThrough   = MSR_IA32_MTRR_CACHE_WRITE_THROUGH,
> +  CacheWriteProtected = MSR_IA32_MTRR_CACHE_WRITE_PROTECTED,
> +  CacheWriteBack      = MSR_IA32_MTRR_CACHE_WRITE_BACK,
> +  CacheInvalid        = MSR_IA32_MTRR_CACHE_INVALID_TYPE,
>  } MTRR_MEMORY_CACHE_TYPE;
>  
> -#define  MTRR_CACHE_UNCACHEABLE      0
> -#define  MTRR_CACHE_WRITE_COMBINING  1
> -#define  MTRR_CACHE_WRITE_THROUGH    4
> -#define  MTRR_CACHE_WRITE_PROTECTED  5
> -#define  MTRR_CACHE_WRITE_BACK       6
> -#define  MTRR_CACHE_INVALID_TYPE     7
> +#define  MTRR_CACHE_UNCACHEABLE      MSR_IA32_MTRR_CACHE_UNCACHEABLE
> +#define  MTRR_CACHE_WRITE_COMBINING  MSR_IA32_MTRR_CACHE_WRITE_COMBINING
> +#define  MTRR_CACHE_WRITE_THROUGH    MSR_IA32_MTRR_CACHE_WRITE_THROUGH
> +#define  MTRR_CACHE_WRITE_PROTECTED  MSR_IA32_MTRR_CACHE_WRITE_PROTECTED
> +#define  MTRR_CACHE_WRITE_BACK       MSR_IA32_MTRR_CACHE_WRITE_BACK
> +#define  MTRR_CACHE_INVALID_TYPE     MSR_IA32_MTRR_CACHE_INVALID_TYPE
>  
>  typedef struct {
>    UINT64                    BaseAddress;



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

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

[Laszlo Ersek]

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)

Thanks
Laszlo

> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> 
> 
> 
> 
> 



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

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

[Min Xu]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sun, Yi Y]

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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

  Hi,

> > 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.

Hmm.  Sure this caused by this patch series?  For the PEI-less TDX
build this series moves the MTRR setup to a different place in SEC.
Once the DXE phase started the MTRR configuration should be identical
with and without this patch series, and the series also doesn't touch
any control register.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114939): https://edk2.groups.io/g/devel/message/114939
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

On Thu, Feb 01, 2024 at 06:10:26AM +0000, Sun, Yi Y wrote:
> 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!

Both?  For OVMF it surely makes sense to properly setup MTRRs.  And if
the cache coherency behavior of vfio is not correct that should be fixed
too of course.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114940): https://edk2.groups.io/g/devel/message/114940
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Sun, Yi Y]

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Thursday, February 1, 2024 5:43 PM
> To: Sun, Yi Y <yi.y.sun@intel.com>
> Cc: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io;
> lersek@redhat.com; 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>;
> Huang, Jiaqing <jiaqing.huang@intel.com>
> Subject: Re: RE: [edk2-devel] [PATCH v3 1/4] OvmfPkg/Sec: Setup MTRR early
> in the boot process.
> 
> On Thu, Feb 01, 2024 at 06:10:26AM +0000, Sun, Yi Y wrote:
> > 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!
> 
> Both?  For OVMF it surely makes sense to properly setup MTRRs.  And if the
> cache coherency behavior of vfio is not correct that should be fixed too of
> course.
> 
Sounds good. Thanks!

BRs,
Yi Sun

> take care,
>   Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114979): https://edk2.groups.io/g/devel/message/114979
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

On Thu, Feb 01, 2024 at 10:38:43AM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > 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.
> 
> Hmm.  Sure this caused by this patch series?  For the PEI-less TDX
> build this series moves the MTRR setup to a different place in SEC.
> Once the DXE phase started the MTRR configuration should be identical
> with and without this patch series, and the series also doesn't touch
> any control register.

Ping.  Can you double-check please?  Our QE ran a test build with this
series applied through regression testing (including TDX) and has not
found any issues.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115352): https://edk2.groups.io/g/devel/message/115352
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Min Xu]

On Monday, February 12, 2024 11:22 PM, Gerd Hoffmann wrote:
> On Thu, Feb 01, 2024 at 10:38:43AM +0100, Gerd Hoffmann wrote:
> >   Hi,
> >
> > > > 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.
> >
> > Hmm.  Sure this caused by this patch series?  For the PEI-less TDX
> > build this series moves the MTRR setup to a different place in SEC.
> > Once the DXE phase started the MTRR configuration should be identical
> > with and without this patch series, and the series also doesn't touch
> > any control register.
> 
> Ping.  Can you double-check please?  Our QE ran a test build with this series
> applied through regression testing (including TDX) and has not found any
> issues.

We double check the patch-set (v3) for both OvmfPkgX64 and IntelTdx. It triggered EXIT_REASON_CR_ACCESS in DXE phase when launching a td-guest.
@Gerd, what's the qemu command and test environment your QE run the case? We'd like run it in our side.

Thanks
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115624): https://edk2.groups.io/g/devel/message/115624
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Gerd Hoffmann]

On Tue, Feb 20, 2024 at 06:27:21AM +0000, Min Xu wrote:
> On Monday, February 12, 2024 11:22 PM, Gerd Hoffmann wrote:
> > On Thu, Feb 01, 2024 at 10:38:43AM +0100, Gerd Hoffmann wrote:
> > >   Hi,
> > >
> > > > > 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.
> > >
> > > Hmm.  Sure this caused by this patch series?  For the PEI-less TDX
> > > build this series moves the MTRR setup to a different place in SEC.
> > > Once the DXE phase started the MTRR configuration should be identical
> > > with and without this patch series, and the series also doesn't touch
> > > any control register.
> > 
> > Ping.  Can you double-check please?  Our QE ran a test build with this series
> > applied through regression testing (including TDX) and has not found any
> > issues.
> 
> We double check the patch-set (v3) for both OvmfPkgX64 and IntelTdx.
> It triggered EXIT_REASON_CR_ACCESS in DXE phase when launching a
> td-guest.

Have you been able to figure which control register access caused the
EXIT_REASON_CR_ACCESS?

> @Gerd, what's the qemu command and test environment your QE
> run the case? We'd like run it in our side.

<quote>

Tested edk2-ovmf-20231122-1.el9.rhel21704.20240202.1130.noarch with TDX guest, no issue found

Version:

edk2-ovmf-20231122-1.el9.rhel21704.20240202.1130.noarch

guest kernel: 5.14.0-415.el9.x86_64

qemu-kvm-8.0.0-15.el9s.x86_64
host kernel-5.14.0-411.test.el9s.x86_64

Steps:

$ sudo /usr/libexec/qemu-kvm  -accel kvm   -drive file=/home/zixchen/rhel94_tdx.qcow2,if=none,id=virtio-disk0   -device virtio-blk-pci,drive=virtio-disk0   -cpu host -smp 16 -m 10240 -object tdx-guest,id=tdx,debug=on   -machine q35,hpet=off,kernel_irqchip=split,memory-encryption=tdx,confidential-guest-support=tdx,memory-backend=ram1   -object memory-backend-ram,id=ram1,size=10240M,private=on  -nographic -vga none   -nodefaults -bios /usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd  -serial stdio  -netdev user,id=user.0 -device e1000,netdev=user.0

$ dmesg|grep -i tdx
[    0.000000] tdx: Guest detected
[    0.719122] TECH PREVIEW: Intel Trusted Domain Extensions (TDX) may not be fully supported.
[    0.719122]  Intel TDX
[    0.719122] process: using TDX aware idle routine

</quote>

Host configuration with the tdx test packages:
https://sigs.centos.org/virt/tdx/host/

Latest edk2 build (stable202311 + patches) has the patch series
included:

https://kojihub.stream.centos.org/koji/buildinfo?buildID=56985

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115642): https://edk2.groups.io/g/devel/message/115642
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 3789 bytes --]

On Tue, 2024-02-20 at 09:15 +0100, Gerd Hoffmann wrote:
> On Tue, Feb 20, 2024 at 06:27:21AM +0000, Min Xu wrote:
> > On Monday, February 12, 2024 11:22 PM, Gerd Hoffmann wrote:
> > > On Thu, Feb 01, 2024 at 10:38:43AM +0100, Gerd Hoffmann wrote:
> > > >   Hi,
> > > > 
> > > > > > 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.
> > > > 
> > > > Hmm.  Sure this caused by this patch series?  For the PEI-less
> > > > TDX
> > > > build this series moves the MTRR setup to a different place in
> > > > SEC.
> > > > Once the DXE phase started the MTRR configuration should be
> > > > identical
> > > > with and without this patch series, and the series also doesn't
> > > > touch
> > > > any control register.
> > > 
> > > Ping.  Can you double-check please?  Our QE ran a test build with
> > > this series
> > > applied through regression testing (including TDX) and has not
> > > found any
> > > issues.
> > 
> > We double check the patch-set (v3) for both OvmfPkgX64 and
> > IntelTdx.
> > It triggered EXIT_REASON_CR_ACCESS in DXE phase when launching a
> > td-guest.
> 
> Have you been able to figure which control register access caused the
> EXIT_REASON_CR_ACCESS?
> 
> > @Gerd, what's the qemu command and test environment your QE
> > run the case? We'd like run it in our side.
> 
> <quote>
> 
> Tested edk2-ovmf-20231122-1.el9.rhel21704.20240202.1130.noarch with
> TDX guest, no issue found
> 
> Version:
> 
> edk2-ovmf-20231122-1.el9.rhel21704.20240202.1130.noarch
> 
> guest kernel: 5.14.0-415.el9.x86_64
> 
> qemu-kvm-8.0.0-15.el9s.x86_64
> host kernel-5.14.0-411.test.el9s.x86_64
> 
> Steps:
> 
> $ sudo /usr/libexec/qemu-kvm  -accel kvm   -drive
> file=/home/zixchen/rhel94_tdx.qcow2,if=none,id=virtio-disk0   -device
> virtio-blk-pci,drive=virtio-disk0   -cpu host -smp 16 -m 10240 -
> object tdx-guest,id=tdx,debug=on   -machine
> q35,hpet=off,kernel_irqchip=split,memory-encryption=tdx,confidential-
> guest-support=tdx,memory-backend=ram1   -object memory-backend-
> ram,id=ram1,size=10240M,private=on  -nographic -vga none   -
> nodefaults -bios /usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd  -
> serial stdio  -netdev user,id=user.0 -device e1000,netdev=user.0
> 
> $ dmesg|grep -i tdx
> [    0.000000] tdx: Guest detected
> [    0.719122] TECH PREVIEW: Intel Trusted Domain Extensions (TDX)
> may not be fully supported.
> [    0.719122]  Intel TDX
> [    0.719122] process: using TDX aware idle routine
> 
> </quote>
> 
> Host configuration with the tdx test packages:
> https://sigs.centos.org/virt/tdx/host/
> 
> Latest edk2 build (stable202311 + patches) has the patch series
> included:
> 
> https://kojihub.stream.centos.org/koji/buildinfo?buildID=56985
> 
> take care,
>   Gerd
> 
> 
> 
> 
> 
> 

Hi,

any progress on that patch?

I'm currently trying to passthrough the integrated GPU of an Intel
CPUs. When I add the GPU to the qemu command, I'm faced with the
descripted issue. This patch solves the issue.


-- 
Kind regards,
Corvin


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117617): https://edk2.groups.io/g/devel/message/117617
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Gerd Hoffmann]

  Hi,

> > > @Gerd, what's the qemu command and test environment your QE
> > > run the case? We'd like run it in our side.
> > 
> > <quote>
> > 
> > Tested edk2-ovmf-20231122-1.el9.rhel21704.20240202.1130.noarch with
> > TDX guest, no issue found
> > 
> > Version:
> > 
> > edk2-ovmf-20231122-1.el9.rhel21704.20240202.1130.noarch
> > 
> > guest kernel: 5.14.0-415.el9.x86_64
> > 
> > qemu-kvm-8.0.0-15.el9s.x86_64
> > host kernel-5.14.0-411.test.el9s.x86_64
> > 
> > Steps:
> > 
> > $ sudo /usr/libexec/qemu-kvm  -accel kvm   -drive
> > file=/home/zixchen/rhel94_tdx.qcow2,if=none,id=virtio-disk0   -device
> > virtio-blk-pci,drive=virtio-disk0   -cpu host -smp 16 -m 10240 -
> > object tdx-guest,id=tdx,debug=on   -machine
> > q35,hpet=off,kernel_irqchip=split,memory-encryption=tdx,confidential-
> > guest-support=tdx,memory-backend=ram1   -object memory-backend-
> > ram,id=ram1,size=10240M,private=on  -nographic -vga none   -
> > nodefaults -bios /usr/share/edk2/ovmf/OVMF.inteltdx.secboot.fd  -
> > serial stdio  -netdev user,id=user.0 -device e1000,netdev=user.0
> > 
> > $ dmesg|grep -i tdx
> > [    0.000000] tdx: Guest detected
> > [    0.719122] TECH PREVIEW: Intel Trusted Domain Extensions (TDX)
> > may not be fully supported.
> > [    0.719122]  Intel TDX
> > [    0.719122] process: using TDX aware idle routine
> > 
> > </quote>
> > 
> > Host configuration with the tdx test packages:
> > https://sigs.centos.org/virt/tdx/host/
> > 
> > Latest edk2 build (stable202311 + patches) has the patch series
> > included:
> > 
> > https://kojihub.stream.centos.org/koji/buildinfo?buildID=56985
> > 
> > take care,
> >   Gerd
> 
> Hi,
> 
> any progress on that patch?
> 
> I'm currently trying to passthrough the integrated GPU of an Intel
> CPUs. When I add the GPU to the qemu command, I'm faced with the
> descripted issue. This patch solves the issue.

On my end the state of affairs is unchanged.  Our builds have the patch
included and there are zero problems so far, the issue reported by Min
doesn't reproduce and it is still unclear what is going on.

Min, any update?

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117620): https://edk2.groups.io/g/devel/message/117620
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Min Xu]

On Thursday, April 11, 2024 4:13 PM, Gerd Hoffmann wrote:
> 
> On my end the state of affairs is unchanged.  Our builds have the patch
> included and there are zero problems so far, the issue reported by Min
> doesn't reproduce and it is still unclear what is going on.
> 
> Min, any update?
> 
Hi, Gerd & Corvin
We're investigating this issue and will update the status in the community. Thanks for your patience. 

Thanks
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117725): https://edk2.groups.io/g/devel/message/117725
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 6733 bytes --]

On Tue, 2024-01-30 at 14:04 +0100, 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);
>  
>      //

This has to be changed for OvmfPkg/Bhyve/PlatformPei/MemDetect.c too.

> 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.
>    //

-- 
Kind regards,
Corvin


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119114): https://edk2.groups.io/g/devel/message/119114
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Gerd Hoffmann]

  Hi,

> > -    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);
> >  
> >      //
> 
> This has to be changed for OvmfPkg/Bhyve/PlatformPei/MemDetect.c too.

I have a draft PR open with this fixed:
	https://github.com/tianocore/edk2/pull/5696

Can you check this works for bhyve?

thanks,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119383): https://edk2.groups.io/g/devel/message/119383
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 1207 bytes --]

On Thu, 2024-05-30 at 11:03 +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > -    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);
> > >  
> > >      //
> > 
> > This has to be changed for OvmfPkg/Bhyve/PlatformPei/MemDetect.c
> > too.
> 
> I have a draft PR open with this fixed:
> 	https://github.com/tianocore/edk2/pull/5696
> 
> Can you check this works for bhyve?
> 
> thanks,
>   Gerd
> 

Works. Thanks!


-- 
Kind regards,
Corvin


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119423): https://edk2.groups.io/g/devel/message/119423
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Gerd Hoffmann]

  Hi,

> > I have a draft PR open with this fixed:
> > 	https://github.com/tianocore/edk2/pull/5696
> > 
> > Can you check this works for bhyve?
> 
> Works. Thanks!

Thanks for testing, flipped PR to 'Ready'.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#119425): https://edk2.groups.io/g/devel/message/119425
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]
-=-=-=-=-=-=-=-=-=-=-=-