On Thu, Mar 18, 2010 at 06:08:06PM +0100, Juan Quintela wrote:
> Gerd Hoffmann <kra...@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.
