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; Tue, 25 Jun 2019 06:45:06 -0700
Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11])
	(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))
	(No client certificate requested)
	by mx1.redhat.com (Postfix) with ESMTPS id BF0D1C1EB1F4;
	Tue, 25 Jun 2019 13:45:00 +0000 (UTC)
Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-204.ams2.redhat.com [10.36.116.204])
	by smtp.corp.redhat.com (Postfix) with ESMTP id 35CC0600C7;
	Tue, 25 Jun 2019 13:44:57 +0000 (UTC)
Subject: Re: [PATCH v2 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly
To: David Woodhouse <dwmw2@infradead.org>, devel@edk2.groups.io
Cc: Ray Ni <ray.ni@intel.com>
References: <20190625114859.795331-1-dwmw2@infradead.org>
 <20190625114859.795331-6-dwmw2@infradead.org>
From: "Laszlo Ersek" <lersek@redhat.com>
Message-ID: <4936db7b-242a-b125-46fa-cde763306008@redhat.com>
Date: Tue, 25 Jun 2019 15:44:56 +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: <20190625114859.795331-6-dwmw2@infradead.org>
X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Tue, 25 Jun 2019 13:45:05 +0000 (UTC)
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit

On 06/25/19 13:48, 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    | 34 +++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c b/MdeModulePkg/Library/UefiBootManagerLib/BmBootDescription.c
> index 06edba8b4d..95adb9a7d3 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,37 @@ 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
> +                            );
> +      //
> +      // If the same node is a Qumranet/Red Hat PCI device, it's VirtIO.
> +      //
> +      if (!EFI_ERROR (Status) && (Pci.Hdr.VendorId == 0x1AF4))
> +        //
> +        // Separate checks for legacy/transitional VirtIO vs. Virtio 1.0+
> +        //
> +        if (((Pci.Hdr.DeviceId >= 0x1000) && (Pci.Hdr.DeviceId <= 0x103F) &&
> +             (Pci.Hdr.RevisionID == 0x00)) ||
> +            (Pci.Hdr.DeviceId >= 0x1040 && Pci.Hdr.DeviceId <= 0x107F &&
> +             Pci.Hdr.RevisionID >= 0x01 && Pci.Device.SubsystemID >= 0x40 &&
> +             (Pci.Hdr.Status & EFI_PCI_STATUS_CAPABILITY) != 0)) {
> +        Description = L"VirtIO Device";
> +        break;
> +      }

This code is functionally correct, but there are two syntactical issues
with it.

(1) The last three lines -- the assignment to Description, the break
statement, and the closing brace -- should be indented one level more
deeply.

(2) Which in turn shows that the "if" statement that checks "Status" and
"VendorId" lacks an opening brace. The brace is required by the edk2
coding style.

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

With those fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo