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

[PATCH 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.

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>

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

 OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c   | 30 ++++++++++++++++++++
 OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf | 28 ++++++++++++++++++
 OvmfPkg/OvmfPkg.dec                                               |  4 +++
 OvmfPkg/OvmfPkgX64.dsc                                            | 14 +++++++++
 4 files changed, 76 insertions(+)
 create mode 100644 OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c
 create mode 100644 OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf

-- 
2.35.1

[PATCH 1/2] OvmfPkg: Introduce NULL class library 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.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c   | 30 ++++++++++++++++++++
 OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf | 28 ++++++++++++++++++
 OvmfPkg/OvmfPkg.dec                                               |  4 +++
 3 files changed, 62 insertions(+)

diff --git a/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c
new file mode 100644
index 000000000000..dc8544bc38be
--- /dev/null
+++ b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c
@@ -0,0 +1,30 @@
+// @file
+// Copyright (c) 2022, Google LLC. All rights reserved.<BR>
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+
+#include <PiDxe.h>
+
+#include <Library/QemuFwCfgSimpleParserLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+STATIC CHAR16 mExitData[] = L"Driver dispatch inhibited by QEMU fw_cfg variable.";
+
+EFI_STATUS
+EFIAPI
+DriverLoadInhibitorLibConstructor (
+  IN  EFI_HANDLE        Handle,
+  IN  EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  RETURN_STATUS     Status;
+  BOOLEAN           Enabled;
+
+  Status = QemuFwCfgParseBool (FixedPcdGetPtr (PcdDriverInhibitorFwCfgVarName),
+             &Enabled);
+  if (!RETURN_ERROR (Status) && !Enabled) {
+    return gBS->Exit (Handle, EFI_REQUEST_UNLOAD_IMAGE, sizeof mExitData,
+                  mExitData);
+  }
+  return EFI_SUCCESS;
+}
diff --git a/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
new file mode 100644
index 000000000000..ed521d12d335
--- /dev/null
+++ b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
@@ -0,0 +1,28 @@
+## @file
+#  Copyright (c) 2022, Google LLC. All rights reserved.<BR>
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 1.29
+  BASE_NAME                      = DriverLoadInhibitorLib
+  FILE_GUID                      = af4c2c0b-f7ed-4d61-ad97-5953982c3531
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = NULL
+  CONSTRUCTOR                    = DriverLoadInhibitorLibConstructor
+
+[Sources]
+  DriverLoadInhibitorLib.c
+
+[LibraryClasses]
+  QemuFwCfgSimpleParserLib
+  UefiBootServicesTableLib
+
+[Packages]
+  MdePkg/MdePkg.dec
+  OvmfPkg/OvmfPkg.dec
+
+[FixedPcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 5af76a540529..e9a22cab088c 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 DriverLoadInhibitorLib will check to
+  #  decide whether to abort dispatch of the driver it is linked into.
+  gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName|""|VOID*|0x68
+
 [PcdsDynamic, PcdsDynamicEx]
   gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
-- 
2.35.1

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

[Ard Biesheuvel]

Wire up the newly added DriverLoadInhibitorLib 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..0c0ded88f86e 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>
+      NULL|OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
+    <PcdsFixedAtBuild>
+      gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName|"opt/org.tianocore/IPv4Support"
+  }
+
+  NetworkPkg/Ip6Dxe/Ip6Dxe.inf {
+    <LibraryClasses>
+      NULL|OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
+    <PcdsFixedAtBuild>
+      gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName|"opt/org.tianocore/IPv6Support"
+  }
+
 !if $(NETWORK_TLS_ENABLE) == TRUE
   NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigDxe.inf {
     <LibraryClasses>
-- 
2.35.1

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

[Gerd Hoffmann]

On Mon, Aug 15, 2022 at 11:40:30AM +0200, Ard Biesheuvel wrote:
> Wire up the newly added DriverLoadInhibitorLib 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..0c0ded88f86e 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>
> +      NULL|OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
> +    <PcdsFixedAtBuild>
> +      gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName|"opt/org.tianocore/IPv4Support"
> +  }
> +
> +  NetworkPkg/Ip6Dxe/Ip6Dxe.inf {
> +    <LibraryClasses>
> +      NULL|OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
> +    <PcdsFixedAtBuild>
> +      gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName|"opt/org.tianocore/IPv6Support"
> +  }
> +

Hmm.  Not a comment to this specifically, but more to the OVMF *.dsc and
*.fdf files in general.  We have a lot of duplication here.  I've
already moved some bits to include files (OvmfPkg/OvmfTpm*.inc for
example) to reduce that.  Makes maintainance easier and builds more
consistent.

This looks like a prime candidate for a new OvmfNetwork.dsc.inc file,
so we can easily get that for all build variants and not only X64.

And there is more which we can split out.  crypto (have an experimental
branch doing that as part of my CryptoPkg/Driver experiments).  drivers
(usb / virtio / ...).  Shell.efi

Maybe it makes sense to move those include snippets into a subdirectory
so they don't pile up in OvmfPkg/ when we move more stuff to includes?

take care,
  Gerd

Re: [PATCH 1/2] OvmfPkg: Introduce NULL class library to inhibit driver load

[Laszlo Ersek]

On 08/15/22 11:40, 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) typo? (should be "IPv4 and IPv6" I think)

> driver to be controlled from the QEMU command line.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c   | 30 ++++++++++++++++++++
>  OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf | 28 ++++++++++++++++++
>  OvmfPkg/OvmfPkg.dec                                               |  4 +++
>  3 files changed, 62 insertions(+)
>
> diff --git a/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c
> new file mode 100644
> index 000000000000..dc8544bc38be
> --- /dev/null
> +++ b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c
> @@ -0,0 +1,30 @@
> +// @file
> +// Copyright (c) 2022, Google LLC. All rights reserved.<BR>
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +
> +#include <PiDxe.h>
> +
> +#include <Library/QemuFwCfgSimpleParserLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +STATIC CHAR16 mExitData[] = L"Driver dispatch inhibited by QEMU fw_cfg variable.";
> +
> +EFI_STATUS
> +EFIAPI
> +DriverLoadInhibitorLibConstructor (
> +  IN  EFI_HANDLE        Handle,
> +  IN  EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  RETURN_STATUS     Status;
> +  BOOLEAN           Enabled;
> +
> +  Status = QemuFwCfgParseBool (FixedPcdGetPtr (PcdDriverInhibitorFwCfgVarName),
> +             &Enabled);
> +  if (!RETURN_ERROR (Status) && !Enabled) {
> +    return gBS->Exit (Handle, EFI_REQUEST_UNLOAD_IMAGE, sizeof mExitData,
> +                  mExitData);

(2) Per UEFI spec, ExitData should be allocated with
gBS->AllocatePool().

(3) EFI_REQUEST_UNLOAD_IMAGE is from the PI spec; while not wrong, I
think it's strange to use here. EFI_ABORTED or something similar from
the UEFI spec would be a better fit IMO.

(4) And then, the big problem:

I agree that returning an error from the constructor would not be
beneficial, as it would cause an assertion to fail in the
ProcessLibraryConstructorList() function, in the generated "AutoGen.c"
file.

However, calling gBS->Exit() from a constructor seems unsafe to me, with
regard to library destructors.

Now, in the current case (considering patch#2), this unsafety is not
visible. That's because:

(quoting ProcessLibraryConstructorList() and
ProcessLibraryDestructorList() from
"Build/OvmfX64/NOOPT_GCC5/X64/NetworkPkg/Ip4Dxe/Ip4Dxe/DEBUG/AutoGen.c",
from an earlier build on my machine anyway):

> VOID
> EFIAPI
> ProcessLibraryConstructorList (
>   IN EFI_HANDLE        ImageHandle,
>   IN EFI_SYSTEM_TABLE  *SystemTable
>   )
> {
>   EFI_STATUS  Status;
>
>   Status = PlatformDebugLibIoPortConstructor ();
>   ASSERT_RETURN_ERROR (Status);
>
>   Status = UefiBootServicesTableLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
>
>   Status = DevicePathLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
>
>   Status = UefiRuntimeServicesTableLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
>
>   Status = UefiLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
>
>   Status = UefiHiiServicesLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
>
>   Status = DpcLibConstructor (ImageHandle, SystemTable);
>   ASSERT_EFI_ERROR (Status);
>
> }
>
>
>
> VOID
> EFIAPI
> ProcessLibraryDestructorList (
>   IN EFI_HANDLE        ImageHandle,
>   IN EFI_SYSTEM_TABLE  *SystemTable
>   )
> {
>
> }

there is no library destruction to speak of -- none of the used library
instances have resources they need to release at destruction time.

However, the general case looks problematic. The new library constructor
call would be inserted *somewhere* in ProcessLibraryConstructorList() --
the insertion point is likely "mostly unspecified", as no library
instance depends on DriverLoadInhibitorLib, and DriverLoadInhibitorLib
seems to depend on relatively few lib classes too. Therefore, in theory
anyway, the new lib constructor could call gBS->Exit() somewhere in the
middle of ProcessLibraryConstructorList(), with only some of the library
constructors having been executed.

Then the questions are

- does gBS->Exit() call ProcessLibraryDestructorList() or not?

  - if it does not, that could lead to memory leaks.

  - If it does though, is ProcessLibraryDestructorList() smart enough to
    call only those destructors whose constructors have previously run?

    - If not, it could call destructors on never-constructed data.

Unfortunately, this looks really tough to figure out; testing it (with
some actual library destructors) could be easier.


FWIW, there are two call sites for ProcessLibraryDestructorList() (for
UEFI/DXE drivers anyway); both in
"MdePkg/Library/UefiDriverEntryPoint/DriverEntryPoint.c":

- One is inside the _ModuleEntryPoint() function.

  This call is reached only when the function designated as ENTRY_POINT
  in the driver's INF file returns (note, said function is not the
  actual entry point function of the driver -- the actual entry point is
  the _ModuleEntryPoint() function).

  When gBS->Exit() is called, the CoreExit() function
  [MdeModulePkg/Core/Dxe/Image/Image.c] long-jumps back to
  CoreStartImage(), and no part of the driver's _ModuleEntryPoint() is
  again used. So the first ProcessLibraryDestructorList() call site,
  namely the one in ModuleEntryPoint(), is not reached when gBS->Exit()
  is called.

- The other ProcessLibraryDestructorList() call site is in
  _DriverUnloadHandler()
  [MdePkg/Library/UefiDriverEntryPoint/DriverEntryPoint.c].

  Now it's not easy at all to say whether gBS->Exit() utilizes this
  function or not, when it unloads the image (because, per spec,
  gBS->Exit() *is* responsible for unloading the image).

  However, we need not track that down right now, to see that the
  proposed patch is unsafe in this aspect. That's because
  _ModuleEntryPoint() does the following:

>   //
>   // 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;
>   }

  In other words, even if CoreExit() might call Unload -->
  _DriverUnloadHandler() --> ProcessLibraryDestructorList() somewhere,
  _ModuleEntryPoint() sets "Unload" to "_DriverUnloadHandler" only
  *after* ProcessLibraryConstructorList() returns. And the proposed
  patch calls gBS->Exit() from *inside* ProcessLibraryConstructorList(),
  that is, when "Unload" is not set yet.

On physical machines, I've seen firmware options for disabling the IP
stack entirely; I wonder how those firmwares do it...

Laszlo

On 08/15/22 11:40, Ard Biesheuvel wrote:
> +  }
> +  return EFI_SUCCESS;
> +}
> diff --git a/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
> new file mode 100644
> index 000000000000..ed521d12d335
> --- /dev/null
> +++ b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
> @@ -0,0 +1,28 @@
> +## @file
> +#  Copyright (c) 2022, Google LLC. All rights reserved.<BR>
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 1.29
> +  BASE_NAME                      = DriverLoadInhibitorLib
> +  FILE_GUID                      = af4c2c0b-f7ed-4d61-ad97-5953982c3531
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = NULL
> +  CONSTRUCTOR                    = DriverLoadInhibitorLibConstructor
> +
> +[Sources]
> +  DriverLoadInhibitorLib.c
> +
> +[LibraryClasses]
> +  QemuFwCfgSimpleParserLib
> +  UefiBootServicesTableLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[FixedPcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 5af76a540529..e9a22cab088c 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 DriverLoadInhibitorLib will check to
> +  #  decide whether to abort dispatch of the driver it is linked into.
> +  gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName|""|VOID*|0x68
> +
>  [PcdsDynamic, PcdsDynamicEx]
>    gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>

Re: [edk2-devel] [PATCH 1/2] OvmfPkg: Introduce NULL class library to inhibit driver load

[Brian J. Johnson]

On 8/16/22 07:30, Laszlo Ersek wrote:
> On 08/15/22 11:40, 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) typo? (should be "IPv4 and IPv6" I think)
> 
>> driver to be controlled from the QEMU command line.
>>
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>>   OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c   | 30 ++++++++++++++++++++
>>   OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf | 28 ++++++++++++++++++
>>   OvmfPkg/OvmfPkg.dec                                               |  4 +++
>>   3 files changed, 62 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c
>> new file mode 100644
>> index 000000000000..dc8544bc38be
>> --- /dev/null
>> +++ b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c
>> @@ -0,0 +1,30 @@
>> +// @file
>> +// Copyright (c) 2022, Google LLC. All rights reserved.<BR>
>> +// SPDX-License-Identifier: BSD-2-Clause-Patent
>> +//
>> +
>> +#include <PiDxe.h>
>> +
>> +#include <Library/QemuFwCfgSimpleParserLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +
>> +STATIC CHAR16 mExitData[] = L"Driver dispatch inhibited by QEMU fw_cfg variable.";
>> +
>> +EFI_STATUS
>> +EFIAPI
>> +DriverLoadInhibitorLibConstructor (
>> +  IN  EFI_HANDLE        Handle,
>> +  IN  EFI_SYSTEM_TABLE  *SystemTable
>> +  )
>> +{
>> +  RETURN_STATUS     Status;
>> +  BOOLEAN           Enabled;
>> +
>> +  Status = QemuFwCfgParseBool (FixedPcdGetPtr (PcdDriverInhibitorFwCfgVarName),
>> +             &Enabled);
>> +  if (!RETURN_ERROR (Status) && !Enabled) {
>> +    return gBS->Exit (Handle, EFI_REQUEST_UNLOAD_IMAGE, sizeof mExitData,
>> +                  mExitData);
> 
> (2) Per UEFI spec, ExitData should be allocated with
> gBS->AllocatePool().
> 
> (3) EFI_REQUEST_UNLOAD_IMAGE is from the PI spec; while not wrong, I
> think it's strange to use here. EFI_ABORTED or something similar from
> the UEFI spec would be a better fit IMO.
> 
> (4) And then, the big problem:
> 
> I agree that returning an error from the constructor would not be
> beneficial, as it would cause an assertion to fail in the
> ProcessLibraryConstructorList() function, in the generated "AutoGen.c"
> file.
> 
> However, calling gBS->Exit() from a constructor seems unsafe to me, with
> regard to library destructors.
> 
> Now, in the current case (considering patch#2), this unsafety is not
> visible. That's because:
> 
> (quoting ProcessLibraryConstructorList() and
> ProcessLibraryDestructorList() from
> "Build/OvmfX64/NOOPT_GCC5/X64/NetworkPkg/Ip4Dxe/Ip4Dxe/DEBUG/AutoGen.c",
> from an earlier build on my machine anyway):
> 
>> VOID
>> EFIAPI
>> ProcessLibraryConstructorList (
>>    IN EFI_HANDLE        ImageHandle,
>>    IN EFI_SYSTEM_TABLE  *SystemTable
>>    )
>> {
>>    EFI_STATUS  Status;
>>
>>    Status = PlatformDebugLibIoPortConstructor ();
>>    ASSERT_RETURN_ERROR (Status);
>>
>>    Status = UefiBootServicesTableLibConstructor (ImageHandle, SystemTable);
>>    ASSERT_EFI_ERROR (Status);
>>
>>    Status = DevicePathLibConstructor (ImageHandle, SystemTable);
>>    ASSERT_EFI_ERROR (Status);
>>
>>    Status = UefiRuntimeServicesTableLibConstructor (ImageHandle, SystemTable);
>>    ASSERT_EFI_ERROR (Status);
>>
>>    Status = UefiLibConstructor (ImageHandle, SystemTable);
>>    ASSERT_EFI_ERROR (Status);
>>
>>    Status = UefiHiiServicesLibConstructor (ImageHandle, SystemTable);
>>    ASSERT_EFI_ERROR (Status);
>>
>>    Status = DpcLibConstructor (ImageHandle, SystemTable);
>>    ASSERT_EFI_ERROR (Status);
>>
>> }
>>
>>
>>
>> VOID
>> EFIAPI
>> ProcessLibraryDestructorList (
>>    IN EFI_HANDLE        ImageHandle,
>>    IN EFI_SYSTEM_TABLE  *SystemTable
>>    )
>> {
>>
>> }
> 
> there is no library destruction to speak of -- none of the used library
> instances have resources they need to release at destruction time.
> 
> However, the general case looks problematic. The new library constructor
> call would be inserted *somewhere* in ProcessLibraryConstructorList() --
> the insertion point is likely "mostly unspecified", as no library
> instance depends on DriverLoadInhibitorLib, and DriverLoadInhibitorLib
> seems to depend on relatively few lib classes too. Therefore, in theory
> anyway, the new lib constructor could call gBS->Exit() somewhere in the
> middle of ProcessLibraryConstructorList(), with only some of the library
> constructors having been executed.
> 
> Then the questions are
> 
> - does gBS->Exit() call ProcessLibraryDestructorList() or not?
> 
>    - if it does not, that could lead to memory leaks.
> 
>    - If it does though, is ProcessLibraryDestructorList() smart enough to
>      call only those destructors whose constructors have previously run?
> 
>      - If not, it could call destructors on never-constructed data.
> 
> Unfortunately, this looks really tough to figure out; testing it (with
> some actual library destructors) could be easier.
> 
> 
> FWIW, there are two call sites for ProcessLibraryDestructorList() (for
> UEFI/DXE drivers anyway); both in
> "MdePkg/Library/UefiDriverEntryPoint/DriverEntryPoint.c":
> 
> - One is inside the _ModuleEntryPoint() function.
> 
>    This call is reached only when the function designated as ENTRY_POINT
>    in the driver's INF file returns (note, said function is not the
>    actual entry point function of the driver -- the actual entry point is
>    the _ModuleEntryPoint() function).
> 
>    When gBS->Exit() is called, the CoreExit() function
>    [MdeModulePkg/Core/Dxe/Image/Image.c] long-jumps back to
>    CoreStartImage(), and no part of the driver's _ModuleEntryPoint() is
>    again used. So the first ProcessLibraryDestructorList() call site,
>    namely the one in ModuleEntryPoint(), is not reached when gBS->Exit()
>    is called.
> 
> - The other ProcessLibraryDestructorList() call site is in
>    _DriverUnloadHandler()
>    [MdePkg/Library/UefiDriverEntryPoint/DriverEntryPoint.c].
> 
>    Now it's not easy at all to say whether gBS->Exit() utilizes this
>    function or not, when it unloads the image (because, per spec,
>    gBS->Exit() *is* responsible for unloading the image).
> 
>    However, we need not track that down right now, to see that the
>    proposed patch is unsafe in this aspect. That's because
>    _ModuleEntryPoint() does the following:
> 
>>    //
>>    // 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;
>>    }
> 
>    In other words, even if CoreExit() might call Unload -->
>    _DriverUnloadHandler() --> ProcessLibraryDestructorList() somewhere,
>    _ModuleEntryPoint() sets "Unload" to "_DriverUnloadHandler" only
>    *after* ProcessLibraryConstructorList() returns. And the proposed
>    patch calls gBS->Exit() from *inside* ProcessLibraryConstructorList(),
>    that is, when "Unload" is not set yet.
> 
> On physical machines, I've seen firmware options for disabling the IP
> stack entirely; I wonder how those firmwares do it...

I don't know how any physical machine handles that particular option.
But one approach would be to add a GUID to the depex of the module you 
want to control, and install it only when you want the module to be 
dispatched.  That's pretty straightforward, although it does result in 
"Driver %g was discovered but not loaded!!" messages from 
CoreDisplayDiscoveredNotDispatched() if sufficient debugging is enabled.

Brian J. Johnson

> Laszlo
> 
> On 08/15/22 11:40, Ard Biesheuvel wrote:
>> +  }
>> +  return EFI_SUCCESS;
>> +}
>> diff --git a/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
>> new file mode 100644
>> index 000000000000..ed521d12d335
>> --- /dev/null
>> +++ b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf
>> @@ -0,0 +1,28 @@
>> +## @file
>> +#  Copyright (c) 2022, Google LLC. All rights reserved.<BR>
>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 1.29
>> +  BASE_NAME                      = DriverLoadInhibitorLib
>> +  FILE_GUID                      = af4c2c0b-f7ed-4d61-ad97-5953982c3531
>> +  MODULE_TYPE                    = DXE_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = NULL
>> +  CONSTRUCTOR                    = DriverLoadInhibitorLibConstructor
>> +
>> +[Sources]
>> +  DriverLoadInhibitorLib.c
>> +
>> +[LibraryClasses]
>> +  QemuFwCfgSimpleParserLib
>> +  UefiBootServicesTableLib
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +  OvmfPkg/OvmfPkg.dec
>> +
>> +[FixedPcd]
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName
>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>> index 5af76a540529..e9a22cab088c 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 DriverLoadInhibitorLib will check to
>> +  #  decide whether to abort dispatch of the driver it is linked into.
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdDriverInhibitorFwCfgVarName|""|VOID*|0x68
>> +
>>   [PcdsDynamic, PcdsDynamicEx]
>>     gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>>     gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>>
> 
> 
> 
> 
> 
> 

-- 

                                                 Brian

--------------------------------------------------------------------

    "You're the flavor packet in the Ramen noodle brick of life, Roy."
                                            -- Three Fellows

Re: [edk2-devel] [PATCH 1/2] OvmfPkg: Introduce NULL class library to inhibit driver load

[Ard Biesheuvel]

On Tue, 16 Aug 2022 at 23:10, Brian J. Johnson <brian.johnson@hpe.com> wrote:
>
> On 8/16/22 07:30, Laszlo Ersek wrote:
> > On 08/15/22 11:40, 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) typo? (should be "IPv4 and IPv6" I think)
> >
> >> driver to be controlled from the QEMU command line.
> >>
> >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >> ---
> >>   OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c   | 30 ++++++++++++++++++++
> >>   OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.inf | 28 ++++++++++++++++++
> >>   OvmfPkg/OvmfPkg.dec                                               |  4 +++
> >>   3 files changed, 62 insertions(+)
> >>
> >> diff --git a/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c
> >> new file mode 100644
> >> index 000000000000..dc8544bc38be
> >> --- /dev/null
> >> +++ b/OvmfPkg/Library/DriverLoadInhibitorLib/DriverLoadInhibitorLib.c
> >> @@ -0,0 +1,30 @@
> >> +// @file
> >> +// Copyright (c) 2022, Google LLC. All rights reserved.<BR>
> >> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> >> +//
> >> +
> >> +#include <PiDxe.h>
> >> +
> >> +#include <Library/QemuFwCfgSimpleParserLib.h>
> >> +#include <Library/UefiBootServicesTableLib.h>
> >> +
> >> +STATIC CHAR16 mExitData[] = L"Driver dispatch inhibited by QEMU fw_cfg variable.";
> >> +
> >> +EFI_STATUS
> >> +EFIAPI
> >> +DriverLoadInhibitorLibConstructor (
> >> +  IN  EFI_HANDLE        Handle,
> >> +  IN  EFI_SYSTEM_TABLE  *SystemTable
> >> +  )
> >> +{
> >> +  RETURN_STATUS     Status;
> >> +  BOOLEAN           Enabled;
> >> +
> >> +  Status = QemuFwCfgParseBool (FixedPcdGetPtr (PcdDriverInhibitorFwCfgVarName),
> >> +             &Enabled);
> >> +  if (!RETURN_ERROR (Status) && !Enabled) {
> >> +    return gBS->Exit (Handle, EFI_REQUEST_UNLOAD_IMAGE, sizeof mExitData,
> >> +                  mExitData);
> >
> > (2) Per UEFI spec, ExitData should be allocated with
> > gBS->AllocatePool().
> >
> > (3) EFI_REQUEST_UNLOAD_IMAGE is from the PI spec; while not wrong, I
> > think it's strange to use here. EFI_ABORTED or something similar from
> > the UEFI spec would be a better fit IMO.
> >
> > (4) And then, the big problem:
> >
> > I agree that returning an error from the constructor would not be
> > beneficial, as it would cause an assertion to fail in the
> > ProcessLibraryConstructorList() function, in the generated "AutoGen.c"
> > file.
> >
> > However, calling gBS->Exit() from a constructor seems unsafe to me, with
> > regard to library destructors.
> >
> > Now, in the current case (considering patch#2), this unsafety is not
> > visible. That's because:
> >
> > (quoting ProcessLibraryConstructorList() and
> > ProcessLibraryDestructorList() from
> > "Build/OvmfX64/NOOPT_GCC5/X64/NetworkPkg/Ip4Dxe/Ip4Dxe/DEBUG/AutoGen.c",
> > from an earlier build on my machine anyway):
> >
> >> VOID
> >> EFIAPI
> >> ProcessLibraryConstructorList (
> >>    IN EFI_HANDLE        ImageHandle,
> >>    IN EFI_SYSTEM_TABLE  *SystemTable
> >>    )
> >> {
> >>    EFI_STATUS  Status;
> >>
> >>    Status = PlatformDebugLibIoPortConstructor ();
> >>    ASSERT_RETURN_ERROR (Status);
> >>
> >>    Status = UefiBootServicesTableLibConstructor (ImageHandle, SystemTable);
> >>    ASSERT_EFI_ERROR (Status);
> >>
> >>    Status = DevicePathLibConstructor (ImageHandle, SystemTable);
> >>    ASSERT_EFI_ERROR (Status);
> >>
> >>    Status = UefiRuntimeServicesTableLibConstructor (ImageHandle, SystemTable);
> >>    ASSERT_EFI_ERROR (Status);
> >>
> >>    Status = UefiLibConstructor (ImageHandle, SystemTable);
> >>    ASSERT_EFI_ERROR (Status);
> >>
> >>    Status = UefiHiiServicesLibConstructor (ImageHandle, SystemTable);
> >>    ASSERT_EFI_ERROR (Status);
> >>
> >>    Status = DpcLibConstructor (ImageHandle, SystemTable);
> >>    ASSERT_EFI_ERROR (Status);
> >>
> >> }
> >>
> >>
> >>
> >> VOID
> >> EFIAPI
> >> ProcessLibraryDestructorList (
> >>    IN EFI_HANDLE        ImageHandle,
> >>    IN EFI_SYSTEM_TABLE  *SystemTable
> >>    )
> >> {
> >>
> >> }
> >
> > there is no library destruction to speak of -- none of the used library
> > instances have resources they need to release at destruction time.
> >
> > However, the general case looks problematic. The new library constructor
> > call would be inserted *somewhere* in ProcessLibraryConstructorList() --
> > the insertion point is likely "mostly unspecified", as no library
> > instance depends on DriverLoadInhibitorLib, and DriverLoadInhibitorLib
> > seems to depend on relatively few lib classes too. Therefore, in theory
> > anyway, the new lib constructor could call gBS->Exit() somewhere in the
> > middle of ProcessLibraryConstructorList(), with only some of the library
> > constructors having been executed.
> >
> > Then the questions are
> >
> > - does gBS->Exit() call ProcessLibraryDestructorList() or not?
> >
> >    - if it does not, that could lead to memory leaks.
> >
> >    - If it does though, is ProcessLibraryDestructorList() smart enough to
> >      call only those destructors whose constructors have previously run?
> >
> >      - If not, it could call destructors on never-constructed data.
> >
> > Unfortunately, this looks really tough to figure out; testing it (with
> > some actual library destructors) could be easier.
> >
> >
> > FWIW, there are two call sites for ProcessLibraryDestructorList() (for
> > UEFI/DXE drivers anyway); both in
> > "MdePkg/Library/UefiDriverEntryPoint/DriverEntryPoint.c":
> >
> > - One is inside the _ModuleEntryPoint() function.
> >
> >    This call is reached only when the function designated as ENTRY_POINT
> >    in the driver's INF file returns (note, said function is not the
> >    actual entry point function of the driver -- the actual entry point is
> >    the _ModuleEntryPoint() function).
> >
> >    When gBS->Exit() is called, the CoreExit() function
> >    [MdeModulePkg/Core/Dxe/Image/Image.c] long-jumps back to
> >    CoreStartImage(), and no part of the driver's _ModuleEntryPoint() is
> >    again used. So the first ProcessLibraryDestructorList() call site,
> >    namely the one in ModuleEntryPoint(), is not reached when gBS->Exit()
> >    is called.
> >
> > - The other ProcessLibraryDestructorList() call site is in
> >    _DriverUnloadHandler()
> >    [MdePkg/Library/UefiDriverEntryPoint/DriverEntryPoint.c].
> >
> >    Now it's not easy at all to say whether gBS->Exit() utilizes this
> >    function or not, when it unloads the image (because, per spec,
> >    gBS->Exit() *is* responsible for unloading the image).
> >
> >    However, we need not track that down right now, to see that the
> >    proposed patch is unsafe in this aspect. That's because
> >    _ModuleEntryPoint() does the following:
> >
> >>    //
> >>    // 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;
> >>    }
> >
> >    In other words, even if CoreExit() might call Unload -->
> >    _DriverUnloadHandler() --> ProcessLibraryDestructorList() somewhere,
> >    _ModuleEntryPoint() sets "Unload" to "_DriverUnloadHandler" only
> >    *after* ProcessLibraryConstructorList() returns. And the proposed
> >    patch calls gBS->Exit() from *inside* ProcessLibraryConstructorList(),
> >    that is, when "Unload" is not set yet.
> >

Agree with all of the above. At this point, I think the only way to do
this properly is to create an alternative UefiDriverEntrypoint library
with the fw_cfg check folded into it. This is easy to do and addresses
all the concerns raised here (as it can force the driver entry point
function to return any value at any point) but the code duplication is
unfortunate.


> > On physical machines, I've seen firmware options for disabling the IP
> > stack entirely; I wonder how those firmwares do it...
>
> I don't know how any physical machine handles that particular option.
> But one approach would be to add a GUID to the depex of the module you
> want to control, and install it only when you want the module to be
> dispatched.  That's pretty straightforward, although it does result in
> "Driver %g was discovered but not loaded!!" messages from
> CoreDisplayDiscoveredNotDispatched() if sufficient debugging is enabled.
>

I think the diagnostic is fine. But I don't think adding DEPEXes to
UEFI drivers (as opposed to DXE drivers) is proper, or even supported.
It would also require the drivers in other packages to be updated.

What i i like about the current approach is that the library can tie
any driver or app dispatch to any fw_cfg variable in QEMU (provided
that its build is directed by the .dsc in question). Switching to an
alternate UefiDriverEntrypoint implementation would limit this to
drivers, but this is fine for the purpose at hand.

Re: [edk2-devel] [PATCH 1/2] OvmfPkg: Introduce NULL class library to inhibit driver load

[Laszlo Ersek]

On 08/16/22 23:08, Brian J. Johnson wrote:
> On 8/16/22 07:30, Laszlo Ersek wrote:

>> On physical machines, I've seen firmware options for disabling the IP
>> stack entirely; I wonder how those firmwares do it...
> 
> I don't know how any physical machine handles that particular option.
> But one approach would be to add a GUID to the depex of the module you
> want to control, and install it only when you want the module to be
> dispatched.  That's pretty straightforward, although it does result in
> "Driver %g was discovered but not loaded!!" messages from
> CoreDisplayDiscoveredNotDispatched() if sufficient debugging is enabled.

Indeed, thanks for the reminder! ArmVirtPkg and OvmfPkg uses this
pattern with several core drivers:

PlatformHasIoMmuLib:
- depex:       gEdkiiIoMmuProtocolGuid OR gIoMmuAbsentProtocolGuid
- hooked into: PciHostBridgeDxe

PlatformHasAcpiLib:
- depex:       gEdkiiPlatformHasAcpiGuid
- hooked into: AcpiTableDxe

NvVarStoreFormattedLib:
- depex:       gEdkiiNvVarStoreFormattedGuid
- hooked into: VariableRuntimeDxe

>From these, the first and third examples are not full matches, as the
depexes injected via PlatformHasIoMmuLib and NvVarStoreFormattedLib are
always supposed to be satisfied *eventually* -- the drivers that the
dependencies are injected into are always required; the dependencies
ensure some platform specific ordering requirements.

But PlatformHasAcpiLib seems like a 100% match; it *is* meant to block
AcpiTableDxe indefinitely, if the platform chooses so (dynamically).

Thanks!
Laszlo

Re: [edk2-devel] [PATCH 1/2] OvmfPkg: Introduce NULL class library to inhibit driver load

[Laszlo Ersek]

On 08/17/22 10:39, Ard Biesheuvel wrote:

> Agree with all of the above. At this point, I think the only way to do
> this properly is to create an alternative UefiDriverEntrypoint library
> with the fw_cfg check folded into it. This is easy to do and addresses
> all the concerns raised here (as it can force the driver entry point
> function to return any value at any point) but the code duplication is
> unfortunate.

Ah, that's very creative. I didn't think of duplicating and customizing
the entry point library!

> On Tue, 16 Aug 2022 at 23:10, Brian J. Johnson <brian.johnson@hpe.com>
> wrote:
>> I don't know how any physical machine handles that particular option.
>> But one approach would be to add a GUID to the depex of the module
>> you want to control, and install it only when you want the module to
>> be dispatched.  That's pretty straightforward, although it does
>> result in "Driver %g was discovered but not loaded!!" messages from
>> CoreDisplayDiscoveredNotDispatched() if sufficient debugging is
>> enabled.
>>
> 
> I think the diagnostic is fine. But I don't think adding DEPEXes to
> UEFI drivers (as opposed to DXE drivers) is proper, or even supported.
> It would also require the drivers in other packages to be updated.

Sigh, I'm totally getting rusty on all this (which is quite expected).
You are right about the difference between DXE and UEFI drivers wrt.
depexes. :/

> What i i like about the current approach is that the library can tie
> any driver or app dispatch to any fw_cfg variable in QEMU (provided
> that its build is directed by the .dsc in question). Switching to an
> alternate UefiDriverEntrypoint implementation would limit this to
> drivers, but this is fine for the purpose at hand.

Just don't forget that the number of fw_cfg slots in QEMU is limited. It
is exposed as an experimental property (x-file-slots) of the fw_cfg_io
and fw_cfg_mem on-board devices. It matters for migration, and so it is
versioned. It used to be 0x10,  for the 2.8 and earlier machine types,
and has been 0x20 for later machine types. See QEMU commit a5b3ebfd23bc.

IOW, while the mechanism in edk2 could scale "indefinitely", QEMU will
in effect limit the number of fw_cfg files that can be used for this
purpose too.

Re: [edk2-devel] [PATCH 1/2] OvmfPkg: Introduce NULL class library to inhibit driver load

[Ard Biesheuvel]

On Wed, 17 Aug 2022 at 11:22, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 08/17/22 10:39, Ard Biesheuvel wrote:
>
> > Agree with all of the above. At this point, I think the only way to do
> > this properly is to create an alternative UefiDriverEntrypoint library
> > with the fw_cfg check folded into it. This is easy to do and addresses
> > all the concerns raised here (as it can force the driver entry point
> > function to return any value at any point) but the code duplication is
> > unfortunate.
>
> Ah, that's very creative. I didn't think of duplicating and customizing
> the entry point library!
>

Yeah, I'll spin a v2 based on this idea.

> > On Tue, 16 Aug 2022 at 23:10, Brian J. Johnson <brian.johnson@hpe.com>
> > wrote:
> >> I don't know how any physical machine handles that particular option.
> >> But one approach would be to add a GUID to the depex of the module
> >> you want to control, and install it only when you want the module to
> >> be dispatched.  That's pretty straightforward, although it does
> >> result in "Driver %g was discovered but not loaded!!" messages from
> >> CoreDisplayDiscoveredNotDispatched() if sufficient debugging is
> >> enabled.
> >>
> >
> > I think the diagnostic is fine. But I don't think adding DEPEXes to
> > UEFI drivers (as opposed to DXE drivers) is proper, or even supported.
> > It would also require the drivers in other packages to be updated.
>
> Sigh, I'm totally getting rusty on all this (which is quite expected).
> You are right about the difference between DXE and UEFI drivers wrt.
> depexes. :/
>
> > What i i like about the current approach is that the library can tie
> > any driver or app dispatch to any fw_cfg variable in QEMU (provided
> > that its build is directed by the .dsc in question). Switching to an
> > alternate UefiDriverEntrypoint implementation would limit this to
> > drivers, but this is fine for the purpose at hand.
>
> Just don't forget that the number of fw_cfg slots in QEMU is limited. It
> is exposed as an experimental property (x-file-slots) of the fw_cfg_io
> and fw_cfg_mem on-board devices. It matters for migration, and so it is
> versioned. It used to be 0x10,  for the 2.8 and earlier machine types,
> and has been 0x20 for later machine types. See QEMU commit a5b3ebfd23bc.
>
> IOW, while the mechanism in edk2 could scale "indefinitely", QEMU will
> in effect limit the number of fw_cfg files that can be used for this
> purpose too.
>

Right, that is good to know. But I don't think the point here is to
make each individual driver controllable from the command line. The
goal here is really to make this work in a way that is generic (i.e.,
not specific to networking) that doesn't require changes to the other
packages.