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 <address@hidden> wrote:

> On 20 April 2015 at 15:08, Cornelia Huck <address@hidden> 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.