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

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

[Shannon Zhao]

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.

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.

This is also backward compatible for s390 and x86.

Shannon Zhao (4):
  hw/s390x/virtio-ccw: Add virtio_ccw_device_plugged for virtio-ccw
  hw/s390x/s390-virtio-bus: Add virtio_s390_device_plugged for
    s390-virtio
  hw/net/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             |  4 ++++
 hw/s390x/s390-virtio-bus.c      | 12 ++++++++++--
 hw/s390x/virtio-ccw.c           | 13 +++++++++++--
 hw/scsi/virtio-scsi.c           |  5 +++++
 hw/virtio/virtio-pci.c          |  2 --
 include/hw/virtio/virtio-net.h  |  1 +
 include/hw/virtio/virtio-scsi.h |  1 +
 7 files changed, 32 insertions(+), 6 deletions(-)

-- 
2.0.4

[Qemu-devel] [PATCH 1/4] hw/s390x/virtio-ccw: Add virtio_ccw_device_plugged for virtio-ccw

[Shannon Zhao]

Add virtio_ccw_device_plugged, it can be used to get backend's features.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/s390x/virtio-ccw.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 130535c..30ca377 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1395,6 +1395,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
     return 0;
 }
 
+/* This is called by virtio-bus just after the device is plugged. */
+static void virtio_ccw_device_plugged(DeviceState *d)
+{
+    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
+
+    /* Only the first 32 feature bits are used. */
+    dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus,
+                                                         dev->host_features[0]);
+}
+
 /**************** Virtio-ccw Bus Device Descriptions *******************/
 
 static Property virtio_ccw_net_properties[] = {
@@ -1711,6 +1721,7 @@ static void virtio_ccw_bus_class_init(ObjectClass *klass, void *data)
     k->load_queue = virtio_ccw_load_queue;
     k->save_config = virtio_ccw_save_config;
     k->load_config = virtio_ccw_load_config;
+    k->device_plugged = virtio_ccw_device_plugged;
 }
 
 static const TypeInfo virtio_ccw_bus_info = {
-- 
2.0.4

Re: [Qemu-devel] [PATCH 1/4] hw/s390x/virtio-ccw: Add virtio_ccw_device_plugged for virtio-ccw

[Cornelia Huck]

On Fri, 17 Apr 2015 20:13:43 +0800
Shannon Zhao <shannon.zhao@linaro.org> wrote:

> Add virtio_ccw_device_plugged, it can be used to get backend's features.
> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/s390x/virtio-ccw.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 130535c..30ca377 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1395,6 +1395,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
>      return 0;
>  }
> 
> +/* This is called by virtio-bus just after the device is plugged. */
> +static void virtio_ccw_device_plugged(DeviceState *d)
> +{
> +    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> +
> +    /* Only the first 32 feature bits are used. */
> +    dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus,
> +                                                         dev->host_features[0]);
> +}
> +

So how does this help? We already fetch the host features in the
realize function.

>  /**************** Virtio-ccw Bus Device Descriptions *******************/
> 
>  static Property virtio_ccw_net_properties[] = {

Re: [Qemu-devel] [PATCH 1/4] hw/s390x/virtio-ccw: Add virtio_ccw_device_plugged for virtio-ccw

[Shannon Zhao]

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

On Friday, 17 April 2015, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Fri, 17 Apr 2015 20:13:43 +0800
> Shannon Zhao <shannon.zhao@linaro.org> wrote:
>
>> Add virtio_ccw_device_plugged, it can be used to get backend's features.
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  hw/s390x/virtio-ccw.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index 130535c..30ca377 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -1395,6 +1395,16 @@ static int virtio_ccw_load_config(DeviceState *d,
QEMUFile *f)
>>      return 0;
>>  }
>>
>> +/* This is called by virtio-bus just after the device is plugged. */
>> +static void virtio_ccw_device_plugged(DeviceState *d)
>> +{
>> +    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
>> +
>> +    /* Only the first 32 feature bits are used. */
>> +    dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus,
>> +
 dev->host_features[0]);
>> +}
>> +
>
> So how does this help? We already fetch the host features in the
> realize function.
>

please see patch 4/4, in this patch we will move the properties to
backends. So the features can't fetch through realize function.
If you ask me why we need to move, it's because these properties actually
belongs to the backends and then we can support virtio-mmio to have these
properties.

>>  /**************** Virtio-ccw Bus Device Descriptions
*******************/
>>
>>  static Property virtio_ccw_net_properties[] = {
>
>

[-- Attachment #2: Type: text/html, Size: 2267 bytes --]

Re: [Qemu-devel] [PATCH 1/4] hw/s390x/virtio-ccw: Add virtio_ccw_device_plugged for virtio-ccw

[Peter Maydell]

On 17 April 2015 at 13:13, Shannon Zhao <shannon.zhao@linaro.org> wrote:
> Add virtio_ccw_device_plugged, it can be used to get backend's features.
>
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/s390x/virtio-ccw.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 130535c..30ca377 100644
> --- a/hw/s390x/virtio-ccw.c
> +++ b/hw/s390x/virtio-ccw.c
> @@ -1395,6 +1395,16 @@ static int virtio_ccw_load_config(DeviceState *d, QEMUFile *f)
>      return 0;
>  }
>
> +/* This is called by virtio-bus just after the device is plugged. */
> +static void virtio_ccw_device_plugged(DeviceState *d)
> +{
> +    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
> +
> +    /* Only the first 32 feature bits are used. */
> +    dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus,
> +                                                         dev->host_features[0]);
> +}

This means that this transport now calls virtio_bus_get_vdev_features
twice, which doesn't look right. In particular, we call it from
realize to set dev->host_features[0], and then add some features to
dev->host_features[0]. Then I think we will call the 'plugged'
method which will throw away those extra features.

So I think that if we need to call this from 'plugged'
rather than 'realize' we need to move all the code for
setting host_features from 'realize' to here.

But I'm confused about why this change is necessary --
don't the blk backends already use the "properties are
on the backend" approach, and they work with this transport?

-- PMM

Re: [Qemu-devel] [PATCH 1/4] hw/s390x/virtio-ccw: Add virtio_ccw_device_plugged for virtio-ccw

[Shannon Zhao]

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

On Friday, 17 April 2015, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 17 April 2015 at 13:13, Shannon Zhao <shannon.zhao@linaro.org> wrote:
>> Add virtio_ccw_device_plugged, it can be used to get backend's features.
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  hw/s390x/virtio-ccw.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
>> index 130535c..30ca377 100644
>> --- a/hw/s390x/virtio-ccw.c
>> +++ b/hw/s390x/virtio-ccw.c
>> @@ -1395,6 +1395,16 @@ static int virtio_ccw_load_config(DeviceState *d,
QEMUFile *f)
>>      return 0;
>>  }
>>
>> +/* This is called by virtio-bus just after the device is plugged. */
>> +static void virtio_ccw_device_plugged(DeviceState *d)
>> +{
>> +    VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d);
>> +
>> +    /* Only the first 32 feature bits are used. */
>> +    dev->host_features[0] = virtio_bus_get_vdev_features(&dev->bus,
>> +
 dev->host_features[0]);
>> +}
>
> This means that this transport now calls virtio_bus_get_vdev_features
> twice, which doesn't look right. In particular, we call it from
> realize to set dev->host_features[0], and then add some features to
> dev->host_features[0]. Then I think we will call the 'plugged'
> method which will throw away those extra features.
>
> So I think that if we need to call this from 'plugged'
> rather than 'realize' we need to move all the code for
> setting host_features from 'realize' to here.
>

So sorry, when I reply this mail I'm using mobile phone, no codes on hand.
So I didn't confirm that.

> But I'm confused about why this change is necessary --
> don't the blk backends already use the "properties are
> on the backend" approach, and they work with this transport?
>

Ok, maybe I missed that the transport already get the features when
realized. If so, this change is not necessary.

[-- Attachment #2: Type: text/html, Size: 2694 bytes --]

[Qemu-devel] [PATCH 2/4] hw/s390x/s390-virtio-bus: Add virtio_s390_device_plugged for s390-virtio

[Shannon Zhao]

Add virtio_s390_device_plugged, it can be used to get backend's features.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 hw/s390x/s390-virtio-bus.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 047c963..31fdf94 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -504,6 +504,15 @@ static unsigned virtio_s390_get_features(DeviceState *d)
     return dev->host_features;
 }
 
+/* This is called by virtio-bus just after the device is plugged. */
+static void virtio_s390_device_plugged(DeviceState *d)
+{
+    VirtIOS390Device *dev = to_virtio_s390_device(d);
+
+    dev->host_features = virtio_bus_get_vdev_features(&dev->bus,
+                                                      dev->host_features);
+}
+
 /**************** S390 Virtio Bus Device Descriptions *******************/
 
 static Property s390_virtio_net_properties[] = {
@@ -715,6 +724,7 @@ static void virtio_s390_bus_class_init(ObjectClass *klass, void *data)
     bus_class->max_dev = 1;
     k->notify = virtio_s390_notify;
     k->get_features = virtio_s390_get_features;
+    k->device_plugged = virtio_s390_device_plugged;
 }
 
 static const TypeInfo virtio_s390_bus_info = {
-- 
2.0.4

[Qemu-devel] [PATCH 3/4] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Shannon Zhao]

Move DEFINE_VIRTIO_NET_FEATURES to the backend virtio-net.
The transports just sync the host features from backend.

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

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 27adcc5..5d72e2d 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);
 
+    /* First 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)) {
@@ -1714,6 +1717,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 31fdf94..49c13e2 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -517,7 +517,6 @@ static void virtio_s390_device_plugged(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 30ca377..acd3844 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1409,7 +1409,6 @@ static void virtio_ccw_device_plugged(DeviceState *d)
 
 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..772244e 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(),
 };
 
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index 4c2fe83..5bee4df 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;
-- 
2.0.4

Re: [Qemu-devel] [PATCH 3/4] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Peter Maydell]

On 17 April 2015 at 13:13, Shannon Zhao <shannon.zhao@linaro.org> wrote:
> Move DEFINE_VIRTIO_NET_FEATURES to the backend virtio-net.
> The transports just sync the host features from backend.
>
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  hw/net/virtio-net.c            | 4 ++++
>  hw/s390x/s390-virtio-bus.c     | 1 -
>  hw/s390x/virtio-ccw.c          | 1 -
>  hw/virtio/virtio-pci.c         | 1 -
>  include/hw/virtio/virtio-net.h | 1 +
>  5 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 27adcc5..5d72e2d 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);
>
> +    /* First 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)) {
> @@ -1714,6 +1717,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 31fdf94..49c13e2 100644
> --- a/hw/s390x/s390-virtio-bus.c
> +++ b/hw/s390x/s390-virtio-bus.c
> @@ -517,7 +517,6 @@ static void virtio_s390_device_plugged(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(),
>  };

I'm confused. This seems to be the same "breaks backwards compatibility"
implementation you suggested to me off-list and which I said
wouldn't work.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/4] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Shannon Zhao]

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

On Friday, 17 April 2015, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 17 April 2015 at 13:13, Shannon Zhao <shannon.zhao@linaro.org> wrote:
>> Move DEFINE_VIRTIO_NET_FEATURES to the backend virtio-net.
>> The transports just sync the host features from backend.
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  hw/net/virtio-net.c            | 4 ++++
>>  hw/s390x/s390-virtio-bus.c     | 1 -
>>  hw/s390x/virtio-ccw.c          | 1 -
>>  hw/virtio/virtio-pci.c         | 1 -
>>  include/hw/virtio/virtio-net.h | 1 +
>>  5 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 27adcc5..5d72e2d 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);
>>
>> +    /* First 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)) {
>> @@ -1714,6 +1717,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 31fdf94..49c13e2 100644
>> --- a/hw/s390x/s390-virtio-bus.c
>> +++ b/hw/s390x/s390-virtio-bus.c
>> @@ -517,7 +517,6 @@ static void virtio_s390_device_plugged(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(),
>>  };
>
> I'm confused. This seems to be the same "breaks backwards compatibility"
> implementation you suggested to me off-list and which I said
> wouldn't work.
>


Hi Peter,

The backwards compatibility is that with this patch the virtio-net-pci can
still set these properties through the qemu comand line, right?

As I mentioned off list and in the cover letter, the virtio-net-pci has the
ability to access the child's(here is virtio-net) properties. So it look
like the wrapper has these properties.

> thanks
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 3452 bytes --]

Re: [Qemu-devel] [PATCH 3/4] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Peter Maydell]

On 17 April 2015 at 15:50, Shannon Zhao <shannon.zhao@linaro.org> wrote:
> The backwards compatibility is that with this patch the virtio-net-pci can
> still set these properties through the qemu comand line, right?
>
> As I mentioned off list and in the cover letter, the virtio-net-pci has the
> ability to access the child's(here is virtio-net) properties. So it look
> like the wrapper has these properties.

Hmm, you're right -- if I test (using "-device virtio-net-pci,help"
to list properties) they're all still there. I don't see how this
works, but it does seem to :-)

-- PMM

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

[Shannon Zhao]

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

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 49c13e2..f77ac7a 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -632,7 +632,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 acd3844..0ea252c 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1513,7 +1513,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 da0cff8..719740e 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -627,6 +627,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);
+
+    /* First sync all virtio-scsi possible supported features */
+    requested_features |= s->host_features;
     return requested_features;
 }
 
@@ -941,6 +945,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 772244e..7e2ac9e 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.0.4

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

[Cornelia Huck]

On Fri, 17 Apr 2015 20:13:42 +0800
Shannon Zhao <shannon.zhao@linaro.org> wrote:

[Some questions may be silly, but I'm not familiar with the virtio-mmio
code]

> 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.

So how does virtio-mmio expose any host features?

> 
> 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.

Is there a way virtio-mmio could make use of instance_init?

> 
> 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.
> 
> This is also backward compatible for s390 and x86.
> 
> Shannon Zhao (4):
>   hw/s390x/virtio-ccw: Add virtio_ccw_device_plugged for virtio-ccw
>   hw/s390x/s390-virtio-bus: Add virtio_s390_device_plugged for
>     s390-virtio
>   hw/net/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             |  4 ++++
>  hw/s390x/s390-virtio-bus.c      | 12 ++++++++++--
>  hw/s390x/virtio-ccw.c           | 13 +++++++++++--
>  hw/scsi/virtio-scsi.c           |  5 +++++
>  hw/virtio/virtio-pci.c          |  2 --
>  include/hw/virtio/virtio-net.h  |  1 +
>  include/hw/virtio/virtio-scsi.h |  1 +
>  7 files changed, 32 insertions(+), 6 deletions(-)
> 

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

[Peter Maydell]

On 17 April 2015 at 14:43, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Fri, 17 Apr 2015 20:13:42 +0800
> Shannon Zhao <shannon.zhao@linaro.org> wrote:
>
> [Some questions may be silly, but I'm not familiar with the virtio-mmio
> code]
>
>> 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.
>
> So how does virtio-mmio expose any host features?

The features are properties of the backend, not the transport.
So for devices where we didn't set these up as "properties
exist on the backend and the compatibility transport+backend
wrapper devices just forward those properties to the backend",
you can't set the properties. We got this right for some of
the backends (eg blk) but not all of them, I think.

NB: I haven't looked at the code in these patches yet, so this is
just the way I think it ought to work... it's not clear to me
yet why we need to change the s390 transports in this patchset.

-- PMM

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

[Cornelia Huck]

On Fri, 17 Apr 2015 15:00:45 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 17 April 2015 at 14:43, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > On Fri, 17 Apr 2015 20:13:42 +0800
> > Shannon Zhao <shannon.zhao@linaro.org> wrote:
> >
> > [Some questions may be silly, but I'm not familiar with the virtio-mmio
> > code]
> >
> >> 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.
> >
> > So how does virtio-mmio expose any host features?
> 
> The features are properties of the backend, not the transport.
> So for devices where we didn't set these up as "properties
> exist on the backend and the compatibility transport+backend
> wrapper devices just forward those properties to the backend",
> you can't set the properties. We got this right for some of
> the backends (eg blk) but not all of them, I think.

The reason why blk is ok is that it adds the feature bits in its
->get_features() callback. net expects the feature bits already present
and removes not supported ones and therefore requires
statically-defined bits somewhere.

If we move the feature bits to virtio-net and virtio-scsi, it should
work for virtio-mmio - but the feature bit propagation from the device
into the transport becomes a bit useless.

Could net and scsi add the feature bits dynamically in their
->get_features() callback instead? This should work for virtio-mmio as
well afaics.

In the end, we should probably end up with the same mechanism for all
device types.

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

[Peter Maydell]

On 20 April 2015 at 09:54, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> The reason why blk is ok is that it adds the feature bits in its
> ->get_features() callback. net expects the feature bits already present
> and removes not supported ones and therefore requires
> statically-defined bits somewhere.
>
> If we move the feature bits to virtio-net and virtio-scsi, it should
> work for virtio-mmio - but the feature bit propagation from the device
> into the transport becomes a bit useless.
>
> Could net and scsi add the feature bits dynamically in their
> ->get_features() callback instead? This should work for virtio-mmio as
> well afaics.

I think this is what Shannon's v2 patchset just posted does -- could
you check that it doesn't break s390 by accident?

> In the end, we should probably end up with the same mechanism for all
> device types.

Definitely agreed. I suspect we just missed the net and scsi devices
because getting this wrong doesn't make virtio-mmio totally unusable...

thanks
-- PMM

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

[Michael S. Tsirkin]

On Mon, Apr 20, 2015 at 10:54:11AM +0200, Cornelia Huck wrote:
> On Fri, 17 Apr 2015 15:00:45 +0100
> Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> > On 17 April 2015 at 14:43, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > > On Fri, 17 Apr 2015 20:13:42 +0800
> > > Shannon Zhao <shannon.zhao@linaro.org> wrote:
> > >
> > > [Some questions may be silly, but I'm not familiar with the virtio-mmio
> > > code]
> > >
> > >> 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.
> > >
> > > So how does virtio-mmio expose any host features?
> > 
> > The features are properties of the backend, not the transport.
> > So for devices where we didn't set these up as "properties
> > exist on the backend and the compatibility transport+backend
> > wrapper devices just forward those properties to the backend",
> > you can't set the properties. We got this right for some of
> > the backends (eg blk) but not all of them, I think.
> 
> The reason why blk is ok is that it adds the feature bits in its
> ->get_features() callback. net expects the feature bits already present
> and removes not supported ones and therefore requires
> statically-defined bits somewhere.
> 
> If we move the feature bits to virtio-net and virtio-scsi, it should
> work for virtio-mmio - but the feature bit propagation from the device
> into the transport becomes a bit useless.
> 
> Could net and scsi add the feature bits dynamically in their
> ->get_features() callback instead? This should work for virtio-mmio as
> well afaics.
> 
> In the end, we should probably end up with the same mechanism for all
> device types.

I think I would also prefer that the host features live in the
generic virtio device structure. This would make it possible
e.g. to validate guest features on vm load in generic code.

-- 
MST

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

[Cornelia Huck]

On Mon, 20 Apr 2015 11:12:37 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Apr 20, 2015 at 10:54:11AM +0200, Cornelia Huck wrote:
> > On Fri, 17 Apr 2015 15:00:45 +0100
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> > 
> > > On 17 April 2015 at 14:43, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > > > On Fri, 17 Apr 2015 20:13:42 +0800
> > > > Shannon Zhao <shannon.zhao@linaro.org> wrote:
> > > >
> > > > [Some questions may be silly, but I'm not familiar with the virtio-mmio
> > > > code]
> > > >
> > > >> 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.
> > > >
> > > > So how does virtio-mmio expose any host features?
> > > 
> > > The features are properties of the backend, not the transport.
> > > So for devices where we didn't set these up as "properties
> > > exist on the backend and the compatibility transport+backend
> > > wrapper devices just forward those properties to the backend",
> > > you can't set the properties. We got this right for some of
> > > the backends (eg blk) but not all of them, I think.
> > 
> > The reason why blk is ok is that it adds the feature bits in its
> > ->get_features() callback. net expects the feature bits already present
> > and removes not supported ones and therefore requires
> > statically-defined bits somewhere.
> > 
> > If we move the feature bits to virtio-net and virtio-scsi, it should
> > work for virtio-mmio - but the feature bit propagation from the device
> > into the transport becomes a bit useless.
> > 
> > Could net and scsi add the feature bits dynamically in their
> > ->get_features() callback instead? This should work for virtio-mmio as
> > well afaics.
> > 
> > In the end, we should probably end up with the same mechanism for all
> > device types.
> 
> I think I would also prefer that the host features live in the
> generic virtio device structure. 

You mean in the virtio device instead of the proxy device? I think that
makes sense conceptually, provided we get the realize/plug sequence
correct.

> This would make it possible
> e.g. to validate guest features on vm load in generic code.
> 

Doesn't virtio_load() already do some validation?

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

[Michael S. Tsirkin]

On Mon, Apr 20, 2015 at 02:14:30PM +0200, Cornelia Huck wrote:
> On Mon, 20 Apr 2015 11:12:37 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Apr 20, 2015 at 10:54:11AM +0200, Cornelia Huck wrote:
> > > On Fri, 17 Apr 2015 15:00:45 +0100
> > > Peter Maydell <peter.maydell@linaro.org> wrote:
> > > 
> > > > On 17 April 2015 at 14:43, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > > > > On Fri, 17 Apr 2015 20:13:42 +0800
> > > > > Shannon Zhao <shannon.zhao@linaro.org> wrote:
> > > > >
> > > > > [Some questions may be silly, but I'm not familiar with the virtio-mmio
> > > > > code]
> > > > >
> > > > >> 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.
> > > > >
> > > > > So how does virtio-mmio expose any host features?
> > > > 
> > > > The features are properties of the backend, not the transport.
> > > > So for devices where we didn't set these up as "properties
> > > > exist on the backend and the compatibility transport+backend
> > > > wrapper devices just forward those properties to the backend",
> > > > you can't set the properties. We got this right for some of
> > > > the backends (eg blk) but not all of them, I think.
> > > 
> > > The reason why blk is ok is that it adds the feature bits in its
> > > ->get_features() callback. net expects the feature bits already present
> > > and removes not supported ones and therefore requires
> > > statically-defined bits somewhere.
> > > 
> > > If we move the feature bits to virtio-net and virtio-scsi, it should
> > > work for virtio-mmio - but the feature bit propagation from the device
> > > into the transport becomes a bit useless.
> > > 
> > > Could net and scsi add the feature bits dynamically in their
> > > ->get_features() callback instead? This should work for virtio-mmio as
> > > well afaics.
> > > 
> > > In the end, we should probably end up with the same mechanism for all
> > > device types.
> > 
> > I think I would also prefer that the host features live in the
> > generic virtio device structure. 
> 
> You mean in the virtio device instead of the proxy device?

Yes.

> I think that
> makes sense conceptually, provided we get the realize/plug sequence
> correct.
> 
> > This would make it possible
> > e.g. to validate guest features on vm load in generic code.
> > 
> 
> Doesn't virtio_load() already do some validation?

You are right here.

-- 
MST