From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web10.2314.1590098347357496466 for ; Thu, 21 May 2020 14:59:07 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id E22A531B; Thu, 21 May 2020 14:59:05 -0700 (PDT) Received: from [192.168.1.81] (unknown [10.37.8.250]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7125B3F305; Thu, 21 May 2020 14:59:04 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/NonDiscoverablePciDeviceDxe: connect handles on registration To: devel@edk2.groups.io, lersek@redhat.com Cc: liming.gao@intel.com, leif@nuviainc.com, Hao A Wu , Ray Ni References: <20200521111028.25864-1-ard.biesheuvel@arm.com> <57a1a8c5-2f07-dced-f28c-b1892560881f@redhat.com> From: "Ard Biesheuvel" Message-ID: <280a3ab4-2dcc-9551-4fdf-4354ba6c52fd@arm.com> Date: Thu, 21 May 2020 23:58:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <57a1a8c5-2f07-dced-f28c-b1892560881f@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Cc: Ray Ni >> Signed-off-by: Ard Biesheuvel >> --- >> 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.