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

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

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