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

Thu Jul 29 10:07:57 UTC 2021

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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-