[Qemu-devel] [PATCH 0/4] virtio: move common virtio properties to bus class device

[Qemu-devel] [PATCH 0/4] virtio: move common virtio properties to bus class device

[Ming Lei]

Hi,

This patchset is suggested by Paolo, and takes the approach in below
patch for virtio-mmio device:

	http://marc.info/?t=140184945400001&r=1&w=2

Then we can move the virtio common properties into bus class device, and
make code clean.

Thanks,
--
Ming Lei

[Qemu-devel] [PATCH 1/4] virtio-pci: move common virtio properties to virtio-pci class device

[Ming Lei]

The two common virtio features can be defined per bus, so move all
into virtio-pci class device to make code more clean.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 hw/virtio/virtio-pci.c          |   12 ++++++------
 include/hw/virtio/virtio-net.h  |    1 -
 include/hw/virtio/virtio-scsi.h |    1 -
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index b437f19..af2e1c3 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -917,7 +917,6 @@ static Property virtio_9p_pci_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_VIRTIO_9P_PROPERTIES(V9fsPCIState, vdev.fsconf),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -1038,11 +1037,17 @@ static void virtio_pci_reset(DeviceState *qdev)
     proxy->flags &= ~VIRTIO_PCI_FLAG_BUS_MASTER_BUG;
 }
 
+static Property virtio_pci_properties[] = {
+    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void virtio_pci_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
+    dc->props = virtio_pci_properties;
     k->init = virtio_pci_init;
     k->exit = virtio_pci_exit;
     k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
@@ -1071,7 +1076,6 @@ static Property virtio_blk_pci_properties[] = {
     DEFINE_PROP_BIT("x-data-plane", VirtIOBlkPCI, blk.data_plane, 0, false),
     DEFINE_PROP_UINT32("num_queues", VirtIOBlkPCI, blk.num_queues, 1),
 #endif
-    DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkPCI, blk),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -1195,7 +1199,6 @@ static const TypeInfo virtio_scsi_pci_info = {
 static Property vhost_scsi_pci_properties[] = {
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
                        DEV_NVECTORS_UNSPECIFIED),
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_VHOST_SCSI_PROPERTIES(VHostSCSIPCI, vdev.parent_obj.conf),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -1276,7 +1279,6 @@ static void balloon_pci_stats_set_poll_interval(Object *obj, struct Visitor *v,
 }
 
 static Property virtio_balloon_pci_properties[] = {
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -1379,7 +1381,6 @@ static Property virtio_serial_pci_properties[] = {
                     VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
     DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_VIRTIO_SERIAL_PROPERTIES(VirtIOSerialPCI, vdev.serial),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -1475,7 +1476,6 @@ static const TypeInfo virtio_net_pci_info = {
 /* virtio-rng-pci */
 
 static Property virtio_rng_pci_properties[] = {
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_VIRTIO_RNG_PROPERTIES(VirtIORngPCI, vdev.conf),
     DEFINE_PROP_END_OF_LIST(),
 };
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index df60f16..bf65b26 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -242,7 +242,6 @@ struct virtio_net_ctrl_mq {
  #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET        0
 
 #define DEFINE_VIRTIO_NET_FEATURES(_state, _field) \
-        DEFINE_VIRTIO_COMMON_FEATURES(_state, _field), \
         DEFINE_PROP_BIT("any_layout", _state, _field, VIRTIO_F_ANY_LAYOUT, true), \
         DEFINE_PROP_BIT("csum", _state, _field, VIRTIO_NET_F_CSUM, true), \
         DEFINE_PROP_BIT("guest_csum", _state, _field, VIRTIO_NET_F_GUEST_CSUM, true), \
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 42b1024..367afc6 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -180,7 +180,6 @@ typedef struct {
     DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128)
 
 #define DEFINE_VIRTIO_SCSI_FEATURES(_state, _feature_field)                    \
-    DEFINE_VIRTIO_COMMON_FEATURES(_state, _feature_field),                     \
     DEFINE_PROP_BIT("hotplug", _state, _feature_field, VIRTIO_SCSI_F_HOTPLUG,  \
                                                        true),                  \
     DEFINE_PROP_BIT("param_change", _state, _feature_field,                    \
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Ming Lei]

The two common virtio features can be defined per bus, so move all
into virtio-s390 class device to make code more clean.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 hw/s390x/s390-virtio-bus.c |   15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 9c71afa..ab9758e 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = {
     .class_init    = s390_virtio_net_class_init,
 };
 
-static Property s390_virtio_blk_properties[] = {
-    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
 {
-    DeviceClass *dc = DEVICE_CLASS(klass);
     VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
 
     k->init = s390_virtio_blk_init;
-    dc->props = s390_virtio_blk_properties;
 }
 
 static const TypeInfo s390_virtio_blk = {
@@ -571,7 +564,6 @@ static const TypeInfo s390_virtio_serial = {
 };
 
 static Property s390_virtio_rng_properties[] = {
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
     DEFINE_VIRTIO_RNG_PROPERTIES(VirtIORNGS390, vdev.conf),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -610,10 +602,16 @@ static void s390_virtio_busdev_reset(DeviceState *dev)
     virtio_reset(_dev->vdev);
 }
 
+static Property virtio_s390_properties[] = {
+    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void virtio_s390_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->props = virtio_s390_properties;
     dc->init = s390_virtio_busdev_init;
     dc->bus_type = TYPE_S390_VIRTIO_BUS;
     dc->unplug = qdev_simple_unplug_cb;
@@ -654,7 +652,6 @@ static const TypeInfo s390_virtio_scsi = {
 
 #ifdef CONFIG_VHOST_SCSI
 static Property s390_vhost_scsi_properties[] = {
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
     DEFINE_VHOST_SCSI_PROPERTIES(VHostSCSIS390, vdev.parent_obj.conf),
     DEFINE_PROP_END_OF_LIST(),
 };
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Cornelia Huck]

On Mon, 16 Jun 2014 23:40:50 +0800
Ming Lei <ming.lei@canonical.com> wrote:

> The two common virtio features can be defined per bus, so move all
> into virtio-s390 class device to make code more clean.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  hw/s390x/s390-virtio-bus.c |   15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)

This one breaks for me:

qemu-system-s390x: -device virtio-blk-s390,scsi=off,config-wce=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1: Property '.scsi' not found

> 
> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> index 9c71afa..ab9758e 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = {
>      .class_init    = s390_virtio_net_class_init,
>  };
> 
> -static Property s390_virtio_blk_properties[] = {
> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>  static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
>  {
> -    DeviceClass *dc = DEVICE_CLASS(klass);
>      VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
> 
>      k->init = s390_virtio_blk_init;
> -    dc->props = s390_virtio_blk_properties;

...which is probably because you removed the block properties here.

>  }
> 
>  static const TypeInfo s390_virtio_blk = {
> @@ -571,7 +564,6 @@ static const TypeInfo s390_virtio_serial = {
>  };
> 
>  static Property s390_virtio_rng_properties[] = {
> -    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
>      DEFINE_VIRTIO_RNG_PROPERTIES(VirtIORNGS390, vdev.conf),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> @@ -610,10 +602,16 @@ static void s390_virtio_busdev_reset(DeviceState *dev)
>      virtio_reset(_dev->vdev);
>  }
> 
> +static Property virtio_s390_properties[] = {
> +    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void virtio_s390_device_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> 
> +    dc->props = virtio_s390_properties;
>      dc->init = s390_virtio_busdev_init;
>      dc->bus_type = TYPE_S390_VIRTIO_BUS;
>      dc->unplug = qdev_simple_unplug_cb;
> @@ -654,7 +652,6 @@ static const TypeInfo s390_virtio_scsi = {
> 
>  #ifdef CONFIG_VHOST_SCSI
>  static Property s390_vhost_scsi_properties[] = {
> -    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
>      DEFINE_VHOST_SCSI_PROPERTIES(VHostSCSIS390, vdev.parent_obj.conf),
>      DEFINE_PROP_END_OF_LIST(),
>  };

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Ming Lei]

Hi Cornelia,

Thanks for your test.

On Tue, Jun 17, 2014 at 12:04 AM, Cornelia Huck
<cornelia.huck@de.ibm.com> wrote:
> On Mon, 16 Jun 2014 23:40:50 +0800
> Ming Lei <ming.lei@canonical.com> wrote:
>
>> The two common virtio features can be defined per bus, so move all
>> into virtio-s390 class device to make code more clean.
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  hw/s390x/s390-virtio-bus.c |   15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> This one breaks for me:
>
> qemu-system-s390x: -device virtio-blk-s390,scsi=off,config-wce=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1: Property '.scsi' not found
>
>>
>> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
>> index 9c71afa..ab9758e 100644
>> --- a/hw/s390x/s390-virtio-bus.c
>> +++ b/hw/s390x/s390-virtio-bus.c
>> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = {
>>      .class_init    = s390_virtio_net_class_init,
>>  };
>>
>> -static Property s390_virtio_blk_properties[] = {
>> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
>> -    DEFINE_PROP_END_OF_LIST(),
>> -};
>> -
>>  static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
>>  {
>> -    DeviceClass *dc = DEVICE_CLASS(klass);
>>      VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
>>
>>      k->init = s390_virtio_blk_init;
>> -    dc->props = s390_virtio_blk_properties;
>
> ...which is probably because you removed the block properties here.

That looks a bit weird because qemu still continues to add parent's
properties if the device hasn't define property.

I will investigate the issue further tomorrow.

thanks,
--
Ming Lei

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Ming Lei]

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

On Tue, Jun 17, 2014 at 12:04 AM, Cornelia Huck
<cornelia.huck@de.ibm.com> wrote:
> On Mon, 16 Jun 2014 23:40:50 +0800
> Ming Lei <ming.lei@canonical.com> wrote:
>
>> The two common virtio features can be defined per bus, so move all
>> into virtio-s390 class device to make code more clean.
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  hw/s390x/s390-virtio-bus.c |   15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> This one breaks for me:
>
> qemu-system-s390x: -device virtio-blk-s390,scsi=off,config-wce=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1: Property '.scsi' not found
>
>>
>> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
>> index 9c71afa..ab9758e 100644
>> --- a/hw/s390x/s390-virtio-bus.c
>> +++ b/hw/s390x/s390-virtio-bus.c
>> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = {
>>      .class_init    = s390_virtio_net_class_init,
>>  };
>>
>> -static Property s390_virtio_blk_properties[] = {
>> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
>> -    DEFINE_PROP_END_OF_LIST(),
>> -};
>> -
>>  static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
>>  {
>> -    DeviceClass *dc = DEVICE_CLASS(klass);
>>      VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
>>
>>      k->init = s390_virtio_blk_init;
>> -    dc->props = s390_virtio_blk_properties;
>
> ...which is probably because you removed the block properties here.

You are right, DEFINE_VIRTIO_BLK_PROPERTIES() should have been
kept.

Could you test attached patch?

Thanks,
--
Ming Lei

[-- Attachment #2: 0002-s390-virtio-bus-move-common-virtio-properties-to-vir.patch --]
[-- Type: text/x-patch, Size: 1905 bytes --]

From 8b58db209e5703583f9d8133e32c69a03ecca6ec Mon Sep 17 00:00:00 2001
From: Ming Lei <ming.lei@canonical.com>
Date: Mon, 16 Jun 2014 23:23:28 +0800
Subject: [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio
 s390 device class

The two common virtio features can be defined per bus, so move all
into virtio-s390 class device to make code more clean.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 hw/s390x/s390-virtio-bus.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 9c71afa..34f6de8 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -571,7 +571,6 @@ static const TypeInfo s390_virtio_serial = {
 };
 
 static Property s390_virtio_rng_properties[] = {
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
     DEFINE_VIRTIO_RNG_PROPERTIES(VirtIORNGS390, vdev.conf),
     DEFINE_PROP_END_OF_LIST(),
 };
@@ -610,10 +609,16 @@ static void s390_virtio_busdev_reset(DeviceState *dev)
     virtio_reset(_dev->vdev);
 }
 
+static Property virtio_s390_properties[] = {
+    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void virtio_s390_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->props = virtio_s390_properties;
     dc->init = s390_virtio_busdev_init;
     dc->bus_type = TYPE_S390_VIRTIO_BUS;
     dc->unplug = qdev_simple_unplug_cb;
@@ -654,7 +659,6 @@ static const TypeInfo s390_virtio_scsi = {
 
 #ifdef CONFIG_VHOST_SCSI
 static Property s390_vhost_scsi_properties[] = {
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
     DEFINE_VHOST_SCSI_PROPERTIES(VHostSCSIS390, vdev.parent_obj.conf),
     DEFINE_PROP_END_OF_LIST(),
 };
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Cornelia Huck]

On Tue, 17 Jun 2014 10:44:11 +0800
Ming Lei <ming.lei@canonical.com> wrote:

> On Tue, Jun 17, 2014 at 12:04 AM, Cornelia Huck
> <cornelia.huck@de.ibm.com> wrote:
> > On Mon, 16 Jun 2014 23:40:50 +0800
> > Ming Lei <ming.lei@canonical.com> wrote:
> >
> >> The two common virtio features can be defined per bus, so move all
> >> into virtio-s390 class device to make code more clean.
> >>
> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> >> ---
> >>  hw/s390x/s390-virtio-bus.c |   15 ++++++---------
> >>  1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > This one breaks for me:
> >
> > qemu-system-s390x: -device virtio-blk-s390,scsi=off,config-wce=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1: Property '.scsi' not found
> >
> >>
> >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> >> index 9c71afa..ab9758e 100644
> >> --- a/hw/s390x/s390-virtio-bus.c
> >> +++ b/hw/s390x/s390-virtio-bus.c
> >> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = {
> >>      .class_init    = s390_virtio_net_class_init,
> >>  };
> >>
> >> -static Property s390_virtio_blk_properties[] = {
> >> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
> >> -    DEFINE_PROP_END_OF_LIST(),
> >> -};
> >> -
> >>  static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
> >>  {
> >> -    DeviceClass *dc = DEVICE_CLASS(klass);
> >>      VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
> >>
> >>      k->init = s390_virtio_blk_init;
> >> -    dc->props = s390_virtio_blk_properties;
> >
> > ...which is probably because you removed the block properties here.
> 
> You are right, DEFINE_VIRTIO_BLK_PROPERTIES() should have been
> kept.
> 
> Could you test attached patch?
> 
Hm, with the attached patch qemu starts, but the guest will not come up
(same commandline + kernel comes up fine on master). Let me dig around
a bit.

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Cornelia Huck]

On Tue, 17 Jun 2014 09:07:41 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Tue, 17 Jun 2014 10:44:11 +0800
> Ming Lei <ming.lei@canonical.com> wrote:
> 
> > On Tue, Jun 17, 2014 at 12:04 AM, Cornelia Huck
> > <cornelia.huck@de.ibm.com> wrote:
> > > On Mon, 16 Jun 2014 23:40:50 +0800
> > > Ming Lei <ming.lei@canonical.com> wrote:
> > >
> > >> The two common virtio features can be defined per bus, so move all
> > >> into virtio-s390 class device to make code more clean.
> > >>
> > >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> > >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> > >> ---
> > >>  hw/s390x/s390-virtio-bus.c |   15 ++++++---------
> > >>  1 file changed, 6 insertions(+), 9 deletions(-)
> > >
> > > This one breaks for me:
> > >
> > > qemu-system-s390x: -device virtio-blk-s390,scsi=off,config-wce=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1: Property '.scsi' not found
> > >
> > >>
> > >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> > >> index 9c71afa..ab9758e 100644
> > >> --- a/hw/s390x/s390-virtio-bus.c
> > >> +++ b/hw/s390x/s390-virtio-bus.c
> > >> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = {
> > >>      .class_init    = s390_virtio_net_class_init,
> > >>  };
> > >>
> > >> -static Property s390_virtio_blk_properties[] = {
> > >> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
> > >> -    DEFINE_PROP_END_OF_LIST(),
> > >> -};
> > >> -
> > >>  static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
> > >>  {
> > >> -    DeviceClass *dc = DEVICE_CLASS(klass);
> > >>      VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
> > >>
> > >>      k->init = s390_virtio_blk_init;
> > >> -    dc->props = s390_virtio_blk_properties;
> > >
> > > ...which is probably because you removed the block properties here.
> > 
> > You are right, DEFINE_VIRTIO_BLK_PROPERTIES() should have been
> > kept.
> > 
> > Could you test attached patch?
> > 
> Hm, with the attached patch qemu starts, but the guest will not come up
> (same commandline + kernel comes up fine on master). Let me dig around
> a bit.

And the winner is: event_idx

s390-virtio wants event_idx=false or we can't get I/O through. I guess
we can't use DEFINE_VIRTIO_COMMON_FEATURES for this transport.

(Probably nobody tested virtio-rng and vhost-scsi on this transport,
either :)

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Ming Lei]

On Tue, Jun 17, 2014 at 3:07 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Tue, 17 Jun 2014 10:44:11 +0800
> Ming Lei <ming.lei@canonical.com> wrote:
>
>> On Tue, Jun 17, 2014 at 12:04 AM, Cornelia Huck
>> <cornelia.huck@de.ibm.com> wrote:
>> > On Mon, 16 Jun 2014 23:40:50 +0800
>> > Ming Lei <ming.lei@canonical.com> wrote:
>> >
>> >> The two common virtio features can be defined per bus, so move all
>> >> into virtio-s390 class device to make code more clean.
>> >>
>> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> >> ---
>> >>  hw/s390x/s390-virtio-bus.c |   15 ++++++---------
>> >>  1 file changed, 6 insertions(+), 9 deletions(-)
>> >
>> > This one breaks for me:
>> >
>> > qemu-system-s390x: -device virtio-blk-s390,scsi=off,config-wce=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1: Property '.scsi' not found
>> >
>> >>
>> >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
>> >> index 9c71afa..ab9758e 100644
>> >> --- a/hw/s390x/s390-virtio-bus.c
>> >> +++ b/hw/s390x/s390-virtio-bus.c
>> >> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = {
>> >>      .class_init    = s390_virtio_net_class_init,
>> >>  };
>> >>
>> >> -static Property s390_virtio_blk_properties[] = {
>> >> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
>> >> -    DEFINE_PROP_END_OF_LIST(),
>> >> -};
>> >> -
>> >>  static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
>> >>  {
>> >> -    DeviceClass *dc = DEVICE_CLASS(klass);
>> >>      VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
>> >>
>> >>      k->init = s390_virtio_blk_init;
>> >> -    dc->props = s390_virtio_blk_properties;
>> >
>> > ...which is probably because you removed the block properties here.
>>
>> You are right, DEFINE_VIRTIO_BLK_PROPERTIES() should have been
>> kept.
>>
>> Could you test attached patch?
>>
> Hm, with the attached patch qemu starts, but the guest will not come up
> (same commandline + kernel comes up fine on master). Let me dig around
> a bit.

Looks like s390 virtio-blk never enables the two common features, is
there any reason the two features can't be supported by s390?

Thanks,
--
Ming Lei

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Cornelia Huck]

On Tue, 17 Jun 2014 15:44:28 +0800
Ming Lei <ming.lei@canonical.com> wrote:

> On Tue, Jun 17, 2014 at 3:07 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > On Tue, 17 Jun 2014 10:44:11 +0800
> > Ming Lei <ming.lei@canonical.com> wrote:
> >
> >> On Tue, Jun 17, 2014 at 12:04 AM, Cornelia Huck
> >> <cornelia.huck@de.ibm.com> wrote:
> >> > On Mon, 16 Jun 2014 23:40:50 +0800
> >> > Ming Lei <ming.lei@canonical.com> wrote:
> >> >
> >> >> The two common virtio features can be defined per bus, so move all
> >> >> into virtio-s390 class device to make code more clean.
> >> >>
> >> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> >> >> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> >> >> ---
> >> >>  hw/s390x/s390-virtio-bus.c |   15 ++++++---------
> >> >>  1 file changed, 6 insertions(+), 9 deletions(-)
> >> >
> >> > This one breaks for me:
> >> >
> >> > qemu-system-s390x: -device virtio-blk-s390,scsi=off,config-wce=off,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1: Property '.scsi' not found
> >> >
> >> >>
> >> >> diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
> >> >> index 9c71afa..ab9758e 100644
> >> >> --- a/hw/s390x/s390-virtio-bus.c
> >> >> +++ b/hw/s390x/s390-virtio-bus.c
> >> >> @@ -526,18 +526,11 @@ static const TypeInfo s390_virtio_net = {
> >> >>      .class_init    = s390_virtio_net_class_init,
> >> >>  };
> >> >>
> >> >> -static Property s390_virtio_blk_properties[] = {
> >> >> -    DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkS390, blk),
> >> >> -    DEFINE_PROP_END_OF_LIST(),
> >> >> -};
> >> >> -
> >> >>  static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
> >> >>  {
> >> >> -    DeviceClass *dc = DEVICE_CLASS(klass);
> >> >>      VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
> >> >>
> >> >>      k->init = s390_virtio_blk_init;
> >> >> -    dc->props = s390_virtio_blk_properties;
> >> >
> >> > ...which is probably because you removed the block properties here.
> >>
> >> You are right, DEFINE_VIRTIO_BLK_PROPERTIES() should have been
> >> kept.
> >>
> >> Could you test attached patch?
> >>
> > Hm, with the attached patch qemu starts, but the guest will not come up
> > (same commandline + kernel comes up fine on master). Let me dig around
> > a bit.
> 
> Looks like s390 virtio-blk never enables the two common features, is
> there any reason the two features can't be supported by s390?

Indirect descriptors are fine. event_idx will not work IIUC because we
always need to do a sync before we see changes, and this needs an
interrupt to trigger.

The correct way, of course, is to unset VIRTIO_RING_F_EVENT_IDX in the
guest driver, and I verified that this works - this does not help with
old guests, however.

Note that this applies only to the old s390-virtio transport -
virtio-ccw is fine.

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Ming Lei]

On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>
>> Looks like s390 virtio-blk never enables the two common features, is
>> there any reason the two features can't be supported by s390?
>
> Indirect descriptors are fine. event_idx will not work IIUC because we
> always need to do a sync before we see changes, and this needs an
> interrupt to trigger.

Sounds like the old s390 isn't cache coherent? Because you mean
write in one side can only be observed from another side with an
explicit notification or interrupt.

On arm/arm64, we didn't see any problem with event_idx.

>
> The correct way, of course, is to unset VIRTIO_RING_F_EVENT_IDX in the
> guest driver, and I verified that this works - this does not help with
> old guests, however.

Both sides should be ok since it is decided by negotiation.

>
> Note that this applies only to the old s390-virtio transport -
> virtio-ccw is fine.

Could you share me how s390 virtio is supported in kernel since
I only see virtio-mmio and virtio-pci support under drivers/virtio/ of
linux kernel?

Thanks,
--
Ming Lei

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Cornelia Huck]

On Tue, 17 Jun 2014 18:15:39 +0800
Ming Lei <ming.lei@canonical.com> wrote:

> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >>
> >> Looks like s390 virtio-blk never enables the two common features, is
> >> there any reason the two features can't be supported by s390?
> >
> > Indirect descriptors are fine. event_idx will not work IIUC because we
> > always need to do a sync before we see changes, and this needs an
> > interrupt to trigger.
> 
> Sounds like the old s390 isn't cache coherent? Because you mean
> write in one side can only be observed from another side with an
> explicit notification or interrupt.
> 
> On arm/arm64, we didn't see any problem with event_idx.

But you probably have the queues in guest memory, as on other
transports (including virtio-ccw)? The old s390-virtio transport keeps
the devices and their virtqueues in a memory area behind the guest
memory - the guest does not see that memory directly, but a sync has to
be performed to see virtqueue movement (see s390_virtio_device_sync()).

> 
> >
> > The correct way, of course, is to unset VIRTIO_RING_F_EVENT_IDX in the
> > guest driver, and I verified that this works - this does not help with
> > old guests, however.
> 
> Both sides should be ok since it is decided by negotiation.

Yes, but the guest driver currently does not drop the feature bit, as
it only asks about the features that the common code supports - and
that does include VIRTIO_RING_F_EVENT_IDX.

> 
> >
> > Note that this applies only to the old s390-virtio transport -
> > virtio-ccw is fine.
> 
> Could you share me how s390 virtio is supported in kernel since
> I only see virtio-mmio and virtio-pci support under drivers/virtio/ of
> linux kernel?

They're hiding under drivers/s390/kvm/. The old s390-virtio transport is
supported by kvm_virtio.c, the virtio-ccw transport by virtio_ccw.c.

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Ming Lei]

On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Tue, 17 Jun 2014 18:15:39 +0800
> Ming Lei <ming.lei@canonical.com> wrote:
>
>> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>> >>
>> >> Looks like s390 virtio-blk never enables the two common features, is
>> >> there any reason the two features can't be supported by s390?
>> >
>> > Indirect descriptors are fine. event_idx will not work IIUC because we
>> > always need to do a sync before we see changes, and this needs an
>> > interrupt to trigger.
>>
>> Sounds like the old s390 isn't cache coherent? Because you mean
>> write in one side can only be observed from another side with an
>> explicit notification or interrupt.
>>
>> On arm/arm64, we didn't see any problem with event_idx.
>
> But you probably have the queues in guest memory, as on other
> transports (including virtio-ccw)? The old s390-virtio transport keeps
> the devices and their virtqueues in a memory area behind the guest
> memory - the guest does not see that memory directly, but a sync has to
> be performed to see virtqueue movement (see s390_virtio_device_sync()).

OK, it looks like a real physical device, :-)

I will keep s390-virtio as it is, thanks for your explanation.


Thanks,
--
Ming Lei

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Ming Lei]

On Tue, Jun 17, 2014 at 6:40 PM, Ming Lei <ming.lei@canonical.com> wrote:
> On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>> On Tue, 17 Jun 2014 18:15:39 +0800
>> Ming Lei <ming.lei@canonical.com> wrote:
>>
>>> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>> >>
>>> >> Looks like s390 virtio-blk never enables the two common features, is
>>> >> there any reason the two features can't be supported by s390?
>>> >
>>> > Indirect descriptors are fine. event_idx will not work IIUC because we
>>> > always need to do a sync before we see changes, and this needs an
>>> > interrupt to trigger.
>>>
>>> Sounds like the old s390 isn't cache coherent? Because you mean
>>> write in one side can only be observed from another side with an
>>> explicit notification or interrupt.
>>>
>>> On arm/arm64, we didn't see any problem with event_idx.
>>
>> But you probably have the queues in guest memory, as on other
>> transports (including virtio-ccw)? The old s390-virtio transport keeps
>> the devices and their virtqueues in a memory area behind the guest
>> memory - the guest does not see that memory directly, but a sync has to
>> be performed to see virtqueue movement (see s390_virtio_device_sync()).
>
> OK, it looks like a real physical device, :-)
>
> I will keep s390-virtio as it is, thanks for your explanation.

BTW, do you want me to add DEFINE_VIRTIO_COMMON_FEATURES()
to s390_virtio_net_properties and s390_virtio_scsi_properties since
I remove them from their default properties?

Thanks,
--
Ming Lei

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Cornelia Huck]

On Tue, 17 Jun 2014 18:45:09 +0800
Ming Lei <ming.lei@canonical.com> wrote:

> On Tue, Jun 17, 2014 at 6:40 PM, Ming Lei <ming.lei@canonical.com> wrote:
> > On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >> On Tue, 17 Jun 2014 18:15:39 +0800
> >> Ming Lei <ming.lei@canonical.com> wrote:
> >>
> >>> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >>> >>
> >>> >> Looks like s390 virtio-blk never enables the two common features, is
> >>> >> there any reason the two features can't be supported by s390?
> >>> >
> >>> > Indirect descriptors are fine. event_idx will not work IIUC because we
> >>> > always need to do a sync before we see changes, and this needs an
> >>> > interrupt to trigger.
> >>>
> >>> Sounds like the old s390 isn't cache coherent? Because you mean
> >>> write in one side can only be observed from another side with an
> >>> explicit notification or interrupt.
> >>>
> >>> On arm/arm64, we didn't see any problem with event_idx.
> >>
> >> But you probably have the queues in guest memory, as on other
> >> transports (including virtio-ccw)? The old s390-virtio transport keeps
> >> the devices and their virtqueues in a memory area behind the guest
> >> memory - the guest does not see that memory directly, but a sync has to
> >> be performed to see virtqueue movement (see s390_virtio_device_sync()).
> >
> > OK, it looks like a real physical device, :-)
> >
> > I will keep s390-virtio as it is, thanks for your explanation.
> 
> BTW, do you want me to add DEFINE_VIRTIO_COMMON_FEATURES()
> to s390_virtio_net_properties and s390_virtio_scsi_properties since
> I remove them from their default properties?

It might be better to remove them for good :)

Nobody has probably tried to use them for some time...
I just start a very minimal guest with a virtio-console and a
virtio-blk device. Don't know whether Alex has a more advanced setup
at hand?

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Alexander Graf]

On 17.06.14 14:04, Cornelia Huck wrote:
> On Tue, 17 Jun 2014 18:45:09 +0800
> Ming Lei <ming.lei@canonical.com> wrote:
>
>> On Tue, Jun 17, 2014 at 6:40 PM, Ming Lei <ming.lei@canonical.com> wrote:
>>> On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>>> On Tue, 17 Jun 2014 18:15:39 +0800
>>>> Ming Lei <ming.lei@canonical.com> wrote:
>>>>
>>>>> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>>>>>> Looks like s390 virtio-blk never enables the two common features, is
>>>>>>> there any reason the two features can't be supported by s390?
>>>>>> Indirect descriptors are fine. event_idx will not work IIUC because we
>>>>>> always need to do a sync before we see changes, and this needs an
>>>>>> interrupt to trigger.
>>>>> Sounds like the old s390 isn't cache coherent? Because you mean
>>>>> write in one side can only be observed from another side with an
>>>>> explicit notification or interrupt.
>>>>>
>>>>> On arm/arm64, we didn't see any problem with event_idx.
>>>> But you probably have the queues in guest memory, as on other
>>>> transports (including virtio-ccw)? The old s390-virtio transport keeps
>>>> the devices and their virtqueues in a memory area behind the guest
>>>> memory - the guest does not see that memory directly, but a sync has to
>>>> be performed to see virtqueue movement (see s390_virtio_device_sync()).
>>> OK, it looks like a real physical device, :-)
>>>
>>> I will keep s390-virtio as it is, thanks for your explanation.
>> BTW, do you want me to add DEFINE_VIRTIO_COMMON_FEATURES()
>> to s390_virtio_net_properties and s390_virtio_scsi_properties since
>> I remove them from their default properties?
> It might be better to remove them for good :)
>
> Nobody has probably tried to use them for some time...
> I just start a very minimal guest with a virtio-console and a
> virtio-blk device. Don't know whether Alex has a more advanced setup
> at hand?

It's the only target that works with TCG, no?


Alex

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Cornelia Huck]

On Tue, 17 Jun 2014 14:06:25 +0200
Alexander Graf <agraf@suse.de> wrote:

> 
> On 17.06.14 14:04, Cornelia Huck wrote:
> > On Tue, 17 Jun 2014 18:45:09 +0800
> > Ming Lei <ming.lei@canonical.com> wrote:
> >
> >> On Tue, Jun 17, 2014 at 6:40 PM, Ming Lei <ming.lei@canonical.com> wrote:
> >>> On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >>>> On Tue, 17 Jun 2014 18:15:39 +0800
> >>>> Ming Lei <ming.lei@canonical.com> wrote:
> >>>>
> >>>>> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >>>>>>> Looks like s390 virtio-blk never enables the two common features, is
> >>>>>>> there any reason the two features can't be supported by s390?
> >>>>>> Indirect descriptors are fine. event_idx will not work IIUC because we
> >>>>>> always need to do a sync before we see changes, and this needs an
> >>>>>> interrupt to trigger.
> >>>>> Sounds like the old s390 isn't cache coherent? Because you mean
> >>>>> write in one side can only be observed from another side with an
> >>>>> explicit notification or interrupt.
> >>>>>
> >>>>> On arm/arm64, we didn't see any problem with event_idx.
> >>>> But you probably have the queues in guest memory, as on other
> >>>> transports (including virtio-ccw)? The old s390-virtio transport keeps
> >>>> the devices and their virtqueues in a memory area behind the guest
> >>>> memory - the guest does not see that memory directly, but a sync has to
> >>>> be performed to see virtqueue movement (see s390_virtio_device_sync()).
> >>> OK, it looks like a real physical device, :-)
> >>>
> >>> I will keep s390-virtio as it is, thanks for your explanation.
> >> BTW, do you want me to add DEFINE_VIRTIO_COMMON_FEATURES()
> >> to s390_virtio_net_properties and s390_virtio_scsi_properties since
> >> I remove them from their default properties?
> > It might be better to remove them for good :)
> >
> > Nobody has probably tried to use them for some time...
> > I just start a very minimal guest with a virtio-console and a
> > virtio-blk device. Don't know whether Alex has a more advanced setup
> > at hand?
> 
> It's the only target that works with TCG, no?

Yup. Do you have a test setup with net, scsi, ...? I usually test
networking (for example) only via a libvirt setup, which will only work
with virtio-ccw.

Re: [Qemu-devel] [PATCH 2/4] s390-virtio-bus: move common virtio properties to virtio s390 device class

[Alexander Graf]

On 17.06.14 14:56, Cornelia Huck wrote:
> On Tue, 17 Jun 2014 14:06:25 +0200
> Alexander Graf <agraf@suse.de> wrote:
>
>> On 17.06.14 14:04, Cornelia Huck wrote:
>>> On Tue, 17 Jun 2014 18:45:09 +0800
>>> Ming Lei <ming.lei@canonical.com> wrote:
>>>
>>>> On Tue, Jun 17, 2014 at 6:40 PM, Ming Lei <ming.lei@canonical.com> wrote:
>>>>> On Tue, Jun 17, 2014 at 6:25 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>>>>> On Tue, 17 Jun 2014 18:15:39 +0800
>>>>>> Ming Lei <ming.lei@canonical.com> wrote:
>>>>>>
>>>>>>> On Tue, Jun 17, 2014 at 4:46 PM, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>>>>>>>>> Looks like s390 virtio-blk never enables the two common features, is
>>>>>>>>> there any reason the two features can't be supported by s390?
>>>>>>>> Indirect descriptors are fine. event_idx will not work IIUC because we
>>>>>>>> always need to do a sync before we see changes, and this needs an
>>>>>>>> interrupt to trigger.
>>>>>>> Sounds like the old s390 isn't cache coherent? Because you mean
>>>>>>> write in one side can only be observed from another side with an
>>>>>>> explicit notification or interrupt.
>>>>>>>
>>>>>>> On arm/arm64, we didn't see any problem with event_idx.
>>>>>> But you probably have the queues in guest memory, as on other
>>>>>> transports (including virtio-ccw)? The old s390-virtio transport keeps
>>>>>> the devices and their virtqueues in a memory area behind the guest
>>>>>> memory - the guest does not see that memory directly, but a sync has to
>>>>>> be performed to see virtqueue movement (see s390_virtio_device_sync()).
>>>>> OK, it looks like a real physical device, :-)
>>>>>
>>>>> I will keep s390-virtio as it is, thanks for your explanation.
>>>> BTW, do you want me to add DEFINE_VIRTIO_COMMON_FEATURES()
>>>> to s390_virtio_net_properties and s390_virtio_scsi_properties since
>>>> I remove them from their default properties?
>>> It might be better to remove them for good :)
>>>
>>> Nobody has probably tried to use them for some time...
>>> I just start a very minimal guest with a virtio-console and a
>>> virtio-blk device. Don't know whether Alex has a more advanced setup
>>> at hand?
>> It's the only target that works with TCG, no?
> Yup. Do you have a test setup with net, scsi, ...? I usually test
> networking (for example) only via a libvirt setup, which will only work
> with virtio-ccw.

I don't regularly test it, but OBS uses that target for example.


Alex

[Qemu-devel] [PATCH 3/4] s390 virtio-ccw: move common virtio properties to virtio ccw device class

[Ming Lei]

The two common virtio features can be defined per bus, so move all
into virtio-ccw class device to make code more clean.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 hw/s390x/virtio-ccw.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 2bf0af8..c0124e1 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1126,7 +1126,6 @@ static const TypeInfo virtio_ccw_net = {
 
 static Property virtio_ccw_blk_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
-    DEFINE_VIRTIO_BLK_FEATURES(VirtioCcwDevice, host_features[0]),
     DEFINE_VIRTIO_BLK_PROPERTIES(VirtIOBlkCcw, blk),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
@@ -1158,7 +1157,6 @@ static const TypeInfo virtio_ccw_blk = {
 static Property virtio_ccw_serial_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
     DEFINE_VIRTIO_SERIAL_PROPERTIES(VirtioSerialCcw, vdev.serial),
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtioCcwDevice, host_features[0]),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
@@ -1185,7 +1183,6 @@ static const TypeInfo virtio_ccw_serial = {
 
 static Property virtio_ccw_balloon_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtioCcwDevice, host_features[0]),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
@@ -1242,7 +1239,6 @@ static const TypeInfo virtio_ccw_scsi = {
 static Property vhost_ccw_scsi_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
     DEFINE_VHOST_SCSI_PROPERTIES(VirtIOSCSICcw, vdev.parent_obj.conf),
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtioCcwDevice, host_features[0]),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1279,7 +1275,6 @@ static void virtio_ccw_rng_instance_init(Object *obj)
 
 static Property virtio_ccw_rng_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
-    DEFINE_VIRTIO_COMMON_FEATURES(VirtioCcwDevice, host_features[0]),
     DEFINE_VIRTIO_RNG_PROPERTIES(VirtIORNGCcw, vdev.conf),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
@@ -1345,10 +1340,16 @@ static int virtio_ccw_busdev_unplug(DeviceState *dev)
     return 0;
 }
 
+static Property virtio_ccw_properties[] = {
+    DEFINE_VIRTIO_COMMON_FEATURES(VirtioCcwDevice, host_features[0]),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void virtio_ccw_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->props = virtio_ccw_properties;
     dc->init = virtio_ccw_busdev_init;
     dc->exit = virtio_ccw_busdev_exit;
     dc->unplug = virtio_ccw_busdev_unplug;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 3/4] s390 virtio-ccw: move common virtio properties to virtio ccw device class

[Cornelia Huck]

On Mon, 16 Jun 2014 23:40:51 +0800
Ming Lei <ming.lei@canonical.com> wrote:

> The two common virtio features can be defined per bus, so move all
> into virtio-ccw class device to make code more clean.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  hw/s390x/virtio-ccw.c |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Seems to work fine.

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

[Qemu-devel] [PATCH 4/4] virtio-blk: remove DEFINE_VIRTIO_BLK_FEATURES

[Ming Lei]

Now remove the macro since no one use it any more.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 include/hw/virtio/virtio-blk.h |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 0135ca2..03f7ed4 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -144,9 +144,6 @@ typedef struct VirtIOBlock {
 #endif
 } VirtIOBlock;
 
-#define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
-        DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)
-
 #ifdef __linux__
 #define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field)                          \
         DEFINE_BLOCK_PROPERTIES(_state, _field.conf),                         \
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 0/4] virtio: move common virtio properties to bus class device

[Paolo Bonzini]

Il 16/06/2014 17:40, Ming Lei ha scritto:
> Hi,
>
> This patchset is suggested by Paolo, and takes the approach in below
> patch for virtio-mmio device:
>
> 	http://marc.info/?t=140184945400001&r=1&w=2
>
> Then we can move the virtio common properties into bus class device, and
> make code clean.
>
> Thanks,
> --
> Ming Lei
>

I think they have to be all squashed together for perfect bisectability. 
  Apart from this,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Michael, can you queue http://marc.info/?t=140184945400001&r=1&w=2 for 
2.1?  These probably are more suited to 2.2.

Paolo

Re: [Qemu-devel] [PATCH 0/4] virtio: move common virtio properties to bus class device

[Ming Lei]

On Mon, Jun 16, 2014 at 11:44 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:

> I think they have to be all squashed together for perfect bisectability.
> Apart from this,
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks, and I will put all the four patches in one.


Thanks,
--
Ming Lei