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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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.



> 
> 
>> 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
>>