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

["Yao, Jiewen" <jiewen.yao@intel.com>]

The metadata table definition for TDX is at https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.pdf, Chapter 11.2 TDVF descriptor. And we will add more entry there.
May I get a proposed SEV or OVMF meta-table definition somewhere?
Before we get a clear definition for SEV meta-table, I feel it is too early to say one-table.

Another thing I would like to claim *TDVF metadata design principle*: We treat metadata is the *interface* between TDVF and VMM.
The metadata table should contain and only contain the information that is consumed by VMM.
If it is something VMM does not need to know, then it should NOT be in metadata table.



For https://github.com/AMDESE/ovmf/blob/snp-v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm, I am not sure why we put below structure there.
ExtractHandlerTable:
  DD  GUID_EXTRACT_HANDLER_TABLE_BASE
  DD  GUID_EXTRACT_HANDLER_TABLE_SIZE
  DD  OVMF_SECTION_TYPE_SEC_MEM

Does VMM need to know the Extraction Handler Table???

Thank you
Yao Jiewen

> -----Original Message-----
> From: Brijesh Singh <brijesh.singh@amd.com>
> Sent: Thursday, September 23, 2021 8:55 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; 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>; Erdem Aktas <erdemaktas@google.com>;
> James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> 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