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

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

[Rebecca Cran]

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(+)

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)
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
 
-- 
2.29.2

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

[Laszlo Ersek]

+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

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

[Lendacky, Thomas]

On 11/11/20 1:57 PM, Laszlo Ersek wrote:
> +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.

I think I missed the Bhyve support being added as I rebased to newer
levels of the tree, sorry about that.

Thanks,
Tom

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

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

[Rebecca Cran]

On 11/11/20 12:57 PM, Laszlo Ersek wrote:

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

I didn't expect people to take on the work of updating Bhyve when making 
incompatible changes to OvmfPkg, but that would be nice if they could!

It's why I have a task to set up a Jenkins CI server, so I can catch 
regressions earlier. Being a lower priority platform, I suspect it's not 
something that should go into the existing Azure/Github based CI system.


-- 
Rebecca Cran

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

[Laszlo Ersek]

On 11/12/20 06:41, Rebecca Cran wrote:
> On 11/11/20 12:57 PM, Laszlo Ersek wrote:
> 
>>
>> Optimally, these changes should have been part of the SEV-ES feature
>> series, but we didn't realize. Sorry about the regression!
> 
> I didn't expect people to take on the work of updating Bhyve when making
> incompatible changes to OvmfPkg, but that would be nice if they could!
> 
> It's why I have a task to set up a Jenkins CI server, so I can catch
> regressions earlier. Being a lower priority platform, I suspect it's not
> something that should go into the existing Azure/Github based CI system.

Well, I'm torn. I'd really like the Bhyve platform to be covered with
the other OVMF DSC files, in the same CI system. On the other hand, I'm
aware that keeping OvmfPkg in the main edk2 repository is not
universally welcomed. If Bhyve required additional work (beyond the
usual OVMF DSC files) for merging a core package series (such as for
MdeModulePkg), *I* personally wouldn't be annoyed in the least ("just do
it"), but other contributors -- not as convinced about the benefits of
having Bhyve in-tree -- could be.

If you don't mind catching Bhyve build issues "slightly after the fact",
then I suggest sticking with the status quo. If you have more time to
spend, you can still catch regressions early -- watch out for OvmfPkg
patches, and whenever something is posted for OvmfPkg, push it through
your own CI, and report back *on the list at once* if your build breaks.
I guess you could even script this somehow.

Thanks!
Laszlo