Re: [edk2-devel] [PATCH v8 34/46] OvmfPkg: Reserve a page in memory for the SEV-ES usage

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 5/25/20 11:00 AM, Laszlo Ersek wrote:
> On 05/19/20 23:51, Lendacky, Thomas wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7C498df3e8d335449e596508d800c4c955%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260192476035384&sdata=UKux5gXwpNe59RKQHTyk577b%2B%2FBmTIdblij8JWhXBG4%3D&reserved=0
>>
>> Reserve a fixed area of memory for SEV-ES use and set a fixed PCD,
>> PcdSevEsWorkAreaBase, to this value.
>>
>> This area will be used by SEV-ES support for two purposes:
>>    1. Communicating the SEV-ES status during BSP boot to SEC:
>>       Using a byte of memory from the page, the BSP reset vector code can
>>       communicate the SEV-ES status to SEC for use before exception
>>       handling can be enabled in SEC. After SEC, this field is no longer
>>       valid and the standard way of determine if SEV-ES is active should
>>       be used.
>>
>>    2. Establishing an area of memory for AP boot support:
>>       A hypervisor is not allowed to update an SEV-ES guest's register
>>       state, so when booting an SEV-ES guest AP, the hypervisor is not
>>       allowed to set the RIP to the guest requested value. Instead an
>>       SEV-ES AP must be re-directed from within the guest to the actual
>>       requested staring location as specified in the INIT-SIPI-SIPI
>>       sequence.
>>
>>       Use this memory for reset vector code that can be programmed to have
>>       the AP jump to the desired RIP location after starting the AP. This
>>       is required for only the very first AP reset.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   OvmfPkg/OvmfPkgX64.fdf                    |  3 +++
>>   OvmfPkg/ResetVector/ResetVector.inf       |  1 +
>>   OvmfPkg/ResetVector/Ia32/PageTables64.asm | 11 +++++++++++
>>   OvmfPkg/ResetVector/ResetVector.nasmb     |  1 +
>>   4 files changed, 16 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>> index 88b1e880e603..8836b30a0cef 100644
>> --- a/OvmfPkg/OvmfPkgX64.fdf
>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>> @@ -82,6 +82,9 @@ [FD.MEMFD]
>>   0x009000|0x002000
>>   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>>   
>> +0x00B000|0x001000
>> +gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
>> +
>>   0x010000|0x010000
>>   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>   
>> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
>> index 483fd90fe785..e94e1bfcce7e 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.inf
>> +++ b/OvmfPkg/ResetVector/ResetVector.inf
>> @@ -34,6 +34,7 @@ [BuildOptions]
>>      *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/
>>   
>>   [Pcd]
>> +  gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase
>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> index c3587a1b7814..73a4eaadb1b6 100644
>> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> @@ -89,6 +89,10 @@ SevExit:
>>   ; If SEV-ES is disabled then EAX will be zero.
>>   ;
>>   CheckSevEsFeature:
>> +    ; Initialize the first byte of the workarea to zero to communicate to
>> +    ; the SEC phase that SEV-ES is not enabled.
>> +    mov     byte[SEV_ES_WORK_AREA], 0
>> +
>>       xor       eax, eax
>>   
>>       ; SEV-ES can't be enabled if SEV isn't, so first check the encryption
>> @@ -108,6 +112,13 @@ CheckSevEsFeature:
>>       ; Restore encryption mask
>>       mov       edx, ebx
>>   
>> +    test      eax, eax
>> +    jz        NoSevEs
>> +
>> +    ; Set the first byte of the workarea to one to communicate to the SEC
>> +    ; phase that SEV-ES is enabled.
>> +    mov       byte[SEV_ES_WORK_AREA], 1
>> +
>>   NoSevEs:
>>       OneTimeCallRet CheckSevEsFeature
>>   
>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
>> index bfb77e439105..2967617bfaa0 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
>> @@ -72,6 +72,7 @@
>>     %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
>>     %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
>>     %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
>> +  %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
>>   %include "Ia32/PageTables64.asm"
>>   %endif
>>   
>>
> 
> The OvmfPkg/ResetVector modifications have been moved to this patch, at
> least in part, from patch "OvmfPkg/ResetVector: Add support for a 32-bit
> SEV check".
> 
> And I don't understand why.

I was trying to keep everything logically grouped. The early use of this 
area is to communicate the SEV-ES status to SEC and so logically I thought 
that should be done when the area was introduced.

> 
> I mean it's possible that setting the first byte of the work area to 1
> does not belong in "OvmfPkg/ResetVector: Add support for a 32-bit SEV
> check". That's OK; then said manipulation of the work area should be
> split to its own patch, which I should then review afresh.
> 
> What's not OK is to move code between two reviewed patches *and* keep my
> R-b on both.

Sorry about that. A bad assumption on my part about being able to do that 
here and in a few other places.

> 
> Please be more transparent about incremental changes.
> 
> (1) Please revert this patch to its v7 state, and keep my R-b on it.

Will do.

> 
> (2) Please split the ResetVector changes to a new patch. For the subject
> line, I suggest:
> 
> OvmfPkg/ResetVector: communicate SEV-ES status to SEC before exceptions
> 
> or something similar.

Will do.

Thanks,
Tom

> 
> Thanks
> Laszlo
>