Hi Min,

On 9/21/21 4:05 AM, Min Xu wrote:

;
; Modified: EAX, EBX, ECX, EDX
;
SetCr3ForPageTables64:
-
- ; Clear the WorkArea header. The SEV probe routines will populate the
- ; work area when detected.
- mov byte[WORK_AREA_GUEST_TYPE], 0

Why you are removing the above block ? The workarea hdr must be
initialized to zero, its not safe to assume that the guest memory is
zero'ed in the non-encrypted case.

thanks

On 9/22/21 3:21 AM, Gerd Hoffmann wrote:

On Mon, Sep 20, 2021 at 01:45:49PM -0500, Brijesh Singh wrote:

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cdb7ae27f87684e0252d008d97da1f85f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637678956888503398%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Cm1E%2BJSrQ%2B%2BCjv5ZqC%2BXNqVGbzwZ32PFGDTZtoL8e84%3D&reserved=0

The initial page built during the SEC phase is used by the
MemEncryptSevSnpValidateSystemRam() for the system RAM validation. The
page validation process requires using the PVALIDATE instruction; the
instruction accepts a virtual address of the memory region that needs
to be validated. If hardware encounters a page table walk failure (due
to page-not-present) then it raises #GP.

The initial page table built in SEC phase address up to 4GB. Add an
internal function to extend the page table to cover > 4GB. The function
builds 1GB entries in the page table for access > 4GB. This will provide
the support to call PVALIDATE instruction for the virtual address >
4GB in PEI phase.

Hmm, well, playing with page tables like that in sev-specific code
instead of having memory core handle this properly is quite hackish.

What is the long-term plan with this? I assume once support for lazy
acceptance/validation is merged we can simply delete this?

Yes, this is just an interim problem. Once we move to lazy validation
then this will be removed.

Assuming this is only a temporary solution I think we can tolerate the
hacks.

take care,
Gerd

On 9/22/21 3:00 AM, Gerd Hoffmann wrote:

Hi,

STATIC BOOLEAN mSevStatus = FALSE;
STATIC BOOLEAN mSevEsStatus = FALSE;
+STATIC BOOLEAN mSevSnpStatus = FALSE;
STATIC BOOLEAN mSevStatusChecked = FALSE;

Better use the new PcdConfidentialComputingAttr instead?
At least in Dxe Phase, maybe Pei too (not sure what the initialization
order is)?

Let me see where I can use the new Pcd. We need to check the Sev status
in SEC and before the dynamic PCD is set so in those case its still
required but once PCD is set then we can switch to using it.

take care,
Gerd

You are right. My statement for page table is wrong. Both TDX and SEV need them.

That is NOT our original design. But I can understand why it is changed today.

I compare https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2-staging%2Fblob%2FTDVF%2FOvmfPkg%2FResetVector%2FX64%2FTdxMetadata.asm&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc5ee156422614a843cf408d97f43bc9b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680751169724127%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=GI6%2BzLASgUtBT9kS%2B%2BVDfzLlImYhZJeETpNfDMIimAk%3D&reserved=0 and https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-v8%2FOvmfPkg%2FResetVector%2FX64%2FOvmfMetadata.asm&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc5ee156422614a843cf408d97f43bc9b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680751169724127%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Dq2pLkdBsYXFbYM5nmy43yv4DWn7IDUceMpQg2%2F0jYo%3D&reserved=0.

There are 8 entries in TDX, and 10 entries in SEV.
2 of them are same, page table and TEMP RAM.
6 entries are TDX unique. 8 entries are SEV unique.

In the SEV patches you are seeing more sections because I tried to keep it in sync with the MEMFD [1] so that its much more readable. In TDX patches, Min decided to breakdown things a bit further and in some case skip sections. e.g  in TDX patches the SecPeiTempRamBase is broken into stack and heap section.

[1] https://github.com/tianocore/edk2/blob/master/OvmfPkg/OvmfPkgX64.fdf#L63

thanks

Again. Two topics. We need discuss them separately.

Topic 1: TD metadata table is an architecture way to communicate with VMM.
We took the design from PE/COFF image section, which is flexible to support different binary format.
EDKII TDVF is just one possible producer. There could be other producer in the future. We don't want to define something only meet current TDVF need.
The ATTRIBUTE field are separated from TYPE field, because they are different thing. Currently, you may use TYPE to predict what ATTRIBUTE will be. But that is NOT always correct. We are adding more ATTRIBUTE for the future.

Topic 2: In config-B we remove PEI.
I think we should say it in different way: We add PEI back in config-A.
In our original design we totally eliminated PEI, because it is unnecessary. IMHO, it is totally an overdesign in OVMF to include PEI.
However, in order to be compatible with current OVMF and make "minimal" change for config-A, we add PEI back.

Thank you
Yao Jiewen

On 9/24/21 2:39 AM, Yao, Jiewen wrote:

Comment below:

-----Original Message-----
From: Gerd Hoffmann <kraxel@...>
Sent: Friday, September 24, 2021 12:54 PM
To: Yao, Jiewen <jiewen.yao@...>
Cc: Xu, Min M <min.m.xu@...>; devel@edk2.groups.io;
brijesh.singh@...; Ard Biesheuvel <ardb+tianocore@...>; Justen,
Jordan L <jordan.l.justen@...>; Erdem Aktas <erdemaktas@...>;
James Bottomley <jejb@...>; Tom Lendacky
<thomas.lendacky@...>
Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

On Thu, Sep 23, 2021 at 01:38:52PM +0000, Yao, Jiewen wrote:

Good point, Min.

If https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAMDESE%2Fovmf%2Fblob%2Fsnp-&;data=04%7C01%7Cbrijesh.singh%40amd.com%7C5452ded75af34b9c73aa08d97f2e8426%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680660025337914%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3yXoKMmZndyiJkpr2gADbq6KvPLoF1r5sCv32MxK4Ms%3D&amp;reserved=0

v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm is the proposal, then I have
more comment:

Type: OVMF_SECTION_TYPE_CODE, OVMF_SECTION_TYPE_VARS are NOT

used for SEV. I am not sure why they are there.

tdx needs them (for measurement). It's not a tdx-specific concept,
possibly sev-snp wants use that too in the future.

That means this is only for TDX. SEV does not need this type. Then this is TDX specific.

FYI, SEV also measures code or data put by the VMM in the guest memory
space. In SEV, qemu calls a routine to encrypt the pflash unit0 -- while
handling the callback you can convert the GPA to HVA and thus avoid the
section. But having the section available is not going to create any
issues with the SEV and in some cases it may be useful.

Type: OVMF_SECTION_TYPE_CPUID should be SEV specific. TDX does not need

CPUID page.

A cpuid page can be used without sev too.

I don't think TDX need this field. This is SEV specific.

You are right that CPUID page is currently SEV specific but nothing
stops another VMM to use CPUID page on the non-SEV platform to minimize
the number of VM exits. I think once guest kernel gets support for using
the CPUID page it may become reality sooner.

Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not

need this special memory, such as Page table. It is already covered by code.

These are "needs pre-validation / pre-acceptance" regions.
TDX surely needs that too.

I don't think TDX need this. The page table should be covered by CODE already.

Page table is data page and are not covered by the CODE section.  IIUC,
TDX also need to accept memory before the access. There are several data
pages (such page table, stack, heap, ...)  used in the SEC phase; you
have two choice, you either accept them during the SEC phase boot or
accept them before the boot. If accepted before the boot then they will
be measured so that security is not compromised. For simplicity it seems
both SNP and TDX decided to accept those data pages before the boot. The
metadata simply provides the description of those pre accepted pages.

Type: OVMF_SECTION_TYPE_SNP_SECRETS /

OVMF_SECTION_TYPE_SNP_SEC_MEM is SEV specific.

Yes.

The SEV table is totally different with TDX metadata table.

I can't see a fundamental difference. In both cases the VMM needs
to know the firmware memory layout for (a) attestation, and (b)
pre-validating/pre-acceptance of memory, and (c) some
hardware-specific ranges such as snp secrets page.

I really cannot see the benefit to merge into one table.

Keep reset vector small?
Have common parser structs and code?

I think it is opposite. This proposal makes reset vector larger, if we need define more structure to satisfy TDX, but it is not needed by SEV. Or Define something purely for SEV, but not useful for TDX.
I don't treat it as benefit. Instead I think it is big burden.

take care,
Gerd

You are right. My statement for page table is wrong. Both TDX and SEV need them.

That is NOT our original design. But I can understand why it is changed today.

I compare https://github.com/tianocore/edk2-staging/blob/TDVF/OvmfPkg/ResetVector/X64/TdxMetadata.asm and https://github.com/AMDESE/ovmf/blob/snp-v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm.

There are 8 entries in TDX, and 10 entries in SEV.
2 of them are same, page table and TEMP RAM.
6 entries are TDX unique. 8 entries are SEV unique.

I still feel it is burden to merge them, because some attributes field is NOT required for SEV but needed for TDX.
And TDX parsing tool need rule out SEV entries, and SEV parsing tool need rule out TDX entries.



Thank you
Yao Jiewen

Hi,

The question why TDX_BFV_RAW_DATA_OFFSET and
TDX_BFV_RAW_DATA_SIZE are needed and why TDX_BFV_MEMORY_BASE +
TDX_BFV_MEMORY_SIZE can't be used is still open.

Here is a BFV metadata.
37 <1> _Bfv:
38 00000140 00400800 <1> DD TDX_BFV_RAW_DATA_OFFSET
39 00000144 00C03700 <1> DD TDX_BFV_RAW_DATA_SIZE
40 00000148 0040C8FF00000000 <1> DQ TDX_BFV_MEMORY_BASE
41 00000150 00C0370000000000 <1> DQ TDX_BFV_MEMORY_SIZE
42 00000158 00000000 <1> DD TDX_METADATA_SECTION_TYPE_BFV
43 0000015C 01000000 <1> DD TDX_METADATA_ATTRIBUTES_EXTENDMR

TDX_BFV_RAW_DATA_OFFSET/TDX_BFV_RAW_DATA_SIZE indicates the offset/size of BFV in Ovmf.fd.
TDX_BFV_MEMORY_BASE/ TDX_BFV_MEMORY_SIZE is the physical memory address which is to be mapped.
TDX-QEMU use these information to 1) do the measurement of the BFV 2) map the BFV to the physical memory

TDX_BFV_RAW_DATA_SIZE + TDX_BFV_MEMORY_SIZE are identical. Why do you
need both? Yes, I know, some entries have RAW_DATA_SIZE=0 because
nothing is loaded for them. You can also figure that by looking at the
section type.

TDX_BFV_RAW_DATA_OFFSET can be calculated from TDX_BFV_MEMORY_BASE, it's
a fixed offset (depending on firmware size). At least as long as the
firmware loading uses to the usual x86 workflow (place right below 4G).

Also: When placing the firmware below 4G MEMORY_BASE + MEMORY_SIZE can
be DD.

The attribute field can be added to the ovmf meta data, or we make that
part of the section type.

*Config-A* enables a minimum functionality in OvmfPkgX64.dsc without
breaking existing functionality.

*Config-B* is a separate platform configuration file can be used to
provide all the security guarantees that TDX provides.

Why does config-b need a completely different initialization code path
(i.e. skip PEI, see slide 11 of the tdvf design review)?

take care,
Gerd

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3642
when the module is not building in IA32 mode which will lead to
building error. when a module built-in X64 function pointer will be the
size of 64bit width which cannot be fit in 32bit address which will lead
to error. to overcome this issue introducing the 2 new PCD's
for the 64bit modules can consume it.
Creating the API's to support different architecture

Cc: Chasel Chiu <chasel.chiu@...>
Cc: Nate DeSimone <nathaniel.l.desimone@...>
Cc: Star Zeng <star.zeng@...>
Cc: Kuo Ted <ted.kuo@...>
Cc: Duggapu Chinni B <chinni.b.duggapu@...>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@...>
Cc: Digant H Solanki <digant.h.solanki@...>
Cc: Sangeetha V <sangeetha.v@...>
Cc: Ray Ni <ray.ni@...>
Signed-off-by: Ashraf Ali S <ashraf.ali.s@...>
---
.../FspmWrapperPeim/FspmWrapperPeim.c | 19 +++++++++++---
.../FspmWrapperPeim/FspmWrapperPeim.inf | 16 ++++++++++--
.../FspmWrapperPeim/IA32/FspmHelper.c | 26 +++++++++++++++++++
.../FspmWrapperPeim/X64/FspmHelper.c | 26 +++++++++++++++++++
.../FspsWrapperPeim/FspsWrapperPeim.c | 17 +++++++++---
.../FspsWrapperPeim/FspsWrapperPeim.inf | 14 +++++++++-
.../FspsWrapperPeim/IA32/FspsHelper.c | 26 +++++++++++++++++++
.../FspsWrapperPeim/X64/FspsHelper.c | 26 +++++++++++++++++++
.../Include/Library/FspWrapperPlatformLib.h | 2 +-
IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dec | 2 ++
10 files changed, 163 insertions(+), 11 deletions(-)
create mode 100644 IntelFsp2WrapperPkg/FspmWrapperPeim/IA32/FspmHelper.c
create mode 100644 IntelFsp2WrapperPkg/FspmWrapperPeim/X64/FspmHelper.c
create mode 100644 IntelFsp2WrapperPkg/FspsWrapperPeim/IA32/FspsHelper.c
create mode 100644 IntelFsp2WrapperPkg/FspsWrapperPeim/X64/FspsHelper.c

diff --git a/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c b/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c
index 24ab534620..4a15136c39 100644
--- a/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c
+++ b/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c
@@ -3,7 +3,7 @@
register TemporaryRamDonePpi to call TempRamExit API, and register MemoryDiscoveredPpi
notify to call FspSiliconInit API.

- Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -39,6 +39,17 @@

extern EFI_GUID gFspHobGuid;

+/**
+ Get the Fspm Upd Data Address from the PCD
+
+ @return FSPM UPD Data Address
+**/
+UINTN
+EFIAPI
+GetFspmUpdDataAddress (
+ VOID
+ );
+
/**
Call FspMemoryInit API.

@@ -59,7 +70,7 @@ PeiFspMemoryInit (

DEBUG ((DEBUG_INFO, "PeiFspMemoryInit enter\n"));

- FspHobListPtr = NULL;
+ FspHobListPtr = NULL;
FspmUpdDataPtr = NULL;

FspmHeaderPtr = (FSP_INFO_HEADER *) FspFindFspHeader (PcdGet32 (PcdFspmBaseAddress));
@@ -68,7 +79,7 @@ PeiFspMemoryInit (
return EFI_DEVICE_ERROR;
}

- if (PcdGet32 (PcdFspmUpdDataAddress) == 0 && (FspmHeaderPtr->CfgRegionSize != 0) && (FspmHeaderPtr->CfgRegionOffset != 0)) {
+ if (GetFspmUpdDataAddress () == 0 && (FspmHeaderPtr->CfgRegionSize != 0) && (FspmHeaderPtr->CfgRegionOffset != 0)) {
//
// Copy default FSP-M UPD data from Flash
//
@@ -80,7 +91,7 @@ PeiFspMemoryInit (
//
// External UPD is ready, get the buffer from PCD pointer.
//
- FspmUpdDataPtr = (FSPM_UPD_COMMON *)PcdGet32 (PcdFspmUpdDataAddress);
+ FspmUpdDataPtr = (FSPM_UPD_COMMON *) GetFspmUpdDataAddress ();
ASSERT (FspmUpdDataPtr != NULL);
}

diff --git a/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.inf b/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.inf
index 00166e56a0..5b4ad531e7 100644
--- a/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.inf
+++ b/IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.inf
@@ -6,7 +6,7 @@
# register TemporaryRamDonePpi to call TempRamExit API, and register MemoryDiscoveredPpi
# notify to call FspSiliconInit API.
#
-# Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -45,6 +45,7 @@
FspWrapperApiLib
FspWrapperApiTestLib
FspMeasurementLib
+ PcdLib

[Packages]
MdePkg/MdePkg.dec
@@ -56,14 +57,25 @@

[Pcd]
gIntelFsp2WrapperTokenSpaceGuid.PcdFspmBaseAddress ## CONSUMES
- gIntelFsp2WrapperTokenSpaceGuid.PcdFspmUpdDataAddress ## CONSUMES
gIntelFsp2WrapperTokenSpaceGuid.PcdFspModeSelection ## CONSUMES
gIntelFsp2WrapperTokenSpaceGuid.PcdFsptBaseAddress ## CONSUMES
gIntelFsp2WrapperTokenSpaceGuid.PcdFspMeasurementConfig ## CONSUMES

+[Pcd.IA32]
+ gIntelFsp2WrapperTokenSpaceGuid.PcdFspmUpdDataAddress ## CONSUMES
+
+[Pcd.X64]
+ gIntelFsp2WrapperTokenSpaceGuid.PcdFspmUpdDataAddress64 ## CONSUMES
+
[Sources]
FspmWrapperPeim.c

+[Sources.IA32]
+ IA32/FspmHelper.c
+
+[Sources.X64]
+ X64/FspmHelper.c
+
[Guids]
gFspHobGuid ## PRODUCES ## HOB
gFspApiPerformanceGuid ## SOMETIMES_CONSUMES ## GUID
diff --git a/IntelFsp2WrapperPkg/FspmWrapperPeim/IA32/FspmHelper.c b/IntelFsp2WrapperPkg/FspmWrapperPeim/IA32/FspmHelper.c
new file mode 100644
index 0000000000..cab11173cc
--- /dev/null
+++ b/IntelFsp2WrapperPkg/FspmWrapperPeim/IA32/FspmHelper.c
@@ -0,0 +1,26 @@
+/** @file
+ Sample to provide FSP wrapper related function.
+
+ Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+
+#include <Library/PcdLib.h>
+#include <Library/FspWrapperPlatformLib.h>
+#include <Uefi/UefiBaseType.h>
+
+/**
+ Get the Fspm Upd Data Address from the PCD
+
+ @return FSPM UPD Data Address
+**/
+UINTN
+EFIAPI
+GetFspmUpdDataAddress (
+ VOID
+ )
+{
+ return PcdGet32 (PcdFspmUpdDataAddress);
+}
diff --git a/IntelFsp2WrapperPkg/FspmWrapperPeim/X64/FspmHelper.c b/IntelFsp2WrapperPkg/FspmWrapperPeim/X64/FspmHelper.c
new file mode 100644
index 0000000000..25b89ff2e1
--- /dev/null
+++ b/IntelFsp2WrapperPkg/FspmWrapperPeim/X64/FspmHelper.c
@@ -0,0 +1,26 @@
+/** @file
+ Sample to provide FSP wrapper related function.
+
+ Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+
+#include <Library/PcdLib.h>
+#include <Library/FspWrapperPlatformLib.h>
+#include <Uefi/UefiBaseType.h>
+
+/**
+ Get the Fspm Upd Data Address from the PCD
+
+ @return FSPM UPD Data Address
+**/
+UINTN
+EFIAPI
+GetFspmUpdDataAddress (
+ VOID
+ )
+{
+ return PcdGet64 (PcdFspmUpdDataAddress64);
+}
diff --git a/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c b/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c
index 9d4f279e81..8bd510502f 100644
--- a/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c
+++ b/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c
@@ -3,7 +3,7 @@
register TemporaryRamDonePpi to call TempRamExit API, and register MemoryDiscoveredPpi
notify to call FspSiliconInit API.

- Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
@@ -58,6 +58,17 @@ S3EndOfPeiNotify(
IN VOID *Ppi
);

+/**
+ Get the Fsps Upd Data Address from the PCD
+
+ @return FSPS UPD Data Address
+**/
+UINTN
+EFIAPI
+GetFspsUpdDataAddress (
+ VOID
+ );
+
EFI_PEI_NOTIFY_DESCRIPTOR mS3EndOfPeiNotifyDesc = {
(EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),
&gEfiEndOfPeiSignalPpiGuid,
@@ -283,7 +294,7 @@ PeiMemoryDiscoveredNotify (
return EFI_DEVICE_ERROR;
}

- if (PcdGet32 (PcdFspsUpdDataAddress) == 0 && (FspsHeaderPtr->CfgRegionSize != 0) && (FspsHeaderPtr->CfgRegionOffset != 0)) {
+ if (GetFspsUpdDataAddress () == 0 && (FspsHeaderPtr->CfgRegionSize != 0) && (FspsHeaderPtr->CfgRegionOffset != 0)) {
//
// Copy default FSP-S UPD data from Flash
//
@@ -292,7 +303,7 @@ PeiMemoryDiscoveredNotify (
SourceData = (UINTN *)((UINTN)FspsHeaderPtr->ImageBase + (UINTN)FspsHeaderPtr->CfgRegionOffset);
CopyMem (FspsUpdDataPtr, SourceData, (UINTN)FspsHeaderPtr->CfgRegionSize);
} else {
- FspsUpdDataPtr = (FSPS_UPD_COMMON *)PcdGet32 (PcdFspsUpdDataAddress);
+ FspsUpdDataPtr = (FSPS_UPD_COMMON *) GetFspsUpdDataAddress ();
ASSERT (FspsUpdDataPtr != NULL);
}

diff --git a/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.inf b/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.inf
index aeeca58d6d..e988ebab21 100644
--- a/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.inf
+++ b/IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.inf
@@ -45,6 +45,7 @@
FspWrapperApiLib
FspWrapperApiTestLib
FspMeasurementLib
+ PcdLib

[Packages]
MdePkg/MdePkg.dec
@@ -65,10 +66,15 @@

[Pcd]
gIntelFsp2WrapperTokenSpaceGuid.PcdFspsBaseAddress ## CONSUMES
- gIntelFsp2WrapperTokenSpaceGuid.PcdFspsUpdDataAddress ## CONSUMES
gIntelFsp2WrapperTokenSpaceGuid.PcdFspModeSelection ## CONSUMES
gIntelFsp2WrapperTokenSpaceGuid.PcdFspMeasurementConfig ## CONSUMES

+[Pcd.IA32]
+ gIntelFsp2WrapperTokenSpaceGuid.PcdFspsUpdDataAddress ## CONSUMES
+
+[Pcd.X64]
+ gIntelFsp2WrapperTokenSpaceGuid.PcdFspsUpdDataAddress64 ## CONSUMES
+
[Guids]
gFspHobGuid ## CONSUMES ## HOB
gFspApiPerformanceGuid ## SOMETIMES_CONSUMES ## GUID
@@ -76,5 +82,11 @@
[Sources]
FspsWrapperPeim.c

+[Sources.IA32]
+ IA32/FspsHelper.c
+
+[Sources.X64]
+ X64/FspsHelper.c
+
[Depex]
gEfiPeiMemoryDiscoveredPpiGuid
diff --git a/IntelFsp2WrapperPkg/FspsWrapperPeim/IA32/FspsHelper.c b/IntelFsp2WrapperPkg/FspsWrapperPeim/IA32/FspsHelper.c
new file mode 100644
index 0000000000..c4ae292ffb
--- /dev/null
+++ b/IntelFsp2WrapperPkg/FspsWrapperPeim/IA32/FspsHelper.c
@@ -0,0 +1,26 @@
+/** @file
+ Sample to provide FSP wrapper related function.
+
+ Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+
+#include <Library/PcdLib.h>
+#include <Library/FspWrapperPlatformLib.h>
+#include <Uefi/UefiBaseType.h>
+
+/**
+ Get the Fsps Upd Data Address from the PCD
+
+ @return FSPS UPD Data Address
+**/
+UINTN
+EFIAPI
+GetFspsUpdDataAddress (
+ VOID
+ )
+{
+ return PcdGet32 (PcdFspsUpdDataAddress);
+}
diff --git a/IntelFsp2WrapperPkg/FspsWrapperPeim/X64/FspsHelper.c b/IntelFsp2WrapperPkg/FspsWrapperPeim/X64/FspsHelper.c
new file mode 100644
index 0000000000..a0d6adb281
--- /dev/null
+++ b/IntelFsp2WrapperPkg/FspsWrapperPeim/X64/FspsHelper.c
@@ -0,0 +1,26 @@
+/** @file
+ Sample to provide FSP wrapper related function.
+
+ Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+
+#include <Library/PcdLib.h>
+#include <Library/FspWrapperPlatformLib.h>
+#include <Uefi/UefiBaseType.h>
+
+/**
+ Get the Fsps Upd Data Address from the PCD
+
+ @return FSPS UPD Data Address
+**/
+UINTN
+EFIAPI
+GetFspsUpdDataAddress (
+ VOID
+ )
+{
+ return PcdGet64 (PcdFspsUpdDataAddress64);
+}
diff --git a/IntelFsp2WrapperPkg/Include/Library/FspWrapperPlatformLib.h b/IntelFsp2WrapperPkg/Include/Library/FspWrapperPlatformLib.h
index 2aa14c92fd..4a75c3b536 100644
--- a/IntelFsp2WrapperPkg/Include/Library/FspWrapperPlatformLib.h
+++ b/IntelFsp2WrapperPkg/Include/Library/FspWrapperPlatformLib.h
@@ -1,7 +1,7 @@
/** @file
Provide FSP wrapper platform related function.

- Copyright (c) 2014 - 2016, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/
diff --git a/IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dec b/IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dec
index a3b9363779..8c98dbd55d 100644
--- a/IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dec
+++ b/IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dec
@@ -121,3 +121,5 @@
#
gIntelFsp2WrapperTokenSpaceGuid.PcdFspmUpdDataAddress|0x00000000|UINT32|0x50000000
gIntelFsp2WrapperTokenSpaceGuid.PcdFspsUpdDataAddress|0x00000000|UINT32|0x50000001
+ gIntelFsp2WrapperTokenSpaceGuid.PcdFspmUpdDataAddress64|0x00000000|UINT64|0x50000002
+ gIntelFsp2WrapperTokenSpaceGuid.PcdFspsUpdDataAddress64|0x00000000|UINT64|0x50000003
--
2.30.2.windows.1

I think we are discussing two topics. Please allow me to separate them.

1) Topic one: A unified build for config-A and config-B

I think we have discussed that before in EDKII, when Laszlo suggested:
A) don't put all TDX features into OvmfPkg.dsc, just put a basic feature them - we call it config-A.
B) Put full TDX feature into another TdvfPkg.dsc - we call it config-B.
Once we finish A) and B), we can evaluate how to merge B into A.
I do recommend you track back or have a discussion in RedHat to see what is high level suggestion from RedHat.


2) Topic two: A unified metadata table for SEV and TDX.

To me, I don't see it is necessary. I would say: I agree with you that we can align the design as much as possible, such as MemEncryption, ExceptionLib, IOMMU, etc.
However, if there is something totally different, I see no benefit to merge.
One example I could give is ACPI MADT table, X86 system defines its own interrupt table (APIC), while ARM system defines its own interrupt table (GIC). There is NO need to define a common interrupt table to cover both X86 and ARM.

I think current two table approach is good enough. TDX owner maintains its own table. SEV owner maintains its own table. Just like APIC table and GIC in ACPI MADT.
Although they are called confidential computing technology, the hardware implementation is different and features are different.
From software layer, we can have HAL. But it does not mean we only have one common HAL for SEV and TDX. Two different HAL implementation are acceptable.

For the one table proposal, I would like to understand
A) What is the problem statement with current implementation?
B) What is the goal we want to achieve?
C) What is the benefit we can get?

Please be as specific as possible.

BTW: For C), I don't think we will have smaller code size, because we align we have to define some unnecessary field.
For your statement to remove duplication, please give me some real example. The page table example is invalid, because TDX does not need define an page table entry.
SEV requires GHCB, CPUID page but TDX does not. While TDX need indicate which range extend to MRTD, which is NOT. Also TDX metadata table will indicate which region may use AUG page, and which use ADD page in the future. I am not sure if SEV need those info.


Thank you
Yao Jiewen

Hi,

Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not

need this special memory, such as Page table. It is already covered by code.

These are "needs pre-validation / pre-acceptance" regions.
TDX surely needs that too.

I don't think TDX need this. The page table should be covered by CODE already.

I think you are wrong here, the patch has this ...

+_OvmfPageTable:
+ DD 0
+ DD 0
+ DQ OVMF_PAGE_TABLE_BASE
+ DQ OVMF_PAGE_TABLE_SIZE
+ DD TDX_METADATA_SECTION_TYPE_TEMP_MEM
+ DD 0

... and a few simliar entries.

I really cannot see the benefit to merge into one table.

Keep reset vector small?
Have common parser structs and code?

I think it is opposite. This proposal makes reset vector larger, if we
need define more structure to satisfy TDX, but it is not needed by
SEV.

The sev and tdx specific entries will be there anyway, no matter
whenever we place them into one or two separate tables.

Shared items like the page table memory will be there only once
when we use a unified table, but twice with two separate tables.

take care,
Gerd

On Fri, Sep 24, 2021 at 07:36:10AM +0000, Yao, Jiewen wrote:

That is my question.
AMD has its own extension. TDX has its own extension.
Why we have to unify the firmware binary, and to make both us unconfirmable?

Isn't that the plan anyway? At least for "config-a" with a basic
feature set? See other mail just sent for comments on "config-b".

Or do we want to unify ARM/AARch64/RISC-V ?

Not sure what you are trying to tell me.

take care,
Gerd

On Fri, Sep 24, 2021 at 07:32:33AM +0000, Yao, Jiewen wrote:

Hi Gerd
Having config-a and config-b is proposed by original RedHat rep in EDKII - Laszlo.
We reach the agreement to separate those 2 configuration and AMD SEV is taking same approach.

Are you saying you want to reset the high level plan and unify config-a and config-b into one binary?

There isn't that much of a difference between the normal and amd sev
build. It has additional drivers and the grub boot loader added, smm
support turned off, network stack removed.

The differences between config-a and config-b are much larger according
to the design document. config-b has a completely different
initialization code path, skipping the PEI phase. I see that as a
major problem when it comes to long-term maintenance, and so far nobody
could explain the reason for this.

I'll go read the links to old discussions sent by Min, maybe I find
something there.

Having a config-b with network stack disabled, driver for RTMR trusted
boot added, maybe some other little tweaks but otherwise a boot workflow
identical to config-a is reasonable in my eyes. Merging it with AmdSev
should also be relatively easy then.

take care,
Gerd

Hi,

This looks good to me!

[...]

+
+/** A parser for EArmObjFixedFeatureFlags.
+*/
+STATIC CONST CM_OBJ_PARSER CmArmFixedFeatureFlagsParser[] = {
+ {"Flags", 4, "0x%x", NULL}
+};
+
+/** A parser for EArmObjItsGroup.
+*/
+STATIC CONST CM_OBJ_PARSER CmArmItsGroupNodeParser[] = {
+ {"GTBlockTimerFrameToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL},

This should just be Token, not GTBlockTimerFrameToken.

+ {"ItsIdCount", 4, "0x%x", NULL},
+ {"ItsIdToken", sizeof (CM_OBJECT_TOKEN), "0x%p", NULL}
+};
+

[...]

diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.h b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.h
new file mode 100644
index 000000000000..e229df7095d9
--- /dev/null
+++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.h

[...]

+/**
+ The CM_OBJ_PARSER structure describes the fields of an CmObject and
+ provides means for the parser to interpret and trace appropriately.
+
+ ParseAcpi() uses the format string specified by 'Format' for tracing
+ the field data.
+*/
+typedef struct CmObjParser CM_OBJ_PARSER;
+struct CmObjParser {
+
+ /// String describing the Cm Object
+ CONST CHAR8* NameStr;
+
+ /// The length of the field.
+ UINT32 Length;
+
+ /// Optional Print() style format string for tracing the data. If not
+ /// used this must be set to NULL.
+ CONST CHAR8* Format;
+
+ /// Optional pointer to a print formatter function which
+ /// is typically used to trace complex field information.
+ /// If not used this must be set to NULL.
+ /// The Format string is passed to the PrintFormatter function
+ /// but may be ignored by the implementation code.
+ FNPTR_PRINT_FORMATTER PrintFormatter;
+
+ /// Optional pointer to print the fields of another CM_OBJ_PARSER
+ /// structure. This is useful to print sub-structures.
+ CONST CM_OBJ_PARSER *SubObjParser;
+
+ /// Count of items in the SubObj.
+ UINTN SubObjItemCount;

The SubObjParser doesn't actually seem to be used by any of the objects? (Unless I misread when reading the list of them..)

+
+ /// Count of items
+ UINTN ItemCount;
+} CM_OBJ_PARSER_ARRAY;
+
+#endif // CONFIGURATION_MANAGER_OBJECT_PARSER_H_
diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf b/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf
index 5435f74aa0b8..abbf4bc38cab 100644
--- a/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf
+++ b/DynamicTablesPkg/Library/Common/TableHelperLib/TableHelperLib.inf
@@ -15,6 +15,8 @@ [Defines]
LIBRARY_CLASS = TableHelperLib

[Sources]
+ ConfigurationManagerObjectParser.c
+ ConfigurationManagerObjectParser.h
TableHelper.c

[Packages]

Need to update the copyright year.

Otherwise:
Reviewed-by: Joey Gouly <joey.gouly@...>

Thanks,
Joey
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Apologies, I have already shared the patch.
I will take this input for future patches.

Regards,
JP

Comment below:

That is my question.
AMD has its own extension. TDX has its own extension.
Why we have to unify the firmware binary, and to make both us unconfirmable?
Or do we want to unify ARM/AARch64/RISC-V ?

I agree we can unify as much as possible.
But due to hardware difference i don't think we achieve 100% unifying.

Thank you
Yao Jiewen

Hi Rebecca,

I also reviewed/tested the patch:
Reviewed-by: Pierre Gondois <Pierre.Gondois@...>

Regards,

Pierre

Hi Gerd
Having config-a and config-b is proposed by original RedHat rep in EDKII - Laszlo.
We reach the agreement to separate those 2 configuration and AMD SEV is taking same approach.

Are you saying you want to reset the high level plan and unify config-a and config-b into one binary?

Thank you
Yao Jiewen

On September 24, 2021 1:28 PM, Gerd Hoffmann wrote:

Hi,

SEV hardware does not have a concept of the metadata. To boot SEV
guest we need to pass some information to VMM and in past those
information were passed through SNP_BOOT_BLOCK (GUIDed structure)
but Gerd recommended that it will be good idea if both SEV and TDX
uses a common metadata approach to pass these information. I
personally think it was a good suggestion. So, in SNP series I went
ahead and created a generic metadata structure and hope that TDX
will build on it. The user of the metadata structure is VMM (qemu,
etc); while launching the guest the VMM knows whether its creating the

SEV or TDX guest and will process the entries accordingly.

As per the number of fields in the metadata is concerns, I felt 3
fields (start, size and type) should be good enough for all the
cases. There was a question from Gerd to Min asking why do you need
the dataoffset/rawdatasize etc and I don't remember seeing the answer

for it.

The discussion is in this link.
https://edk2.groups.io/g/devel/message/80289

The question why TDX_BFV_RAW_DATA_OFFSET and
TDX_BFV_RAW_DATA_SIZE are needed and why TDX_BFV_MEMORY_BASE +
TDX_BFV_MEMORY_SIZE can't be used is still open.

Here is a BFV metadata.
37 <1> _Bfv:
38 00000140 00400800 <1> DD TDX_BFV_RAW_DATA_OFFSET
39 00000144 00C03700 <1> DD TDX_BFV_RAW_DATA_SIZE
40 00000148 0040C8FF00000000 <1> DQ TDX_BFV_MEMORY_BASE
41 00000150 00C0370000000000 <1> DQ TDX_BFV_MEMORY_SIZE
42 00000158 00000000 <1> DD TDX_METADATA_SECTION_TYPE_BFV
43 0000015C 01000000 <1> DD TDX_METADATA_ATTRIBUTES_EXTENDMR

TDX_BFV_RAW_DATA_OFFSET/TDX_BFV_RAW_DATA_SIZE indicates the offset/size of BFV in Ovmf.fd.
TDX_BFV_MEMORY_BASE/ TDX_BFV_MEMORY_SIZE is the physical memory address which is to be mapped.
TDX-QEMU use these information to 1) do the measurement of the BFV 2) map the BFV to the physical memory

While being at it: The question why "config-b" with a completely different
initialization code path is needed is still open too. The tdvf design guide is
not helpful here. Although explains what is different in "config-a" vs. "config-
b" it does not explain the background, i.e. why some features are supported
by "config-b" only.

The solution of Config-A and Config-B is to address the concerns of One-Binary requirement.
See Erdem Aktas's comments in this link https://edk2.groups.io/g/devel/message/76339
Quote: "What we are asking with "one binary" is: Simply enabling OVMF + a guest OS to boot in a TDX domain without breaking any existing functionality. Intel should put everything else (specifically related to remote attestation) in a separate platform configuration."

*Config-A* enables a minimum functionality in OvmfPkgX64.dsc without breaking existing functionality.
*Config-B* is a separate platform configuration file can be used to provide all the security guarantees that TDX provides.

And I guess these two questions are related. With "config-a" there is a fixed
offset between TDX_BFV_RAW_DATA_OFFSET + TDX_BFV_MEMORY_BASE,
so if you know one of them you can easily calculate the other. With "config-
b" this is possibly not the case.

So, can you please shed some light on this?

As I explained above how BFV metadata is used by TDX-QEMU, there is no different in Config-A and Config-B.

Below are some links discussing the Config-A and Config-B. Hope it is helpful.

Config-A and Config-B:
https://edk2.groups.io/g/devel/message/76367
Erdem Aktas's comments:
https://edk2.groups.io/g/devel/message/76339
Laszlo's comments
https://edk2.groups.io/g/devel/message/76836
https://edk2.groups.io/g/devel/message/76837

Thanks!
Min