Re: [Qemu-devel] Can we improve virtio data structures with QOM?

[Andreas Färber]

Am 30.05.2012 14:01, schrieb Markus Armbruster:
> Ordinary device models have a single state struct.  The first member is
> a DeviceState or a specialization of DeviceState, e.g. a PCIDevice.
> Simple enough.
> 
> Virtio device models are different.  Their "state struct" is really a
> proxy object that contains (a suitable specialization of) DeviceState,
> configuration information, and a pointer to the real state struct.
> 
> The real state struct contains device model state plus a whole bunch of
> pointers pointing back into the proxy.
> 
> This results in a pointer thicket which I find rather disagreeable.
> 
> In code:
> 
> typedef struct {
>     PCIDevice pci_dev;
>     VirtIODevice *vdev;         // points to VirtIOBlock for virtio-blk-*
>                                 //           VirtIONet for virtio-net-*
>                                 //           ...
>     [...]
>     VirtIOBlkConf blk;          // used only for virtio-blk-*
>     NICConf nic;                // used only for virtio-net-*
>     [...]                       // more of the same for other dev models
> } VirtIOPCIProxy;
> 
> struct VirtIOBlkConf
> {
>     BlockConf conf;
>     char *serial;
>     uint32_t scsi;
> };
> 
> typedef struct VirtIOBlock
> {
>     VirtIODevice vdev;
>     BlockDriverState *bs;       // copy of $proxy.blk->bs (for speed?)
>     [...]
>     BlockConf *conf;            // points to $proxy.blk.conf
>                                 // where $proxy is either VirtIOPCIProxy
>                                 // or VirtIOS390Device
>     
>     VirtIOBlkConf *blk;         // points to $proxy.blk
>     [...]
>     DeviceState *qdev;          // points to $proxy.pci_dev.qdev
> } VirtIOBlock;
> 
> Moreover, since all device configuration sits in the proxy object, all
> qdev properties need to be defined there, too.  While that's the
> appropriate place for the ones configuring the proxy, it's annoying for
> the ones configuring the vdev.
> 
> In code:
> 
> // virtio-blk-pci, in hw/virtio-pci.c:
> static Property virtio_blk_properties[] = {
>     DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>     DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, blk.conf),
>     DEFINE_PROP_STRING("serial", VirtIOPCIProxy, blk.serial),
> #ifdef __linux__
>     DEFINE_PROP_BIT("scsi", VirtIOPCIProxy, blk.scsi, 0, true),
> #endif
>     DEFINE_PROP_BIT("ioeventfd", VirtIOPCIProxy, flags, 
> VIRTIO_PCI_FLAG_USE_IOEVENTFD_BIT, true),
>     DEFINE_PROP_UINT32("vectors", VirtIOPCIProxy, nvectors, 2),
>     DEFINE_VIRTIO_BLK_FEATURES(VirtIOPCIProxy, host_features),
>     DEFINE_PROP_END_OF_LIST(),
> };
> 
> // virtio-blk-s390, in s390-virtio-bus.c:
> static Property s390_virtio_blk_properties[] = {
>     DEFINE_BLOCK_PROPERTIES(VirtIOS390Device, blk.conf),
>     DEFINE_PROP_STRING("serial", VirtIOS390Device, blk.serial),
> #ifdef __linux__
>     DEFINE_PROP_BIT("scsi", VirtIOS390Device, blk.scsi, 0, true),
> #endif
>     DEFINE_PROP_END_OF_LIST(),
> };
> 
> Note how the vdev's properties are duplicated.
> 
> 
> I'm not a QOM guru, but this doesn't feel like natural QOM to me.  Any
> ideas on how to do this right?  I got some, but they'd need thought.

I'm not a virtio guru, but QOM does not support multiple inheritance. We
could have a hierarchy PCIDevice -> VirtIOPCIDevice ->
VirtIOBlockPCIDevice for the PCI case but that would still cause
duplication for MMIO or Channel I/O versions IIUC.

As for the static properties, I'm still working on a refactored version
of Paolo's series that I hope to post this week.

If it's just about not duplicating typing the properties, we could of
course simply introduce a DEFINE_VIRTIO_PROPERTIES() macro or so. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg