Re: [edk2-devel] [PATCH 05/10] OvmfPkg/ResetVector: split SEV and non-CoCo workflows

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

On 2/22/24 12:54, Gerd Hoffmann wrote:
> Use separate control flows for SEV and non-CoCo cases.
> 
> SevClearPageEncMaskForGhcbPage and GetSevCBitMaskAbove31 will now only
> be called when running in SEV mode, so the SEV check in these functions
> is not needed any more.
> 
> No functional change.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/ResetVector/Ia32/AmdSev.asm       | 16 ++--------------
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm | 17 ++++++++++++++---
>  2 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> index 043c88a7abbe..ed94f1dc668f 100644
> --- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> +++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
> @@ -152,12 +152,8 @@ SevEsUnexpectedRespTerminate:
>  
>  %ifdef ARCH_X64
>  
> -; If SEV-ES is enabled then initialize and make the GHCB page shared
> +; initialize and make the GHCB page shared

(1) This comment update is unjustified, I suggest reverting it.

(The SEV check is indeed superfluous below, but you -- correctly -- keep
the SEV-ES check, and the comment here is about SEV-ES, not SEV. Because
the check stays, the comment should stay too.)

>  SevClearPageEncMaskForGhcbPage:
> -    ; Check if SEV is enabled
> -    cmp       byte[WORK_AREA_GUEST_TYPE], 1
> -    jnz       SevClearPageEncMaskForGhcbPageExit
> -
>      ; Check if SEV-ES is enabled
>      mov       ecx, 1
>      bt        [SEV_ES_WORK_AREA_STATUS_MSR], ecx
> @@ -195,20 +191,12 @@ pageTableEntries4kLoop:
>  SevClearPageEncMaskForGhcbPageExit:
>      OneTimeCallRet SevClearPageEncMaskForGhcbPage
>  
> -; Check if SEV is enabled, and get the C-bit mask above 31.
> +; Get the C-bit mask above 31.
>  ; Modified: EDX
>  ;
>  ; The value is returned in the EDX
>  GetSevCBitMaskAbove31:
> -    xor       edx, edx
> -
> -    ; Check if SEV is enabled
> -    cmp       byte[WORK_AREA_GUEST_TYPE], 1
> -    jnz       GetSevCBitMaskAbove31Exit
> -
>      mov       edx, dword[SEV_ES_WORK_AREA_ENC_MASK + 4]
> -
> -GetSevCBitMaskAbove31Exit:
>      OneTimeCallRet GetSevCBitMaskAbove31
>  
>  %endif
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index 166e80293c89..84a7b4efc019 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -118,15 +118,26 @@ SetCr3ForPageTables64:
>  
>      ; Check whether the SEV is active and populate the SevEsWorkArea
>      OneTimeCall   CheckSevFeatures
> +    cmp       byte[WORK_AREA_GUEST_TYPE], 1
> +    jz        SevInit
>  
> +    ;
> +    ; normal (non-CoCo) workflow
> +    ;
> +    ClearOvmfPageTables
> +    CreatePageTables4Level 0
> +    jmp SetCr3
> +
> +SevInit:
> +    ;
> +    ; SEV workflow
> +    ;
> +    ClearOvmfPageTables
>      ; If SEV is enabled, the C-bit position is always above 31.
>      ; The mask will be saved in the EDX and applied during the
>      ; the page table build below.
>      OneTimeCall   GetSevCBitMaskAbove31
> -
> -    ClearOvmfPageTables
>      CreatePageTables4Level edx
> -
>      ; Clear the C-bit from the GHCB page if the SEV-ES is enabled.
>      OneTimeCall   SevClearPageEncMaskForGhcbPage
>      jmp SetCr3

Nice.

The patch also sneakily reorders ClearOvmfPageTables against
GetSevCBitMaskAbove31 -- but that's an improvement: this way we no
longer depend on ClearOvmfPageTables not modifying EDX; instead, EDX
directly passes from GetSevCBitMaskAbove31 to CreatePageTables4Level.

With (1) undone:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116084): https://edk2.groups.io/g/devel/message/116084
Mute This Topic: https://groups.io/mt/104506794/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-