Re: [PATCH v2] ArmPkg/PlatformBootManagerLib: connect non-discoverable USB hosts

["Leif Lindholm" <leif@nuviainc.com>]

On Fri, May 22, 2020 at 14:06:16 +0200, Ard Biesheuvel wrote:
> On 5/22/20 2:03 PM, Leif Lindholm wrote:
> > On Fri, May 22, 2020 at 10:40:06 +0200, Ard Biesheuvel wrote:
> > > The way the BDS handles the short-form USB device path of the console
> > > keyboard relies on USB host controllers to be locatable via their PCI
> > > metadata, which implies that these controllers already have a PCI I/O
> > > protocol installed on their handle.
> > > 
> > > This is not the case for non-discoverable USB host controllers that are
> > > supported by the NonDiscoverable PCI device driver. These controllers
> > > must be connected first, or the BDS will never notice their existence,
> > > and will not enable any USB keyboards connected through them.
> > > 
> > > Let's work around this by connecting these handles explicitly. This is
> > > a bit of a stopgap, but it is the cleanest way of dealing with this
> > > without violating the UEFI driver model entirely. This ensures that
> > > platforms that do not rely on ConnectAll() will keep working as
> > > expected.
> > 
> > The downside to doing it properly is that this now becomes another
> > thing that needs to be copied around to every PlatformBootManagerLib
> > implementation (i.e.
> > Platform/RaspberryPi/Library/PlatformBootManagerLib/
> > Silicon/Hisilicon/Library/PlatformBootManagerLib/, but also
> > out-of-tree implementations).
> > 
> > Is there any way of avoiding that?
> 
> Not really. We will get rid of the RPi one as soon as we can, though. If the
> HiSilicon one keeps the ConnectAll() call [which is in the same file], they
> don't need this change to begin with.

OK, fair enough.

Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> 
> 
> 
> > 
> > 
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > ---
> > > 
> > > This is tagged as v2 since it is a followup to
> > > 
> > >    'MdeModulePkg/NonDiscoverablePciDeviceDxe: connect handles on registration'
> > > 
> > > that I sent out yesterday. Laslzo made the point that it would be
> > > unfortunate to add yet another hack that defeats the UEFI driver model
> > > entirely, so this time, the change is in the BDS where it belongs.
> > > 
> > > Note that only USB console devices are affected by this: short form
> > > block IO device paths used for booting are expanded and cached in a
> > > "HDDP" variable, and so they work as expected with fast boot without
> > > connect all.
> > > 
> > >   ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf |  4 ++
> > >   ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c               | 42 ++++++++++++++++++++
> > >   2 files changed, 46 insertions(+)
> > > 
> > > diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> > > index 87c9b1150c54..2f726d117d7d 100644
> > > --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> > > +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
> > > @@ -66,6 +66,9 @@ [Pcd]
> > >     gEfiMdePkgTokenSpaceGuid.PcdPlatformBootTimeOut
> > >   [Guids]
> > > +  gEdkiiNonDiscoverableEhciDeviceGuid
> > > +  gEdkiiNonDiscoverableUhciDeviceGuid
> > > +  gEdkiiNonDiscoverableXhciDeviceGuid
> > >     gEfiFileInfoGuid
> > >     gEfiFileSystemInfoGuid
> > >     gEfiFileSystemVolumeLabelInfoIdGuid
> > > @@ -74,6 +77,7 @@ [Guids]
> > >     gUefiShellFileGuid
> > >   [Protocols]
> > > +  gEdkiiNonDiscoverableDeviceProtocolGuid
> > >     gEfiDevicePathProtocolGuid
> > >     gEfiGraphicsOutputProtocolGuid
> > >     gEfiLoadedImageProtocolGuid
> > > diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > > index 3411219fbfdb..4aca1382b042 100644
> > > --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > > +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c
> > > @@ -23,10 +23,12 @@
> > >   #include <Protocol/EsrtManagement.h>
> > >   #include <Protocol/GraphicsOutput.h>
> > >   #include <Protocol/LoadedImage.h>
> > > +#include <Protocol/NonDiscoverableDevice.h>
> > >   #include <Protocol/PciIo.h>
> > >   #include <Protocol/PciRootBridgeIo.h>
> > >   #include <Protocol/PlatformBootManager.h>
> > >   #include <Guid/EventGroup.h>
> > > +#include <Guid/NonDiscoverableDevice.h>
> > >   #include <Guid/TtyTerm.h>
> > >   #include <Guid/SerialPortLibVendor.h>
> > > @@ -254,6 +256,37 @@ IsPciDisplay (
> > >   }
> > > +/**
> > > +  This FILTER_FUNCTION checks if a handle corresponds to a non-discoverable
> > > +  USB host controller.
> > > +**/
> > > +STATIC
> > > +BOOLEAN
> > > +EFIAPI
> > > +IsUsbHost (
> > > +  IN EFI_HANDLE   Handle,
> > > +  IN CONST CHAR16 *ReportText
> > > +  )
> > > +{
> > > +  NON_DISCOVERABLE_DEVICE   *Device;
> > > +  EFI_STATUS                Status;
> > > +
> > > +  Status = gBS->HandleProtocol (Handle,
> > > +                  &gEdkiiNonDiscoverableDeviceProtocolGuid,
> > > +                  (VOID **)&Device);
> > > +  if (EFI_ERROR (Status)) {
> > > +    return FALSE;
> > > +  }
> > > +
> > > +  if (CompareGuid (Device->Type, &gEdkiiNonDiscoverableUhciDeviceGuid) ||
> > > +      CompareGuid (Device->Type, &gEdkiiNonDiscoverableEhciDeviceGuid) ||
> > > +      CompareGuid (Device->Type, &gEdkiiNonDiscoverableXhciDeviceGuid)) {
> > > +    return TRUE;
> > > +  }
> > > +  return FALSE;
> > > +}
> > > +
> > > +
> > >   /**
> > >     This CALLBACK_FUNCTION attempts to connect a handle non-recursively, asking
> > >     the matching driver to produce all first-level child handles.
> > > @@ -574,6 +607,15 @@ PlatformBootManagerBeforeConsole (
> > >     //
> > >     FilterAndProcess (&gEfiGraphicsOutputProtocolGuid, NULL, AddOutput);
> > > +  //
> > > +  // The core BDS code connects short-form USB device paths by explicitly
> > > +  // looking for handles with PCI I/O installed, and checking the PCI class
> > > +  // code whether it matches the one for a USB host controller. This means
> > > +  // non-discoverable USB host controllers need to have the non-discoverable
> > > +  // PCI driver attached first.
> > > +  //
> > > +  FilterAndProcess (&gEdkiiNonDiscoverableDeviceProtocolGuid, IsUsbHost, Connect);
> > > +
> > >     //
> > >     // Add the hardcoded short-form USB keyboard device path to ConIn.
> > >     //
> > > -- 
> > > 2.17.1
> > > 
>