Re: [PATCH v2 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

["Laszlo Ersek" <lersek@redhat.com>]

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