From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.11520.1590149185129361905 for ; Fri, 22 May 2020 05:06:25 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id ADD7B55D; Fri, 22 May 2020 05:06:24 -0700 (PDT) Received: from [192.168.1.81] (unknown [10.37.8.250]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 828783F52E; Fri, 22 May 2020 05:06:23 -0700 (PDT) Subject: Re: [PATCH v2] ArmPkg/PlatformBootManagerLib: connect non-discoverable USB hosts To: Leif Lindholm Cc: devel@edk2.groups.io, liming.gao@intel.com, lersek@redhat.com, philmd@redhat.com References: <20200522084006.32006-1-ard.biesheuvel@arm.com> <20200522120348.GB1923@vanye> From: "Ard Biesheuvel" Message-ID: <94ddc6a5-3765-7891-5236-6210648295e8@arm.com> Date: Fri, 22 May 2020 14:06:16 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200522120348.GB1923@vanye> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >> --- >> >> 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 >> #include >> #include >> +#include >> #include >> #include >> #include >> #include >> +#include >> #include >> #include >> >> @@ -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 >>