ArmPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf | 4 ++ ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 42 ++++++++++++++++++++ 2 files changed, 46 insertions(+)
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.
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60092): https://edk2.groups.io/g/devel/message/60092
Mute This Topic: https://groups.io/mt/74395026/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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?
/
Leif
> 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60133): https://edk2.groups.io/g/devel/message/60133
Mute This Topic: https://groups.io/mt/74395026/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60135): https://edk2.groups.io/g/devel/message/60135
Mute This Topic: https://groups.io/mt/74395026/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
> > >
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60137): https://edk2.groups.io/g/devel/message/60137
Mute This Topic: https://groups.io/mt/74395026/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/22/20 10:40, 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.
>
> 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.
> //
>
Awesome! So we don't mind the full proto stack on these handles, as long
as "these handles" means exactly the small set we want.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60162): https://edk2.groups.io/g/devel/message/60162
Mute This Topic: https://groups.io/mt/74395026/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 5/22/20 8:57 PM, Laszlo Ersek via groups.io wrote:
> On 05/22/20 10:40, 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.
>>
>> 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.
>> //
>>
>
> Awesome! So we don't mind the full proto stack on these handles, as long
> as "these handles" means exactly the small set we want.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
Indeed.
One thing I could not figure out is whether we should pass the 'end
device path' as remaining device path to ConnectController, to prevent
the USB bus driver from instantiating any new handles, and only attach
to the one that has the PCI I/O and USB host controller protocols
installed. However, in order for the BDS to be able to connect any USB
keyboards, it will have to enumerate the entire subordinate USB
hierarchy anyway, so that does not really matter in practice.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60164): https://edk2.groups.io/g/devel/message/60164
Mute This Topic: https://groups.io/mt/74395026/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.