Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

["Min Xu" <min.m.xu@intel.com>]

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.xu@intel.com>
> >> Sent: Thursday, September 23, 2021 9:20 PM
> >> To: devel@edk2.groups.io; brijesh.singh@amd.com; Yao, Jiewen
> >> <jiewen.yao@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> >> <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>;
> >> James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> >> <thomas.lendacky@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-
> &amp;data=04%7C01%7Cbrijesh.sin
> >>
> gh%40amd.com%7C52f6327efa33480bf4a308d97e977ff0%7C3dd8961fe488
> 4e608e1
> >>
> 1a82d994e183d%7C0%7C0%7C637680011416117826%7CUnknown%7CTWF
> pbGZsb3d8ey
> >>
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C10
> >>
> 00&amp;sdata=at43H9sgmlaD773wsU5%2BoPPSjImo0UiCxQ0nmwdV9ds%3D
> &amp;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 <kraxel@redhat.com>
> >>>>> Sent: Thursday, September 23, 2021 4:48 PM
> >>>>> To: Xu, Min M <min.m.xu@intel.com>
> >>>>> Cc: devel@edk2.groups.io; Ard Biesheuvel
> >>>>> <ardb+tianocore@kernel.org>; Justen, Jordan L
> >>>>> <jordan.l.justen@intel.com>; Brijesh Singh
> >>>>> <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>;
> >> James
> >>>>> Bottomley <jejb@linux.ibm.com>; Yao, Jiewen
> >>>>> <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@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
> >>>
> >>>
> >>> 
> >>>
> >