From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.19554.1590173860087388390 for ; Fri, 22 May 2020 11:57:41 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=MKW3QbMP; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590173859; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=A+Wq2bKE5OL5aTETpnM0kxkP+gzXoEb6jdDSbfEK/ds=; b=MKW3QbMPRKkEqjURd/av3b8yv7LDtvykEZhgaFHtNeL+h3uDaP3TdMaq8tIA44PO+tbunf PsMHlFADVntW8LkNi8KaMj0fIXLWSxgmVxwGiB2ItkpUUeC4uhu1o7i3/3wLQ1ZlkoTf5Q hNhRgSSlq2M6snw/6tDd4AFSdSHV4Xg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-499-3g9UBZUdN-yD7-4U7_vCUw-1; Fri, 22 May 2020 14:57:37 -0400 X-MC-Unique: 3g9UBZUdN-yD7-4U7_vCUw-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3DB4C107ACCA; Fri, 22 May 2020 18:57:36 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-40.ams2.redhat.com [10.36.112.40]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7C7765D9CC; Fri, 22 May 2020 18:57:34 +0000 (UTC) Subject: Re: [PATCH v2] ArmPkg/PlatformBootManagerLib: connect non-discoverable USB hosts To: Ard Biesheuvel , devel@edk2.groups.io Cc: liming.gao@intel.com, leif@nuviainc.com, philmd@redhat.com References: <20200522084006.32006-1-ard.biesheuvel@arm.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 22 May 2020 20:57:33 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200522084006.32006-1-ard.biesheuvel@arm.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 > --- > > 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. > // > 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 Thanks! Laszlo