[Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

The reason to do this is that the virtio-net-device can't expose host
features to guest while using virtio-mmio. So the performance is low.

The virtio-*-pci, virtio-*-s390, and virtio-*-ccw already have the
ability to forward property accesses to the backend child, by calling 
*_virtio_*_instance_init -> qdev_alias_all_properties. So if we move the
host features to backends, it doesn't break the backwards compatibility
for virtio-*-pci, virtio-*-s390, and virtio-*-ccw.

Here we move the host features to backends, involving
DEFINE_VIRTIO_NET_FEATURES, DEFINE_VIRTIO_SCSI_FEATURES. So the
virtio-mmio devices could have the host freatures, and this has a great
performance improvement to virtio-mmio, especially to virtio-net-device.

changes since v3:
  * detail the changes in commit log
  * don't expose virtio_net_set_config_size

changes since v2:
  * move virtio_net_set_config_size to virtio-net (Cornelia)

changes since v1:
  * drop unnecessary change of adding device_plugged hook for
    virtio-ccw and s390-virtio-bus (Cornelia)

Shannon Zhao (2):
  virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
  virtio-scsi: Move DEFINE_VIRTIO_SCSI_FEATURES to virtio-scsi

 hw/net/virtio-net.c             | 7 ++++++-
 hw/s390x/s390-virtio-bus.c      | 3 ---
 hw/s390x/virtio-ccw.c           | 3 ---
 hw/scsi/virtio-scsi.c           | 5 +++++
 hw/virtio/virtio-pci.c          | 3 ---
 include/hw/virtio/virtio-net.h  | 2 +-
 include/hw/virtio/virtio-scsi.h | 1 +
 7 files changed, 13 insertions(+), 11 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v4 1/2] virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

So far virtio-net-device can't expose host features to guest while
using virtio-mmio because it doesn't set DEFINE_VIRTIO_NET_FEATURES on
backend or transport. So the performance is low.

The host features belong to the backend while virtio-net-pci,
virtio-net-s390 and virtio-net-ccw set the DEFINE_VIRTIO_NET_FEATURES
on transports. But they already have the ability to forward property
accesses to the backend child. So if we move the host features to
backends, it doesn't break the backwards compatibility for them and
make host features work while using virtio-mmio.

Here we move DEFINE_VIRTIO_NET_FEATURES to the backend virtio-net. The
transports just sync the host features from backend. Meanwhile move
virtio_net_set_config_size to virtio-net to make sure the config size
is correct and don't expose it.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/net/virtio-net.c            | 7 ++++++-
 hw/s390x/s390-virtio-bus.c     | 2 --
 hw/s390x/virtio-ccw.c          | 2 --
 hw/virtio/virtio-pci.c         | 2 --
 include/hw/virtio/virtio-net.h | 2 +-
 5 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 59f76bc..5c38ac2 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -446,6 +446,9 @@ static uint32_t virtio_net_get_features(VirtIODevice *vdev, uint32_t features)
     VirtIONet *n = VIRTIO_NET(vdev);
     NetClientState *nc = qemu_get_queue(n->nic);
 
+    /* Firstly sync all virtio-net possible supported features */
+    features |= n->host_features;
+
     virtio_add_feature(&features, VIRTIO_NET_F_MAC);
 
     if (!peer_has_vnet_hdr(n)) {
@@ -1552,7 +1555,7 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
                              vdev, idx, mask);
 }
 
-void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
+static void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features)
 {
     int i, config_size = 0;
     virtio_add_feature(&host_features, VIRTIO_NET_F_MAC);
@@ -1585,6 +1588,7 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
     NetClientState *nc;
     int i;
 
+    virtio_net_set_config_size(n, n->host_features);
     virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
 
     n->max_queues = MAX(n->nic_conf.peers.queues, 1);
@@ -1721,6 +1725,7 @@ static void virtio_net_instance_init(Object *obj)
 }
 
 static Property virtio_net_properties[] = {
+    DEFINE_VIRTIO_NET_FEATURES(VirtIONet, host_features),
     DEFINE_NIC_PROPERTIES(VirtIONet, nic_conf),
     DEFINE_PROP_UINT32("x-txtimer", VirtIONet, net_conf.txtimer,
                                                TX_TIMER_INTERVAL),
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 047c963..b893e02 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -145,7 +145,6 @@ static void s390_virtio_net_realize(VirtIOS390Device *s390_dev, Error **errp)
     DeviceState *vdev = DEVICE(&dev->vdev);
     Error *err = NULL;
 
-    virtio_net_set_config_size(&dev->vdev, s390_dev->host_features);
     virtio_net_set_netclient_name(&dev->vdev, qdev->id,
                                   object_get_typename(OBJECT(qdev)));
     qdev_set_parent_bus(vdev, BUS(&s390_dev->bus));
@@ -508,7 +507,6 @@ static unsigned virtio_s390_get_features(DeviceState *d)
 
 static Property s390_virtio_net_properties[] = {
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
-    DEFINE_VIRTIO_NET_FEATURES(VirtIOS390Device, host_features),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index d32ecaf..1252162 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -779,7 +779,6 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     DeviceState *vdev = DEVICE(&dev->vdev);
     Error *err = NULL;
 
-    virtio_net_set_config_size(&dev->vdev, ccw_dev->host_features[0]);
     virtio_net_set_netclient_name(&dev->vdev, qdev->id,
                                   object_get_typename(OBJECT(qdev)));
     qdev_set_parent_bus(vdev, BUS(&ccw_dev->bus));
@@ -1403,7 +1402,6 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
 
 static Property virtio_ccw_net_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
-    DEFINE_VIRTIO_NET_FEATURES(VirtioCcwDevice, host_features[0]),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index c7c3f72..c6b99f9 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1360,7 +1360,6 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags,
                     VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, false),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 3),
-    DEFINE_VIRTIO_NET_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -1370,7 +1369,6 @@ static void virtio_net_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
     VirtIONetPCI *dev = VIRTIO_NET_PCI(vpci_dev);
     DeviceState *vdev = DEVICE(&dev->vdev);
 
-    virtio_net_set_config_size(&dev->vdev, vpci_dev->host_features);
     virtio_net_set_netclient_name(&dev->vdev, qdev->id,
                                   object_get_typename(OBJECT(qdev)));
     qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 4c2fe83..e0dbb41 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -68,6 +68,7 @@ typedef struct VirtIONet {
     uint32_t has_vnet_hdr;
     size_t host_hdr_len;
     size_t guest_hdr_len;
+    uint32_t host_features;
     uint8_t has_ufo;
     int mergeable_rx_bufs;
     uint8_t promisc;
@@ -137,7 +138,6 @@ typedef struct VirtIONet {
     DEFINE_PROP_INT32("x-txburst", _state, _field.txburst, TX_BURST),          \
     DEFINE_PROP_STRING("tx", _state, _field.tx)
 
-void virtio_net_set_config_size(VirtIONet *n, uint32_t host_features);
 void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
                                    const char *type);
 
-- 
2.1.0

[Qemu-devel] [PATCH v4 2/2] virtio-scsi: Move DEFINE_VIRTIO_SCSI_FEATURES to virtio-scsi

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

So far virtio-scsi-device can't expose host features to guest while
using virtio-mmio because it doesn't set DEFINE_VIRTIO_SCSI_FEATURES on
backend or transport.

The host features belong to the backends while virtio-scsi-pci,
virtio-scsi-s390 and virtio-scsi-ccw set the DEFINE_VIRTIO_SCSI_FEATURES
on transports. But they already have the ability to forward property
accesses to the backend child. So if we move the host features to
backends, it doesn't break the backwards compatibility for them and
make host features work while using virtio-mmio.

Move DEFINE_VIRTIO_SCSI_FEATURES to the backend virtio-scsi. The
transports just sync the host features from backends.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/s390x/s390-virtio-bus.c      | 1 -
 hw/s390x/virtio-ccw.c           | 1 -
 hw/scsi/virtio-scsi.c           | 5 +++++
 hw/virtio/virtio-pci.c          | 1 -
 include/hw/virtio/virtio-scsi.h | 1 +
 5 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index b893e02..c8a78ba 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -622,7 +622,6 @@ static const TypeInfo virtio_s390_device_info = {
 
 static Property s390_virtio_scsi_properties[] = {
     DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
-    DEFINE_VIRTIO_SCSI_FEATURES(VirtIOS390Device, host_features),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 1252162..ef97fe9 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1506,7 +1506,6 @@ static const TypeInfo virtio_ccw_balloon = {
 
 static Property virtio_ccw_scsi_properties[] = {
     DEFINE_PROP_STRING("devno", VirtioCcwDevice, bus_id),
-    DEFINE_VIRTIO_SCSI_FEATURES(VirtioCcwDevice, host_features[0]),
     DEFINE_PROP_BIT("ioeventfd", VirtioCcwDevice, flags,
                     VIRTIO_CCW_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index c9bea06..e242fef 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -631,6 +631,10 @@ static void virtio_scsi_set_config(VirtIODevice *vdev,
 static uint32_t virtio_scsi_get_features(VirtIODevice *vdev,
                                          uint32_t requested_features)
 {
+    VirtIOSCSI *s = VIRTIO_SCSI(vdev);
+
+    /* Firstly sync all virtio-scsi possible supported features */
+    requested_features |= s->host_features;
     return requested_features;
 }
 
@@ -945,6 +949,7 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp)
 
 static Property virtio_scsi_properties[] = {
     DEFINE_VIRTIO_SCSI_PROPERTIES(VirtIOSCSI, parent_obj.conf),
+    DEFINE_VIRTIO_SCSI_FEATURES(VirtIOSCSI, host_features),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index c6b99f9..5c173c4 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1078,7 +1078,6 @@ static Property virtio_scsi_pci_properties[] = {
                     VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors,
                        DEV_NVECTORS_UNSPECIFIED),
-    DEFINE_VIRTIO_SCSI_FEATURES(VirtIOPCIProxy, host_features),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index f93b57d..b42e7f1 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -98,6 +98,7 @@ typedef struct VirtIOSCSI {
     bool dataplane_fenced;
     Error *blocker;
     Notifier migration_state_notifier;
+    uint32_t host_features;
 } VirtIOSCSI;
 
 typedef struct VirtIOSCSIReq {
-- 
2.1.0

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 07:51:11PM +0800, shannon.zhao@linaro.org wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> The reason to do this is that the virtio-net-device can't expose host
> features to guest while using virtio-mmio. So the performance is low.
> 
> The virtio-*-pci, virtio-*-s390, and virtio-*-ccw already have the
> ability to forward property accesses to the backend child, by calling 
> *_virtio_*_instance_init -> qdev_alias_all_properties. So if we move the
> host features to backends, it doesn't break the backwards compatibility
> for virtio-*-pci, virtio-*-s390, and virtio-*-ccw.
> 
> Here we move the host features to backends, involving
> DEFINE_VIRTIO_NET_FEATURES, DEFINE_VIRTIO_SCSI_FEATURES. So the
> virtio-mmio devices could have the host freatures, and this has a great
> performance improvement to virtio-mmio, especially to virtio-net-device.

Can you move COMMON_FEATURES too please?

> changes since v3:
>   * detail the changes in commit log
>   * don't expose virtio_net_set_config_size
> 
> changes since v2:
>   * move virtio_net_set_config_size to virtio-net (Cornelia)
> 
> changes since v1:
>   * drop unnecessary change of adding device_plugged hook for
>     virtio-ccw and s390-virtio-bus (Cornelia)
> 
> Shannon Zhao (2):
>   virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
>   virtio-scsi: Move DEFINE_VIRTIO_SCSI_FEATURES to virtio-scsi
> 
>  hw/net/virtio-net.c             | 7 ++++++-
>  hw/s390x/s390-virtio-bus.c      | 3 ---
>  hw/s390x/virtio-ccw.c           | 3 ---
>  hw/scsi/virtio-scsi.c           | 5 +++++
>  hw/virtio/virtio-pci.c          | 3 ---
>  include/hw/virtio/virtio-net.h  | 2 +-
>  include/hw/virtio/virtio-scsi.h | 1 +
>  7 files changed, 13 insertions(+), 11 deletions(-)
> 
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Peter Maydell]

On 28 April 2015 at 13:48, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Apr 28, 2015 at 07:51:11PM +0800, shannon.zhao@linaro.org wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>> Here we move the host features to backends, involving
>> DEFINE_VIRTIO_NET_FEATURES, DEFINE_VIRTIO_SCSI_FEATURES. So the
>> virtio-mmio devices could have the host freatures, and this has a great
>> performance improvement to virtio-mmio, especially to virtio-net-device.
>
> Can you move COMMON_FEATURES too please?

I think that would be wrong -- COMMON_FEATURES are transport features,
not backend-specific features, so they belong on the transports
(and on the convenience wrappers), not the backends.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 01:52:40PM +0100, Peter Maydell wrote:
> On 28 April 2015 at 13:48, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Apr 28, 2015 at 07:51:11PM +0800, shannon.zhao@linaro.org wrote:
> >> From: Shannon Zhao <shannon.zhao@linaro.org>
> >> Here we move the host features to backends, involving
> >> DEFINE_VIRTIO_NET_FEATURES, DEFINE_VIRTIO_SCSI_FEATURES. So the
> >> virtio-mmio devices could have the host freatures, and this has a great
> >> performance improvement to virtio-mmio, especially to virtio-net-device.
> >
> > Can you move COMMON_FEATURES too please?
> 
> I think that would be wrong -- COMMON_FEATURES are transport features,
> not backend-specific features, so they belong on the transports
> (and on the convenience wrappers), not the backends.
> 
> thanks
> -- PMM

Hmm you are right. The problem is with s390 which
puts this DEFINE_VIRTIO_COMMON_FEATURES in all devices.

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Cornelia Huck]

On Tue, 28 Apr 2015 15:05:47 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 28, 2015 at 01:52:40PM +0100, Peter Maydell wrote:
> > On 28 April 2015 at 13:48, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Tue, Apr 28, 2015 at 07:51:11PM +0800, shannon.zhao@linaro.org wrote:
> > >> From: Shannon Zhao <shannon.zhao@linaro.org>
> > >> Here we move the host features to backends, involving
> > >> DEFINE_VIRTIO_NET_FEATURES, DEFINE_VIRTIO_SCSI_FEATURES. So the
> > >> virtio-mmio devices could have the host freatures, and this has a great
> > >> performance improvement to virtio-mmio, especially to virtio-net-device.
> > >
> > > Can you move COMMON_FEATURES too please?
> > 
> > I think that would be wrong -- COMMON_FEATURES are transport features,
> > not backend-specific features, so they belong on the transports
> > (and on the convenience wrappers), not the backends.
> > 
> > thanks
> > -- PMM
> 
> Hmm you are right. The problem is with s390 which
> puts this DEFINE_VIRTIO_COMMON_FEATURES in all devices.

All save virtio-blk. There were some problems when switching on
event_idx on virtio-blk, IIRC.

The other transport add DEFINE_VIRTIO_COMMON FEATURES in their base
class. Because of the not-quite-understood problem with virtio-blk, we
couldn't do that with s390-virtio.

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 03:20:54PM +0200, Cornelia Huck wrote:
> On Tue, 28 Apr 2015 15:05:47 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Apr 28, 2015 at 01:52:40PM +0100, Peter Maydell wrote:
> > > On 28 April 2015 at 13:48, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Tue, Apr 28, 2015 at 07:51:11PM +0800, shannon.zhao@linaro.org wrote:
> > > >> From: Shannon Zhao <shannon.zhao@linaro.org>
> > > >> Here we move the host features to backends, involving
> > > >> DEFINE_VIRTIO_NET_FEATURES, DEFINE_VIRTIO_SCSI_FEATURES. So the
> > > >> virtio-mmio devices could have the host freatures, and this has a great
> > > >> performance improvement to virtio-mmio, especially to virtio-net-device.
> > > >
> > > > Can you move COMMON_FEATURES too please?
> > > 
> > > I think that would be wrong -- COMMON_FEATURES are transport features,
> > > not backend-specific features, so they belong on the transports
> > > (and on the convenience wrappers), not the backends.
> > > 
> > > thanks
> > > -- PMM
> > 
> > Hmm you are right. The problem is with s390 which
> > puts this DEFINE_VIRTIO_COMMON_FEATURES in all devices.
> 
> All save virtio-blk. There were some problems when switching on
> event_idx on virtio-blk, IIRC.

Specifically on s390?

> The other transport add DEFINE_VIRTIO_COMMON FEATURES in their base
> class. Because of the not-quite-understood problem with virtio-blk, we
> couldn't do that with s390-virtio.

-- 
MST

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Peter Maydell]

On 28 April 2015 at 12:51,  <shannon.zhao@linaro.org> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> The reason to do this is that the virtio-net-device can't expose host
> features to guest while using virtio-mmio. So the performance is low.
>
> The virtio-*-pci, virtio-*-s390, and virtio-*-ccw already have the
> ability to forward property accesses to the backend child, by calling
> *_virtio_*_instance_init -> qdev_alias_all_properties. So if we move the
> host features to backends, it doesn't break the backwards compatibility
> for virtio-*-pci, virtio-*-s390, and virtio-*-ccw.
>
> Here we move the host features to backends, involving
> DEFINE_VIRTIO_NET_FEATURES, DEFINE_VIRTIO_SCSI_FEATURES. So the
> virtio-mmio devices could have the host freatures, and this has a great
> performance improvement to virtio-mmio, especially to virtio-net-device.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

(but not tested :-))

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 02:06:35PM +0100, Peter Maydell wrote:
> On 28 April 2015 at 12:51,  <shannon.zhao@linaro.org> wrote:
> > From: Shannon Zhao <shannon.zhao@linaro.org>
> >
> > The reason to do this is that the virtio-net-device can't expose host
> > features to guest while using virtio-mmio. So the performance is low.
> >
> > The virtio-*-pci, virtio-*-s390, and virtio-*-ccw already have the
> > ability to forward property accesses to the backend child, by calling
> > *_virtio_*_instance_init -> qdev_alias_all_properties. So if we move the
> > host features to backends, it doesn't break the backwards compatibility
> > for virtio-*-pci, virtio-*-s390, and virtio-*-ccw.
> >
> > Here we move the host features to backends, involving
> > DEFINE_VIRTIO_NET_FEATURES, DEFINE_VIRTIO_SCSI_FEATURES. So the
> > virtio-mmio devices could have the host freatures, and this has a great
> > performance improvement to virtio-mmio, especially to virtio-net-device.
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> (but not tested :-))
> 
> thanks
> -- PMM

The patches look correct to me too, but I want s390
cleaned up so it does not include COMMON_FEATURES
in 100 places, and I prefer merging it all together.

-- 
MST

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Peter Maydell]

On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> The patches look correct to me too, but I want s390
> cleaned up so it does not include COMMON_FEATURES
> in 100 places, and I prefer merging it all together.

It seems a bit harsh to ask Shannon to do s390 cleanup when
he doesn't have any access to s390 guests or test cases...
Making S390 put COMMON_FEATURES in the right places seems
to me like a separate bit of s390-specific cleanup.

(The other cleanup we could do after this patchset would
be to just expand out the DEFINE_VIRTIO_NET/RNG/SCSI/etc_FEATURES
macros which are all now used in exactly one place. But
again I think that's separate and can be done later.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Cornelia Huck]

On Tue, 28 Apr 2015 14:16:40 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> > The patches look correct to me too, but I want s390
> > cleaned up so it does not include COMMON_FEATURES
> > in 100 places, and I prefer merging it all together.
> 
> It seems a bit harsh to ask Shannon to do s390 cleanup when
> he doesn't have any access to s390 guests or test cases...
> Making S390 put COMMON_FEATURES in the right places seems
> to me like a separate bit of s390-specific cleanup.

Yep, see my other reply... I'm not quite sure what's wrong with
event_idx on virtio-blk for s390-virtio, or I would gladly make this
consistent with the other transports. Any hints appreciated :)

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
> On Tue, 28 Apr 2015 14:16:40 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > The patches look correct to me too, but I want s390
> > > cleaned up so it does not include COMMON_FEATURES
> > > in 100 places, and I prefer merging it all together.
> > 
> > It seems a bit harsh to ask Shannon to do s390 cleanup when
> > he doesn't have any access to s390 guests or test cases...
> > Making S390 put COMMON_FEATURES in the right places seems
> > to me like a separate bit of s390-specific cleanup.
> 
> Yep, see my other reply... I'm not quite sure what's wrong with
> event_idx on virtio-blk for s390-virtio, or I would gladly make this
> consistent with the other transports. Any hints appreciated :)

Is this still happening?

It is possible that what was missing was
92045d80badc43c9f95897aad675dc7ef17a3b3f
and/or
a281ebc11a6917fbc27e1a93bb5772cd14e241fc


-- 
MST

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
> On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
> > On Tue, 28 Apr 2015 14:16:40 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> > 
> > > On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > The patches look correct to me too, but I want s390
> > > > cleaned up so it does not include COMMON_FEATURES
> > > > in 100 places, and I prefer merging it all together.
> > > 
> > > It seems a bit harsh to ask Shannon to do s390 cleanup when
> > > he doesn't have any access to s390 guests or test cases...
> > > Making S390 put COMMON_FEATURES in the right places seems
> > > to me like a separate bit of s390-specific cleanup.
> > 
> > Yep, see my other reply... I'm not quite sure what's wrong with
> > event_idx on virtio-blk for s390-virtio, or I would gladly make this
> > consistent with the other transports. Any hints appreciated :)
> 
> Is this still happening?
> 
> It is possible that what was missing was
> 92045d80badc43c9f95897aad675dc7ef17a3b3f
> and/or
> a281ebc11a6917fbc27e1a93bb5772cd14e241fc
> 

Found this:
http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
so it's unlikely: these commits are from 2012, you saw
issues in 2014.

We really need to fix it. virtio 1 work will be much easier if
we can just move features into virtio dev.

> -- 
> MST

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 08:14:44PM +0200, Michael S. Tsirkin wrote:
> On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
> > > On Tue, 28 Apr 2015 14:16:40 +0100
> > > Peter Maydell <peter.maydell@linaro.org> wrote:
> > > 
> > > > On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > The patches look correct to me too, but I want s390
> > > > > cleaned up so it does not include COMMON_FEATURES
> > > > > in 100 places, and I prefer merging it all together.
> > > > 
> > > > It seems a bit harsh to ask Shannon to do s390 cleanup when
> > > > he doesn't have any access to s390 guests or test cases...
> > > > Making S390 put COMMON_FEATURES in the right places seems
> > > > to me like a separate bit of s390-specific cleanup.
> > > 
> > > Yep, see my other reply... I'm not quite sure what's wrong with
> > > event_idx on virtio-blk for s390-virtio, or I would gladly make this
> > > consistent with the other transports. Any hints appreciated :)
> > 
> > Is this still happening?
> > 
> > It is possible that what was missing was
> > 92045d80badc43c9f95897aad675dc7ef17a3b3f
> > and/or
> > a281ebc11a6917fbc27e1a93bb5772cd14e241fc
> > 
> 
> Found this:
> http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
> so it's unlikely: these commits are from 2012, you saw
> issues in 2014.
> 
> We really need to fix it. virtio 1 work will be much easier if
> we can just move features into virtio dev.

I'm beginning to suspect this is a wrong implementation of barriers.
Questions:
    - which compiler to you use?
    - can you pls disassemble code for smp_wmb smp_rmb and smp_mb?
      They all must do br %r14 I think, and this is what
      s390x-linux-gnu-gcc generated for me:
        s390x-linux-gnu-gcc (GCC) 4.9.1

> > -- 
> > MST

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 08:32:51PM +0200, Michael S. Tsirkin wrote:
> On Tue, Apr 28, 2015 at 08:14:44PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
> > > On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
> > > > On Tue, 28 Apr 2015 14:16:40 +0100
> > > > Peter Maydell <peter.maydell@linaro.org> wrote:
> > > > 
> > > > > On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > > The patches look correct to me too, but I want s390
> > > > > > cleaned up so it does not include COMMON_FEATURES
> > > > > > in 100 places, and I prefer merging it all together.
> > > > > 
> > > > > It seems a bit harsh to ask Shannon to do s390 cleanup when
> > > > > he doesn't have any access to s390 guests or test cases...
> > > > > Making S390 put COMMON_FEATURES in the right places seems
> > > > > to me like a separate bit of s390-specific cleanup.
> > > > 
> > > > Yep, see my other reply... I'm not quite sure what's wrong with
> > > > event_idx on virtio-blk for s390-virtio, or I would gladly make this
> > > > consistent with the other transports. Any hints appreciated :)
> > > 
> > > Is this still happening?
> > > 
> > > It is possible that what was missing was
> > > 92045d80badc43c9f95897aad675dc7ef17a3b3f
> > > and/or
> > > a281ebc11a6917fbc27e1a93bb5772cd14e241fc
> > > 
> > 
> > Found this:
> > http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
> > so it's unlikely: these commits are from 2012, you saw
> > issues in 2014.
> > 
> > We really need to fix it. virtio 1 work will be much easier if
> > we can just move features into virtio dev.
> 
> I'm beginning to suspect this is a wrong implementation of barriers.
> Questions:
>     - which compiler to you use?
>     - can you pls disassemble code for smp_wmb smp_rmb and smp_mb?
>       They all must do br %r14 I think, and this is what
>       s390x-linux-gnu-gcc generated for me:
>         s390x-linux-gnu-gcc (GCC) 4.9.1


Also, what does the following do?

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/include/qemu/atomic.h b/include/qemu/atomic.h
index 98e05ca..1c8051b 100644
--- a/include/qemu/atomic.h
+++ b/include/qemu/atomic.h
@@ -43,7 +43,7 @@
 #define smp_read_barrier_depends()   asm volatile("mb":::"memory")
 #endif
 
-#if defined(__i386__) || defined(__x86_64__) || defined(__s390x__)
+#if defined(__i386__) || defined(__x86_64__)
 
 /*
  * Because of the strongly ordered storage model, wmb() and rmb() are nops
@@ -53,6 +53,10 @@
 #define smp_wmb()   barrier()
 #define smp_rmb()   barrier()
 
+#endif
+
+#if defined(__i386__) || defined(__x86_64__) || defined(__s390x__)
+
 /*
  * __sync_lock_test_and_set() is documented to be an acquire barrier only,
  * but it is a full barrier at the hardware level.  Add a compiler barrier
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index c8a78ba..0f31ae3 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -527,11 +527,17 @@ static const TypeInfo s390_virtio_net = {
     .class_init    = s390_virtio_net_class_init,
 };
 
+static Property s390_virtio_blk_properties[] = {
+    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOS390Device, host_features),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void s390_virtio_blk_class_init(ObjectClass *klass, void *data)
 {
     VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
 
     k->realize = s390_virtio_blk_realize;
+    dc->props = s390_virtio_blk_properties;
 }
 
 static const TypeInfo s390_virtio_blk = {

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Christian Borntraeger]

Am 28.04.2015 um 20:32 schrieb Michael S. Tsirkin:
> On Tue, Apr 28, 2015 at 08:14:44PM +0200, Michael S. Tsirkin wrote:
>> On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
>>> On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
>>>> On Tue, 28 Apr 2015 14:16:40 +0100
>>>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>
>>>>> On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>> The patches look correct to me too, but I want s390
>>>>>> cleaned up so it does not include COMMON_FEATURES
>>>>>> in 100 places, and I prefer merging it all together.
>>>>>
>>>>> It seems a bit harsh to ask Shannon to do s390 cleanup when
>>>>> he doesn't have any access to s390 guests or test cases...
>>>>> Making S390 put COMMON_FEATURES in the right places seems
>>>>> to me like a separate bit of s390-specific cleanup.
>>>>
>>>> Yep, see my other reply... I'm not quite sure what's wrong with
>>>> event_idx on virtio-blk for s390-virtio, or I would gladly make this
>>>> consistent with the other transports. Any hints appreciated :)
>>>
>>> Is this still happening?
>>>
>>> It is possible that what was missing was
>>> 92045d80badc43c9f95897aad675dc7ef17a3b3f
>>> and/or
>>> a281ebc11a6917fbc27e1a93bb5772cd14e241fc
>>>
>>
>> Found this:
>> http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
>> so it's unlikely: these commits are from 2012, you saw
>> issues in 2014.
>>
>> We really need to fix it. virtio 1 work will be much easier if
>> we can just move features into virtio dev.

Yes, we have to understand why event_idx breaks for the s390-virtio transport.
> 
> I'm beginning to suspect this is a wrong implementation of barriers.
> Questions:
>     - which compiler to you use?
>     - can you pls disassemble code for smp_wmb smp_rmb and smp_mb?
>       They all must do br %r14 I think, and this is what
>       s390x-linux-gnu-gcc generated for me:
>         s390x-linux-gnu-gcc (GCC) 4.9.1

s390 has strong memory ordering. Reads are in order, writes are in order. 
bcr 14,0 or bcr 15,0 then only serialize the reads against the writes.
So smp_rmb and smp_wmb can be implemented as no-ops like QEMU.
If your change "fixes" the issue then we have a problem somewhere else

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Cornelia Huck]

On Wed, 29 Apr 2015 10:17:55 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> Am 28.04.2015 um 20:32 schrieb Michael S. Tsirkin:
> > On Tue, Apr 28, 2015 at 08:14:44PM +0200, Michael S. Tsirkin wrote:
> >> On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
> >>> On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
> >>>> On Tue, 28 Apr 2015 14:16:40 +0100
> >>>> Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>>
> >>>>> On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>> The patches look correct to me too, but I want s390
> >>>>>> cleaned up so it does not include COMMON_FEATURES
> >>>>>> in 100 places, and I prefer merging it all together.
> >>>>>
> >>>>> It seems a bit harsh to ask Shannon to do s390 cleanup when
> >>>>> he doesn't have any access to s390 guests or test cases...
> >>>>> Making S390 put COMMON_FEATURES in the right places seems
> >>>>> to me like a separate bit of s390-specific cleanup.
> >>>>
> >>>> Yep, see my other reply... I'm not quite sure what's wrong with
> >>>> event_idx on virtio-blk for s390-virtio, or I would gladly make this
> >>>> consistent with the other transports. Any hints appreciated :)
> >>>
> >>> Is this still happening?
> >>>
> >>> It is possible that what was missing was
> >>> 92045d80badc43c9f95897aad675dc7ef17a3b3f
> >>> and/or
> >>> a281ebc11a6917fbc27e1a93bb5772cd14e241fc
> >>>
> >>
> >> Found this:
> >> http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
> >> so it's unlikely: these commits are from 2012, you saw
> >> issues in 2014.
> >>
> >> We really need to fix it. virtio 1 work will be much easier if
> >> we can just move features into virtio dev.
> 
> Yes, we have to understand why event_idx breaks for the s390-virtio transport.
> > 
> > I'm beginning to suspect this is a wrong implementation of barriers.
> > Questions:
> >     - which compiler to you use?
> >     - can you pls disassemble code for smp_wmb smp_rmb and smp_mb?
> >       They all must do br %r14 I think, and this is what
> >       s390x-linux-gnu-gcc generated for me:
> >         s390x-linux-gnu-gcc (GCC) 4.9.1
> 
> s390 has strong memory ordering. Reads are in order, writes are in order. 
> bcr 14,0 or bcr 15,0 then only serialize the reads against the writes.
> So smp_rmb and smp_wmb can be implemented as no-ops like QEMU.
> If your change "fixes" the issue then we have a problem somewhere else

And (surprise, surprise) virtio-blk now works - but it also works when
I back out the atomic.h change again. No barrier problems :)

Good news is that we can change s390-virtio to be just like the other
transports. Although I'd like to understand why it was broken before.
Maybe a guest change?

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Wed, Apr 29, 2015 at 10:52:15AM +0200, Cornelia Huck wrote:
> On Wed, 29 Apr 2015 10:17:55 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
> > Am 28.04.2015 um 20:32 schrieb Michael S. Tsirkin:
> > > On Tue, Apr 28, 2015 at 08:14:44PM +0200, Michael S. Tsirkin wrote:
> > >> On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
> > >>> On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
> > >>>> On Tue, 28 Apr 2015 14:16:40 +0100
> > >>>> Peter Maydell <peter.maydell@linaro.org> wrote:
> > >>>>
> > >>>>> On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> > >>>>>> The patches look correct to me too, but I want s390
> > >>>>>> cleaned up so it does not include COMMON_FEATURES
> > >>>>>> in 100 places, and I prefer merging it all together.
> > >>>>>
> > >>>>> It seems a bit harsh to ask Shannon to do s390 cleanup when
> > >>>>> he doesn't have any access to s390 guests or test cases...
> > >>>>> Making S390 put COMMON_FEATURES in the right places seems
> > >>>>> to me like a separate bit of s390-specific cleanup.
> > >>>>
> > >>>> Yep, see my other reply... I'm not quite sure what's wrong with
> > >>>> event_idx on virtio-blk for s390-virtio, or I would gladly make this
> > >>>> consistent with the other transports. Any hints appreciated :)
> > >>>
> > >>> Is this still happening?
> > >>>
> > >>> It is possible that what was missing was
> > >>> 92045d80badc43c9f95897aad675dc7ef17a3b3f
> > >>> and/or
> > >>> a281ebc11a6917fbc27e1a93bb5772cd14e241fc
> > >>>
> > >>
> > >> Found this:
> > >> http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
> > >> so it's unlikely: these commits are from 2012, you saw
> > >> issues in 2014.
> > >>
> > >> We really need to fix it. virtio 1 work will be much easier if
> > >> we can just move features into virtio dev.
> > 
> > Yes, we have to understand why event_idx breaks for the s390-virtio transport.
> > > 
> > > I'm beginning to suspect this is a wrong implementation of barriers.
> > > Questions:
> > >     - which compiler to you use?
> > >     - can you pls disassemble code for smp_wmb smp_rmb and smp_mb?
> > >       They all must do br %r14 I think, and this is what
> > >       s390x-linux-gnu-gcc generated for me:
> > >         s390x-linux-gnu-gcc (GCC) 4.9.1
> > 
> > s390 has strong memory ordering. Reads are in order, writes are in order. 
> > bcr 14,0 or bcr 15,0 then only serialize the reads against the writes.
> > So smp_rmb and smp_wmb can be implemented as no-ops like QEMU.
> > If your change "fixes" the issue then we have a problem somewhere else
> 
> And (surprise, surprise) virtio-blk now works - but it also works when
> I back out the atomic.h change again. No barrier problems :)
> 
> Good news is that we can change s390-virtio to be just like the other
> transports. Although I'd like to understand why it was broken before.
> Maybe a guest change?

Or a compiler change? Try compiling some old release, see what happens.
Anyway, let's move DEFINE_VIRTIO_COMMON_FEATURES into the base class
now.  Can you send a patch pls?

-- 
MST

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Cornelia Huck]

On Wed, 29 Apr 2015 12:32:04 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Apr 29, 2015 at 10:52:15AM +0200, Cornelia Huck wrote:

> > And (surprise, surprise) virtio-blk now works - but it also works when
> > I back out the atomic.h change again. No barrier problems :)
> > 
> > Good news is that we can change s390-virtio to be just like the other
> > transports. Although I'd like to understand why it was broken before.
> > Maybe a guest change?
> 
> Or a compiler change? Try compiling some old release, see what happens.

Nope, same compiler as back then (old setup). My current guess is some
change in other code.

> Anyway, let's move DEFINE_VIRTIO_COMMON_FEATURES into the base class
> now.  Can you send a patch pls?

I think virtio-blk-s390 not working anymore will be easy to catch (it
is used in some of the iotests). I'll move the features.

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Wed, Apr 29, 2015 at 04:33:44PM +0200, Cornelia Huck wrote:
> On Wed, 29 Apr 2015 12:32:04 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Apr 29, 2015 at 10:52:15AM +0200, Cornelia Huck wrote:
> 
> > > And (surprise, surprise) virtio-blk now works - but it also works when
> > > I back out the atomic.h change again. No barrier problems :)
> > > 
> > > Good news is that we can change s390-virtio to be just like the other
> > > transports. Although I'd like to understand why it was broken before.
> > > Maybe a guest change?
> > 
> > Or a compiler change? Try compiling some old release, see what happens.
> 
> Nope, same compiler as back then (old setup). My current guess is some
> change in other code.

bisect is you friend then :)

> > Anyway, let's move DEFINE_VIRTIO_COMMON_FEATURES into the base class
> > now.  Can you send a patch pls?
> 
> I think virtio-blk-s390 not working anymore will be easy to catch (it
> is used in some of the iotests). I'll move the features.

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Christian Borntraeger]

Am 29.04.2015 um 12:32 schrieb Michael S. Tsirkin:
> On Wed, Apr 29, 2015 at 10:52:15AM +0200, Cornelia Huck wrote:
>> On Wed, 29 Apr 2015 10:17:55 +0200
>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>
>>> Am 28.04.2015 um 20:32 schrieb Michael S. Tsirkin:
>>>> On Tue, Apr 28, 2015 at 08:14:44PM +0200, Michael S. Tsirkin wrote:
>>>>> On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
>>>>>> On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
>>>>>>> On Tue, 28 Apr 2015 14:16:40 +0100
>>>>>>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>>>>
>>>>>>>> On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>> The patches look correct to me too, but I want s390
>>>>>>>>> cleaned up so it does not include COMMON_FEATURES
>>>>>>>>> in 100 places, and I prefer merging it all together.
>>>>>>>>
>>>>>>>> It seems a bit harsh to ask Shannon to do s390 cleanup when
>>>>>>>> he doesn't have any access to s390 guests or test cases...
>>>>>>>> Making S390 put COMMON_FEATURES in the right places seems
>>>>>>>> to me like a separate bit of s390-specific cleanup.
>>>>>>>
>>>>>>> Yep, see my other reply... I'm not quite sure what's wrong with
>>>>>>> event_idx on virtio-blk for s390-virtio, or I would gladly make this
>>>>>>> consistent with the other transports. Any hints appreciated :)
>>>>>>
>>>>>> Is this still happening?
>>>>>>
>>>>>> It is possible that what was missing was
>>>>>> 92045d80badc43c9f95897aad675dc7ef17a3b3f
>>>>>> and/or
>>>>>> a281ebc11a6917fbc27e1a93bb5772cd14e241fc
>>>>>>
>>>>>
>>>>> Found this:
>>>>> http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
>>>>> so it's unlikely: these commits are from 2012, you saw
>>>>> issues in 2014.
>>>>>
>>>>> We really need to fix it. virtio 1 work will be much easier if
>>>>> we can just move features into virtio dev.
>>>
>>> Yes, we have to understand why event_idx breaks for the s390-virtio transport.
>>>>
>>>> I'm beginning to suspect this is a wrong implementation of barriers.
>>>> Questions:
>>>>     - which compiler to you use?
>>>>     - can you pls disassemble code for smp_wmb smp_rmb and smp_mb?
>>>>       They all must do br %r14 I think, and this is what
>>>>       s390x-linux-gnu-gcc generated for me:
>>>>         s390x-linux-gnu-gcc (GCC) 4.9.1
>>>
>>> s390 has strong memory ordering. Reads are in order, writes are in order. 
>>> bcr 14,0 or bcr 15,0 then only serialize the reads against the writes.
>>> So smp_rmb and smp_wmb can be implemented as no-ops like QEMU.
>>> If your change "fixes" the issue then we have a problem somewhere else
>>
>> And (surprise, surprise) virtio-blk now works - but it also works when
>> I back out the atomic.h change again. No barrier problems :)
>>
>> Good news is that we can change s390-virtio to be just like the other
>> transports. Although I'd like to understand why it was broken before.
>> Maybe a guest change?
> 
> Or a compiler change? Try compiling some old release, see what happens.
> Anyway, let's move DEFINE_VIRTIO_COMMON_FEATURES into the base class
> now.  Can you send a patch pls?

3.17 as guest fails, 3.18 as guest works. Not sure yet why.
 

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Wed, Apr 29, 2015 at 04:43:19PM +0200, Christian Borntraeger wrote:
> Am 29.04.2015 um 12:32 schrieb Michael S. Tsirkin:
> > On Wed, Apr 29, 2015 at 10:52:15AM +0200, Cornelia Huck wrote:
> >> On Wed, 29 Apr 2015 10:17:55 +0200
> >> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>
> >>> Am 28.04.2015 um 20:32 schrieb Michael S. Tsirkin:
> >>>> On Tue, Apr 28, 2015 at 08:14:44PM +0200, Michael S. Tsirkin wrote:
> >>>>> On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
> >>>>>> On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
> >>>>>>> On Tue, 28 Apr 2015 14:16:40 +0100
> >>>>>>> Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>>>>>
> >>>>>>>> On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>>>>> The patches look correct to me too, but I want s390
> >>>>>>>>> cleaned up so it does not include COMMON_FEATURES
> >>>>>>>>> in 100 places, and I prefer merging it all together.
> >>>>>>>>
> >>>>>>>> It seems a bit harsh to ask Shannon to do s390 cleanup when
> >>>>>>>> he doesn't have any access to s390 guests or test cases...
> >>>>>>>> Making S390 put COMMON_FEATURES in the right places seems
> >>>>>>>> to me like a separate bit of s390-specific cleanup.
> >>>>>>>
> >>>>>>> Yep, see my other reply... I'm not quite sure what's wrong with
> >>>>>>> event_idx on virtio-blk for s390-virtio, or I would gladly make this
> >>>>>>> consistent with the other transports. Any hints appreciated :)
> >>>>>>
> >>>>>> Is this still happening?
> >>>>>>
> >>>>>> It is possible that what was missing was
> >>>>>> 92045d80badc43c9f95897aad675dc7ef17a3b3f
> >>>>>> and/or
> >>>>>> a281ebc11a6917fbc27e1a93bb5772cd14e241fc
> >>>>>>
> >>>>>
> >>>>> Found this:
> >>>>> http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
> >>>>> so it's unlikely: these commits are from 2012, you saw
> >>>>> issues in 2014.
> >>>>>
> >>>>> We really need to fix it. virtio 1 work will be much easier if
> >>>>> we can just move features into virtio dev.
> >>>
> >>> Yes, we have to understand why event_idx breaks for the s390-virtio transport.
> >>>>
> >>>> I'm beginning to suspect this is a wrong implementation of barriers.
> >>>> Questions:
> >>>>     - which compiler to you use?
> >>>>     - can you pls disassemble code for smp_wmb smp_rmb and smp_mb?
> >>>>       They all must do br %r14 I think, and this is what
> >>>>       s390x-linux-gnu-gcc generated for me:
> >>>>         s390x-linux-gnu-gcc (GCC) 4.9.1
> >>>
> >>> s390 has strong memory ordering. Reads are in order, writes are in order. 
> >>> bcr 14,0 or bcr 15,0 then only serialize the reads against the writes.
> >>> So smp_rmb and smp_wmb can be implemented as no-ops like QEMU.
> >>> If your change "fixes" the issue then we have a problem somewhere else
> >>
> >> And (surprise, surprise) virtio-blk now works - but it also works when
> >> I back out the atomic.h change again. No barrier problems :)
> >>
> >> Good news is that we can change s390-virtio to be just like the other
> >> transports. Although I'd like to understand why it was broken before.
> >> Maybe a guest change?
> > 
> > Or a compiler change? Try compiling some old release, see what happens.
> > Anyway, let's move DEFINE_VIRTIO_COMMON_FEATURES into the base class
> > now.  Can you send a patch pls?
> 
> 3.17 as guest fails, 3.18 as guest works. Not sure yet why.
>  

Fascinating. block core changes? bisect will tell.

-- 
MST

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Christian Borntraeger]

Am 29.04.2015 um 20:35 schrieb Michael S. Tsirkin:
> On Wed, Apr 29, 2015 at 04:43:19PM +0200, Christian Borntraeger wrote:
>> Am 29.04.2015 um 12:32 schrieb Michael S. Tsirkin:
>>> On Wed, Apr 29, 2015 at 10:52:15AM +0200, Cornelia Huck wrote:
>>>> On Wed, 29 Apr 2015 10:17:55 +0200
>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>>
>>>>> Am 28.04.2015 um 20:32 schrieb Michael S. Tsirkin:
>>>>>> On Tue, Apr 28, 2015 at 08:14:44PM +0200, Michael S. Tsirkin wrote:
>>>>>>> On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
>>>>>>>> On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
>>>>>>>>> On Tue, 28 Apr 2015 14:16:40 +0100
>>>>>>>>> Peter Maydell <peter.maydell@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>>> On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>>>> The patches look correct to me too, but I want s390
>>>>>>>>>>> cleaned up so it does not include COMMON_FEATURES
>>>>>>>>>>> in 100 places, and I prefer merging it all together.
>>>>>>>>>>
>>>>>>>>>> It seems a bit harsh to ask Shannon to do s390 cleanup when
>>>>>>>>>> he doesn't have any access to s390 guests or test cases...
>>>>>>>>>> Making S390 put COMMON_FEATURES in the right places seems
>>>>>>>>>> to me like a separate bit of s390-specific cleanup.
>>>>>>>>>
>>>>>>>>> Yep, see my other reply... I'm not quite sure what's wrong with
>>>>>>>>> event_idx on virtio-blk for s390-virtio, or I would gladly make this
>>>>>>>>> consistent with the other transports. Any hints appreciated :)
>>>>>>>>
>>>>>>>> Is this still happening?
>>>>>>>>
>>>>>>>> It is possible that what was missing was
>>>>>>>> 92045d80badc43c9f95897aad675dc7ef17a3b3f
>>>>>>>> and/or
>>>>>>>> a281ebc11a6917fbc27e1a93bb5772cd14e241fc
>>>>>>>>
>>>>>>>
>>>>>>> Found this:
>>>>>>> http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
>>>>>>> so it's unlikely: these commits are from 2012, you saw
>>>>>>> issues in 2014.
>>>>>>>
>>>>>>> We really need to fix it. virtio 1 work will be much easier if
>>>>>>> we can just move features into virtio dev.
>>>>>
>>>>> Yes, we have to understand why event_idx breaks for the s390-virtio transport.
>>>>>>
>>>>>> I'm beginning to suspect this is a wrong implementation of barriers.
>>>>>> Questions:
>>>>>>     - which compiler to you use?
>>>>>>     - can you pls disassemble code for smp_wmb smp_rmb and smp_mb?
>>>>>>       They all must do br %r14 I think, and this is what
>>>>>>       s390x-linux-gnu-gcc generated for me:
>>>>>>         s390x-linux-gnu-gcc (GCC) 4.9.1
>>>>>
>>>>> s390 has strong memory ordering. Reads are in order, writes are in order. 
>>>>> bcr 14,0 or bcr 15,0 then only serialize the reads against the writes.
>>>>> So smp_rmb and smp_wmb can be implemented as no-ops like QEMU.
>>>>> If your change "fixes" the issue then we have a problem somewhere else
>>>>
>>>> And (surprise, surprise) virtio-blk now works - but it also works when
>>>> I back out the atomic.h change again. No barrier problems :)
>>>>
>>>> Good news is that we can change s390-virtio to be just like the other
>>>> transports. Although I'd like to understand why it was broken before.
>>>> Maybe a guest change?
>>>
>>> Or a compiler change? Try compiling some old release, see what happens.
>>> Anyway, let's move DEFINE_VIRTIO_COMMON_FEATURES into the base class
>>> now.  Can you send a patch pls?
>>
>> 3.17 as guest fails, 3.18 as guest works. Not sure yet why.
>>  
> 
> Fascinating. block core changes? bisect will tell.
> 

This commit made it work.

commit 7a11370e5e6c26566904bb7f08281093a3002ff2
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Wed Oct 15 10:22:30 2014 +1030

    virtio_blk: enable VQs early
    
    virtio spec requires drivers to set DRIVER_OK before using VQs.
    This is set automatically after probe returns, virtio block violated this
    rule by calling add_disk, which causes the VQ to be used directly within
    probe.
    
    To fix, call virtio_device_ready before using VQs.
    
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
    Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
    Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Wed, Apr 29, 2015 at 09:49:53PM +0200, Christian Borntraeger wrote:
> Am 29.04.2015 um 20:35 schrieb Michael S. Tsirkin:
> > On Wed, Apr 29, 2015 at 04:43:19PM +0200, Christian Borntraeger wrote:
> >> Am 29.04.2015 um 12:32 schrieb Michael S. Tsirkin:
> >>> On Wed, Apr 29, 2015 at 10:52:15AM +0200, Cornelia Huck wrote:
> >>>> On Wed, 29 Apr 2015 10:17:55 +0200
> >>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> >>>>
> >>>>> Am 28.04.2015 um 20:32 schrieb Michael S. Tsirkin:
> >>>>>> On Tue, Apr 28, 2015 at 08:14:44PM +0200, Michael S. Tsirkin wrote:
> >>>>>>> On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
> >>>>>>>> On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
> >>>>>>>>> On Tue, 28 Apr 2015 14:16:40 +0100
> >>>>>>>>> Peter Maydell <peter.maydell@linaro.org> wrote:
> >>>>>>>>>
> >>>>>>>>>> On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>>>>>>>>> The patches look correct to me too, but I want s390
> >>>>>>>>>>> cleaned up so it does not include COMMON_FEATURES
> >>>>>>>>>>> in 100 places, and I prefer merging it all together.
> >>>>>>>>>>
> >>>>>>>>>> It seems a bit harsh to ask Shannon to do s390 cleanup when
> >>>>>>>>>> he doesn't have any access to s390 guests or test cases...
> >>>>>>>>>> Making S390 put COMMON_FEATURES in the right places seems
> >>>>>>>>>> to me like a separate bit of s390-specific cleanup.
> >>>>>>>>>
> >>>>>>>>> Yep, see my other reply... I'm not quite sure what's wrong with
> >>>>>>>>> event_idx on virtio-blk for s390-virtio, or I would gladly make this
> >>>>>>>>> consistent with the other transports. Any hints appreciated :)
> >>>>>>>>
> >>>>>>>> Is this still happening?
> >>>>>>>>
> >>>>>>>> It is possible that what was missing was
> >>>>>>>> 92045d80badc43c9f95897aad675dc7ef17a3b3f
> >>>>>>>> and/or
> >>>>>>>> a281ebc11a6917fbc27e1a93bb5772cd14e241fc
> >>>>>>>>
> >>>>>>>
> >>>>>>> Found this:
> >>>>>>> http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
> >>>>>>> so it's unlikely: these commits are from 2012, you saw
> >>>>>>> issues in 2014.
> >>>>>>>
> >>>>>>> We really need to fix it. virtio 1 work will be much easier if
> >>>>>>> we can just move features into virtio dev.
> >>>>>
> >>>>> Yes, we have to understand why event_idx breaks for the s390-virtio transport.
> >>>>>>
> >>>>>> I'm beginning to suspect this is a wrong implementation of barriers.
> >>>>>> Questions:
> >>>>>>     - which compiler to you use?
> >>>>>>     - can you pls disassemble code for smp_wmb smp_rmb and smp_mb?
> >>>>>>       They all must do br %r14 I think, and this is what
> >>>>>>       s390x-linux-gnu-gcc generated for me:
> >>>>>>         s390x-linux-gnu-gcc (GCC) 4.9.1
> >>>>>
> >>>>> s390 has strong memory ordering. Reads are in order, writes are in order. 
> >>>>> bcr 14,0 or bcr 15,0 then only serialize the reads against the writes.
> >>>>> So smp_rmb and smp_wmb can be implemented as no-ops like QEMU.
> >>>>> If your change "fixes" the issue then we have a problem somewhere else
> >>>>
> >>>> And (surprise, surprise) virtio-blk now works - but it also works when
> >>>> I back out the atomic.h change again. No barrier problems :)
> >>>>
> >>>> Good news is that we can change s390-virtio to be just like the other
> >>>> transports. Although I'd like to understand why it was broken before.
> >>>> Maybe a guest change?
> >>>
> >>> Or a compiler change? Try compiling some old release, see what happens.
> >>> Anyway, let's move DEFINE_VIRTIO_COMMON_FEATURES into the base class
> >>> now.  Can you send a patch pls?
> >>
> >> 3.17 as guest fails, 3.18 as guest works. Not sure yet why.
> >>  
> > 
> > Fascinating. block core changes? bisect will tell.
> > 
> 
> This commit made it work.
> 
> commit 7a11370e5e6c26566904bb7f08281093a3002ff2
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Wed Oct 15 10:22:30 2014 +1030
> 
>     virtio_blk: enable VQs early
>     
>     virtio spec requires drivers to set DRIVER_OK before using VQs.
>     This is set automatically after probe returns, virtio block violated this
>     rule by calling add_disk, which causes the VQ to be used directly within
>     probe.
>     
>     To fix, call virtio_device_ready before using VQs.
>     
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>     Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>     Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

I guess this means s390 code somehow lost kicks that happened before
DRIVER_OK. Without event index you would typically get another one on
the next request.


-- 
MST

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Christian Borntraeger]

Am 29.04.2015 um 22:19 schrieb Michael S. Tsirkin:
[...]
>>
>> This commit made it work.
>>
>> commit 7a11370e5e6c26566904bb7f08281093a3002ff2
>> Author: Michael S. Tsirkin <mst@redhat.com>
>> Date:   Wed Oct 15 10:22:30 2014 +1030
>>
>>     virtio_blk: enable VQs early
>>     
>>     virtio spec requires drivers to set DRIVER_OK before using VQs.
>>     This is set automatically after probe returns, virtio block violated this
>>     rule by calling add_disk, which causes the VQ to be used directly within
>>     probe.
>>     
>>     To fix, call virtio_device_ready before using VQs.
>>     
>>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>     Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>>     Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> 
> I guess this means s390 code somehow lost kicks that happened before
> DRIVER_OK. Without event index you would typically get another one on
> the next request.

The problem is that feature updates are not a synchronous op for this 
transport. This transport syncs the guest feature bits (those from finalize)
on the set_status call. Before that qemu thinks that features are zero, which
means QEMU will not write the event index thus the 2nd kick will be lost.

This quick hack makes the problem go away with older kernels

diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 15b2c0f..25abb10 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -85,6 +85,14 @@ static int s390_virtio_hcall_notify(const uint64_t *args)
     if (mem > ram_size) {
         VirtIOS390Device *dev = s390_virtio_bus_find_vring(s390_bus, mem, &i);
         if (dev) {
+            /*
+             * older kernels will use the virtqueue before setting DRIVER_OK.
+             * In this case the feature bits are not yet up to date, meaning
+             * that several funny things can happen, e.g. the guest thinks
+             * EVENT_IDX is on and QEMU thinks its off. Force a feature sync.
+             */
+            if (dev->vdev->status != VIRTIO_CONFIG_S_DRIVER_OK)
+                s390_virtio_device_update_status(dev);
             virtio_queue_notify(dev->vdev, i);
         } else {
             r = -EINVAL;


Unless there are better ideas, I will respin that as a proper patch.

Christian

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Peter Maydell]

On 28 April 2015 at 19:14, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
>> On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
>> > Yep, see my other reply... I'm not quite sure what's wrong with
>> > event_idx on virtio-blk for s390-virtio, or I would gladly make this
>> > consistent with the other transports. Any hints appreciated :)
>>
>> Is this still happening?
>>
>> It is possible that what was missing was
>> 92045d80badc43c9f95897aad675dc7ef17a3b3f
>> and/or
>> a281ebc11a6917fbc27e1a93bb5772cd14e241fc
>>
>
> Found this:
> http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
> so it's unlikely: these commits are from 2012, you saw
> issues in 2014.
>
> We really need to fix it. virtio 1 work will be much easier if
> we can just move features into virtio dev.

If the comments in that thread are correct, it suggests that
*all* s390 virtio devices need to not have event_idx set, ie
this is not particularly special to virtio-blk. In that case
could we move the common properties to the base class where
they belong, but have the s390 virtio base class override
the properties to always suppress event-idx ?

-- PMM

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Michael S. Tsirkin]

On Tue, Apr 28, 2015 at 07:34:29PM +0100, Peter Maydell wrote:
> On 28 April 2015 at 19:14, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Tue, Apr 28, 2015 at 04:35:16PM +0200, Michael S. Tsirkin wrote:
> >> On Tue, Apr 28, 2015 at 03:24:19PM +0200, Cornelia Huck wrote:
> >> > Yep, see my other reply... I'm not quite sure what's wrong with
> >> > event_idx on virtio-blk for s390-virtio, or I would gladly make this
> >> > consistent with the other transports. Any hints appreciated :)
> >>
> >> Is this still happening?
> >>
> >> It is possible that what was missing was
> >> 92045d80badc43c9f95897aad675dc7ef17a3b3f
> >> and/or
> >> a281ebc11a6917fbc27e1a93bb5772cd14e241fc
> >>
> >
> > Found this:
> > http://thread.gmane.org/gmane.comp.emulators.qemu/280334/focus=280357
> > so it's unlikely: these commits are from 2012, you saw
> > issues in 2014.
> >
> > We really need to fix it. virtio 1 work will be much easier if
> > we can just move features into virtio dev.
> 
> If the comments in that thread are correct, it suggests that
> *all* s390 virtio devices need to not have event_idx set, ie
> this is not particularly special to virtio-blk. In that case
> could we move the common properties to the base class where
> they belong, but have the s390 virtio base class override
> the properties to always suppress event-idx ?
> 
> -- PMM

That would be a reasonable work-around, yes.
I still hope we can resolve it properly though.

-- 
MST

Re: [Qemu-devel] [PATCH v4 0/2] virtio: Move host features to backends

[Shannon Zhao]

On 2015/4/28 21:16, Peter Maydell wrote:
> On 28 April 2015 at 14:13, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > The patches look correct to me too, but I want s390
>> > cleaned up so it does not include COMMON_FEATURES
>> > in 100 places, and I prefer merging it all together.
> It seems a bit harsh to ask Shannon to do s390 cleanup when
> he doesn't have any access to s390 guests or test cases...
> Making S390 put COMMON_FEATURES in the right places seems
> to me like a separate bit of s390-specific cleanup.
> 
> (The other cleanup we could do after this patchset would
> be to just expand out the DEFINE_VIRTIO_NET/RNG/SCSI/etc_FEATURES
> macros which are all now used in exactly one place. But

right, will send another patchset to do this.

-- 
Shannon