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 <kra...@redhat.com> > >> Sent: Thursday, September 23, 2021 4:48 PM > >> To: Xu, Min M <min.m...@intel.com> > >> Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianoc...@kernel.org>; > >> Justen, Jordan L <jordan.l.jus...@intel.com>; Brijesh Singh > >> <brijesh.si...@amd.com>; Erdem Aktas <erdemak...@google.com>; James > >> Bottomley <j...@linux.ibm.com>; Yao, Jiewen <jiewen....@intel.com>; > >> Tom Lendacky <thomas.lenda...@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 > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#81034): https://edk2.groups.io/g/devel/message/81034 Mute This Topic: https://groups.io/mt/85761661/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
