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...@intel.com> > > Sent: Thursday, July 29, 2021 7:54 PM > > To: Brijesh Singh <brijesh.si...@amd.com>; Yao, Jiewen > > <jiewen....@intel.com>; devel@edk2.groups.io > > Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>; Justen, Jordan L > > <jordan.l.jus...@intel.com>; Erdem Aktas <erdemak...@google.com>; > > James Bottomley <j...@linux.ibm.com>; Tom Lendacky > > <thomas.lenda...@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 (#78371): https://edk2.groups.io/g/devel/message/78371 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] -=-=-=-=-=-=-=-=-=-=-=-