Good point, Min. If https://github.com/AMDESE/ovmf/blob/snp-v8/OvmfPkg/ResetVector/X64/OvmfMetadata.asm is the proposal, then I have more comment:
Type: OVMF_SECTION_TYPE_CODE, OVMF_SECTION_TYPE_VARS are NOT used for SEV. I am not sure why they are there. Type: OVMF_SECTION_TYPE_CPUID should be SEV specific. TDX does not need CPUID page. Type: OVMF_SECTION_TYPE_SEC_MEM also seems for SEV. TDX does not need this special memory, such as Page table. It is already covered by code. Type: OVMF_SECTION_TYPE_SNP_SECRETS / OVMF_SECTION_TYPE_SNP_SEC_MEM is SEV specific. The SEV table is totally different with TDX metadata table. I really cannot see the benefit to merge into one table. Thank you Yao Jiewen > -----Original Message----- > From: Xu, Min M <min.m...@intel.com> > Sent: Thursday, September 23, 2021 9:20 PM > To: devel@edk2.groups.io; brijesh.si...@amd.com; Yao, Jiewen > <jiewen....@intel.com>; Gerd Hoffmann <kra...@redhat.com> > Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>; Justen, Jordan L > <jordan.l.jus...@intel.com>; Erdem Aktas <erdemak...@google.com>; James > Bottomley <j...@linux.ibm.com>; Tom Lendacky <thomas.lenda...@amd.com> > Subject: RE: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector > > 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 (#81036): https://edk2.groups.io/g/devel/message/81036 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] -=-=-=-=-=-=-=-=-=-=-=-
