[edk2-devel] [PATCH] MdeModulePkg/NonDiscoverablePciDeviceDxe: connect handles on registration
Ard Biesheuvel
ard.biesheuvel at arm.com
Thu May 21 21:58:57 UTC 2020
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 at intel.com>
>> Cc: Ray Ni <ray.ni at intel.com>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list