From mboxrd@z Thu Jan  1 00:00:00 1970
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com)
Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28])
 by groups.io with SMTP; Mon, 24 Jun 2019 16:16:34 -0700
Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mx1.redhat.com (Postfix) with ESMTPS id E323D3082AF2;
	Mon, 24 Jun 2019 23:16:27 +0000 (UTC)
Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-226.ams2.redhat.com [10.36.116.226])
	by smtp.corp.redhat.com (Postfix) with ESMTP id 11C8B6085B;
	Mon, 24 Jun 2019 23:16:24 +0000 (UTC)
Subject: Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly
To: devel@edk2.groups.io, dwmw2@infradead.org
Cc: Ray Ni <ray.ni@intel.com>
References: <20190621223156.701502-1-dwmw2@infradead.org>
 <20190621223156.701502-6-dwmw2@infradead.org>
From: "Laszlo Ersek" <lersek@redhat.com>
Message-ID: <3d62cecb-d235-2b7e-3342-b1f6e4a619c6@redhat.com>
Date: Tue, 25 Jun 2019 01:16:24 +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: <20190621223156.701502-6-dwmw2@infradead.org>
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Mon, 24 Jun 2019 23:16:33 +0000 (UTC)
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

On 06/22/19 00:31, David Woodhouse wrote:
> I know, I said it was Someone Else's Problem. But it annoyed me.
> 
> My initial thought was to look for VIRTIO_DEVICE_PROTOCOL on the same
> handle but I don't think I can do that if I can't rely on VirtIO being
> present in the build. This will do.
> 
> Signed-off-by: David Woodhouse <dwmw2@infradead.org>
> ---
>  .../UefiBootManagerLib/BmBootDescription.c    | 30 +++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> index dd4d160f31..b7d9e98790 100644
> --- a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> @@ -661,6 +661,8 @@ BmGetMiscDescription (
>    CHAR16                          *Description;
>    EFI_BLOCK_IO_PROTOCOL           *BlockIo;
>    EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *Fs;
> +  EFI_PCI_IO_PROTOCOL             *PciIo;
> +  PCI_TYPE00                      Pci;
>  
>    switch (BmDevicePathType (DevicePathFromHandle (Handle))) {
>    case BmAcpiFloppyBoot:
> @@ -698,9 +700,33 @@ BmGetMiscDescription (
>      Status = gBS->HandleProtocol (Handle, &gEfiSimpleFileSystemProtocolGuid, (VOID **) &Fs);
>      if (!EFI_ERROR (Status)) {
>        Description = L"Non-Block Boot Device";
> -    } else {
> -      Description = L"Misc Device";
> +      break;
> +    }
> +    Status = gBS->HandleProtocol (Handle, &gEfiPciIoProtocolGuid, (VOID **) &PciIo);
> +    if (!EFI_ERROR (Status)) {
> +      Status = PciIo->Pci.Read (
> +                            PciIo,                        // (protocol, device)
> +                                                          // handle
> +                            EfiPciIoWidthUint32,          // access width & copy
> +                                                          // mode
> +                            0,                            // Offset
> +                            sizeof Pci / sizeof (UINT32), // Count
> +                            &Pci                          // target buffer
> +                            );

(1) Can we save some cycles by reading just
PCI_DEVICE_INDEPENDENT_REGION, rather than PCI_TYPE00? It would not
complicate the code either.

> +      //
> +      // If the same node is a Qumranet/Red Hat PCI device, it's VirtIO.
> +      //
> +      if (!EFI_ERROR (Status) &&
> +          (Pci.Hdr.VendorId == 0x1AF4) &&
> +          (Pci.Hdr.DeviceId >= 0x1000) &&
> +          (Pci.Hdr.DeviceId <= 0x103F) &&
> +          (Pci.Hdr.RevisionID == 0x00)) {

(2) This will match legacy and transitional virtio devices, but not
modern-only virtio devices. (SeaBIOS supports modern-only devices too.)
Please see the check for the latter in Virtio10BindingSupported().

(Admittedly, "Pci.Device.SubsystemID" cannot be determined from
PCI_DEVICE_INDEPENDENT_REGION...)


In general I think this approach is viable; at the worst we might have
to gate the code with a Feature PCD. Let's see what Ray says.

Thanks
Laszlo

> +        Description = L"VirtIO Device";
> +        break;
> +      }
>      }
> +
> +    Description = L"Misc Device";
>      break;
>    }
>  
>