On 5/21/20 11:12 PM, Laszlo Ersek via groups.io wrote:
Hi Ard,

On 05/21/20 13:10, Ard Biesheuvel wrote:
The way EDK2 invokes the UEFI driver model assumes that PCI I/O
protocol instances exist for all PCI I/O controllers in the system.

For instance, UefiBootManagerLib connects the short-form USB device
path of the console input by looking for PCI I/O controllers that
have the 'USB host controller' class code, and passing each one to
ConnectController(), using the device path as the 'RemainingDevicePath'

For true PCI I/O protocol instances produced by the PCI root bridge
driver, this works fine, since it always enumerates the PCIe hierarchy
exhaustively. However, for platform devices that are wired to PCI class
drivers using the non-discoverable PCIe driver, this breaks down, due
to the fact that the PCI I/O protocol instance does not exist unless the
non-discoverable device protocol handle is connected first.

So let's connect these handles non-recursively as soon as they appear.

Cc: Hao A Wu <hao.a...@intel.com>
Cc: Ray Ni <ray...@intel.com>
Signed-off-by: Ard Biesheuvel <ard.biesheu...@arm.com>
| 43 ++++++++++++++++++++
  1 file changed, 43 insertions(+)

diff --git 
index 5c93e2a7663c..a14c06e7f4e1 100644
@@ -15,6 +15,8 @@
  STATIC UINTN               mUniqueIdCounter = 0;

+STATIC VOID                *mProtocolNotifyRegistration;
  // We only support the following device types
@@ -250,6 +252,43 @@ STATIC EFI_DRIVER_BINDING_PROTOCOL gDriverBinding = {

+NonDiscoverablePciDeviceProtocolNotify (
+  IN EFI_EVENT            Event,
+  IN VOID                 *Context
+  )
+  EFI_STATUS        Status;
+  EFI_HANDLE        *Handles;
+  UINTN             HandleCount;
+  UINTN             Index;
+  Status = gBS->LocateHandleBuffer (ByRegisterNotify, NULL,
+                  mProtocolNotifyRegistration, &HandleCount, &Handles);
+  if (EFI_ERROR (Status)) {
+    if (Status != EFI_NOT_FOUND) {
+      DEBUG ((DEBUG_WARN, "%a: LocateHandleBuffer() failed - %r\n",
+        __FUNCTION__, Status));
+      }
+    return;
+  }
+  for (Index = 0; Index < HandleCount; Index++) {
+    //
+    // Connect each newly registered gEdkiiNonDiscoverableDeviceProtocolGuid
+    // instance non-recursively to this driver specifically. This ensures that
+    // PCI I/O instances exist for each, regardless of whether ConnectAll() is
+    // used at any point.
+    //
+    Status = gBS->ConnectController (Handles[Index], gImageHandle, NULL, 
+    DEBUG ((DEBUG_VERBOSE, "%a: ConnectController () returned %r\n",
+      __FUNCTION__, Status));
+  }
+  gBS->FreePool (Handles);
    Entry point of this driver.

@@ -272,6 +311,10 @@ NonDiscoverablePciDeviceDxeEntryPoint (
    Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID 

+  EfiCreateProtocolNotifyEvent (&gEdkiiNonDiscoverableDeviceProtocolGuid,
+    TPL_CALLBACK, NonDiscoverablePciDeviceProtocolNotify, NULL,
+    &mProtocolNotifyRegistration);
    return EfiLibInstallDriverBindingComponentName2 (

this problem is tricky. :)

First, I'm just generally unhappy that it turns a perfectly nice driver
that follows the UEFI driver model into what is basically a DXE driver
(= jump at new protocol instances as soon as they appear).

Second, the "true" PciIo instances are not produced by the root bridge
driver; they are produced by the PCI Bus Driver; the root bridge
driver's services are "only" consumed to that end.

Third, I think the fact that "true" PciIo instances are always produced
"exhaustively" (in a full go over the PCI hierarchy) is actually
happenstance in edk2. The UEFI v2.8 spec writes, in section
"Driver Binding Protocol for PCI Bus Drivers":

     The PCI Bus Driver has the option of creating all of its children
     in one call to Start(), or spreading it across several calls to
     Start(). In general, if it is possible to design a bus driver to
     create one child at a time, it should do so to support the rapid
     boot capability in the UEFI Driver Model. [...]

     A PCI Bus Driver must perform several steps to manage a PCI Host
     Bus Controller, as follows:


     * Discover all the PCI Controllers on all the PCI Root Bridges.

     * Create a device handle for each PCI Controller found. If a
       request is being made to start only one PCI Controller, then
       only create one device handle.


Fourth, while I agree that generic BDS code in edk2 may expect "all"
PciIo instances to exist in practice, I feel that assumption doesn't put
a requirement on PciBusDxe. Instead, this silent requirement is
presented for platform BDS. It is platform BDS that connects the root
bridge(s), thereby telling PciBusDxe to call into the root bridge driver
and to produce "true" PciIo instances.

What I'm saying is, if a platform includes NonDiscoverablePciDeviceDxe,
then the PlatformBootManagerLib instance used by that particular
platform should also invoke the following logic, right before, or right
after, connecting the root bridges:

(1) Enumerate all handles with gEdkiiNonDiscoverableDeviceProtocolGuid
instances on them, in one go -- these are produced by DXE drivers, and
so they exist by the time we get into BDS.

(2) Connect all the controller handles found in the exact same way as
the PCI root bridge handles are connected. The only difference is that
it won't be PciBusDxe that ends up binding the controller handles, but

For example, consider
"ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c", function

   // Locate the PCI root bridges and make the PCI bus driver connect each,
   // non-recursively. This will produce a number of child handles with PciIo on
   // them.
   FilterAndProcess (&gEfiPciRootBridgeIoProtocolGuid, NULL, Connect);

If we had any use for NonDiscoverablePciDeviceDxe in the ArmVirtQemu and
ArmVirtQemuKernel platforms -- which are the platforms using this
PlatformBootManagerLib instance --, then the above location would be
exactly where we should append the following call:

   FilterAndProcess (&gEdkiiNonDiscoverableDeviceProtocolGuid, NULL, Connect);

After this second call, we would have ensured the invariant "all PciIo
instances that *can* exist, *do* exist".

Then we'd advance to connecting consoles and such, just like the
PlatformBootManagerBeforeConsole() function does indeed.

Sorry about the hugely verbose email, I had to gather my thoughts.

Thanks for the comments. I made a bit of progress in the mean time: ConnectController() does not work for these handles - that will result in the created PciIo protocol to be connected immediately as well (the non-recursive bit about ConnectController() appears to refer to connecting newly created handles by bus drivers). I don't want those PCI I/O handles to be connected to anything, I just want them to exist.

So what I ended up doing in my [preliminary, unposted] v2 is

      Status = NonDiscoverablePciDeviceSupported (&gDriverBinding,

                 Handles[Index], NULL);

      if (EFI_ERROR (Status)) {



      Status = NonDiscoverablePciDeviceStart (&gDriverBinding,
                 Handles[Index], NULL);

in the protocol notify callback, for all handles that have the non-discoverable protocol installed, so that only the PCI I/O protocol is added to them.

I agree that going around the driver model's back is a bit nasty, and I would welcome any improvements over this. But I think the above can only be done from inside the driver - I don't see any way to do this from the BDS.

Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#60069): https://edk2.groups.io/g/devel/message/60069
Mute This Topic: https://groups.io/mt/74372159/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]

Reply via email to