[Qemu-devel] Re: [PATCH 0/9] Virtio cleanups

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Mar 18, 2010 at 06:08:06PM +0100, Juan Quintela wrote:
> Gerd Hoffmann <kraxel@redhat.com> wrote:
> >   Hi,
> >
> >> But I still think that this is independent that VirtIO being 1st (or
> >> not) memer of VirtIOBlock.
> >
> > There is no strong reason for this other than memory allocation.  As
> > long as virtio_common_init() does the allocation there is no way
> > around VirtIODevice being the first member.  If this changes (and we
> > must change it if we want embed VirtIODevice and superclasses into
> > other structs) no reason is left.
> 
> It is, a 1st look at virtio-pci.c shows <removing the savevm stuff>
> 
> static void virtio_pci_reset(DeviceState *d)
> {
>     VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
>     virtio_reset(proxy->vdev);
>     msix_reset(&proxy->pci_dev);
> }
> 
> We have to call virtio_reset() that needs a virtio object, and the only
> thing that we have is a VirtIOPCIProxy.
>  We would have to implement
> something like:
> 
> struct VirtioPCIProxy_fields {
>        .... current VirtioPCIProxy fields
> }
> 
> struct VirtioPCIProxy {
>        struct VirtioPCIProxy_fields fields;
>        struct VirtIODevice vdev;
> }
> 
> struct VirtioBlockPCI {
>        struct VirtiPCIProxy_fields field;
>        struct VirtIOBlock vdev;
> }
> 
> <some for net and balloon>
> 
> Why?  All code in virtio.c wants/need a VirtIODevice.
> All code in virtio-pci.c wants a VirtIODevice also, it is not interested
> in the specific bits.
> 
> I can't see how you want to mov VirtIODevice from the 1st place of
> VirtIOBlock (and similars) without having to rewrite all of virtio-pci.c
> for each device.

The whole '1st place' hack is really not necessary, if we know the
offset we can use container_of.

> > Just changing it for the snake of change isn't a good reason
> > either. But if it helps cleaning the code we can change it without
> > running into trouble.  You can't cast VirtIODevice to VirtIOBlock any
> > more, but you don't really want to anyway for type checking reasons.
> 
> As I have show, from the time that virtio-pci only uses the virtio.c
> functions (and needs VirtIODevice fields), I don't see how to embed it
> and share the current functions in virtio-pci.c.

We can just keep the pointer from VirtiPCIProxy to virtio device.

> Notice that what we really want is create the devices in 
> 
>         .qdev.name = "virtio-balloon-pci",
>         .qdev.size = sizeof(VirtIOPCIProxy),
> 
> as
> 
>         .qdev.name = "virtio-balloon-pci",
>         .qdev.size = sizeof(VirtIOPCIProxy) + sizeof(VirtIOBlock) - sizeof(VirtIODevice),
> 
> And having a single definition of:
> 
> struct VirtioPCIProxy {
>        <current virtioPCIProxy fields>
>        struct VirtIODevice vdev;
> }
> 
> Yes, it is ugly/subtle as hell, but no other simple way.  You allocate
> the struct on each virtio-*.c file, where you know the size, and no
> embed.  Or you embed the struct, and replicate lots of code to have it
> type safe.

We don't need to drop the vdev pointer if it's helpful.
If we have it around there will be close to no code
changes.

> Any of the two options are worse than current way IMHO.  Having to
> export VirtIOBlock only to know its size looks wrong in addition.
> 
> Depending on size of the last element is ... too hackish?

Yes.

> Later, Juan.