From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.1455.1590095553721472414 for ; Thu, 21 May 2020 14:12:33 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=eSRIOtrg; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590095552; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=s7SFviQrEo3hNMfOCqePwttgIGYruZUko6rl734IImo=; b=eSRIOtrgAFBEBq8kQmUpyOTbm1vyfhX/zAW1xLeDrCnK9WTtZhuTjF86z6haL7HUuxtJ1A CX7zr4sYGgwD/z0Gx/UJy9kBMgQjjkHRiFwiOaMHZN8mhUA6/PseceRZm9VmqRCLBh25Rh wC1sraOC/IRpLIyiksG+Ys8Z4XmV8zo= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-157-zuW85cWtO32MQz51_xs4dg-1; Thu, 21 May 2020 17:12:31 -0400 X-MC-Unique: zuW85cWtO32MQz51_xs4dg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 666081005510; Thu, 21 May 2020 21:12:29 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-225.ams2.redhat.com [10.36.113.225]) by smtp.corp.redhat.com (Postfix) with ESMTP id A738160F89; Thu, 21 May 2020 21:12:27 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg/NonDiscoverablePciDeviceDxe: connect handles on registration To: Ard Biesheuvel , devel@edk2.groups.io Cc: liming.gao@intel.com, leif@nuviainc.com, Hao A Wu , Ray Ni References: <20200521111028.25864-1-ard.biesheuvel@arm.com> From: "Laszlo Ersek" Message-ID: <57a1a8c5-2f07-dced-f28c-b1892560881f@redhat.com> Date: Thu, 21 May 2020 23:12:26 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200521111028.25864-1-ard.biesheuvel@arm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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, Laszlo