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

["Laszlo Ersek" <lersek@redhat.com>]

On 05/26/20 23:47, Tom Lendacky wrote:
> 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.

Yes, that works perfectly fine too; I just thought you really wanted to
move the "communicate SEV-ES status to SEC before exceptions" part to a
different patch.

If you are OK with keeping version 6 of the patch, I'm completely OK
with that!

When you post v9 of the series, please explain in the blurb what patches
you've re-set to version 6.

If you can use "git notes" (or manual editing) to mark the same
information on the individual patches (outside of their commit
messages), that's even better.

Thanks!
Laszlo