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

[Juan Quintela]

Gerd Hoffmann <address@hidden> 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.

> 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.

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.

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?

Later, Juan.