Re: [edk2-devel] [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector

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

On July 29, 2021 6:08 PM, Brijesh Singh wrote:
> On 7/29/21 1:07 AM, Xu, Min M wrote:
> > On July 29, 2021 12:29 PM, Brijesh Singh wrote:
> >> On 7/28/21 9:44 PM, Xu, Min M wrote:
> >>> Jiewen & Singh
> >>>
> >>> From the discussion I am thinking we have below rules to follow to
> >>> the design the structure of TEE_WORK_AREA:
> >>> 1. Design should be flexible but not too complicated 2. Reuse the
> >>> current SEV_ES_WORK_AREA (PcdSevEsWorkAreaBase) as
> TEE_WORK_AREA 3.
> >>> TEE_WORK_AREA should be initialized to all-0 at the beginning of
> >>> ResetVecotr 4. Reduce the changes to exiting code if possible
> >>>
> >>> So I try to make below conclusions below: (Please review) 1.
> >>> SEV_ES_WORK_AREA is used as the TEE_WORK_AREA by both TDX and
> SEV,
> >>> maybe in the future it can be used by other CC technologies.
> >>>
> >>> 2. In MEMFD, add below initial value. So that TEE_WORK_AREA is
> >>> guaranteed to be cleared in legacy guest. In TDX this memory region
> >>> is initialized to be all-0 by host VMM. In SEV the memory region is
> cleared as well.
> >>>   0x00B000|0x001000
> >>>
> >>
> gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpa
> ce
> >> Guid.PcdSevEsWorkAreaSize
> >>>   DATA = {
> >>>     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> >>>     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> >>>     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> >>>     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> >>>   }
> >> Hmm, I thought the contents of the data pages are controlled by the host
> VMM.
> >> If the backing pages are not zero filled then there is no guarantee
> >> that memory will be zero.  To verify it:
> >>
> >> 1. I applied your above change in OvmfPkgX86.fdt. I modified the DATA
> >> values from 0x00 -> 0xCC
> >>
> >> 2. Modified the SecMain.c to dump the SevEsWorkArea on entry
> >>
> >> And dump does not contain the 0xcc.
> >>
> >> And to confirm further,  I attached to the qemu with the GDB before
> >> the booting the OVMF, and modified the SevEsWorkArea with some
> >> garbage number  and this time the dump printed garbage value I put
> through the debugger.
> >>
> >> In summary, the OVMF to zero the workarea memory on the entry and
> we
> >> cannot rely on the DATA={0x00, 0x00...} to zero the CCWorkArea.
> > So in legacy guest, CCWorkArea is cleared to all-0 without the
> DATA={0x00,0x00...}, right?
> 
> Okay, maybe I was not able to communicate it correctly.
> 
> The run I did is for the legacy guest. For the legacy guest, the contents of the
> CCWorkArea may *not* be always zero even when you use the DATA={0x00,
> 0x00...}.
> 
> Currently, Qemu uses zero filled backing pages, so we will get a zero filled
> CCWorkArea; but nothing says that a backing page *must* be zero.
> Another VMM may choose to do things differently. In summary, the OVMF
> reset vector code must zero  the CCWorkArea  before calling SEV or TDX
> probes.
> 
Ah, I see. 
In current CheckSevFeatures, byte[SEV_ES_WORK_AREA] is cleared to0. 
Then its values is set based on the result of SEV probe. 

There is a bug here. CheckTdxFeatures does the similar work and it sets the 
WORK_AREA to 2. If CheckSevFeatures is called after CheckTdxFeatures, then
WORK_AREA is cleared and it is set to 0 because it is not SEV. The value is override.

I think there are 2 options:
Option 1:
Neither CheckTdxFeatures nor CheckSevFeatures should clear WORK_AREA. Instead
It should be cleared to 0 outside and before these 2 calls. So in Main16 after
TransitionFromReal16To32BitFlat WORK_AREA is cleared to 0. In Tdx guest this WORK_AREA
is initialized to 0 by host VMM.

Option 2:
Another option is to figure out a mechanism that only one CheckXXXFeatures is called.
Since there are 2 entry point in Main.asm: Main16 and Main32.
In Main16 CheckSevFeatures is called after TransitionFromReal16To32BitFlat. (eax should
be saved because it is used in SetCr3ForPageTables64)
In Main32 CheckTdxFeatures is called after ReloadFlat32.

What's your opinion?