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

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

On September 25, 2021 11:21 AM, Brijesh Singh wrote:
> Hi Min,
> 
> 
> On 9/24/21 7:03 PM, Xu, Min M wrote:
> > On September 24, 2021 6:58 PM, Brijesh Singh wrote:
> >> Hi Min,
> >>
> >> On 9/21/21 4:05 AM, Min Xu wrote:
> >>>  ;
> >>>  ; Modified:  EAX, EBX, ECX, EDX
> >>>  ;
> >>>  SetCr3ForPageTables64:
> >>> -
> >>> -    ; Clear the WorkArea header. The SEV probe routines will populate the
> >>> -    ; work area when detected.
> >>> -    mov     byte[WORK_AREA_GUEST_TYPE], 0
> >> Why you are removing the above block ? The workarea hdr must be
> >> initialized to zero, its not safe to assume that the guest memory is
> >> zero'ed in the non- encrypted case.
> >>
> > Hi, Brijesh
> > Please see below explanation (It is in the commit message)
> > - In Main16 entry point, after TransitionFromReal16To32BitFlat,
> >    WORK_AREA_GUEST_TYPE is cleared to 0. WORK_AREA_GUEST_TYPE was
> >    previously cleared in SetCr3ForPageTables64 (see commit ab77b60).
> >    This doesn't work after TDX is introduced in Ovmf. It is because all
> >    TDX CPUs (BSP and APs) start to run from 0xfffffff0. In previous code
> >    WORK_AREA_GUEST_TYPE will be cleared multi-times in TDX guest. So for
> >    SEV and Legacy guest it is moved to Main16 entry point (after
> >    TransitionFromReal16To32BitFlat). For TDX guest
> WORK_AREA_GUEST_TYPE
> >    is cleared and set in InitTdxWorkarea
> 
> thanks for clarifying it.
> 
> This is very busy commit and making several changes at once, so some of
> important common code movement is getting lost. Maybe I recommend you to
> please break it into multiple. e,g  this particular change can be very easily broken
> into two commits
> 
> 1) Since TDX support need the change in the boot flow, and you are no longer
> using the Main.asm from the UefiCpuPkg. This can be a pre-patch in which you
> copy UefiCpuPkg/ResetVector/Vtf0/main.asm ->
> OvmfPkg/ResetVector/Main.asm and document reason behind the move.
> 
> 2) Remove clearing of workarea from SetCr3ForPageTables64 to Main.asm
> 
> Now that we have override for the Main.asm, I think clearing of the workarea
> should be done for all architecture (Ia32, x64) to cover the cases where
> someone builds the OVMF for 32bit or IA32 and X64.
> 
Thanks for reminder. I have updated the patch-set as you mentioned. But I am waiting for a conclusion of the Metadata, a unified metadata or two separate metadata.

Hoffmann and Jiewen
Do we have a conclusion?

Thanks
Min