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

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

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@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/VariablePolicyLib.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:

On 11/20/20 19:45, James Bottomley wrote:

[...]

> +[LibraryClasses.common.DXE_RUNTIME_DRIVER]

[...]

> +  QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf

(2) the following (DXE_RUNTIME_DRIVER-specific) lib class resolution is
missing here:

  VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf


... So I think that, for (1)+(2) together, simply the "OvmfXen.dsc"
hunks from 435a05aff54d should be incorporated into this patch.

[...]

> +[PcdsFixedAtBuild]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|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|0x40000

(3) The above two PCD settings are conditional on NETWORK_TLS_ENABLE
being TRUE, so please remove them together with the condition.

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.

The rest looks good to me.

Thanks!
Laszlo