Re: [edk2-devel] [PATCH] MdeModulePkg/NonDiscoverablePciDeviceDxe: connect handles on registration

["Laszlo Ersek" <lersek@redhat.com>]

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