Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector


Min Xu
 

I suggest SEV and TDX keep their own metadata in separate files. This is because SEV and TDX has different item structure.

From the OvmfMetadata definition in SEV (https://github.com/AMDESE/ovmf/blob/snp-v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm) there are 3 fields in the item. (Base/Size/Type).

But for TDX, there are 6 fields (DataOffset/RawDataSize/MemoryAddress/MemorySize/Type/Attribute) in one item.
That is because TDX-QEMU not only initialize the memory region, but also does more tasks (measurement) if the Attribute indicates.
DataOffset/RawDataSize is used by the TDX-QEMU to do the measurement if the Attribute field is MR.EXTEND.
MemoryAddress/MemorySize indicates the TDX-QEMU how to initialize the memory region.

We can add more fields in the item to make it workable for both SEV and TDX, (for example, add DataOffset/RawDataSize/Attribute), but it also restrict the changes in the future if more fields is needed (TDX's change will impact the existing SEV-QEMU).

On September 23, 2021 8:55 PM, Brijesh Singh wrote:

Like Gerd I would prefer to have one metadata table in the reset GUID.
The metadata table will contain multiple entries; lot of entries are common
between SNP and TDX. Some entries will have specific meaning for the platform.
Those special entries should be marked using the
OVMF_SECTION_TYPE_{TDX,SNP}_XXXX. It is perfectly fine to have a more than
one entry for the same region with different type, e.g

GhcbBookkeepingSnp:

  GHCB_BOOKKEPING_BASE_ADDRESS

  GHCB_BOOKKEEPING_SIZE

  OVMF_SECTION_TYPE_SNP_MEM

TdxMailBoxExt:

  GHCB_BOOKKEPING_BASE_ADDRESS

  GHCB_BOOKKEEPING_SIZE

  OVMF_SECTION_TYPE_TDX_MAILBOX

If we want all the OVMF_SECTION_TYPE_SNP_xxx should be defined in a
separate file then that is also doable. I put everything in one place because I was
trying to keep entry order similar to what is present in MEMFD.

thanks

On 9/23/21 6:39 AM, Yao, Jiewen wrote:
I strongly recommend to separate SEV and TDX in all context, if it is something
SEV or TDX specific.
Then each file has clear ownership.
If it is something generic for both SEV and TDX, it can in one file.

For example, SecPeiTempRam/SecPageTable can be in common file.
But SevSnpSecrets/GhcbBookkeeping should be in SEV file.

Thank you
Yao Jiewen

-----Original Message-----
From: Gerd Hoffmann <kraxel@redhat.com>
Sent: Thursday, September 23, 2021 4:48 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>;
Justen, Jordan L <jordan.l.justen@intel.com>; Brijesh Singh
<brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; James
Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>;
Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

On Thu, Sep 23, 2021 at 12:38:24AM +0000, Xu, Min M wrote:
On September 22, 2021 3:49 PM, Gerd Hoffmann wrote:
Hi,

+%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 tdxMetadataOffsetStart - 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
This should be switched to common ovmf metadata (see patches 4-7 of
the SEV-SNP series).

Min: please have a look at these patches.
Hi, Gerd
I checked the patches 4-7 of the SEV-SNP series. The common
OvmfMetadata is designed for both SEV and TDX, right?
That is the idea, yes.

If so, then it means the SEV and TDX metadata will be mixed in this
OvmfMetadata.
Yes.

I am thinking there will always be different fields for SEV and TDX.
For example, SEV has PcdOvmfSecGhcbPageTable but TDX doesn't need
that page. If the common OvmfMetadata is consumed by TDX-QEMU, then
PcdOvmfSecGhcbPageTableBase will be initialized too.
That doesn't make sense.
We have different range types. OVMF_* are the common areas. SEV_*
will be used by sev only, TDX_* will be used by tdx only. TDX and
SEV entries are allowed to overlap, i.e. PcdOvmfSecGhcbPageTableBase
should have some SEV_* type for sev (I think this needs fixing in the
series), and tdx can use the page for something else by adding an
TDX_* entry for the same range.

I am thinking that SEV and TDX can keep their own Metadata (in
separate files, SevMetadata.asm and TdxMetadata.asm) which are
pointed by the SEV or TDX offsets in the GUID-ed chain in ResetVector.
I'd very much prefer to have a single table to avoid duplication for
the common memory areas and keep the reset vector small.

Having separate SevMetadata.asm + TdxMetadata.asm files (then have
OvmfMetadata.asm include these two) is an option. I think this isn't
needed, we can also just group the entries in OvmfMetadata.asm.

In this case, SEV and TDX can design their own metadata flexibly,
for example, the attribute, the item structure, add/remove/update
the items, etc.
Why have two ways to do the same thing?

take care,
Gerd


Join devel@edk2.groups.io to automatically receive all group messages.