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

[Juan Quintela <quintela@redhat.com>]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Mar 22, 2010 at 03:51:43PM +0000, Paul Brook wrote:
>> > > It's a classic OOP problem.
>> > >
>> > > VirtIOBlock is-a VirtIODevice, VirtIODevice is-a DeviceState
>> > >
>> > > VirtIOPCI is-a PCIDevice, PCIDevice is-a Device State.
>> > >
>> > > But VirtIODevice is-a VirtIOPCI device isn't always true so it can't be
>> > > an is-a relationship.  Initially, this was true and that's why the code
>> > > was structured that way.  Now that we have two type so of virtio
>> > > transports, we need to change the modelling.  It needs to get inverted
>> > > into a has-a relationship.  IOW, VirtIOPCI has-a VirtIODevice.
>> > >
>> > > When one device has-a one or more other devices, we model that as a Bus.
>> > 
>> > Hmm. Is anything wrong with VirtIOPCIBlock which would be both a
>> >  VirtIOBlock and VirtIOPCI device?
>> 
>> You need to solve multiple inheritance with common (but divergent) ancestors.
>> 
>> A single device may not have more than one DeviceState. i.e. the DeviceState 
>> that is part of the VirtIOBlock must be the same as the DeviceState that is 
>> part of the PCIDevice. However VirtIOBlock and PCIDevice can not know about 
>> each other or about VirtIOPCIBlock.
>> 
>> In the current code VirtIOPCIBlock already exists, though only for the 
>> purposes of device creation/configuration. The remainder of the code and data 
>> structures have clean separation between PCI and Virtio code.
>
> We can have VirtIOPCIBlock have a single DeviceState.

How?

That is the real problem he are having here.

struct VirtIODevice
{
..... No DeviceState at all
};

typedef struct VirtIOBlock
{
    VirtIODevice vdev;
    ....
} VirtIOBlock;

typedef struct {
    PCIDevice pci_dev;
    VirtIODevice *vdev;
    ....
} VirtIOPCIProxy;

Not relevant fields for the discussion removed.  Virtio-blk taken as an
example, but virtio-net/balloon/serial has the same structure.


Some virtio-pci functions (chosen to not be a vmstate one, but there are
more with this same structure).

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 a something that cames somehow from qdev or PCIDevice and we had
to also do something on the vdev.

Current situation:
- we have "sub-classes" of VirtIODevice (VirtIOBlock in this case).
  Everything outside of virtio-blk only cares for VirtIODevice fields.
  They are common for ol VirtIO* devices.

  Moving VirtIODevice out of offset 0 brings us nothing, until this
  reqeriment is removed.

- How do we arrive here?  We have three "kinds" of virtio devices:
   - pci ones (kvm)
   - sysborg (not pci)
   - s390

   And they share the code except for how to access the bus.

Solutions:
- VirtIOPCIBus and hang devices from there (anthony).  Why?  because
  this is a simulated pci bus, we can implement the features that we
  need (not full pci) in the three showed architectures.   We will have
  VirtIOPCIBLock everywhere, and its VirtIOPCIBus implmentation will
  hide the details.  Notice that this will make VirtIODevices more
  similar to the other kind of devices (see LSI example from Anthony in
  other email in the thread, see also mst answer that VirtIO is not like
  scsi either for the other point of view).

- Create something like:
  struct VirtIOPCIBlock {
     PCIDevice pci_dev;
     VirtIODevice vdev;
     <rest of VirtIOBlock fields>
  }
  this make lots of things easier, but makes impossible/very difficult
  to:  - share virtio-pci code (now we dont' have a common layaut
  struct).

  mst suggestion is to have something like:

  struct VirtIOPCIBlock {
     PCIDevice pci_dev;
     VirtIODevice *vdev;
     struct VirtIOBlock block;
  }
  and having vdev point to the right place.  The same for all devices.
  My question is how _requiring a zero_ offset for vdev inside
  VirtIOBlock is ugly, but having this vdev pointer to an internal part
  of the same struct is more elegant (in my book it is worse).

- Just replicate virtio-pci.c for each VirtIO* device and change the
  types accordingly.  This is way clearer in the sense of not having
  pointers to inside the same struct, but it has obvious maintenance
  nightmares.

If there are any better ideas around, I haven't seen them yet :-(
As Anthony described it: it is the best of two devils.  My point to not
like the mst change: I really think that it would never be used, it is
just over-engenieering.  Why do I think that?   Because something like
Anthony suggestion (VirtPCIBlock) fits so much better with qemu/qdev
device model.  Having that a VirtPCIBlock is-a VirtPCIProxy and a
VirtIODevice don't fit with the qemu model, I haven't seen any other
device that uses _double inheritance_.  And here is where the matter of
the discussion with mst appears:

- having single inheritance makes everything simpler -> virtio has to
  adapt to single inheritance (me)

- having multiple inheritance is "more natural" to virtio, then we have
  to change all the system to be able to allow multiple inherintance,
  even if it is more complex, and nothing else uses/needs it (mst).

Later, Juan.

(*): Note that this is VirtIO code, it will not ever use DO_UPCAST()
     neither for PCIDevice stuff, just in case qdev requirement
     disappears anytime soon.