Hi, Brijesh When I go thru the code I find a potential bug in MemEncryptSevEsIsEnabled().
In the current code both SEV and TDX leverage the OvmfWorkArea to record the SEV/TDX information. Byte [0] record the guest type, 0 for legacy guest. 1 for sev, 2 for tdx. Byte [3:1] are reserved. From Byte[4] on its meaning depends on the guest type (byte[0]). For SEV it is SEC_SEV_ES_WORK_AREA. In the InternalMemEncryptSevStatus(), it check the SEC_SEV_ES_WORK_AREA directly without checking if it is SEV guest. (byte[0]) SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase); I am afraid it is not correct. Would you please double check it? Thanks! Min > -----Original Message----- > From: Brijesh Singh <brijesh.si...@amd.com> > Sent: Tuesday, September 7, 2021 9:27 PM > To: kra...@redhat.com; Xu, Min M <min.m...@intel.com> > Cc: brijesh.si...@amd.com; devel@edk2.groups.io; James Bottomley > <j...@linux.ibm.com>; Yao, Jiewen <jiewen....@intel.com>; Tom Lendacky > <thomas.lenda...@amd.com>; Justen, Jordan L <jordan.l.jus...@intel.com>; > Ard Biesheuvel <ardb+tianoc...@kernel.org>; Erdem Aktas > <erdemak...@google.com>; Michael Roth <michael.r...@amd.com> > Subject: Re: [edk2-devel] [PATCH v6 06/29] OvmfPkg/ResetVector: pre-validate > the data pages used in SEC phase > > > > On 9/7/21 2:07 AM, kra...@redhat.com wrote: > > Hi, > > > >>> [ Looking at > >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww > >>> w.mail- > %2F&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb2dfcc7e0f93 > >>> > 4cacdce408d971ce2f5a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0% > 7C63 > >>> > 7665952633333113%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiL > CJQIjo > >>> > iV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=WsSaU8kFv > w1 > >>> NWES%2BYOw7xENZr9cpPgQvBjsXWkc8nkg%3D&reserved=0 > >>> archive.com/devel@edk2.groups.io/msg33605.html ] > >>> > >>> So, there isn't much tdx-specific in tdx-metadata. Most ranges are > >>> TDX_METADATA_SECTION_TYPE_TEMP_MEM which I think basically means > >>> these ranges should be accepted by the hypervisor, which is pretty > >>> much the same issue snp tries to solve with this pre-validation > >>> range. Then there are the ranges for code (aka bfv), for vars (aka cfv) > >>> and > td_hob. > >>> > >>> td_hob is the only tdx-specific item there, and even that concept > >>> (pass memory ranges as hob list from hypervisor to guest) might be > >>> useful outside tdx. > >> Mailbox is tdx-specific too. But > >> Stack/Heap/OvmfWorkarea/OvmfPageTable are common. BFV/CFV are > common too. > > > > Mailbox is tagged "TDX_METADATA_SECTION_TYPE_TEMP_MEM", so nothing > > special to do when loading the firmware, right? > > > >>> I'd suggest we generalize the tdx-metadata idea and define both > >>> generic and vmm-specific section types: > >>> > >>> enum { > >>> OVMF_SECTION_TYPE_UNDEFINED = 0; > >>> > >>> /* generic */ > >>> OVMF_SECTION_TYPE_CODE = 0x100, > >>> OVMF_SECTION_TYPE_VARS > >>> OVMF_SECTION_TYPE_SEC_MEM /* vmm should accept/validate this */ > >>> > >>> /* sev */ > >>> OVMF_SECTION_TYPE_SEV_SECRETS = 0x200, > >>> OVMF_SECTION_TYPE_SEV_CPUID /* or move to generic? */ > >>> > >>> /* tdx */ > >>> OVMV_SECTION_TYPE_TDX_TD_HOB = 0x300, }; > >>> > >>> Comments? > >> TDX has similar section type. > > > > Yes. Both TDX and SNP have simliar requirements, they want store > > memory ranges in the firmware binary in a way that allows qemu finding > > them and using them when initializing the guest. > > > > SNP stores the ranges directly in the GUID-chained block in the reset > > vector. The range types are implicit (first is pre-validate area, > > second is cpuid page, ...). > > > > TDX stores a pointer to tdx-metadata in the GUID-chained block, then > > the tdx-metadata has a list of ranges. The ranges are explicitly > > typed (section type field). > > > > The indirection used by TDX keeps the reset vector small. Also the > > explicit typing of the ranges makes it easier to extend later on if > > needed. > > > > IMHO SEV should at minimum add explicit types to the memory ranges in > > the boot block, but I'd very much prefer it if SEV and TDX can agree > > on a way to store the memory ranges. > > > >> But I am not sure if SEV can use this metadata mechanism. > >> Need SEV's comments. > > > > Brijesh? > > > > We should be able to make use of the metadata approach for the SEV-SNP. > I will update the SNP patches to use the metadata approach in next rev. > > thanks -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#80321): https://edk2.groups.io/g/devel/message/80321 Mute This Topic: https://groups.io/mt/85306660/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
