| Message ID | 1429518001-1040-2-git-send-email-shannon.zhao@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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;