[Qemu-devel] [PATCH 0/4] virtio-1/virtio-ccw related patches

[Qemu-devel] [PATCH 0/4] virtio-1/virtio-ccw related patches

[Cornelia Huck]

Hi,

here's a set of patches for virtio-core and virtio-ccw, fixing a common
code issue and finally enabling virtio-1 for ccw devices.

The first issue was noted by Jason: We need to care about changed
ring sizes. We also want to support different ring sizes in ccw for
virtio-1 devices (existing code did not propagate changes to core).

The final patch enabling virtio-1 support also introduces a new property
that allows to specify a maximum revision. I don't see a need to fence
off legacy; the maximum revision approach instead allows us to easily
introduce new revisions and still cap at a revision for compat machines.

Cornelia Huck (4):
  virtio: ring sizes vs. reset
  virtio-ccw: support ring size changes
  virtio-ccw: feature bits > 31 handling
  virtio-ccw: enable virtio-1

 hw/s390x/s390-virtio-ccw.c |  4 +++
 hw/s390x/virtio-ccw.c      | 64 +++++++++++++++++++++++++++++++++-------------
 hw/s390x/virtio-ccw.h      |  6 +++--
 hw/virtio/virtio.c         | 63 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h |  1 +
 5 files changed, 118 insertions(+), 20 deletions(-)

-- 
2.3.8

[Qemu-devel] [PATCH 1/4] virtio: ring sizes vs. reset

[Cornelia Huck]

We allow guests to change the size of the virtqueue rings by supplying
a number of buffers that is different from the number of buffers the
device was initialized with. Current code has some problems, however,
since reset does not reset the ringsizes to the default values (as this
is not saved anywhere).

Let's extend the core code to keep track of the default ringsizes and
migrate them once the guest changed them for any of the virtqueues
for a device.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/virtio/virtio.c         | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio.h |  1 +
 2 files changed, 64 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 788b556..687116e 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -60,6 +60,7 @@ typedef struct VRingUsed
 typedef struct VRing
 {
     unsigned int num;
+    unsigned int num_default;
     unsigned int align;
     hwaddr desc;
     hwaddr avail;
@@ -633,7 +634,9 @@ void virtio_reset(void *opaque)
         vdev->vq[i].signalled_used = 0;
         vdev->vq[i].signalled_used_valid = false;
         vdev->vq[i].notification = true;
+        vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
     }
+    vdev->non_default_ringsizes = false;
 }
 
 uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
@@ -855,6 +858,10 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
         return;
     }
     vdev->vq[n].vring.num = num;
+    if (num != vdev->vq[n].vring.num_default) {
+        /* save ringsizes once one of them has been changed */
+        vdev->non_default_ringsizes = true;
+    }
 }
 
 VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector)
@@ -964,6 +971,7 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
         abort();
 
     vdev->vq[i].vring.num = queue_size;
+    vdev->vq[i].vring.num_default = queue_size;
     vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
     vdev->vq[i].handle_output = handle_output;
 
@@ -977,6 +985,7 @@ void virtio_del_queue(VirtIODevice *vdev, int n)
     }
 
     vdev->vq[n].vring.num = 0;
+    vdev->vq[n].vring.num_default = 0;
 }
 
 void virtio_irq(VirtQueue *vq)
@@ -1056,6 +1065,13 @@ static bool virtio_virtqueue_needed(void *opaque)
     return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1);
 }
 
+static bool virtio_ringsize_needed(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+
+    return vdev->non_default_ringsizes;
+}
+
 static void put_virtqueue_state(QEMUFile *f, void *pv, size_t size)
 {
     VirtIODevice *vdev = pv;
@@ -1104,6 +1120,52 @@ static const VMStateDescription vmstate_virtio_virtqueues = {
     }
 };
 
+static void put_ringsize_state(QEMUFile *f, void *pv, size_t size)
+{
+    VirtIODevice *vdev = pv;
+    int i;
+
+    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+        qemu_put_be32(f, vdev->vq[i].vring.num_default);
+    }
+}
+
+static int get_ringsize_state(QEMUFile *f, void *pv, size_t size)
+{
+    VirtIODevice *vdev = pv;
+    int i;
+
+    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+        vdev->vq[i].vring.num_default = qemu_get_be32(f);
+    }
+    return 0;
+}
+
+static VMStateInfo vmstate_info_ringsize = {
+    .name = "ringsize_state",
+    .get = get_ringsize_state,
+    .put = put_ringsize_state,
+};
+
+static const VMStateDescription vmstate_virtio_ringsize = {
+    .name = "virtio/ringsize",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = &virtio_ringsize_needed,
+    .fields = (VMStateField[]) {
+        {
+            .name         = "ringsize",
+            .version_id   = 0,
+            .field_exists = NULL,
+            .size         = 0,
+            .info         = &vmstate_info_ringsize,
+            .flags        = VMS_SINGLE,
+            .offset       = 0,
+        },
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio_device_endian = {
     .name = "virtio/device_endian",
     .version_id = 1,
@@ -1138,6 +1200,7 @@ static const VMStateDescription vmstate_virtio = {
         &vmstate_virtio_device_endian,
         &vmstate_virtio_64bit_features,
         &vmstate_virtio_virtqueues,
+        &vmstate_virtio_ringsize,
         NULL
     }
 };
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index cccae89..29870c8 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -90,6 +90,7 @@ struct VirtIODevice
     VMChangeStateEntry *vmstate;
     char *bus_name;
     uint8_t device_endian;
+    bool non_default_ringsizes;
     QLIST_HEAD(, VirtQueue) *vector_queues;
 };
 
-- 
2.3.8

Re: [Qemu-devel] [PATCH 1/4] virtio: ring sizes vs. reset

[Michael S. Tsirkin]

On Fri, Sep 04, 2015 at 10:54:26AM +0200, Cornelia Huck wrote:
> We allow guests to change the size of the virtqueue rings by supplying
> a number of buffers that is different from the number of buffers the
> device was initialized with. Current code has some problems, however,
> since reset does not reset the ringsizes to the default values (as this
> is not saved anywhere).
> 
> Let's extend the core code to keep track of the default ringsizes and
> migrate them once the guest changed them for any of the virtqueues
> for a device.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  hw/virtio/virtio.c         | 63 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio.h |  1 +
>  2 files changed, 64 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 788b556..687116e 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -60,6 +60,7 @@ typedef struct VRingUsed
>  typedef struct VRing
>  {
>      unsigned int num;
> +    unsigned int num_default;
>      unsigned int align;
>      hwaddr desc;
>      hwaddr avail;
> @@ -633,7 +634,9 @@ void virtio_reset(void *opaque)
>          vdev->vq[i].signalled_used = 0;
>          vdev->vq[i].signalled_used_valid = false;
>          vdev->vq[i].notification = true;
> +        vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
>      }
> +    vdev->non_default_ringsizes = false;
>  }
>  
>  uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr)
> @@ -855,6 +858,10 @@ void virtio_queue_set_num(VirtIODevice *vdev, int n, int num)
>          return;
>      }
>      vdev->vq[n].vring.num = num;
> +    if (num != vdev->vq[n].vring.num_default) {
> +        /* save ringsizes once one of them has been changed */
> +        vdev->non_default_ringsizes = true;
> +    }
>  }
>  
>  VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector)
> @@ -964,6 +971,7 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>          abort();
>  
>      vdev->vq[i].vring.num = queue_size;
> +    vdev->vq[i].vring.num_default = queue_size;
>      vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
>      vdev->vq[i].handle_output = handle_output;
>  
> @@ -977,6 +985,7 @@ void virtio_del_queue(VirtIODevice *vdev, int n)
>      }
>  
>      vdev->vq[n].vring.num = 0;
> +    vdev->vq[n].vring.num_default = 0;
>  }
>  
>  void virtio_irq(VirtQueue *vq)
> @@ -1056,6 +1065,13 @@ static bool virtio_virtqueue_needed(void *opaque)
>      return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1);
>  }
>  
> +static bool virtio_ringsize_needed(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +
> +    return vdev->non_default_ringsizes;
> +}
> +
>  static void put_virtqueue_state(QEMUFile *f, void *pv, size_t size)
>  {
>      VirtIODevice *vdev = pv;
> @@ -1104,6 +1120,52 @@ static const VMStateDescription vmstate_virtio_virtqueues = {
>      }
>  };
>  
> +static void put_ringsize_state(QEMUFile *f, void *pv, size_t size)
> +{
> +    VirtIODevice *vdev = pv;
> +    int i;
> +
> +    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +        qemu_put_be32(f, vdev->vq[i].vring.num_default);
> +    }
> +}
> +
> +static int get_ringsize_state(QEMUFile *f, void *pv, size_t size)
> +{
> +    VirtIODevice *vdev = pv;
> +    int i;
> +
> +    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +        vdev->vq[i].vring.num_default = qemu_get_be32(f);
> +    }
> +    return 0;
> +}
> +
> +static VMStateInfo vmstate_info_ringsize = {
> +    .name = "ringsize_state",
> +    .get = get_ringsize_state,
> +    .put = put_ringsize_state,
> +};
> +
> +static const VMStateDescription vmstate_virtio_ringsize = {
> +    .name = "virtio/ringsize",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = &virtio_ringsize_needed,
> +    .fields = (VMStateField[]) {
> +        {
> +            .name         = "ringsize",
> +            .version_id   = 0,
> +            .field_exists = NULL,
> +            .size         = 0,
> +            .info         = &vmstate_info_ringsize,
> +            .flags        = VMS_SINGLE,
> +            .offset       = 0,
> +        },
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_virtio_device_endian = {
>      .name = "virtio/device_endian",
>      .version_id = 1,
> @@ -1138,6 +1200,7 @@ static const VMStateDescription vmstate_virtio = {
>          &vmstate_virtio_device_endian,
>          &vmstate_virtio_64bit_features,
>          &vmstate_virtio_virtqueues,
> +        &vmstate_virtio_ringsize,
>          NULL
>      }
>  };
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index cccae89..29870c8 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -90,6 +90,7 @@ struct VirtIODevice
>      VMChangeStateEntry *vmstate;
>      char *bus_name;
>      uint8_t device_endian;
> +    bool non_default_ringsizes;


Let's not try to track this separately. Just go over
rings before migration, and check whether guest changed
anything.

>      QLIST_HEAD(, VirtQueue) *vector_queues;
>  };
>  
> -- 
> 2.3.8

Re: [Qemu-devel] [PATCH 1/4] virtio: ring sizes vs. reset

[Cornelia Huck]

On Thu, 10 Sep 2015 12:02:44 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Sep 04, 2015 at 10:54:26AM +0200, Cornelia Huck wrote:
> > We allow guests to change the size of the virtqueue rings by supplying
> > a number of buffers that is different from the number of buffers the
> > device was initialized with. Current code has some problems, however,
> > since reset does not reset the ringsizes to the default values (as this
> > is not saved anywhere).
> > 
> > Let's extend the core code to keep track of the default ringsizes and
> > migrate them once the guest changed them for any of the virtqueues
> > for a device.
> > 
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> >  hw/virtio/virtio.c         | 63 ++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/virtio/virtio.h |  1 +
> >  2 files changed, 64 insertions(+)
> > 

> > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > index cccae89..29870c8 100644
> > --- a/include/hw/virtio/virtio.h
> > +++ b/include/hw/virtio/virtio.h
> > @@ -90,6 +90,7 @@ struct VirtIODevice
> >      VMChangeStateEntry *vmstate;
> >      char *bus_name;
> >      uint8_t device_endian;
> > +    bool non_default_ringsizes;
> 
> 
> Let's not try to track this separately. Just go over
> rings before migration, and check whether guest changed
> anything.

I dunno. If we try to figure this out while doing migration, we'll need
to go over the rings and check whether any of them has changed, then go
over the rings again to save the values - while here, we just set this
bool once when the driver changes the value, check once to find out
whether we need to migrate the values and reset once.

> 
> >      QLIST_HEAD(, VirtQueue) *vector_queues;
> >  };
> >  
> > -- 
> > 2.3.8
> 

Re: [Qemu-devel] [PATCH 1/4] virtio: ring sizes vs. reset

[Michael S. Tsirkin]

On Thu, Sep 10, 2015 at 12:22:54PM +0200, Cornelia Huck wrote:
> On Thu, 10 Sep 2015 12:02:44 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Sep 04, 2015 at 10:54:26AM +0200, Cornelia Huck wrote:
> > > We allow guests to change the size of the virtqueue rings by supplying
> > > a number of buffers that is different from the number of buffers the
> > > device was initialized with. Current code has some problems, however,
> > > since reset does not reset the ringsizes to the default values (as this
> > > is not saved anywhere).
> > > 
> > > Let's extend the core code to keep track of the default ringsizes and
> > > migrate them once the guest changed them for any of the virtqueues
> > > for a device.
> > > 
> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > ---
> > >  hw/virtio/virtio.c         | 63 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/virtio/virtio.h |  1 +
> > >  2 files changed, 64 insertions(+)
> > > 
> 
> > > diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> > > index cccae89..29870c8 100644
> > > --- a/include/hw/virtio/virtio.h
> > > +++ b/include/hw/virtio/virtio.h
> > > @@ -90,6 +90,7 @@ struct VirtIODevice
> > >      VMChangeStateEntry *vmstate;
> > >      char *bus_name;
> > >      uint8_t device_endian;
> > > +    bool non_default_ringsizes;
> > 
> > 
> > Let's not try to track this separately. Just go over
> > rings before migration, and check whether guest changed
> > anything.
> 
> I dunno. If we try to figure this out while doing migration, we'll need
> to go over the rings and check whether any of them has changed, then go
> over the rings again to save the values - while here, we just set this
> bool once when the driver changes the value, check once to find out
> whether we need to migrate the values and reset once.

Then restore it on load ...
State is worse than stateless, without a state you are
never out of sync.

> > 
> > >      QLIST_HEAD(, VirtQueue) *vector_queues;
> > >  };
> > >  
> > > -- 
> > > 2.3.8
> > 

[Qemu-devel] [PATCH v2] virtio: ring sizes vs. reset

[Cornelia Huck]

We allow guests to change the size of the virtqueue rings by supplying
a number of buffers that is different from the number of buffers the
device was initialized with. Current code has some problems, however,
since reset does not reset the ringsizes to the default values (as this
is not saved anywhere).

Let's extend the core code to keep track of the default ringsizes and
migrate them once the guest changed them for any of the virtqueues
for a device.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---

v1->v2: lose the changed ringsize tracking, calculate directly before
        migrating

---
 hw/virtio/virtio.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 788b556..458fcbe 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -60,6 +60,7 @@ typedef struct VRingUsed
 typedef struct VRing
 {
     unsigned int num;
+    unsigned int num_default;
     unsigned int align;
     hwaddr desc;
     hwaddr avail;
@@ -633,6 +634,7 @@ void virtio_reset(void *opaque)
         vdev->vq[i].signalled_used = 0;
         vdev->vq[i].signalled_used_valid = false;
         vdev->vq[i].notification = true;
+        vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
     }
 }
 
@@ -964,6 +966,7 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
         abort();
 
     vdev->vq[i].vring.num = queue_size;
+    vdev->vq[i].vring.num_default = queue_size;
     vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
     vdev->vq[i].handle_output = handle_output;
 
@@ -977,6 +980,7 @@ void virtio_del_queue(VirtIODevice *vdev, int n)
     }
 
     vdev->vq[n].vring.num = 0;
+    vdev->vq[n].vring.num_default = 0;
 }
 
 void virtio_irq(VirtQueue *vq)
@@ -1056,6 +1060,19 @@ static bool virtio_virtqueue_needed(void *opaque)
     return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1);
 }
 
+static bool virtio_ringsize_needed(void *opaque)
+{
+    VirtIODevice *vdev = opaque;
+    int i;
+
+    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+        if (vdev->vq[i].vring.num != vdev->vq[i].vring.num_default) {
+            return true;
+        }
+    }
+    return false;
+}
+
 static void put_virtqueue_state(QEMUFile *f, void *pv, size_t size)
 {
     VirtIODevice *vdev = pv;
@@ -1104,6 +1121,52 @@ static const VMStateDescription vmstate_virtio_virtqueues = {
     }
 };
 
+static void put_ringsize_state(QEMUFile *f, void *pv, size_t size)
+{
+    VirtIODevice *vdev = pv;
+    int i;
+
+    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+        qemu_put_be32(f, vdev->vq[i].vring.num_default);
+    }
+}
+
+static int get_ringsize_state(QEMUFile *f, void *pv, size_t size)
+{
+    VirtIODevice *vdev = pv;
+    int i;
+
+    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
+        vdev->vq[i].vring.num_default = qemu_get_be32(f);
+    }
+    return 0;
+}
+
+static VMStateInfo vmstate_info_ringsize = {
+    .name = "ringsize_state",
+    .get = get_ringsize_state,
+    .put = put_ringsize_state,
+};
+
+static const VMStateDescription vmstate_virtio_ringsize = {
+    .name = "virtio/ringsize",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = &virtio_ringsize_needed,
+    .fields = (VMStateField[]) {
+        {
+            .name         = "ringsize",
+            .version_id   = 0,
+            .field_exists = NULL,
+            .size         = 0,
+            .info         = &vmstate_info_ringsize,
+            .flags        = VMS_SINGLE,
+            .offset       = 0,
+        },
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_virtio_device_endian = {
     .name = "virtio/device_endian",
     .version_id = 1,
@@ -1138,6 +1201,7 @@ static const VMStateDescription vmstate_virtio = {
         &vmstate_virtio_device_endian,
         &vmstate_virtio_64bit_features,
         &vmstate_virtio_virtqueues,
+        &vmstate_virtio_ringsize,
         NULL
     }
 };
-- 
2.3.8

Re: [Qemu-devel] [PATCH v2] virtio: ring sizes vs. reset

[Jason Wang]

On 09/10/2015 07:48 PM, Cornelia Huck wrote:
> We allow guests to change the size of the virtqueue rings by supplying
> a number of buffers that is different from the number of buffers the
> device was initialized with. Current code has some problems, however,
> since reset does not reset the ringsizes to the default values (as this
> is not saved anywhere).
>
> Let's extend the core code to keep track of the default ringsizes and
> migrate them once the guest changed them for any of the virtqueues
> for a device.
>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>
> v1->v2: lose the changed ringsize tracking, calculate directly before
>         migrating

Reviewed-by: Jason Wang <jasowang@redhat.com>

>
> ---
>  hw/virtio/virtio.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 788b556..458fcbe 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -60,6 +60,7 @@ typedef struct VRingUsed
>  typedef struct VRing
>  {
>      unsigned int num;
> +    unsigned int num_default;
>      unsigned int align;
>      hwaddr desc;
>      hwaddr avail;
> @@ -633,6 +634,7 @@ void virtio_reset(void *opaque)
>          vdev->vq[i].signalled_used = 0;
>          vdev->vq[i].signalled_used_valid = false;
>          vdev->vq[i].notification = true;
> +        vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
>      }
>  }
>  
> @@ -964,6 +966,7 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int queue_size,
>          abort();
>  
>      vdev->vq[i].vring.num = queue_size;
> +    vdev->vq[i].vring.num_default = queue_size;
>      vdev->vq[i].vring.align = VIRTIO_PCI_VRING_ALIGN;
>      vdev->vq[i].handle_output = handle_output;
>  
> @@ -977,6 +980,7 @@ void virtio_del_queue(VirtIODevice *vdev, int n)
>      }
>  
>      vdev->vq[n].vring.num = 0;
> +    vdev->vq[n].vring.num_default = 0;
>  }
>  
>  void virtio_irq(VirtQueue *vq)
> @@ -1056,6 +1060,19 @@ static bool virtio_virtqueue_needed(void *opaque)
>      return virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1);
>  }
>  
> +static bool virtio_ringsize_needed(void *opaque)
> +{
> +    VirtIODevice *vdev = opaque;
> +    int i;
> +
> +    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +        if (vdev->vq[i].vring.num != vdev->vq[i].vring.num_default) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
> +
>  static void put_virtqueue_state(QEMUFile *f, void *pv, size_t size)
>  {
>      VirtIODevice *vdev = pv;
> @@ -1104,6 +1121,52 @@ static const VMStateDescription vmstate_virtio_virtqueues = {
>      }
>  };
>  
> +static void put_ringsize_state(QEMUFile *f, void *pv, size_t size)
> +{
> +    VirtIODevice *vdev = pv;
> +    int i;
> +
> +    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +        qemu_put_be32(f, vdev->vq[i].vring.num_default);
> +    }
> +}
> +
> +static int get_ringsize_state(QEMUFile *f, void *pv, size_t size)
> +{
> +    VirtIODevice *vdev = pv;
> +    int i;
> +
> +    for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +        vdev->vq[i].vring.num_default = qemu_get_be32(f);
> +    }
> +    return 0;
> +}
> +
> +static VMStateInfo vmstate_info_ringsize = {
> +    .name = "ringsize_state",
> +    .get = get_ringsize_state,
> +    .put = put_ringsize_state,
> +};
> +
> +static const VMStateDescription vmstate_virtio_ringsize = {
> +    .name = "virtio/ringsize",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = &virtio_ringsize_needed,
> +    .fields = (VMStateField[]) {
> +        {
> +            .name         = "ringsize",
> +            .version_id   = 0,
> +            .field_exists = NULL,
> +            .size         = 0,
> +            .info         = &vmstate_info_ringsize,
> +            .flags        = VMS_SINGLE,
> +            .offset       = 0,
> +        },
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_virtio_device_endian = {
>      .name = "virtio/device_endian",
>      .version_id = 1,
> @@ -1138,6 +1201,7 @@ static const VMStateDescription vmstate_virtio = {
>          &vmstate_virtio_device_endian,
>          &vmstate_virtio_64bit_features,
>          &vmstate_virtio_virtqueues,
> +        &vmstate_virtio_ringsize,
>          NULL
>      }
>  };

[Qemu-devel] [PATCH 2/4] virtio-ccw: support ring size changes

[Cornelia Huck]

Wire up changing the ring size for virtio-1 devices.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/virtio-ccw.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index d36373e..85e2a5d 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -307,11 +307,18 @@ static int virtio_ccw_set_vqs(SubchDev *sch, VqInfoBlock *info,
     if (!desc) {
         virtio_queue_set_vector(vdev, index, VIRTIO_NO_VECTOR);
     } else {
-        /* Fail if we don't have a big enough queue. */
-        /* TODO: Add interface to handle vring.num changing */
-        if (virtio_queue_get_num(vdev, index) > num) {
+        if (info) {
+            /* virtio-1 allows changing the ring size. */
+            if (virtio_queue_get_num(vdev, index) < num) {
+                /* Fail if we exceed the maximum number. */
+                return -EINVAL;
+            }
+            virtio_queue_set_num(vdev, index, num);
+        } else if (virtio_queue_get_num(vdev, index) > num) {
+            /* Fail if we don't have a big enough queue. */
             return -EINVAL;
         }
+        /* We ignore possible increased num for legacy for compatibility. */
         virtio_queue_set_vector(vdev, index, index);
     }
     /* tell notify handler in case of config change */
-- 
2.3.8

[Qemu-devel] [PATCH 3/4] virtio-ccw: feature bits > 31 handling

[Cornelia Huck]

We currently switch off the VERSION_1 feature bit if the guest has
not negotiated at least revision 1. As no feature bits beyond 31 are
valid however unless VERSION_1 has been negotiated, make sure that
legacy guests never see a feature bit beyond 31.

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/virtio-ccw.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 85e2a5d..eed7b3e 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -468,15 +468,12 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                                                 NULL);
             if (features.index == 0) {
                 features.features = (uint32_t)vdev->host_features;
-            } else if (features.index == 1) {
-                features.features = (uint32_t)(vdev->host_features >> 32);
+            } else if ((features.index == 1) && (dev->revision >= 1)) {
                 /*
-                 * Don't offer version 1 to the guest if it did not
-                 * negotiate at least revision 1.
+                 * Only offer feature bits beyond 31 if the guest has
+                 * negotiated at least revision 1.
                  */
-                if (dev->revision <= 0) {
-                    features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
-                }
+                features.features = (uint32_t)(vdev->host_features >> 32);
             } else {
                 /* Return zeroes if the guest supports more feature bits. */
                 features.features = 0;
@@ -515,14 +512,12 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                 virtio_set_features(vdev,
                                     (vdev->guest_features & 0xffffffff00000000ULL) |
                                     features.features);
-            } else if (features.index == 1) {
+            } else if ((features.index == 1) && (dev->revision >= 1)) {
                 /*
-                 * The guest should not set version 1 if it didn't
-                 * negotiate a revision >= 1.
+                 * If the guest did not negotiate at least revision 1,
+                 * we did not offer it any feature bits beyond 31. Such a
+                 * guest passing us any bit here is therefore buggy.
                  */
-                if (dev->revision <= 0) {
-                    features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
-                }
                 virtio_set_features(vdev,
                                     (vdev->guest_features & 0x00000000ffffffffULL) |
                                     ((uint64_t)features.features << 32));
-- 
2.3.8

Re: [Qemu-devel] [PATCH 3/4] virtio-ccw: feature bits > 31 handling

[David Hildenbrand]

> We currently switch off the VERSION_1 feature bit if the guest has
> not negotiated at least revision 1. As no feature bits beyond 31 are
> valid however unless VERSION_1 has been negotiated, make sure that
> legacy guests never see a feature bit beyond 31.
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  hw/s390x/virtio-ccw.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 85e2a5d..eed7b3e 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -468,15 +468,12 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>                                                  NULL);
>              if (features.index == 0) {
>                  features.features = (uint32_t)vdev->host_features;
> -            } else if (features.index == 1) {
> -                features.features = (uint32_t)(vdev->host_features >> 32);
> +            } else if ((features.index == 1) && (dev->revision >= 1)) {
>                  /*
> -                 * Don't offer version 1 to the guest if it did not
> -                 * negotiate at least revision 1.
> +                 * Only offer feature bits beyond 31 if the guest has
> +                 * negotiated at least revision 1.
>                   */
> -                if (dev->revision <= 0) {
> -                    features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> -                }
> +                features.features = (uint32_t)(vdev->host_features >> 32);
>              } else {
>                  /* Return zeroes if the guest supports more feature bits. */
>                  features.features = 0;
> @@ -515,14 +512,12 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>                  virtio_set_features(vdev,
>                                      (vdev->guest_features & 0xffffffff00000000ULL) |
>                                      features.features);
> -            } else if (features.index == 1) {
> +            } else if ((features.index == 1) && (dev->revision >= 1)) {
>                  /*
> -                 * The guest should not set version 1 if it didn't
> -                 * negotiate a revision >= 1.
> +                 * If the guest did not negotiate at least revision 1,
> +                 * we did not offer it any feature bits beyond 31. Such a
> +                 * guest passing us any bit here is therefore buggy.
>                   */
> -                if (dev->revision <= 0) {
> -                    features.features &= ~(1 << (VIRTIO_F_VERSION_1 - 32));
> -                }
>                  virtio_set_features(vdev,
>                                      (vdev->guest_features & 0x00000000ffffffffULL) |
>                                      ((uint64_t)features.features << 32));

Looks sane to me!

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>

David

[Qemu-devel] [PATCH 4/4] virtio-ccw: enable virtio-1

[Cornelia Huck]

Let's enable revision 1 for virtio-ccw devices. We can always offer
VERSION_1 as drivers in legacy mode won't be able to see it anyway.

We have to introduce a way to set a lower maximum revision for a device
to accommodate the following cases:
- compat machines (to enforce legacy only)
- virtio-blk with scsi support (version 1 + scsi is fenced by common
  code, with a user-configured max revision of 0 we can allow scsi
  via not offering VERSION_1)

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c |  4 ++++
 hw/s390x/virtio-ccw.c      | 30 ++++++++++++++++++++++++++++--
 hw/s390x/virtio-ccw.h      |  6 ++++--
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e2a26e9..fb6e45c 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -287,6 +287,10 @@ static const TypeInfo ccw_machine_info = {
             .driver   = TYPE_S390_SKEYS,\
             .property = "migration-enabled",\
             .value    = "off",\
+        },{\
+            .driver = TYPE_VIRTIO_CCW_DEVICE,\
+            .property = "max_revision",\
+            .value = 0,\
         },
 
 static void ccw_machine_2_4_class_init(ObjectClass *oc, void *data)
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index eed7b3e..fb103b7 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -467,7 +467,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
                                                 MEMTXATTRS_UNSPECIFIED,
                                                 NULL);
             if (features.index == 0) {
-                features.features = (uint32_t)vdev->host_features;
+                if (dev->revision >= 1) {
+                    /* Don't offer legacy features for modern devices. */
+                    features.features = (uint32_t)
+                        (vdev->host_features & ~VIRTIO_LEGACY_FEATURES);
+                } else {
+                    features.features = (uint32_t)vdev->host_features;
+                }
             } else if ((features.index == 1) && (dev->revision >= 1)) {
                 /*
                  * Only offer feature bits beyond 31 if the guest has
@@ -768,7 +774,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
          * need to fetch it here. Nothing to do for now, though.
          */
         if (dev->revision >= 0 ||
-            revinfo.revision > virtio_ccw_rev_max(vdev)) {
+            revinfo.revision > virtio_ccw_rev_max(dev)) {
             ret = -ENOSYS;
             break;
         }
@@ -1541,6 +1547,10 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
 
     sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
 
+    if (dev->max_rev >= 1) {
+        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
+    }
+
     css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
                           d->hotplugged, 1);
 }
@@ -1557,6 +1567,8 @@ static Property virtio_ccw_net_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+                       VIRTIO_CCW_MAX_REV),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1584,6 +1596,8 @@ static Property virtio_ccw_blk_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+                       VIRTIO_CCW_MAX_REV),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1611,6 +1625,8 @@ static Property virtio_ccw_serial_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+                       VIRTIO_CCW_MAX_REV),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1638,6 +1654,8 @@ static Property virtio_ccw_balloon_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+                       VIRTIO_CCW_MAX_REV),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1665,6 +1683,8 @@ static Property virtio_ccw_scsi_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+                       VIRTIO_CCW_MAX_REV),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1691,6 +1711,8 @@ static const TypeInfo virtio_ccw_scsi = {
 #ifdef CONFIG_VHOST_SCSI
 static Property vhost_ccw_scsi_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
+    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+                       VIRTIO_CCW_MAX_REV),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1729,6 +1751,8 @@ static Property virtio_ccw_rng_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+                       VIRTIO_CCW_MAX_REV),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1882,6 +1906,8 @@ static Property virtio_ccw_9p_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
             VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
+    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
+                       VIRTIO_CCW_MAX_REV),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
index 692ddd7..7ab8367 100644
--- a/hw/s390x/virtio-ccw.h
+++ b/hw/s390x/virtio-ccw.h
@@ -88,6 +88,7 @@ struct VirtioCcwDevice {
     SubchDev *sch;
     char *bus_id;
     int revision;
+    uint32_t max_rev;
     VirtioBusState bus;
     bool ioeventfd_started;
     bool ioeventfd_disabled;
@@ -102,9 +103,10 @@ struct VirtioCcwDevice {
 };
 
 /* The maximum virtio revision we support. */
-static inline int virtio_ccw_rev_max(VirtIODevice *vdev)
+#define VIRTIO_CCW_MAX_REV 1
+static inline int virtio_ccw_rev_max(VirtioCcwDevice *dev)
 {
-    return 0;
+    return dev->max_rev;
 }
 
 /* virtual css bus type */
-- 
2.3.8

Re: [Qemu-devel] [PATCH 4/4] virtio-ccw: enable virtio-1

[Michael S. Tsirkin]

On Fri, Sep 04, 2015 at 10:54:29AM +0200, Cornelia Huck wrote:
> Let's enable revision 1 for virtio-ccw devices. We can always offer
> VERSION_1 as drivers in legacy mode won't be able to see it anyway.
> 
> We have to introduce a way to set a lower maximum revision for a device
> to accommodate the following cases:
> - compat machines (to enforce legacy only)

But you don't actually set this for any compat machines.
If you don't, this seems a bit pointless.

> - virtio-blk with scsi support (version 1 + scsi is fenced by common
>   code, with a user-configured max revision of 0 we can allow scsi
>   via not offering VERSION_1)

For this use, for pci users need to do disable_modern=true.
I find it unfortunate that for ccw one needs to do max_revision=0.

Revision numbers generally are a ccw specific concept. I'm not sure it
is wise to expose it to users.

> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c |  4 ++++
>  hw/s390x/virtio-ccw.c      | 30 ++++++++++++++++++++++++++++--
>  hw/s390x/virtio-ccw.h      |  6 ++++--
>  3 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e2a26e9..fb6e45c 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -287,6 +287,10 @@ static const TypeInfo ccw_machine_info = {
>              .driver   = TYPE_S390_SKEYS,\
>              .property = "migration-enabled",\
>              .value    = "off",\
> +        },{\
> +            .driver = TYPE_VIRTIO_CCW_DEVICE,\
> +            .property = "max_revision",\
> +            .value = 0,\
>          },
>  
>  static void ccw_machine_2_4_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index eed7b3e..fb103b7 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -467,7 +467,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>                                                  MEMTXATTRS_UNSPECIFIED,
>                                                  NULL);
>              if (features.index == 0) {
> -                features.features = (uint32_t)vdev->host_features;
> +                if (dev->revision >= 1) {
> +                    /* Don't offer legacy features for modern devices. */
> +                    features.features = (uint32_t)
> +                        (vdev->host_features & ~VIRTIO_LEGACY_FEATURES);
> +                } else {
> +                    features.features = (uint32_t)vdev->host_features;
> +                }
>              } else if ((features.index == 1) && (dev->revision >= 1)) {
>                  /*
>                   * Only offer feature bits beyond 31 if the guest has
> @@ -768,7 +774,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
>           * need to fetch it here. Nothing to do for now, though.
>           */
>          if (dev->revision >= 0 ||
> -            revinfo.revision > virtio_ccw_rev_max(vdev)) {
> +            revinfo.revision > virtio_ccw_rev_max(dev)) {
>              ret = -ENOSYS;
>              break;
>          }
> @@ -1541,6 +1547,10 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
>  
>      sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
>  
> +    if (dev->max_rev >= 1) {
> +        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> +    }
> +
>      css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
>                            d->hotplugged, 1);
>  }
> @@ -1557,6 +1567,8 @@ static Property virtio_ccw_net_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1584,6 +1596,8 @@ static Property virtio_ccw_blk_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1611,6 +1625,8 @@ static Property virtio_ccw_serial_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1638,6 +1654,8 @@ static Property virtio_ccw_balloon_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1665,6 +1683,8 @@ static Property virtio_ccw_scsi_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1691,6 +1711,8 @@ static const TypeInfo virtio_ccw_scsi = {
>  #ifdef CONFIG_VHOST_SCSI
>  static Property vhost_ccw_scsi_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1729,6 +1751,8 @@ static Property virtio_ccw_rng_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> @@ -1882,6 +1906,8 @@ static Property virtio_ccw_9p_properties[] = {
>      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
>      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
>              VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> +                       VIRTIO_CCW_MAX_REV),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> index 692ddd7..7ab8367 100644
> --- a/hw/s390x/virtio-ccw.h
> +++ b/hw/s390x/virtio-ccw.h
> @@ -88,6 +88,7 @@ struct VirtioCcwDevice {
>      SubchDev *sch;
>      char *bus_id;
>      int revision;
> +    uint32_t max_rev;
>      VirtioBusState bus;
>      bool ioeventfd_started;
>      bool ioeventfd_disabled;
> @@ -102,9 +103,10 @@ struct VirtioCcwDevice {
>  };
>  
>  /* The maximum virtio revision we support. */
> -static inline int virtio_ccw_rev_max(VirtIODevice *vdev)
> +#define VIRTIO_CCW_MAX_REV 1
> +static inline int virtio_ccw_rev_max(VirtioCcwDevice *dev)
>  {
> -    return 0;
> +    return dev->max_rev;
>  }
>  
>  /* virtual css bus type */
> -- 
> 2.3.8

Re: [Qemu-devel] [PATCH 4/4] virtio-ccw: enable virtio-1

[Cornelia Huck]

On Thu, 10 Sep 2015 12:07:18 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Sep 04, 2015 at 10:54:29AM +0200, Cornelia Huck wrote:
> > Let's enable revision 1 for virtio-ccw devices. We can always offer
> > VERSION_1 as drivers in legacy mode won't be able to see it anyway.
> > 
> > We have to introduce a way to set a lower maximum revision for a device
> > to accommodate the following cases:
> > - compat machines (to enforce legacy only)
> 
> But you don't actually set this for any compat machines.
> If you don't, this seems a bit pointless.

Huh? The first hunk of this patch does this.

> 
> > - virtio-blk with scsi support (version 1 + scsi is fenced by common
> >   code, with a user-configured max revision of 0 we can allow scsi
> >   via not offering VERSION_1)
> 
> For this use, for pci users need to do disable_modern=true.
> I find it unfortunate that for ccw one needs to do max_revision=0.

I don't like the pci concept: much too coarse-grained and not very
future proof.

> 
> Revision numbers generally are a ccw specific concept. I'm not sure it
> is wise to expose it to users.

What is wrong about exposing transport-specific concepts?

> 
> > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > ---
> >  hw/s390x/s390-virtio-ccw.c |  4 ++++
> >  hw/s390x/virtio-ccw.c      | 30 ++++++++++++++++++++++++++++--
> >  hw/s390x/virtio-ccw.h      |  6 ++++--
> >  3 files changed, 36 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > index e2a26e9..fb6e45c 100644
> > --- a/hw/s390x/s390-virtio-ccw.c
> > +++ b/hw/s390x/s390-virtio-ccw.c
> > @@ -287,6 +287,10 @@ static const TypeInfo ccw_machine_info = {
> >              .driver   = TYPE_S390_SKEYS,\
> >              .property = "migration-enabled",\
> >              .value    = "off",\
> > +        },{\
> > +            .driver = TYPE_VIRTIO_CCW_DEVICE,\
> > +            .property = "max_revision",\
> > +            .value = 0,\
> >          },
> >  
> >  static void ccw_machine_2_4_class_init(ObjectClass *oc, void *data)
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index eed7b3e..fb103b7 100644
> > --- a/hw/s390x/virtio-ccw.c
> > +++ b/hw/s390x/virtio-ccw.c
> > @@ -467,7 +467,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >                                                  MEMTXATTRS_UNSPECIFIED,
> >                                                  NULL);
> >              if (features.index == 0) {
> > -                features.features = (uint32_t)vdev->host_features;
> > +                if (dev->revision >= 1) {
> > +                    /* Don't offer legacy features for modern devices. */
> > +                    features.features = (uint32_t)
> > +                        (vdev->host_features & ~VIRTIO_LEGACY_FEATURES);
> > +                } else {
> > +                    features.features = (uint32_t)vdev->host_features;
> > +                }
> >              } else if ((features.index == 1) && (dev->revision >= 1)) {
> >                  /*
> >                   * Only offer feature bits beyond 31 if the guest has
> > @@ -768,7 +774,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> >           * need to fetch it here. Nothing to do for now, though.
> >           */
> >          if (dev->revision >= 0 ||
> > -            revinfo.revision > virtio_ccw_rev_max(vdev)) {
> > +            revinfo.revision > virtio_ccw_rev_max(dev)) {
> >              ret = -ENOSYS;
> >              break;
> >          }
> > @@ -1541,6 +1547,10 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
> >  
> >      sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
> >  
> > +    if (dev->max_rev >= 1) {
> > +        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> > +    }
> > +
> >      css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
> >                            d->hotplugged, 1);
> >  }
> > @@ -1557,6 +1567,8 @@ static Property virtio_ccw_net_properties[] = {
> >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > +                       VIRTIO_CCW_MAX_REV),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -1584,6 +1596,8 @@ static Property virtio_ccw_blk_properties[] = {
> >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > +                       VIRTIO_CCW_MAX_REV),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -1611,6 +1625,8 @@ static Property virtio_ccw_serial_properties[] = {
> >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > +                       VIRTIO_CCW_MAX_REV),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -1638,6 +1654,8 @@ static Property virtio_ccw_balloon_properties[] = {
> >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > +                       VIRTIO_CCW_MAX_REV),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -1665,6 +1683,8 @@ static Property virtio_ccw_scsi_properties[] = {
> >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > +                       VIRTIO_CCW_MAX_REV),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -1691,6 +1711,8 @@ static const TypeInfo virtio_ccw_scsi = {
> >  #ifdef CONFIG_VHOST_SCSI
> >  static Property vhost_ccw_scsi_properties[] = {
> >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > +                       VIRTIO_CCW_MAX_REV),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -1729,6 +1751,8 @@ static Property virtio_ccw_rng_properties[] = {
> >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > +                       VIRTIO_CCW_MAX_REV),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > @@ -1882,6 +1906,8 @@ static Property virtio_ccw_9p_properties[] = {
> >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> >              VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > +                       VIRTIO_CCW_MAX_REV),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >  
> > diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> > index 692ddd7..7ab8367 100644
> > --- a/hw/s390x/virtio-ccw.h
> > +++ b/hw/s390x/virtio-ccw.h
> > @@ -88,6 +88,7 @@ struct VirtioCcwDevice {
> >      SubchDev *sch;
> >      char *bus_id;
> >      int revision;
> > +    uint32_t max_rev;
> >      VirtioBusState bus;
> >      bool ioeventfd_started;
> >      bool ioeventfd_disabled;
> > @@ -102,9 +103,10 @@ struct VirtioCcwDevice {
> >  };
> >  
> >  /* The maximum virtio revision we support. */
> > -static inline int virtio_ccw_rev_max(VirtIODevice *vdev)
> > +#define VIRTIO_CCW_MAX_REV 1
> > +static inline int virtio_ccw_rev_max(VirtioCcwDevice *dev)
> >  {
> > -    return 0;
> > +    return dev->max_rev;
> >  }
> >  
> >  /* virtual css bus type */
> > -- 
> > 2.3.8
> 

Re: [Qemu-devel] [PATCH 4/4] virtio-ccw: enable virtio-1

[Michael S. Tsirkin]

On Thu, Sep 10, 2015 at 11:11:20AM +0200, Cornelia Huck wrote:
> On Thu, 10 Sep 2015 12:07:18 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Fri, Sep 04, 2015 at 10:54:29AM +0200, Cornelia Huck wrote:
> > > Let's enable revision 1 for virtio-ccw devices. We can always offer
> > > VERSION_1 as drivers in legacy mode won't be able to see it anyway.
> > > 
> > > We have to introduce a way to set a lower maximum revision for a device
> > > to accommodate the following cases:
> > > - compat machines (to enforce legacy only)
> > 
> > But you don't actually set this for any compat machines.
> > If you don't, this seems a bit pointless.
> 
> Huh? The first hunk of this patch does this.

Sure. Donnu how I could miss it, sorry.

> > 
> > > - virtio-blk with scsi support (version 1 + scsi is fenced by common
> > >   code, with a user-configured max revision of 0 we can allow scsi
> > >   via not offering VERSION_1)
> > 
> > For this use, for pci users need to do disable_modern=true.
> > I find it unfortunate that for ccw one needs to do max_revision=0.
> 
> I don't like the pci concept: much too coarse-grained and not very
> future proof.
> 
> > 
> > Revision numbers generally are a ccw specific concept. I'm not sure it
> > is wise to expose it to users.
> 
> What is wrong about exposing transport-specific concepts?

Nothing. Go ahead and expose as much of the low level as
makes sense.

But it would be nice if there was also a portable way for people
that just want "virtio" and don't care about the low level details
of which transport it is.

OTOH a conservative estimate of the # of people that will want
to play with this is pretty close to 0, so maybe it does not matter
much.

> > 
> > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > ---
> > >  hw/s390x/s390-virtio-ccw.c |  4 ++++
> > >  hw/s390x/virtio-ccw.c      | 30 ++++++++++++++++++++++++++++--
> > >  hw/s390x/virtio-ccw.h      |  6 ++++--
> > >  3 files changed, 36 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > > index e2a26e9..fb6e45c 100644
> > > --- a/hw/s390x/s390-virtio-ccw.c
> > > +++ b/hw/s390x/s390-virtio-ccw.c
> > > @@ -287,6 +287,10 @@ static const TypeInfo ccw_machine_info = {
> > >              .driver   = TYPE_S390_SKEYS,\
> > >              .property = "migration-enabled",\
> > >              .value    = "off",\
> > > +        },{\
> > > +            .driver = TYPE_VIRTIO_CCW_DEVICE,\
> > > +            .property = "max_revision",\
> > > +            .value = 0,\
> > >          },
> > >  
> > >  static void ccw_machine_2_4_class_init(ObjectClass *oc, void *data)
> > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > > index eed7b3e..fb103b7 100644
> > > --- a/hw/s390x/virtio-ccw.c
> > > +++ b/hw/s390x/virtio-ccw.c
> > > @@ -467,7 +467,13 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> > >                                                  MEMTXATTRS_UNSPECIFIED,
> > >                                                  NULL);
> > >              if (features.index == 0) {
> > > -                features.features = (uint32_t)vdev->host_features;
> > > +                if (dev->revision >= 1) {
> > > +                    /* Don't offer legacy features for modern devices. */
> > > +                    features.features = (uint32_t)
> > > +                        (vdev->host_features & ~VIRTIO_LEGACY_FEATURES);
> > > +                } else {
> > > +                    features.features = (uint32_t)vdev->host_features;
> > > +                }
> > >              } else if ((features.index == 1) && (dev->revision >= 1)) {
> > >                  /*
> > >                   * Only offer feature bits beyond 31 if the guest has
> > > @@ -768,7 +774,7 @@ static int virtio_ccw_cb(SubchDev *sch, CCW1 ccw)
> > >           * need to fetch it here. Nothing to do for now, though.
> > >           */
> > >          if (dev->revision >= 0 ||
> > > -            revinfo.revision > virtio_ccw_rev_max(vdev)) {
> > > +            revinfo.revision > virtio_ccw_rev_max(dev)) {
> > >              ret = -ENOSYS;
> > >              break;
> > >          }
> > > @@ -1541,6 +1547,10 @@ static void virtio_ccw_device_plugged(DeviceState *d, Error **errp)
> > >  
> > >      sch->id.cu_model = virtio_bus_get_vdev_id(&dev->bus);
> > >  
> > > +    if (dev->max_rev >= 1) {
> > > +        virtio_add_feature(&vdev->host_features, VIRTIO_F_VERSION_1);
> > > +    }
> > > +
> > >      css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
> > >                            d->hotplugged, 1);
> > >  }
> > > @@ -1557,6 +1567,8 @@ static Property virtio_ccw_net_properties[] = {
> > >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> > >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> > >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > > +                       VIRTIO_CCW_MAX_REV),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > > @@ -1584,6 +1596,8 @@ static Property virtio_ccw_blk_properties[] = {
> > >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> > >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> > >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > > +                       VIRTIO_CCW_MAX_REV),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > > @@ -1611,6 +1625,8 @@ static Property virtio_ccw_serial_properties[] = {
> > >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> > >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> > >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > > +                       VIRTIO_CCW_MAX_REV),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > > @@ -1638,6 +1654,8 @@ static Property virtio_ccw_balloon_properties[] = {
> > >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> > >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> > >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > > +                       VIRTIO_CCW_MAX_REV),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > > @@ -1665,6 +1683,8 @@ static Property virtio_ccw_scsi_properties[] = {
> > >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> > >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> > >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > > +                       VIRTIO_CCW_MAX_REV),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > > @@ -1691,6 +1711,8 @@ static const TypeInfo virtio_ccw_scsi = {
> > >  #ifdef CONFIG_VHOST_SCSI
> > >  static Property vhost_ccw_scsi_properties[] = {
> > >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> > > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > > +                       VIRTIO_CCW_MAX_REV),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > > @@ -1729,6 +1751,8 @@ static Property virtio_ccw_rng_properties[] = {
> > >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> > >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> > >                      VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > > +                       VIRTIO_CCW_MAX_REV),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > > @@ -1882,6 +1906,8 @@ static Property virtio_ccw_9p_properties[] = {
> > >      DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
> > >      DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
> > >              VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
> > > +    DEFINE_PROP_UINT32("max_revision", VirtioCcwDevice, max_rev,
> > > +                       VIRTIO_CCW_MAX_REV),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >  
> > > diff --git a/hw/s390x/virtio-ccw.h b/hw/s390x/virtio-ccw.h
> > > index 692ddd7..7ab8367 100644
> > > --- a/hw/s390x/virtio-ccw.h
> > > +++ b/hw/s390x/virtio-ccw.h
> > > @@ -88,6 +88,7 @@ struct VirtioCcwDevice {
> > >      SubchDev *sch;
> > >      char *bus_id;
> > >      int revision;
> > > +    uint32_t max_rev;
> > >      VirtioBusState bus;
> > >      bool ioeventfd_started;
> > >      bool ioeventfd_disabled;
> > > @@ -102,9 +103,10 @@ struct VirtioCcwDevice {
> > >  };
> > >  
> > >  /* The maximum virtio revision we support. */
> > > -static inline int virtio_ccw_rev_max(VirtIODevice *vdev)
> > > +#define VIRTIO_CCW_MAX_REV 1
> > > +static inline int virtio_ccw_rev_max(VirtioCcwDevice *dev)
> > >  {
> > > -    return 0;
> > > +    return dev->max_rev;
> > >  }
> > >  
> > >  /* virtual css bus type */
> > > -- 
> > > 2.3.8
> > 

Re: [Qemu-devel] [PATCH 4/4] virtio-ccw: enable virtio-1

[Cornelia Huck]

On Thu, 10 Sep 2015 12:22:42 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Sep 10, 2015 at 11:11:20AM +0200, Cornelia Huck wrote:
> > On Thu, 10 Sep 2015 12:07:18 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Fri, Sep 04, 2015 at 10:54:29AM +0200, Cornelia Huck wrote:
> > > > Let's enable revision 1 for virtio-ccw devices. We can always offer
> > > > VERSION_1 as drivers in legacy mode won't be able to see it anyway.
> > > > 
> > > > We have to introduce a way to set a lower maximum revision for a device
> > > > to accommodate the following cases:
> > > > - compat machines (to enforce legacy only)
> > > 
> > > But you don't actually set this for any compat machines.
> > > If you don't, this seems a bit pointless.
> > 
> > Huh? The first hunk of this patch does this.
> 
> Sure. Donnu how I could miss it, sorry.
> 
> > > 
> > > > - virtio-blk with scsi support (version 1 + scsi is fenced by common
> > > >   code, with a user-configured max revision of 0 we can allow scsi
> > > >   via not offering VERSION_1)
> > > 
> > > For this use, for pci users need to do disable_modern=true.
> > > I find it unfortunate that for ccw one needs to do max_revision=0.
> > 
> > I don't like the pci concept: much too coarse-grained and not very
> > future proof.
> > 
> > > 
> > > Revision numbers generally are a ccw specific concept. I'm not sure it
> > > is wise to expose it to users.
> > 
> > What is wrong about exposing transport-specific concepts?
> 
> Nothing. Go ahead and expose as much of the low level as
> makes sense.
> 
> But it would be nice if there was also a portable way for people
> that just want "virtio" and don't care about the low level details
> of which transport it is.

Proxy devices already look different depending on what transport you
use, so I don't think it really matters.

(And for most users, I'd expect they let a management layer take care
of it anyway.)

> 
> OTOH a conservative estimate of the # of people that will want
> to play with this is pretty close to 0, so maybe it does not matter
> much.

The scsi vs. virtio-1 on ccw handling? Not very many (if anyone), yes.

> 
> > > 
> > > > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> > > > ---
> > > >  hw/s390x/s390-virtio-ccw.c |  4 ++++
> > > >  hw/s390x/virtio-ccw.c      | 30 ++++++++++++++++++++++++++++--
> > > >  hw/s390x/virtio-ccw.h      |  6 ++++--
> > > >  3 files changed, 36 insertions(+), 4 deletions(-)

Re: [Qemu-devel] [PATCH 4/4] virtio-ccw: enable virtio-1

[Cornelia Huck]

On Fri,  4 Sep 2015 10:54:29 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> Let's enable revision 1 for virtio-ccw devices. We can always offer
> VERSION_1 as drivers in legacy mode won't be able to see it anyway.
> 
> We have to introduce a way to set a lower maximum revision for a device
> to accommodate the following cases:
> - compat machines (to enforce legacy only)
> - virtio-blk with scsi support (version 1 + scsi is fenced by common
>   code, with a user-configured max revision of 0 we can allow scsi
>   via not offering VERSION_1)
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c |  4 ++++
>  hw/s390x/virtio-ccw.c      | 30 ++++++++++++++++++++++++++++--
>  hw/s390x/virtio-ccw.h      |  6 ++++--
>  3 files changed, 36 insertions(+), 4 deletions(-)

Scratch this; stale patch with broken compat handling.

I'll send a v2 of the whole series.