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/19/20 23:51, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> 
> 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 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.

Please be more transparent about incremental changes.

(1) Please revert this patch to its v7 state, and keep my R-b on it.

(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.

Thanks
Laszlo