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
