Re: [edk2-devel] [PATCH] OvmfPkg: Make an Ia32/X64 hybrid build work with SEV

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

Explicitly adding Liming to the To: line for visibility.

Thanks,
Tom

On 5/17/22 11:29, Ard Biesheuvel wrote:
> On Tue, 17 May 2022 at 18:26, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 5/16/22 15:24, Lendacky, Thomas via groups.io wrote:
>>> The BaseMemEncryptSevLib functionality was updated to rely on the use of
>>> the OVMF/SEV workarea to check for SEV guests. However, this area is only
>>> updated when running the X64 OVMF build, not the hybrid Ia32/X64 build.
>>> Base SEV support is allowed under the Ia32/X64 build, but it now fails
>>> to boot as a result of the change.
>>>
>>> Update the ResetVector code to check for SEV features when built for
>>> 32-bit mode, not just 64-bit mode (requiring updates to both the Ia32
>>> and Ia32X64 fdf files).
>>
>> So this is a regression and it would be great if it could be applied to
>> the 202205 release. Can folks take a look and make sure it looks safe to
>> them for applying during hard feature freeze?
>>
>> If it's ok to be applied now, is there a particular process for applying
>> this during hard freeze?
>>
> 
> For the change itself:
> 
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
> 
> and I am fine with taking this during hard freeze, but I'll defer to
> Liming to make the final call.
> 
> 
> 
>>
>>>
>>> Fixes: f1d1c337e7c0575da7fd248b2dd9cffc755940df
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: Erdem Aktas <erdemaktas@google.com>
>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>> Cc: Michael Roth <michael.roth@amd.com>
>>> Cc: Min Xu <min.m.xu@intel.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> ---
>>>    OvmfPkg/OvmfPkgIa32.fdf               | 11 +++
>>>    OvmfPkg/OvmfPkgIa32X64.fdf            |  8 +++
>>>    OvmfPkg/OvmfPkgX64.fdf                |  3 +-
>>>    OvmfPkg/ResetVector/Ia32/AmdSev.asm   |  4 ++
>>>    OvmfPkg/ResetVector/Main.asm          |  6 ++
>>>    OvmfPkg/ResetVector/ResetVector.nasmb | 72 ++++++++++----------
>>>    6 files changed, 67 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf
>>> index 3ab1755749d4..57d13b7130bc 100644
>>> --- a/OvmfPkg/OvmfPkgIa32.fdf
>>> +++ b/OvmfPkg/OvmfPkgIa32.fdf
>>> @@ -76,6 +76,9 @@ [FD.MEMFD]
>>>    0x007000|0x001000
>>>    gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>>>
>>> +0x008000|0x001000
>>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize
>>> +
>>>    0x010000|0x010000
>>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>>
>>> @@ -87,6 +90,14 @@ [FD.MEMFD]
>>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
>>>    FV = DXEFV
>>>
>>> +##########################################################################################
>>> +# Set the SEV-ES specific work area PCDs (used for all forms of SEV since the
>>> +# the SEV STATUS MSR is now saved in the work area)
>>> +#
>>> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase = $(MEMFD_BASE_ADDRESS) +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
>>> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
>>> +##########################################################################################
>>> +
>>>    ################################################################################
>>>
>>>    [FV.SECFV]
>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf
>>> index e1638fa6ea38..ccde366887a9 100644
>>> --- a/OvmfPkg/OvmfPkgIa32X64.fdf
>>> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf
>>> @@ -90,6 +90,14 @@ [FD.MEMFD]
>>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
>>>    FV = DXEFV
>>>
>>> +##########################################################################################
>>> +# Set the SEV-ES specific work area PCDs (used for all forms of SEV since the
>>> +# the SEV STATUS MSR is now saved in the work area)
>>> +#
>>> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase = $(MEMFD_BASE_ADDRESS) +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
>>> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
>>> +##########################################################################################
>>> +
>>>    ################################################################################
>>>
>>>    [FV.SECFV]
>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>>> index aa9a83032d9b..438806fba8f1 100644
>>> --- a/OvmfPkg/OvmfPkgX64.fdf
>>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>>> @@ -106,7 +106,8 @@ [FD.MEMFD]
>>>    FV = DXEFV
>>>
>>>    ##########################################################################################
>>> -# Set the SEV-ES specific work area PCDs
>>> +# Set the SEV-ES specific work area PCDs (used for all forms of SEV since the
>>> +# the SEV STATUS MSR is now saved in the work area)
>>>    #
>>>    SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase = $(MEMFD_BASE_ADDRESS) +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
>>>    SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize - gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHeader
>>> diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>>> index 864d68385342..9350b0406833 100644
>>> --- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>>> +++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>>> @@ -150,6 +150,8 @@ BITS    32
>>>    SevEsUnexpectedRespTerminate:
>>>        TerminateVmgExit    TERM_UNEXPECTED_RESP_CODE
>>>
>>> +%ifdef ARCH_X64
>>> +
>>>    ; If SEV-ES is enabled then initialize and make the GHCB page shared
>>>    SevClearPageEncMaskForGhcbPage:
>>>        ; Check if SEV is enabled
>>> @@ -209,6 +211,8 @@ GetSevCBitMaskAbove31:
>>>    GetSevCBitMaskAbove31Exit:
>>>        OneTimeCallRet GetSevCBitMaskAbove31
>>>
>>> +%endif
>>> +
>>>    ; Check if Secure Encrypted Virtualization (SEV) features are enabled.
>>>    ;
>>>    ; Register usage is tight in this routine, so multiple calls for the
>>> diff --git a/OvmfPkg/ResetVector/Main.asm b/OvmfPkg/ResetVector/Main.asm
>>> index 5cfc0b5c72b1..46cfa87c4c0a 100644
>>> --- a/OvmfPkg/ResetVector/Main.asm
>>> +++ b/OvmfPkg/ResetVector/Main.asm
>>> @@ -75,6 +75,12 @@ SearchBfv:
>>>
>>>    %ifdef ARCH_IA32
>>>
>>> +    ;
>>> +    ; SEV support can be built and run using the Ia32/X64 split environment.
>>> +    ; Set the OVMF/SEV work area as appropriate.
>>> +    ;
>>> +    OneTimeCall CheckSevFeatures
>>> +
>>>        ;
>>>        ; Restore initial EAX value into the EAX register
>>>        ;
>>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
>>> index 9421f4818907..94fbb0a87b37 100644
>>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
>>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
>>> @@ -47,7 +47,36 @@
>>>    %include "Ia32/SearchForBfvBase.asm"
>>>    %include "Ia32/SearchForSecEntry.asm"
>>>
>>> -%define WORK_AREA_GUEST_TYPE (FixedPcdGet32 (PcdOvmfWorkAreaBase))
>>> +%define WORK_AREA_GUEST_TYPE          (FixedPcdGet32 (PcdOvmfWorkAreaBase))
>>> +%define PT_ADDR(Offset)               (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
>>> +
>>> +%define GHCB_PT_ADDR                  (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
>>> +%define GHCB_BASE                     (FixedPcdGet32 (PcdOvmfSecGhcbBase))
>>> +%define GHCB_SIZE                     (FixedPcdGet32 (PcdOvmfSecGhcbSize))
>>> +%define SEV_ES_WORK_AREA              (FixedPcdGet32 (PcdSevEsWorkAreaBase))
>>> +%define SEV_ES_WORK_AREA_SIZE         25
>>> +%define SEV_ES_WORK_AREA_STATUS_MSR   (FixedPcdGet32 (PcdSevEsWorkAreaBase))
>>> +%define SEV_ES_WORK_AREA_RDRAND       (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
>>> +%define SEV_ES_WORK_AREA_ENC_MASK     (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16)
>>> +%define SEV_ES_WORK_AREA_RECEIVED_VC  (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 24)
>>> +%define SEV_ES_VC_TOP_OF_STACK        (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
>>> +%define SEV_SNP_SECRETS_BASE          (FixedPcdGet32 (PcdOvmfSnpSecretsBase))
>>> +%define SEV_SNP_SECRETS_SIZE          (FixedPcdGet32 (PcdOvmfSnpSecretsSize))
>>> +%define CPUID_BASE                    (FixedPcdGet32 (PcdOvmfCpuidBase))
>>> +%define CPUID_SIZE                    (FixedPcdGet32 (PcdOvmfCpuidSize))
>>> +%define SNP_SEC_MEM_BASE_DESC_1       (FixedPcdGet32 (PcdOvmfSecPageTablesBase))
>>> +%define SNP_SEC_MEM_SIZE_DESC_1       (FixedPcdGet32 (PcdOvmfSecGhcbBase) - SNP_SEC_MEM_BASE_DESC_1)
>>> +;
>>> +; The PcdOvmfSecGhcbBase reserves two GHCB pages. The first page is used
>>> +; as GHCB shared page and second is used for bookkeeping to support the
>>> +; nested GHCB in SEC phase. The bookkeeping page is mapped private. The VMM
>>> +; does not need to validate the shared page but it need to validate the
>>> +; bookkeeping page.
>>> +;
>>> +%define SNP_SEC_MEM_BASE_DESC_2       (GHCB_BASE + 0x1000)
>>> +%define SNP_SEC_MEM_SIZE_DESC_2       (SEV_SNP_SECRETS_BASE - SNP_SEC_MEM_BASE_DESC_2)
>>> +%define SNP_SEC_MEM_BASE_DESC_3       (CPUID_BASE + CPUID_SIZE)
>>> +%define SNP_SEC_MEM_SIZE_DESC_3       (FixedPcdGet32 (PcdOvmfPeiMemFvBase) - SNP_SEC_MEM_BASE_DESC_3)
>>>
>>>    %ifdef ARCH_X64
>>>      #include <AutoGen.h>
>>> @@ -94,43 +123,14 @@
>>>      %define TDX_WORK_AREA_PGTBL_READY (FixedPcdGet32 (PcdOvmfWorkAreaBase) + 4)
>>>      %define TDX_WORK_AREA_GPAW        (FixedPcdGet32 (PcdOvmfWorkAreaBase) + 8)
>>>
>>> -  %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
>>> +  %include "X64/IntelTdxMetadata.asm"
>>> +  %include "Ia32/Flat32ToFlat64.asm"
>>> +  %include "Ia32/PageTables64.asm"
>>> +  %include "Ia32/IntelTdx.asm"
>>> +  %include "X64/OvmfSevMetadata.asm"
>>> +%endif
>>>
>>> -  %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
>>> -  %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
>>> -  %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
>>> -  %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
>>> -  %define SEV_ES_WORK_AREA_SIZE 25
>>> -  %define SEV_ES_WORK_AREA_STATUS_MSR (FixedPcdGet32 (PcdSevEsWorkAreaBase))
>>> -  %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
>>> -  %define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16)
>>> -  %define SEV_ES_WORK_AREA_RECEIVED_VC (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 24)
>>> -  %define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
>>> -  %define SEV_SNP_SECRETS_BASE  (FixedPcdGet32 (PcdOvmfSnpSecretsBase))
>>> -  %define SEV_SNP_SECRETS_SIZE  (FixedPcdGet32 (PcdOvmfSnpSecretsSize))
>>> -  %define CPUID_BASE  (FixedPcdGet32 (PcdOvmfCpuidBase))
>>> -  %define CPUID_SIZE  (FixedPcdGet32 (PcdOvmfCpuidSize))
>>> -  %define SNP_SEC_MEM_BASE_DESC_1 (FixedPcdGet32 (PcdOvmfSecPageTablesBase))
>>> -  %define SNP_SEC_MEM_SIZE_DESC_1 (FixedPcdGet32 (PcdOvmfSecGhcbBase) - SNP_SEC_MEM_BASE_DESC_1)
>>> -  ;
>>> -  ; The PcdOvmfSecGhcbBase reserves two GHCB pages. The first page is used
>>> -  ; as GHCB shared page and second is used for bookkeeping to support the
>>> -  ; nested GHCB in SEC phase. The bookkeeping page is mapped private. The VMM
>>> -  ; does not need to validate the shared page but it need to validate the
>>> -  ; bookkeeping page.
>>> -  ;
>>> -  %define SNP_SEC_MEM_BASE_DESC_2 (GHCB_BASE + 0x1000)
>>> -  %define SNP_SEC_MEM_SIZE_DESC_2 (SEV_SNP_SECRETS_BASE - SNP_SEC_MEM_BASE_DESC_2)
>>> -  %define SNP_SEC_MEM_BASE_DESC_3 (CPUID_BASE + CPUID_SIZE)
>>> -  %define SNP_SEC_MEM_SIZE_DESC_3 (FixedPcdGet32 (PcdOvmfPeiMemFvBase) - SNP_SEC_MEM_BASE_DESC_3)
>>> -
>>> -%include "X64/IntelTdxMetadata.asm"
>>> -%include "Ia32/Flat32ToFlat64.asm"
>>>    %include "Ia32/AmdSev.asm"
>>> -%include "Ia32/PageTables64.asm"
>>> -%include "Ia32/IntelTdx.asm"
>>> -%include "X64/OvmfSevMetadata.asm"
>>> -%endif
>>>
>>>    %include "Ia16/Real16ToFlat32.asm"
>>>    %include "Ia16/Init16.asm"