* [Qemu-devel] [PATCH v2 0/2] virtio: Move host features to backends
@ 2015-04-20 8:19 shannon.zhao
2015-04-20 8:20 ` [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net shannon.zhao
2015-04-20 8:20 ` [Qemu-devel] [PATCH v2 2/2] virtio-scsi: Move DEFINE_VIRTIO_SCSI_FEATURES to virtio-scsi shannon.zhao
0 siblings, 2 replies; 12+ messages in thread
From: shannon.zhao @ 2015-04-20 8:19 UTC (permalink / raw)
To: qemu-devel, peter.maydell, cornelia.huck, mst, pbonzini,
christoffer.dall
Cc: hangaohuai, peter.huangpeng, zhaoshenglong
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
2015-04-20 8:19 [Qemu-devel] [PATCH v2 0/2] virtio: Move host features to backends shannon.zhao
@ 2015-04-20 8:20 ` shannon.zhao
2015-04-20 9:20 ` Cornelia Huck
2015-04-20 11:32 ` Cornelia Huck
2015-04-20 8:20 ` [Qemu-devel] [PATCH v2 2/2] virtio-scsi: Move DEFINE_VIRTIO_SCSI_FEATURES to virtio-scsi shannon.zhao
1 sibling, 2 replies; 12+ messages in thread
From: shannon.zhao @ 2015-04-20 8:20 UTC (permalink / raw)
To: qemu-devel, peter.maydell, cornelia.huck, mst, pbonzini,
christoffer.dall
Cc: hangaohuai, peter.huangpeng, zhaoshenglong
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] virtio-scsi: Move DEFINE_VIRTIO_SCSI_FEATURES to virtio-scsi
2015-04-20 8:19 [Qemu-devel] [PATCH v2 0/2] virtio: Move host features to backends shannon.zhao
2015-04-20 8:20 ` [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net shannon.zhao
@ 2015-04-20 8:20 ` shannon.zhao
1 sibling, 0 replies; 12+ messages in thread
From: shannon.zhao @ 2015-04-20 8:20 UTC (permalink / raw)
To: qemu-devel, peter.maydell, cornelia.huck, mst, pbonzini,
christoffer.dall
Cc: hangaohuai, peter.huangpeng, zhaoshenglong
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
2015-04-20 8:20 ` [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net shannon.zhao
@ 2015-04-20 9:20 ` Cornelia Huck
2015-04-20 11:32 ` Cornelia Huck
1 sibling, 0 replies; 12+ messages in thread
From: Cornelia Huck @ 2015-04-20 9:20 UTC (permalink / raw)
To: shannon.zhao
Cc: peter.maydell, hangaohuai, mst, peter.huangpeng, qemu-devel,
zhaoshenglong, pbonzini, christoffer.dall
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
2015-04-20 8:20 ` [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net shannon.zhao
2015-04-20 9:20 ` Cornelia Huck
@ 2015-04-20 11:32 ` Cornelia Huck
2015-04-20 13:32 ` Shannon Zhao
2015-04-21 1:43 ` Shannon Zhao
1 sibling, 2 replies; 12+ messages in thread
From: Cornelia Huck @ 2015-04-20 11:32 UTC (permalink / raw)
To: shannon.zhao
Cc: peter.maydell, hangaohuai, mst, peter.huangpeng, qemu-devel,
zhaoshenglong, pbonzini, christoffer.dall
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.
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
2015-04-20 11:32 ` Cornelia Huck
@ 2015-04-20 13:32 ` Shannon Zhao
2015-04-20 14:08 ` Cornelia Huck
2015-04-21 1:43 ` Shannon Zhao
1 sibling, 1 reply; 12+ messages in thread
From: Shannon Zhao @ 2015-04-20 13:32 UTC (permalink / raw)
To: Cornelia Huck
Cc: peter.maydell, hangaohuai, mst, peter.huangpeng, qemu-devel,
zhaoshenglong, pbonzini, christoffer.dall
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
2015-04-20 13:32 ` Shannon Zhao
@ 2015-04-20 14:08 ` Cornelia Huck
2015-04-20 14:34 ` Peter Maydell
0 siblings, 1 reply; 12+ messages in thread
From: Cornelia Huck @ 2015-04-20 14:08 UTC (permalink / raw)
To: Shannon Zhao
Cc: peter.maydell, hangaohuai, mst, peter.huangpeng, qemu-devel,
zhaoshenglong, pbonzini, christoffer.dall
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
2015-04-20 14:08 ` Cornelia Huck
@ 2015-04-20 14:34 ` Peter Maydell
2015-04-20 16:44 ` Cornelia Huck
0 siblings, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2015-04-20 14:34 UTC (permalink / raw)
To: Cornelia Huck
Cc: hangaohuai, Michael S. Tsirkin, Huangpeng (Peter),
QEMU Developers, Shannon Zhao, Shannon Zhao, Paolo Bonzini,
Christoffer Dall
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
2015-04-20 14:34 ` Peter Maydell
@ 2015-04-20 16:44 ` Cornelia Huck
0 siblings, 0 replies; 12+ messages in thread
From: Cornelia Huck @ 2015-04-20 16:44 UTC (permalink / raw)
To: Peter Maydell
Cc: hangaohuai, Michael S. Tsirkin, Huangpeng (Peter),
QEMU Developers, Shannon Zhao, Shannon Zhao, Paolo Bonzini,
Christoffer Dall
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
2015-04-20 11:32 ` Cornelia Huck
2015-04-20 13:32 ` Shannon Zhao
@ 2015-04-21 1:43 ` Shannon Zhao
2015-04-21 7:30 ` Cornelia Huck
1 sibling, 1 reply; 12+ messages in thread
From: Shannon Zhao @ 2015-04-21 1:43 UTC (permalink / raw)
To: Cornelia Huck
Cc: peter.maydell, hangaohuai, mst, peter.huangpeng, qemu-devel,
zhaoshenglong, pbonzini, christoffer.dall
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.
>
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
2015-04-21 1:43 ` Shannon Zhao
@ 2015-04-21 7:30 ` Cornelia Huck
2015-04-21 10:33 ` Shannon Zhao
0 siblings, 1 reply; 12+ messages in thread
From: Cornelia Huck @ 2015-04-21 7:30 UTC (permalink / raw)
To: Shannon Zhao
Cc: peter.maydell, hangaohuai, mst, peter.huangpeng, qemu-devel,
zhaoshenglong, pbonzini, christoffer.dall
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
2015-04-21 7:30 ` Cornelia Huck
@ 2015-04-21 10:33 ` Shannon Zhao
0 siblings, 0 replies; 12+ messages in thread
From: Shannon Zhao @ 2015-04-21 10:33 UTC (permalink / raw)
To: Cornelia Huck
Cc: peter.maydell, hangaohuai, mst, peter.huangpeng, qemu-devel,
zhaoshenglong, pbonzini, christoffer.dall
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-04-21 10:34 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-20 8:19 [Qemu-devel] [PATCH v2 0/2] virtio: Move host features to backends shannon.zhao
2015-04-20 8:20 ` [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net shannon.zhao
2015-04-20 9:20 ` Cornelia Huck
2015-04-20 11:32 ` Cornelia Huck
2015-04-20 13:32 ` Shannon Zhao
2015-04-20 14:08 ` Cornelia Huck
2015-04-20 14:34 ` Peter Maydell
2015-04-20 16:44 ` Cornelia Huck
2015-04-21 1:43 ` Shannon Zhao
2015-04-21 7:30 ` Cornelia Huck
2015-04-21 10:33 ` Shannon Zhao
2015-04-20 8:20 ` [Qemu-devel] [PATCH v2 2/2] virtio-scsi: Move DEFINE_VIRTIO_SCSI_FEATURES to virtio-scsi shannon.zhao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.