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.16910.1590166020899067536 for ; Fri, 22 May 2020 09:47:01 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=M+fqvUlX; 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=1590166020; 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=4a4h4qLohsYcnIj2xfy2KHLViQlnh28G728jI9OtEQo=; b=M+fqvUlXdeUrkqULYVOmLpPTmx9vfYgASF2DFACFoHwVadGOFuIIzIhOmjVLQCI0OjKh94 5k4s34SalAKAKC7kYFT5kCJQ8TqWHUE4rrm2DhAEypEQWstamQzvsQg9f8jbNfKD70NoXs 3hOQs1Fn49CGAJGSakBM/g60UBzGNrU= 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-47-QmRFZXTAMLmP0WghF59GQA-1; Fri, 22 May 2020 12:46:57 -0400 X-MC-Unique: QmRFZXTAMLmP0WghF59GQA-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 29ABE1005510; Fri, 22 May 2020 16:46:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-40.ams2.redhat.com [10.36.112.40]) by smtp.corp.redhat.com (Postfix) with ESMTP id 82AF719167; Fri, 22 May 2020 16:46:54 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] MdeModulePkg/NonDiscoverablePciDeviceDxe: connect handles on registration From: "Laszlo Ersek" 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> <57a1a8c5-2f07-dced-f28c-b1892560881f@redhat.com> <280a3ab4-2dcc-9551-4fdf-4354ba6c52fd@arm.com> <6bbf1245-94a4-6621-be0d-699c803f9d7d@redhat.com> Message-ID: <2e364452-eb05-c724-096f-89ffc1421226@redhat.com> Date: Fri, 22 May 2020 18:46:53 +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: <6bbf1245-94a4-6621-be0d-699c803f9d7d@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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 On 05/22/20 18:36, Laszlo Ersek wrote: > On 05/21/20 23:58, Ard Biesheuvel wrote: > >> 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. > > Right, thanks for the reminder. "Recursive" controls recursion onto new > child handles produced by bus drivers, and not whether new protocols are > stacked (by further drivers) on existent protocols on the *same* handle. > >> 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. > > I can imagine two ways for that. > > (You may want to jump forward to the [short version] marker now. You've > been warned :) ) > > > (1) Turn the NonDiscoverablePciDeviceDxe driver into a bus driver. For > each input handle, produce just one child handle. In other words, don't > install PciIo on the same handle that carries > gEdkiiNonDiscoverableDeviceProtocolGuid, but on a new (child) handle. > This will make "Recursive" work as we need it. > > Now, the new child handle will also need a new device path protocol. > IIUC the input (parent) handle (with > gEdkiiNonDiscoverableDeviceProtocolGuid on it) already has a unique > device path protocol, so you could simply append a constant vendor > devpath node, to the parent's path. (I think VenHw() would be most > appropriate; VenMedia() or VenMsg() look less applicable.) > > > (2) This alternative means more work in Platform BDS, but (almost) no > changes to NonDiscoverablePciDeviceDxe. > > gBS->ConnectController() takes a DriverImageHandle parameter too, and > that one *does* restrict the "protocol stacking" on the same controller > handle. It points to a NULL-terminated (not a typo) array of > EFI_HANDLEs. We'd place just one non-NULL driver image handle in this > array, namely that of NonDiscoverablePciDeviceDxe. > > How do we find NonDiscoverablePciDeviceDxe in BDS? > > (2a) look up all handles carrying EFI_DRIVER_BINDING_PROTOCOL, with > gBS->LocateHandleBuffer(), > > (2b) on each DriverBindingHandle in that array, get > EFI_DRIVER_BINDING_PROTOCOL, and read the ImageHandle field, > > (2c) on said ImageHandle, open EFI_LOADED_IMAGE_PROTOCOL, and read the > FilePath field, > > (2d) check whether the first node in FilePath is an FvFile(GUID) node, > where the GUID equals the FILE_GUID of NonDiscoverablePciDeviceDxe > (71fd84cd-353b-464d-b7a4-6ea7b96995cb). > > (2e) If there is a match, then that's the "DriverBindingHandle" (from > step (2b)) that we need to pass to gBS->ConnectController(). > > > We expect NonDiscoverablePciDeviceDxe to come from a firmware volume, > hence expecting the FvFile(GUID) node in (2d). > > Furthermore, we don't care which firmware volume the driver comes from, > as the FILE_GUID of the driver is supposed to be unique anyway. That's > why we use the device-relative "EFI_LOADED_IMAGE_PROTOCOL.FilePath" > field in step (2c), and not the full device path that is installed > separately with gEfiLoadedImageDevicePathProtocolGuid on "ImageHandle". > > (The full device path would have an Fv(GUID) node prepended to the > FvFile node, and that GUID would come from the "FvNameGuid" directive in > the platform FDF file.) > > > Now, for cleanly referring to the FILE_GUID of > NonDiscoverablePciDeviceDxe in C code, we'd have to introduce the same > GUID in "MdeModulePkg.dec", in the [Guids] section. This was actually > attempted before (for SerialDxe), but it was rejected, for two reasons: > > - it's a mess to keep the INF file's FILE_GUID in sync with the [Guids] > section of the DEC file, > > - FILE_GUIDs of driver INF files can be overridden in DSC files, and > then the one from [Guids] wouldn't match. > > The solution chosen was commit cf78c9d18a81 ("MdeModulePkg: Introduce > EDKII_SERIAL_PORT_LIB_VENDOR_GUID", 2019-06-14) -- introduce a brand new > GUID, and use that, rather than FILE_GUID. > > > (3) And that's what we should ultimately do here as well: > > -- [short version] -- > > Introduce a brand new *protocol* GUID in MdeModulePkg.dec's [Protocols] > section, for example "gEdkiiNonDiscoverablePciDeviceDxeDriverGuid". > > In the entry point function of NonDiscoverablePciDeviceDxe, install a > NULL interface with that protocol GUID on the same handle that receives > EFI_DRIVER_BINDING_PROTOCOL. In practice, that handle is the driver's > image handle (available as the "ImageHandle" parameter, or the > "gImageHandle" global var). > > Then in Platform BDS, look up the handle with > "gEdkiiNonDiscoverablePciDeviceDxeDriverGuid", and use that handle as > the sole non-NULL element in the "DriverImageHandle" array that's passed > to gBS->ConnectController(). Meh, this is not good enough -- the spec led me to believe that ConnectController() would *stop* looking for drivers at the end of the "DriverImageHandle" array, if DriverImageHandle were not NULL. But looking at CoreConnectSingleController() in "MdeModulePkg/Core/Dxe/Hand/DriverSupport.c", this doesn't seem to be the case: // // Then add all the remaining Driver Binding Protocols // and // // Loop until no more drivers can be started on ControllerHandle // :( So I guess it's really only option (1) that lets us move the "shallow connect" to Platform BDS. Not sure if you want to pursue that... Thanks Laszlo