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

[Min Xu]

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|gUefiCpuPkgTokenSpace
> 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?

> 
> Did I miss something ?
> 
> 
> >
> > 3. The structure of TEE_WORK_AREA
> >   The current SEV_ES_WORK_AREA is defined as below:
> >   typedef struct {
> >     UINT8    SevEsEnabled;
> >     UINT8    Reserved1[7];
> >     [Others...]
> > } SEC_SEV_ES_WORK_AREA;
> >
> > So I think the TEE_WORK_AREA can be:
> >   Byte[0] Type:
> >     	0: legacy 1: SEV 	2: TDX
> >   Byte[1] Subtype:
> >               If Type is 0, then it is 0
> > 	If Type is 1, then it is up to SEV's definition
> > 	If Type is 2, then it is up to TDX's definition  Byte[] other bytes
> > are defined based on the Type/Subtype
> 
> I personally like Yao Jiewen's struct definition, but I can also live with this one as
> well :). The only question I had was with his proposal was what if we remove the
> Length and Version fields. If the header length was fixed then life would be much
> easier in the ASM code.
Yao Jiewen's structure is like below. If the HeaderVersion/HeaderLength are removed
you will find it is just what I am saying. The first 2 bytes are used to distinguish the
legacy/SEV/TDX. The left bytes are up to the first 2 bytes.
typedef struct {
     UINT8    HeaderVersion; // 0
     UINT8    HeadLength; // 4
     UINT8    Type; // 0 - legacy, 1 - SEV, 2 - TDX
     UINT8    SubType; // Type specific sub type, if needed.
} CC_COMMON_WORK_AREA_HEADER;

typedef struct {
     CC_COMMON_WORK_AREA_HEADER Header;
     // reset is valid if Type == 1
     UINT8    Reserved1[4];
     UINT64   RandomData;
     UINT64   EncryptionMask;
} SEC_SEV_ES_WORK_AREA;

typedef struct {
     CC_COMMON_WORK_AREA_HEADER Header;
     // reset is valid if Type == 2
     UINT8    TdxSpecific[];  // TBD
} TDX_WORK_AREA;
> 
> 
> > I check the code in SecMain.c.
> >  SevEsIsEnabled() need updated to check SevEsWorkarea->SevEsEnabled == 1,
> not non-0.
> > @Brijesh Singh Is there any other code need update?
> 
> As noted before, the SevEsWorkAreas is used to pass the information from the
> Sec to PEI phase. The workarea gets reused for totally different purpose after
> the PEI phase.
So only the above line in SecMain.c/SevEsIsEnabled() need updated, right?
> 
Thanks!
Xu, Min


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78345): https://edk2.groups.io/g/devel/message/78345
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]
-=-=-=-=-=-=-=-=-=-=-=-