Date   

[PATCH v2] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

Ma, Hua
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3680

This patch fixes the following issue:

The global variable gHandleList is a linked list.
This list is locked when a entry is added or removed from the list,
but there is no lock when iterating this list in function
CoreValidateHandle().
It can lead to "Handle.c (76): CR has Bad Signature" assertion if the
iterated entry in the list is just removed by other task during iterating.
Locking the list when iterating can fix this issue.

v2 changes:
- Add lock check and comments in CoreGetProtocolInterface() before
calling CoreValidateHandle()
- Update the comments in CoreValidateHandle() header file

v1: https://edk2.groups.io/g/devel/topic/86233569

Cc: Jian J Wang <jian.j.wang@...>
Cc: Liming Gao <gaoliming@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Ray Ni <ray.ni@...>
Signed-off-by: Hua Ma <hua.ma@...>
---
MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 10 ++--
MdeModulePkg/Core/Dxe/Hand/Handle.c | 56 +++++++++++++++-------
MdeModulePkg/Core/Dxe/Hand/Handle.h | 5 +-
MdeModulePkg/Core/Dxe/Hand/Notify.c | 2 +-
4 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
index feabf12faf..eb8a765d2c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
+++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
@@ -68,7 +68,7 @@ CoreConnectController (
//
// Make sure ControllerHandle is valid
//
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -154,7 +154,7 @@ CoreConnectController (
//
// Make sure the DriverBindingHandle is valid
//
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, TRUE);
if (EFI_ERROR (Status)) {
//
// Release the protocol lock on the handle database
@@ -268,7 +268,7 @@ AddSortedDriverBindingProtocol (
//
// Make sure the DriverBindingHandle is valid
//
- Status = CoreValidateHandle (DriverBindingHandle);
+ Status = CoreValidateHandle (DriverBindingHandle, FALSE);
if (EFI_ERROR (Status)) {
return;
}
@@ -746,7 +746,7 @@ CoreDisconnectController (
//
// Make sure ControllerHandle is valid
//
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -755,7 +755,7 @@ CoreDisconnectController (
// Make sure ChildHandle is valid if it is not NULL
//
if (ChildHandle != NULL) {
- Status = CoreValidateHandle (ChildHandle);
+ Status = CoreValidateHandle (ChildHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 6eccb41ecb..46f67d3d6a 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -55,31 +55,46 @@ CoreReleaseProtocolLock (
Check whether a handle is a valid EFI_HANDLE

@param UserHandle The handle to check
+ @param IsLocked The protocol lock is acquried or not

@retval EFI_INVALID_PARAMETER The handle is NULL or not a valid EFI_HANDLE.
+ @retval EFI_NOT_FOUND The handle is not found in the handle database.
@retval EFI_SUCCESS The handle is valid EFI_HANDLE.

**/
EFI_STATUS
CoreValidateHandle (
- IN EFI_HANDLE UserHandle
+ IN EFI_HANDLE UserHandle,
+ IN BOOLEAN IsLocked
)
{
IHANDLE *Handle;
LIST_ENTRY *Link;
+ EFI_STATUS Status;

if (UserHandle == NULL) {
return EFI_INVALID_PARAMETER;
}

+ if (IsLocked) {
+ ASSERT_LOCKED(&gProtocolDatabaseLock);
+ } else {
+ CoreAcquireProtocolLock ();
+ }
+
+ Status = EFI_NOT_FOUND;
for (Link = gHandleList.BackLink; Link != &gHandleList; Link = Link->BackLink) {
Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
if (Handle == (IHANDLE *) UserHandle) {
- return EFI_SUCCESS;
+ Status = EFI_SUCCESS;
+ break;
}
}

- return EFI_INVALID_PARAMETER;
+ if (!IsLocked) {
+ CoreReleaseProtocolLock ();
+ }
+ return Status;
}


@@ -428,7 +443,7 @@ CoreInstallProtocolInterfaceNotify (
//
InsertTailList (&gHandleList, &Handle->AllHandles);
} else {
- Status = CoreValidateHandle (Handle);
+ Status = CoreValidateHandle (Handle, TRUE);
if (EFI_ERROR (Status)) {
DEBUG((DEBUG_ERROR, "InstallProtocolInterface: input handle at 0x%x is invalid\n", Handle));
goto Done;
@@ -723,7 +738,7 @@ CoreUninstallProtocolInterface (
//
// Check that UserHandle is a valid handle
//
- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -899,7 +914,16 @@ CoreGetProtocolInterface (
IHANDLE *Handle;
LIST_ENTRY *Link;

- Status = CoreValidateHandle (UserHandle);
+ //
+ // The gProtocolDatabaseLock must be owned
+ //
+ ASSERT_LOCKED(&gProtocolDatabaseLock);
+
+ //
+ // The gProtocolDatabaseLock is owned
+ // Call CoreValidateHandle () with IsLocked == TRUE
+ //
+ Status = CoreValidateHandle (UserHandle, TRUE);
if (EFI_ERROR (Status)) {
return NULL;
}
@@ -1013,7 +1037,7 @@ CoreOpenProtocol (
//
// Check for invalid UserHandle
//
- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1023,11 +1047,11 @@ CoreOpenProtocol (
//
switch (Attributes) {
case EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER :
- Status = CoreValidateHandle (ImageHandle);
+ Status = CoreValidateHandle (ImageHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1037,17 +1061,17 @@ CoreOpenProtocol (
break;
case EFI_OPEN_PROTOCOL_BY_DRIVER :
case EFI_OPEN_PROTOCOL_BY_DRIVER | EFI_OPEN_PROTOCOL_EXCLUSIVE :
- Status = CoreValidateHandle (ImageHandle);
+ Status = CoreValidateHandle (ImageHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
break;
case EFI_OPEN_PROTOCOL_EXCLUSIVE :
- Status = CoreValidateHandle (ImageHandle);
+ Status = CoreValidateHandle (ImageHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1249,16 +1273,16 @@ CoreCloseProtocol (
//
// Check for invalid parameters
//
- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
- Status = CoreValidateHandle (AgentHandle);
+ Status = CoreValidateHandle (AgentHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
if (ControllerHandle != NULL) {
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1443,7 +1467,7 @@ CoreProtocolsPerHandle (
UINTN ProtocolCount;
EFI_GUID **Buffer;

- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.h b/MdeModulePkg/Core/Dxe/Hand/Handle.h
index 83eb2b9f3a..20d886beca 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.h
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h
@@ -244,14 +244,17 @@ CoreReleaseProtocolLock (
Check whether a handle is a valid EFI_HANDLE

@param UserHandle The handle to check
+ @param IsLocked The protocol lock is acquried or not

@retval EFI_INVALID_PARAMETER The handle is NULL or not a valid EFI_HANDLE.
+ @retval EFI_NOT_FOUND The handle is not found in the handle database.
@retval EFI_SUCCESS The handle is valid EFI_HANDLE.

**/
EFI_STATUS
CoreValidateHandle (
- IN EFI_HANDLE UserHandle
+ IN EFI_HANDLE UserHandle,
+ IN BOOLEAN IsLocked
);

//
diff --git a/MdeModulePkg/Core/Dxe/Hand/Notify.c b/MdeModulePkg/Core/Dxe/Hand/Notify.c
index 553413a350..f4e7c01e96 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Notify.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Notify.c
@@ -188,7 +188,7 @@ CoreReinstallProtocolInterface (
PROTOCOL_INTERFACE *Prot;
PROTOCOL_ENTRY *ProtEntry;

- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
--
2.32.0.windows.2


Re: [PATCH edk2-test v2 1/1] uefi-sct/SctPkg: fix BuildAtaDeviceNode()

G Edhaya Chandran
 

Reviewed-by: G Edhaya Chandran<edhaya.chandran@...>


Re: [PATCH V2 05/28] MdePkg: Add TdxLib to wrap Tdx operations

Gerd Hoffmann
 

+// PageSize is mapped to PageLevel like below:
+// 4KB - 0, 2MB - 1
+UINT64 mTdxAcceptPageLevelMap[2] = {
+ SIZE_4KB,
+ SIZE_2MB
No 1G pages?

+UINTN
+GetGpaPageLevel (
+ UINT64 PageSize
Uh, UINT32 is not enough? Keep the door open for 512G pages?

+{
+ UINTN Index;
+
+ for (Index = 0; Index < sizeof (mTdxAcceptPageLevelMap) / sizeof (mTdxAcceptPageLevelMap[0]); Index++) {
There is an ARRAY_SIZE() macro, no need to open code the sizeof() trick.

+ if (mTdxAcceptPageLevelMap[Index] == PageSize) {
+ break;
+ }
+ }
+
+ return Index;
+}
No error handling (invalid PageSize) here?

+/**
+ This function accept a pending private page, and initialize the page to
+ all-0 using the TD ephemeral private key.
+
+ Sometimes TDCALL [TDG.MEM.PAGE.ACCEPT] may return
+ TDX_EXIT_REASON_PAGE_SIZE_MISMATCH. It indicates the input PageLevel is
+ not workable. In this case we need to try to fallback to a smaller
+ PageLevel if possible.
+
+ @param[in] StartAddress Guest physical address of the private
+ page to accept.
+ @param[in] NumberOfPages Number of the pages to be accepted.
+ @param[in] PageSize GPA page size. Only accept 1G/2M/4K size.
+
+ @return EFI_SUCCESS Accept successfully
+ @return others Indicate other errors
+**/
+EFI_STATUS
+EFIAPI
+TdAcceptPages (
+ IN UINT64 StartAddress,
+ IN UINT64 NumberOfPages,
+ IN UINT64 PageSize
+ )
+{
+ EFI_STATUS Status;
+ UINT64 Address;
+ UINT64 TdxStatus;
+ UINT64 Index;
+ UINT64 GpaPageLevel;
+ UINT64 PageSize2;
+
+ Address = StartAddress;
+
+ GpaPageLevel = (UINT64) GetGpaPageLevel (PageSize);
Why cast?

+ if (GpaPageLevel > sizeof (mTdxAcceptPageLevelMap) / sizeof (mTdxAcceptPageLevelMap[0])) {
+ DEBUG ((DEBUG_ERROR, "Accept page size must be 4K/2M. Invalid page size - 0x%llx\n", PageSize));
Ah. Errors are catched here. Well, no. The check is wrong,
it should be ">=" not ">".

Better would be GetGpaPageLevel explicitly returning a specific value
(for example -1) on error.

+ Status = EFI_SUCCESS;
+ for (Index = 0; Index < NumberOfPages; Index++) {
+ TdxStatus = TdCall (TDCALL_TDACCEPTPAGE, Address | GpaPageLevel, 0, 0, 0);
+ if (TdxStatus != TDX_EXIT_REASON_SUCCESS) {
+ if ((TdxStatus & ~0xFFFFULL) == TDX_EXIT_REASON_PAGE_ALREADY_ACCEPTED) {
+ //
+ // Already accepted
+ //
+ mNumberOfDuplicatedAcceptedPages++;
Hmm. When this happens we have a bug somewhere, right?
So should this be an assert()?
Or should we at least log the address?

+#define RTMR_COUNT 4
+#define TD_EXTEND_BUFFER_LEN (64 + 64)
+#define EXTEND_BUFFER_ADDRESS_MASK 0x3f
+
+
+#pragma pack(16)
+typedef struct {
+ UINT8 Buffer[TD_EXTEND_BUFFER_LEN];
+} TDX_EXTEND_BUFFER;
+#pragma pack()
+
+UINT8 *mExtendBufferAddress = NULL;
+TDX_EXTEND_BUFFER mExtendBuffer;
+
+/**
+ TD.RTMR.EXTEND requires 64B-aligned guest physical address of
+ 48B-extension data. In runtime we walk thru the Buffer to find
+ out a 64B-aligned start address.
Can't you just use __attribute__((aligned(64))) for that?

take care,
Gerd


[PATCH EDK2 v1 1/1] EmbeddedPkg:Fix compiler warning

wenyi,xie
 

Fixes the following compiler warning in VS2019.

edk2\EmbeddedPkg\Library\GdbSerialDebugPortLib\GdbSerialDebugPortLib.c(127):
error C2220: the following warning is treated as an error
edk2\EmbeddedPkg\Library\GdbSerialDebugPortLib\GdbSerialDebugPortLib.c(127):
warning C4244: 'function': conversion from 'UINTN' to 'UINT32', possible
loss of data

edk2\EmbeddedPkg\Library\PrePiLib\FwVol.c(347): error C2220: the following
warning is treated as an error
edk2\EmbeddedPkg\Library\PrePiLib\FwVol.c(347): warning C4244: 'function':
conversion from 'UINTN' to 'UINT32', possible loss of data

Cc: Leif Lindholm <leif@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Abner Chang <abner.chang@...>
Cc: Daniel Schaefer <daniel.schaefer@...>
Signed-off-by: Wenyi Xie <xiewenyi2@...>
---
EmbeddedPkg/Library/GdbSerialDebugPortLib/GdbSerialDebugPortLib.c | 2 +-
EmbeddedPkg/Library/PrePiLib/FwVol.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/EmbeddedPkg/Library/GdbSerialDebugPortLib/GdbSerialDebugPortLib.c b/EmbeddedPkg/Library/GdbSerialDebugPortLib/GdbSerialDebugPortLib.c
index d2bafcf69b60..0f50a8b64191 100644
--- a/EmbeddedPkg/Library/GdbSerialDebugPortLib/GdbSerialDebugPortLib.c
+++ b/EmbeddedPkg/Library/GdbSerialDebugPortLib/GdbSerialDebugPortLib.c
@@ -18,7 +18,7 @@


EFI_DEBUGPORT_PROTOCOL *gDebugPort = NULL;
-UINTN gTimeOut = 0;
+UINT32 gTimeOut = 0;

/**
The constructor function initializes the UART.
diff --git a/EmbeddedPkg/Library/PrePiLib/FwVol.c b/EmbeddedPkg/Library/PrePiLib/FwVol.c
index 881506edddaf..46ea5f733f60 100644
--- a/EmbeddedPkg/Library/PrePiLib/FwVol.c
+++ b/EmbeddedPkg/Library/PrePiLib/FwVol.c
@@ -298,7 +298,7 @@ FfsProcessSection (
UINT16 SectionAttribute;
UINT32 AuthenticationStatus;
CHAR8 *CompressedData;
- UINTN CompressedDataLength;
+ UINT32 CompressedDataLength;


*OutputBuffer = NULL;
--
2.20.1.windows.1


[PATCH EDK2 v1 0/1] EmbeddedPkg:Fix compiler warning

wenyi,xie
 

Main Changes :
1.Changing definition of gTimeOut from UINTN to UINT32
2.Changing definition of CompressedDataLength from UINTN to UINT32

Wenyi Xie (1):
EmbeddedPkg:Fix compiler warning

EmbeddedPkg/Library/GdbSerialDebugPortLib/GdbSerialDebugPortLib.c | 2 +-
EmbeddedPkg/Library/PrePiLib/FwVol.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

--
2.20.1.windows.1


Re: [PATCH edk2-test 1/1] uefi-sct/SctPkg: unsupported TEXT_INPUT_EX.SetState

G Edhaya Chandran
 

On Thu, Sep 9, 2021 at 12:45 AM, Heinrich Schuchardt wrote:
BBTestReadKeyStrokeExFunctionAutoTestCheckpoint1
Reviewed-by: G Edhaya Chandran<edhaya.chandran@...>


Re: [PATCH V2 04/28] MdePkg: Add Tdx.h

Gerd Hoffmann
 

On Tue, Oct 05, 2021 at 11:39:15AM +0800, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

Tdx.h includes the Intel Trust Domain Extension definitions.

Detailed information can be found in below document:
https://software.intel.com/content/dam/develop/external/us/en/
documents/tdx-module-1eas-v0.85.039.pdf

Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Cc: Jiewen Yao <jiewen.yao@...>
Signed-off-by: Min Xu <min.m.xu@...>
Acked-by: Gerd Hoffmann <kraxel@...>


Re: [PATCH V9 4/4] OvmfPkg: Enable TDX in ResetVector

Gerd Hoffmann
 

Hi,

+; Load the GDT and set the CR0.
+;
+; Modified: EAX, EBX, CR0, CR4, DS, ES, FS, GS, SS, CS
+;
+ReloadFlat32:
+
+ cli
+ mov ebx, ADDR_OF(gdtr)
+ lgdt [ebx]
No need to modify ebx here, eax should do fine.

+ mov eax, SEC_DEFAULT_CR0
+ mov cr0, eax
+
+ jmp LINEAR_CODE_SEL:dword ADDR_OF(jumpToFlat32BitAndLandHere)
+
+jumpToFlat32BitAndLandHere:
Strictly speaking this is not correct, you are already in Flat32 mode,
so this only loads cs.

+InitTdx:
+ ;
+ ; Save EBX in EBP because EBX will be changed in ReloadFlat32
+ ;
+ mov ebp, ebx
See above, there is no need to modify ebx in ReloadFlat32.
Also: seems ebx is never restored ...

take care,
Gerd


Re: [PATCH V9 3/4] OvmfPkg: Add IntelTdxMetadata.asm

Gerd Hoffmann
 

Hi,

+; - Type field means this section is of BFV. This field is designed for the
+; purpose that in some case host VMM may do some additional processing based
+; upon the section type. TdHob section is an example. Host VMM pass the
+; physical memory information to the guest firmware by writing the data in
+; the memory region designated by TdHob section.
Brijesh? What are your plans? Do you want use this too?

If so, then the section types need some more structure. I think best
would be reserve blocks, something like 0x00 -> 0xff common types (bfv,
cfv, temp_mem), 0x100 -> 0x1ff tdx (td_hob), 0x200 -> 0x2ff sev (cpuid,
secrets, ...).

take care,
Gerd


Re: [PATCH V9 2/4] OvmfPkg: Clear WORK_AREA_GUEST_TYPE in Main.asm

Gerd Hoffmann
 

On Tue, Oct 12, 2021 at 10:37:48AM +0800, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

Previously WORK_AREA_GUEST_TYPE was cleared in SetCr3ForPageTables64.
This is workable for Legacy guest and SEV guest. But it doesn't work
after Intel TDX is introduced. It is because all TDX CPUs (BSP and APs)
start to run from 0xfffffff0, thus WORK_AREA_GUEST_TYPE will be cleared
multi-times if it is TDX guest. So the clearance of WORK_AREA_GUEST_TYPE
is moved to Main16 entry point in Main.asm.
Note: WORK_AREA_GUEST_TYPE is only defined for ARCH_X64.

For Intel TDX, its corresponding entry point is Main32 (which will be
introduced in next commit in this patch-set). WORK_AREA_GUEST_TYPE will
be cleared there.
Acked-by: Gerd Hoffmann <kraxel@...>

take care,
Gerd


Re: [PATCH V9 1/4] OvmfPkg: Copy Main.asm from UefiCpuPkg to OvmfPkg's ResetVector

Gerd Hoffmann
 

On Tue, Oct 12, 2021 at 10:37:47AM +0800, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

Previously OvmfPkg/ResetVector uses the Main.asm in
UefiCpuPkg/ReseteVector/Vtf0. In this Main.asm there is only Main16
entry point.

This patch-set is to introduce Intel TDX into Ovmf. Main32 entry point
is needed in Main.asm by Intel TDX. To reduce the complexity of Main.asm
in UefiCpuPkg, OvmfPkg create its own Main.asm to meet the requirement
of Intel TDX.

UefiCpuPkg/ResetVector/Vtf0/main.asm -> OvmfPkg/ResetVector/Main.asm
You should mention that this is an unmodified copy (so no functional
change) and the actual changes for tdx come as incremental patches.

With that:
Acked-by: Gerd Hoffmann <kraxel@...>

take care,
Gerd


Re: [PATCH V3 00/12] Migrate ArmVirtPkg modules to OvmfPkg

Abner Chang
 

Hi package maintainers,

The review process of this patch set is almost done and please allow me to merge it because the corresponding changes on edk2-platform is also required to merge.

 

Ard and Leif, do I need the Reviewed-by or Acked-by from either of you? Or I can just proceed the merge process as Ard has no problem with this patch set?

 

Abner

 

From: gaoliming [mailto:gaoliming@...]
Sent: Monday, October 11, 2021 9:22 AM
To: devel@edk2.groups.io; Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>; ardb@...
Cc: 'Ard Biesheuvel' <ardb+tianocore@...>; 'Leif Lindholm' <leif@...>; 'Sami Mujawar' <sami.mujawar@...>; 'Jiewen Yao' <jiewen.yao@...>; 'Jordan Justen' <jordan.l.justen@...>; 'Gerd Hoffmann' <kraxel@...>; Schaefer, Daniel <daniel.schaefer@...>; 'Sunil V L' <sunilvl@...>; 'Zhiguang Liu' <zhiguang.liu@...>; 'Michael D Kinney' <michael.d.kinney@...>
Subject: 回复: [edk2-devel] [PATCH V3 00/12] Migrate ArmVirtPkg modules to OvmfPkg

 

The change in MdePkg is good to me. Reviewed-by: Liming Gao <gaoliming@...>

 

发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Abner Chang
发送时间: 2021108 11:39
收件人: devel@edk2.groups.io; gaoliming@...; ardb@...
抄送: 'Ard Biesheuvel' <ardb+tianocore@...>; 'Leif Lindholm' <leif@...>; 'Sami Mujawar' <sami.mujawar@...>; 'Jiewen Yao' <jiewen.yao@...>; 'Jordan Justen' <jordan.l.justen@...>; 'Gerd Hoffmann' <kraxel@...>; Schaefer, Daniel <daniel.schaefer@...>; 'Sunil V L' <sunilvl@...>; 'Zhiguang Liu' <zhiguang.liu@...>; 'Michael D Kinney' <michael.d.kinney@...>
主题: Re: [edk2-devel] [PATCH V3 00/12] Migrate ArmVirtPkg modules to OvmfPkg

 

Thanks Liming, could you please also give the reviewed-by.

 

Hi Ard, also need your reviewed-by for ArmPkg. Then this changes could be upstream and finished.

Thanks

Abner

 

 

From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of gaoliming
Sent: Friday, October 8, 2021 11:14 AM
To: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>; 'edk2-devel-groups-io' <devel@edk2.groups.io>; ardb@...
Cc: 'Ard Biesheuvel' <ardb+tianocore@...>; 'Leif Lindholm' <leif@...>; 'Sami Mujawar' <sami.mujawar@...>; 'Jiewen Yao' <jiewen.yao@...>; 'Jordan Justen' <jordan.l.justen@...>; 'Gerd Hoffmann' <kraxel@...>; Schaefer, Daniel <daniel.schaefer@...>; 'Sunil V L' <sunilvl@...>; 'Zhiguang Liu' <zhiguang.liu@...>; 'Michael D Kinney' <michael.d.kinney@...>
Subject: 回复: [edk2-devel] [PATCH V3 00/12] Migrate ArmVirtPkg modules to OvmfPkg

 

Ard and Abner:

 I am OK to add these three PCDs PcdPciMmio32Translation, PcdPciMmio64Translation, PcdPciIoTranslation to MdePkg.

 

Thanks

Liming

发件人: Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
发送时间: 2021106 17:27
收件人: edk2-devel-groups-io <devel@edk2.groups.io>; ardb@...; Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
抄送: Ard Biesheuvel <ardb+tianocore@...>; Leif Lindholm <leif@...>; Sami Mujawar <sami.mujawar@...>; Jiewen Yao <jiewen.yao@...>; Jordan Justen <jordan.l.justen@...>; Gerd Hoffmann <kraxel@...>; Schaefer, Daniel <daniel.schaefer@...>; Sunil V L <sunilvl@...>; Liming Gao <gaoliming@...>; Zhiguang Liu <zhiguang.liu@...>; Michael D Kinney <michael.d.kinney@...>
主题: Re: [edk2-devel] [PATCH V3 00/12] Migrate ArmVirtPkg modules to OvmfPkg

 

Hi Ard,

I realized there is a problem if we duplicate ArmPkg defined PCD to under OvmfPkg (e.g. PcdPciIoTranslate PCD) when I was duplicating this PCD to OvmfPkg.

FdtPciProducerLib is relocated to OvmfPkg/Fdt and uses PcdPciIoTranslate PCD declared with OvmfPkg namespace. FdtPciProducerLib is also used by both ArmVirtPkg  and RiscVVirtPkg.

ArmVirtPkg uses ArmPciCpuIoDxe provided by ArmPkg however PcdPciIoTranslate used by ArmPciCpuIoDxe  is declared with ArmPkg namespace.

I think this results in the problem because PcdPciIoTranslate(s) that are referred by ArmPkg and ArmVirtPkg come from two different namespaces, right? Unless ArmPciCpuIoDxe uses the one declared in OvmfPkg, but I don't think we want to do this.

Thought? Otherwise, we should still keep the original patch that relocates these PCDs under MdePkg.

 

Thanks

Abner

 

 


From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Abner Chang <abner.chang@...>
Sent: Tuesday, October 5, 2021 11:00 PM
To: edk2-devel-groups-io <devel@edk2.groups.io>; ardb@... <ardb@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>; Leif Lindholm <leif@...>; Sami Mujawar <sami.mujawar@...>; Jiewen Yao <jiewen.yao@...>; Jordan Justen <jordan.l.justen@...>; Gerd Hoffmann <kraxel@...>; Schaefer, Daniel <daniel.schaefer@...>; Sunil V L <sunilvl@...>; Liming Gao <gaoliming@...>; Zhiguang Liu <zhiguang.liu@...>; Michael D Kinney <michael.d.kinney@...>
Subject: Re: [edk2-devel] [PATCH V3 00/12] Migrate ArmVirtPkg modules to OvmfPkg

 

Hi Ard,

This way reduces the impact of MdePkg. We can try it.

 

Thanks

Abner

 


From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ard Biesheuvel <ardb@...>
Sent: Tuesday, October 5, 2021 5:30 PM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Chang, Abner (HPS SW/FW Technologist) <abner.chang@...>
Cc: Ard Biesheuvel <ardb+tianocore@...>; Leif Lindholm <leif@...>; Sami Mujawar <sami.mujawar@...>; Jiewen Yao <jiewen.yao@...>; Jordan Justen <jordan.l.justen@...>; Gerd Hoffmann <kraxel@...>; Schaefer, Daniel <daniel.schaefer@...>; Sunil V L <sunilvl@...>; Liming Gao <gaoliming@...>; Zhiguang Liu <zhiguang.liu@...>; Michael D Kinney <michael.d.kinney@...>
Subject: Re: [edk2-devel] [PATCH V3 00/12] Migrate ArmVirtPkg modules to OvmfPkg

 

On Thu, 30 Sept 2021 at 03:43, Abner Chang <abner.chang@...> wrote:
>
> In V3: Address comments on V2.
> In V2: Remove HPE license on the files that just moved around or
>        the changes in the file are just code removal.
>
> edk2 BZ #: 3665
> edk2 platform corresponding changes will be submitted after
> this pactch set is reviewed.
>
> This pacthes set is to migrate some modules from ArmVirtPkg
> to under OvmfPkg for the upcoming RiscVVirtPkg that can leverage
> those modules without the dependency with Arm*Pkg.
>
> The modules moved from ArmVirtPkg to OvmfPkg are,
> - FdtClientDxe
> - PciPcdProducerLib
> - HighMemDxe
> - QemuFwCfgLib
> - FdtPciHostBridgeLib
> - VirtioFdtDxe
>
> Below PCDs are moved to under MdePkg and leverage by RiscVVirtPkg.
> This change also remove the dependency on ArmPkg of OvmfPkg.
> - PcdPciIoTranslation
> - PcdPciIoTranslation
> - PcdPciMmio32(64)Translation
>
> Signed-off-by: Abner Chang <abner.chang@...>
> Cc: Ard Biesheuvel <ardb+tianocore@...>
> Cc: Leif Lindholm <leif@...>
> Cc: Sami Mujawar <sami.mujawar@...>
> Cc: Jiewen Yao <jiewen.yao@...>
> Cc: Jordan Justen <jordan.l.justen@...>
> Cc: Gerd Hoffmann <kraxel@...>
> Cc: Daniel Schaefer <daniel.schaefer@...>
> Cc: Sunil V L <sunilvl@...>
> Cc: Liming Gao <gaoliming@...>
> Cc: Zhiguang Liu <zhiguang.liu@...>
> Cc: Michael D Kinney <michael.d.kinney@...>
>
> Abner Chang (12):
>   ArmVirtPkg/FdtClintDxe: Move FdtClientDxe to EmbeddedPkg
>   MdePkg: Add PcdPciIoTranslation PCD
>   ArmPkg: Use PcdPciIoTranslation PCD from MdePkg
>   ArmVirtPkg/FdtPciPcdProducerLib: Relocate PciPcdProducerLib to OvmfPkg
>   ArmVirtPkg/HighMemDxe: Relocate HighMemDxe to OvmfPkg
>   OvmfPkg/HighMemDxe: Add RISC-V in the supported arch.
>   ArmVirtPkg/QemuFwCfgLib: Relocate QemuFwCfgLib to OvmfPkg
>   OvmfPkg/QemuFwCfgLibMMIO: Add RISC-V arch support
>   MdePkg: Add PcdPciMmio32(64)Translation PCDs
>   ArmVirtPkg/FdtPciHostBridgeLib: Relocate FdtPciHostBridgeLib to
>     OvmfPkg/Fdt
>   OvmfPkg/FdtPciHostBridgeLib: Add RISC-V in the supported arch.
>   ArmVirtPkg/VirtioFdtDxe: Relocate VirtioFdtDxe to OvmfPkg/Fdt
>

Hello all,

These patches look ok to me, but I wonder if the MdePkg maintainers
are happy taking these PCD declaration changes. Translations for PCIe
are typically defined per host bridge, and I would rather move away
from using PCDs for this entirely than 'promote' them by carrying them
in MdePkg.

As this issue is somewhat orthogonal to what Abner is trying to fix,
perhaps it is better to avoid MdePkg changes for now, and just
duplicate these PCDs into OvmfPkg. This is reasonable, given that we
know that QEMU only exposes a single host bridge.

The one in ArmPkg can hopefully be removed and replaced with something
that is more appropriate.


>  ArmPkg/ArmPkg.dec                             | 15 ++++++--------
>  ArmVirtPkg/ArmVirtPkg.dec                     |  3 ---
>  EmbeddedPkg/EmbeddedPkg.dec                   |  1 +
>  MdePkg/MdePkg.dec                             | 12 +++++++++++
>  ArmVirtPkg/ArmVirtCloudHv.dsc                 | 18 ++++++++---------
>  ArmVirtPkg/ArmVirtKvmTool.dsc                 | 18 ++++++++---------
>  ArmVirtPkg/ArmVirtQemu.dsc                    | 20 +++++++++----------
>  ArmVirtPkg/ArmVirtQemuKernel.dsc              | 20 +++++++++----------
>  ArmVirtPkg/ArmVirtXen.dsc                     |  2 +-
>  EmbeddedPkg/EmbeddedPkg.dsc                   |  1 +
>  ArmVirtPkg/ArmVirtCloudHv.fdf                 |  6 +++---
>  ArmVirtPkg/ArmVirtKvmTool.fdf                 |  6 +++---
>  ArmVirtPkg/ArmVirtXen.fdf                     |  2 +-
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc          |  6 +++---
>  .../ArmPciCpuIo2Dxe/ArmPciCpuIo2Dxe.inf       |  2 +-
>  .../ArmVirtGicArchLib/ArmVirtGicArchLib.inf   |  1 +
>  .../ArmVirtPL031FdtClientLib.inf              |  1 +
>  .../ArmVirtPsciResetSystemLib.inf             |  1 +
>  .../ArmVirtTimerFdtClientLib.inf              |  1 +
>  .../KvmtoolRtcFdtClientLib.inf                |  1 +
>  .../NorFlashKvmtoolLib/NorFlashKvmtoolLib.inf |  1 +
>  .../NorFlashQemuLib/NorFlashQemuLib.inf       |  1 +
>  .../XenAcpiPlatformDxe/XenAcpiPlatformDxe.inf |  1 +
>  ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.inf        |  1 +
>  .../Drivers}/FdtClientDxe/FdtClientDxe.inf    |  1 -
>  .../FdtPciHostBridgeLib.inf                   | 11 +++++-----
>  .../FdtPciPcdProducerLib.inf                  |  5 ++---
>  .../Fdt}/HighMemDxe/HighMemDxe.inf            |  7 ++++---
>  .../Fdt}/VirtioFdtDxe/VirtioFdtDxe.inf        |  2 +-
>  .../Library/QemuFwCfgLib/QemuFwCfgLibMmio.inf |  6 +++---
>  .../Include/Protocol/FdtClient.h              |  0
>  .../Drivers}/FdtClientDxe/FdtClientDxe.c      |  0
>  .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c |  0
>  .../FdtPciPcdProducerLib.c                    |  0
>  .../Fdt}/HighMemDxe/HighMemDxe.c              |  3 ++-
>  .../Fdt}/VirtioFdtDxe/VirtioFdtDxe.c          |  0
>  .../Library/QemuFwCfgLib/QemuFwCfgLibMmio.c   |  7 ++++---
>  Maintainers.txt                               |  6 ++++++
>  38 files changed, 106 insertions(+), 83 deletions(-)
>  rename {ArmVirtPkg => EmbeddedPkg/Drivers}/FdtClientDxe/FdtClientDxe.inf (92%)
>  rename {ArmVirtPkg/Library => OvmfPkg/Fdt}/FdtPciHostBridgeLib/FdtPciHostBridgeLib.inf (77%)
>  rename {ArmVirtPkg/Library => OvmfPkg/Fdt}/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf (87%)
>  rename {ArmVirtPkg => OvmfPkg/Fdt}/HighMemDxe/HighMemDxe.inf (83%)
>  rename {ArmVirtPkg => OvmfPkg/Fdt}/VirtioFdtDxe/VirtioFdtDxe.inf (92%)
>  rename ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf => OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.inf (86%)
>  rename {ArmVirtPkg => EmbeddedPkg}/Include/Protocol/FdtClient.h (100%)
>  rename {ArmVirtPkg => EmbeddedPkg/Drivers}/FdtClientDxe/FdtClientDxe.c (100%)
>  rename {ArmVirtPkg/Library => OvmfPkg/Fdt}/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c (100%)
>  rename {ArmVirtPkg/Library => OvmfPkg/Fdt}/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c (100%)
>  rename {ArmVirtPkg => OvmfPkg/Fdt}/HighMemDxe/HighMemDxe.c (95%)
>  rename {ArmVirtPkg => OvmfPkg/Fdt}/VirtioFdtDxe/VirtioFdtDxe.c (100%)
>  rename ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c => OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibMmio.c (93%)
>
> --
> 2.17.1
>
>
>
>
>
>


Re: [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating gHandleList

Dandan Bi
 

Hi Hua,

One minor comment inline, please check.



Thanks,
Dandan

-----Original Message-----
From: Ma, Hua <hua.ma@...>
Sent: Monday, October 11, 2021 6:45 PM
To: devel@edk2.groups.io
Cc: Ma, Hua <hua.ma@...>; Wang, Jian J <jian.j.wang@...>;
Liming Gao <gaoliming@...>; Bi, Dandan <dandan.bi@...>;
Ni, Ray <ray.ni@...>
Subject: [PATCH] MdeModulePkg/Core/Dxe: Acquire a lock when iterating
gHandleList

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3680

This patch fixes the following issue:

The global variable gHandleList is a linked list.
This list is locked when a entry is added or removed from the list, but there is no
lock when iterating this list in function CoreValidateHandle().
It can lead to "Handle.c (76): CR has Bad Signature" assertion if the iterated
entry in the list is just removed by other task during iterating.
Locking the list when iterating can fix this issue.

Cc: Jian J Wang <jian.j.wang@...>
Cc: Liming Gao <gaoliming@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Ray Ni <ray.ni@...>
Signed-off-by: Hua Ma <hua.ma@...>
---
MdeModulePkg/Core/Dxe/Hand/DriverSupport.c | 10 ++---
MdeModulePkg/Core/Dxe/Hand/Handle.c | 47 ++++++++++++++--------
MdeModulePkg/Core/Dxe/Hand/Handle.h | 3 +-
MdeModulePkg/Core/Dxe/Hand/Notify.c | 2 +-
4 files changed, 39 insertions(+), 23 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
index feabf12faf..eb8a765d2c 100644
--- a/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
+++ b/MdeModulePkg/Core/Dxe/Hand/DriverSupport.c
@@ -68,7 +68,7 @@ CoreConnectController (
//
// Make sure ControllerHandle is valid
//
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -154,7 +154,7 @@ CoreConnectController (
//
// Make sure the DriverBindingHandle is valid
//
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, TRUE);
if (EFI_ERROR (Status)) {
//
// Release the protocol lock on the handle database @@ -268,7 +268,7 @@
AddSortedDriverBindingProtocol (
//
// Make sure the DriverBindingHandle is valid
//
- Status = CoreValidateHandle (DriverBindingHandle);
+ Status = CoreValidateHandle (DriverBindingHandle, FALSE);
if (EFI_ERROR (Status)) {
return;
}
@@ -746,7 +746,7 @@ CoreDisconnectController (
//
// Make sure ControllerHandle is valid
//
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -755,7 +755,7 @@ CoreDisconnectController (
// Make sure ChildHandle is valid if it is not NULL
//
if (ChildHandle != NULL) {
- Status = CoreValidateHandle (ChildHandle);
+ Status = CoreValidateHandle (ChildHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.c
b/MdeModulePkg/Core/Dxe/Hand/Handle.c
index 6eccb41ecb..b4f389bb35 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.c
@@ -55,31 +55,46 @@ CoreReleaseProtocolLock (
Check whether a handle is a valid EFI_HANDLE

@param UserHandle The handle to check
+ @param IsLocked The protocol lock is acquried or not

@retval EFI_INVALID_PARAMETER The handle is NULL or not a valid
EFI_HANDLE.
+ @retval EFI_NOT_FOUND The handle is not found in the handle
database.
@retval EFI_SUCCESS The handle is valid EFI_HANDLE.

**/
EFI_STATUS
CoreValidateHandle (
- IN EFI_HANDLE UserHandle
+ IN EFI_HANDLE UserHandle,
+ IN BOOLEAN IsLocked
)
{
IHANDLE *Handle;
LIST_ENTRY *Link;
+ EFI_STATUS Status;

if (UserHandle == NULL) {
return EFI_INVALID_PARAMETER;
}

+ if (IsLocked) {
+ ASSERT_LOCKED(&gProtocolDatabaseLock);
+ } else {
+ CoreAcquireProtocolLock ();
+ }
+
+ Status = EFI_NOT_FOUND;
for (Link = gHandleList.BackLink; Link != &gHandleList; Link = Link->BackLink) {
Handle = CR (Link, IHANDLE, AllHandles, EFI_HANDLE_SIGNATURE);
if (Handle == (IHANDLE *) UserHandle) {
- return EFI_SUCCESS;
+ Status = EFI_SUCCESS;
+ break;
}
}

- return EFI_INVALID_PARAMETER;
+ if (!IsLocked) {
+ CoreReleaseProtocolLock ();
+ }
+ return Status;
}


@@ -428,7 +443,7 @@ CoreInstallProtocolInterfaceNotify (
//
InsertTailList (&gHandleList, &Handle->AllHandles);
} else {
- Status = CoreValidateHandle (Handle);
+ Status = CoreValidateHandle (Handle, TRUE);
if (EFI_ERROR (Status)) {
DEBUG((DEBUG_ERROR, "InstallProtocolInterface: input handle at 0x%x is
invalid\n", Handle));
goto Done;
@@ -723,7 +738,7 @@ CoreUninstallProtocolInterface (
//
// Check that UserHandle is a valid handle
//
- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -899,7 +914,7 @@ CoreGetProtocolInterface (
IHANDLE *Handle;
LIST_ENTRY *Link;

- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, TRUE);
[Dandan] As current all callers of CoreGetProtocolInterface () has acquired the lock, it's OK to pass TRUE to indicate the lock is acquired.
I am just thinking about following small question.
Could we add some comments to explain why it's true here and also notify that caller should also acquire the lock firstly by itself if there is any new caller added to call this function?

if (EFI_ERROR (Status)) {
return NULL;
}
@@ -1013,7 +1028,7 @@ CoreOpenProtocol (
//
// Check for invalid UserHandle
//
- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1023,11 +1038,11 @@ CoreOpenProtocol (
//
switch (Attributes) {
case EFI_OPEN_PROTOCOL_BY_CHILD_CONTROLLER :
- Status = CoreValidateHandle (ImageHandle);
+ Status = CoreValidateHandle (ImageHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1037,17 +1052,17 @@ CoreOpenProtocol (
break;
case EFI_OPEN_PROTOCOL_BY_DRIVER :
case EFI_OPEN_PROTOCOL_BY_DRIVER | EFI_OPEN_PROTOCOL_EXCLUSIVE :
- Status = CoreValidateHandle (ImageHandle);
+ Status = CoreValidateHandle (ImageHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
break;
case EFI_OPEN_PROTOCOL_EXCLUSIVE :
- Status = CoreValidateHandle (ImageHandle);
+ Status = CoreValidateHandle (ImageHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1249,16 +1264,16 @@ CoreCloseProtocol (
//
// Check for invalid parameters
//
- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
- Status = CoreValidateHandle (AgentHandle);
+ Status = CoreValidateHandle (AgentHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
if (ControllerHandle != NULL) {
- Status = CoreValidateHandle (ControllerHandle);
+ Status = CoreValidateHandle (ControllerHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
@@ -1443,7 +1458,7 @@ CoreProtocolsPerHandle (
UINTN ProtocolCount;
EFI_GUID **Buffer;

- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
diff --git a/MdeModulePkg/Core/Dxe/Hand/Handle.h
b/MdeModulePkg/Core/Dxe/Hand/Handle.h
index 83eb2b9f3a..b962d80a29 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Handle.h
+++ b/MdeModulePkg/Core/Dxe/Hand/Handle.h
@@ -251,7 +251,8 @@ CoreReleaseProtocolLock ( **/ EFI_STATUS
CoreValidateHandle (
- IN EFI_HANDLE UserHandle
+ IN EFI_HANDLE UserHandle,
+ IN BOOLEAN IsLocked
);

//
diff --git a/MdeModulePkg/Core/Dxe/Hand/Notify.c
b/MdeModulePkg/Core/Dxe/Hand/Notify.c
index 553413a350..f4e7c01e96 100644
--- a/MdeModulePkg/Core/Dxe/Hand/Notify.c
+++ b/MdeModulePkg/Core/Dxe/Hand/Notify.c
@@ -188,7 +188,7 @@ CoreReinstallProtocolInterface (
PROTOCOL_INTERFACE *Prot;
PROTOCOL_ENTRY *ProtEntry;

- Status = CoreValidateHandle (UserHandle);
+ Status = CoreValidateHandle (UserHandle, FALSE);
if (EFI_ERROR (Status)) {
return Status;
}
--
2.32.0.windows.2


[Patch V4 1/1] BaseTools: Change RealPath to AbsPath

Yuwei Chen
 

Currently the realpath is used when parse modules, which shows the
path with a drive letter in build log. In Windows 'subst' comand is
used to associates a path with a drive letter, when use the mapped
drive letter for build, with realpath function the build log will
have different disk letter info which will cause confusion. In this
situation, if use adspath function to show the path info, it will keep
same letter with the mapped drive letter, which avoids confusion.
This patch modifies the realpath to abspath.

Cc: Bob Feng <bob.c.feng@...>
Cc: Liming Gao <gaoliming@...>
Signed-off-by: Yuwei Chen <yuwei.chen@...>
---
BaseTools/Source/Python/Ecc/EccMain.py | 2 +-
BaseTools/Source/Python/GenFds/FfsInfStatement.py | 4 ++--
BaseTools/Source/Python/GenFds/GenFds.py | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/BaseTools/Source/Python/Ecc/EccMain.py b/BaseTools/Source/Python/Ecc/EccMain.py
index 72edbea3b883..a349cd80147f 100644
--- a/BaseTools/Source/Python/Ecc/EccMain.py
+++ b/BaseTools/Source/Python/Ecc/EccMain.py
@@ -105,7 +105,7 @@ class Ecc(object):

def InitDefaultConfigIni(self):
paths = map(lambda p: os.path.join(p, 'Ecc', 'config.ini'), sys.path)
- paths = (os.path.realpath('config.ini'),) + tuple(paths)
+ paths = (os.path.abspath('config.ini'),) + tuple(paths)
for path in paths:
if os.path.exists(path):
self.ConfigFile = path
diff --git a/BaseTools/Source/Python/GenFds/FfsInfStatement.py b/BaseTools/Source/Python/GenFds/FfsInfStatement.py
index 20573ca28d2f..568efb6d7685 100644
--- a/BaseTools/Source/Python/GenFds/FfsInfStatement.py
+++ b/BaseTools/Source/Python/GenFds/FfsInfStatement.py
@@ -707,8 +707,8 @@ class FfsInfStatement(FfsInfStatementClassObject):
FileName,
'DEBUG'
)
- OutputPath = os.path.realpath(OutputPath)
- DebugPath = os.path.realpath(DebugPath)
+ OutputPath = os.path.abspath(OutputPath)
+ DebugPath = os.path.abspath(DebugPath)
return OutputPath, DebugPath

## __GenSimpleFileSection__() method
diff --git a/BaseTools/Source/Python/GenFds/GenFds.py b/BaseTools/Source/Python/GenFds/GenFds.py
index c34104500059..17b71b7cd347 100644
--- a/BaseTools/Source/Python/GenFds/GenFds.py
+++ b/BaseTools/Source/Python/GenFds/GenFds.py
@@ -153,7 +153,7 @@ def GenFdsApi(FdsCommandDict, WorkSpaceDataBase=None):
FdfFilename = GenFdsGlobalVariable.ReplaceWorkspaceMacro(FdfFilename)

if FdfFilename[0:2] == '..':
- FdfFilename = os.path.realpath(FdfFilename)
+ FdfFilename = os.path.abspath(FdfFilename)
if not os.path.isabs(FdfFilename):
FdfFilename = mws.join(GenFdsGlobalVariable.WorkSpaceDir, FdfFilename)
if not os.path.exists(FdfFilename):
@@ -175,7 +175,7 @@ def GenFdsApi(FdsCommandDict, WorkSpaceDataBase=None):
ActivePlatform = GenFdsGlobalVariable.ReplaceWorkspaceMacro(ActivePlatform)

if ActivePlatform[0:2] == '..':
- ActivePlatform = os.path.realpath(ActivePlatform)
+ ActivePlatform = os.path.abspath(ActivePlatform)

if not os.path.isabs (ActivePlatform):
ActivePlatform = mws.join(GenFdsGlobalVariable.WorkSpaceDir, ActivePlatform)
@@ -299,7 +299,7 @@ def GenFdsApi(FdsCommandDict, WorkSpaceDataBase=None):
for Key in GenFdsGlobalVariable.OutputDirDict:
OutputDir = GenFdsGlobalVariable.OutputDirDict[Key]
if OutputDir[0:2] == '..':
- OutputDir = os.path.realpath(OutputDir)
+ OutputDir = os.path.abspath(OutputDir)

if OutputDir[1] != ':':
OutputDir = os.path.join (GenFdsGlobalVariable.WorkSpaceDir, OutputDir)
--
2.27.0.windows.1


Re: [PATCH v9 1/1] MdePkg/BaseLib: Add QuickSort function on BaseLib

IanX Kuo
 

@Liming Gao and @Kinney, Michael D

May I get one of yours help for the reviewed from MdePkg maintainer side ?
Have any concern, please also share for me.

Thanks,
Ian Kuo

-----Original Message-----
From: Ni, Ray <ray.ni@...>
Sent: Tuesday, October 12, 2021 10:22 AM
To: Kuo, IanX <ianx.kuo@...>; devel@edk2.groups.io
Cc: Chan, Amy <amy.chan@...>; Kinney, Michael D <michael.d.kinney@...>; Liming Gao <gaoliming@...>; Liu, Zhiguang <zhiguang.liu@...>
Subject: RE: [PATCH v9 1/1] MdePkg/BaseLib: Add QuickSort function on BaseLib

Reviewed-by: Ray Ni <ray.ni@...>

Ian, please take the approval from maintainers of MdePkg as the formal approval.

Thanks,
Ray

-----Original Message-----
From: Kuo, IanX <ianx.kuo@...>
Sent: Tuesday, October 12, 2021 8:06 AM
To: devel@edk2.groups.io
Cc: Chan, Amy <amy.chan@...>; Ni, Ray <ray.ni@...>; Kuo,
IanX <ianx.kuo@...>; Kinney, Michael D
<michael.d.kinney@...>; Liming Gao <gaoliming@...>;
Liu, Zhiguang <zhiguang.liu@...>
Subject: [PATCH v9 1/1] MdePkg/BaseLib: Add QuickSort function on
BaseLib

From: IanX Kuo <ianx.kuo@...>

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3675

Add QuickSort function into BaseLib

Cc: Ray Ni <ray.ni@...>
Cc: Michael D Kinney <michael.d.kinney@...>
Cc: Liming Gao <gaoliming@...>
Cc: Zhiguang Liu <zhiguang.liu@...>
Signed-off-by: IanX Kuo <ianx.kuo@...>
---
MdePkg/Include/Library/BaseLib.h | 49 ++++++++
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Library/BaseLib/QuickSort.c | 116 ++++++++++++++++++
.../Library/BaseLib/UnitTestHostBaseLib.inf | 3 +-
4 files changed, 168 insertions(+), 1 deletion(-) create mode 100644
MdePkg/Library/BaseLib/QuickSort.c

diff --git a/MdePkg/Include/Library/BaseLib.h
b/MdePkg/Include/Library/BaseLib.h
index 2452c1d92e..0ae0f4e6af 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -2856,6 +2856,55 @@ RemoveEntryList ( //

// Math Services

//

+/**

+ Prototype for comparison function for any two element types.

+

+ @param[in] Buffer1 The pointer to first buffer.

+ @param[in] Buffer2 The pointer to second buffer.

+

+ @retval 0 Buffer1 equal to Buffer2.

+ @return <0 Buffer1 is less than Buffer2.

+ @return >0 Buffer1 is greater than Buffer2.

+**/

+typedef

+INTN

+(EFIAPI *BASE_SORT_COMPARE)(

+ IN CONST VOID *Buffer1,

+ IN CONST VOID *Buffer2

+ );

+

+/**

+ This function is identical to perform QuickSort,

+ except that is uses the pre-allocated buffer so the in place
+ sorting does not need to

+ allocate and free buffers constantly.

+

+ Each element must be equal sized.

+

+ if BufferToSort is NULL, then ASSERT.

+ if CompareFunction is NULL, then ASSERT.

+ if BufferOneElement is NULL, then ASSERT.

+ if ElementSize is < 1, then ASSERT.

+

+ if Count is < 2 then perform no action.

+

+ @param[in, out] BufferToSort on call a Buffer of (possibly sorted) elements

+ on return a buffer of sorted
+ elements

+ @param[in] Count the number of elements in the buffer to sort

+ @param[in] ElementSize Size of an element in bytes

+ @param[in] CompareFunction The function to call to perform the comparison

+ of any 2 elements

+ @param[out] BufferOneElement Caller provided buffer whose size equals to ElementSize.

+ It's used by QuickSort() for swapping in sorting.

+**/

+VOID

+EFIAPI

+QuickSort (

+ IN OUT VOID *BufferToSort,

+ IN CONST UINTN Count,

+ IN CONST UINTN ElementSize,

+ IN BASE_SORT_COMPARE CompareFunction,

+ OUT VOID *BufferOneElement

+ );



/**

Shifts a 64-bit integer left between 0 and 63 bits. The low bits
are filled

diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
b/MdePkg/Library/BaseLib/BaseLib.inf
index 6efa5315b6..cebda3b210 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -32,6 +32,7 @@
SwapBytes16.c

LongJump.c

SetJump.c

+ QuickSort.c

RShiftU64.c

RRotU64.c

RRotU32.c

diff --git a/MdePkg/Library/BaseLib/QuickSort.c
b/MdePkg/Library/BaseLib/QuickSort.c
new file mode 100644
index 0000000000..a825c072b0
--- /dev/null
+++ b/MdePkg/Library/BaseLib/QuickSort.c
@@ -0,0 +1,116 @@
+/** @file

+ Math worker functions.

+

+ Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>

+ SPDX-License-Identifier: BSD-2-Clause-Patent

+

+**/

+

+#include "BaseLibInternals.h"

+

+/**

+ This function is identical to perform QuickSort,

+ except that is uses the pre-allocated buffer so the in place
+ sorting does not need to

+ allocate and free buffers constantly.

+

+ Each element must be equal sized.

+

+ if BufferToSort is NULL, then ASSERT.

+ if CompareFunction is NULL, then ASSERT.

+ if BufferOneElement is NULL, then ASSERT.

+ if ElementSize is < 1, then ASSERT.

+

+ if Count is < 2 then perform no action.

+

+ @param[in, out] BufferToSort on call a Buffer of (possibly sorted) elements

+ on return a buffer of sorted
+ elements

+ @param[in] Count the number of elements in the buffer to sort

+ @param[in] ElementSize Size of an element in bytes

+ @param[in] CompareFunction The function to call to perform the comparison

+ of any 2 elements

+ @param[out] BufferOneElement Caller provided buffer whose size equals to ElementSize.

+ It's used by QuickSort() for swapping in sorting.

+**/

+VOID

+EFIAPI

+QuickSort (

+ IN OUT VOID *BufferToSort,

+ IN CONST UINTN Count,

+ IN CONST UINTN ElementSize,

+ IN BASE_SORT_COMPARE CompareFunction,

+ OUT VOID *BufferOneElement

+ )

+{

+ VOID *Pivot;

+ UINTN LoopCount;

+ UINTN NextSwapLocation;

+

+ ASSERT (BufferToSort != NULL);

+ ASSERT (CompareFunction != NULL);

+ ASSERT (BufferOneElement != NULL);

+ ASSERT (ElementSize >= 1);

+

+ if (Count < 2) {

+ return;

+ }

+

+ NextSwapLocation = 0;

+

+ //

+ // pick a pivot (we choose last element)

+ //

+ Pivot = ((UINT8*) BufferToSort + ((Count - 1) * ElementSize));

+

+ //

+ // Now get the pivot such that all on "left" are below it

+ // and everything "right" are above it

+ //

+ for (LoopCount = 0; LoopCount < Count -1; LoopCount++) {

+ //

+ // if the element is less than or equal to the pivot

+ //

+ if (CompareFunction ((VOID*) ((UINT8*) BufferToSort +
+ ((LoopCount) * ElementSize)), Pivot) <= 0){

+ //

+ // swap

+ //

+ CopyMem (BufferOneElement, (UINT8*) BufferToSort +
+ (NextSwapLocation * ElementSize), ElementSize);

+ CopyMem ((UINT8*) BufferToSort + (NextSwapLocation *
+ ElementSize), (UINT8*) BufferToSort + ((LoopCount) *
ElementSize), ElementSize);

+ CopyMem ((UINT8*) BufferToSort + ((LoopCount)*ElementSize),
+ BufferOneElement, ElementSize);

+

+ //

+ // increment NextSwapLocation

+ //

+ NextSwapLocation++;

+ }

+ }

+ //

+ // swap pivot to it's final position (NextSwapLocation)

+ //

+ CopyMem (BufferOneElement, Pivot, ElementSize);

+ CopyMem (Pivot, (UINT8*) BufferToSort + (NextSwapLocation *
+ ElementSize), ElementSize);

+ CopyMem ((UINT8*) BufferToSort + (NextSwapLocation * ElementSize),
+ BufferOneElement, ElementSize);

+

+ //

+ // Now recurse on 2 partial lists. neither of these will have the
+ 'pivot' element

+ // IE list is sorted left half, pivot element, sorted right half...

+ //

+ if (NextSwapLocation >= 2) {

+ QuickSort (

+ BufferToSort,

+ NextSwapLocation,

+ ElementSize,

+ CompareFunction,

+ BufferOneElement

+ );

+ }

+

+ if ((Count - NextSwapLocation - 1) >= 2) {

+ QuickSort (

+ (UINT8 *)BufferToSort + (NextSwapLocation + 1) * ElementSize,

+ Count - NextSwapLocation - 1,

+ ElementSize,

+ CompareFunction,

+ BufferOneElement

+ );

+ }

+}

diff --git a/MdePkg/Library/BaseLib/UnitTestHostBaseLib.inf
b/MdePkg/Library/BaseLib/UnitTestHostBaseLib.inf
index eae1a7158d..d09bd12bef 100644
--- a/MdePkg/Library/BaseLib/UnitTestHostBaseLib.inf
+++ b/MdePkg/Library/BaseLib/UnitTestHostBaseLib.inf
@@ -1,7 +1,7 @@
## @file

# Base Library implementation for use with host based unit tests.

#

-# Copyright (c) 2007 - 2020, Intel Corporation. All rights
reserved.<BR>

+# Copyright (c) 2007 - 2021, Intel Corporation. All rights
+reserved.<BR>

# Portions copyright (c) 2008 - 2009, Apple Inc. All rights
reserved.<BR>

# Portions copyright (c) 2011 - 2013, ARM Ltd. All rights
reserved.<BR>

# Copyright (c) 2020, Hewlett Packard Enterprise Development LP. All
rights reserved.<BR>

@@ -33,6 +33,7 @@
SwapBytes16.c

LongJump.c

SetJump.c

+ QuickSort.c

RShiftU64.c

RRotU64.c

RRotU32.c

--
2.30.0.windows.1


[PATCH V9 4/4] OvmfPkg: Enable TDX in ResetVector

Min Xu
 

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

Intel's Trust Domain Extensions (Intel TDX) refers to an Intel technology
that extends Virtual Machines Extensions (VMX) and Multi-Key Total Memory
Encryption (MKTME) with a new kind of virutal machines guest called a
Trust Domain (TD). A TD is desinged to run in a CPU mode that protects the
confidentiality of TD memory contents and the TD's CPU state from other
software, including the hosting Virtual-Machine Monitor (VMM), unless
explicitly shared by the TD itself.

Note: Intel TDX is only available on X64, so the Tdx related changes are
in X64 path. In IA32 path, there may be null stub to make the build
success.

This patch includes below major changes.

1. Ia32/IntelTdx.asm
IntelTdx.asm includes below routines used in ResetVector
- IsTdx
Check if the running system is Tdx guest.

- InitTdxWorkarea
It initialize the TDX_WORK_AREA. Because it is called by both BSP and
APs and to avoid the race condition, only BSP can initialize the
WORK_AREA. AP will wait until the field of TDX_WORK_AREA_PGTBL_READY
is set.

- ReloadFlat32
After reset all CPUs in TDX are initialized to 32-bit protected mode.
But GDT register is not set. So this routine loads the GDT and set the
CR0, then jump to Flat 32 protected mode again. After that CR4 and
other registers are set.

- InitTdx
This routine wrap above 3 routines together to do Tdx initialization
in ResetVector phase.

- IsTdxEnabled
It is a OneTimeCall to probe if TDX is enabled by checking the
CC_WORK_AREA.

- CheckTdxFeaturesBeforeBuildPagetables
This routine is called to check if it is Non-TDX guest, TDX-Bsp or
TDX-APs. Because in TDX guest all the initialization is done by BSP
(including the page tables). APs should not build the tables.

- TdxPostBuildPageTables
It is called after Page Tables are built by BSP.
byte[TDX_WORK_AREA_PGTBL_READY] is set by BSP to indicate APs can
leave spin and go.

2. Ia32/PageTables64.asm
As described above only the TDX BSP build the page tables. So
PageTables64.asm is updated to make sure only TDX BSP build the
PageTables. TDX APs will skip the page table building and set Cr3
directly.

3. Ia16/ResetVectorVtf0.asm
In Tdx all CPUs "reset" to run on 32-bit protected mode with flat
descriptor (paging disabled). But in Non-Td guest the initial state of
CPUs is 16-bit real mode. To resolve this conflict, BITS 16/32 is used
in the ResetVectorVtf0.asm. It checks the 32-bit protected mode or 16-bit
real mode, then jump to the corresponding entry point.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Gerd Hoffmann <kraxel@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Signed-off-by: Min Xu <min.m.xu@...>
---
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 20 ++
OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm | 11 +
OvmfPkg/ResetVector/Ia32/IntelTdx.asm | 235 +++++++++++++++++++
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 18 ++
OvmfPkg/ResetVector/Main.asm | 10 +
OvmfPkg/ResetVector/ResetVector.nasmb | 1 +
6 files changed, 295 insertions(+)
create mode 100644 OvmfPkg/ResetVector/Ia32/IntelTdx.asm

diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 7be43fb44a69..dee2e3f9de31 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -177,10 +177,30 @@ resetVector:
;
; This is where the processor will begin execution
;
+; In IA32 we follow the standard reset vector flow. While in X64, Td guest
+; may be supported. Td guest requires the startup mode to be 32-bit
+; protected mode but the legacy VM startup mode is 16-bit real mode.
+; To make NASM generate such shared entry code that behaves correctly in
+; both 16-bit and 32-bit mode, more BITS directives are added.
+;
+%ifdef ARCH_IA32
nop
nop
jmp EarlyBspInitReal16

+%else
+
+ mov eax, cr0
+ test al, 1
+ jz .Real
+BITS 32
+ jmp Main32
+BITS 16
+.Real:
+ jmp EarlyBspInitReal16
+
+%endif
+
ALIGN 16

fourGigabytes:
diff --git a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
index c6d0d898bcd1..eb3546668ef8 100644
--- a/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
+++ b/OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm
@@ -21,6 +21,17 @@ Transition32FlatTo64Flat:
bts eax, 5 ; enable PAE
mov cr4, eax

+ ;
+ ; In TDX LME has already been set. So we're done and jump to enable
+ ; paging directly if Tdx is enabled.
+ ; EBX is cleared because in the later it will be used to check if
+ ; the second step of the SEV-ES mitigation is to be performed.
+ ;
+ xor ebx, ebx
+ OneTimeCall IsTdxEnabled
+ test eax, eax
+ jnz EnablePaging
+
mov ecx, 0xc0000080
rdmsr
bts eax, 8 ; set LME
diff --git a/OvmfPkg/ResetVector/Ia32/IntelTdx.asm b/OvmfPkg/ResetVector/Ia32/IntelTdx.asm
new file mode 100644
index 000000000000..f67b1bcf0b2e
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/IntelTdx.asm
@@ -0,0 +1,235 @@
+;------------------------------------------------------------------------------
+; @file
+; Intel TDX routines
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+%define SEC_DEFAULT_CR0 0x00000023
+%define SEC_DEFAULT_CR4 0x640
+%define VM_GUEST_TDX 2
+
+BITS 32
+
+;
+; Check if it is Intel Tdx
+;
+; Modified: EAX, EBX, ECX, EDX
+;
+; If it is Intel Tdx, EAX is 1
+; If it is not Intel Tdx, EAX is 0
+;
+IsTdx:
+ ;
+ ; CPUID (0)
+ ;
+ mov eax, 0
+ cpuid
+ cmp ebx, 0x756e6547 ; "Genu"
+ jne IsNotTdx
+ cmp edx, 0x49656e69 ; "ineI"
+ jne IsNotTdx
+ cmp ecx, 0x6c65746e ; "ntel"
+ jne IsNotTdx
+
+ ;
+ ; CPUID (1)
+ ;
+ mov eax, 1
+ cpuid
+ test ecx, 0x80000000
+ jz IsNotTdx
+
+ ;
+ ; CPUID[0].EAX >= 0x21?
+ ;
+ mov eax, 0
+ cpuid
+ cmp eax, 0x21
+ jl IsNotTdx
+
+ ;
+ ; CPUID (0x21,0)
+ ;
+ mov eax, 0x21
+ mov ecx, 0
+ cpuid
+
+ cmp ebx, 0x65746E49 ; "Inte"
+ jne IsNotTdx
+ cmp edx, 0x5844546C ; "lTDX"
+ jne IsNotTdx
+ cmp ecx, 0x20202020 ; " "
+ jne IsNotTdx
+
+ mov eax, 1
+ jmp ExitIsTdx
+
+IsNotTdx:
+ xor eax, eax
+
+ExitIsTdx:
+
+ OneTimeCallRet IsTdx
+
+;
+; Initialize work area if it is Tdx guest. Detailed definition is in
+; OvmfPkg/Include/WorkArea.h.
+; BSP and APs all go here. Only BSP initialize this work area.
+;
+; Param[in] EBP[5:0] CPU Supported GPAW (48 or 52)
+; Param[in] ESI[31:0] vCPU ID (BSP is 0, others are AP)
+;
+; Modified: EBP
+;
+InitTdxWorkarea:
+
+ ;
+ ; First check if it is Tdx
+ ;
+ OneTimeCall IsTdx
+
+ test eax, eax
+ jz ExitInitTdxWorkarea
+
+ cmp esi, 0
+ je TdxBspEntry
+
+ ;
+ ; In Td guest, BSP/AP shares the same entry point
+ ; BSP builds up the page table, while APs shouldn't do the same task.
+ ; Instead, APs just leverage the page table which is built by BSP.
+ ; APs will wait until the page table is ready.
+ ;
+TdxApWait:
+ cmp byte[TDX_WORK_AREA_PGTBL_READY], 0
+ je TdxApWait
+ jmp ExitInitTdxWorkarea
+
+TdxBspEntry:
+ ;
+ ; Set Type of WORK_AREA_GUEST_TYPE so that the following code can use
+ ; these information.
+ ;
+ mov byte[WORK_AREA_GUEST_TYPE], VM_GUEST_TDX
+
+ ;
+ ; EBP[5:0] CPU supported GPA width
+ ;
+ and ebp, 0x3f
+ mov DWORD[TDX_WORK_AREA_GPAW], ebp
+
+ExitInitTdxWorkarea:
+ OneTimeCallRet InitTdxWorkarea
+
+;
+; Load the GDT and set the CR0.
+;
+; Modified: EAX, EBX, CR0, CR4, DS, ES, FS, GS, SS, CS
+;
+ReloadFlat32:
+
+ cli
+ mov ebx, ADDR_OF(gdtr)
+ lgdt [ebx]
+
+ mov eax, SEC_DEFAULT_CR0
+ mov cr0, eax
+
+ jmp LINEAR_CODE_SEL:dword ADDR_OF(jumpToFlat32BitAndLandHere)
+
+jumpToFlat32BitAndLandHere:
+
+ mov eax, SEC_DEFAULT_CR4
+ mov cr4, eax
+
+ debugShowPostCode POSTCODE_32BIT_MODE
+
+ mov ax, LINEAR_SEL
+ mov ds, ax
+ mov es, ax
+ mov fs, ax
+ mov gs, ax
+ mov ss, ax
+
+ OneTimeCallRet ReloadFlat32
+
+;
+; Tdx initialization after entering into ResetVector
+;
+; Modified: EAX, EBX, ECX, EDX, EBP, EDI, ESP
+;
+InitTdx:
+ ;
+ ; Save EBX in EBP because EBX will be changed in ReloadFlat32
+ ;
+ mov ebp, ebx
+
+ ;
+ ; First load the GDT and jump to Flat32 mode
+ ;
+ OneTimeCall ReloadFlat32
+
+ ;
+ ; Initialization of Tdx work area
+ ;
+ OneTimeCall InitTdxWorkarea
+
+ OneTimeCallRet InitTdx
+
+;
+; Check TDX features, TDX or TDX-BSP or TDX-APs?
+;
+; By design TDX BSP is reponsible for initializing the PageTables.
+; After PageTables are ready, byte[TDX_WORK_AREA_PGTBL_READY] is set to 1.
+; APs will spin when byte[TDX_WORK_AREA_PGTBL_READY] is 0 until it is set to 1.
+;
+; When this routine is run on TDX BSP, byte[TDX_WORK_AREA_PGTBL_READY] should be 0.
+; When this routine is run on TDX APs, byte[TDX_WORK_AREA_PGTBL_READY] should be 1.
+;
+;
+; Modified: EAX, EDX
+;
+; 0-NonTdx, 1-TdxBsp, 2-TdxAps
+;
+CheckTdxFeaturesBeforeBuildPagetables:
+ xor eax, eax
+ cmp byte[WORK_AREA_GUEST_TYPE], VM_GUEST_TDX
+ jne NotTdx
+
+ xor edx, edx
+ mov al, byte[TDX_WORK_AREA_PGTBL_READY]
+ inc eax
+
+NotTdx:
+ OneTimeCallRet CheckTdxFeaturesBeforeBuildPagetables
+
+;
+; Set byte[TDX_WORK_AREA_PGTBL_READY] to 1
+;
+TdxPostBuildPageTables:
+ cmp byte[WORK_AREA_GUEST_TYPE], VM_GUEST_TDX
+ jne ExitTdxPostBuildPageTables
+ mov byte[TDX_WORK_AREA_PGTBL_READY], 1
+
+ExitTdxPostBuildPageTables:
+ OneTimeCallRet TdxPostBuildPageTables
+
+;
+; Check if TDX is enabled
+;
+; Modified: EAX
+;
+; If TDX is enabled then EAX will be 1
+; If TDX is disabled then EAX will be 0.
+;
+IsTdxEnabled:
+ xor eax, eax
+ cmp byte[WORK_AREA_GUEST_TYPE], VM_GUEST_TDX
+ jne TdxNotEnabled
+ mov eax, 1
+
+TdxNotEnabled:
+ OneTimeCallRet IsTdxEnabled
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 02528221e560..317cad430f29 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -37,10 +37,23 @@ BITS 32
PAGE_READ_WRITE + \
PAGE_PRESENT)

+%define TDX_BSP 1
+%define TDX_AP 2
+
;
; Modified: EAX, EBX, ECX, EDX
;
SetCr3ForPageTables64:
+ ; Check the TDX features.
+ ; If it is TDX APs, then jump to SetCr3 directly.
+ ; In TD guest the initialization is done by BSP, including building
+ ; the page tables. APs will spin on until byte[TDX_WORK_AREA_PGTBL_READY]
+ ; is set.
+ OneTimeCall CheckTdxFeaturesBeforeBuildPagetables
+ cmp eax, TDX_BSP
+ je ClearOvmfPageTables
+ cmp eax, TDX_AP
+ je SetCr3

; Check whether the SEV is active and populate the SevEsWorkArea
OneTimeCall CheckSevFeatures
@@ -50,6 +63,7 @@ SetCr3ForPageTables64:
; the page table build below.
OneTimeCall GetSevCBitMaskAbove31

+ClearOvmfPageTables:
;
; For OVMF, build some initial page tables at
; PcdOvmfSecPageTablesBase - (PcdOvmfSecPageTablesBase + 0x6000).
@@ -101,6 +115,10 @@ pageTableEntriesLoop:
; Clear the C-bit from the GHCB page if the SEV-ES is enabled.
OneTimeCall SevClearPageEncMaskForGhcbPage

+ ; TDX will do some PostBuildPages task, such as setting
+ ; byte[TDX_WORK_AREA_PGTBL_READY].
+ OneTimeCall TdxPostBuildPageTables
+
SetCr3:
;
; Set CR3 now that the paging structures are available
diff --git a/OvmfPkg/ResetVector/Main.asm b/OvmfPkg/ResetVector/Main.asm
index a501fbe880f2..07033e9470b5 100644
--- a/OvmfPkg/ResetVector/Main.asm
+++ b/OvmfPkg/ResetVector/Main.asm
@@ -42,6 +42,16 @@ BITS 32
; work area when detected.
mov byte[WORK_AREA_GUEST_TYPE], 0

+ jmp SearchBfv
+
+;
+; Entry point of Main32
+;
+Main32:
+ OneTimeCall InitTdx
+
+SearchBfv:
+
%endif

;
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 711e0dac4cdf..75ec800af089 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -107,6 +107,7 @@
%include "Ia32/Flat32ToFlat64.asm"
%include "Ia32/AmdSev.asm"
%include "Ia32/PageTables64.asm"
+%include "Ia32/IntelTdx.asm"
%endif

%include "Ia16/Real16ToFlat32.asm"
--
2.29.2.windows.2


[PATCH V9 1/4] OvmfPkg: Copy Main.asm from UefiCpuPkg to OvmfPkg's ResetVector

Min Xu
 

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

Previously OvmfPkg/ResetVector uses the Main.asm in
UefiCpuPkg/ReseteVector/Vtf0. In this Main.asm there is only Main16
entry point.

This patch-set is to introduce Intel TDX into Ovmf. Main32 entry point
is needed in Main.asm by Intel TDX. To reduce the complexity of Main.asm
in UefiCpuPkg, OvmfPkg create its own Main.asm to meet the requirement
of Intel TDX.

UefiCpuPkg/ResetVector/Vtf0/main.asm -> OvmfPkg/ResetVector/Main.asm

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Gerd Hoffmann <kraxel@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Signed-off-by: Min Xu <min.m.xu@...>
---
OvmfPkg/ResetVector/Main.asm | 103 +++++++++++++++++++++++++++++++++++
1 file changed, 103 insertions(+)
create mode 100644 OvmfPkg/ResetVector/Main.asm

diff --git a/OvmfPkg/ResetVector/Main.asm b/OvmfPkg/ResetVector/Main.asm
new file mode 100644
index 000000000000..ae90a148fce7
--- /dev/null
+++ b/OvmfPkg/ResetVector/Main.asm
@@ -0,0 +1,103 @@
+;------------------------------------------------------------------------------
+; @file
+; Main routine of the pre-SEC code up through the jump into SEC
+;
+; Copyright (c) 2008 - 2009, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+
+BITS 16
+
+;
+; Modified: EBX, ECX, EDX, EBP
+;
+; @param[in,out] RAX/EAX Initial value of the EAX register
+; (BIST: Built-in Self Test)
+; @param[in,out] DI 'BP': boot-strap processor, or
+; 'AP': application processor
+; @param[out] RBP/EBP Address of Boot Firmware Volume (BFV)
+; @param[out] DS Selector allowing flat access to all addresses
+; @param[out] ES Selector allowing flat access to all addresses
+; @param[out] FS Selector allowing flat access to all addresses
+; @param[out] GS Selector allowing flat access to all addresses
+; @param[out] SS Selector allowing flat access to all addresses
+;
+; @return None This routine jumps to SEC and does not return
+;
+Main16:
+ OneTimeCall EarlyInit16
+
+ ;
+ ; Transition the processor from 16-bit real mode to 32-bit flat mode
+ ;
+ OneTimeCall TransitionFromReal16To32BitFlat
+
+BITS 32
+
+ ;
+ ; Search for the Boot Firmware Volume (BFV)
+ ;
+ OneTimeCall Flat32SearchForBfvBase
+
+ ;
+ ; EBP - Start of BFV
+ ;
+
+ ;
+ ; Search for the SEC entry point
+ ;
+ OneTimeCall Flat32SearchForSecEntryPoint
+
+ ;
+ ; ESI - SEC Core entry point
+ ; EBP - Start of BFV
+ ;
+
+%ifdef ARCH_IA32
+
+ ;
+ ; Restore initial EAX value into the EAX register
+ ;
+ mov eax, esp
+
+ ;
+ ; Jump to the 32-bit SEC entry point
+ ;
+ jmp esi
+
+%else
+
+ ;
+ ; Transition the processor from 32-bit flat mode to 64-bit flat mode
+ ;
+ OneTimeCall Transition32FlatTo64Flat
+
+BITS 64
+
+ ;
+ ; Some values were calculated in 32-bit mode. Make sure the upper
+ ; 32-bits of 64-bit registers are zero for these values.
+ ;
+ mov rax, 0x00000000ffffffff
+ and rsi, rax
+ and rbp, rax
+ and rsp, rax
+
+ ;
+ ; RSI - SEC Core entry point
+ ; RBP - Start of BFV
+ ;
+
+ ;
+ ; Restore initial EAX value into the RAX register
+ ;
+ mov rax, rsp
+
+ ;
+ ; Jump to the 64-bit SEC entry point
+ ;
+ jmp rsi
+
+%endif
--
2.29.2.windows.2


[PATCH V9 0/4] Add Intel TDX support in OvmfPkg/ResetVector

Min Xu
 

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

Intel's Trust Domain Extensions (Intel TDX) refers to an Intel technology
that extends Virtual Machines Extensions (VMX) and Multi-Key Total Memory
Encryption (MKTME) with a new kind of virutal machines guest called a
Trust Domain (TD). A TD is desinged to run in a CPU mode that protects the
confidentiality of TD memory contents and the TD's CPU state from other
software, including the hosting Virtual-Machine Monitor (VMM), unless
explicitly shared by the TD itself.

The patch-sets to support Intel TDX in OvmfPkg is split into several
waves. This is wave-1 which adds Intel TDX support in OvmfPkg/ResetVector.
Note: TDX only works in X64.

Patch #1: Ovmf uses its own Main.asm to reduce the complexity of Main.asm
in UefiCpuPkg

Patch #2: WORK_AREA_GUEST_TYPE is cleared in Main.asm instead of in
WORK_AREA_GUEST_TYPE.

Patch #3: Introduce IntelTdxMetadata.asm which describes the information
about the image for VMM use.

Patch #4: Enable TDX in OvmfPkg/ResetVector for ARCH_X64.

[TDX]: https://software.intel.com/content/dam/develop/external/us/en/
documents/tdx-whitepaper-final9-17.pdf

[TDVF]: https://software.intel.com/content/dam/develop/external/us/en/
documents/tdx-virtual-firmware-design-guide-rev-1.pdf

Code is at https://github.com/mxu9/edk2/tree/tdvf_wave1.v9

v9 changes:
- Introduce IntelTdxMetadata.asm in a separate commit.
- Use absolute offset for the start of TdxMetadata so that VMM can
easily reach to the start of the metadata.

v8 changes:
- Create a separate commit for Main.asm.
- Create a separate commit for the clearance of WORK_AREA_GUEST_TYPE.
- Fix some inaccurate comments.

v7 changes:
- Refine the offset of TdxMetadata and remove the definition of
PcdOvmfImageSizeInKB
- Use MOV CR* instead of smsw in ResetVector
- Remove the new field (SubType) in
CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER.

v6 changes:
- Remove the 5-level paging support. 5-level paging enabling is *NOT*
super critical for TDX enabling at this moment. It will be enabled
later in a separate patch.
- Add a new field (SubType) in CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER
to record the VM Guest SubType.
- In Main16 entry point, after TransitionFromReal16To32BitFlat,
WORK_AREA_GUEST_TYPE is cleared to 0. WORK_AREA_GUEST_TYPE was
previously cleared in SetCr3ForPageTables64 (see commit ab77b60).
This doesn't work after TDX is introduced in Ovmf. It is because all
TDX CPUs (BSP and APs) start to run from 0xfffffff0. In previous code
WORK_AREA_GUEST_TYPE will be cleared multi-times in TDX guest. So for
SEV and Legacy guest it is moved to Main16 entry point (after
TransitionFromReal16To32BitFlat). For TDX guest WORK_AREA_GUEST_TYPE
is cleared and set in InitTdxWorkarea.
- Make the return result of IsTdx be consistent with IsTdxEnabled.
- Fix some typo in the code comments.

v5 changes:
- Remove the changes of OVMF_WORK_AREA because Commit ab77b60 covers
those changes.
- Refine the TDX related changes in PageTables64.asm and
Flat32ToFlat64.asm.
- Add CheckTdxFeaturesBeforeBuildPagetables to check Non-Tdx, Tdx-BSP or
Tdx-APs. This routine is called before building page tables.

v4 changes:
- Refine the PageTables64.asm and Flat32ToFlat64.asm to enable TDX.
- Refine SEV_ES_WORK_AREA so that SEV/TDX/Legach guest all can use this
memory region. https://edk2.groups.io/g/devel/message/78345 is the
discussion.
- AmdSev.asm is removed because Brijesh Singh has done it in
https://edk2.groups.io/g/devel/message/78241.

v3 changes:
- Refine PageTables64.asm and Flat32ToFlat64.asm based on the review
comments in [ReviewComment-1] and [ReviewComment-2].
- SEV codes are in AmdSev.asm
- TDX codes are in IntelTdx.asm
- Main.asm is created in OvmfPkg/ResetVector. The one in
UefiCpuPkg/ResetVector/Vtf0 is not used.
- Init32.asm/ReloadFlat32.asm in UefiCpuPkg/ResetVector/Vtf0/Ia32 are
deleted. They're moved to OvmfPkg/ResetVector/Ia32.
- InitTdx.asm is renamed to InteTdx.asm

v2 changes:
- Move InitTdx.asm and ReloadFlat32.asm from UefiCpuPkg/ResetVector/Vtf0
to OvmfPkg/ResetVector. Init32.asm is created which is a null stub of
32-bit initialization. In Main32 just simply call Init32. It makes
the Main.asm in UefiCpuPkg/ResetVector clean and clear.
- Init32.asm/InitTdx.asm/ReloadFlat32.asm are created under
OvmfPkg/ResetVector/Ia32.
- Update some descriptions of the patch-sets.
- Update the REF link in cover letter.
- Add Ard Biesheuvel in Cc list.

v1: https://edk2.groups.io/g/devel/message/77675

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Gerd Hoffmann <kraxel@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Signed-off-by: Min Xu <min.m.xu@...>

Min Xu (4):
OvmfPkg: Copy Main.asm from UefiCpuPkg to OvmfPkg's ResetVector
OvmfPkg: Clear WORK_AREA_GUEST_TYPE in Main.asm
OvmfPkg: Add IntelTdxMetadata.asm
OvmfPkg: Enable TDX in ResetVector

OvmfPkg/OvmfPkg.dec | 9 +
OvmfPkg/OvmfPkgDefines.fdf.inc | 9 +
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 39 +++
OvmfPkg/ResetVector/Ia32/Flat32ToFlat64.asm | 11 +
OvmfPkg/ResetVector/Ia32/IntelTdx.asm | 235 +++++++++++++++++++
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 22 +-
OvmfPkg/ResetVector/Main.asm | 121 ++++++++++
OvmfPkg/ResetVector/ResetVector.inf | 9 +
OvmfPkg/ResetVector/ResetVector.nasmb | 28 +++
OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm | 115 +++++++++
10 files changed, 594 insertions(+), 4 deletions(-)
create mode 100644 OvmfPkg/ResetVector/Ia32/IntelTdx.asm
create mode 100644 OvmfPkg/ResetVector/Main.asm
create mode 100644 OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm

--
2.29.2.windows.2


[PATCH V9 3/4] OvmfPkg: Add IntelTdxMetadata.asm

Min Xu
 

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

In TDX when host VMM creates a new guest TD, some initial set of
TD-private pages are added using the TDH.MEM.PAGE.ADD function. These
pages typically contain Virtual BIOS code and data along with some clear
pages for stacks and heap. In the meanwhile, some configuration data
need be measured by host VMM. Tdx Metadata is designed for this purpose
to indicate host VMM how to do the above tasks.

More detailed information of Metadata is in [TDVF] Section 11.

Tdx Metadata describes the information about the image for VMM use.
For example, the base address and length of the TdHob, Bfv, Cfv, etc.
The offset of the Metadata is stored in a GUID-ed structure which is
appended in the GUID-ed chain from a fixed GPA (0xffffffd0).

In this commit there are 2 new definitions of BFV & CFV.
Tdx Virtual Firmware (TDVF) includes one Firmware Volume (FV) known
as the Boot Firmware Volume (BFV). The FV format is defined in the
UEFI Platform Initialization (PI) spec. BFV includes all TDVF
components required during boot.

TDVF also include a configuration firmware volume (CFV) that is
separated from the BFV. The reason is because the CFV is measured in
RTMR, while the BFV is measured in MRTD.

In practice BFV is the code part of Ovmf image (OVMF_CODE.fd). CFV is
the vars part of Ovmf image (OVMF_VARS.fd).

Since AMD SEV has already defined some SEV specific memory region in
MEMFD. TDX re-uses some of the memory regions defined by SEV.
- MailBox : PcdOvmfSecGhcbBackupBase|PcdOvmfSecGhcbBackupSize
- TdHob : PcdOvmfSecGhcbBase|PcdOvmfSecGhcbSize

[TDVF] https://software.intel.com/content/dam/develop/external/us/en/
documents/tdx-virtual-firmware-design-guide-rev-1.pdf

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Gerd Hoffmann <kraxel@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Signed-off-by: Min Xu <min.m.xu@...>
---
OvmfPkg/OvmfPkg.dec | 9 ++
OvmfPkg/OvmfPkgDefines.fdf.inc | 9 ++
OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 19 +++
OvmfPkg/ResetVector/ResetVector.inf | 9 ++
OvmfPkg/ResetVector/ResetVector.nasmb | 27 +++++
OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm | 115 +++++++++++++++++++
6 files changed, 188 insertions(+)
create mode 100644 OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 1be8d5dccbc7..340d83f794d0 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -340,6 +340,15 @@
# header definition.
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader|4|UINT32|0x51

+ ## The base address and size of the TDX Cfv base and size.
+ gUefiOvmfPkgTokenSpaceGuid.PcdCfvBase|0|UINT32|0x52
+ gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataOffset|0|UINT32|0x53
+ gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize|0|UINT32|0x54
+
+ ## The base address and size of the TDX Bfv base and size.
+ gUefiOvmfPkgTokenSpaceGuid.PcdBfvBase|0|UINT32|0x55
+ gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataOffset|0|UINT32|0x56
+ gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataSize|0|UINT32|0x57

[PcdsDynamic, PcdsDynamicEx]
gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
diff --git a/OvmfPkg/OvmfPkgDefines.fdf.inc b/OvmfPkg/OvmfPkgDefines.fdf.inc
index 3b5e45253916..6170c5993ce5 100644
--- a/OvmfPkg/OvmfPkgDefines.fdf.inc
+++ b/OvmfPkg/OvmfPkgDefines.fdf.inc
@@ -9,6 +9,7 @@
##

DEFINE BLOCK_SIZE = 0x1000
+DEFINE VARS_OFFSET = 0

#
# A firmware binary built with FD_SIZE_IN_KB=1024, and a firmware binary built
@@ -88,6 +89,14 @@ SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize = $(VARS_SPARE_
# Computing Work Area header defined in the Include/WorkArea.h
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader = 4

+SET gUefiOvmfPkgTokenSpaceGuid.PcdCfvBase = $(FW_BASE_ADDRESS)
+SET gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataOffset = $(VARS_OFFSET)
+SET gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize = $(VARS_SIZE)
+
+SET gUefiOvmfPkgTokenSpaceGuid.PcdBfvBase = $(CODE_BASE_ADDRESS)
+SET gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataOffset = $(VARS_SIZE)
+SET gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataSize = $(CODE_SIZE)
+
!if $(SMM_REQUIRE) == TRUE
SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase
diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
index 7ec3c6e980c3..7be43fb44a69 100644
--- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
+++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
@@ -47,6 +47,25 @@ TIMES (15 - ((guidedStructureEnd - guidedStructureStart + 15) % 16)) DB 0
;
guidedStructureStart:

+%ifdef ARCH_X64
+;
+; TDX Metadata offset block
+;
+; TdxMetadata.asm is included in ARCH_X64 because Inte TDX is only
+; available in ARCH_X64. Below block describes the offset of
+; TdxMetadata block in Ovmf image
+;
+; GUID : e47a6535-984a-4798-865e-4685a7bf8ec2
+;
+tdxMetadataOffsetStart:
+ DD fourGigabytes - TdxMetadataGuid - 16
+ DW tdxMetadataOffsetEnd - tdxMetadataOffsetStart
+ DB 0x35, 0x65, 0x7a, 0xe4, 0x4a, 0x98, 0x98, 0x47
+ DB 0x86, 0x5e, 0x46, 0x85, 0xa7, 0xbf, 0x8e, 0xc2
+tdxMetadataOffsetEnd:
+
+%endif
+
; SEV Hash Table Block
;
; This describes the guest ram area where the hypervisor should
diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
index a2520dde5508..320e5f2c6527 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -44,6 +44,15 @@
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdCfvBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataOffset
+ gUefiOvmfPkgTokenSpaceGuid.PcdCfvRawDataSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdBfvBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataOffset
+ gUefiOvmfPkgTokenSpaceGuid.PcdBfvRawDataSize

[FixedPcd]
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index d1d800c56745..711e0dac4cdf 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -67,6 +67,31 @@
%error "This implementation inherently depends on PcdOvmfSecGhcbBase not straddling a 2MB boundary"
%endif

+ %define TDX_BFV_RAW_DATA_OFFSET FixedPcdGet32 (PcdBfvRawDataOffset)
+ %define TDX_BFV_RAW_DATA_SIZE FixedPcdGet32 (PcdBfvRawDataSize)
+ %define TDX_BFV_MEMORY_BASE FixedPcdGet32 (PcdBfvBase)
+ %define TDX_BFV_MEMORY_SIZE FixedPcdGet32 (PcdBfvRawDataSize)
+
+ %define TDX_CFV_RAW_DATA_OFFSET FixedPcdGet32 (PcdCfvRawDataOffset)
+ %define TDX_CFV_RAW_DATA_SIZE FixedPcdGet32 (PcdCfvRawDataSize)
+ %define TDX_CFV_MEMORY_BASE FixedPcdGet32 (PcdCfvBase),
+ %define TDX_CFV_MEMORY_SIZE FixedPcdGet32 (PcdCfvRawDataSize),
+
+ %define TDX_HEAP_STACK_BASE FixedPcdGet32 (PcdOvmfSecPeiTempRamBase)
+ %define TDX_HEAP_STACK_SIZE FixedPcdGet32 (PcdOvmfSecPeiTempRamSize)
+
+ %define TDX_HOB_MEMORY_BASE FixedPcdGet32 (PcdOvmfSecGhcbBase)
+ %define TDX_HOB_MEMORY_SIZE FixedPcdGet32 (PcdOvmfSecGhcbSize)
+
+ %define TDX_INIT_MEMORY_BASE FixedPcdGet32 (PcdOvmfWorkAreaBase)
+ %define TDX_INIT_MEMORY_SIZE (FixedPcdGet32 (PcdOvmfWorkAreaSize) + FixedPcdGet32 (PcdOvmfSecGhcbBackupSize))
+
+ %define OVMF_PAGE_TABLE_BASE FixedPcdGet32 (PcdOvmfSecPageTablesBase)
+ %define OVMF_PAGE_TABLE_SIZE FixedPcdGet32 (PcdOvmfSecPageTablesSize)
+
+ %define TDX_WORK_AREA_PGTBL_READY (FixedPcdGet32 (PcdOvmfWorkAreaBase) + 4)
+ %define TDX_WORK_AREA_GPAW (FixedPcdGet32 (PcdOvmfWorkAreaBase) + 8)
+
%define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))

%define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
@@ -77,6 +102,8 @@
%define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
%define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16)
%define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
+
+%include "X64/IntelTdxMetadata.asm"
%include "Ia32/Flat32ToFlat64.asm"
%include "Ia32/AmdSev.asm"
%include "Ia32/PageTables64.asm"
diff --git a/OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm b/OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm
new file mode 100644
index 000000000000..07f89ef4931f
--- /dev/null
+++ b/OvmfPkg/ResetVector/X64/IntelTdxMetadata.asm
@@ -0,0 +1,115 @@
+;------------------------------------------------------------------------------
+; @file
+; Tdx Virtual Firmware metadata
+;
+; When host VMM creates a new guest TD, some initial set of TD-private pages
+; are added using the TDH.MEM.PAGE.ADD function. These pages typically contain
+; Virtual BIOS code and data along with some clear pages for stacks and heap.
+; In the meanwhile, some configuration data need be measured by host VMM.
+; Tdx Metadata is designed for this purpose to indicate host VMM how to do the
+; above tasks.
+;
+; Tdx Metadata consists of a DESCRIPTOR as the header followed by several
+; SECTIONs. Host VMM sets up the memory for TDVF according to these sections.
+;
+; _Bfv is the example (Bfv refers to the Virtual BIOS code).
+; - By DataOffset/RawDataSize host VMM knows about the position of the code
+; in the binary image.
+; - MemoryAddress/MemoryDataSize indicates the guest physical address/size of
+; the Bfv to be loaded.
+; - Type field means this section is of BFV. This field is designed for the
+; purpose that in some case host VMM may do some additional processing based
+; upon the section type. TdHob section is an example. Host VMM pass the
+; physical memory information to the guest firmware by writing the data in
+; the memory region designated by TdHob section.
+; - By design code part of the binary image (Bfv) should be measured by host
+; VMM. This is indicated by the Attributes field.
+;
+; So put all these information together, when a new guest is being created,
+; the initial TD-private pages for BFV is added by TDH.MEM.PAGE.ADD function,
+; and Bfv is loaded at the guest physical address indicated by MemoryAddress.
+; Since the Attributes is TDX_METADATA_ATTRIBUTES_EXTENDMR, Bfv is measured by
+; host VMM.
+;
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+BITS 64
+
+%define TDX_METADATA_SECTION_TYPE_BFV 0
+%define TDX_METADATA_SECTION_TYPE_CFV 1
+%define TDX_METADATA_SECTION_TYPE_TD_HOB 2
+%define TDX_METADATA_SECTION_TYPE_TEMP_MEM 3
+%define TDX_METADATA_VERSION 1
+%define TDX_METADATA_ATTRIBUTES_EXTENDMR 0x00000001
+
+ALIGN 16
+TIMES (15 - ((TdxGuidedStructureEnd - TdxGuidedStructureStart + 15) % 16)) DB 0
+
+TdxGuidedStructureStart:
+
+;
+; TDVF meta data
+;
+TdxMetadataGuid:
+ DB 0xf3, 0xf9, 0xea, 0xe9, 0x8e, 0x16, 0xd5, 0x44
+ DB 0xa8, 0xeb, 0x7f, 0x4d, 0x87, 0x38, 0xf6, 0xae
+
+_Descriptor:
+ DB 'T','D','V','F' ; Signature
+ DD TdxGuidedStructureEnd - _Descriptor ; Length
+ DD TDX_METADATA_VERSION ; Version
+ DD (TdxGuidedStructureEnd - _Descriptor - 16)/32 ; Number of sections
+
+_Bfv:
+ DD TDX_BFV_RAW_DATA_OFFSET
+ DD TDX_BFV_RAW_DATA_SIZE
+ DQ TDX_BFV_MEMORY_BASE
+ DQ TDX_BFV_MEMORY_SIZE
+ DD TDX_METADATA_SECTION_TYPE_BFV
+ DD TDX_METADATA_ATTRIBUTES_EXTENDMR
+
+_Cfv:
+ DD TDX_CFV_RAW_DATA_OFFSET
+ DD TDX_CFV_RAW_DATA_SIZE
+ DQ TDX_CFV_MEMORY_BASE
+ DQ TDX_CFV_MEMORY_SIZE
+ DD TDX_METADATA_SECTION_TYPE_CFV
+ DD 0
+
+_TdxHeapStack:
+ DD 0
+ DD 0
+ DQ TDX_HEAP_STACK_BASE
+ DQ TDX_HEAP_STACK_SIZE
+ DD TDX_METADATA_SECTION_TYPE_TEMP_MEM
+ DD 0
+
+_TdxInitMem:
+ DD 0
+ DD 0
+ DQ TDX_INIT_MEMORY_BASE
+ DQ TDX_INIT_MEMORY_SIZE
+ DD TDX_METADATA_SECTION_TYPE_TEMP_MEM
+ DD 0
+
+_TdHob:
+ DD 0
+ DD 0
+ DQ TDX_HOB_MEMORY_BASE
+ DQ TDX_HOB_MEMORY_SIZE
+ DD TDX_METADATA_SECTION_TYPE_TD_HOB
+ DD 0
+
+_OvmfPageTable:
+ DD 0
+ DD 0
+ DQ OVMF_PAGE_TABLE_BASE
+ DQ OVMF_PAGE_TABLE_SIZE
+ DD TDX_METADATA_SECTION_TYPE_TEMP_MEM
+ DD 0
+
+TdxGuidedStructureEnd:
+ALIGN 16
--
2.29.2.windows.2


[PATCH V9 2/4] OvmfPkg: Clear WORK_AREA_GUEST_TYPE in Main.asm

Min Xu
 

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

Previously WORK_AREA_GUEST_TYPE was cleared in SetCr3ForPageTables64.
This is workable for Legacy guest and SEV guest. But it doesn't work
after Intel TDX is introduced. It is because all TDX CPUs (BSP and APs)
start to run from 0xfffffff0, thus WORK_AREA_GUEST_TYPE will be cleared
multi-times if it is TDX guest. So the clearance of WORK_AREA_GUEST_TYPE
is moved to Main16 entry point in Main.asm.
Note: WORK_AREA_GUEST_TYPE is only defined for ARCH_X64.

For Intel TDX, its corresponding entry point is Main32 (which will be
introduced in next commit in this patch-set). WORK_AREA_GUEST_TYPE will
be cleared there.

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Gerd Hoffmann <kraxel@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Signed-off-by: Min Xu <min.m.xu@...>
---
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 4 ----
OvmfPkg/ResetVector/Main.asm | 8 ++++++++
2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 07b6ca070909..02528221e560 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -42,10 +42,6 @@ BITS 32
;
SetCr3ForPageTables64:

- ; Clear the WorkArea header. The SEV probe routines will populate the
- ; work area when detected.
- mov byte[WORK_AREA_GUEST_TYPE], 0
-
; Check whether the SEV is active and populate the SevEsWorkArea
OneTimeCall CheckSevFeatures

diff --git a/OvmfPkg/ResetVector/Main.asm b/OvmfPkg/ResetVector/Main.asm
index ae90a148fce7..a501fbe880f2 100644
--- a/OvmfPkg/ResetVector/Main.asm
+++ b/OvmfPkg/ResetVector/Main.asm
@@ -36,6 +36,14 @@ Main16:

BITS 32

+%ifdef ARCH_X64
+
+ ; Clear the WorkArea header. The SEV probe routines will populate the
+ ; work area when detected.
+ mov byte[WORK_AREA_GUEST_TYPE], 0
+
+%endif
+
;
; Search for the Boot Firmware Volume (BFV)
;
--
2.29.2.windows.2

10761 - 10780 of 92496