Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cc probe.v4 #2788

Closed
wants to merge 7 commits into from
Closed

Cc probe.v4 #2788

wants to merge 7 commits into from

Conversation

mxu9
Copy link
Contributor

@mxu9 mxu9 commented Apr 18, 2022

No description provided.

@mxu9
MdePkg: Add CC_GUEST_TYPE in ConfidentialComputingGuestAttr.h
95d714b 
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3902

The confidential computing guest type (GUEST_TYPE) was defined in
OvmfPkg/Include/WorkArea.h. Now it is to be moved to
MdePkg/Include/ConfidentialComputingGuestAttr.h and renamed as
CC_GUEST_TYPE.

There are 2 reasons for this change.
1. CC_GUEST_TYPE is a generic definition and will be used in CcProbeLib
   which is defined in MdePkg.
2. Based on the latest edk2 coding style:
 - First character should be upper case
 - Must contain lower case characters
 - No white space characters
 - Global variable name must start with a 'g'

As the first step CC_GUEST_TYPE is defined in this patch. In the
next patch GUEST_TYPE will be deleted. This is to make sure the
bisect work correctly.

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>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
@mxu9
OvmfPkg: Replace GUEST_TYPE with CC_GUEST_TYPE
d94a8bb 
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3902

Replace GUEST_TYPE with CC_GUEST_TYPE which is defined in
MdePkg/Include/ConfidentialComputingGuestAttr.h.

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>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
@mxu9
MdePkg: Add CcProbeLibNull
f1b660e 
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3902

CcProbeLib is used to probe the Confidential Computing guest type.
This library is designed to run on SEC / PEI / DXE phases. A null
instance of the library always returns CCGuestTypeNonEncrypted.
A platform specific CcProbeLib will be implemented, for example,
in OvmfPkg.

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>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
@mxu9
OvmfPkg: Add CcProbeLib
9914f8d 
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3902

This is the OvmfPkg specific CcProbeLib. It checks the Ovmf WorkArea
(PcdOvmfWorkAreaBase) to return the 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>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
@mxu9
OvmfPkg: Add CcProbeLib in *.dsc
3c3f8a3 
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3902

CcProbeLib is imported in BaseIoLibIntrinsicSev.
OvmfPkg/Library/CcProbeLib is the OvmfPkg version which checks
OvmfWorkArea to return the Cc guest type. It is included
in OvmfPkgX64.dsc and IntelTdx/IntelTdxX64.dsc.

Other .dsc include the MdePkg/Library/CcProbeLibNull because Cc guest
is not supported in those projects.

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>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
@mxu9
MdePkg: Probe Cc guest in BaseIoLibIntrinsicSev
afcb27f 
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 b6b2de8 - "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>
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
@mxu9
OvmfPkg: Call CcProbe in SecMain.c instead of TsIsEnabled
969e3cc 
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3902

TdIsEnabled() uses the CPUID instruction. At this point, exception
handling is not established and a CPUID instruction will generate
a #VC and cause the booting guest to crash.

CcProbe() checks Ovmf work area to return the guest type. So call
of CcProbe() instead of TdIsEnabled() to fix the above issue.

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>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
@mxu9
Copy link
Contributor Author

mxu9 commented Apr 19, 2022

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 b6b2de8 - "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.v4

v4 changes:

  • Rename the field name in CC_GUEST_TYPE from CCXxx to CcXxx based
    on the community feedback.

v3 changes:

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 Xen ACPI tables support in OVMF #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

Min Xu (7):
MdePkg: Add CC_GUEST_TYPE in ConfidentialComputingGuestAttr.h
OvmfPkg: Replace GUEST_TYPE with CC_GUEST_TYPE
MdePkg: Add CcProbeLibNull
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

@mergify
Copy link

mergify bot commented May 9, 2022

PR can not be merged due to conflict. Please rebase and resubmit

1 similar comment
@mergify
Copy link

mergify bot commented Sep 23, 2022

PR can not be merged due to conflict. Please rebase and resubmit

@lersek lersek closed this Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mxu9 @lersek