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|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?
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.
thanks
>
>> 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 (#78362): https://edk2.groups.io/g/devel/message/78362
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]
-=-=-=-=-=-=-=-=-=-=-=-
