[PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler: Add base support for the #VE exception #ve


Min Xu
 

RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

Add base support to handle #VE exceptions. Update the common exception
handlers to invoke the VmTdExitHandleVe () function of the VmTdExitLib
library when a #VE is encountered. A non-zero return code will propagate
to the targeted exception handler.

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
.../DxeCpuExceptionHandlerLib.inf | 1 +
.../PeiCpuExceptionHandlerLib.inf | 1 +
.../PeiDxeSmmCpuException.c | 18 ++++++++++++++++++
.../SecPeiCpuException.c | 19 +++++++++++++++++++
.../SecPeiCpuExceptionHandlerLib.inf | 1 +
.../SmmCpuExceptionHandlerLib.inf | 1 +
.../Xcode5SecPeiCpuExceptionHandlerLib.inf | 1 +
7 files changed, 42 insertions(+)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
index e7a81bebdb13..630a83bf003b 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
@@ -61,3 +61,4 @@
MemoryAllocationLib
DebugLib
VmgExitLib
+ VmTdExitLib
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
index cf5bfe40832b..63a7abfb6242 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
@@ -53,6 +53,7 @@
MemoryAllocationLib
SynchronizationLib
VmgExitLib
+ VmTdExitLib

[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard # CONSUMES
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c
index 892d349d4b37..0976a880825b 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiDxeSmmCpuException.c
@@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

#include <Library/DebugLib.h>
#include <Library/VmgExitLib.h>
+#include <Library/VmTdExitLib.h>
#include "CpuExceptionCommon.h"

/**
@@ -45,6 +46,23 @@ CommonExceptionHandlerWorker (
}
}

+ if (ExceptionType == VE_EXCEPTION) {
+ EFI_STATUS Status;
+ //
+ // #VE needs to be handled immediately upon enabling exception handling
+ // and therefore can't use the RegisterCpuInterruptHandler() interface.
+ //
+ // Handle the #VE:
+ // On EFI_SUCCESS - Exception has been handled, return
+ // On other - ExceptionType contains (possibly new) exception
+ // value
+ //
+ Status = VmTdExitHandleVe (&ExceptionType, SystemContext);
+ if (!EFI_ERROR (Status)) {
+ return;
+ }
+ }
+
ExceptionHandlerContext = (EXCEPTION_HANDLER_CONTEXT *) (UINTN) (SystemContext.SystemContextIa32);
ReservedVectors = ExceptionHandlerData->ReservedVectors;
ExternalInterruptHandler = ExceptionHandlerData->ExternalInterruptHandler;
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c
index 01b5a2f1f4fc..173047a6b494 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuException.c
@@ -8,6 +8,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

#include <PiPei.h>
#include <Library/VmgExitLib.h>
+#include <Library/VmTdExitLib.h>
#include "CpuExceptionCommon.h"

CONST UINTN mDoFarReturnFlag = 0;
@@ -43,6 +44,24 @@ CommonExceptionHandler (
}
}

+ if (ExceptionType == VE_EXCEPTION) {
+ EFI_STATUS Status;
+ //
+ // #VE needs to be handled immediately upon enabling exception handling
+ // and therefore can't use the RegisterCpuInterruptHandler() interface
+ // (which isn't supported under Sec and Pei anyway).
+ //
+ // Handle the #VE:
+ // On EFI_SUCCESS - Exception has been handled, return
+ // On other - ExceptionType contains (possibly new) exception
+ // value
+ //
+ Status = VmTdExitHandleVe (&ExceptionType, SystemContext);
+ if (!EFI_ERROR (Status)) {
+ return;
+ }
+ }
+
//
// Initialize the serial port before dumping.
//
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
index 8ae4feae6238..4aeab2057b08 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
@@ -49,6 +49,7 @@
LocalApicLib
PeCoffGetEntryPointLib
VmgExitLib
+ VmTdExitLib

[FeaturePcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard ## CONSUMES
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
index c9f20da05860..2622e48103f3 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
@@ -52,6 +52,7 @@
PeCoffGetEntryPointLib
DebugLib
VmgExitLib
+ VmTdExitLib

[FeaturePcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard ## CONSUMES
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
index a15f125d5b5e..36ccb7ef97ec 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
@@ -54,6 +54,7 @@
LocalApicLib
PeCoffGetEntryPointLib
VmgExitLib
+ VmTdExitLib

[FeaturePcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard ## CONSUMES
--
2.29.2.windows.2


Gerd Hoffmann
 

Hi,

+ if (ExceptionType == VE_EXCEPTION) {
+ EFI_STATUS Status;
+ //
+ // #VE needs to be handled immediately upon enabling exception handling
+ // and therefore can't use the RegisterCpuInterruptHandler() interface.
Can please you explain in more detail why this is the case?

thanks,
Gerd


Min Xu
 

On October 12, 2021 6:27 PM, Gerd Hoffmann wrote:
+ if (ExceptionType == VE_EXCEPTION) {
+ EFI_STATUS Status;
+ //
+ // #VE needs to be handled immediately upon enabling exception handling
+ // and therefore can't use the RegisterCpuInterruptHandler() interface.
Can please you explain in more detail why this is the case?
VE Exception may happen before a component registers exception. So it has to be implemented inside the exception lib.

Thanks.
Min


Gerd Hoffmann
 

On Tue, Oct 26, 2021 at 05:06:21AM +0000, Xu, Min M wrote:
On October 12, 2021 6:27 PM, Gerd Hoffmann wrote:
+ if (ExceptionType == VE_EXCEPTION) {
+ EFI_STATUS Status;
+ //
+ // #VE needs to be handled immediately upon enabling exception handling
+ // and therefore can't use the RegisterCpuInterruptHandler() interface.
Can please you explain in more detail why this is the case?
VE Exception may happen before a component registers exception.

So it has to be implemented inside the exception lib.
Well, no, you can also change the code to avoid triggering an exception.

Adding a new lib for the exception means the lib must be added into each
and every *.dsc file (either the tdx impl or the null variant), not only
in the tianocore core itself but also all projects depending on tianocore.

So IMHO it is worth checking out how much effort it would be to avoid
early (before component registration) exceptions.

Which early exception do actually happen?

take care,
Gerd


Min Xu
 

On October 26, 2021 2:12 PM, Gerd Hoffmann wrote:
On Tue, Oct 26, 2021 at 05:06:21AM +0000, Xu, Min M wrote:
On October 12, 2021 6:27 PM, Gerd Hoffmann wrote:
+ if (ExceptionType == VE_EXCEPTION) {
+ EFI_STATUS Status;
+ //
+ // #VE needs to be handled immediately upon enabling exception
handling
+ // and therefore can't use the RegisterCpuInterruptHandler()
interface.

Can please you explain in more detail why this is the case?
VE Exception may happen before a component registers exception.

So it has to be implemented inside the exception lib.
Well, no, you can also change the code to avoid triggering an exception.

Adding a new lib for the exception means the lib must be added into each
and every *.dsc file (either the tdx impl or the null variant), not only in the
tianocore core itself but also all projects depending on tianocore.

So IMHO it is worth checking out how much effort it would be to avoid early
(before component registration) exceptions.

Which early exception do actually happen?
RegisterCpuInterfaceHandler() is not supported in SEC/PEI phase. But there are still some scenarios in SEC/PEI which will trigger #VE.
CPUID is the sample. See below call chain in CpuMpPei.
InitializeCpuMpWorker --> CollectBitsDataFromPpi --> MpInitLibGetProcessorInfo --> GetProcessorLocationByApicId()

Actually #VE handler follows the same way as #VC handler (by SEV). See discussions in below link.
https://edk2.groups.io/g/devel/topic/73201885

Thanks
Min


Gerd Hoffmann
 

Hi,

So it has to be implemented inside the exception lib.
Well, no, you can also change the code to avoid triggering an exception.

Adding a new lib for the exception means the lib must be added into each
and every *.dsc file (either the tdx impl or the null variant), not only in the
tianocore core itself but also all projects depending on tianocore.

So IMHO it is worth checking out how much effort it would be to avoid early
(before component registration) exceptions.

Which early exception do actually happen?
RegisterCpuInterfaceHandler() is not supported in SEC/PEI phase. But there are still some scenarios in SEC/PEI which will trigger #VE.
CPUID is the sample. See below call chain in CpuMpPei.
InitializeCpuMpWorker --> CollectBitsDataFromPpi --> MpInitLibGetProcessorInfo --> GetProcessorLocationByApicId()
Bad example ;)

TDX needs its own Mp implementations anyway, so that
one specifically should be quite easy to avoid.

Actually #VE handler follows the same way as #VC handler (by SEV). See discussions in below link.
https://edk2.groups.io/g/devel/topic/73201885
I guess the list of instructions which trap on tdx is quite simliar
to sev? cpuid, msr access, io instructions?

I suspect there isn't an easy way around that then (as discussed at
length in the email thread linked, thanks for that).

How about adding the tdx exception handler to the existing library, so
we don't have the churn of adding a new library everywhere *again*?

take care,
Gerd


Min Xu
 

On October 26, 2021 6:25 PM, Gerd Hoffmann wrote:
Hi,

So it has to be implemented inside the exception lib.
Well, no, you can also change the code to avoid triggering an exception.

Adding a new lib for the exception means the lib must be added into
each and every *.dsc file (either the tdx impl or the null variant),
not only in the tianocore core itself but also all projects depending on
tianocore.

So IMHO it is worth checking out how much effort it would be to
avoid early (before component registration) exceptions.

Which early exception do actually happen?
RegisterCpuInterfaceHandler() is not supported in SEC/PEI phase. But there
are still some scenarios in SEC/PEI which will trigger #VE.
CPUID is the sample. See below call chain in CpuMpPei.
InitializeCpuMpWorker --> CollectBitsDataFromPpi -->
MpInitLibGetProcessorInfo --> GetProcessorLocationByApicId()
Bad example ;)

TDX needs its own Mp implementations anyway, so that one specifically should
be quite easy to avoid.

Actually #VE handler follows the same way as #VC handler (by SEV). See
discussions in below link.
https://edk2.groups.io/g/devel/topic/73201885
I guess the list of instructions which trap on tdx is quite simliar to sev? cpuid,
msr access, io instructions?

I suspect there isn't an easy way around that then (as discussed at length in the
email thread linked, thanks for that).

How about adding the tdx exception handler to the existing library, so we don't
have the churn of adding a new library everywhere *again*?
Do you mean add the VmTdExitVeHandler.c/VmTdExitLibNull.c in CpuExceptionHandlerLib, then include the corresponding source file in each *CpuExceptionHandlerLib.inf?
If this is the case, then the implementation of #VE handler (TDX) will be in-consistent with #VC handler (SEV).
Shall we keep these 2 implementation consistent? Or will SEV agree to update the #VC handler in the same way?

Thanks
Min


Gerd Hoffmann
 

Hi,

How about adding the tdx exception handler to the existing library, so we don't
have the churn of adding a new library everywhere *again*?
Do you mean add the VmTdExitVeHandler.c/VmTdExitLibNull.c in CpuExceptionHandlerLib, then include the corresponding source file in each *CpuExceptionHandlerLib.inf?
No, I mean extend the existing VmgExitLib instead of adding a new
VmTdExitLib, i.e. place the tdx handler in
OvmfPkg/Library/VmgExitLib/TdxExitHandler.c

take care,
Gerd


Yao, Jiewen
 

Hi Gerd
I tend to agree with you on the direction to use one TEE specific Exception lib.

However, I have naming concern.
The VMG is very SEV specific term. I don't believe it is a right name to cover the TEE exception lib.

If Brijesh agree to merge, I think we should rename it to a neutral name, such as TeeExitLib.

What do you think, Brijesh?

Thank you
Yao Jiewen

-----Original Message-----
From: kraxel@redhat.com <kraxel@redhat.com>
Sent: Wednesday, October 27, 2021 3:20 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
<jiewen.yao@intel.com>; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric <eric.dong@intel.com>; Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

Hi,

How about adding the tdx exception handler to the existing library, so we
don't
have the churn of adding a new library everywhere *again*?
Do you mean add the VmTdExitVeHandler.c/VmTdExitLibNull.c in
CpuExceptionHandlerLib, then include the corresponding source file in each
*CpuExceptionHandlerLib.inf?

No, I mean extend the existing VmgExitLib instead of adding a new
VmTdExitLib, i.e. place the tdx handler in
OvmfPkg/Library/VmgExitLib/TdxExitHandler.c

take care,
Gerd


Yao, Jiewen
 

Besides VmgExitLib -
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Include/Library/VmgExitLib.h

We have another potential issue - MemEncryptSevLib -
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/Library/MemEncryptSevLib.h
We might need rename it to MemEncryptionTeeLib.

I think we need setup direction on how to hand those cases in a consistent way.

Option 1: Keep using current name: SEV and TDX as two class name. Add two instances.

Option 2: Define a new architecture neutral class name such as TEE. Add one instance to cover both SEV and TDX.

Thought?

Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
Sent: Thursday, October 28, 2021 10:00 AM
To: kraxel@redhat.com; Xu, Min M <min.m.xu@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>; devel@edk2.groups.io; Erdem
Aktas <erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
Tom Lendacky <thomas.lendacky@amd.com>; Dong, Eric
<eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul1
<rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

Hi Gerd
I tend to agree with you on the direction to use one TEE specific Exception lib.

However, I have naming concern.
The VMG is very SEV specific term. I don't believe it is a right name to cover the
TEE exception lib.

If Brijesh agree to merge, I think we should rename it to a neutral name, such as
TeeExitLib.

What do you think, Brijesh?

Thank you
Yao Jiewen


-----Original Message-----
From: kraxel@redhat.com <kraxel@redhat.com>
Sent: Wednesday, October 27, 2021 3:20 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
<jiewen.yao@intel.com>; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric <eric.dong@intel.com>;
Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

Hi,

How about adding the tdx exception handler to the existing library, so we
don't
have the churn of adding a new library everywhere *again*?
Do you mean add the VmTdExitVeHandler.c/VmTdExitLibNull.c in
CpuExceptionHandlerLib, then include the corresponding source file in each
*CpuExceptionHandlerLib.inf?

No, I mean extend the existing VmgExitLib instead of adding a new
VmTdExitLib, i.e. place the tdx handler in
OvmfPkg/Library/VmgExitLib/TdxExitHandler.c

take care,
Gerd




Gerd Hoffmann
 

On Thu, Oct 28, 2021 at 02:07:58AM +0000, Yao, Jiewen wrote:
Besides VmgExitLib -
https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Include/Library/VmgExitLib.h

We have another potential issue - MemEncryptSevLib -
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/Library/MemEncryptSevLib.h
We might need rename it to MemEncryptionTeeLib.

I think we need setup direction on how to hand those cases in a consistent way.

Option 1: Keep using current name: SEV and TDX as two class name. Add two instances.

Option 2: Define a new architecture neutral class name such as TEE. Add one instance to cover both SEV and TDX.
(2) looks better to me (for libraries, drivers is a different story).
Would also have the advantage that we can probably move (some of) the
dispatcher code (if sev call this, if tdx call that, else do nothing)
into the library too.

take care,
Gerd


Brijesh Singh
 

On 10/27/21 8:59 PM, Yao, Jiewen wrote:
Hi Gerd
I tend to agree with you on the direction to use one TEE specific Exception lib.
However, I have naming concern.
The VMG is very SEV specific term. I don't believe it is a right name to cover the TEE exception lib.
If Brijesh agree to merge, I think we should rename it to a neutral name, such as TeeExitLib.
What do you think, Brijesh?
I am good with merging both the TDX and SEV feature into one library but I am not sure about the "TEE" name in it. TEE generally is used on the ARM. In Linux kernel and everywhere else we have been using the COCO (Confidential Computing), so something along that line makes much more sense.

We can rename the library after the SNP patches are merged. I would prefer to avoid renaming because all of the SNP patches are Ack-ed.

-Brijesh
Thank you
Yao Jiewen

-----Original Message-----
From: kraxel@redhat.com <kraxel@redhat.com>
Sent: Wednesday, October 27, 2021 3:20 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
<jiewen.yao@intel.com>; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric <eric.dong@intel.com>; Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

Hi,

How about adding the tdx exception handler to the existing library, so we
don't
have the churn of adding a new library everywhere *again*?
Do you mean add the VmTdExitVeHandler.c/VmTdExitLibNull.c in
CpuExceptionHandlerLib, then include the corresponding source file in each
*CpuExceptionHandlerLib.inf?

No, I mean extend the existing VmgExitLib instead of adding a new
VmTdExitLib, i.e. place the tdx handler in
OvmfPkg/Library/VmgExitLib/TdxExitHandler.c

take care,
Gerd


Yao, Jiewen
 

Thanks Brijesh.

We can merge SNP patches at first, then decide next step. Not a problem.

TEE is just my initial thought. And I am open to change if we have a better name.

We already have EFI_TEE_MEASUREMENT_PROTOCOL. I did not see your feedback on that. So I assume you agree with that.

If you have different idea, please feedback to this patch. I hope we have one name.

COCO seems weird to me, btw. :(

Thank you
Yao Jiewen

-----Original Message-----
From: Brijesh Singh <brijesh.singh@amd.com>
Sent: Thursday, October 28, 2021 11:35 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; kraxel@redhat.com; Xu, Min M
<min.m.xu@intel.com>
Cc: brijesh.singh@amd.com; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric <eric.dong@intel.com>; Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception



On 10/27/21 8:59 PM, Yao, Jiewen wrote:
Hi Gerd
I tend to agree with you on the direction to use one TEE specific Exception lib.

However, I have naming concern.
The VMG is very SEV specific term. I don't believe it is a right name to cover
the TEE exception lib.

If Brijesh agree to merge, I think we should rename it to a neutral name, such
as TeeExitLib.

What do you think, Brijesh?
I am good with merging both the TDX and SEV feature into one library but
I am not sure about the "TEE" name in it. TEE generally is used on the
ARM. In Linux kernel and everywhere else we have been using the COCO
(Confidential Computing), so something along that line makes much more
sense.

We can rename the library after the SNP patches are merged. I would
prefer to avoid renaming because all of the SNP patches are Ack-ed.

-Brijesh

Thank you
Yao Jiewen


-----Original Message-----
From: kraxel@redhat.com <kraxel@redhat.com>
Sent: Wednesday, October 27, 2021 3:20 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
<jiewen.yao@intel.com>; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric <eric.dong@intel.com>;
Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28]
UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

Hi,

How about adding the tdx exception handler to the existing library, so we
don't
have the churn of adding a new library everywhere *again*?
Do you mean add the VmTdExitVeHandler.c/VmTdExitLibNull.c in
CpuExceptionHandlerLib, then include the corresponding source file in each
*CpuExceptionHandlerLib.inf?

No, I mean extend the existing VmgExitLib instead of adding a new
VmTdExitLib, i.e. place the tdx handler in
OvmfPkg/Library/VmgExitLib/TdxExitHandler.c

take care,
Gerd


Lendacky, Thomas
 

On 10/28/21 10:52 AM, Yao, Jiewen wrote:
Thanks Brijesh.
We can merge SNP patches at first, then decide next step. Not a problem.
TEE is just my initial thought. And I am open to change if we have a better name.
We already have EFI_TEE_MEASUREMENT_PROTOCOL. I did not see your feedback on that. So I assume you agree with that.
If you have different idea, please feedback to this patch. I hope we have one name.
COCO seems weird to me, btw. :(
Like Brijesh, I worry about confusion with the ARM TEE feature. Maybe just CC then?

Thanks,
Tom

Thank you
Yao Jiewen

-----Original Message-----
From: Brijesh Singh <brijesh.singh@amd.com>
Sent: Thursday, October 28, 2021 11:35 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; kraxel@redhat.com; Xu, Min M
<min.m.xu@intel.com>
Cc: brijesh.singh@amd.com; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric <eric.dong@intel.com>; Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception



On 10/27/21 8:59 PM, Yao, Jiewen wrote:
Hi Gerd
I tend to agree with you on the direction to use one TEE specific Exception lib.

However, I have naming concern.
The VMG is very SEV specific term. I don't believe it is a right name to cover
the TEE exception lib.

If Brijesh agree to merge, I think we should rename it to a neutral name, such
as TeeExitLib.

What do you think, Brijesh?
I am good with merging both the TDX and SEV feature into one library but
I am not sure about the "TEE" name in it. TEE generally is used on the
ARM. In Linux kernel and everywhere else we have been using the COCO
(Confidential Computing), so something along that line makes much more
sense.

We can rename the library after the SNP patches are merged. I would
prefer to avoid renaming because all of the SNP patches are Ack-ed.

-Brijesh

Thank you
Yao Jiewen


-----Original Message-----
From: kraxel@redhat.com <kraxel@redhat.com>
Sent: Wednesday, October 27, 2021 3:20 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
<jiewen.yao@intel.com>; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric <eric.dong@intel.com>;
Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28]
UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

Hi,

How about adding the tdx exception handler to the existing library, so we
don't
have the churn of adding a new library everywhere *again*?
Do you mean add the VmTdExitVeHandler.c/VmTdExitLibNull.c in
CpuExceptionHandlerLib, then include the corresponding source file in each
*CpuExceptionHandlerLib.inf?

No, I mean extend the existing VmgExitLib instead of adding a new
VmTdExitLib, i.e. place the tdx handler in
OvmfPkg/Library/VmgExitLib/TdxExitHandler.c

take care,
Gerd


Yao, Jiewen
 

I am OK to use EFI_CC_MEASUREMENT_PROTOCOL to replace EFI_TEE_MEASUREMENT_PROTOCOL. (much better than COCO)

Samy
What do you think?

-----Original Message-----
From: Tom Lendacky <thomas.lendacky@amd.com>
Sent: Friday, October 29, 2021 2:29 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Brijesh Singh
<brijesh.singh@amd.com>; kraxel@redhat.com; Xu, Min M
<min.m.xu@intel.com>
Cc: devel@edk2.groups.io; Erdem Aktas <erdemaktas@google.com>; James
Bottomley <jejb@linux.ibm.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

On 10/28/21 10:52 AM, Yao, Jiewen wrote:
Thanks Brijesh.

We can merge SNP patches at first, then decide next step. Not a problem.

TEE is just my initial thought. And I am open to change if we have a better
name.

We already have EFI_TEE_MEASUREMENT_PROTOCOL. I did not see your
feedback on that. So I assume you agree with that.

If you have different idea, please feedback to this patch. I hope we have one
name.

COCO seems weird to me, btw. :(
Like Brijesh, I worry about confusion with the ARM TEE feature. Maybe just
CC then?

Thanks,
Tom


Thank you
Yao Jiewen

-----Original Message-----
From: Brijesh Singh <brijesh.singh@amd.com>
Sent: Thursday, October 28, 2021 11:35 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; kraxel@redhat.com; Xu, Min M
<min.m.xu@intel.com>
Cc: brijesh.singh@amd.com; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric <eric.dong@intel.com>;
Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28]
UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception



On 10/27/21 8:59 PM, Yao, Jiewen wrote:
Hi Gerd
I tend to agree with you on the direction to use one TEE specific Exception
lib.

However, I have naming concern.
The VMG is very SEV specific term. I don't believe it is a right name to cover
the TEE exception lib.

If Brijesh agree to merge, I think we should rename it to a neutral name,
such
as TeeExitLib.

What do you think, Brijesh?
I am good with merging both the TDX and SEV feature into one library but
I am not sure about the "TEE" name in it. TEE generally is used on the
ARM. In Linux kernel and everywhere else we have been using the COCO
(Confidential Computing), so something along that line makes much more
sense.

We can rename the library after the SNP patches are merged. I would
prefer to avoid renaming because all of the SNP patches are Ack-ed.

-Brijesh

Thank you
Yao Jiewen


-----Original Message-----
From: kraxel@redhat.com <kraxel@redhat.com>
Sent: Wednesday, October 27, 2021 3:20 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
<jiewen.yao@intel.com>; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric
<eric.dong@intel.com>;
Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28]
UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

Hi,

How about adding the tdx exception handler to the existing library, so
we
don't
have the churn of adding a new library everywhere *again*?
Do you mean add the VmTdExitVeHandler.c/VmTdExitLibNull.c in
CpuExceptionHandlerLib, then include the corresponding source file in
each
*CpuExceptionHandlerLib.inf?

No, I mean extend the existing VmgExitLib instead of adding a new
VmTdExitLib, i.e. place the tdx handler in
OvmfPkg/Library/VmgExitLib/TdxExitHandler.c

take care,
Gerd


Yao, Jiewen
 

Just to clarify the proposal: We will use EFI_CC_MEASUREMENT_PROTOCOL, CcMemoryEncryptionLib, and CcExceptionLib, right?

Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
Sent: Friday, October 29, 2021 8:17 AM
To: Tom Lendacky <thomas.lendacky@amd.com>; Brijesh Singh
<brijesh.singh@amd.com>; kraxel@redhat.com; Xu, Min M
<min.m.xu@intel.com>; sami.mujawar@arm.com
Cc: devel@edk2.groups.io; Erdem Aktas <erdemaktas@google.com>; James
Bottomley <jejb@linux.ibm.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

I am OK to use EFI_CC_MEASUREMENT_PROTOCOL to replace
EFI_TEE_MEASUREMENT_PROTOCOL. (much better than COCO)

Samy
What do you think?



-----Original Message-----
From: Tom Lendacky <thomas.lendacky@amd.com>
Sent: Friday, October 29, 2021 2:29 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Brijesh Singh
<brijesh.singh@amd.com>; kraxel@redhat.com; Xu, Min M
<min.m.xu@intel.com>
Cc: devel@edk2.groups.io; Erdem Aktas <erdemaktas@google.com>; James
Bottomley <jejb@linux.ibm.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

On 10/28/21 10:52 AM, Yao, Jiewen wrote:
Thanks Brijesh.

We can merge SNP patches at first, then decide next step. Not a problem.

TEE is just my initial thought. And I am open to change if we have a better
name.

We already have EFI_TEE_MEASUREMENT_PROTOCOL. I did not see your
feedback on that. So I assume you agree with that.

If you have different idea, please feedback to this patch. I hope we have one
name.

COCO seems weird to me, btw. :(
Like Brijesh, I worry about confusion with the ARM TEE feature. Maybe just
CC then?

Thanks,
Tom


Thank you
Yao Jiewen

-----Original Message-----
From: Brijesh Singh <brijesh.singh@amd.com>
Sent: Thursday, October 28, 2021 11:35 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; kraxel@redhat.com; Xu, Min M
<min.m.xu@intel.com>
Cc: brijesh.singh@amd.com; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric
<eric.dong@intel.com>;
Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28]
UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception



On 10/27/21 8:59 PM, Yao, Jiewen wrote:
Hi Gerd
I tend to agree with you on the direction to use one TEE specific Exception
lib.

However, I have naming concern.
The VMG is very SEV specific term. I don't believe it is a right name to
cover
the TEE exception lib.

If Brijesh agree to merge, I think we should rename it to a neutral name,
such
as TeeExitLib.

What do you think, Brijesh?
I am good with merging both the TDX and SEV feature into one library but
I am not sure about the "TEE" name in it. TEE generally is used on the
ARM. In Linux kernel and everywhere else we have been using the COCO
(Confidential Computing), so something along that line makes much more
sense.

We can rename the library after the SNP patches are merged. I would
prefer to avoid renaming because all of the SNP patches are Ack-ed.

-Brijesh

Thank you
Yao Jiewen


-----Original Message-----
From: kraxel@redhat.com <kraxel@redhat.com>
Sent: Wednesday, October 27, 2021 3:20 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
<jiewen.yao@intel.com>; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric
<eric.dong@intel.com>;
Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28]
UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

Hi,

How about adding the tdx exception handler to the existing library, so
we
don't
have the churn of adding a new library everywhere *again*?
Do you mean add the VmTdExitVeHandler.c/VmTdExitLibNull.c in
CpuExceptionHandlerLib, then include the corresponding source file in
each
*CpuExceptionHandlerLib.inf?

No, I mean extend the existing VmgExitLib instead of adding a new
VmTdExitLib, i.e. place the tdx handler in
OvmfPkg/Library/VmgExitLib/TdxExitHandler.c

take care,
Gerd



Brijesh Singh
 

On 10/28/21 7:20 PM, Yao, Jiewen wrote:
Just to clarify the proposal: We will use EFI_CC_MEASUREMENT_PROTOCOL, CcMemoryEncryptionLib, and CcExceptionLib, right?
Ack.



Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
Sent: Friday, October 29, 2021 8:17 AM
To: Tom Lendacky <thomas.lendacky@amd.com>; Brijesh Singh
<brijesh.singh@amd.com>; kraxel@redhat.com; Xu, Min M
<min.m.xu@intel.com>; sami.mujawar@arm.com
Cc: devel@edk2.groups.io; Erdem Aktas <erdemaktas@google.com>; James
Bottomley <jejb@linux.ibm.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

I am OK to use EFI_CC_MEASUREMENT_PROTOCOL to replace
EFI_TEE_MEASUREMENT_PROTOCOL. (much better than COCO)

Samy
What do you think?



-----Original Message-----
From: Tom Lendacky <thomas.lendacky@amd.com>
Sent: Friday, October 29, 2021 2:29 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Brijesh Singh
<brijesh.singh@amd.com>; kraxel@redhat.com; Xu, Min M
<min.m.xu@intel.com>
Cc: devel@edk2.groups.io; Erdem Aktas <erdemaktas@google.com>; James
Bottomley <jejb@linux.ibm.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray
<ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28] UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

On 10/28/21 10:52 AM, Yao, Jiewen wrote:
Thanks Brijesh.

We can merge SNP patches at first, then decide next step. Not a problem.

TEE is just my initial thought. And I am open to change if we have a better
name.
We already have EFI_TEE_MEASUREMENT_PROTOCOL. I did not see your
feedback on that. So I assume you agree with that.
If you have different idea, please feedback to this patch. I hope we have one
name.
COCO seems weird to me, btw. :(
Like Brijesh, I worry about confusion with the ARM TEE feature. Maybe just
CC then?

Thanks,
Tom

Thank you
Yao Jiewen

-----Original Message-----
From: Brijesh Singh <brijesh.singh@amd.com>
Sent: Thursday, October 28, 2021 11:35 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; kraxel@redhat.com; Xu, Min M
<min.m.xu@intel.com>
Cc: brijesh.singh@amd.com; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric
<eric.dong@intel.com>;
Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28]
UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception



On 10/27/21 8:59 PM, Yao, Jiewen wrote:
Hi Gerd
I tend to agree with you on the direction to use one TEE specific Exception
lib.
However, I have naming concern.
The VMG is very SEV specific term. I don't believe it is a right name to
cover
the TEE exception lib.
If Brijesh agree to merge, I think we should rename it to a neutral name,
such
as TeeExitLib.
What do you think, Brijesh?
I am good with merging both the TDX and SEV feature into one library but
I am not sure about the "TEE" name in it. TEE generally is used on the
ARM. In Linux kernel and everywhere else we have been using the COCO
(Confidential Computing), so something along that line makes much more
sense.

We can rename the library after the SNP patches are merged. I would
prefer to avoid renaming because all of the SNP patches are Ack-ed.

-Brijesh
Thank you
Yao Jiewen


-----Original Message-----
From: kraxel@redhat.com <kraxel@redhat.com>
Sent: Wednesday, October 27, 2021 3:20 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
<jiewen.yao@intel.com>; devel@edk2.groups.io; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>;
Tom
Lendacky <thomas.lendacky@amd.com>; Dong, Eric
<eric.dong@intel.com>;
Ni,
Ray <ray.ni@intel.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH V2 12/28]
UefiCpuPkg/CpuExceptionHandler:
Add base support for the #VE exception

Hi,

How about adding the tdx exception handler to the existing library, so
we
don't
have the churn of adding a new library everywhere *again*?
Do you mean add the VmTdExitVeHandler.c/VmTdExitLibNull.c in
CpuExceptionHandlerLib, then include the corresponding source file in
each
*CpuExceptionHandlerLib.inf?

No, I mean extend the existing VmgExitLib instead of adding a new
VmTdExitLib, i.e. place the tdx handler in
OvmfPkg/Library/VmgExitLib/TdxExitHandler.c

take care,
Gerd


Gerd Hoffmann
 

On Fri, Oct 29, 2021 at 12:17:05AM +0000, Yao, Jiewen wrote:
I am OK to use EFI_CC_MEASUREMENT_PROTOCOL to replace EFI_TEE_MEASUREMENT_PROTOCOL. (much better than COCO)
Looks good to me. The PCD uses the term ConfidentialComputing too,
so using that or 'CC' as shortcut consistently everywhere makes sense.

take care,
Gerd


Min Xu
 

On October 29, 2021 12:53 PM, Gerd Hoffmann wrote:
On Fri, Oct 29, 2021 at 12:17:05AM +0000, Yao, Jiewen wrote:
I am OK to use EFI_CC_MEASUREMENT_PROTOCOL to replace
EFI_TEE_MEASUREMENT_PROTOCOL. (much better than COCO)
Looks good to me. The PCD uses the term ConfidentialComputing too, so using
that or 'CC' as shortcut consistently everywhere makes sense.
So do we reach consensus that as the first step, extend the existing VmgExitLib to place the tdx handler (OvmfPkg/Library/VmgExitLib/TdxExitHandler.c). After SNP patch is merged, rename VmgExitLib to CcExitLib. Right?

Thanks.
Min


Gerd Hoffmann
 

On Fri, Oct 29, 2021 at 07:51:13AM +0000, Xu, Min M wrote:
On October 29, 2021 12:53 PM, Gerd Hoffmann wrote:
On Fri, Oct 29, 2021 at 12:17:05AM +0000, Yao, Jiewen wrote:
I am OK to use EFI_CC_MEASUREMENT_PROTOCOL to replace
EFI_TEE_MEASUREMENT_PROTOCOL. (much better than COCO)
Looks good to me. The PCD uses the term ConfidentialComputing too, so using
that or 'CC' as shortcut consistently everywhere makes sense.
So do we reach consensus that as the first step, extend the existing
VmgExitLib to place the tdx handler
(OvmfPkg/Library/VmgExitLib/TdxExitHandler.c). After SNP patch is
merged, rename VmgExitLib to CcExitLib. Right?
snp series goes first.

Whenever adding TdxExitHandler.c or renaming the library goes next
doesn't matter much, just do whatever is easier for you.

The renaming can be a separate series, so maybe easiest to do that
first and merge quickly so you have less unmerged patches to handle.

take care,
Gerd