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

[Yao, Jiewen]

Hey
I am not sure why Min did not response to my latest email.
I did give suggestion in my previous comment.

=====
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 at intel.com>
> Sent: Thursday, July 29, 2021 7:54 PM
> To: Brijesh Singh <brijesh.singh at amd.com>; Yao, Jiewen
> <jiewen.yao at intel.com>; devel at edk2.groups.io
> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>; Justen, Jordan L
> <jordan.l.justen at intel.com>; Erdem Aktas <erdemaktas at google.com>; James
> Bottomley <jejb at linux.ibm.com>; Tom Lendacky <thomas.lendacky at 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?



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78368): https://edk2.groups.io/g/devel/message/78368
Mute This Topic: https://groups.io/mt/84476064/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-