From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.16960.1590165380243783106 for ; Fri, 22 May 2020 09:36:21 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=XOse9bP2; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590165379; 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=ljOF9dySGxjySIFbIRwZhdvVvkqcGB0ZyxIYePyFO/w=; b=XOse9bP21WLVIWpVz8EuMjR104IlNY543e7mgspsMZkveN5rPKeM0MNWf7Ou421LytFx6h 5FtlU91D+XWDUMKRvTcNfchL2yixITY2IvdZdgMq+Yn72HM6KzPnXlNphEUMfmvOVvvyhc xuRd6jvB24xkHlAq7t7ymGGTK5LWld8= 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-235-LXfLm8h7PE2At-MKVhkZng-1; Fri, 22 May 2020 12:36:15 -0400 X-MC-Unique: LXfLm8h7PE2At-MKVhkZng-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 05D5818A072B; Fri, 22 May 2020 16:36:14 +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 4477D4EE3E; Fri, 22 May 2020 16:36:12 +0000 (UTC) Subject: Re: [edk2-devel] [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> <57a1a8c5-2f07-dced-f28c-b1892560881f@redhat.com> <280a3ab4-2dcc-9551-4fdf-4354ba6c52fd@arm.com> From: "Laszlo Ersek" Message-ID: <6bbf1245-94a4-6621-be0d-699c803f9d7d@redhat.com> Date: Fri, 22 May 2020 18:36:11 +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: <280a3ab4-2dcc-9551-4fdf-4354ba6c52fd@arm.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 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/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(). Thanks, Laszlo