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