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'
argument.

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.wu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
---
 MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c | 43 ++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
index 5c93e2a7663c..a14c06e7f4e1 100644
--- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
+++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
@@ -15,6 +15,8 @@
 STATIC UINTN               mUniqueIdCounter = 0;
 EFI_CPU_ARCH_PROTOCOL      *mCpu;
 
+STATIC VOID                *mProtocolNotifyRegistration;
+
 //
 // We only support the following device types
 //
@@ -250,6 +252,43 @@ STATIC EFI_DRIVER_BINDING_PROTOCOL gDriverBinding = {
   NULL
 };
 
+STATIC
+VOID
+EFIAPI
+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, FALSE);
+    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 **)&mCpu);
   ASSERT_EFI_ERROR(Status);
 
+  EfiCreateProtocolNotifyEvent (&gEdkiiNonDiscoverableDeviceProtocolGuid,
+    TPL_CALLBACK, NonDiscoverablePciDeviceProtocolNotify, NULL,
+    &mProtocolNotifyRegistration);
+
   return EfiLibInstallDriverBindingComponentName2 (
            ImageHandle,
            SystemTable,
-- 
2.17.1


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

View/Reply Online (#60022): https://edk2.groups.io/g/devel/message/60022
Mute This Topic: https://groups.io/mt/74372159/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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'
> argument.
>
> 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.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c | 43 ++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
> index 5c93e2a7663c..a14c06e7f4e1 100644
> --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
> +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
> @@ -15,6 +15,8 @@
>  STATIC UINTN               mUniqueIdCounter = 0;
>  EFI_CPU_ARCH_PROTOCOL      *mCpu;
>
> +STATIC VOID                *mProtocolNotifyRegistration;
> +
>  //
>  // We only support the following device types
>  //
> @@ -250,6 +252,43 @@ STATIC EFI_DRIVER_BINDING_PROTOCOL gDriverBinding = {
>    NULL
>  };
>
> +STATIC
> +VOID
> +EFIAPI
> +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, FALSE);
> +    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 **)&mCpu);
>    ASSERT_EFI_ERROR(Status);
>
> +  EfiCreateProtocolNotifyEvent (&gEdkiiNonDiscoverableDeviceProtocolGuid,
> +    TPL_CALLBACK, NonDiscoverablePciDeviceProtocolNotify, NULL,
> +    &mProtocolNotifyRegistration);
> +
>    return EfiLibInstallDriverBindingComponentName2 (
>             ImageHandle,
>             SystemTable,
>

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 14.3.2.1
"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
NonDiscoverablePciDeviceDxe.


For example, consider
"ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c", function
PlatformBootManagerBeforeConsole():

>   //
>   // 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,
Laszlo


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

View/Reply Online (#60068): https://edk2.groups.io/g/devel/message/60068
Mute This Topic: https://groups.io/mt/74372159/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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'
>> argument.
>>
>> 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.wu@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>   MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c | 43 ++++++++++++++++++++
>>   1 file changed, 43 insertions(+)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
>> index 5c93e2a7663c..a14c06e7f4e1 100644
>> --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
>> +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
>> @@ -15,6 +15,8 @@
>>   STATIC UINTN               mUniqueIdCounter = 0;
>>   EFI_CPU_ARCH_PROTOCOL      *mCpu;
>>
>> +STATIC VOID                *mProtocolNotifyRegistration;
>> +
>>   //
>>   // We only support the following device types
>>   //
>> @@ -250,6 +252,43 @@ STATIC EFI_DRIVER_BINDING_PROTOCOL gDriverBinding = {
>>     NULL
>>   };
>>
>> +STATIC
>> +VOID
>> +EFIAPI
>> +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, FALSE);
>> +    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 **)&mCpu);
>>     ASSERT_EFI_ERROR(Status);
>>
>> +  EfiCreateProtocolNotifyEvent (&gEdkiiNonDiscoverableDeviceProtocolGuid,
>> +    TPL_CALLBACK, NonDiscoverablePciDeviceProtocolNotify, NULL,
>> +    &mProtocolNotifyRegistration);
>> +
>>     return EfiLibInstallDriverBindingComponentName2 (
>>              ImageHandle,
>>              SystemTable,
>>
> 
> 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 14.3.2.1
> "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
> NonDiscoverablePciDeviceDxe.
> 
> 
> For example, consider
> "ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c", function
> PlatformBootManagerBeforeConsole():
> 
>>    //
>>    // 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)) {

         continue;

       }

       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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 05/21/20 23:58, Ard Biesheuvel wrote:

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

Right, thanks for the reminder. "Recursive" controls recursion onto new
child handles produced by bus drivers, and not whether new protocols are
stacked (by further drivers) on existent protocols on the *same* handle.

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

I can imagine two ways for that.

(You may want to jump forward to the [short version] marker now. You've
been warned :) )


(1) Turn the NonDiscoverablePciDeviceDxe driver into a bus driver. For
each input handle, produce just one child handle. In other words, don't
install PciIo on the same handle that carries
gEdkiiNonDiscoverableDeviceProtocolGuid, but on a new (child) handle.
This will make "Recursive" work as we need it.

Now, the new child handle will also need a new device path protocol.
IIUC the input (parent) handle (with
gEdkiiNonDiscoverableDeviceProtocolGuid on it) already has a unique
device path protocol, so you could simply append a constant vendor
devpath node, to the parent's path. (I think VenHw() would be most
appropriate; VenMedia() or VenMsg() look less applicable.)


(2) This alternative means more work in Platform BDS, but (almost) no
changes to NonDiscoverablePciDeviceDxe.

gBS->ConnectController() takes a DriverImageHandle parameter too, and
that one *does* restrict the "protocol stacking" on the same controller
handle. It points to a NULL-terminated (not a typo) array of
EFI_HANDLEs. We'd place just one non-NULL driver image handle in this
array, namely that of NonDiscoverablePciDeviceDxe.

How do we find NonDiscoverablePciDeviceDxe in BDS?

(2a) look up all handles carrying EFI_DRIVER_BINDING_PROTOCOL, with
gBS->LocateHandleBuffer(),

(2b) on each DriverBindingHandle in that array, get
EFI_DRIVER_BINDING_PROTOCOL, and read the ImageHandle field,

(2c) on said ImageHandle, open EFI_LOADED_IMAGE_PROTOCOL, and read the
FilePath field,

(2d) check whether the first node in FilePath is an FvFile(GUID) node,
where the GUID equals the FILE_GUID of NonDiscoverablePciDeviceDxe
(71fd84cd-353b-464d-b7a4-6ea7b96995cb).

(2e) If there is a match, then that's the "DriverBindingHandle" (from
step (2b)) that we need to pass to gBS->ConnectController().


We expect NonDiscoverablePciDeviceDxe to come from a firmware volume,
hence expecting the FvFile(GUID) node in (2d).

Furthermore, we don't care which firmware volume the driver comes from,
as the FILE_GUID of the driver is supposed to be unique anyway. That's
why we use the device-relative "EFI_LOADED_IMAGE_PROTOCOL.FilePath"
field in step (2c), and not the full device path that is installed
separately with gEfiLoadedImageDevicePathProtocolGuid on "ImageHandle".

(The full device path would have an Fv(GUID) node prepended to the
FvFile node, and that GUID would come from the "FvNameGuid" directive in
the platform FDF file.)


Now, for cleanly referring to the FILE_GUID of
NonDiscoverablePciDeviceDxe in C code, we'd have to introduce the same
GUID in "MdeModulePkg.dec", in the [Guids]  section. This was actually
attempted before (for SerialDxe), but it was rejected, for two reasons:

- it's a mess to keep the INF file's FILE_GUID in sync with the [Guids]
section of the DEC file,

- FILE_GUIDs of driver INF files can be overridden in DSC files, and
then the one from [Guids] wouldn't match.

The solution chosen was commit cf78c9d18a81 ("MdeModulePkg: Introduce
EDKII_SERIAL_PORT_LIB_VENDOR_GUID", 2019-06-14) -- introduce a brand new
GUID, and use that, rather than FILE_GUID.


(3) And that's what we should ultimately do here as well:

-- [short version] --

Introduce a brand new *protocol* GUID in MdeModulePkg.dec's [Protocols]
section, for example "gEdkiiNonDiscoverablePciDeviceDxeDriverGuid".

In the entry point function of NonDiscoverablePciDeviceDxe, install a
NULL interface with that protocol GUID on the same handle that receives
EFI_DRIVER_BINDING_PROTOCOL. In practice, that handle is the driver's
image handle (available as the "ImageHandle" parameter, or the
"gImageHandle" global var).

Then in Platform BDS, look up the handle with
"gEdkiiNonDiscoverablePciDeviceDxeDriverGuid", and use that handle as
the sole non-NULL element in the "DriverImageHandle" array that's passed
to gBS->ConnectController().

Thanks,
Laszlo


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

View/Reply Online (#60157): https://edk2.groups.io/g/devel/message/60157
Mute This Topic: https://groups.io/mt/74372159/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 05/22/20 18:36, Laszlo Ersek wrote:
> On 05/21/20 23:58, Ard Biesheuvel wrote:
> 
>> 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.
> 
> Right, thanks for the reminder. "Recursive" controls recursion onto new
> child handles produced by bus drivers, and not whether new protocols are
> stacked (by further drivers) on existent protocols on the *same* handle.
> 
>> 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.
> 
> I can imagine two ways for that.
> 
> (You may want to jump forward to the [short version] marker now. You've
> been warned :) )
> 
> 
> (1) Turn the NonDiscoverablePciDeviceDxe driver into a bus driver. For
> each input handle, produce just one child handle. In other words, don't
> install PciIo on the same handle that carries
> gEdkiiNonDiscoverableDeviceProtocolGuid, but on a new (child) handle.
> This will make "Recursive" work as we need it.
> 
> Now, the new child handle will also need a new device path protocol.
> IIUC the input (parent) handle (with
> gEdkiiNonDiscoverableDeviceProtocolGuid on it) already has a unique
> device path protocol, so you could simply append a constant vendor
> devpath node, to the parent's path. (I think VenHw() would be most
> appropriate; VenMedia() or VenMsg() look less applicable.)
> 
> 
> (2) This alternative means more work in Platform BDS, but (almost) no
> changes to NonDiscoverablePciDeviceDxe.
> 
> gBS->ConnectController() takes a DriverImageHandle parameter too, and
> that one *does* restrict the "protocol stacking" on the same controller
> handle. It points to a NULL-terminated (not a typo) array of
> EFI_HANDLEs. We'd place just one non-NULL driver image handle in this
> array, namely that of NonDiscoverablePciDeviceDxe.
> 
> How do we find NonDiscoverablePciDeviceDxe in BDS?
> 
> (2a) look up all handles carrying EFI_DRIVER_BINDING_PROTOCOL, with
> gBS->LocateHandleBuffer(),
> 
> (2b) on each DriverBindingHandle in that array, get
> EFI_DRIVER_BINDING_PROTOCOL, and read the ImageHandle field,
> 
> (2c) on said ImageHandle, open EFI_LOADED_IMAGE_PROTOCOL, and read the
> FilePath field,
> 
> (2d) check whether the first node in FilePath is an FvFile(GUID) node,
> where the GUID equals the FILE_GUID of NonDiscoverablePciDeviceDxe
> (71fd84cd-353b-464d-b7a4-6ea7b96995cb).
> 
> (2e) If there is a match, then that's the "DriverBindingHandle" (from
> step (2b)) that we need to pass to gBS->ConnectController().
> 
> 
> We expect NonDiscoverablePciDeviceDxe to come from a firmware volume,
> hence expecting the FvFile(GUID) node in (2d).
> 
> Furthermore, we don't care which firmware volume the driver comes from,
> as the FILE_GUID of the driver is supposed to be unique anyway. That's
> why we use the device-relative "EFI_LOADED_IMAGE_PROTOCOL.FilePath"
> field in step (2c), and not the full device path that is installed
> separately with gEfiLoadedImageDevicePathProtocolGuid on "ImageHandle".
> 
> (The full device path would have an Fv(GUID) node prepended to the
> FvFile node, and that GUID would come from the "FvNameGuid" directive in
> the platform FDF file.)
> 
> 
> Now, for cleanly referring to the FILE_GUID of
> NonDiscoverablePciDeviceDxe in C code, we'd have to introduce the same
> GUID in "MdeModulePkg.dec", in the [Guids]  section. This was actually
> attempted before (for SerialDxe), but it was rejected, for two reasons:
> 
> - it's a mess to keep the INF file's FILE_GUID in sync with the [Guids]
> section of the DEC file,
> 
> - FILE_GUIDs of driver INF files can be overridden in DSC files, and
> then the one from [Guids] wouldn't match.
> 
> The solution chosen was commit cf78c9d18a81 ("MdeModulePkg: Introduce
> EDKII_SERIAL_PORT_LIB_VENDOR_GUID", 2019-06-14) -- introduce a brand new
> GUID, and use that, rather than FILE_GUID.
> 
> 
> (3) And that's what we should ultimately do here as well:
> 
> -- [short version] --
> 
> Introduce a brand new *protocol* GUID in MdeModulePkg.dec's [Protocols]
> section, for example "gEdkiiNonDiscoverablePciDeviceDxeDriverGuid".
> 
> In the entry point function of NonDiscoverablePciDeviceDxe, install a
> NULL interface with that protocol GUID on the same handle that receives
> EFI_DRIVER_BINDING_PROTOCOL. In practice, that handle is the driver's
> image handle (available as the "ImageHandle" parameter, or the
> "gImageHandle" global var).
> 
> Then in Platform BDS, look up the handle with
> "gEdkiiNonDiscoverablePciDeviceDxeDriverGuid", and use that handle as
> the sole non-NULL element in the "DriverImageHandle" array that's passed
> to gBS->ConnectController().

Meh, this is not good enough -- the spec led me to believe that
ConnectController() would *stop* looking for drivers at the end of the
"DriverImageHandle" array, if DriverImageHandle were not NULL.

But looking at CoreConnectSingleController() in
"MdeModulePkg/Core/Dxe/Hand/DriverSupport.c", this doesn't seem to be
the case:

  //
  // Then add all the remaining Driver Binding Protocols
  //

and

  //
  // Loop until no more drivers can be started on ControllerHandle
  //

:(

So I guess it's really only option (1) that lets us move the "shallow
connect" to Platform BDS. Not sure if you want to pursue that...

Thanks
Laszlo


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

View/Reply Online (#60158): https://edk2.groups.io/g/devel/message/60158
Mute This Topic: https://groups.io/mt/74372159/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 05/22/20 18:46, Laszlo Ersek wrote:

> the spec led me to believe

Well, if I had read a few more pages from the spec... It's totally my
fault! :) sorry, it's Friday! :)


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

View/Reply Online (#60159): https://edk2.groups.io/g/devel/message/60159
Mute This Topic: https://groups.io/mt/74372159/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 5/22/20 6:48 PM, Laszlo Ersek wrote:
> On 05/22/20 18:46, Laszlo Ersek wrote:
> 
>> the spec led me to believe
> 
> Well, if I had read a few more pages from the spec... It's totally my
> fault! :) sorry, it's Friday! :)
> 


No worries, thanks for taking the time to dig into this.

I had already noticed that the DriverImageHandle[] approach does not 
work, it indeed simply changes the order in which drivers are considered.

So I found a way to fix this in the BDS, which is not as clean as I 
like, but not that intrusive either. It turns out the the existing code 
plays nicely with the driver model in most cases, the only place where 
it cuts corners is when it connects the short-form USB device path for 
the console keyboard - this is the only place where it mucks around with 
PCI I/O handles explicitly, to connect USB host controllers.

So we can simply do the same for non-discoverable uhci/ehci/xhci 
devices, i.e., connect them non-recursively so that the PCI I/O protocol 
as well as the USB host controller protocol are installed (which is 
fine, as the latter was going to be installed by the BDS anyway)


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

View/Reply Online (#60160): https://edk2.groups.io/g/devel/message/60160
Mute This Topic: https://groups.io/mt/74372159/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, May 21, 2020 at 13:10:28 +0200, 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'
> argument.
> 
> 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.wu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>

Acked-by: Leif Lindholm <leif@nuviainc.com>

> ---
>  MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c | 43 ++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
> index 5c93e2a7663c..a14c06e7f4e1 100644
> --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
> +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
> @@ -15,6 +15,8 @@
>  STATIC UINTN               mUniqueIdCounter = 0;
>  EFI_CPU_ARCH_PROTOCOL      *mCpu;
>  
> +STATIC VOID                *mProtocolNotifyRegistration;
> +
>  //
>  // We only support the following device types
>  //
> @@ -250,6 +252,43 @@ STATIC EFI_DRIVER_BINDING_PROTOCOL gDriverBinding = {
>    NULL
>  };
>  
> +STATIC
> +VOID
> +EFIAPI
> +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, FALSE);
> +    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 **)&mCpu);
>    ASSERT_EFI_ERROR(Status);
>  
> +  EfiCreateProtocolNotifyEvent (&gEdkiiNonDiscoverableDeviceProtocolGuid,
> +    TPL_CALLBACK, NonDiscoverablePciDeviceProtocolNotify, NULL,
> +    &mProtocolNotifyRegistration);
> +
>    return EfiLibInstallDriverBindingComponentName2 (
>             ImageHandle,
>             SystemTable,
> -- 
> 2.17.1
> 

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

View/Reply Online (#60023): https://edk2.groups.io/g/devel/message/60023
Mute This Topic: https://groups.io/mt/74372159/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 5/21/20 1:16 PM, Leif Lindholm wrote:
> On Thu, May 21, 2020 at 13:10:28 +0200, 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'
>> argument.
>>
>> 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.wu@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> 
> Acked-by: Leif Lindholm <leif@nuviainc.com>
> 

Thanks,

I realized that this version is broken, though: passing gImageHandle as 
*DriverImageHandle is not correct, it should be an array { gImageHandle, 
NULL } instead.


>> ---
>>   MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c | 43 ++++++++++++++++++++
>>   1 file changed, 43 insertions(+)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
>> index 5c93e2a7663c..a14c06e7f4e1 100644
>> --- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
>> +++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
>> @@ -15,6 +15,8 @@
>>   STATIC UINTN               mUniqueIdCounter = 0;
>>   EFI_CPU_ARCH_PROTOCOL      *mCpu;
>>   
>> +STATIC VOID                *mProtocolNotifyRegistration;
>> +
>>   //
>>   // We only support the following device types
>>   //
>> @@ -250,6 +252,43 @@ STATIC EFI_DRIVER_BINDING_PROTOCOL gDriverBinding = {
>>     NULL
>>   };
>>   
>> +STATIC
>> +VOID
>> +EFIAPI
>> +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, FALSE);
>> +    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 **)&mCpu);
>>     ASSERT_EFI_ERROR(Status);
>>   
>> +  EfiCreateProtocolNotifyEvent (&gEdkiiNonDiscoverableDeviceProtocolGuid,
>> +    TPL_CALLBACK, NonDiscoverablePciDeviceProtocolNotify, NULL,
>> +    &mProtocolNotifyRegistration);
>> +
>>     return EfiLibInstallDriverBindingComponentName2 (
>>              ImageHandle,
>>              SystemTable,
>> -- 
>> 2.17.1
>>


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

View/Reply Online (#60048): https://edk2.groups.io/g/devel/message/60048
Mute This Topic: https://groups.io/mt/74372159/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-