[PATCH 0/4] Virtio non-discoverable devices

[PATCH 0/4] Virtio non-discoverable devices

[Daniil Egranov]

This is an attempt to add MMIO Virtio devices into the
non-discoverable device registration procedure and allow
Virtio PCI drivers to recognize and program such devices 
correctly.
The main issue is that the set of MMIO registers is different
from PCI, plus the width of similar registers are not 
always the same. The code implements the translation of 
the PCI IO registers to MMIO registers. 
Another difference between PCI and MMIO Virtio devices found 
during the testing is that MMIO devices may require more 
registers to be programmed compared to PCI. The VirtioPciDeviceDxe
was patched to detect non-discoverable MMIO devices and allow
calling a PCI MemIo protocol function.

This set of patches was tested with MMIO Virtio Block and
Virtio Net devices.

Daniil Egranov (4):
  MdeModulePkg: Added new Virtio non-discoverable type and GUID
  NonDiscoverableDeviceRegistrationLib: Added Virtio support
  NonDiscoverablePciDeviceDxe: Added MMIO Virtio support
  VirtioPciDeviceDxe: Added non-discoverable Virtio support

 .../NonDiscoverablePciDeviceDxe.c                  |   3 +-
 .../NonDiscoverablePciDeviceDxe.inf                |   5 +-
 .../NonDiscoverablePciDeviceIo.c                   | 240 ++++++++++++++++++++-
 MdeModulePkg/Include/Guid/NonDiscoverableDevice.h  |   3 +
 .../Library/NonDiscoverableDeviceRegistrationLib.h |   1 +
 .../NonDiscoverableDeviceRegistrationLib.c         |   3 +
 .../NonDiscoverableDeviceRegistrationLib.inf       |   1 +
 MdeModulePkg/MdeModulePkg.dec                      |   1 +
 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c       | 143 +++++++++++-
 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h       |  21 +-
 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf  |   4 +-
 OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c    | 117 +++++++++-
 12 files changed, 528 insertions(+), 14 deletions(-)

-- 
2.11.0

[PATCH 1/4] MdeModulePkg: Added new Virtio non-discoverable type and GUID

[Daniil Egranov]

Added Virtio type and GUID to the list of supported
non-discoverable devices.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
---
 MdeModulePkg/Include/Guid/NonDiscoverableDevice.h                   | 3 +++
 MdeModulePkg/Include/Library/NonDiscoverableDeviceRegistrationLib.h | 1 +
 MdeModulePkg/MdeModulePkg.dec                                       | 1 +
 3 files changed, 5 insertions(+)

diff --git a/MdeModulePkg/Include/Guid/NonDiscoverableDevice.h b/MdeModulePkg/Include/Guid/NonDiscoverableDevice.h
index d182e4b9d2..9d598d0487 100644
--- a/MdeModulePkg/Include/Guid/NonDiscoverableDevice.h
+++ b/MdeModulePkg/Include/Guid/NonDiscoverableDevice.h
@@ -44,6 +44,8 @@
 #define EDKII_NON_DISCOVERABLE_XHCI_DEVICE_GUID \
   { 0xB1BE0BC5, 0x6C28, 0x442D, {0xAA, 0x37, 0x15, 0x1B, 0x42, 0x57, 0xBD, 0x78 } }
 
+#define EDKII_NON_DISCOVERABLE_VIRTIO_DEVICE_GUID \
+  { 0xA72A1646, 0x1A4D, 0x4A84, {0xB6, 0xAC, 0xEF, 0x18, 0x13, 0x62, 0xC5, 0x85 } }
 
 extern EFI_GUID gEdkiiNonDiscoverableAhciDeviceGuid;
 extern EFI_GUID gEdkiiNonDiscoverableAmbaDeviceGuid;
@@ -54,5 +56,6 @@ extern EFI_GUID gEdkiiNonDiscoverableSdhciDeviceGuid;
 extern EFI_GUID gEdkiiNonDiscoverableUfsDeviceGuid;
 extern EFI_GUID gEdkiiNonDiscoverableUhciDeviceGuid;
 extern EFI_GUID gEdkiiNonDiscoverableXhciDeviceGuid;
+extern EFI_GUID gEdkiiNonDiscoverableVirtioDeviceGuid;
 
 #endif
diff --git a/MdeModulePkg/Include/Library/NonDiscoverableDeviceRegistrationLib.h b/MdeModulePkg/Include/Library/NonDiscoverableDeviceRegistrationLib.h
index c2d9e48353..a3d35af275 100644
--- a/MdeModulePkg/Include/Library/NonDiscoverableDeviceRegistrationLib.h
+++ b/MdeModulePkg/Include/Library/NonDiscoverableDeviceRegistrationLib.h
@@ -26,6 +26,7 @@ typedef enum {
   NonDiscoverableDeviceTypeUfs,
   NonDiscoverableDeviceTypeUhci,
   NonDiscoverableDeviceTypeXhci,
+  NonDiscoverableDeviceTypeVirtio,
   NonDiscoverableDeviceTypeMax,
 } NON_DISCOVERABLE_DEVICE_TYPE;
 
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 5d561ff484..cd18bbfbd2 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -396,6 +396,7 @@
   gEdkiiNonDiscoverableUfsDeviceGuid = { 0x2EA77912, 0x80A8, 0x4947, {0xBE, 0x69, 0xCD, 0xD0, 0x0A, 0xFB, 0xE5, 0x56 } }
   gEdkiiNonDiscoverableUhciDeviceGuid = { 0xA8CDA0A2, 0x4F37, 0x4A1B, {0x8E, 0x10, 0x8E, 0xF3, 0xCC, 0x3B, 0xF3, 0xA8 } }
   gEdkiiNonDiscoverableXhciDeviceGuid = { 0xB1BE0BC5, 0x6C28, 0x442D, {0xAA, 0x37, 0x15, 0x1B, 0x42, 0x57, 0xBD, 0x78 } }
+  gEdkiiNonDiscoverableVirtioDeviceGuid = { 0xA72A1646, 0x1A4D, 0x4A84, {0xB6, 0xAC, 0xEF, 0x18, 0x13, 0x62, 0xC5, 0x85 } }
 
   ## Include/Guid/PlatformHasAcpi.h
   gEdkiiPlatformHasAcpiGuid = { 0xf0966b41, 0xc23f, 0x41b9, { 0x96, 0x04, 0x0f, 0xf7, 0xe1, 0x11, 0x96, 0x5a } }
-- 
2.11.0

[PATCH 2/4] NonDiscoverableDeviceRegistrationLib: Added Virtio support

[Daniil Egranov]

Added Virtio non-discoverable device case.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
---
 .../NonDiscoverableDeviceRegistrationLib.c                             | 3 +++
 .../NonDiscoverableDeviceRegistrationLib.inf                           | 1 +
 2 files changed, 4 insertions(+)

diff --git a/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.c b/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.c
index 536dfc7297..2cc3da7772 100644
--- a/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.c
+++ b/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.c
@@ -67,6 +67,9 @@ GetGuidFromType (
   case NonDiscoverableDeviceTypeXhci:
     return &gEdkiiNonDiscoverableXhciDeviceGuid;
 
+  case NonDiscoverableDeviceTypeVirtio:
+    return &gEdkiiNonDiscoverableVirtioDeviceGuid;
+
   default:
     return NULL;
   }
diff --git a/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf b/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf
index 763f9a0a6f..8ebf869d1d 100644
--- a/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf
+++ b/MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf
@@ -46,3 +46,4 @@
   gEdkiiNonDiscoverableUfsDeviceGuid     ## CONSUMES   ## GUID
   gEdkiiNonDiscoverableUhciDeviceGuid    ## CONSUMES   ## GUID
   gEdkiiNonDiscoverableXhciDeviceGuid    ## CONSUMES   ## GUID
+  gEdkiiNonDiscoverableVirtioDeviceGuid  ## CONSUMES   ## GUID
-- 
2.11.0

[PATCH 3/4] NonDiscoverablePciDeviceDxe: Added MMIO Virtio support

[Daniil Egranov]

Added PCI IO to MMIO translation for Virtio case into the PCI IO 
protocol functions.
Added Virtio device type into the PCI IO protocol initialization
procedure.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
---
 .../NonDiscoverablePciDeviceDxe.c                  |   3 +-
 .../NonDiscoverablePciDeviceDxe.inf                |   5 +-
 .../NonDiscoverablePciDeviceIo.c                   | 240 ++++++++++++++++++++-
 3 files changed, 241 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
index 3e9ff6620d..2c3eeca914 100644
--- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
+++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.c
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (C) 2016, Linaro Ltd. All rights reserved.<BR>
+  Copyright (C) 2016-2018, Linaro Ltd. All rights reserved.<BR>
 
   This program and the accompanying materials are licensed and made available
   under the terms and conditions of the BSD License which accompanies this
@@ -32,6 +32,7 @@ SupportedNonDiscoverableDevices[] = {
   &gEdkiiNonDiscoverableUfsDeviceGuid,
   &gEdkiiNonDiscoverableUhciDeviceGuid,
   &gEdkiiNonDiscoverableXhciDeviceGuid,
+  &gEdkiiNonDiscoverableVirtioDeviceGuid
 };
 
 //
diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
index ac551a82ab..a7bf5a5fc1 100644
--- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
+++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceDxe.inf
@@ -1,7 +1,7 @@
 ## @file
 # PCI I/O driver for non-discoverable devices.
 #
-# Copyright (C) 2016, Linaro Ltd.
+# Copyright (C) 2016-2018, Linaro Ltd.
 #
 # This program and the accompanying materials are licensed and made available
 # under the terms and conditions of the BSD License which accompanies this
@@ -30,6 +30,7 @@
 [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
+  OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
   BaseMemoryLib
@@ -54,3 +55,5 @@
   gEdkiiNonDiscoverableUfsDeviceGuid        ## CONSUMES ## GUID
   gEdkiiNonDiscoverableUhciDeviceGuid       ## CONSUMES ## GUID
   gEdkiiNonDiscoverableXhciDeviceGuid       ## CONSUMES ## GUID
+  gEdkiiNonDiscoverableVirtioDeviceGuid     ## CONSUMES ## GUID
+
diff --git a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
index 0e42ae4bf6..cb99bf0ba6 100644
--- a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
+++ b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverablePciDeviceIo.c
@@ -1,7 +1,7 @@
 /** @file
 
   Copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
-  Copyright (c) 2016, Linaro, Ltd. All rights reserved.<BR>
+  Copyright (c) 2016-2018, Linaro, Ltd. All rights reserved.<BR>
 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
@@ -17,8 +17,12 @@
 
 #include <Library/DxeServicesTableLib.h>
 
+#include <Library/IoLib.h>
+
 #include <IndustryStandard/Acpi.h>
 
+#include <IndustryStandard/Virtio.h>
+
 #include <Protocol/PciRootBridgeIo.h>
 
 typedef struct {
@@ -374,6 +378,82 @@ PciIoMemWrite (
 }
 
 /**
+  Translate PCI IO to MMIO for VirtIo Non-Discoverable devices.
+
+  @param  BaseAddress           A base MMIO memory address of a VirtIo device.
+  @param  PciIoOffset           PCI IO offset.
+  @param  DevConfigAddress      The address is a device specific config space.
+
+  @retval EFI_SUCCESS           The address translated successfully.
+  @retval EFI_UNSUPPORTED       The PCI IO address can not be translated.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+VirtioPciIoToMemIoTranslation (
+  IN     UINTN                        BaseAddress,
+  IN     UINTN                        PciIoOffset,
+  IN OUT UINTN                        *Address,
+  IN OUT BOOLEAN                      *DevConfigAddress
+  )
+{
+  EFI_STATUS  Status;
+  UINTN       Offset;
+  INTN        DevConfigSpaceOffset;
+
+  Status               = EFI_SUCCESS;
+  Offset               = PciIoOffset;
+  *DevConfigAddress    = FALSE;
+  DevConfigSpaceOffset = 0;
+
+  //
+  //Check if it's device specific config space
+  //
+  if (PciIoOffset >= VIRTIO_DEVICE_SPECIFIC_CONFIGURATION_OFFSET_PCI ) {
+    Offset = VIRTIO_DEVICE_SPECIFIC_CONFIGURATION_OFFSET_PCI;
+    DevConfigSpaceOffset = PciIoOffset - VIRTIO_DEVICE_SPECIFIC_CONFIGURATION_OFFSET_PCI;
+    *DevConfigAddress = TRUE;
+  }
+
+  switch (Offset) {
+  case VIRTIO_PCI_OFFSET_DEVICE_FEATURES:
+    *Address = BaseAddress + VIRTIO_MMIO_OFFSET_HOST_FEATURES;
+    break;
+  case VIRTIO_PCI_OFFSET_GUEST_FEATURES:
+    *Address = BaseAddress + VIRTIO_MMIO_OFFSET_GUEST_FEATURES;
+    break;
+  case VIRTIO_PCI_OFFSET_QUEUE_ADDRESS:
+    *Address = BaseAddress + VIRTIO_MMIO_OFFSET_QUEUE_PFN;
+    break;
+  case VIRTIO_PCI_OFFSET_QUEUE_SIZE:
+    *Address = BaseAddress + VIRTIO_MMIO_OFFSET_QUEUE_NUM_MAX;
+    break;
+  case VIRTIO_PCI_OFFSET_QUEUE_SELECT:
+    *Address = BaseAddress + VIRTIO_MMIO_OFFSET_QUEUE_SEL;
+    break;
+  case VIRTIO_PCI_OFFSET_QUEUE_NOTIFY:
+    *Address = BaseAddress + VIRTIO_MMIO_OFFSET_QUEUE_NOTIFY;
+    break;
+  case VIRTIO_PCI_OFFSET_QUEUE_DEVICE_STATUS:
+    *Address = BaseAddress + VIRTIO_MMIO_OFFSET_STATUS;
+    break;
+  case VIRTIO_PCI_OFFSET_QUEUE_DEVICE_ISR:
+    Status = EFI_UNSUPPORTED;
+    break;
+  case VIRTIO_DEVICE_SPECIFIC_CONFIGURATION_OFFSET_PCI:
+  case VIRTIO_DEVICE_SPECIFIC_CONFIGURATION_OFFSET_PCI_WITH_MSI_X:
+    *Address = BaseAddress + VIRTIO_DEVICE_SPECIFIC_CONFIGURATION_OFFSET_MMIO + DevConfigSpaceOffset;
+    break;
+  default:
+    Status = EFI_UNSUPPORTED;
+    break;
+  }
+
+  return Status;
+}
+
+/**
   Enable a PCI driver to access PCI controller registers in the PCI memory or I/O space.
 
   @param  This                  A pointer to the EFI_PCI_IO_PROTOCOL instance.
@@ -398,8 +478,72 @@ PciIoIoRead (
   IN OUT VOID                         *Buffer
   )
 {
-  ASSERT (FALSE);
-  return EFI_UNSUPPORTED;
+  NON_DISCOVERABLE_PCI_DEVICE         *Dev;
+  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR   *Desc;
+  UINTN                               Length;
+  UINTN                               Address;
+  UINT32                              ReadData;
+  BOOLEAN                             IsDevConfigSpace;
+  EFI_STATUS                          Status;
+
+  if (Buffer == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO (This);
+
+  //
+  // Only allow accesses to the BARs we emulate
+  //
+  Status = GetBarResource (Dev, BarIndex, &Desc);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Length = Count << ((UINTN)Width & 0x3);
+  if (Offset + Length > Desc->AddrLen) {
+    return EFI_UNSUPPORTED;
+  }
+
+  //
+  // Non-Discoverable devices are MMIO devices. Translate PCI IO requests to MMIO.
+  //
+
+  //
+  // VirtIo
+  //
+  if (CompareGuid (Dev->Device->Type, &gEdkiiNonDiscoverableVirtioDeviceGuid)) {
+    //
+    // Check for a valid data size
+    //
+    if ((Length != 1) && (Length != 2) &&
+        (Length != 4) && (Length != 8)) {
+      return EFI_INVALID_PARAMETER;
+    }
+
+    Status = VirtioPciIoToMemIoTranslation (Desc->AddrRangeMin, Offset, &Address, &IsDevConfigSpace);
+    if (!EFI_ERROR (Status)) {
+      if (IsDevConfigSpace == TRUE) { // Device-specific configuration registers
+        //
+        // The device-specific memory area of Virtio-MMIO can only be written in
+        // byte accesses. This is not currently in the Virtio spec.
+        //
+        MmioReadBuffer8 (Address, Length, Buffer);
+      } else if (Length < 8) {  // 32-bit MMIO registers
+        ReadData = MmioRead32 (Address);
+        // The PCI IO register width may not be the same as MMIO register.
+        // Virtio MMIO registers are always 32-bit
+        CopyMem (Buffer, &ReadData, Length);
+      } else {
+        Status = EFI_INVALID_PARAMETER;
+      }
+    }
+  } else {
+    Status = EFI_UNSUPPORTED;
+  }
+
+  ASSERT_EFI_ERROR (Status);
+  return Status;
 }
 
 /**
@@ -427,8 +571,72 @@ PciIoIoWrite (
   IN OUT VOID                         *Buffer
   )
 {
-  ASSERT (FALSE);
-  return EFI_UNSUPPORTED;
+  NON_DISCOVERABLE_PCI_DEVICE         *Dev;
+  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR   *Desc;
+  UINTN                               Length;
+  UINTN                               Address;
+  UINT32                              WriteData;
+  BOOLEAN                             IsDevConfigSpace;
+  EFI_STATUS                          Status;
+
+  if (Buffer == NULL) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Dev = NON_DISCOVERABLE_PCI_DEVICE_FROM_PCI_IO (This);
+
+  //
+  // Only allow accesses to the BARs we emulate
+  //
+  Status = GetBarResource (Dev, BarIndex, &Desc);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  Length = Count << ((UINTN)Width & 0x3);
+  if (Offset + Length > Desc->AddrLen) {
+    return EFI_UNSUPPORTED;
+  }
+
+  //
+  // Non-Discoverable devices are MMIO devices. Translate PCI IO requests to MMIO.
+  //
+
+  //
+  // VirtIo
+  //
+  if (CompareGuid (Dev->Device->Type, &gEdkiiNonDiscoverableVirtioDeviceGuid)) {
+    //
+    // Check for a valid data size
+    //
+    if ((Length != 1) && (Length != 2) &&
+        (Length != 4) && (Length != 8)) {
+      return EFI_INVALID_PARAMETER;
+    }
+
+    Status = VirtioPciIoToMemIoTranslation (Desc->AddrRangeMin, Offset, &Address, &IsDevConfigSpace);
+    if (!EFI_ERROR (Status)) {
+      if (IsDevConfigSpace == TRUE) { // Device-specific configuration registers
+        //
+        // The device-specific memory area of Virtio-MMIO can only be written in
+        // byte accesses. This is not currently in the Virtio spec.
+        //
+        MmioWriteBuffer8 (Address, Length, Buffer);
+      } else if (Length < 8) {  // 32-bit MMIO registers
+        // The PCI IO register width may not be the same as MMIO register.
+        // Virtio MMIO registers are always 32-bit
+        CopyMem (&WriteData, Buffer, Length);
+        MmioWrite32 (Address, WriteData);
+      } else {
+        return EFI_INVALID_PARAMETER;
+      }
+    }
+  } else {
+    Status = EFI_UNSUPPORTED;
+  }
+
+  ASSERT_EFI_ERROR (Status);
+  return Status;
 }
 
 /**
@@ -1408,6 +1616,7 @@ InitializePciIoProtocol (
 {
   EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR   *Desc;
   INTN                                Idx;
+  UINT32                              VirtioMagicValue;
 
   InitializeListHead (&Dev->UncachedAllocationList);
 
@@ -1471,6 +1680,15 @@ InitializePciIoProtocol (
     Dev->ConfigSpace.Hdr.ClassCode[1] = 0x9; // UFS controller subclass;
     Dev->ConfigSpace.Hdr.ClassCode[2] = PCI_CLASS_MASS_STORAGE;
     Dev->BarOffset = 0;
+  } else if (CompareGuid (Dev->Device->Type,
+                          &gEdkiiNonDiscoverableVirtioDeviceGuid)) {
+    Dev->ConfigSpace.Hdr.VendorId = VIRTIO_VENDOR_ID; // Required Virtio vendor
+    Dev->ConfigSpace.Hdr.DeviceId = 0x1000; // Required Virtio device id
+    Dev->ConfigSpace.Hdr.RevisionID = 0x0;
+    Dev->ConfigSpace.Hdr.ClassCode[0] = 0x0; // don't care
+    Dev->ConfigSpace.Hdr.ClassCode[1] = 0x0; // don't care
+    Dev->ConfigSpace.Hdr.ClassCode[2] = 0x0; // don't care
+    Dev->BarOffset = 0;
   } else {
     ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
   }
@@ -1496,6 +1714,18 @@ InitializePciIoProtocol (
     }
 
     Dev->ConfigSpace.Device.Bar[Idx] = (UINT32)Desc->AddrRangeMin;
+
+    //
+    // Initialize Virtio device Id
+    //
+    if (CompareGuid (Dev->Device->Type, &gEdkiiNonDiscoverableVirtioDeviceGuid)) {
+      VirtioMagicValue = MmioRead32 (Dev->ConfigSpace.Device.Bar[Idx] + VIRTIO_MMIO_OFFSET_MAGIC);
+      if (VirtioMagicValue == VIRTIO_MMIO_MAGIC) {
+        Dev->ConfigSpace.Device.SubsystemID = MmioRead32 (Dev->ConfigSpace.Device.Bar[Idx] +
+                                                          VIRTIO_MMIO_OFFSET_DEVICE_ID);
+      }
+    }
+
     Dev->BarCount++;
 
     if (Desc->AddrSpaceGranularity == 64) {
-- 
2.11.0

[PATCH 4/4] VirtioPciDeviceDxe: Added non-discoverable Virtio support

[Daniil Egranov]

The VirtioPciDeviceDxe was extended to support a non-discoverable
MMIO Virtio case.
The Virtio spec defines both PCI and MMIO device types with the 
set of registers that are not the same between these two types
of devices. All PCI registers have corresponding MMIO registers
but the number of registers is less then MMIO. The width of MMIO
registers and PCI registers is not always the same.
Compared to PCI, MMIO Virtio devices required more registers to be
programmed in some cases. Added detection that a PCI device is 
based on a non-discoverable device and allows MMIO transactions.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
---
 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c      | 143 +++++++++++++++++++++-
 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h      |  21 +++-
 OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf |   4 +-
 OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c   | 117 +++++++++++++++++-
 4 files changed, 278 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
index d4b4ec21c3..df2a4ea116 100644
--- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
+++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c
@@ -4,7 +4,7 @@
 
   Copyright (C) 2012, Red Hat, Inc.
   Copyright (c) 2012 - 2016, Intel Corporation. All rights reserved.<BR>
-  Copyright (C) 2013, ARM Ltd.
+  Copyright (C) 2013 - 2018, ARM Ltd.
   Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
 
   This program and the accompanying materials are licensed and made available
@@ -215,6 +215,147 @@ VirtioPciIoWrite (
 
 /**
 
+  Read a word from Region 0 of the device specified by PciMemIo.
+
+  Region 0 must be an iomem region. This is an internal function for the PCI
+  implementation of the protocol.
+
+  @param[in] Dev          Virtio PCI device.
+
+  @param[in] FieldOffset  Source offset.
+
+  @param[in] FieldSize    Source field size, must be in { 1, 2, 4, 8 }.
+
+  @param[in] BufferSize   Number of bytes available in the target buffer. Must
+                          equal FieldSize.
+
+  @param[out] Buffer      Target buffer.
+
+
+  @return  Status code returned by PciIo->Io.Read().
+
+**/
+EFI_STATUS
+EFIAPI
+VirtioPciMemIoRead (
+  IN  VIRTIO_PCI_DEVICE         *Dev,
+  IN  UINTN                     FieldOffset,
+  IN  UINTN                     FieldSize,
+  IN  UINTN                     BufferSize,
+  OUT VOID                      *Buffer
+  )
+{
+  UINTN                     Count;
+  EFI_PCI_IO_PROTOCOL_WIDTH Width;
+  EFI_PCI_IO_PROTOCOL       *PciIo;
+
+  ASSERT (FieldSize == BufferSize);
+
+  PciIo = Dev->PciIo;
+  Count = 1;
+
+  switch (FieldSize) {
+  case 1:
+    Width = EfiPciIoWidthUint8;
+    break;
+
+  case 2:
+    Width = EfiPciIoWidthUint16;
+    break;
+
+  case 4:
+    Width = EfiPciIoWidthUint32;
+    break;
+
+  case 8:
+    Width = EfiPciIoWidthUint64;
+    break;
+
+  default:
+    ASSERT (FALSE);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  return PciIo->Mem.Read (
+                     PciIo,
+                     Width,
+                     PCI_BAR_IDX0,
+                     FieldOffset,
+                     Count,
+                     Buffer
+                     );
+}
+
+/**
+
+  Write a word into Region 0 of the device specified by PciMemIo.
+
+  Region 0 must be an iomem region. This is an internal function for the PCI
+  implementation of the protocol.
+
+  @param[in] Dev          Virtio PCI device.
+
+  @param[in] FieldOffset  Destination offset.
+
+  @param[in] FieldSize    Destination field size, must be in { 1, 2, 4, 8 }.
+
+  @param[in] Value        Little endian value to write, converted to UINT64.
+                          The least significant FieldSize bytes will be used.
+
+
+  @return  Status code returned by PciIo->Io.Write().
+
+**/
+EFI_STATUS
+EFIAPI
+VirtioPciMemIoWrite (
+  IN  VIRTIO_PCI_DEVICE         *Dev,
+  IN UINTN                      FieldOffset,
+  IN UINTN                      FieldSize,
+  IN UINT64                     Value
+  )
+{
+  UINTN                     Count;
+  EFI_PCI_IO_PROTOCOL_WIDTH Width;
+  EFI_PCI_IO_PROTOCOL       *PciIo;
+
+  PciIo = Dev->PciIo;
+  Count = 1;
+
+  switch (FieldSize) {
+  case 1:
+    Width = EfiPciIoWidthUint8;
+    break;
+
+  case 2:
+    Width = EfiPciIoWidthUint16;
+    break;
+
+  case 4:
+    Width = EfiPciIoWidthUint32;
+    break;
+
+  case 8:
+    Width = EfiPciIoWidthUint64;
+    break;
+
+  default:
+    ASSERT (FALSE);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  return PciIo->Mem.Write (
+                     PciIo,
+                     Width,
+                     PCI_BAR_IDX0,
+                     FieldOffset,
+                     Count,
+                     &Value
+                     );
+}
+
+/**
+
   Device probe function for this driver.
 
   The DXE core calls this function for any given device in order to see if the
diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
index 1f0dc45d50..e91191689c 100644
--- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
+++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h
@@ -2,7 +2,7 @@
 
   Internal definitions for the VirtIo PCI Device driver
 
-  Copyright (C) 2013, ARM Ltd
+  Copyright (C) 2013-2018, ARM Ltd
   Copyright (c) 2017, AMD Inc, All rights reserved.<BR>
 
   This program and the accompanying materials are licensed and made available
@@ -58,6 +58,25 @@ VirtioPciIoWrite (
   IN UINT64                      Value
   );
 
+EFI_STATUS
+EFIAPI
+VirtioPciMemIoRead (
+  IN  VIRTIO_PCI_DEVICE         *Dev,
+  IN  UINTN                      FieldOffset,
+  IN  UINTN                      FieldSize,
+  IN  UINTN                      BufferSize,
+  OUT VOID                       *Buffer
+  );
+
+EFI_STATUS
+EFIAPI
+VirtioPciMemIoWrite (
+  IN  VIRTIO_PCI_DEVICE         *Dev,
+  IN UINTN                       FieldOffset,
+  IN UINTN                       FieldSize,
+  IN UINT64                      Value
+  );
+
 /********************************************
  * PCI Functions for VIRTIO_DEVICE_PROTOCOL
  *******************************************/
diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
index 4b5d1a493e..675b156c82 100644
--- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
+++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf
@@ -2,7 +2,7 @@
 # This driver produces the VirtIo Device Protocol instances for VirtIo PCI
 # Device
 #
-# Copyright (C) 2013, ARM Ltd
+# Copyright (C) 2013-2018, ARM Ltd
 #
 # This program and the accompanying materials are licensed and made available
 # under the terms and conditions of the BSD License which accompanies this
@@ -28,6 +28,7 @@
 
 [Packages]
   MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
   OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
@@ -41,3 +42,4 @@
 [Protocols]
   gEfiPciIoProtocolGuid     ## TO_START
   gVirtioDeviceProtocolGuid ## BY_START
+  gEdkiiNonDiscoverableDeviceProtocolGuid
diff --git a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
index b52060d13d..45b49f0915 100644
--- a/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
+++ b/OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c
@@ -4,7 +4,7 @@
 
   Copyright (C) 2012, Red Hat, Inc.
   Copyright (c) 2012, Intel Corporation. All rights reserved.<BR>
-  Copyright (C) 2013, ARM Ltd.
+  Copyright (C) 2013-2018, ARM Ltd.
   Copyright (C) 2017, AMD Inc, All rights reserved.<BR>
 
   This program and the accompanying materials are licensed and made available
@@ -21,10 +21,76 @@
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiLib.h>
+
+#include <Protocol/NonDiscoverableDevice.h>
+
 #include "VirtioPciDevice.h"
 
 /**
 
+  Check if virtvo PCI device is a non discoverable virtio MMIO device.
+
+  @param[in] Dev       Pointer to the virtio PCI device structure.
+
+  @return              TRUE if PCI device is a non discoverable device.
+
+**/
+STATIC
+BOOLEAN
+VirtioIsNonDiscoverableMmioDevice (
+  IN VIRTIO_PCI_DEVICE  *Dev
+  )
+{
+  EFI_HANDLE                          *HandleBuffer;
+  EFI_PCI_IO_PROTOCOL                 *PciIo;
+  NON_DISCOVERABLE_DEVICE             *Device;
+  EFI_STATUS                          Status;
+  UINTN                               HIndex;
+  UINTN                               HandleCount;
+  BOOLEAN                             RetStatus;
+
+  RetStatus = FALSE;
+  Status = gBS->LocateHandleBuffer (ByProtocol,
+                                    &gEfiPciIoProtocolGuid,
+                                    NULL,
+                                    &HandleCount,
+                                    &HandleBuffer);
+
+  if (!EFI_ERROR (Status)) {
+    for (HIndex = 0; HIndex < HandleCount; ++HIndex) {
+      Status = gBS->OpenProtocol (HandleBuffer[HIndex],
+                                  &gEfiPciIoProtocolGuid,
+                                  (VOID **) &PciIo,
+                                  NULL,
+                                  NULL,
+                                  EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+
+      if (EFI_ERROR (Status)) {
+        continue;
+      }
+
+      if (PciIo == Dev->PciIo) {
+        Status = gBS->OpenProtocol (HandleBuffer[HIndex],
+                                    &gEdkiiNonDiscoverableDeviceProtocolGuid,
+                                    (VOID **) &Device,
+                                    NULL,
+                                    NULL,
+                                    EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+
+        if (!EFI_ERROR (Status)) {
+          RetStatus = TRUE;
+        }
+        break;
+      }
+    }
+  }
+
+  gBS->FreePool (HandleBuffer);
+  return RetStatus;
+}
+
+/**
+
   Read a word from Region 0 of the device specified by VirtIo Device protocol.
 
   The function implements the ReadDevice protocol member of
@@ -219,7 +285,21 @@ VirtioPciSetQueueAlignment (
   IN  UINT32                  Alignment
   )
 {
-  return EFI_SUCCESS;
+  VIRTIO_PCI_DEVICE *Dev;
+  EFI_STATUS Status;
+
+  Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This);
+  Status = EFI_SUCCESS;
+
+  // This register is required to be programmed for MMIO type of devices.
+  // Virtio MMIO device can be registered as a non-discoverable device on
+  // PCI bus, check if it's the case.
+  if (VirtioIsNonDiscoverableMmioDevice (Dev)) {
+    Status = VirtioPciMemIoWrite (Dev, VIRTIO_MMIO_OFFSET_QUEUE_ALIGN,
+                                  sizeof (UINT32), Alignment);
+  }
+
+  return Status;
 }
 
 EFI_STATUS
@@ -229,7 +309,22 @@ VirtioPciSetPageSize (
   IN  UINT32                  PageSize
   )
 {
-  return (PageSize == EFI_PAGE_SIZE) ? EFI_SUCCESS : EFI_UNSUPPORTED;
+  VIRTIO_PCI_DEVICE *Dev;
+  EFI_STATUS Status;
+
+  Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This);
+
+  // This register is required to be programmed for MMIO type of devices.
+  // Virtio MMIO device can be registered as a non-discoverable device on
+  // PCI bus, check if it's the case.
+  if (VirtioIsNonDiscoverableMmioDevice (Dev)) {
+    Status = VirtioPciMemIoWrite (Dev, VIRTIO_MMIO_OFFSET_GUEST_PAGE_SIZE,
+                                  sizeof (UINT32), PageSize);
+  } else {
+    Status = (PageSize == EFI_PAGE_SIZE) ? EFI_SUCCESS : EFI_UNSUPPORTED;
+  }
+
+  return Status;
 }
 
 EFI_STATUS
@@ -254,11 +349,25 @@ VirtioPciSetQueueSize (
   IN  UINT16                 Size
   )
 {
+  VIRTIO_PCI_DEVICE *Dev;
+  EFI_STATUS Status;
+
   //
   // This function is only applicable in Virtio-MMIO.
   // (The QueueSize field is read-only in Virtio proper (PCI))
   //
-  return EFI_SUCCESS;
+  Dev = VIRTIO_PCI_DEVICE_FROM_VIRTIO_DEVICE (This);
+  Status = EFI_SUCCESS;
+
+  // This register is required to be programmed for MMIO type of devices.
+  // Virtio MMIO device can be registered as a non-discoverable device on
+  // PCI bus, check if it's the case.
+  if (VirtioIsNonDiscoverableMmioDevice (Dev)) {
+    Status = VirtioPciMemIoWrite (Dev, VIRTIO_MMIO_OFFSET_QUEUE_NUM,
+                                  sizeof (UINT32), (UINT32) Size);
+  }
+
+  return Status;
 }
 
 EFI_STATUS
-- 
2.11.0

Re: [PATCH 0/4] Virtio non-discoverable devices

[Ard Biesheuvel]

(+ Laszlo)

Hello Daniil,

On 7 March 2018 at 01:36, Daniil Egranov <daniil.egranov@arm.com> wrote:
> This is an attempt to add MMIO Virtio devices into the
> non-discoverable device registration procedure and allow
> Virtio PCI drivers to recognize and program such devices
> correctly.

Why? The purpose of the non-discoverable device layer is to make
non-PCI controllers that can be driven by PCI class drivers appear as
PCI devices. We have started using the base non-discoverable device
protocol for other devices as well, but the PCI wrapper is really only
intended for PCI class drivers.

For VirtIO MMIO, we already have a driver model driver, and given that
you need to patch up differences between MMIO and PCI based virtio in
your code, I am reluctant to incorporate modifications in to the PCI
driver to support MMIO devices.

Is this related to virtio 1.0 support?

Also, could you please ensure next time that you cc all the relevant
people? (Please check the Maintainers file)

> The main issue is that the set of MMIO registers is different
> from PCI, plus the width of similar registers are not
> always the same. The code implements the translation of
> the PCI IO registers to MMIO registers.
> Another difference between PCI and MMIO Virtio devices found
> during the testing is that MMIO devices may require more
> registers to be programmed compared to PCI. The VirtioPciDeviceDxe
> was patched to detect non-discoverable MMIO devices and allow
> calling a PCI MemIo protocol function.
>
> This set of patches was tested with MMIO Virtio Block and
> Virtio Net devices.
>
> Daniil Egranov (4):
>   MdeModulePkg: Added new Virtio non-discoverable type and GUID
>   NonDiscoverableDeviceRegistrationLib: Added Virtio support
>   NonDiscoverablePciDeviceDxe: Added MMIO Virtio support
>   VirtioPciDeviceDxe: Added non-discoverable Virtio support
>
>  .../NonDiscoverablePciDeviceDxe.c                  |   3 +-
>  .../NonDiscoverablePciDeviceDxe.inf                |   5 +-
>  .../NonDiscoverablePciDeviceIo.c                   | 240 ++++++++++++++++++++-
>  MdeModulePkg/Include/Guid/NonDiscoverableDevice.h  |   3 +
>  .../Library/NonDiscoverableDeviceRegistrationLib.h |   1 +
>  .../NonDiscoverableDeviceRegistrationLib.c         |   3 +
>  .../NonDiscoverableDeviceRegistrationLib.inf       |   1 +
>  MdeModulePkg/MdeModulePkg.dec                      |   1 +
>  OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c       | 143 +++++++++++-
>  OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h       |  21 +-
>  OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf  |   4 +-
>  OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c    | 117 +++++++++-
>  12 files changed, 528 insertions(+), 14 deletions(-)
>
> --
> 2.11.0
>

Re: [PATCH 0/4] Virtio non-discoverable devices

[Laszlo Ersek]

Hi Daniil,

On 03/07/18 02:36, Daniil Egranov wrote:
> This is an attempt to add MMIO Virtio devices into the
> non-discoverable device registration procedure and allow
> Virtio PCI drivers to recognize and program such devices 
> correctly.
> The main issue is that the set of MMIO registers is different
> from PCI, plus the width of similar registers are not 
> always the same. The code implements the translation of 
> the PCI IO registers to MMIO registers. 
> Another difference between PCI and MMIO Virtio devices found 
> during the testing is that MMIO devices may require more 
> registers to be programmed compared to PCI. The VirtioPciDeviceDxe
> was patched to detect non-discoverable MMIO devices and allow
> calling a PCI MemIo protocol function.
> 
> This set of patches was tested with MMIO Virtio Block and
> Virtio Net devices.

I'm commenting on this series because of patch 4/4, which is for OvmfPkg.

OvmfPkg supports the following virtio transports:

- virtio-pci, spec version 0.9.5 ("legacy"):

  OvmfPkg/VirtioPciDeviceDxe consumes EFI_PCI_IO_PROTOCOL,
  and produces VIRTIO_DEVICE_PROTOCOL;

- virtio-pci, spec version 1.0 ("modern"):

  OvmfPkg/Virtio10Dxe consumes EFI_PCI_IO_PROTOCOL and produces
  VIRTIO_DEVICE_PROTOCOL;

- virtio-mmio, spec version 0.9.5 ("legacy"):

  OvmfPkg/Library/VirtioMmioDeviceLib takes (a) an EFI_HANDLE that
  carries a vendor-specific device path protocol instance, (b) the base
  address of the virtio-mmio transport (register block), and produces
  VIRTIO_DEVICE_PROTOCOL.

The individual virtio device drivers under OvmfPkg -- such as
VirtioBlkDxe, VirtioGpuDxe, VirtioNetDxe, VirtioRngDxe and VirtioScsiDxe
-- consume VIRTIO_DEVICE_PROTOCOL, and produce the corresponding (device
type specific) UEFI protocols on top. They perform a *union* of the
steps that are required for generic device configuration over either
virtio transport (going through VIRTIO_DEVICE_PROTOCOL), and the
transport drivers take care of mapping the actions to the actual
transports. In some cases, this means simply ignoring an action (because
it has no mapping defined for the given transport type).

Now, this patch set:

(1) extends NonDiscoverableDeviceRegistrationLib, so that a platform
DXE_DRIVER can register a new kind of non-discoverable device,

(2) extends the NonDiscoverablePciDeviceDxe PCI emulation driver in
MdeModulePkg to recognize the new kind of device.

However: the PciIo protocol instances that are consequently produced
represent virtio devices that do *not* conform to the PCI binding of
either virtio specification (0.9.5 or 1.0). Namely,

- The QueueAlign register (at offset 0x3C in the MMIO register block)
  and the GuestPageSize register (at offset 0x28) are only defined for
  the virtio-mmio binding, spec version 0.9.5 ("legacy").

- The QueueNum register (at offset 0x38) is:

  - defined in the virtio-mmio binding, spec version 0.9.5 ("legacy"),

  - defined in the virtio-mmio binding, spec version 1.0 ("modern"),

  - "sort of defined" in the virtio-pci binding, spec version 1.0
    only ("modern" only). (By "sort of defined", I mean the fact that
    the "queue_size" register of the PCI binding is read-write in spec
    version 1.0.)

Therefore adding any actual handling for these registers to
VirtioPciDeviceDxe, which implements the 0.9.5 PCI binding, is wrong.

If your platform provides a virtio device over an MMIO transport, then
the right way to expose the device to the firmware is *not* to

(a) simulate a PciIo interface that does not conform to the PCI binding
    of the virtio-0.9.5 spec,

(b) then patch that shortcoming up in VirtioPciDeviceDxe, which already
    conforms to the PCI binding of the virtio-0.9.5 spec.

Instead, the right way is to use "OvmfPkg/Library/VirtioMmioDeviceLib"
in a platform driver, for producing VIRTIO_DEVICE_PROTOCOL instances on
top of the virtio-mmio transports (MMIO register blocks) directly.

You must already have a platform DXE_DRIVER that produces the
non-discoverable device protocol instances described in (1). I suggest
that you modify that driver to use VirtioMmioDeviceLib instead:
enumerate the same set of virtio-mmio transports that you must already
be doing, and call VirtioMmioInstallDevice() on each. (You can see an
example in "ArmVirtPkg/VirtioFdtDxe".)

This will give you VIRTIO_DEVICE_PROTOCOL instances right off the bat
that can be driven by VirtioBlkDxe, VirtioGpuDxe, etc.

Thanks,
Laszlo

Re: [PATCH 0/4] Virtio non-discoverable devices

[Laszlo Ersek]

On 03/07/18 09:03, Ard Biesheuvel wrote:
> (+ Laszlo)
> 
> Hello Daniil,
> 
> On 7 March 2018 at 01:36, Daniil Egranov <daniil.egranov@arm.com> wrote:
>> This is an attempt to add MMIO Virtio devices into the
>> non-discoverable device registration procedure and allow
>> Virtio PCI drivers to recognize and program such devices
>> correctly.
> 
> Why? The purpose of the non-discoverable device layer is to make
> non-PCI controllers that can be driven by PCI class drivers appear as
> PCI devices. We have started using the base non-discoverable device
> protocol for other devices as well, but the PCI wrapper is really only
> intended for PCI class drivers.
> 
> For VirtIO MMIO, we already have a driver model driver, and given that
> you need to patch up differences between MMIO and PCI based virtio in
> your code, I am reluctant to incorporate modifications in to the PCI
> driver to support MMIO devices.

I'm impressed and thankful that you managed to say it in two paragraphs
-- I needed ten or more :)

Thanks,
Laszlo

Re: [PATCH 0/4] Virtio non-discoverable devices

[Daniil Egranov]

Hello Ard,

Thanks for reply. Please see my comments below.

On 03/07/2018 02:03 AM, Ard Biesheuvel wrote:
> (+ Laszlo)
>
> Hello Daniil,
>
> On 7 March 2018 at 01:36, Daniil Egranov<daniil.egranov@arm.com>  wrote:
>> This is an attempt to add MMIO Virtio devices into the
>> non-discoverable device registration procedure and allow
>> Virtio PCI drivers to recognize and program such devices
>> correctly.
> Why? The purpose of the non-discoverable device layer is to make
> non-PCI controllers that can be driven by PCI class drivers appear as
> PCI devices. We have started using the base non-discoverable device
> protocol for other devices as well, but the PCI wrapper is really only
> intended for PCI class drivers.

I am looking for a proper way to handle multiple MMIO Virtio devices on a single platform. As both PCI and MMIO types of Virtio device programmed in about the same way, non-discoverable devices approach was looking valid.

I understand you point. Correct me if I am wrong but all non-discoverable devices are MMIO devices so if there is PCI version of the device exists, PCI wrapper can be used. The Virtio PCI class devices are using VirtioPciDeviceDxe driver. Is this driver not fitting to the category of PCI class drivers?


> For VirtIO MMIO, we already have a driver model driver, and given that
> you need to patch up differences between MMIO and PCI based virtio in
> your code, I am reluctant to incorporate modifications in to the PCI
> driver to support MMIO devices.

There is no problem with using this driver model. I was interested to 
check if using unified access for both types of Virtio devices through 
the PCI driver is a right thing.

Regarding MMIO devices support in PCI driver. It's an additional MemIo 
call and it's is part of PC IO protocol interface. Is there a concern to 
call it from the PCI driver? It's up to the protocol provider to 
implement it or keep it empty.

> Is this related to virtio 1.0 support?

No it's 0.9.5.

>
> Also, could you please ensure next time that you cc all the relevant
> people? (Please check the Maintainers file)

Sorry, missed that part.

Thanks,
Daniil

>> The main issue is that the set of MMIO registers is different
>> from PCI, plus the width of similar registers are not
>> always the same. The code implements the translation of
>> the PCI IO registers to MMIO registers.
>> Another difference between PCI and MMIO Virtio devices found
>> during the testing is that MMIO devices may require more
>> registers to be programmed compared to PCI. The VirtioPciDeviceDxe
>> was patched to detect non-discoverable MMIO devices and allow
>> calling a PCI MemIo protocol function.
>>
>> This set of patches was tested with MMIO Virtio Block and
>> Virtio Net devices.
>>
>> Daniil Egranov (4):
>>    MdeModulePkg: Added new Virtio non-discoverable type and GUID
>>    NonDiscoverableDeviceRegistrationLib: Added Virtio support
>>    NonDiscoverablePciDeviceDxe: Added MMIO Virtio support
>>    VirtioPciDeviceDxe: Added non-discoverable Virtio support
>>
>>   .../NonDiscoverablePciDeviceDxe.c                  |   3 +-
>>   .../NonDiscoverablePciDeviceDxe.inf                |   5 +-
>>   .../NonDiscoverablePciDeviceIo.c                   | 240 ++++++++++++++++++++-
>>   MdeModulePkg/Include/Guid/NonDiscoverableDevice.h  |   3 +
>>   .../Library/NonDiscoverableDeviceRegistrationLib.h |   1 +
>>   .../NonDiscoverableDeviceRegistrationLib.c         |   3 +
>>   .../NonDiscoverableDeviceRegistrationLib.inf       |   1 +
>>   MdeModulePkg/MdeModulePkg.dec                      |   1 +
>>   OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.c       | 143 +++++++++++-
>>   OvmfPkg/VirtioPciDeviceDxe/VirtioPciDevice.h       |  21 +-
>>   OvmfPkg/VirtioPciDeviceDxe/VirtioPciDeviceDxe.inf  |   4 +-
>>   OvmfPkg/VirtioPciDeviceDxe/VirtioPciFunctions.c    | 117 +++++++++-
>>   12 files changed, 528 insertions(+), 14 deletions(-)
>>
>> --
>> 2.11.0
>>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/4] Virtio non-discoverable devices

[Ard Biesheuvel]

Hello Daniil,

Could you please use a text based email client? Gmail does not
consider the indentation as threading, so the context below is
unintelligible

On 8 March 2018 at 08:21, Daniil Egranov <daniil.egranov@arm.com> wrote:
> Hello Ard,
>
> Thanks for reply. Please see my comments below.
>
> On 03/07/2018 02:03 AM, Ard Biesheuvel wrote:
>
> (+ Laszlo)
>
> Hello Daniil,
>
> On 7 March 2018 at 01:36, Daniil Egranov <daniil.egranov@arm.com> wrote:
>
> This is an attempt to add MMIO Virtio devices into the
> non-discoverable device registration procedure and allow
> Virtio PCI drivers to recognize and program such devices
> correctly.
>
> Why? The purpose of the non-discoverable device layer is to make
> non-PCI controllers that can be driven by PCI class drivers appear as
> PCI devices. We have started using the base non-discoverable device
> protocol for other devices as well, but the PCI wrapper is really only
> intended for PCI class drivers.
>
>
> I am looking for a proper way to handle multiple MMIO Virtio devices on a
> single platform. As both PCI and MMIO types of Virtio device programmed in
> about the same way, non-discoverable devices approach was looking valid.
>
> I understand you point. Correct me if I am wrong but all non-discoverable
> devices are MMIO devices so if there is PCI version of the device exists,
> PCI wrapper can be used. The Virtio PCI class devices are using
> VirtioPciDeviceDxe driver. Is this driver not fitting to the category of PCI
> class drivers?
>

That is not the point. The Intel guys have decided that the AHCI, XHCI
and other drivers (whose specs do no mandate PCI) are implemented as
PCI drivers only, which means that they are essentially combining two
layers of the driver stack (the PCI part and the _HCI part)

Splitting all of those drivers into PCI drivers that produce _HCI
protocols and _HCI drivers that produce the USB host, SATA host, etc
protocols is not going to be accepted by upstream EDK2, so instead, we
decided to turn the PCI 'emulation' that was duplicated across many
platforms into a proper abstraction.

In the virtio case, we don't have that problem. We have MMIO and PCI
support using drivers that use the proper abstractions, and so
presenting MMIO devices as PCI devices is really a step backwards.

What are you trying to achieve that the current code won't let you?

-- 
Ard.

Re: [PATCH 0/4] Virtio non-discoverable devices

[Daniil Egranov]

Hello Ard,

On 03/08/2018 02:29 AM, Ard Biesheuvel wrote:
> Hello Daniil,
> 
> Could you please use a text based email client? Gmail does not
> consider the indentation as threading, so the context below is
> unintelligible
> 

I forced Thunderbird to plain text. I assumed it should follow a 
previous email composition style. At least i did not have problem with 
it so far. I hope it's formated correctly now.

> On 8 March 2018 at 08:21, Daniil Egranov <daniil.egranov@arm.com> wrote:
>> Hello Ard,
>>
>> Thanks for reply. Please see my comments below.
>>
>> On 03/07/2018 02:03 AM, Ard Biesheuvel wrote:
>>
>> (+ Laszlo)
>>
>> Hello Daniil,
>>
>> On 7 March 2018 at 01:36, Daniil Egranov <daniil.egranov@arm.com> wrote:
>>
>> This is an attempt to add MMIO Virtio devices into the
>> non-discoverable device registration procedure and allow
>> Virtio PCI drivers to recognize and program such devices
>> correctly.
>>
>> Why? The purpose of the non-discoverable device layer is to make
>> non-PCI controllers that can be driven by PCI class drivers appear as
>> PCI devices. We have started using the base non-discoverable device
>> protocol for other devices as well, but the PCI wrapper is really only
>> intended for PCI class drivers.
>>
>>
>> I am looking for a proper way to handle multiple MMIO Virtio devices on a
>> single platform. As both PCI and MMIO types of Virtio device programmed in
>> about the same way, non-discoverable devices approach was looking valid.
>>
>> I understand you point. Correct me if I am wrong but all non-discoverable
>> devices are MMIO devices so if there is PCI version of the device exists,
>> PCI wrapper can be used. The Virtio PCI class devices are using
>> VirtioPciDeviceDxe driver. Is this driver not fitting to the category of PCI
>> class drivers?
>>
> 
> That is not the point. The Intel guys have decided that the AHCI, XHCI
> and other drivers (whose specs do no mandate PCI) are implemented as
> PCI drivers only, which means that they are essentially combining two
> layers of the driver stack (the PCI part and the _HCI part)
> 
> Splitting all of those drivers into PCI drivers that produce _HCI
> protocols and _HCI drivers that produce the USB host, SATA host, etc
> protocols is not going to be accepted by upstream EDK2, so instead, we
> decided to turn the PCI 'emulation' that was duplicated across many
> platforms into a proper abstraction.
> 
> In the virtio case, we don't have that problem. We have MMIO and PCI
> support using drivers that use the proper abstractions, and so
> presenting MMIO devices as PCI devices is really a step backwards.
> 

I see, thanks for details.

> What are you trying to achieve that the current code won't let you?
> 

I have a situation when a platform has multiple Virtio MMIO devices. The 
initial way to handle them was installing a device path protocol and 
calling VirtioMmioDeviceLib for each device as part of the platform DXE 
driver. The non-discoverable way was hiding all these operations and was 
looking like more clean approach.
In case of multiple Virtio MMIO devices, what will be a proper way to 
handle them?

Thanks,
Daniil

Re: [PATCH 0/4] Virtio non-discoverable devices

[Ard Biesheuvel]

On 12 March 2018 at 06:19, Daniil Egranov <daniil.egranov@arm.com> wrote:
> Hello Ard,
>
> On 03/08/2018 02:29 AM, Ard Biesheuvel wrote:
>>
>> Hello Daniil,
>>
>> Could you please use a text based email client? Gmail does not
>> consider the indentation as threading, so the context below is
>> unintelligible
>>
>
> I forced Thunderbird to plain text. I assumed it should follow a previous
> email composition style. At least i did not have problem with it so far. I
> hope it's formated correctly now.
>

Thanks.

>
>> On 8 March 2018 at 08:21, Daniil Egranov <daniil.egranov@arm.com> wrote:
>>>
>>> Hello Ard,
>>>
>>> Thanks for reply. Please see my comments below.
>>>
>>> On 03/07/2018 02:03 AM, Ard Biesheuvel wrote:
>>>
>>> (+ Laszlo)
>>>
>>> Hello Daniil,
>>>
>>> On 7 March 2018 at 01:36, Daniil Egranov <daniil.egranov@arm.com> wrote:
>>>
>>> This is an attempt to add MMIO Virtio devices into the
>>> non-discoverable device registration procedure and allow
>>> Virtio PCI drivers to recognize and program such devices
>>> correctly.
>>>
>>> Why? The purpose of the non-discoverable device layer is to make
>>> non-PCI controllers that can be driven by PCI class drivers appear as
>>> PCI devices. We have started using the base non-discoverable device
>>> protocol for other devices as well, but the PCI wrapper is really only
>>> intended for PCI class drivers.
>>>
>>>
>>> I am looking for a proper way to handle multiple MMIO Virtio devices on a
>>> single platform. As both PCI and MMIO types of Virtio device programmed
>>> in
>>> about the same way, non-discoverable devices approach was looking valid.
>>>
>>> I understand you point. Correct me if I am wrong but all non-discoverable
>>> devices are MMIO devices so if there is PCI version of the device exists,
>>> PCI wrapper can be used. The Virtio PCI class devices are using
>>> VirtioPciDeviceDxe driver. Is this driver not fitting to the category of
>>> PCI
>>> class drivers?
>>>
>>
>> That is not the point. The Intel guys have decided that the AHCI, XHCI
>> and other drivers (whose specs do no mandate PCI) are implemented as
>> PCI drivers only, which means that they are essentially combining two
>> layers of the driver stack (the PCI part and the _HCI part)
>>
>> Splitting all of those drivers into PCI drivers that produce _HCI
>> protocols and _HCI drivers that produce the USB host, SATA host, etc
>> protocols is not going to be accepted by upstream EDK2, so instead, we
>> decided to turn the PCI 'emulation' that was duplicated across many
>> platforms into a proper abstraction.
>>
>> In the virtio case, we don't have that problem. We have MMIO and PCI
>> support using drivers that use the proper abstractions, and so
>> presenting MMIO devices as PCI devices is really a step backwards.
>>
>
> I see, thanks for details.
>
>> What are you trying to achieve that the current code won't let you?
>>
>
> I have a situation when a platform has multiple Virtio MMIO devices. The
> initial way to handle them was installing a device path protocol and calling
> VirtioMmioDeviceLib for each device as part of the platform DXE driver. The
> non-discoverable way was hiding all these operations and was looking like
> more clean approach.

Well, that is only true because you are using
NonDiscoverableDeviceRegistrationLib, which encapsulates those exact
same things.

> In case of multiple Virtio MMIO devices, what will be a proper way to handle
> them?
>

Re: [PATCH 0/4] Virtio non-discoverable devices

[Daniil Egranov]

Hi Laszlo,

Thanks for all the information. Sorry, I missed your reply so i asked 
Ard a question which you already answered. I have a pretty clear picture 
regarding PCI/non-discoverable devices from yours and Ard's replies. 
Thanks a lot.

For the initial MMIO Virtio support implementation I have generally 
followed the steps you described below. The VirtioMmioDeviceLib and 
device path protocol are initialized as part of the platform code.

The idea behind the patches were to abstract all these steps from a 
platform code to EDK2. These steps are common for Virtio MMIO devices so 
each platform that needs it may not need to implement it. I did not know 
all details regarding PCI and non-discoverable devices so this approach 
was looking like a good fit, as it allowed EDK2 code to handle almost 
all parts of the process. I see all the issues now.

Thanks,
Daniil


On 03/07/2018 04:56 AM, Laszlo Ersek wrote:
> Hi Daniil,
> 
> On 03/07/18 02:36, Daniil Egranov wrote:
>> This is an attempt to add MMIO Virtio devices into the
>> non-discoverable device registration procedure and allow
>> Virtio PCI drivers to recognize and program such devices
>> correctly.
>> The main issue is that the set of MMIO registers is different
>> from PCI, plus the width of similar registers are not
>> always the same. The code implements the translation of
>> the PCI IO registers to MMIO registers.
>> Another difference between PCI and MMIO Virtio devices found
>> during the testing is that MMIO devices may require more
>> registers to be programmed compared to PCI. The VirtioPciDeviceDxe
>> was patched to detect non-discoverable MMIO devices and allow
>> calling a PCI MemIo protocol function.
>>
>> This set of patches was tested with MMIO Virtio Block and
>> Virtio Net devices.
> 
> I'm commenting on this series because of patch 4/4, which is for OvmfPkg.
> 
> OvmfPkg supports the following virtio transports:
> 
> - virtio-pci, spec version 0.9.5 ("legacy"):
> 
>    OvmfPkg/VirtioPciDeviceDxe consumes EFI_PCI_IO_PROTOCOL,
>    and produces VIRTIO_DEVICE_PROTOCOL;
> 
> - virtio-pci, spec version 1.0 ("modern"):
> 
>    OvmfPkg/Virtio10Dxe consumes EFI_PCI_IO_PROTOCOL and produces
>    VIRTIO_DEVICE_PROTOCOL;
> 
> - virtio-mmio, spec version 0.9.5 ("legacy"):
> 
>    OvmfPkg/Library/VirtioMmioDeviceLib takes (a) an EFI_HANDLE that
>    carries a vendor-specific device path protocol instance, (b) the base
>    address of the virtio-mmio transport (register block), and produces
>    VIRTIO_DEVICE_PROTOCOL.
> 
> The individual virtio device drivers under OvmfPkg -- such as
> VirtioBlkDxe, VirtioGpuDxe, VirtioNetDxe, VirtioRngDxe and VirtioScsiDxe
> -- consume VIRTIO_DEVICE_PROTOCOL, and produce the corresponding (device
> type specific) UEFI protocols on top. They perform a *union* of the
> steps that are required for generic device configuration over either
> virtio transport (going through VIRTIO_DEVICE_PROTOCOL), and the
> transport drivers take care of mapping the actions to the actual
> transports. In some cases, this means simply ignoring an action (because
> it has no mapping defined for the given transport type).
> 
> Now, this patch set:
> 
> (1) extends NonDiscoverableDeviceRegistrationLib, so that a platform
> DXE_DRIVER can register a new kind of non-discoverable device,
> 
> (2) extends the NonDiscoverablePciDeviceDxe PCI emulation driver in
> MdeModulePkg to recognize the new kind of device.
> 
> However: the PciIo protocol instances that are consequently produced
> represent virtio devices that do *not* conform to the PCI binding of
> either virtio specification (0.9.5 or 1.0). Namely,
> 
> - The QueueAlign register (at offset 0x3C in the MMIO register block)
>    and the GuestPageSize register (at offset 0x28) are only defined for
>    the virtio-mmio binding, spec version 0.9.5 ("legacy").
> 
> - The QueueNum register (at offset 0x38) is:
> 
>    - defined in the virtio-mmio binding, spec version 0.9.5 ("legacy"),
> 
>    - defined in the virtio-mmio binding, spec version 1.0 ("modern"),
> 
>    - "sort of defined" in the virtio-pci binding, spec version 1.0
>      only ("modern" only). (By "sort of defined", I mean the fact that
>      the "queue_size" register of the PCI binding is read-write in spec
>      version 1.0.)
> 
> Therefore adding any actual handling for these registers to
> VirtioPciDeviceDxe, which implements the 0.9.5 PCI binding, is wrong.
> 
> If your platform provides a virtio device over an MMIO transport, then
> the right way to expose the device to the firmware is *not* to
> 
> (a) simulate a PciIo interface that does not conform to the PCI binding
>      of the virtio-0.9.5 spec,
> 
> (b) then patch that shortcoming up in VirtioPciDeviceDxe, which already
>      conforms to the PCI binding of the virtio-0.9.5 spec.
> 
> Instead, the right way is to use "OvmfPkg/Library/VirtioMmioDeviceLib"
> in a platform driver, for producing VIRTIO_DEVICE_PROTOCOL instances on
> top of the virtio-mmio transports (MMIO register blocks) directly.
> 
> You must already have a platform DXE_DRIVER that produces the
> non-discoverable device protocol instances described in (1). I suggest
> that you modify that driver to use VirtioMmioDeviceLib instead:
> enumerate the same set of virtio-mmio transports that you must already
> be doing, and call VirtioMmioInstallDevice() on each. (You can see an
> example in "ArmVirtPkg/VirtioFdtDxe".)
> 
> This will give you VIRTIO_DEVICE_PROTOCOL instances right off the bat
> that can be driven by VirtioBlkDxe, VirtioGpuDxe, etc.
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>