BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3902
Bad IO performance in SEC phase is observed after TDX features was
introduced. (after commit b6b2de884864 - "MdePkg: Support mmio for
Tdx guest in BaseIoLibIntrinsic").
This is because IsTdxGuest() will be called in each MMIO operation.
It is trying to cache the result of the probe in the efi data segment.
However, that doesn't work in SEC, because the data segment is read only
(so the write seems to succeed but a read will always return the
original value), leading to us calling TdIsEnabled() check for every
mmio we do, which is causing the slowdown because it's very expensive.
This patch is to call CcProbe instead of TdIsEnabled in IsTdxGuest.
Null instance of CcProbe always returns CCGuestTypeNonEncrypted. Its
OvmfPkg version returns the guest type in Ovmf work area.
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
.../BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf | 1 +
.../Library/BaseIoLibIntrinsic/IoLibInternalTdx.c | 13 ++-----------
2 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
index 7fe1c60f046e..e1b8298ac451 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
+++ b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
@@ -55,6 +55,7 @@
DebugLib
BaseLib
RegisterFilterLib
+ CcProbeLib
[LibraryClasses.X64]
TdxLib
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdx.c b/MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdx.c
index 1e539dbfbbad..8af6fc35c591 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdx.c
+++ b/MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdx.c
@@ -10,6 +10,7 @@
#include <Include/IndustryStandard/Tdx.h>
#include <Library/TdxLib.h>
#include <Register/Intel/Cpuid.h>
+#include <Library/CcProbeLib.h>
#include "IoLibTdx.h"
// Size of TDVMCALL Access, including IO and MMIO
@@ -22,9 +23,6 @@
#define TDVMCALL_ACCESS_READ 0
#define TDVMCALL_ACCESS_WRITE 1
-BOOLEAN mTdxEnabled = FALSE;
-BOOLEAN mTdxProbed = FALSE;
-
/**
Check if it is Tdx guest.
@@ -38,14 +36,7 @@ IsTdxGuest (
VOID
)
{
- if (mTdxProbed) {
- return mTdxEnabled;
- }
-
- mTdxEnabled = TdIsEnabled ();
- mTdxProbed = TRUE;
-
- return mTdxEnabled;
+ return CcProbe () == CCGuestTypeIntelTdx;
}
/**
--
2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88921): https://edk2.groups.io/g/devel/message/88921
Mute This Topic: https://groups.io/mt/90477280/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-On Fri, Apr 15, 2022 at 08:07:08AM +0800, Min Xu wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3902
>
> Bad IO performance in SEC phase is observed after TDX features was
> introduced. (after commit b6b2de884864 - "MdePkg: Support mmio for
> Tdx guest in BaseIoLibIntrinsic").
>
> This is because IsTdxGuest() will be called in each MMIO operation.
> It is trying to cache the result of the probe in the efi data segment.
> However, that doesn't work in SEC, because the data segment is read only
> (so the write seems to succeed but a read will always return the
> original value), leading to us calling TdIsEnabled() check for every
> mmio we do, which is causing the slowdown because it's very expensive.
>
> This patch is to call CcProbe instead of TdIsEnabled in IsTdxGuest.
> Null instance of CcProbe always returns CCGuestTypeNonEncrypted. Its
> OvmfPkg version returns the guest type in Ovmf work area.
Hi!
I ran through our tests on stable-202205-rc1, and I'm finding that all
of the tests using 2M FD_SIZE & SMM_REQUIRE=TRUE are failing with
QEMU hanging w/o output. Equivalent tests w/ 4M FD_SIZE are working
fine. I bisected it down to this commit, and also confirmed that
reverting this commit on top of 202205-rc1 also avoids the problem.
I might have a chance to debug more tomorrow, but for now I just
wanted to flag it.
-dann
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
> .../BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf | 1 +
> .../Library/BaseIoLibIntrinsic/IoLibInternalTdx.c | 13 ++-----------
> 2 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
> index 7fe1c60f046e..e1b8298ac451 100644
> --- a/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
> +++ b/MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsicSev.inf
> @@ -55,6 +55,7 @@
> DebugLib
> BaseLib
> RegisterFilterLib
> + CcProbeLib
>
> [LibraryClasses.X64]
> TdxLib
> diff --git a/MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdx.c b/MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdx.c
> index 1e539dbfbbad..8af6fc35c591 100644
> --- a/MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdx.c
> +++ b/MdePkg/Library/BaseIoLibIntrinsic/IoLibInternalTdx.c
> @@ -10,6 +10,7 @@
> #include <Include/IndustryStandard/Tdx.h>
> #include <Library/TdxLib.h>
> #include <Register/Intel/Cpuid.h>
> +#include <Library/CcProbeLib.h>
> #include "IoLibTdx.h"
>
> // Size of TDVMCALL Access, including IO and MMIO
> @@ -22,9 +23,6 @@
> #define TDVMCALL_ACCESS_READ 0
> #define TDVMCALL_ACCESS_WRITE 1
>
> -BOOLEAN mTdxEnabled = FALSE;
> -BOOLEAN mTdxProbed = FALSE;
> -
> /**
> Check if it is Tdx guest.
>
> @@ -38,14 +36,7 @@ IsTdxGuest (
> VOID
> )
> {
> - if (mTdxProbed) {
> - return mTdxEnabled;
> - }
> -
> - mTdxEnabled = TdIsEnabled ();
> - mTdxProbed = TRUE;
> -
> - return mTdxEnabled;
> + return CcProbe () == CCGuestTypeIntelTdx;
> }
>
> /**
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89782): https://edk2.groups.io/g/devel/message/89782
Mute This Topic: https://groups.io/mt/90477280/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On May 17, 2022 6:22 AM, dann frazier wrote: > I ran through our tests on stable-202205-rc1, and I'm finding that all of the > tests using 2M FD_SIZE & SMM_REQUIRE=TRUE are failing with QEMU > hanging w/o output. Equivalent tests w/ 4M FD_SIZE are working fine. I > bisected it down to this commit, and also confirmed that reverting this > commit on top of 202205-rc1 also avoids the problem. > > I might have a chance to debug more tomorrow, but for now I just wanted to > flag it. This patch calls CcProbe () to get the Confidential Computing guest type. There are 2 versions of CcProbeLib, one is to get the Cc guest type from PcdOvmfWorkArea, the other is a null instance and it always return CcGuestTypeNonEncrypted (which means it is a legacy vm guest). Only OvmfPkgX64.dsc and IntelTdxX64.dsc include the first one (which probe the PcdOvmfWorkArea). If this patch is reverted, it means it is to check the guest type by calling CPUID, not reading the PcdOvmfWorkArea. Can you share your build command and qemu command so that I can try it in my side? Thanks Min -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89787): https://edk2.groups.io/g/devel/message/89787 Mute This Topic: https://groups.io/mt/90477280/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On May 17, 2022 9:15 AM, Xu Min wrote: > On May 17, 2022 6:22 AM, dann frazier wrote: > > I ran through our tests on stable-202205-rc1, and I'm finding that all > > of the tests using 2M FD_SIZE & SMM_REQUIRE=TRUE are failing with > QEMU > > hanging w/o output. Equivalent tests w/ 4M FD_SIZE are working fine. I > > bisected it down to this commit, and also confirmed that reverting > > this commit on top of 202205-rc1 also avoids the problem. > > > > I might have a chance to debug more tomorrow, but for now I just > > wanted to flag it. > This patch calls CcProbe () to get the Confidential Computing guest type. > There are 2 versions of CcProbeLib, one is to get the Cc guest type from > PcdOvmfWorkArea, the other is a null instance and it always return > CcGuestTypeNonEncrypted (which means it is a legacy vm guest). Only > OvmfPkgX64.dsc and IntelTdxX64.dsc include the first one (which probe the > PcdOvmfWorkArea). > > If this patch is reverted, it means it is to check the guest type by calling CPUID, > not reading the PcdOvmfWorkArea. > More investigation shows that the root cause is the wrong memory access in SMM driver (PiSmmCpuDxeSmm.inf). This issue can be triggered when SMM_REQUIRE is TRUE. IoLib is used in PiSmmCpuDxeSmm.inf. In OvmfPkgX64 BaseIoLibIntrinsicSev.inf is included and it probes if the working guest is td guest by CcProbe(). CcProbe reads PcdOvmfWorkArea (0x80B000) to get the guest type. It works in Non-SMM mode. But in SMM mode it is illegal. So reverting the patch makes the probe to call CPUID (0x21) instead of reading PcdOvmfWorkArea. It does work. I am thinking how to fix this issue and then send out the patch-set for review. Thanks Min -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89804): https://edk2.groups.io/g/devel/message/89804 Mute This Topic: https://groups.io/mt/90477280/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.