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.

TdProbe is introduced in this patch-set. It is called in
BaseIoLibIntrinsicSev instead of IsTdxGuest. There are 2 versions of the
TdProbeLib. Null instance of TdProbe always returns TD_PROBE_NON. Its
OvmfPkg version checks the Ovmf work area to determine the Td guest type.

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

Min Xu (4):
  MdePkg: Add TdProbeLib
  OvmfPkg/IntelTdx: Add TdProbeLib
  MdePkg: Probe Td guest in BaseIoLibIntrinsicSev
  OvmfPkg: Add TdProbeLib in *.dsc

 MdePkg/Include/Library/TdProbeLib.h           | 26 +++++++++++++
 .../BaseIoLibIntrinsicSev.inf                 |  1 +
 .../BaseIoLibIntrinsic/IoLibInternalTdx.c     | 13 +------
 .../Library/TdProbeLibNull/TdProbeLibNull.c   | 25 ++++++++++++
 .../Library/TdProbeLibNull/TdProbeLibNull.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/IntelTdx/IntelTdxX64.dsc              |  1 +
 OvmfPkg/IntelTdx/TdProbeLib/TdProbeLib.c      | 38 +++++++++++++++++++
 OvmfPkg/IntelTdx/TdProbeLib/TdProbeLib.inf    | 25 ++++++++++++
 OvmfPkg/Microvm/MicrovmX64.dsc                |  1 +
 OvmfPkg/OvmfPkgIa32.dsc                       |  1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                    |  1 +
 OvmfPkg/OvmfPkgX64.dsc                        |  1 +
 OvmfPkg/OvmfXen.dsc                           |  1 +
 18 files changed, 153 insertions(+), 11 deletions(-)
 create mode 100644 MdePkg/Include/Library/TdProbeLib.h
 create mode 100644 MdePkg/Library/TdProbeLibNull/TdProbeLibNull.c
 create mode 100644 MdePkg/Library/TdProbeLibNull/TdProbeLibNull.inf
 create mode 100644 OvmfPkg/IntelTdx/TdProbeLib/TdProbeLib.c
 create mode 100644 OvmfPkg/IntelTdx/TdProbeLib/TdProbeLib.inf

-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88861): https://edk2.groups.io/g/devel/message/88861
Mute This Topic: https://groups.io/mt/90436749/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Wed, 2022-04-13 at 17:08 +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.
> 
> TdProbe is introduced in this patch-set. It is called in
> BaseIoLibIntrinsicSev instead of IsTdxGuest. There are 2 versions of
> the TdProbeLib. Null instance of TdProbe always returns TD_PROBE_NON.
> Its OvmfPkg version checks the Ovmf work area to determine the Td
> guest type.

I tested this out with the TPM code: it restores pretty much all of the
lost performance, thanks!

Tested-by: James Bottomley <jejb@linux.ibm.com>

James




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88867): https://edk2.groups.io/g/devel/message/88867
Mute This Topic: https://groups.io/mt/90436749/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Thank you very much, James and Min, for the quick response.

I have one small concern on the naming - TdProbeLib.
Would it be better if we create a generic CcProbeLib, and just return the CC_GUEST_TYPE?
The benefit is that just in case SEV has some usage in the future, we don’t need create SevProbeLib.

BTW: I also propose to change name BaseIoLibIntrinsicSev.inf to BaseIoLibIntrinsicCc.inf.
It is very confusing to me, that a Sev.inf include a TdProbeLib. :-(
That can be done in a separate patch.

Thank you
Yao Jiewen


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of James
> Bottomley
> Sent: Wednesday, April 13, 2022 8:55 PM
> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Liu, Zhiguang <zhiguang.liu@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Brijesh
> Singh <brijesh.singh@amd.com>; Aktas, Erdem <erdemaktas@google.com>;
> Tom Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH 0/4] Introduce TdProbe in MdePkg
> 
> On Wed, 2022-04-13 at 17:08 +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.
> >
> > TdProbe is introduced in this patch-set. It is called in
> > BaseIoLibIntrinsicSev instead of IsTdxGuest. There are 2 versions of
> > the TdProbeLib. Null instance of TdProbe always returns TD_PROBE_NON.
> > Its OvmfPkg version checks the Ovmf work area to determine the Td
> > guest type.
> 
> I tested this out with the TPM code: it restores pretty much all of the
> lost performance, thanks!
> 
> Tested-by: James Bottomley <jejb@linux.ibm.com>
> 
> James
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88868): https://edk2.groups.io/g/devel/message/88868
Mute This Topic: https://groups.io/mt/90436749/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On April 13, 2022 9:12 PM, Yao Jiewen wrote:
> 
> Thank you very much, James and Min, for the quick response.
> 
> I have one small concern on the naming - TdProbeLib.
> Would it be better if we create a generic CcProbeLib, and just return the
> CC_GUEST_TYPE?
> The benefit is that just in case SEV has some usage in the future, we don’t
> need create SevProbeLib.
>
Thanks for the suggestion. It will be renamed to CcProbeLib in the next version.
> 
> BTW: I also propose to change name BaseIoLibIntrinsicSev.inf to
> BaseIoLibIntrinsicCc.inf.
> It is very confusing to me, that a Sev.inf include a TdProbeLib. :-( That can be
> done in a separate patch.
Agree. I will create a separate patch-set to rename the lib.
> 

Thanks
Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#88869): https://edk2.groups.io/g/devel/message/88869
Mute This Topic: https://groups.io/mt/90436749/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-