HI Brijesh Thanks for the explanation. All fields in TDX metadata are required. So the current SEV proposal (3 fields) does not work for TDX. The extra fields are used to guide VMM on how to copy the binary, allocate memory, and how to measure the component. And we are adding more extension to them to support more use cases.
Based upon the factor that TDX and SEV are using different architecture to boot, I am not sure why we have to combine them into one table. (TDX need assist from TDX module - run in host CPU, and SEV need assist from PSP - a coprocessor) I strongly recommend to use two tables. TDX only defines TDX required info. SEV only defines SEV required info. Each one can add its own extension separately. Otherwise, there will be unnecessary information in the one metadata, which will increase the image size. Thank you Yao Jiewen > -----Original Message----- > From: Xu, Min M <min.m...@intel.com> > Sent: Thursday, September 23, 2021 10:15 PM > To: Brijesh Singh <brijesh.si...@amd.com>; Yao, Jiewen > <jiewen....@intel.com>; devel@edk2.groups.io; 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 > > On September 23, 2021 10:04 PM, Brijesh Singh wrote: > > > > SEV hardware does not have a concept of the metadata. To boot SEV guest we > > need to pass some information to VMM and in past those information were > > passed through SNP_BOOT_BLOCK (GUIDed structure) but Gerd recommended > > that it will be good idea if both SEV and TDX uses a common metadata > approach > > to pass these information. I personally think it was a good suggestion. So, > > in > SNP > > series I went ahead and created a generic metadata structure and hope that > > TDX will build on it. The user of the metadata structure is VMM (qemu, etc); > > while launching the guest the VMM knows whether its creating the SEV or TDX > > guest and will process the entries accordingly. > > > > As per the number of fields in the metadata is concerns, I felt 3 fields > > (start, > size > > and type) should be good enough for all the cases. There was a question from > > Gerd to Min asking why do you need the dataoffset/rawdatasize etc and I > don't > > remember seeing the answer for it. > > > The discussion is in this link. https://edk2.groups.io/g/devel/message/80289 > > > >As I said in the start, SNP hardware does not > > enforce metadata layout so I am flexible to add more field or remove or keep > it > > separate. > > > > thanks > > > > On 9/23/21 8:38 AM, Yao, Jiewen wrote: > > > Good point, Min. > > > > > > If > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub. > > com%2FAMDESE%2Fovmf%2Fblob%2Fsnp- > > v8%2FOvmfPkg%2FResetVector%2FX64%2FOvmfMetadata.asm&data=0 > > 4%7C01%7Cbrijesh.singh%40amd.com%7C52f6327efa33480bf4a308d97e977 > > ff0%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637680011416 > > 117826%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l > > uMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zDfikRYhxd8E > > RY%2Fw6kJLhJKRNWbYTl4D6PpqK%2BVNsus%3D&reserved=0 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi > > >> thub.com%2FAMDESE%2Fovmf%2Fblob%2Fsnp- > > &data=04%7C01%7Cbrijesh.sin > > >> > > gh%40amd.com%7C52f6327efa33480bf4a308d97e977ff0%7C3dd8961fe488 > > 4e608e1 > > >> > > 1a82d994e183d%7C0%7C0%7C637680011416117826%7CUnknown%7CTWF > > pbGZsb3d8ey > > >> > > JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D% > > 7C10 > > >> > > 00&sdata=at43H9sgmlaD773wsU5%2BoPPSjImo0UiCxQ0nmwdV9ds%3D > > &res > > >> erved=0 > > >> 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 (#81042): https://edk2.groups.io/g/devel/message/81042 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] -=-=-=-=-=-=-=-=-=-=-=-
