Re: [PATCH] MdeModulePkg/NonDiscoverablePciDeviceDxe: connect handles on registration

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

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