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.web12.410.1590082179347071158 for ; Thu, 21 May 2020 10:29:39 -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 0110330E; Thu, 21 May 2020 10:29:39 -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 87B973F68F; Thu, 21 May 2020 10:29:37 -0700 (PDT) Subject: Re: [PATCH] MdeModulePkg/NonDiscoverablePciDeviceDxe: connect handles on registration To: Leif Lindholm Cc: devel@edk2.groups.io, liming.gao@intel.com, lersek@redhat.com, Hao A Wu , Ray Ni References: <20200521111028.25864-1-ard.biesheuvel@arm.com> <20200521111651.GR1923@vanye> From: "Ard Biesheuvel" Message-ID: <87f6f6a9-7f99-d779-09e5-7f36fead3731@arm.com> Date: Thu, 21 May 2020 19:29:33 +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: <20200521111651.GR1923@vanye> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> Cc: Ray Ni >> Signed-off-by: Ard Biesheuvel > > Acked-by: Leif Lindholm > 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 >>