public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] OvmfPkg/Bhyve: Update Bhyve following changes to OVMF
@ 2020-11-11  3:10 Rebecca Cran
  2020-11-11 19:57 ` Laszlo Ersek
  0 siblings, 1 reply; 5+ messages in thread
From: Rebecca Cran @ 2020-11-11  3:10 UTC (permalink / raw)
  To: devel
  Cc: Rebecca Cran, Jordan Justen, Laszlo Ersek, Ard Biesheuvel,
	Peter Grehan

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



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] OvmfPkg/Bhyve: Update Bhyve following changes to OVMF
  2020-11-11  3:10 [PATCH] OvmfPkg/Bhyve: Update Bhyve following changes to OVMF Rebecca Cran
@ 2020-11-11 19:57 ` Laszlo Ersek
  2020-11-11 20:51   ` Lendacky, Thomas
  2020-11-12  5:41   ` Rebecca Cran
  0 siblings, 2 replies; 5+ messages in thread
From: Laszlo Ersek @ 2020-11-11 19:57 UTC (permalink / raw)
  To: Rebecca Cran
  Cc: devel, Jordan Justen, Ard Biesheuvel, Peter Grehan, Tom Lendacky

+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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] OvmfPkg/Bhyve: Update Bhyve following changes to OVMF
  2020-11-11 19:57 ` Laszlo Ersek
@ 2020-11-11 20:51   ` Lendacky, Thomas
  2020-11-12  5:41   ` Rebecca Cran
  1 sibling, 0 replies; 5+ messages in thread
From: Lendacky, Thomas @ 2020-11-11 20:51 UTC (permalink / raw)
  To: Laszlo Ersek, Rebecca Cran
  Cc: devel, Jordan Justen, Ard Biesheuvel, Peter Grehan

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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] OvmfPkg/Bhyve: Update Bhyve following changes to OVMF
  2020-11-11 19:57 ` Laszlo Ersek
  2020-11-11 20:51   ` Lendacky, Thomas
@ 2020-11-12  5:41   ` Rebecca Cran
  2020-11-13 19:39     ` Laszlo Ersek
  1 sibling, 1 reply; 5+ messages in thread
From: Rebecca Cran @ 2020-11-12  5:41 UTC (permalink / raw)
  To: Laszlo Ersek
  Cc: devel, Jordan Justen, Ard Biesheuvel, Peter Grehan, Tom Lendacky

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] OvmfPkg/Bhyve: Update Bhyve following changes to OVMF
  2020-11-12  5:41   ` Rebecca Cran
@ 2020-11-13 19:39     ` Laszlo Ersek
  0 siblings, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2020-11-13 19:39 UTC (permalink / raw)
  To: Rebecca Cran
  Cc: devel, Jordan Justen, Ard Biesheuvel, Peter Grehan, Tom Lendacky

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-13 19:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-11  3:10 [PATCH] OvmfPkg/Bhyve: Update Bhyve following changes to OVMF Rebecca Cran
2020-11-11 19:57 ` Laszlo Ersek
2020-11-11 20:51   ` Lendacky, Thomas
2020-11-12  5:41   ` Rebecca Cran
2020-11-13 19:39     ` Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox