Re: [PATCH] OvmfPkg/Bhyve: Update Bhyve following changes to OVMF

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

+Tom

On 11/11/20 04:10, Rebecca Cran wrote:
> Fix BhyveX64.dsc and BhyveX64.fdf to follow breaking
> changes in OVMF.
>
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> ---
>  OvmfPkg/Bhyve/BhyveX64.dsc | 1 +
>  OvmfPkg/Bhyve/BhyveX64.fdf | 6 ++++++
>  2 files changed, 7 insertions(+)

Ouch, I'm sorry.

>
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index 16d2233d77..868338b460 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -225,6 +225,7 @@
>
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> +  VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
>
>  [LibraryClasses.common.SEC]
>  !ifdef $(DEBUG_ON_SERIAL_PORT)

Yep, makes sense.

> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> index 5d2586ae14..8776aaf7ac 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> @@ -76,6 +76,12 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.
>  0x007000|0x001000
>  gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>
> +0x008000|0x001000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
> +
> +0x009000|0x002000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
> +
>  0x010000|0x010000
>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>
>

Hmm, this, on the other hand, makes me wonder. All four PCDs are
[PcdsFixedAtBuild], from "OvmfPkg.dec", so the platform DSC/FDF files
*should not* be required to override defaults.

....

Ah, wait, you're hitting the exact PCD value checks (%error directives)
in "OvmfPkg/ResetVector/ResetVector.nasmb".

During the SEV-ES review, I completely lost track of Bhyve consuming
"OvmfPkg/ResetVector/ResetVector.inf". Sorry about that.

So the following list of commits:

(1) 6995a1b79bab OvmfPkg: Create a GHCB page for use during Sec phase
(2) 8a2732186a53 OvmfPkg/ResetVector: Add support for a 32-bit SEV check
(3) 30937f2f98c4 OvmfPkg: Use the SEV-ES work area for the SEV-ES AP
                 reset vector

causes a problem for the Bhyve platform.

I don't like the "OvmfPkg/Bhyve/BhyveX64.fdf" hack as presented above,
because, while it makes the symptom go away, it causes "BhyveX64.fdf"
appear as if it had anything to do with SEV-ES -- which it doesn't.

Here's what I suggest:


* patch#1:

Subject:

  OvmfPkg/Bhyve: detach ResetVector from before the SEV-ES changes

Commit message:

  Commits 6995a1b79bab, 8a2732186a53 and 30937f2f98c4 modified all four
  regular files under "OvmfPkg/ResetVector" with SEV-ES dependencies.
  These are not relevant for Bhyve. Detach the pre-SEV-ES version of
  ResetVector for Bhyve.

Composing the patch:

  $ git checkout -b bhyve_reset_vector master
  $ rm -r OvmfPkg/ResetVector/
  $ git checkout 6995a1b79bab^ -- OvmfPkg/ResetVector/
  $ mv OvmfPkg/ResetVector/ OvmfPkg/Bhyve/
  $ git checkout master -- OvmfPkg/ResetVector/

  # add your (C) notices to all files under OvmfPkg/Bhyve/ResetVector/
  # namely "PageTables64.asm", "ResetVector.inf", "ResetVector.nasmb"

  # do *not* re-generate the FILE_GUID in the new INF file (this is a
  # well-known GUID, namely "gEfiFirmwareVolumeTopFileGuid")

  $ git add OvmfPkg/Bhyve/ResetVector/
  $ git commit


* patch#2:

Subject:

  OvmfPkg/Bhyve: fix build breakage after SEV-ES changes

Commit message:

  Consume the SEV-ES-independent reset vector restored in the previous
  patch. Use the Null instance of VmgExitLib.

Body:

> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index 16d2233d7788..ba79ceef5563 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -225,6 +225,7 @@ [LibraryClasses]
>
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> +  VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
>
>  [LibraryClasses.common.SEC]
>  !ifdef $(DEBUG_ON_SERIAL_PORT)
> @@ -571,7 +572,7 @@ [PcdsDynamicHii]
>  #
>  ################################################################################
>  [Components]
> -  OvmfPkg/ResetVector/ResetVector.inf
> +  OvmfPkg/Bhyve/ResetVector/ResetVector.inf
>
>    #
>    # SEC Phase modules
> diff --git a/OvmfPkg/Bhyve/BhyveX64.fdf b/OvmfPkg/Bhyve/BhyveX64.fdf
> index 5d2586ae141a..f4050c4934b7 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.fdf
> +++ b/OvmfPkg/Bhyve/BhyveX64.fdf
> @@ -117,7 +117,7 @@ [FV.SECFV]
>  #
>  INF  OvmfPkg/Sec/SecMain.inf
>
> -INF  RuleOverride=RESET_VECTOR OvmfPkg/ResetVector/ResetVector.inf
> +INF  RuleOverride=RESET_VECTOR OvmfPkg/Bhyve/ResetVector/ResetVector.inf
>
>  ################################################################################
>  [FV.PEIFV]

Optimally, these changes should have been part of the SEV-ES feature
series, but we didn't realize. Sorry about the regression!

Thanks
Laszlo