From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) by mx.groups.io with SMTP id smtpd.web11.11479.1590149032402327155 for ; Fri, 22 May 2020 05:03:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=L7xHQs4P; spf=pass (domain: nuviainc.com, ip: 209.85.221.67, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f67.google.com with SMTP id c3so5678693wru.12 for ; Fri, 22 May 2020 05:03:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nuviainc-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=U2Ul755x9zJYmXE6BBSuSFuQI184gFIgs45Xy+xA+rs=; b=L7xHQs4Pb/9+J8RLLkGuf1FH9ftdtWiVEmaqxS0nRPFSV4GIZQoUzGbKhCuM1dN1Da qbvKgDvKswzMlcKWhPvyODPXTihIdx4ecG38D6uvdAC16lbJPeil3p1GzIIy6YqatQrG 7LwIrDfKIjIh4iZacwwKofEnl9GLL0YkE34M6gdRlXUEtDNOSzE4WZBCcInj/6sYLCbi cveGoIVQ4hq6EwRbiK+H4WemIfs90fBRyB5ptL6nAjmvwsNee4wtE57NKh64mYSZBzcz ZXNPvVQ+YFZsD+Xs5bZn2tSQihVn4IJRqlqmpboqAH/Dw8k2Q+s/UQ5r/Vn9VER04CFa yQQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=U2Ul755x9zJYmXE6BBSuSFuQI184gFIgs45Xy+xA+rs=; b=UrHi5DCGyOBLgoxyTU6120Gc1zlPw2fczVK9qdUu8/t6YeH3RSTS0nmv6CUwrWvgGx efhI3neUQ+2O2MTeaLUKBVobPf5/nZAa+c99dFxOSE44R401icVtHNRpxP86p7LoON98 Uomr///7moeWYqTYUZxndWhVJ8PSBiNCR6HPu+7dreeeWsYwbw10gLpc/r2QgwQcVGvb +ly56n+QLhkwBDHDxDAqS9WOlFpk3+dWfTzQubYBqW3D2ePNxKv3rMp3CKT7TRjj+U8Y +BH4NPlINEsrBsFbuCu7057RpdmVGXZtWKdfbHkB4KRlJ504Qh3Teu+hzWNfUkMSKPw8 CLJA== X-Gm-Message-State: AOAM530VcE6kzGUzOJJua8GSXl1WDuj9/uSZMW/MBtv+oQKr7BG6K8K5 ccWq6wosXuxxtWNze83iHobX4w== X-Google-Smtp-Source: ABdhPJx2MbognWAmEfvIY6pWuGCm4zTR+PZEJmxgP3wkOioyr0u6pswEXiADvteloW5A+TMv8GLVTQ== X-Received: by 2002:adf:f601:: with SMTP id t1mr3336991wrp.207.1590149030793; Fri, 22 May 2020 05:03:50 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id x17sm9246298wrp.71.2020.05.22.05.03.49 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 May 2020 05:03:50 -0700 (PDT) Date: Fri, 22 May 2020 13:03:48 +0100 From: "Leif Lindholm" To: Ard Biesheuvel Cc: devel@edk2.groups.io, liming.gao@intel.com, lersek@redhat.com, philmd@redhat.com Subject: Re: [PATCH v2] ArmPkg/PlatformBootManagerLib: connect non-discoverable USB hosts Message-ID: <20200522120348.GB1923@vanye> References: <20200522084006.32006-1-ard.biesheuvel@arm.com> MIME-Version: 1.0 In-Reply-To: <20200522084006.32006-1-ard.biesheuvel@arm.com> User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 > --- > > 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 >