[edk2-devel] [PATCH v2 1/6] OvmfPkg/Amdsev: Base commit to build encrypted boot specific OVMF

[James Bottomley]

On Mon, 2020-11-23 at 19:01 +0100, Laszlo Ersek wrote:
> On 11/20/20 19:45, James Bottomley wrote:
> > This commit represents the file copied from OvmfPkgX64 with minor
> > changes to change the build name.
> > 
> > This package will form the basis for adding Sev specific features.
> > Since everything must go into a single rom file for attestation,
> > the separated build of code and variables is eliminated.
> > 
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3077
> > Signed-off-by: James Bottomley <jejb at linux.ibm.com>
> > 
> > ---
> > 
> > v2: remove secure boot, smm and networking
> > ---
> >  OvmfPkg/AmdSev/AmdSevX64.dsc | 867
> > +++++++++++++++++++++++++++++++++++
> >  OvmfPkg/AmdSev/AmdSevX64.fdf | 461 +++++++++++++++++++
> >  2 files changed, 1328 insertions(+)
> >  create mode 100644 OvmfPkg/AmdSev/AmdSevX64.dsc
> >  create mode 100644 OvmfPkg/AmdSev/AmdSevX64.fdf
> > 
> > diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc
> > b/OvmfPkg/AmdSev/AmdSevX64.dsc
> > new file mode 100644
> > index 000000000000..852be757bfbe
> > --- /dev/null
> > +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> > @@ -0,0 +1,867 @@
> 
> [...]
> 
> > +[LibraryClasses]
> 
> [...]
> 
> > +  VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
> 
> (1) The following two lib class resolutions are missing here:
> 
>  
> VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePoli
> cyLib.inf
>  
> VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/
> VariablePolicyHelperLib.inf
> 
> They were added to the other OVMF DSC files in commit 435a05aff54d
> ("OvmfPkg: Add VariablePolicy engine to OvmfPkg platform", 2020-11-
> 17). This dependency didn't exist at the time of your v1 posting (Nov
> 12th, in my time zone).
> 
> But now the new platform doesn't seem to build without them:
> 
> > OvmfPkg/AmdSev/AmdSevX64.dsc(...): error 4000: Instance of library
> > class [VariablePolicyLib] is not found
> >         in
> > [MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf]
> > [X64]
> >         consumed by module
> > [MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf]
> 
> They are required by
> "MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf"
> too, not just by
> "MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf"
> (which is correctly removed by this patch).
> 
> ... actually, for VariableRuntimeDxe:

Yes, I've rebased and added the necessary new libraries to get it to
build.

> On 11/20/20 19:45, James Bottomley wrote:
> 
> [...]
> 
> > +[LibraryClasses.common.DXE_RUNTIME_DRIVER]
> 
> [...]
> 
> > +  QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibF
> > wCfg.inf
> 
> (2) the following (DXE_RUNTIME_DRIVER-specific) lib class resolution
> is missing here:
> 
>  
> VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePoli
> cyLibRuntimeDxe.inf
> 
> 
> ... So I think that, for (1)+(2) together, simply the "OvmfXen.dsc"
> hunks from 435a05aff54d should be incorporated into this patch.

Yes, I based the fix on that.

> [...]
> 
> > +[PcdsFixedAtBuild]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationCh
> > ange|FALSE
> > +  gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> > +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> > +  # match PcdFlashNvStorageVariableSize purely for convenience
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> > +!endif
> > +!if $(FD_SIZE_IN_KB) == 4096
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> > +  # match PcdFlashNvStorageVariableSize purely for convenience
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
> > +!endif
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x80000
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize|0x4000
> > 0
> 
> (3) The above two PCD settings are conditional on NETWORK_TLS_ENABLE
> being TRUE, so please remove them together with the condition.

I removed them and assumed "with the condition" meant with the
condition you removed in the first iteration.

> Right now, they override the settings in the just preceding blocks
> (which depend on FD_SIZE_IN_KB being 1M / 2M / 4M).
> 
> [...]
> 
> > +[PcdsDynamicDefault]
> > +  # only set when
> > +  #   ($(SMM_REQUIRE) == FALSE)
> 
> (4) please remove the comment (only the comment)
> 
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0
> 
> [...]
> 
> > +[Components]
> 
> [...]
> 
> > +  OvmfPkg/VirtioNetDxe/VirtioNet.inf
> 
> (5) Please remove this driver from the DSC file too (you correctly
> removed it from the FDF file). Right now, the driver is built, just
> not included. We shouldn't build it.

Done.

Thanks!

James

> The rest looks good to me.
> 
> Thanks!
> Laszlo
> 




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67836): https://edk2.groups.io/g/devel/message/67836
Mute This Topic: https://groups.io/mt/78455897/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-