Re: [edk2-devel] [PATCH v6 06/29] OvmfPkg/ResetVector: pre-validate the data pages used in SEC phase

["Brijesh Singh" <brijesh.singh@amd.com>]

On 9/7/21 2:07 AM, kraxel@redhat.com wrote:
>    Hi,
> 
>>> [ Looking at https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.mail-%2F&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cb2dfcc7e0f934cacdce408d971ce2f5a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637665952633333113%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=WsSaU8kFvw1NWES%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