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.gGhcbBookkeepingSnp: 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 (#81032): https://edk2.groups.io/g/devel/message/81032 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] -=-=-=-=-=-=-=-=-=-=-=-
