[RFC,00/11] qemu: towards virtio-1 host support

On Fri, 24 Oct 2014 00:42:20 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > This patchset aims to get us some way to implement virtio-1 compliant
> > and transitional devices in qemu. Branch available at
> > 
> > git://github.com/cohuck/qemu virtio-1
> > 
> > I've mainly focused on:
> > - endianness handling
> > - extended feature bits
> > - virtio-ccw new/changed commands
> 
> So issues identified so far:

Thanks for taking a look.

> - devices not converted yet should not advertize 1.0

Neither should an uncoverted transport. So we either can
- have transport set the bit and rely on devices ->get_features
  callback to mask it out
  (virtio-ccw has to change the calling order for get_features, btw.)
- have device set the bit and the transport mask it out later. Feels a
  bit weird, as virtio-1 is a transport feature bit.

I'm tending towards the first option; smth like this (on top of my
branch):



> - blk has some features removed under 1.0,
>   specifically CONFIG_WCE.
> - net header is different for 1.0

On Fri, 24 Oct 2014 10:38:39 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Fri, 24 Oct 2014 00:42:20 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > > This patchset aims to get us some way to implement virtio-1 compliant
> > > and transitional devices in qemu. Branch available at
> > > 
> > > git://github.com/cohuck/qemu virtio-1
> > > 
> > > I've mainly focused on:
> > > - endianness handling
> > > - extended feature bits
> > > - virtio-ccw new/changed commands
> > 
> > So issues identified so far:
> 
> Thanks for taking a look.
> 
> > - devices not converted yet should not advertize 1.0
> 
> Neither should an uncoverted transport. So we either can
> - have transport set the bit and rely on devices ->get_features
>   callback to mask it out
>   (virtio-ccw has to change the calling order for get_features, btw.)
> - have device set the bit and the transport mask it out later. Feels a
>   bit weird, as virtio-1 is a transport feature bit.
> 
> I'm tending towards the first option; smth like this (on top of my
> branch):

(...)

OK, I played around with this patch on top and the vhost-next branch as
guest. It seems to work reasonably well so far: a virtio-blk device
used virtio-1, a virtio-balloon device legacy.

One thing I noticed, though, is that I may need to think about
virtio-ccw revision vs. virtio version again. As a device can refuse
virtio-1 after the driver negotiated revision 1, we're operating a
legacy device with (some) standard ccws. Probably not a big deal, as
(a) both driver and device already have indicated that they support
revision 1 which those ccws are tied to and (b) some legacy
devices/drivers already support standard ccws (adapter interrupt
support). I might want to clarify the standard a bit, let me think
about that over the weekend.

On Fri, Oct 24, 2014 at 02:37:08PM +0200, Cornelia Huck wrote:
> On Fri, 24 Oct 2014 10:38:39 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Fri, 24 Oct 2014 00:42:20 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > > > This patchset aims to get us some way to implement virtio-1 compliant
> > > > and transitional devices in qemu. Branch available at
> > > > 
> > > > git://github.com/cohuck/qemu virtio-1
> > > > 
> > > > I've mainly focused on:
> > > > - endianness handling
> > > > - extended feature bits
> > > > - virtio-ccw new/changed commands
> > > 
> > > So issues identified so far:
> > 
> > Thanks for taking a look.
> > 
> > > - devices not converted yet should not advertize 1.0
> > 
> > Neither should an uncoverted transport. So we either can
> > - have transport set the bit and rely on devices ->get_features
> >   callback to mask it out
> >   (virtio-ccw has to change the calling order for get_features, btw.)
> > - have device set the bit and the transport mask it out later. Feels a
> >   bit weird, as virtio-1 is a transport feature bit.
> > 
> > I'm tending towards the first option; smth like this (on top of my
> > branch):
> 
> (...)
> 
> OK, I played around with this patch on top and the vhost-next branch as
> guest. It seems to work reasonably well so far: a virtio-blk device
> used virtio-1, a virtio-balloon device legacy.
> 
> One thing I noticed, though, is that I may need to think about
> virtio-ccw revision vs. virtio version again. As a device can refuse
> virtio-1 after the driver negotiated revision 1, we're operating a
> legacy device with (some) standard ccws. Probably not a big deal, as
> (a) both driver and device already have indicated that they support
> revision 1 which those ccws are tied to and (b) some legacy
> devices/drivers already support standard ccws (adapter interrupt
> support). I might want to clarify the standard a bit, let me think
> about that over the weekend.


Thanks!
Please note I have updated the branch several times - I'm not going to
send patches until next week.  I updated vhost net by now.
Just for the kernel, host side, we need to update tun, macvtap,
af_packet, vhost scsi and vhost test.
Guest side, we need to keep going over devices and convert them one by
one.
And of course qemu, including support for old and new tun/macvtap.

On Fri, Oct 24, 2014 at 10:38:39AM +0200, Cornelia Huck wrote:
> On Fri, 24 Oct 2014 00:42:20 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > > This patchset aims to get us some way to implement virtio-1 compliant
> > > and transitional devices in qemu. Branch available at
> > > 
> > > git://github.com/cohuck/qemu virtio-1
> > > 
> > > I've mainly focused on:
> > > - endianness handling
> > > - extended feature bits
> > > - virtio-ccw new/changed commands
> > 
> > So issues identified so far:
> 
> Thanks for taking a look.
> 
> > - devices not converted yet should not advertize 1.0
> 
> Neither should an uncoverted transport. So we either can
> - have transport set the bit and rely on devices ->get_features
>   callback to mask it out
>   (virtio-ccw has to change the calling order for get_features, btw.)
> - have device set the bit and the transport mask it out later. Feels a
>   bit weird, as virtio-1 is a transport feature bit.
> 
> I'm tending towards the first option; smth like this (on top of my
> branch):

I would rather *converted* devices had extra code
than unconverted ones.
Reduces the chance we forget one by mistake.


> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index c29c8c8..f6501ea 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -24,6 +24,9 @@
>  static uint32_t virtio_9p_get_features(VirtIODevice *vdev, unsigned int index,
>                                         uint32_t features)
>  {
> +    if (index == 1) {
> +        features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      if (index > 0) {
>          return features;
>      }
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 0d843fe..07a7a6f 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -472,6 +472,9 @@ static uint32_t get_features(VirtIODevice *vdev, unsigned int index,
>  {
>      VirtIOSerial *vser;
>  
> +    if (index == 1) {
> +        features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      if (index > 0) {
>          return features;
>      }
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 67f91c0..4b75105 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -447,6 +447,9 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, unsigned int index,
>      VirtIONet *n = VIRTIO_NET(vdev);
>      NetClientState *nc = qemu_get_queue(n->nic);
>  
> +    if (index == 1 && get_vhost_net(nc->peer)) {
> +        features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      if (index > 0) {
>          return features;
>      }
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 80efe88..07fbf40 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -839,16 +839,16 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
>      dev->revision = -1;
>  
>      /* Set default feature bits that are offered by the host. */
> +    dev->host_features[0] = 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> +    dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
> +
> +    dev->host_features[1] = 1 << (VIRTIO_F_VERSION_1 - 32);
> +
>      for (i = 0; i < ARRAY_SIZE(dev->host_features); i++) {
>          dev->host_features[i] =
>              virtio_bus_get_vdev_features(&dev->bus, i, dev->host_features[i]);
>      }
>  
> -    dev->host_features[0] |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> -    dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
> -
> -    dev->host_features[1] = 1 << (VIRTIO_F_VERSION_1 - 32);
> -
>      css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
>                            parent->hotplugged, 1);
>      return 0;
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 8e1afa0..8a8fdb9 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -155,6 +155,9 @@ static uint32_t vhost_scsi_get_features(VirtIODevice *vdev, unsigned int index,
>  {
>      VHostSCSI *s = VHOST_SCSI(vdev);
>  
> +    if (index == 1) {
> +        features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      if (index > 0) {
>          return features;
>      }
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 088e688..378783f 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -611,6 +611,9 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
>  static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, unsigned int index,
>                                           uint32_t requested_features)
>  {
> +    if (index == 1) {
> +        requested_features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      return requested_features;
>  }
>  
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5af17e2..bd52845 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -306,6 +306,9 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>  static uint32_t virtio_balloon_get_features(VirtIODevice *vdev,
>                                              unsigned int index, uint32_t f)
>  {
> +    if (index == 1) {
> +        f &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      if (index > 0) {
>          return f;
>      }
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 2814017..bf6101d 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -101,6 +101,9 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>  
>  static uint32_t get_features(VirtIODevice *vdev, unsigned int index, uint32_t f)
>  {
> +    if (index == 1) {
> +        f &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      return f;
>  }
>  
> 
> 
> > - blk has some features removed under 1.0,
> >   specifically CONFIG_WCE.
> > - net header is different for 1.0

On Fri, Oct 24, 2014 at 10:38:39AM +0200, Cornelia Huck wrote:
> On Fri, 24 Oct 2014 00:42:20 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > > This patchset aims to get us some way to implement virtio-1 compliant
> > > and transitional devices in qemu. Branch available at
> > > 
> > > git://github.com/cohuck/qemu virtio-1
> > > 
> > > I've mainly focused on:
> > > - endianness handling
> > > - extended feature bits
> > > - virtio-ccw new/changed commands
> > 
> > So issues identified so far:
> 
> Thanks for taking a look.
> 
> > - devices not converted yet should not advertize 1.0
> 
> Neither should an uncoverted transport. So we either can
> - have transport set the bit and rely on devices ->get_features
>   callback to mask it out
>   (virtio-ccw has to change the calling order for get_features, btw.)
> - have device set the bit and the transport mask it out later. Feels a
>   bit weird, as virtio-1 is a transport feature bit.


I thought more about it, I think the right thing
would be for unconverted transports to clear
high bits on ack and get features.

So bit 32 is set, but not exposed to guests.
In fact at least for PCI, we have a 32 bit field for
features in 0.9 so it's automatic.
Didn't check mmio yet.



> I'm tending towards the first option; smth like this (on top of my
> branch):
> 
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index c29c8c8..f6501ea 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -24,6 +24,9 @@
>  static uint32_t virtio_9p_get_features(VirtIODevice *vdev, unsigned int index,
>                                         uint32_t features)
>  {
> +    if (index == 1) {
> +        features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      if (index > 0) {
>          return features;
>      }
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 0d843fe..07a7a6f 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -472,6 +472,9 @@ static uint32_t get_features(VirtIODevice *vdev, unsigned int index,
>  {
>      VirtIOSerial *vser;
>  
> +    if (index == 1) {
> +        features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      if (index > 0) {
>          return features;
>      }
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 67f91c0..4b75105 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -447,6 +447,9 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, unsigned int index,
>      VirtIONet *n = VIRTIO_NET(vdev);
>      NetClientState *nc = qemu_get_queue(n->nic);
>  
> +    if (index == 1 && get_vhost_net(nc->peer)) {
> +        features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      if (index > 0) {
>          return features;
>      }
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 80efe88..07fbf40 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -839,16 +839,16 @@ static int virtio_ccw_device_init(VirtioCcwDevice *dev, VirtIODevice *vdev)
>      dev->revision = -1;
>  
>      /* Set default feature bits that are offered by the host. */
> +    dev->host_features[0] = 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> +    dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
> +
> +    dev->host_features[1] = 1 << (VIRTIO_F_VERSION_1 - 32);
> +
>      for (i = 0; i < ARRAY_SIZE(dev->host_features); i++) {
>          dev->host_features[i] =
>              virtio_bus_get_vdev_features(&dev->bus, i, dev->host_features[i]);
>      }
>  
> -    dev->host_features[0] |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY;
> -    dev->host_features[0] |= 0x1 << VIRTIO_F_BAD_FEATURE;
> -
> -    dev->host_features[1] = 1 << (VIRTIO_F_VERSION_1 - 32);
> -
>      css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
>                            parent->hotplugged, 1);
>      return 0;
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 8e1afa0..8a8fdb9 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -155,6 +155,9 @@ static uint32_t vhost_scsi_get_features(VirtIODevice *vdev, unsigned int index,
>  {
>      VHostSCSI *s = VHOST_SCSI(vdev);
>  
> +    if (index == 1) {
> +        features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      if (index > 0) {
>          return features;
>      }
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index 088e688..378783f 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -611,6 +611,9 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
>  static uint32_t virtio_scsi_get_features(VirtIODevice *vdev, unsigned int index,
>                                           uint32_t requested_features)
>  {
> +    if (index == 1) {
> +        requested_features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      return requested_features;
>  }
>  
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 5af17e2..bd52845 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -306,6 +306,9 @@ static void virtio_balloon_set_config(VirtIODevice *vdev,
>  static uint32_t virtio_balloon_get_features(VirtIODevice *vdev,
>                                              unsigned int index, uint32_t f)
>  {
> +    if (index == 1) {
> +        f &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      if (index > 0) {
>          return f;
>      }
> diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c
> index 2814017..bf6101d 100644
> --- a/hw/virtio/virtio-rng.c
> +++ b/hw/virtio/virtio-rng.c
> @@ -101,6 +101,9 @@ static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
>  
>  static uint32_t get_features(VirtIODevice *vdev, unsigned int index, uint32_t f)
>  {
> +    if (index == 1) {
> +        f &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> +    }
>      return f;
>  }
>  
> 
> 
> > - blk has some features removed under 1.0,
> >   specifically CONFIG_WCE.
> > - net header is different for 1.0

On Tue, 28 Oct 2014 06:43:29 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Oct 24, 2014 at 10:38:39AM +0200, Cornelia Huck wrote:
> > On Fri, 24 Oct 2014 00:42:20 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Tue, Oct 07, 2014 at 04:39:56PM +0200, Cornelia Huck wrote:
> > > > This patchset aims to get us some way to implement virtio-1 compliant
> > > > and transitional devices in qemu. Branch available at
> > > > 
> > > > git://github.com/cohuck/qemu virtio-1
> > > > 
> > > > I've mainly focused on:
> > > > - endianness handling
> > > > - extended feature bits
> > > > - virtio-ccw new/changed commands
> > > 
> > > So issues identified so far:
> > 
> > Thanks for taking a look.
> > 
> > > - devices not converted yet should not advertize 1.0
> > 
> > Neither should an uncoverted transport. So we either can
> > - have transport set the bit and rely on devices ->get_features
> >   callback to mask it out
> >   (virtio-ccw has to change the calling order for get_features, btw.)
> > - have device set the bit and the transport mask it out later. Feels a
> >   bit weird, as virtio-1 is a transport feature bit.
> 
> 
> I thought more about it, I think the right thing
> would be for unconverted transports to clear
> high bits on ack and get features.

This should work out of the box with my patches (virtio-pci and
virtio-mmio return 0 for high feature bits).

> 
> So bit 32 is set, but not exposed to guests.
> In fact at least for PCI, we have a 32 bit field for
> features in 0.9 so it's automatic.
> Didn't check mmio yet.

We still to make sure the bit is not set for unconverted devices,
though. But you're probably right that having the device set the bit is
less error-prone.