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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-