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

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

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
>