From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f65.google.com (mail-wr1-f65.google.com [209.85.221.65]) by mx.groups.io with SMTP id smtpd.web10.11677.1590149748507371307 for ; Fri, 22 May 2020 05:15:48 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@nuviainc-com.20150623.gappssmtp.com header.s=20150623 header.b=Fv0/Wy+x; spf=pass (domain: nuviainc.com, ip: 209.85.221.65, mailfrom: leif@nuviainc.com) Received: by mail-wr1-f65.google.com with SMTP id y17so1557895wrn.11 for ; Fri, 22 May 2020 05:15:48 -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=II23LM0pLuKymurzZlgsRgt1vnDSx8iA+cXaR0tUxZM=; b=Fv0/Wy+xUyVhoEnNUYycjEsm/gClcQUr23OKI+mymkJA39rLRO0ipwHYIJeNlup9sv McLsyTRpJU27BJ3NiqdjvtvWIKhuhQ+pK1WcNAyBvNjOpEccbJEZFRplJPtvV50QAmDN E/ql2MB40JHJLlVXAVazT3xKwPWlYE1EklqRsCLpY+mQkDB1ske08i+hzTSi6l07ZUvg YJWB86q2LZIROlzfSse5zspDQFKA6IMdrMw1XN3Nd0VpEjd+D7zf1xODUQYqGpbuW9cx uFpq2RQjm9IXpUJ1g+YSW8TwFLql3IK55fQbQVNCiNRe25ZzzvcAmRX0lqF6UEponft3 Y3EA== 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=II23LM0pLuKymurzZlgsRgt1vnDSx8iA+cXaR0tUxZM=; b=bUAjjC3iEUJKuLBAVP+dcY8QoJ47Chu2UxntXk4C3xAd3ghSOk6VGaBHzu22XwkyAg vk2TzX5SDt1OXooSpysTZlOBpUa4K5BeEjKSlwjrACf6gXwrkpJqsXrXfnRC3aSf+fo3 dc+syFM5HowleksvjIT2zqEObY3ZbOpd8fIXEaWgbCKvg113MI/9TyakEhxzaviITpS/ 6eZGkGO5x6DaPEZdn55kIIPpBhUz1qfiMzqQXrmbOphs6NC209xFETH/xn5zDh6N4Ohd HnrDe/dmPy1e4NJ5PJEiZysIXh9A9/iLCG8IpqAGxYxTHMrll9Px2O2V8k04NiS4ab8g OvnA== X-Gm-Message-State: AOAM532La5r5DWtOSyAoBr7otEMNPfiH0p0WtpYE5VrMKwh+2jMPaBX8 14eUuKcCidZH0CRjiUQuMk239Q== X-Google-Smtp-Source: ABdhPJzxlJyjCCnMqaqIv/oJboKI2lmb7kx87jyOPpYrmD96hfFT8AQVczNTycc9i+fNcXDTypd23Q== X-Received: by 2002:a5d:6283:: with SMTP id k3mr3161417wru.62.1590149747077; Fri, 22 May 2020 05:15:47 -0700 (PDT) Return-Path: Received: from vanye ([2001:470:1f09:12f0:b26e:bfff:fea9:f1b8]) by smtp.gmail.com with ESMTPSA id h1sm9921501wme.42.2020.05.22.05.15.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 22 May 2020 05:15:46 -0700 (PDT) Date: Fri, 22 May 2020 13:15:44 +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: <20200522121544.GD1923@vanye> References: <20200522084006.32006-1-ard.biesheuvel@arm.com> <20200522120348.GB1923@vanye> <94ddc6a5-3765-7891-5236-6210648295e8@arm.com> MIME-Version: 1.0 In-Reply-To: <94ddc6a5-3765-7891-5236-6210648295e8@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 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 > > > > > > > > > > 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 > > > >