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

["gaoliming" <gaoliming@byosoft.com.cn>]

Tom:
  This patch fixes the regression issue. So, I am OK to merge it for this stable tag. 

Thanks
Liming
> -----邮件原件-----
> 发件人: Tom Lendacky <thomas.lendacky@amd.com>
> 发送时间: 2022年5月20日 6:02
> 收件人: Ard Biesheuvel <ardb@kernel.org>; Liming Gao
> <gaoliming@byosoft.com.cn>
> 抄送: edk2-devel-groups-io <devel@edk2.groups.io>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Jiewen Yao <jiewen.yao@intel.com>; Jordan
> Justen <jordan.l.justen@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Erdem Aktas <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Michael Roth <michael.roth@amd.com>; Min Xu
> <min.m.xu@intel.com>
> 主题: Re: [edk2-devel] [PATCH] OvmfPkg: Make an Ia32/X64 hybrid build
> work with SEV
> 
> 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|gUefiOv
> mfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
> >>>
> >>> +0x008000|0x001000
> >>>
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase|gUefiOvmfPkgToken
> SpaceGuid.PcdOvmfWorkAreaSize
> >>> +
> >>>    0x010000|0x010000
> >>>
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgT
> okenSpaceGuid.PcdOvmfSecPeiTempRamSize
> >>>
> >>> @@ -87,6 +90,14 @@ [FD.MEMFD]
> >>>
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|gUefiOvmfPkgToken
> SpaceGuid.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.PcdOvmfConfidentialComputingWorkAreaHea
> der
> >>> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize =
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize -
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHea
> der
> >>>
> +#############################################################
> #############################
> >>> +
> >>>
> ##############################################################
> ##################
> >>>
> >>>    [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|gUefiOvmfPkgToken
> SpaceGuid.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.PcdOvmfConfidentialComputingWorkAreaHea
> der
> >>> +SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize =
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize -
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHea
> der
> >>>
> +#############################################################
> #############################
> >>> +
> >>>
> ##############################################################
> ##################
> >>>
> >>>    [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.PcdOvmfConfidentialComputingWorkAreaHea
> der
> >>>    SET gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize =
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize -
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfConfidentialComputingWorkAreaHea
> der
> >>> 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"