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

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

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

> Thanks!
> Min