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

[Qemu-devel] [PATCH v2 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 v1:
  * drop unnecessary change of adding device_plugged hook for
    virtio-ccw and s390-virtio-bus (Cornelia)

Shannon Zhao (2):
  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      | 2 --
 hw/s390x/virtio-ccw.c           | 2 --
 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, 11 insertions(+), 6 deletions(-)

-- 
2.0.4

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

[shannon.zhao]

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

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 047c963..1987873 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -508,7 +508,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 130535c..d9dc80c 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1399,7 +1399,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..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

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

[shannon.zhao]

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

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 1987873..72c205e 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -623,7 +623,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 d9dc80c..754ce18 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -1503,7 +1503,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 v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Cornelia Huck]

On Mon, 20 Apr 2015 16:20:00 +0800
shannon.zhao@linaro.org wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> 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(-)

Hm, this seems to break networking via virtio-ccw for me. I'll try to
find out more.

Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Cornelia Huck]

On Mon, 20 Apr 2015 16:20:00 +0800
shannon.zhao@linaro.org wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> 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(-)

I need the following change to make this work for virtio-ccw:

diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 2252789..7a2bdff 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));
@@ -790,6 +789,7 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
     }
 
     virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
+    virtio_net_set_config_size(&dev->vdev, ccw_dev->host_features[0]);
 }
 
 static void virtio_ccw_net_instance_init(Object *obj)

host_features used to be statically populated, so
virtio_net_set_config_size() was able to use the various feature bits
for its decisions.

It does not seem quite right, however, since the set of feature bits
had not been through virtio-net's ->get_features() routine (or the
feature bit manipulations in virtio-ccw's realize() routine) - it was
just good enough.

Maybe the right place for calling set_config_size() would be in a
virtio-net specific ->plugged() callback?

I'm not sure why virtio-pci works, but they have a different topology
with pci device and virtio-pci device separate, so it might work out
there.

Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Shannon Zhao]

On 2015/4/20 19:32, Cornelia Huck wrote:
> On Mon, 20 Apr 2015 16:20:00 +0800
> shannon.zhao@linaro.org wrote:
>
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> 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(-)
>
> I need the following change to make this work for virtio-ccw:
>
> diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 2252789..7a2bdff 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));
> @@ -790,6 +789,7 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
>       }
>
>       virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
> +    virtio_net_set_config_size(&dev->vdev, ccw_dev->host_features[0]);
>   }
>
>   static void virtio_ccw_net_instance_init(Object *obj)
>
> host_features used to be statically populated, so
> virtio_net_set_config_size() was able to use the various feature bits
> for its decisions.
>
> It does not seem quite right, however, since the set of feature bits
> had not been through virtio-net's ->get_features() routine (or the
> feature bit manipulations in virtio-ccw's realize() routine) - it was
> just good enough.
>
> Maybe the right place for calling set_config_size() would be in a
> virtio-net specific ->plugged() callback?
>
> I'm not sure why virtio-pci works, but they have a different topology
> with pci device and virtio-pci device separate, so it might work out
> there.
>

The virtio-pci works because it calls device_plugged hook to get the 
features and this hook is called before realized function. When calling 
virtio_net_set_config_size the features are already synced, so it works.

I think maybe we should call device_plugged hook to get the features in 
virtio-ccw other than get them in realized function. So the virtio-ccw, 
virtio-pci and virtio-mmio use same ways.

Thanks,
Shannon

Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Cornelia Huck]

On Mon, 20 Apr 2015 21:32:52 +0800
Shannon Zhao <shannon.zhao@linaro.org> wrote:

> 
> 
> On 2015/4/20 19:32, Cornelia Huck wrote:
> > On Mon, 20 Apr 2015 16:20:00 +0800
> > shannon.zhao@linaro.org wrote:
> >
> >> From: Shannon Zhao <shannon.zhao@linaro.org>
> >>
> >> 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(-)
> >
> > I need the following change to make this work for virtio-ccw:
> >
> > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > index 2252789..7a2bdff 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));
> > @@ -790,6 +789,7 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
> >       }
> >
> >       virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
> > +    virtio_net_set_config_size(&dev->vdev, ccw_dev->host_features[0]);
> >   }
> >
> >   static void virtio_ccw_net_instance_init(Object *obj)
> >
> > host_features used to be statically populated, so
> > virtio_net_set_config_size() was able to use the various feature bits
> > for its decisions.
> >
> > It does not seem quite right, however, since the set of feature bits
> > had not been through virtio-net's ->get_features() routine (or the
> > feature bit manipulations in virtio-ccw's realize() routine) - it was
> > just good enough.
> >
> > Maybe the right place for calling set_config_size() would be in a
> > virtio-net specific ->plugged() callback?
> >
> > I'm not sure why virtio-pci works, but they have a different topology
> > with pci device and virtio-pci device separate, so it might work out
> > there.
> >
> 
> The virtio-pci works because it calls device_plugged hook to get the 
> features and this hook is called before realized function. When calling 
> virtio_net_set_config_size the features are already synced, so it works.
> 
> I think maybe we should call device_plugged hook to get the features in 
> virtio-ccw other than get them in realized function. So the virtio-ccw, 
> virtio-pci and virtio-mmio use same ways.

Hmm... isn't ->plugged() called after ->realize()?

Maybe I'm just confused, so let's try to understand the callchain :)

VirtIONetCcw is realized
  -> feature bits are used
  -> embedded VirtIODevice is realized
  -> VirtioCcwDevice is realized
     -> features are populated

My understanding was that ->plugged() happened after step 3, but
re-reading, it might already happen after step 2... very confusing.
(This still would be too late for the feature bits, and we don't set up
the parent bus before step 2.)

virtio-pci might be slightly different due to a different topology, I
think.

I'm not opposed to moving setting up the features into ->plugged(), but
I'm not sure it helps.

Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Peter Maydell]

On 20 April 2015 at 15:08, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> Hmm... isn't ->plugged() called after ->realize()?
>
> Maybe I'm just confused, so let's try to understand the callchain :)
>
> VirtIONetCcw is realized
>   -> feature bits are used
>   -> embedded VirtIODevice is realized
>   -> VirtioCcwDevice is realized
>      -> features are populated
>
> My understanding was that ->plugged() happened after step 3, but
> re-reading, it might already happen after step 2... very confusing.
> (This still would be too late for the feature bits, and we don't set up
> the parent bus before step 2.)

plugged gets called when the virtio backend device is realized
(from the hw/virtio/virtio.c base class realize method).
For virtio-ccw, your virtio_ccw_net_realize function does this
(by setting the 'realized' property on the backend vdev to true).
Since it does this before it calls virtio_ccw_device_realize()
you get the ordering:
 * virtio_ccw_net_realize early stuff
 * virtio-net backend realize
 * virtio_ccw plugged method called (if you had one)
 * virtio_ccw_device_realize called (manually by the subclass)

That's probably not a very helpful order...

> virtio-pci might be slightly different due to a different topology, I
> think.

virtio-pci has three differences:
 (1) its generic 'virtio_pci_realize' is a method on a common
base class, which then invokes the subclass realize
(rather than having the subclass realize call the parent
realize function as virtio-ccw does)
 (2) it implements a plugged method and a lot of work is done there
 (3) the virtio_net_pci_realize realizes the backend as its
final action, not in the middle of doing other things

So the order here is:
 * virtio_pci_realize (as base class realize method)
 * virtio_net_pci_realize
 * virtio-net backend realize
 * virtio_pci plugged method called

> I'm not opposed to moving setting up the features into ->plugged(), but
> I'm not sure it helps.

Conceptually I think if you have code which relies on the
backend existing, it is better placed in the plugged() method
rather than trying to implement the realize method as a sort
of two-stage thing with the backend-realize done in the middle
and manual calls from the subclass back into the base class
done afterwards.

You can probably fix the specific weirdnesses here by
being a bit more careful about what all the
virtio_ccw_net_realize &c functions do before realizing
the backend and what they do afterwards. But it might
be long term cleaner to structure things like virtio-pci.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Cornelia Huck]

On Mon, 20 Apr 2015 15:34:06 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 20 April 2015 at 15:08, Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> > Hmm... isn't ->plugged() called after ->realize()?
> >
> > Maybe I'm just confused, so let's try to understand the callchain :)
> >
> > VirtIONetCcw is realized
> >   -> feature bits are used
> >   -> embedded VirtIODevice is realized
> >   -> VirtioCcwDevice is realized
> >      -> features are populated
> >
> > My understanding was that ->plugged() happened after step 3, but
> > re-reading, it might already happen after step 2... very confusing.
> > (This still would be too late for the feature bits, and we don't set up
> > the parent bus before step 2.)
> 
> plugged gets called when the virtio backend device is realized
> (from the hw/virtio/virtio.c base class realize method).
> For virtio-ccw, your virtio_ccw_net_realize function does this
> (by setting the 'realized' property on the backend vdev to true).
> Since it does this before it calls virtio_ccw_device_realize()
> you get the ordering:
>  * virtio_ccw_net_realize early stuff
>  * virtio-net backend realize
>  * virtio_ccw plugged method called (if you had one)
>  * virtio_ccw_device_realize called (manually by the subclass)
> 
> That's probably not a very helpful order...

Indeed.

> 
> > virtio-pci might be slightly different due to a different topology, I
> > think.
> 
> virtio-pci has three differences:
>  (1) its generic 'virtio_pci_realize' is a method on a common
> base class, which then invokes the subclass realize
> (rather than having the subclass realize call the parent
> realize function as virtio-ccw does)

That actually makes a lot of sense. I'll put checking if I can do
something similar for virtio-ccw on my todo list.

>  (2) it implements a plugged method and a lot of work is done there

I'm not sure how much we can actually do in a plugged method for
virtio-ccw, but it's probably worth checking out.

>  (3) the virtio_net_pci_realize realizes the backend as its
> final action, not in the middle of doing other things
> 
> So the order here is:
>  * virtio_pci_realize (as base class realize method)
>  * virtio_net_pci_realize
>  * virtio-net backend realize
>  * virtio_pci plugged method called

So if the features are propagated in the plugged method, virtio-pci
should have the same problem?

> 
> > I'm not opposed to moving setting up the features into ->plugged(), but
> > I'm not sure it helps.
> 
> Conceptually I think if you have code which relies on the
> backend existing, it is better placed in the plugged() method
> rather than trying to implement the realize method as a sort
> of two-stage thing with the backend-realize done in the middle
> and manual calls from the subclass back into the base class
> done afterwards.
> 
> You can probably fix the specific weirdnesses here by
> being a bit more careful about what all the
> virtio_ccw_net_realize &c functions do before realizing
> the backend and what they do afterwards. But it might
> be long term cleaner to structure things like virtio-pci.

Let me see what makes sense. One of the problems is that we don't have
a clean split between the hardware device (along the lines of a pci
device) and the virtio proxy - which means that the virtio-ccw realize
method does a lot of things that have more to do with channel devices
than with virtio.

Modelling on the old s390-virtio transport is still another problem,
and I don't want to do anything there beyond the minimum changes to
make it work.

Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Shannon Zhao]

On 2015/4/20 19:32, Cornelia Huck wrote:
> On Mon, 20 Apr 2015 16:20:00 +0800
> shannon.zhao@linaro.org wrote:
> 
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> 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(-)
> 
> I need the following change to make this work for virtio-ccw:
>


Maybe we can use following patch. This moves virtio_net_set_config_size to
virtio_net_device_realize function. As the features are moved to virtio-net,
so we should set the config_size in virtio-net too. And this can be useful to
virtio-mmio which now doesn't call virtio_net_set_config_size in
virtio-mmio's realize function.

Cornelia, could you check if this works on s390? Thanks.


diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 27ec5b1..36ba027 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1588,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);
diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index 1987873..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));
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 803526a..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));
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 772244e..c6b99f9 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1369,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/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> index 2252789..7a2bdff 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));
> @@ -790,6 +789,7 @@ static void virtio_ccw_net_realize(VirtioCcwDevice *ccw_dev, Error **errp)
>      }
>  
>      virtio_ccw_device_realize(ccw_dev, VIRTIO_DEVICE(vdev), errp);
> +    virtio_net_set_config_size(&dev->vdev, ccw_dev->host_features[0]);
>  }
>  
>  static void virtio_ccw_net_instance_init(Object *obj)
> 
> host_features used to be statically populated, so
> virtio_net_set_config_size() was able to use the various feature bits
> for its decisions.
> 
> It does not seem quite right, however, since the set of feature bits
> had not been through virtio-net's ->get_features() routine (or the
> feature bit manipulations in virtio-ccw's realize() routine) - it was
> just good enough.
> 
> Maybe the right place for calling set_config_size() would be in a
> virtio-net specific ->plugged() callback?
> 
> I'm not sure why virtio-pci works, but they have a different topology
> with pci device and virtio-pci device separate, so it might work out
> there.
> 

Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Cornelia Huck]

On Tue, 21 Apr 2015 09:43:36 +0800
Shannon Zhao <shannon.zhao@linaro.org> wrote:

> On 2015/4/20 19:32, Cornelia Huck wrote:
> > On Mon, 20 Apr 2015 16:20:00 +0800
> > shannon.zhao@linaro.org wrote:
> > 
> >> From: Shannon Zhao <shannon.zhao@linaro.org>
> >>
> >> 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(-)
> > 
> > I need the following change to make this work for virtio-ccw:
> >
> 
> 
> Maybe we can use following patch. This moves virtio_net_set_config_size to
> virtio_net_device_realize function. As the features are moved to virtio-net,
> so we should set the config_size in virtio-net too. And this can be useful to
> virtio-mmio which now doesn't call virtio_net_set_config_size in
> virtio-mmio's realize function.

I think this makes sense.

> 
> Cornelia, could you check if this works on s390? Thanks.

Networking works again via virtio-ccw with this patch on top.

I'll still try to figure out a better sequence for realizing/plugging
virtio-ccw devices, but I think that is orthogonal to this patch.

Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net

[Shannon Zhao]

On 2015/4/21 15:30, Cornelia Huck wrote:
> On Tue, 21 Apr 2015 09:43:36 +0800
> Shannon Zhao <shannon.zhao@linaro.org> wrote:
> 
>> On 2015/4/20 19:32, Cornelia Huck wrote:
>>> On Mon, 20 Apr 2015 16:20:00 +0800
>>> shannon.zhao@linaro.org wrote:
>>>
>>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>
>>>> 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(-)
>>>
>>> I need the following change to make this work for virtio-ccw:
>>>
>>
>>
>> Maybe we can use following patch. This moves virtio_net_set_config_size to
>> virtio_net_device_realize function. As the features are moved to virtio-net,
>> so we should set the config_size in virtio-net too. And this can be useful to
>> virtio-mmio which now doesn't call virtio_net_set_config_size in
>> virtio-mmio's realize function.
> 
> I think this makes sense.
> 
>>
>> Cornelia, could you check if this works on s390? Thanks.
> 
> Networking works again via virtio-ccw with this patch on top.
> 
> I'll still try to figure out a better sequence for realizing/plugging
> virtio-ccw devices, but I think that is orthogonal to this patch.
> 

Cornelia, thanks for your help :)
Will add this and send them as v3.

-- 
Shannon