Re: [edk2-devel] [PATCH 6/7] MdeModulePkg/UefiBootManagerLib: describe VirtIO devices correctly

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

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;
>    }
>  
>