Re: [PATCH V3 0/7] Introduce CcProbe in MdePkg

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 4/16/22 22:01, 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.
> 
> CcProbe is introduced in this patch-set. It is called in
> BaseIoLibIntrinsicSev instead of IsTdxGuest. There are 2 versions of
> the CcProbeLib. Null instance of CcProbe always returns
> CCGuestTypeNonEncrypted. Its OvmfPkg version checks the Ovmf work area
> and returns the CC guest type.
> 
> In this patch-set another issue is fixed with CcProbe as well. If the
> working guest is SEV and in the beginning of SecMain.c TdIsEnabled()
> was called. At this point, exception handling is not established and
> a CPUID instruction will generate a #VC and cause the booting SEV guest
> to crash. Patch #7 is to fix this broken.
> 
> Code is at: https://github.com/mxu9/edk2/tree/cc_probe.v3
> 
> v3 changes:
>   - Fix the broken issue in SEV guest at SecMain.c. Please refer to
>     Patch #7.
> 
> v2 changes:
>   - Rename TdProbe to CcProbe to make the lib work for Confidential
>     Computing guests.
>   - Rename the GUEST_TYPE to CC_GUEST_TYPE and move it from
>     WorkArea.h@OvmfPkg to ConfidentialComputingGuestAttr.h@MdePkg.
>     This is because CcProbeLib is designed to return the CC Guest
>     type and the lib is located at MdePkg.
>   - Rename the CC_GUEST_TYPE's fields name to Camel style. See the
>     commit message in patch #1.
> 
> 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: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>

After working around the PCI library issue (for which Min will be 
submitting a patch), this series boots successfully for SEV, SEV-ES and 
SEV-SNP when built as X64. I documented the issue that SEV has with 
Ia32X64 in patch 5/7 and I'll have to decide what to do there. So...

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> 
> Min Xu (7):
>    MdePkg: Add CC_GUEST_TYPE in ConfidentialComputingGuestAttr.h
>    OvmfPkg: Replace GUEST_TYPE with CC_GUEST_TYPE
>    MdePkg: Add CcProbeLib
>    OvmfPkg: Add CcProbeLib
>    OvmfPkg: Add CcProbeLib in *.dsc
>    MdePkg: Probe Cc guest in BaseIoLibIntrinsicSev
>    OvmfPkg: Call CcProbe in SecMain.c instead of TsIsEnabled
> 
>   .../Include/ConfidentialComputingGuestAttr.h  | 11 ++++++-
>   MdePkg/Include/Library/CcProbeLib.h           | 26 ++++++++++++++++
>   .../BaseIoLibIntrinsicSev.inf                 |  1 +
>   .../BaseIoLibIntrinsic/IoLibInternalTdx.c     | 13 ++------
>   .../Library/CcProbeLibNull/CcProbeLibNull.c   | 26 ++++++++++++++++
>   .../Library/CcProbeLibNull/CcProbeLibNull.inf | 21 +++++++++++++
>   MdePkg/MdePkg.dec                             |  5 +++
>   MdePkg/MdePkg.dsc                             |  1 +
>   OvmfPkg/AmdSev/AmdSevX64.dsc                  |  1 +
>   OvmfPkg/Bhyve/BhyveX64.dsc                    |  1 +
>   OvmfPkg/CloudHv/CloudHvX64.dsc                |  1 +
>   OvmfPkg/Include/WorkArea.h                    |  9 +-----
>   OvmfPkg/IntelTdx/IntelTdxX64.dsc              |  1 +
>   OvmfPkg/IntelTdx/Sec/SecMain.c                |  6 ++--
>   OvmfPkg/IntelTdx/Sec/SecMain.inf              |  1 +
>   .../PeiMemEncryptSevLibInternal.c             |  2 +-
>   .../SecMemEncryptSevLibInternal.c             |  2 +-
>   OvmfPkg/Library/CcProbeLib/CcProbeLib.c       | 31 +++++++++++++++++++
>   OvmfPkg/Library/CcProbeLib/CcProbeLib.inf     | 25 +++++++++++++++
>   OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c   |  2 +-
>   OvmfPkg/Microvm/MicrovmX64.dsc                |  1 +
>   OvmfPkg/OvmfPkgIa32.dsc                       |  1 +
>   OvmfPkg/OvmfPkgIa32X64.dsc                    |  1 +
>   OvmfPkg/OvmfPkgX64.dsc                        |  1 +
>   OvmfPkg/OvmfXen.dsc                           |  1 +
>   OvmfPkg/Sec/AmdSev.c                          |  2 +-
>   OvmfPkg/Sec/SecMain.c                         |  5 +--
>   OvmfPkg/Sec/SecMain.inf                       |  1 +
>   28 files changed, 170 insertions(+), 29 deletions(-)
>   create mode 100644 MdePkg/Include/Library/CcProbeLib.h
>   create mode 100644 MdePkg/Library/CcProbeLibNull/CcProbeLibNull.c
>   create mode 100644 MdePkg/Library/CcProbeLibNull/CcProbeLibNull.inf
>   create mode 100644 OvmfPkg/Library/CcProbeLib/CcProbeLib.c
>   create mode 100644 OvmfPkg/Library/CcProbeLib/CcProbeLib.inf
>