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

Thu Jul 29 12:46:09 UTC 2021

On 7/29/21 7:12 AM, 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.
>
> =====
> CcWorkArea.Type = 0;
> InitCcWorkAreaSev(); // set Type=1 if SEV
> InitCcWorkAreaTdx(); // set Type=2 if TDX
> =====
>
> That is option 1.

Yes that is exactly what we want Jiewen. 

The OvmfPkg reset vector should initialize the type to zero on entry,
and SEV/TDX will update the value (only if the feature is detected).

> Thank you
> Yao Jiewen
>
>> -----Original Message-----
>> From: Xu, Min M <min.m.xu at intel.com>
>> Sent: Thursday, July 29, 2021 7:54 PM
>> To: Brijesh Singh <brijesh.singh at amd.com>; Yao, Jiewen
>> <jiewen.yao at intel.com>; devel at edk2.groups.io
>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>; Justen, Jordan L
>> <jordan.l.justen at intel.com>; Erdem Aktas <erdemaktas at google.com>; James
>> Bottomley <jejb at linux.ibm.com>; Tom Lendacky <thomas.lendacky at 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 (#78370): https://edk2.groups.io/g/devel/message/78370
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]
-=-=-=-=-=-=-=-=-=-=-=-