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

["Yao, Jiewen" <jiewen.yao@intel.com>]

I can explain why we prefer DQ instead of DD.

You are right that current TD entrypoint is 32bit. However, we cannot predict that is always TRUE for the future.

Back to 16 bit machine, we have entrypoint at F000:FFF0. But in 32bit mode we removed 1M limitation and move entrypoint to below 4G.
There is no limitation that we move 64bit entrypoint above 4G for 64bit machine.
We still choose 4G for easy implementation and compatibility consideration.

And we still leave room for extension in the future. For example, the firmware information table (FIT) defines 64bit address, although only 32bit is used today. (https://software.intel.com/content/dam/develop/external/us/en/documents/firmware-interface-table-bios-specification-r1p2p1.pdf). 

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Tuesday, September 7, 2021 3:08 PM
> To: Xu, Min M <min.m.xu@intel.com>
> Cc: devel@edk2.groups.io; Brijesh Singh <brijesh.singh@amd.com>; James
> Bottomley <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom
> Lendacky <thomas.lendacky@amd.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Erdem Aktas <erdemaktas@google.com>; Michael Roth
> <Michael.Roth@amd.com>
> Subject: Re: [edk2-devel] [PATCH v6 06/29] OvmfPkg/ResetVector: pre-validate
> the data pages used in SEC phase
> 
>   Hi,
> 
> > > [ Looking at https://www.mail-
> > > 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?
> 
> > > Looking at tdx-metadata I have a few questions:
> > >
> > > +_Bfv:
> > > +  DD TDX_BFV_RAW_DATA_OFFSET
> > > +  DD TDX_BFV_RAW_DATA_SIZE
> > >
> > > What is this and why is it needed?
> > Host VMM need to measure the code part (BFV) to MRTD register
> > (which is similar to TPM PCRs).
> 
> Sure, but why you can't use TDX_BFV_MEMORY_BASE +
> TDX_BFV_MEMORY_SIZE
> for that?  The tdvf design guide says it is the file offset.  The
> firmware must be mapped right below 4G, which implies
> RAW_DATA_OFFSET + (4G - FIRMWARE_SIZE) == MEMORY_BASE.
> So having both RAW_DATA_OFFSET and MEMORY_BASE looks redundant to me.
> 
> > > +  DQ TDX_BFV_MEMORY_BASE
> > > +  DQ TDX_BFV_MEMORY_SIZE
> > >
> > > Why "DQ"?  TDX is defined to start in 32bit mode, so you can hardly have
> > > addresses here which do not fit into "DD", correct?
> > Those are the memory. TDX is running in long mode. So it is DQ.
> 
> Hmm?  TDX entry vector is defined to be 32bit.  Which pretty much
> implies you can't map the firmware above 4G, even if one of the first
> actions of the reset vector code is the switch to long mode.
> 
> take care,
>   Gerd
> 
> 
> 
> 
>