qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Cornelia Huck
Subject: Re: [Qemu-devel] [PATCH v2 1/2] hw/net/virtio-net: Move DEFINE_VIRTIO_NET_FEATURES to virtio-net
Date: Mon, 20 Apr 2015 16:08:43 +0200

On Mon, 20 Apr 2015 21:32:52 +0800
Shannon Zhao <address@hidden> wrote:

> 
> 
> On 2015/4/20 19:32, Cornelia Huck wrote:
> > On Mon, 20 Apr 2015 16:20:00 +0800
> > address@hidden wrote:
> >
> >> From: Shannon Zhao <address@hidden>
> >>
> >> Move DEFINE_VIRTIO_NET_FEATURES to the backend virtio-net.
> >> The transports just sync the host features from backend.
> >>
> >> Signed-off-by: Shannon Zhao <address@hidden>
> >> Signed-off-by: Shannon Zhao <address@hidden>
> >> ---
> >>   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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]