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

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

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? 
If so, then it means the SEV and TDX metadata will be mixed in this OvmfMetadata. 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.
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. 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. And it will be more friendly to the reviewer for the Metadata, at least from the name of the items.
> 
> Brijesh: It might be useful to post the metadata patches as separate series.
> 
> > +; Load the GDT and set the CR0, then jump to Flat 32 protected mode.
> 
> That comment isn't correct, you are already in 32-bit mode.
Thanks. It will be updated in the next version.
> 
> > +; Modified:  EAX, EBX, CR0, CR4, DS, ES, FS, GS, SS
> 
> CS too ...
It will be fixed in the next version.
> 
> > +    jmp     LINEAR_CODE_SEL:dword
> ADDR_OF(jumpToFlat32BitAndLandHere)
> > +jumpToFlat32BitAndLandHere:
> 
> ... right here.
> 
> > --- /dev/null
> > +++ b/OvmfPkg/ResetVector/Main.asm
> 
> Can you add a separate patch for "copy Main.asm from UefiCpuPkg
> unmodified" please?  Having the changes for TDX separately is helpful for
> review.
Sure. It will be separated in the next version.
> 

Thanks!
Min