| Message ID | 20150420133203.2fd6caff.cornelia.huck@de.ibm.com |
|---|---|
| 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 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.
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
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.
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)