Re: [PATCH v3 2/4] OvmfPkg/VirtioMmioDeviceLib: Add virtio 1.0 support.

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

Hi Gerd,

On 8/17/21 11:23 AM, Gerd Hoffmann wrote:
> Add support for virtio 1.0 to the mmio transport.  virtio 0.9.5 uses
> page size, page frame number and a fixed layout for the ring.  virtio
> 1.0 uses the physical addresses for base address, used bits and
> available bits instead.
> 
> The ring layout is not changed, so a 0.9.5 compatible layout is used in
> both 0.9.5 and 1.0 mode to to keep the code differences as small as
> possible.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> ---
>  .../VirtioMmioDeviceLib/VirtioMmioDevice.h    |  4 +++
>  .../VirtioMmioDeviceLib/VirtioMmioDevice.c    | 17 +++++++---
>  .../VirtioMmioDeviceFunctions.c               | 31 +++++++++++++++++--
>  3 files changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> index ab53b90d51c9..0c2f99633c46 100644
> --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.h
> @@ -23,9 +23,13 @@
>  #include <Library/MemoryAllocationLib.h>
>  
>  #define VIRTIO_MMIO_DEVICE_SIGNATURE  SIGNATURE_32 ('V', 'M', 'I', 'O')
> +#define VIRTIO_MMIO_DEVICE_VERSION_0_95  1

After looking at the next patch, I wonder if it wouldn't be simpler
to add/use VIRTIO_MMIO_DEVICE_VERSION_0_95 in a preliminary patch
(2/5), returning EFI_UNSUPPORTED when not v0.95.

Next patch (3/5) add QueueNum for v0.95 (returning EFI_UNSUPPORTED
for != v0.95)

Then finally (4/5) add v1.00.

Current 4/4 becomes 5/5.

> +#define VIRTIO_MMIO_DEVICE_VERSION_1_00  2

So remove from this patch,

>  
>  typedef struct {
>    UINT32                 Signature;
> +  UINT32                 Version;
>    VIRTIO_DEVICE_PROTOCOL VirtioDevice;
>    PHYSICAL_ADDRESS       BaseAddress;
>  } VIRTIO_MMIO_DEVICE;
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
> index 6dbbba008c75..a97ef9352d0f 100644
> --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
> @@ -58,7 +58,6 @@ VirtioMmioInit (
>    )
>  {
>    UINT32     MagicValue;
> -  UINT32     Version;
>  
>    //
>    // Initialize VirtIo Mmio Device
> @@ -66,7 +65,6 @@ VirtioMmioInit (
>    CopyMem (&Device->VirtioDevice, &mMmioDeviceProtocolTemplate,
>          sizeof (VIRTIO_DEVICE_PROTOCOL));
>    Device->BaseAddress = BaseAddress;
> -  Device->VirtioDevice.Revision = VIRTIO_SPEC_REVISION (0, 9, 5);
>    Device->VirtioDevice.SubSystemDeviceId =
>            MmioRead32 (BaseAddress + VIRTIO_MMIO_OFFSET_DEVICE_ID);
>  
> @@ -78,8 +76,19 @@ VirtioMmioInit (
>      return EFI_UNSUPPORTED;
>    }
>  
> -  Version = VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_VERSION);
> -  if (Version != 1) {
> +  Device->Version = VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_VERSION);
> +  switch (Device->Version) {
> +  case VIRTIO_MMIO_DEVICE_VERSION_0_95:
> +    DEBUG ((DEBUG_INFO, "%a virtio 0.9.5, id %d\n", __FUNCTION__,
> +            Device->VirtioDevice.SubSystemDeviceId));
> +    Device->VirtioDevice.Revision = VIRTIO_SPEC_REVISION (0, 9, 5);
> +    break;

Remove from here <--

> +  case VIRTIO_MMIO_DEVICE_VERSION_1_00:
> +    DEBUG ((DEBUG_INFO, "%a virtio 1.0, id %d\n", __FUNCTION__,
> +            Device->VirtioDevice.SubSystemDeviceId));
> +    Device->VirtioDevice.Revision = VIRTIO_SPEC_REVISION (1, 0, 0);
> +    break;

--> to here (moved to 4/5).

> +  default:
>      return EFI_UNSUPPORTED;
>    }
>  
> diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> index b0d75fb1dd24..a0ee8e5f3c86 100644
> --- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> +++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDeviceFunctions.c
> @@ -151,7 +151,9 @@ VirtioMmioSetPageSize (
>  
>    Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
>  
> -  VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_GUEST_PAGE_SIZE, PageSize);
> +  if (Device->Version == VIRTIO_MMIO_DEVICE_VERSION_0_95) {
> +    VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_GUEST_PAGE_SIZE, PageSize);

     else
         EFI_UNSUPPORTED
> +  }
>  
>    return EFI_SUCCESS;
>  }
> @@ -181,13 +183,36 @@ VirtioMmioSetQueueAddress (
>    )
>  {
>    VIRTIO_MMIO_DEVICE *Device;
> +  UINT64 Address;
>  
>    ASSERT (RingBaseShift == 0);
>  
>    Device = VIRTIO_MMIO_DEVICE_FROM_VIRTIO_DEVICE (This);
>  
> -  VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_PFN,
> -    (UINT32)((UINTN)Ring->Base >> EFI_PAGE_SHIFT));
> +  if (Device->Version == VIRTIO_MMIO_DEVICE_VERSION_0_95) {
> +    VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_PFN,
> +      (UINT32)((UINTN)Ring->Base >> EFI_PAGE_SHIFT));
> +  } else {

       EFI_UNSUPPORTED for now,

> +    Address = (UINT64)Ring->Base;
> +    VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_DESC_LO,
> +                      (UINT32)Address);
> +    VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_DESC_HI,
> +                      (UINT32)RShiftU64(Address, 32));
> +
> +    Address = (UINT64)Ring->Avail.Flags;
> +    VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_AVAIL_LO,
> +                      (UINT32)Address);
> +    VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_AVAIL_HI,
> +                      (UINT32)RShiftU64(Address, 32));
> +
> +    Address = (UINT64)Ring->Used.Flags;
> +    VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_USED_LO,
> +                      (UINT32)Address);
> +    VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_USED_HI,
> +                      (UINT32)RShiftU64(Address, 32));
> +
> +    VIRTIO_CFG_WRITE (Device, VIRTIO_MMIO_OFFSET_QUEUE_READY, 1);

^ this moved to 4/5.

> +  }
>  
>    return EFI_SUCCESS;
>  }
>