Re: [edk2-devel] [PATCH V2 5/6] MdePkg: Probe Cc guest in BaseIoLibIntrinsicSev

["dann frazier" <dann.frazier@canonical.com>]

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;
>  }
>  
>  /**