Re: [Qemu-devel] [PATCH v2] virtio-mmio: implement modern (v2) personality (virtio-1)

[Cornelia Huck <cohuck@redhat.com>]

On Thu, 29 Aug 2019 18:50:27 +0200
Sergio Lopez <slp@redhat.com> wrote:

> Implement the modern (v2) personality, according to the VirtIO 1.0
> specification.
> 
> Support for v2 among guests is not as widespread as it'd be
> desirable. While the Linux driver has had it for a while, support is
> missing, at least, from Tianocore EDK II, NetBSD and FreeBSD.
> 
> For this reason, the v2 personality is disabled, keeping the legacy
> behavior as default. Machine types willing to use v2, can enable it
> using MachineClass's compat_props.
> 
> Signed-off-by: Sergio Lopez <slp@redhat.com>
> ---
> Changelog:
> 
> v2:
>  - Switch from RFC to PATCH.
>  - Avoid the modern vs. legacy dichotomy. Use legacy or non-legacy
>    instead. (Andrea Bolognani, Cornelia Huck)
>  - Include the register offset in the warning messages. (Stefan
>    Hajnoczi)
>  - Fix device endianness for the non-legacy mode. (Michael S. Tsirkin)
>  - Honor the specs in VIRTIO_MMIO_QUEUE_READY. (Michael S. Tsirkin)
> ---
>  hw/virtio/virtio-mmio.c | 296 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 279 insertions(+), 17 deletions(-)
> 

> @@ -146,28 +163,51 @@ static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned size)
>      case VIRTIO_MMIO_MAGIC_VALUE:
>          return VIRT_MAGIC;
>      case VIRTIO_MMIO_VERSION:
> -        return VIRT_VERSION;
> +        if (proxy->legacy) {
> +            return VIRT_VERSION_LEGACY;
> +        } else {
> +            return VIRT_VERSION;
> +        }
>      case VIRTIO_MMIO_DEVICE_ID:
>          return vdev->device_id;
>      case VIRTIO_MMIO_VENDOR_ID:
>          return VIRT_VENDOR;
>      case VIRTIO_MMIO_DEVICE_FEATURES:
> -        if (proxy->host_features_sel) {
> -            return 0;
> -        }
> -        return vdev->host_features;
> +        return vdev->host_features >> (32 * proxy->host_features_sel);

Hm... I think you want to return 0 for host_features_sel > 0 on legacy
devices.

Also, there's VirtIODeviceClass->legacy_features, which probably should
be masked out for non-legacy devices?

(...)

> @@ -229,17 +275,33 @@ static void virtio_mmio_write(void *opaque, hwaddr offset, uint64_t value,
>      }
>      switch (offset) {
>      case VIRTIO_MMIO_DEVICE_FEATURES_SEL:
> -        proxy->host_features_sel = value;
> +        if (value) {
> +            proxy->host_features_sel = 1;
> +        } else {
> +            proxy->host_features_sel = 0;
> +        }
>          break;
>      case VIRTIO_MMIO_DRIVER_FEATURES:
> -        if (!proxy->guest_features_sel) {
> +        if (!proxy->legacy) {
> +            proxy->guest_features[proxy->guest_features_sel] = value;
> +        } else if (!proxy->guest_features_sel) {
>              virtio_set_features(vdev, value);

If the guest tries to set something !0 for guest_features_sel > 0 on a
legacy device, should that be logged as a guest bug?

>          }
>          break;

(...)

Otherwise, looks good to me.