[PATCH v2 0/2] Ovmf: Allow IPv4 and IPv6 to be disabled at runtime

[PATCH v2 0/2] Ovmf: Allow IPv4 and IPv6 to be disabled at runtime

[Ard Biesheuvel]

Add some generic plumbing and wire it up for OvmfPkgX64 so that IPv4
and/or IPv6 networking can be turned off from the QEMU command line.

This is a follow-up to Yuan's patch '[PATCH v1 0/2] Add support to
disable VirtIo net at runtime' which only targeted the virtio network
driver specifically.i

Changes since v1:
- instead of a NULL class library that calls the Exit() boot service
  from its constructor, use a replacement for the UefiDriverEntryPoint
  library

Cc: Yuan Yu <yuanyu@google.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Pawel Polawski <ppolawsk@redhat.com>
Cc: Oliver Steffen <osteffen@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Brian J. Johnson <brian.johnson@hpe.com>

Ard Biesheuvel (2):
  OvmfPkg: Introduce alternate UefiDriverEntrypoint to inhibit driver
    load
  OvmfPkg/OvmfPkgX64: Allow runtime control of IPv4 and IPv6 support

 OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c   | 147 ++++++++++++++++++++
 OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf |  57 ++++++++
 OvmfPkg/OvmfPkg.dec                                                                           |   4 +
 OvmfPkg/OvmfPkgX64.dsc                                                                        |  14 ++
 4 files changed, 222 insertions(+)
 create mode 100644 OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c
 create mode 100644 OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf

-- 
2.35.1

[PATCH v2 1/2] OvmfPkg: Introduce alternate UefiDriverEntrypoint to inhibit driver load

[Ard Biesheuvel]

Add a new library that can be incorporated into any driver built from
source, and which permits loading of the driver to be inhibited based on
the value of a QEMU fw_cfg boolean variable. This will be used in a
subsequent patch to allow dispatch of the IPv6 and IPv6 network protocol
driver to be controlled from the QEMU command line.

This approach is based on the notion that all UEFI and DXE drivers share
a single UefiDriverEntryPoint implementation, which we can easily swap
out at build time with one that will abort execution based on the value
of some QEMU fw_cfg variable.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c   | 147 ++++++++++++++++++++
 OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf |  57 ++++++++
 OvmfPkg/OvmfPkg.dec                                                                           |   4 +
 3 files changed, 208 insertions(+)

diff --git a/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c
new file mode 100644
index 000000000000..6eaf0cfd16ad
--- /dev/null
+++ b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c
@@ -0,0 +1,147 @@
+/** @file
+  Entry point to a EFI/DXE driver. This version is specific to QEMU, and ties
+  dispatch of the driver in question on the value of a QEMU fw_cfg boolean
+  variable which is referenced by name via a fixed pointer PCD.
+
+Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2022, Google LLC. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Uefi.h>
+
+#include <Protocol/LoadedImage.h>
+
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/QemuFwCfgSimpleParserLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiDriverEntryPoint.h>
+
+/**
+  Unloads an image from memory.
+
+  This function is a callback that a driver registers to do cleanup
+  when the UnloadImage boot service function is called.
+
+  @param  ImageHandle The handle to the image to unload.
+
+  @return Status returned by all unload().
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+_DriverUnloadHandler (
+  EFI_HANDLE  ImageHandle
+  )
+{
+  EFI_STATUS  Status;
+
+  //
+  // If an UnloadImage() handler is specified, then call it
+  //
+  Status = ProcessModuleUnloadList (ImageHandle);
+
+  //
+  // If the driver specific unload handler does not return an error, then call
+  // all of the library destructors.  If the unload handler returned an error,
+  // then the driver can not be unloaded, and the library destructors should
+  // not be called
+  //
+  if (!EFI_ERROR (Status)) {
+    ProcessLibraryDestructorList (ImageHandle, gST);
+  }
+
+  //
+  // Return the status from the driver specific unload handler
+  //
+  return Status;
+}
+
+/**
+  The entry point of PE/COFF Image for a DXE Driver, DXE Runtime Driver, or
+  UEFI Driver.
+
+  @param  ImageHandle                The image handle of the DXE Driver, DXE
+                                     Runtime Driver, or UEFI Driver.
+  @param  SystemTable                A pointer to the EFI System Table.
+
+  @retval  EFI_SUCCESS               The DXE Driver, DXE Runtime Driver, or
+                                     UEFI Driver exited normally.
+  @retval  EFI_INCOMPATIBLE_VERSION  _gUefiDriverRevision is greater than
+                                     SystemTable->Hdr.Revision.
+  @retval  Other                     Return value from
+                                     ProcessModuleEntryPointList().
+
+**/
+EFI_STATUS
+EFIAPI
+_ModuleEntryPoint (
+  IN EFI_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS                 Status;
+  EFI_LOADED_IMAGE_PROTOCOL  *LoadedImage;
+  RETURN_STATUS              RetStatus;
+  BOOLEAN                    Enabled;
+
+  if (_gUefiDriverRevision != 0) {
+    //
+    // Make sure that the EFI/UEFI spec revision of the platform is >= EFI/UEFI
+    // spec revision of the driver
+    //
+    if (SystemTable->Hdr.Revision < _gUefiDriverRevision) {
+      return EFI_INCOMPATIBLE_VERSION;
+    }
+  }
+
+  //
+  // Call constructor for all libraries
+  //
+  ProcessLibraryConstructorList (ImageHandle, SystemTable);
+
+  //
+  //  Install unload handler...
+  //
+  if (_gDriverUnloadImageCount != 0) {
+    Status = gBS->HandleProtocol (
+                    ImageHandle,
+                    &gEfiLoadedImageProtocolGuid,
+                    (VOID **)&LoadedImage
+                    );
+    ASSERT_EFI_ERROR (Status);
+    LoadedImage->Unload = _DriverUnloadHandler;
+  }
+
+  RetStatus = QemuFwCfgParseBool (
+                FixedPcdGetPtr (PcdEntryPointOverrideFwCfgVarName),
+                &Enabled);
+  if (!RETURN_ERROR (RetStatus) && !Enabled) {
+    //
+    // The QEMU fw_cfg variable tells us not to load this image.  So abort.
+    //
+    Status = EFI_ABORTED;
+  } else {
+    //
+    // Call the driver entry point
+    //
+    Status = ProcessModuleEntryPointList (ImageHandle, SystemTable);
+  }
+
+  //
+  // If all of the drivers returned errors, or we if are aborting, then invoke
+  // all of the library destructors
+  //
+  if (EFI_ERROR (Status)) {
+    ProcessLibraryDestructorList (ImageHandle, SystemTable);
+  }
+
+  //
+  // Return the cumulative return status code from all of the driver entry
+  // points
+  //
+  return Status;
+}
diff --git a/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
new file mode 100644
index 000000000000..263e00ceef66
--- /dev/null
+++ b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
@@ -0,0 +1,57 @@
+## @file
+#  Entry point to a EFI/DXE driver. This version is specific to QEMU, and ties
+#  dispatch of the driver in question on the value of a QEMU fw_cfg boolean
+#  variable which is referenced by name via a fixed pointer PCD.
+#
+# Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2022, Google LLC. All rights reserved.<BR>
+#
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#
+##
+
+[Defines]
+  INF_VERSION                    = 1.29
+  BASE_NAME                      = UefiDriverEntryPointFwCfgOverrideLib
+  FILE_GUID                      = 73349b79-f148-43b8-b24e-9098a6f3e1db
+  MODULE_TYPE                    = UEFI_DRIVER
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = UefiDriverEntryPoint|DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_DRIVER
+
+[Sources]
+  UefiDriverEntryPointFwCfgOverrideLib.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[LibraryClasses]
+  BaseLib
+  DebugLib
+  QemuFwCfgSimpleParserLib
+  UefiBootServicesTableLib
+
+[Protocols]
+  gEfiLoadedImageProtocolGuid                   ## SOMETIMES_CONSUMES
+
+[FixedPcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName
+
+#
+# For UEFI drivers, these architectural protocols defined in PI 1.0 spec need
+# to be appended and merged to the final dependency section.
+#
+[Depex.common.UEFI_DRIVER]
+  gEfiBdsArchProtocolGuid AND
+  gEfiCpuArchProtocolGuid AND
+  gEfiMetronomeArchProtocolGuid AND
+  gEfiMonotonicCounterArchProtocolGuid AND
+  gEfiRealTimeClockArchProtocolGuid AND
+  gEfiResetArchProtocolGuid AND
+  gEfiRuntimeArchProtocolGuid AND
+  gEfiSecurityArchProtocolGuid AND
+  gEfiTimerArchProtocolGuid AND
+  gEfiVariableWriteArchProtocolGuid AND
+  gEfiVariableArchProtocolGuid AND
+  gEfiWatchdogTimerArchProtocolGuid
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 5af76a540529..9816aa41377d 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -399,6 +399,10 @@ [PcdsFixedAtBuild]
   ## The Tdx accept page size. 0x1000(4k),0x200000(2M)
   gUefiOvmfPkgTokenSpaceGuid.PcdTdxAcceptPageSize|0x200000|UINT32|0x65
 
+  ## The QEMU fw_cfg variable that UefiDriverEntryPointFwCfgOverrideLib will
+  #  check to decide whether to abort dispatch of the driver it is linked into.
+  gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName|""|VOID*|0x68
+
 [PcdsDynamic, PcdsDynamicEx]
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
-- 
2.35.1

[PATCH v2 2/2] OvmfPkg/OvmfPkgX64: Allow runtime control of IPv4 and IPv6 support

[Ard Biesheuvel]

Wire up the newly added UefiDriverEntrypoint in a way that ties dispatch
of the Ip4Dxe and Ip6Dxe drivers to QEMU fw_cfg variables
'opt/org.tianocore/IPv4Support' and 'opt/org.tianocore/IPv6Support'
respectively.

Setting both variables to 'n' disables IP based networking entirely,
without the need for additional code changes at the NIC driver or
network boot protocol level.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 OvmfPkg/OvmfPkgX64.dsc | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 6e68f60dc90f..2cbe35c95824 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -947,6 +947,20 @@ [Components]
       NULL|OvmfPkg/Library/PxeBcPcdProducerLib/PxeBcPcdProducerLib.inf
   }
 
+  NetworkPkg/Ip4Dxe/Ip4Dxe.inf {
+    <LibraryClasses>
+      UefiDriverEntryPoint|OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
+    <PcdsFixedAtBuild>
+      gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName|"opt/org.tianocore/IPv4Support"
+  }
+
+  NetworkPkg/Ip6Dxe/Ip6Dxe.inf {
+    <LibraryClasses>
+      UefiDriverEntryPoint|OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
+    <PcdsFixedAtBuild>
+      gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName|"opt/org.tianocore/IPv6Support"
+  }
+
 !if $(NETWORK_TLS_ENABLE) == TRUE
   NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
     <LibraryClasses>
-- 
2.35.1

Re: [PATCH v2 1/2] OvmfPkg: Introduce alternate UefiDriverEntrypoint to inhibit driver load

[Laszlo Ersek]

On 08/17/22 17:11, Ard Biesheuvel wrote:
> Add a new library that can be incorporated into any driver built from
> source, and which permits loading of the driver to be inhibited based on
> the value of a QEMU fw_cfg boolean variable. This will be used in a
> subsequent patch to allow dispatch of the IPv6 and IPv6 network protocol

(1) Still a typo? Did you mean "IPv4 and IPv6"?

> driver to be controlled from the QEMU command line.
> 
> This approach is based on the notion that all UEFI and DXE drivers share
> a single UefiDriverEntryPoint implementation, which we can easily swap
> out at build time with one that will abort execution based on the value
> of some QEMU fw_cfg variable.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c   | 147 ++++++++++++++++++++
>  OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf |  57 ++++++++
>  OvmfPkg/OvmfPkg.dec                                                                           |   4 +
>  3 files changed, 208 insertions(+)
> 
> diff --git a/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c
> new file mode 100644
> index 000000000000..6eaf0cfd16ad
> --- /dev/null
> +++ b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c
> @@ -0,0 +1,147 @@
> +/** @file
> +  Entry point to a EFI/DXE driver. This version is specific to QEMU, and ties
> +  dispatch of the driver in question on the value of a QEMU fw_cfg boolean
> +  variable which is referenced by name via a fixed pointer PCD.
> +
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2022, Google LLC. All rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Uefi.h>
> +
> +#include <Protocol/LoadedImage.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/QemuFwCfgSimpleParserLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiDriverEntryPoint.h>
> +
> +/**
> +  Unloads an image from memory.
> +
> +  This function is a callback that a driver registers to do cleanup
> +  when the UnloadImage boot service function is called.
> +
> +  @param  ImageHandle The handle to the image to unload.
> +
> +  @return Status returned by all unload().
> +
> +**/
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +_DriverUnloadHandler (
> +  EFI_HANDLE  ImageHandle
> +  )
> +{
> +  EFI_STATUS  Status;
> +
> +  //
> +  // If an UnloadImage() handler is specified, then call it
> +  //
> +  Status = ProcessModuleUnloadList (ImageHandle);
> +
> +  //
> +  // If the driver specific unload handler does not return an error, then call
> +  // all of the library destructors.  If the unload handler returned an error,
> +  // then the driver can not be unloaded, and the library destructors should
> +  // not be called
> +  //
> +  if (!EFI_ERROR (Status)) {
> +    ProcessLibraryDestructorList (ImageHandle, gST);
> +  }
> +
> +  //
> +  // Return the status from the driver specific unload handler
> +  //
> +  return Status;
> +}
> +
> +/**
> +  The entry point of PE/COFF Image for a DXE Driver, DXE Runtime Driver, or
> +  UEFI Driver.
> +
> +  @param  ImageHandle                The image handle of the DXE Driver, DXE
> +                                     Runtime Driver, or UEFI Driver.
> +  @param  SystemTable                A pointer to the EFI System Table.
> +
> +  @retval  EFI_SUCCESS               The DXE Driver, DXE Runtime Driver, or
> +                                     UEFI Driver exited normally.
> +  @retval  EFI_INCOMPATIBLE_VERSION  _gUefiDriverRevision is greater than
> +                                     SystemTable->Hdr.Revision.
> +  @retval  Other                     Return value from
> +                                     ProcessModuleEntryPointList().
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +_ModuleEntryPoint (
> +  IN EFI_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS                 Status;
> +  EFI_LOADED_IMAGE_PROTOCOL  *LoadedImage;
> +  RETURN_STATUS              RetStatus;
> +  BOOLEAN                    Enabled;
> +
> +  if (_gUefiDriverRevision != 0) {
> +    //
> +    // Make sure that the EFI/UEFI spec revision of the platform is >= EFI/UEFI
> +    // spec revision of the driver
> +    //
> +    if (SystemTable->Hdr.Revision < _gUefiDriverRevision) {
> +      return EFI_INCOMPATIBLE_VERSION;
> +    }
> +  }
> +
> +  //
> +  // Call constructor for all libraries
> +  //
> +  ProcessLibraryConstructorList (ImageHandle, SystemTable);
> +
> +  //
> +  //  Install unload handler...
> +  //
> +  if (_gDriverUnloadImageCount != 0) {
> +    Status = gBS->HandleProtocol (
> +                    ImageHandle,
> +                    &gEfiLoadedImageProtocolGuid,
> +                    (VOID **)&LoadedImage
> +                    );
> +    ASSERT_EFI_ERROR (Status);
> +    LoadedImage->Unload = _DriverUnloadHandler;
> +  }
> +
> +  RetStatus = QemuFwCfgParseBool (
> +                FixedPcdGetPtr (PcdEntryPointOverrideFwCfgVarName),
> +                &Enabled);
> +  if (!RETURN_ERROR (RetStatus) && !Enabled) {
> +    //
> +    // The QEMU fw_cfg variable tells us not to load this image.  So abort.
> +    //
> +    Status = EFI_ABORTED;
> +  } else {
> +    //
> +    // Call the driver entry point
> +    //
> +    Status = ProcessModuleEntryPointList (ImageHandle, SystemTable);
> +  }

Right, I think this is the way to do it -- we've constructed all the lib
instances, so now we can rely on PCDs and QemuFwCfg.

I'm OK with this version, just one idea: in order to avoid the code
duplication (and to benefit from future improvements to the original
entry point lib in MdePkg), we could introduce a new hook API to the
original -- forwarding the ImageHandle and SystemTable parameters to it
--, provide a Null instance implementation for the API, for the general
case, in MdePkg, and provide an fw_cfg-based implementation for OVMF /
ArmVirt.

I think it's worth a try; if Michael, Liming and Zhiguang don't like the
bit of additional complexity in MdePkg, you can still merge this
version. If you don't want to patch MdePkg though, I won't insist, of
course.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

> +
> +  //
> +  // If all of the drivers returned errors, or we if are aborting, then invoke
> +  // all of the library destructors
> +  //
> +  if (EFI_ERROR (Status)) {
> +    ProcessLibraryDestructorList (ImageHandle, SystemTable);
> +  }
> +
> +  //
> +  // Return the cumulative return status code from all of the driver entry
> +  // points
> +  //
> +  return Status;
> +}
> diff --git a/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
> new file mode 100644
> index 000000000000..263e00ceef66
> --- /dev/null
> +++ b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
> @@ -0,0 +1,57 @@
> +## @file
> +#  Entry point to a EFI/DXE driver. This version is specific to QEMU, and ties
> +#  dispatch of the driver in question on the value of a QEMU fw_cfg boolean
> +#  variable which is referenced by name via a fixed pointer PCD.
> +#
> +# Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2022, Google LLC. All rights reserved.<BR>
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 1.29
> +  BASE_NAME                      = UefiDriverEntryPointFwCfgOverrideLib
> +  FILE_GUID                      = 73349b79-f148-43b8-b24e-9098a6f3e1db
> +  MODULE_TYPE                    = UEFI_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = UefiDriverEntryPoint|DXE_DRIVER DXE_RUNTIME_DRIVER UEFI_DRIVER
> +
> +[Sources]
> +  UefiDriverEntryPointFwCfgOverrideLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  QemuFwCfgSimpleParserLib
> +  UefiBootServicesTableLib
> +
> +[Protocols]
> +  gEfiLoadedImageProtocolGuid                   ## SOMETIMES_CONSUMES
> +
> +[FixedPcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName
> +
> +#
> +# For UEFI drivers, these architectural protocols defined in PI 1.0 spec need
> +# to be appended and merged to the final dependency section.
> +#
> +[Depex.common.UEFI_DRIVER]
> +  gEfiBdsArchProtocolGuid AND
> +  gEfiCpuArchProtocolGuid AND
> +  gEfiMetronomeArchProtocolGuid AND
> +  gEfiMonotonicCounterArchProtocolGuid AND
> +  gEfiRealTimeClockArchProtocolGuid AND
> +  gEfiResetArchProtocolGuid AND
> +  gEfiRuntimeArchProtocolGuid AND
> +  gEfiSecurityArchProtocolGuid AND
> +  gEfiTimerArchProtocolGuid AND
> +  gEfiVariableWriteArchProtocolGuid AND
> +  gEfiVariableArchProtocolGuid AND
> +  gEfiWatchdogTimerArchProtocolGuid
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 5af76a540529..9816aa41377d 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -399,6 +399,10 @@ [PcdsFixedAtBuild]
>    ## The Tdx accept page size. 0x1000(4k),0x200000(2M)
>    gUefiOvmfPkgTokenSpaceGuid.PcdTdxAcceptPageSize|0x200000|UINT32|0x65
>  
> +  ## The QEMU fw_cfg variable that UefiDriverEntryPointFwCfgOverrideLib will
> +  #  check to decide whether to abort dispatch of the driver it is linked into.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName|""|VOID*|0x68
> +
>  [PcdsDynamic, PcdsDynamicEx]
>    gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
> 

Re: [PATCH v2 2/2] OvmfPkg/OvmfPkgX64: Allow runtime control of IPv4 and IPv6 support

[Laszlo Ersek]

On 08/17/22 17:11, Ard Biesheuvel wrote:
> Wire up the newly added UefiDriverEntrypoint in a way that ties dispatch
> of the Ip4Dxe and Ip6Dxe drivers to QEMU fw_cfg variables
> 'opt/org.tianocore/IPv4Support' and 'opt/org.tianocore/IPv6Support'
> respectively.
> 
> Setting both variables to 'n' disables IP based networking entirely,
> without the need for additional code changes at the NIC driver or
> network boot protocol level.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  OvmfPkg/OvmfPkgX64.dsc | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 6e68f60dc90f..2cbe35c95824 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -947,6 +947,20 @@ [Components]
>        NULL|OvmfPkg/Library/PxeBcPcdProducerLib/PxeBcPcdProducerLib.inf
>    }
>  
> +  NetworkPkg/Ip4Dxe/Ip4Dxe.inf {
> +    <LibraryClasses>
> +      UefiDriverEntryPoint|OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
> +    <PcdsFixedAtBuild>
> +      gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName|"opt/org.tianocore/IPv4Support"
> +  }
> +
> +  NetworkPkg/Ip6Dxe/Ip6Dxe.inf {
> +    <LibraryClasses>
> +      UefiDriverEntryPoint|OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
> +    <PcdsFixedAtBuild>
> +      gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName|"opt/org.tianocore/IPv6Support"
> +  }
> +
>  !if $(NETWORK_TLS_ENABLE) == TRUE
>    NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
>      <LibraryClasses>
> 

Looks good to me, but should be reflected to the other DSC files, and
perhaps (see Gerd's comments) factored out to some common include snippet.

Laszlo

Re: [PATCH v2 1/2] OvmfPkg: Introduce alternate UefiDriverEntrypoint to inhibit driver load

[Ard Biesheuvel]

On Thu, 18 Aug 2022 at 07:58, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 08/17/22 17:11, Ard Biesheuvel wrote:
> > Add a new library that can be incorporated into any driver built from
> > source, and which permits loading of the driver to be inhibited based on
> > the value of a QEMU fw_cfg boolean variable. This will be used in a
> > subsequent patch to allow dispatch of the IPv6 and IPv6 network protocol
>
> (1) Still a typo? Did you mean "IPv4 and IPv6"?
>
> > driver to be controlled from the QEMU command line.
> >
> > This approach is based on the notion that all UEFI and DXE drivers share
> > a single UefiDriverEntryPoint implementation, which we can easily swap
> > out at build time with one that will abort execution based on the value
> > of some QEMU fw_cfg variable.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c   | 147 ++++++++++++++++++++
> >  OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf |  57 ++++++++
> >  OvmfPkg/OvmfPkg.dec                                                                           |   4 +
> >  3 files changed, 208 insertions(+)
> >
> > diff --git a/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c
> > new file mode 100644
> > index 000000000000..6eaf0cfd16ad
> > --- /dev/null
> > +++ b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c
> > @@ -0,0 +1,147 @@
> > +/** @file
> > +  Entry point to a EFI/DXE driver. This version is specific to QEMU, and ties
> > +  dispatch of the driver in question on the value of a QEMU fw_cfg boolean
> > +  variable which is referenced by name via a fixed pointer PCD.
> > +
> > +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> > +Copyright (c) 2022, Google LLC. All rights reserved.<BR>
> > +SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +
> > +#include <Uefi.h>
> > +
> > +#include <Protocol/LoadedImage.h>
> > +
> > +#include <Library/BaseLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/QemuFwCfgSimpleParserLib.h>
> > +#include <Library/UefiBootServicesTableLib.h>
> > +#include <Library/UefiDriverEntryPoint.h>
> > +
> > +/**
> > +  Unloads an image from memory.
> > +
> > +  This function is a callback that a driver registers to do cleanup
> > +  when the UnloadImage boot service function is called.
> > +
> > +  @param  ImageHandle The handle to the image to unload.
> > +
> > +  @return Status returned by all unload().
> > +
> > +**/
> > +STATIC
> > +EFI_STATUS
> > +EFIAPI
> > +_DriverUnloadHandler (
> > +  EFI_HANDLE  ImageHandle
> > +  )
> > +{
> > +  EFI_STATUS  Status;
> > +
> > +  //
> > +  // If an UnloadImage() handler is specified, then call it
> > +  //
> > +  Status = ProcessModuleUnloadList (ImageHandle);
> > +
> > +  //
> > +  // If the driver specific unload handler does not return an error, then call
> > +  // all of the library destructors.  If the unload handler returned an error,
> > +  // then the driver can not be unloaded, and the library destructors should
> > +  // not be called
> > +  //
> > +  if (!EFI_ERROR (Status)) {
> > +    ProcessLibraryDestructorList (ImageHandle, gST);
> > +  }
> > +
> > +  //
> > +  // Return the status from the driver specific unload handler
> > +  //
> > +  return Status;
> > +}
> > +
> > +/**
> > +  The entry point of PE/COFF Image for a DXE Driver, DXE Runtime Driver, or
> > +  UEFI Driver.
> > +
> > +  @param  ImageHandle                The image handle of the DXE Driver, DXE
> > +                                     Runtime Driver, or UEFI Driver.
> > +  @param  SystemTable                A pointer to the EFI System Table.
> > +
> > +  @retval  EFI_SUCCESS               The DXE Driver, DXE Runtime Driver, or
> > +                                     UEFI Driver exited normally.
> > +  @retval  EFI_INCOMPATIBLE_VERSION  _gUefiDriverRevision is greater than
> > +                                     SystemTable->Hdr.Revision.
> > +  @retval  Other                     Return value from
> > +                                     ProcessModuleEntryPointList().
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +_ModuleEntryPoint (
> > +  IN EFI_HANDLE        ImageHandle,
> > +  IN EFI_SYSTEM_TABLE  *SystemTable
> > +  )
> > +{
> > +  EFI_STATUS                 Status;
> > +  EFI_LOADED_IMAGE_PROTOCOL  *LoadedImage;
> > +  RETURN_STATUS              RetStatus;
> > +  BOOLEAN                    Enabled;
> > +
> > +  if (_gUefiDriverRevision != 0) {
> > +    //
> > +    // Make sure that the EFI/UEFI spec revision of the platform is >= EFI/UEFI
> > +    // spec revision of the driver
> > +    //
> > +    if (SystemTable->Hdr.Revision < _gUefiDriverRevision) {
> > +      return EFI_INCOMPATIBLE_VERSION;
> > +    }
> > +  }
> > +
> > +  //
> > +  // Call constructor for all libraries
> > +  //
> > +  ProcessLibraryConstructorList (ImageHandle, SystemTable);
> > +
> > +  //
> > +  //  Install unload handler...
> > +  //
> > +  if (_gDriverUnloadImageCount != 0) {
> > +    Status = gBS->HandleProtocol (
> > +                    ImageHandle,
> > +                    &gEfiLoadedImageProtocolGuid,
> > +                    (VOID **)&LoadedImage
> > +                    );
> > +    ASSERT_EFI_ERROR (Status);
> > +    LoadedImage->Unload = _DriverUnloadHandler;
> > +  }
> > +
> > +  RetStatus = QemuFwCfgParseBool (
> > +                FixedPcdGetPtr (PcdEntryPointOverrideFwCfgVarName),
> > +                &Enabled);
> > +  if (!RETURN_ERROR (RetStatus) && !Enabled) {
> > +    //
> > +    // The QEMU fw_cfg variable tells us not to load this image.  So abort.
> > +    //
> > +    Status = EFI_ABORTED;
> > +  } else {
> > +    //
> > +    // Call the driver entry point
> > +    //
> > +    Status = ProcessModuleEntryPointList (ImageHandle, SystemTable);
> > +  }
>
> Right, I think this is the way to do it -- we've constructed all the lib
> instances, so now we can rely on PCDs and QemuFwCfg.
>
> I'm OK with this version, just one idea: in order to avoid the code
> duplication (and to benefit from future improvements to the original
> entry point lib in MdePkg), we could introduce a new hook API to the
> original -- forwarding the ImageHandle and SystemTable parameters to it
> --, provide a Null instance implementation for the API, for the general
> case, in MdePkg, and provide an fw_cfg-based implementation for OVMF /
> ArmVirt.
>
> I think it's worth a try; if Michael, Liming and Zhiguang don't like the
> bit of additional complexity in MdePkg, you can still merge this
> version. If you don't want to patch MdePkg though, I won't insist, of
> course.
>

This means every single DSC that describes a driver, including
out-of-tree option ROMs for graphics and network cards will need to be
updated to include a resolution. I don't think this is worth the
hassle, to be honest, at least not until we get support for defining
default resolutions as part of the library class declaration (which I
asked for a number of times)

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks. I highly appreciate that you have taken the time to join the discussion.

Will you be in Dublin next month by any chance?

Re: [edk2-devel] [PATCH v2 2/2] OvmfPkg/OvmfPkgX64: Allow runtime control of IPv4 and IPv6 support

[Ard Biesheuvel]

On Thu, 18 Aug 2022 at 08:00, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 08/17/22 17:11, Ard Biesheuvel wrote:
> > Wire up the newly added UefiDriverEntrypoint in a way that ties dispatch
> > of the Ip4Dxe and Ip6Dxe drivers to QEMU fw_cfg variables
> > 'opt/org.tianocore/IPv4Support' and 'opt/org.tianocore/IPv6Support'
> > respectively.
> >
> > Setting both variables to 'n' disables IP based networking entirely,
> > without the need for additional code changes at the NIC driver or
> > network boot protocol level.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  OvmfPkg/OvmfPkgX64.dsc | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> > index 6e68f60dc90f..2cbe35c95824 100644
> > --- a/OvmfPkg/OvmfPkgX64.dsc
> > +++ b/OvmfPkg/OvmfPkgX64.dsc
> > @@ -947,6 +947,20 @@ [Components]
> >        NULL|OvmfPkg/Library/PxeBcPcdProducerLib/PxeBcPcdProducerLib.inf
> >    }
> >
> > +  NetworkPkg/Ip4Dxe/Ip4Dxe.inf {
> > +    <LibraryClasses>
> > +      UefiDriverEntryPoint|OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
> > +    <PcdsFixedAtBuild>
> > +      gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName|"opt/org.tianocore/IPv4Support"
> > +  }
> > +
> > +  NetworkPkg/Ip6Dxe/Ip6Dxe.inf {
> > +    <LibraryClasses>
> > +      UefiDriverEntryPoint|OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf
> > +    <PcdsFixedAtBuild>
> > +      gUefiOvmfPkgTokenSpaceGuid.PcdEntryPointOverrideFwCfgVarName|"opt/org.tianocore/IPv6Support"
> > +  }
> > +
> >  !if $(NETWORK_TLS_ENABLE) == TRUE
> >    NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
> >      <LibraryClasses>
> >
>
> Looks good to me, but should be reflected to the other DSC files, and
> perhaps (see Gerd's comments) factored out to some common include snippet.
>

Fair enough, although I'm not sure where to look for Gerd's comments?
Did they make it to the list?

Re: [edk2-devel] [PATCH v2 2/2] OvmfPkg/OvmfPkgX64: Allow runtime control of IPv4 and IPv6 support

[Gerd Hoffmann]

  Hi,

> > Looks good to me, but should be reflected to the other DSC files, and
> > perhaps (see Gerd's comments) factored out to some common include snippet.
> 
> Fair enough, although I'm not sure where to look for Gerd's comments?
> Did they make it to the list?

Was a reply to v1 series.

Summary: We have alot of duplication in the Ovmf*.{dsc,fdf} files, I think
moving stuff to include files make sense (similar to OvmfTpm*.inc already in
tree) to reduce duplication, simplify maintainance and keep the build
configs in sync.

With more and more include snippets it possibly makes sense to move them
all into a subdirectory.

take care,
  Gerd

Re: [PATCH v2 1/2] OvmfPkg: Introduce alternate UefiDriverEntrypoint to inhibit driver load

[Laszlo Ersek]

On 08/22/22 18:59, Ard Biesheuvel wrote:
> On Thu, 18 Aug 2022 at 07:58, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 08/17/22 17:11, Ard Biesheuvel wrote:
>>> Add a new library that can be incorporated into any driver built from
>>> source, and which permits loading of the driver to be inhibited based on
>>> the value of a QEMU fw_cfg boolean variable. This will be used in a
>>> subsequent patch to allow dispatch of the IPv6 and IPv6 network protocol
>>
>> (1) Still a typo? Did you mean "IPv4 and IPv6"?
>>
>>> driver to be controlled from the QEMU command line.
>>>
>>> This approach is based on the notion that all UEFI and DXE drivers share
>>> a single UefiDriverEntryPoint implementation, which we can easily swap
>>> out at build time with one that will abort execution based on the value
>>> of some QEMU fw_cfg variable.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>  OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c   | 147 ++++++++++++++++++++
>>>  OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.inf |  57 ++++++++
>>>  OvmfPkg/OvmfPkg.dec                                                                           |   4 +
>>>  3 files changed, 208 insertions(+)
>>>
>>> diff --git a/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c
>>> new file mode 100644
>>> index 000000000000..6eaf0cfd16ad
>>> --- /dev/null
>>> +++ b/OvmfPkg/Library/UefiDriverEntryPointFwCfgOverrideLib/UefiDriverEntryPointFwCfgOverrideLib.c
>>> @@ -0,0 +1,147 @@
>>> +/** @file
>>> +  Entry point to a EFI/DXE driver. This version is specific to QEMU, and ties
>>> +  dispatch of the driver in question on the value of a QEMU fw_cfg boolean
>>> +  variable which is referenced by name via a fixed pointer PCD.
>>> +
>>> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>>> +Copyright (c) 2022, Google LLC. All rights reserved.<BR>
>>> +SPDX-License-Identifier: BSD-2-Clause-Patent
>>> +
>>> +**/
>>> +
>>> +#include <Uefi.h>
>>> +
>>> +#include <Protocol/LoadedImage.h>
>>> +
>>> +#include <Library/BaseLib.h>
>>> +#include <Library/DebugLib.h>
>>> +#include <Library/QemuFwCfgSimpleParserLib.h>
>>> +#include <Library/UefiBootServicesTableLib.h>
>>> +#include <Library/UefiDriverEntryPoint.h>
>>> +
>>> +/**
>>> +  Unloads an image from memory.
>>> +
>>> +  This function is a callback that a driver registers to do cleanup
>>> +  when the UnloadImage boot service function is called.
>>> +
>>> +  @param  ImageHandle The handle to the image to unload.
>>> +
>>> +  @return Status returned by all unload().
>>> +
>>> +**/
>>> +STATIC
>>> +EFI_STATUS
>>> +EFIAPI
>>> +_DriverUnloadHandler (
>>> +  EFI_HANDLE  ImageHandle
>>> +  )
>>> +{
>>> +  EFI_STATUS  Status;
>>> +
>>> +  //
>>> +  // If an UnloadImage() handler is specified, then call it
>>> +  //
>>> +  Status = ProcessModuleUnloadList (ImageHandle);
>>> +
>>> +  //
>>> +  // If the driver specific unload handler does not return an error, then call
>>> +  // all of the library destructors.  If the unload handler returned an error,
>>> +  // then the driver can not be unloaded, and the library destructors should
>>> +  // not be called
>>> +  //
>>> +  if (!EFI_ERROR (Status)) {
>>> +    ProcessLibraryDestructorList (ImageHandle, gST);
>>> +  }
>>> +
>>> +  //
>>> +  // Return the status from the driver specific unload handler
>>> +  //
>>> +  return Status;
>>> +}
>>> +
>>> +/**
>>> +  The entry point of PE/COFF Image for a DXE Driver, DXE Runtime Driver, or
>>> +  UEFI Driver.
>>> +
>>> +  @param  ImageHandle                The image handle of the DXE Driver, DXE
>>> +                                     Runtime Driver, or UEFI Driver.
>>> +  @param  SystemTable                A pointer to the EFI System Table.
>>> +
>>> +  @retval  EFI_SUCCESS               The DXE Driver, DXE Runtime Driver, or
>>> +                                     UEFI Driver exited normally.
>>> +  @retval  EFI_INCOMPATIBLE_VERSION  _gUefiDriverRevision is greater than
>>> +                                     SystemTable->Hdr.Revision.
>>> +  @retval  Other                     Return value from
>>> +                                     ProcessModuleEntryPointList().
>>> +
>>> +**/
>>> +EFI_STATUS
>>> +EFIAPI
>>> +_ModuleEntryPoint (
>>> +  IN EFI_HANDLE        ImageHandle,
>>> +  IN EFI_SYSTEM_TABLE  *SystemTable
>>> +  )
>>> +{
>>> +  EFI_STATUS                 Status;
>>> +  EFI_LOADED_IMAGE_PROTOCOL  *LoadedImage;
>>> +  RETURN_STATUS              RetStatus;
>>> +  BOOLEAN                    Enabled;
>>> +
>>> +  if (_gUefiDriverRevision != 0) {
>>> +    //
>>> +    // Make sure that the EFI/UEFI spec revision of the platform is >= EFI/UEFI
>>> +    // spec revision of the driver
>>> +    //
>>> +    if (SystemTable->Hdr.Revision < _gUefiDriverRevision) {
>>> +      return EFI_INCOMPATIBLE_VERSION;
>>> +    }
>>> +  }
>>> +
>>> +  //
>>> +  // Call constructor for all libraries
>>> +  //
>>> +  ProcessLibraryConstructorList (ImageHandle, SystemTable);
>>> +
>>> +  //
>>> +  //  Install unload handler...
>>> +  //
>>> +  if (_gDriverUnloadImageCount != 0) {
>>> +    Status = gBS->HandleProtocol (
>>> +                    ImageHandle,
>>> +                    &gEfiLoadedImageProtocolGuid,
>>> +                    (VOID **)&LoadedImage
>>> +                    );
>>> +    ASSERT_EFI_ERROR (Status);
>>> +    LoadedImage->Unload = _DriverUnloadHandler;
>>> +  }
>>> +
>>> +  RetStatus = QemuFwCfgParseBool (
>>> +                FixedPcdGetPtr (PcdEntryPointOverrideFwCfgVarName),
>>> +                &Enabled);
>>> +  if (!RETURN_ERROR (RetStatus) && !Enabled) {
>>> +    //
>>> +    // The QEMU fw_cfg variable tells us not to load this image.  So abort.
>>> +    //
>>> +    Status = EFI_ABORTED;
>>> +  } else {
>>> +    //
>>> +    // Call the driver entry point
>>> +    //
>>> +    Status = ProcessModuleEntryPointList (ImageHandle, SystemTable);
>>> +  }
>>
>> Right, I think this is the way to do it -- we've constructed all the lib
>> instances, so now we can rely on PCDs and QemuFwCfg.
>>
>> I'm OK with this version, just one idea: in order to avoid the code
>> duplication (and to benefit from future improvements to the original
>> entry point lib in MdePkg), we could introduce a new hook API to the
>> original -- forwarding the ImageHandle and SystemTable parameters to it
>> --, provide a Null instance implementation for the API, for the general
>> case, in MdePkg, and provide an fw_cfg-based implementation for OVMF /
>> ArmVirt.
>>
>> I think it's worth a try; if Michael, Liming and Zhiguang don't like the
>> bit of additional complexity in MdePkg, you can still merge this
>> version. If you don't want to patch MdePkg though, I won't insist, of
>> course.
>>
> 
> This means every single DSC that describes a driver, including
> out-of-tree option ROMs for graphics and network cards will need to be
> updated to include a resolution. I don't think this is worth the
> hassle, to be honest,

good point

> at least not until we get support for defining
> default resolutions as part of the library class declaration (which I
> asked for a number of times)

this one too

> 
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
> 
> Thanks. I highly appreciate that you have taken the time to join the discussion.
> 
> Will you be in Dublin next month by any chance?
> 

unfortunately not -- the location is good for me, but the time is not,
it conflicts again with my kids' school schedule.

Cheers!
Laszlo

Re: [edk2-devel] [PATCH v2 2/2] OvmfPkg/OvmfPkgX64: Allow runtime control of IPv4 and IPv6 support

[Ard Biesheuvel]

On Tue, 23 Aug 2022 at 09:04, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > Looks good to me, but should be reflected to the other DSC files, and
> > > perhaps (see Gerd's comments) factored out to some common include snippet.
> >
> > Fair enough, although I'm not sure where to look for Gerd's comments?
> > Did they make it to the list?
>
> Was a reply to v1 series.
>
> Summary: We have alot of duplication in the Ovmf*.{dsc,fdf} files, I think
> moving stuff to include files make sense (similar to OvmfTpm*.inc already in
> tree) to reduce duplication, simplify maintainance and keep the build
> configs in sync.
>
> With more and more include snippets it possibly makes sense to move them
> all into a subdirectory.
>

OK, that is a good point. i will take this into account for the next revision.

Thanks,
Ard.