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

["Min Xu" <min.m.xu@intel.com>]

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
> 
> 
> 
>