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

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/21/2010 08:06 PM, Juan Quintela wrote:
> "Michael S. Tsirkin"<mst@redhat.com>  wrote:
>    
>> On Sun, Mar 21, 2010 at 06:11:43PM +0000, Jamie Lokier wrote:
>>      
>>> Michael S. Tsirkin wrote:
>>>        
>>>> That's version 1 of my patch. Version 2 removed even need for macro
>>>> completely by moving allocations to the caller.
>>>>          
>>> The downside of moving allocations are: (1) it's one more call in the
>>> caller, to allocate the type, (2) it needs a virtual destructor for
>>> each type to free the object, which can clutter the code if there is
>>> no other reason for virtual destructors.
>>>
>>> I don't think those are necessarily bad, but they can remove from the
>>> neatness of existing code.  Personally I favour an occasional macro
>>> using sizeof/offsetof/container_of if the result is a natural and
>>> sensible API to all of its callers.
>>>
>>> -- Jamie
>>>        
>> We need to free in caller anyway because structure is used
>> after cleanup. This can be changed by restructuring code ...
>> I don't have a strong preference, anything is better
>> than the hack relying on field being at offset 0 in structure.
>>      
> It is not a hack.
>
> Is the only sane way of sharing virtio common field without having to
> export all virtio particular structs :(
>
> I really hate how this is going :(
>
> a- we have code that assumes that virtio is the 1st element of all
>     virtio structs.
>
> b- we have a patch that codifies that virtio is used in that way (using
>     DO_UPCAST()) (me)
>
> c- we arrived to the point where being it at the beggining of the struct
>     is the bigger cast in the universe (I present everybody thinking that
>     to look at rest of qemu).  Patch is done that makes it possible to
>     alloc memory outside of virtio_common_init().  No code on virtio.c or
>     virtio-pci.c is changed to use this new offset.  (michael).
>
> d- Anthony arrives to the discussion stating that it should exist a
>     VirtIOPCIBus, that way virtio devices should be hanging of that bus
>     (that requires lots of changes).
>
> e- kraxel arrives to the discussion stating that initializing in the
>     middle of one struct is the right thing to do, except if you are
>     qdev, that then it is not.  I point to his suggestion that it makes
>     things still uglier (no answer yet, but it was weekend for him).
>
> f- I show using kraxel example how ugly things go (sizeof (struct A) +
>     sizeof (struct B) - sizeof (struct C)).  And here is when things fall
>     apart IMHO.  Michael states that it is ok to have to had:
>      - an offset field
>      - a pointer in one struct to inside the same struct (vdev)
>     but assuming and stating and using an offset 0 is ugly.
>
> Have I lost anything?  Is there a realistic way of getting struct
> VirtIODevice (and VirtIOBlock, ...) inside VirtIOPCIProxy that don't
> imply using an offset + a pointer + exporting the struct definitions
> only to calculate its size?
>    

The object model is wrong.

A VirtIOBlock device cannot be a VirtIODevice while being a 
VirtIOPCIProxy (proxy is a poor name, btw).

It really ought to be:

DeviceState -> VirtIODevice -> VirtIOBlock

and:

PCIDevice -> VirtIOPCI : implements VirtIOBus

The interface between the VirtIODevice and VirtIOBus is the virtio 
transport.

The main reason a separate bus is needed is the same reason it's needed 
in Linux.  VirtIOBlock has to be tied to some bus.  It cannot be tied to 
the PCI bus without having it be part of the implementation detail.  
Introducing another bus type fixes this (and it's what we do in the kernel).

With respect to save and restore, there really ought to be two separate 
sections.  One section containing just virtio information and another 
section that stores the PCI state.  Admittedly, that's going to be a 
tough change to make but it's the proper approach.

Regards,

Anthony Liguori