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/26/20 9:28 AM, Tom Lendacky wrote:
> 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.

Actually, these changes can remain part of the revert to the v6 version of 
patch 36 ("OvmfPkg/ResetVector: Add support for a 32-bit SEV check") so 
that no changes are seen in that patch from the original v6 that was reviewed.

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>
>> Thanks
>> Laszlo
>>