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

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

On July 29, 2021 8:13 PM, Yao Jiewen wrote:
> Hey
> I am not sure why Min did not response to my latest email.
> I did give suggestion in my previous comment.
> 
Ah, sorry I missed it. There are too many mails. 
> =====
> CcWorkArea.Type = 0;
> InitCcWorkAreaSev(); // set Type=1 if SEV InitCcWorkAreaTdx(); // set Type=2 if
> TDX =====
> 
> That is option 1.
> 
> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: Xu, Min M <min.m.xu@intel.com>
> > Sent: Thursday, July 29, 2021 7:54 PM
> > To: Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; devel@edk2.groups.io
> > 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 V3 06/10] OvmfPkg: Add AmdSev.asm in
> > ResetVector
> >
> > 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?