| Message ID | 20121217214042.GA11926@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 17.12.2012 22:40, schrieb Michael S. Tsirkin: > Move bindings from opaque to DeviceState. > This gives us better type safety with no performance cost. > Add macros to make future QOM work easier, document > which ones are data-path sensitive. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Changes from v1: > - Address comment by Anreas Färber: wrap container_of > macros to make future QOM work easier > - make a couple of bindings that v1 missed typesafe: > virtio doesn't use any void * now > > diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c > index e0ac2d1..8c693b4 100644 > --- a/hw/s390-virtio-bus.c > +++ b/hw/s390-virtio-bus.c > @@ -137,7 +137,7 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev) > > bus->dev_offs += dev_len; > > - virtio_bind_device(vdev, &virtio_s390_bindings, dev); > + virtio_bind_device(vdev, &virtio_s390_bindings, VIRTIO_S390_TO_QDEV(dev)); DEVICE(dev) exists for exactly that purpose, and device init is certainly no hot path. Please don't reinvent the wheel for virtio. > dev->host_features = vdev->get_features(vdev, dev->host_features); > s390_virtio_device_sync(dev); > s390_virtio_reset_idx(dev); > @@ -364,9 +364,17 @@ VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus, ram_addr_t mem) > return NULL; > } > > -static void virtio_s390_notify(void *opaque, uint16_t vector) > +/* VirtIOS390Device to DeviceState */ > +#define VIRTIO_S390_TO_QDEV(dev) (&(dev)->qdev) Unneeded, and "QDEV" naming is not nice either. Definition after use. > + > +/* DeviceState to VirtIOS390Device. Note: used on datapath, > + * be careful and test performance if you change this. > + */ > +#define VIRTIO_S390_DEVICE(d) container_of(d, VirtIOS390Device, qdev) This leaves no name for a non-performance-critical macro we would expect under this name following the QOM naming conventions. Should be harmonized throughout the tree if we do this: Maybe UNCHECKED_* or UNSAFE_*, but see below... > + > +static void virtio_s390_notify(DeviceState *d, uint16_t vector) > { > - VirtIOS390Device *dev = (VirtIOS390Device*)opaque; > + VirtIOS390Device *dev = VIRTIO_S390_DEVICE(d); Why not simply for the hot path: - VirtIOS390Device *dev = (VirtIOS390Device*)opaque; + VirtIOS390Device *dev = (VirtIOS390Device*)d; When doing so, the improvement of this DeviceState* patch is ensuring that a caller doesn't stuff something random into the opaque. The implementation side would remain unchanged; I don't see any change on the path calling these either, so no change in performance. Type safety is the very purpose of the QOM macros that you are trying to circumvent here. Do you have any benchmark numbers justifying not using them? So far Anthony has told us to ignore that overhead, and I have merely been avoiding them on timer/interrupt void* opaques in favor of an old-fashioned C cast. > uint64_t token = s390_virtio_device_vq_token(dev, vector); > S390CPU *cpu = s390_cpu_addr2state(0); > CPUS390XState *env = &cpu->env; > @@ -374,9 +382,9 @@ static void virtio_s390_notify(void *opaque, uint16_t vector) > s390_virtio_irq(env, 0, token); > } > > -static unsigned virtio_s390_get_features(void *opaque) > +static unsigned virtio_s390_get_features(DeviceState *d) > { > - VirtIOS390Device *dev = (VirtIOS390Device*)opaque; > + VirtIOS390Device *dev = VIRTIO_S390_DEVICE(d); > return dev->host_features; > } > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index 3ea4140..1e9566a 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -97,35 +97,42 @@ > bool virtio_is_big_endian(void); > > /* virtio device */ > +/* VirtIOPCIProxy to DeviceState. */ > +#define VIRTIO_PCI_TO_QDEV(proxy) (&(proxy)->pci_dev.qdev) Unneeded. > > -static void virtio_pci_notify(void *opaque, uint16_t vector) > +/* DeviceState to VirtIOPCIProxy. Note: used on datapath, > + * be careful and test performance if you change this. > + */ > +#define VIRTIO_PCI_PROXY(d) container_of(d, VirtIOPCIProxy, pci_dev.qdev) Same comment as for s390. > + > +static void virtio_pci_notify(DeviceState *d, uint16_t vector) > { > - VirtIOPCIProxy *proxy = opaque; > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > if (msix_enabled(&proxy->pci_dev)) > msix_notify(&proxy->pci_dev, vector); > else > qemu_set_irq(proxy->pci_dev.irq[0], proxy->vdev->isr & 1); > } > > -static void virtio_pci_save_config(void * opaque, QEMUFile *f) > +static void virtio_pci_save_config(DeviceState *d, QEMUFile *f) > { > - VirtIOPCIProxy *proxy = opaque; > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); Is saving to a file seriously a hot path? > pci_device_save(&proxy->pci_dev, f); > msix_save(&proxy->pci_dev, f); > if (msix_present(&proxy->pci_dev)) > qemu_put_be16(f, proxy->vdev->config_vector); > } > > -static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f) > +static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f) > { > - VirtIOPCIProxy *proxy = opaque; > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); Ditto? > if (msix_present(&proxy->pci_dev)) > qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n)); > } > > -static int virtio_pci_load_config(void * opaque, QEMUFile *f) > +static int virtio_pci_load_config(DeviceState *d, QEMUFile *f) > { > - VirtIOPCIProxy *proxy = opaque; > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); Same for loading from a file? > int ret; > ret = pci_device_load(&proxy->pci_dev, f); > if (ret) { > @@ -144,9 +151,9 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f) > return 0; > } > > -static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f) > +static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f) > { > - VirtIOPCIProxy *proxy = opaque; > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); Ditto? > uint16_t vector; > if (msix_present(&proxy->pci_dev)) { > qemu_get_be16s(f, &vector); > @@ -244,7 +251,7 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy) > > void virtio_pci_reset(DeviceState *d) > { > - VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev); > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); Reset is not a hot path. > virtio_pci_stop_ioeventfd(proxy); > virtio_reset(proxy->vdev); > msix_unuse_all_vectors(&proxy->pci_dev); > @@ -464,9 +471,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > } > } > > -static unsigned virtio_pci_get_features(void *opaque) > +static unsigned virtio_pci_get_features(DeviceState *d) > { > - VirtIOPCIProxy *proxy = opaque; > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > return proxy->host_features; > } > > @@ -568,9 +575,9 @@ static void kvm_virtio_pci_vector_release(PCIDevice *dev, unsigned vector) > } > } > > -static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign) > +static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign) > { > - VirtIOPCIProxy *proxy = opaque; > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > VirtQueue *vq = virtio_get_queue(proxy->vdev, n); > EventNotifier *notifier = virtio_queue_get_guest_notifier(vq); > > @@ -588,15 +595,15 @@ static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign) > return 0; > } > > -static bool virtio_pci_query_guest_notifiers(void *opaque) > +static bool virtio_pci_query_guest_notifiers(DeviceState *d) > { > - VirtIOPCIProxy *proxy = opaque; > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > return msix_enabled(&proxy->pci_dev); > } > > -static int virtio_pci_set_guest_notifiers(void *opaque, bool assign) > +static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign) > { > - VirtIOPCIProxy *proxy = opaque; > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > VirtIODevice *vdev = proxy->vdev; > int r, n; > > @@ -612,7 +619,7 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign) > break; > } > > - r = virtio_pci_set_guest_notifier(opaque, n, assign); > + r = virtio_pci_set_guest_notifier(d, n, assign); > if (r < 0) { > goto assign_error; > } > @@ -638,14 +645,14 @@ assign_error: > /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */ > assert(assign); > while (--n >= 0) { > - virtio_pci_set_guest_notifier(opaque, n, !assign); > + virtio_pci_set_guest_notifier(d, n, !assign); > } > return r; > } > > -static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign) > +static int virtio_pci_set_host_notifier(DeviceState *d, int n, bool assign) > { > - VirtIOPCIProxy *proxy = opaque; > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > /* Stop using ioeventfd for virtqueue kick if the device starts using host > * notifiers. This makes it easy to avoid stepping on each others' toes. > @@ -661,9 +668,9 @@ static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign) > return virtio_pci_set_host_notifier_internal(proxy, n, assign, false); > } > > -static void virtio_pci_vmstate_change(void *opaque, bool running) > +static void virtio_pci_vmstate_change(DeviceState *d, bool running) > { > - VirtIOPCIProxy *proxy = opaque; > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > if (running) { > /* Try to find out if the guest has bus master disabled, but is > @@ -728,7 +735,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; > } > > - virtio_bind_device(vdev, &virtio_pci_bindings, proxy); > + virtio_bind_device(vdev, &virtio_pci_bindings, VIRTIO_PCI_TO_QDEV(proxy)); DEVICE(proxy) - device init not a hot path. > proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY; > proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE; > proxy->host_features = vdev->get_features(vdev, proxy->host_features); > diff --git a/hw/virtio.c b/hw/virtio.c > index f40a8c5..e65d7c8 100644 > --- a/hw/virtio.c > +++ b/hw/virtio.c > @@ -935,7 +943,7 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, > } > > void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, > - void *opaque) > + DeviceState *opaque) > { > vdev->binding = binding; > vdev->binding_opaque = opaque; > diff --git a/hw/virtio.h b/hw/virtio.h > index 7c17f7b..e2e57a4 100644 > --- a/hw/virtio.h > +++ b/hw/virtio.h > @@ -91,17 +91,17 @@ typedef struct VirtQueueElement > } VirtQueueElement; > > typedef struct { > - void (*notify)(void * opaque, uint16_t vector); > - void (*save_config)(void * opaque, QEMUFile *f); > - void (*save_queue)(void * opaque, int n, QEMUFile *f); > - int (*load_config)(void * opaque, QEMUFile *f); > - int (*load_queue)(void * opaque, int n, QEMUFile *f); > - int (*load_done)(void * opaque, QEMUFile *f); > - unsigned (*get_features)(void * opaque); > - bool (*query_guest_notifiers)(void * opaque); > - int (*set_guest_notifiers)(void * opaque, bool assigned); > - int (*set_host_notifier)(void * opaque, int n, bool assigned); > - void (*vmstate_change)(void * opaque, bool running); > + void (*notify)(DeviceState *d, uint16_t vector); > + void (*save_config)(DeviceState *d, QEMUFile *f); > + void (*save_queue)(DeviceState *d, int n, QEMUFile *f); > + int (*load_config)(DeviceState *d, QEMUFile *f); > + int (*load_queue)(DeviceState *d, int n, QEMUFile *f); > + int (*load_done)(DeviceState *d, QEMUFile *f); > + unsigned (*get_features)(DeviceState *d); > + bool (*query_guest_notifiers)(DeviceState *d); > + int (*set_guest_notifiers)(DeviceState *d, bool assigned); > + int (*set_host_notifier)(DeviceState *d, int n, bool assigned); > + void (*vmstate_change)(DeviceState *d, bool running); > } VirtIOBindings; > > #define VIRTIO_PCI_QUEUE_MAX 64 > @@ -128,7 +128,7 @@ struct VirtIODevice > void (*set_status)(VirtIODevice *vdev, uint8_t val); > VirtQueue *vq; > const VirtIOBindings *binding; > - void *binding_opaque; > + DeviceState *binding_opaque; > uint16_t device_id; > bool vm_running; > VMChangeStateEntry *vmstate; > @@ -187,11 +187,11 @@ uint16_t virtio_queue_vector(VirtIODevice *vdev, int n); > void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector); > void virtio_set_status(VirtIODevice *vdev, uint8_t val); > void virtio_reset(void *opaque); > void virtio_update_irq(VirtIODevice *vdev); > int virtio_set_features(VirtIODevice *vdev, uint32_t val); > > void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, > - void *opaque); > + DeviceState *opaque); > > /* Base devices. */ > typedef struct VirtIOBlkConf VirtIOBlkConf; Andreas
On Mon, Dec 17, 2012 at 11:59:15PM +0100, Andreas Färber wrote: > Am 17.12.2012 22:40, schrieb Michael S. Tsirkin: > > Move bindings from opaque to DeviceState. > > This gives us better type safety with no performance cost. > > Add macros to make future QOM work easier, document > > which ones are data-path sensitive. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > Changes from v1: > > - Address comment by Anreas Färber: wrap container_of > > macros to make future QOM work easier > > - make a couple of bindings that v1 missed typesafe: > > virtio doesn't use any void * now > > > > diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c > > index e0ac2d1..8c693b4 100644 > > --- a/hw/s390-virtio-bus.c > > +++ b/hw/s390-virtio-bus.c > > @@ -137,7 +137,7 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev) > > > > bus->dev_offs += dev_len; > > > > - virtio_bind_device(vdev, &virtio_s390_bindings, dev); > > + virtio_bind_device(vdev, &virtio_s390_bindings, VIRTIO_S390_TO_QDEV(dev)); > > DEVICE(dev) exists for exactly that purpose, and device init is > certainly no hot path. Please don't reinvent the wheel for virtio. OK. Though my beef with DEVICE is that it ignores the type passed in completely. You can give it int * and it will happily cast to devicestate. Your only hope is to catch the error at runtime. It would be better if DEVICE got the name of the qdev field, then we could check it's actually DeviceState before casting. Yes it would mean a bit of churn if you rename the field but it's very rare and trivial to change by a regexp. > > dev->host_features = vdev->get_features(vdev, dev->host_features); > > s390_virtio_device_sync(dev); > > s390_virtio_reset_idx(dev); > > @@ -364,9 +364,17 @@ VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus, ram_addr_t mem) > > return NULL; > > } > > > > -static void virtio_s390_notify(void *opaque, uint16_t vector) > > +/* VirtIOS390Device to DeviceState */ > > +#define VIRTIO_S390_TO_QDEV(dev) (&(dev)->qdev) > > Unneeded, and "QDEV" naming is not nice either. > > Definition after use. > > > + > > +/* DeviceState to VirtIOS390Device. Note: used on datapath, > > + * be careful and test performance if you change this. > > + */ > > +#define VIRTIO_S390_DEVICE(d) container_of(d, VirtIOS390Device, qdev) > > This leaves no name for a non-performance-critical macro we would expect > under this name following the QOM naming conventions. > > Should be harmonized throughout the tree if we do this: Hey I only replaced one use of container_of macro with another. It's fair enough to ask that my patch doesn't make your QOM work harder but don't can't ask me to do all QOM work for you. > Maybe > UNCHECKED_* or UNSAFE_*, but see below... Of course it's UNSAFE if you insist on doing C style macros. If you do this using container_of it's not unchecked - it's compile-time checked. Then we could call it FAST_* > > + > > +static void virtio_s390_notify(DeviceState *d, uint16_t vector) > > { > > - VirtIOS390Device *dev = (VirtIOS390Device*)opaque; > > + VirtIOS390Device *dev = VIRTIO_S390_DEVICE(d); > > Why not simply for the hot path: > - VirtIOS390Device *dev = (VirtIOS390Device*)opaque; > + VirtIOS390Device *dev = (VirtIOS390Device*)d; Because this throws out type checking which is exactly what this patch is about: if d is changed to something incompative there will be no error. Not pretty. > When doing so, the improvement of this DeviceState* patch is ensuring > that a caller doesn't stuff something random into the opaque. The > implementation side would remain unchanged; I don't see any change on > the path calling these either, so no change in performance. > > Type safety is the very purpose of the QOM macros that you are trying to > circumvent here. I am not circumventing anything. I am getting rid of void * pointers. You can write a patch on top and patch the macros to something else like QOM things. You can not claim type safety with void * pointers so let's apply the patch and you can add more type safety with QOM or whatever. > Do you have any benchmark numbers justifying not using > them? So far Anthony has told us to ignore that overhead, and I have > merely been avoiding them on timer/interrupt void* opaques in favor of > an old-fashioned C cast. If you care there you should care here these are called on each interrupt. Anyway, old-fashioned C cast is evil, container_of is better: it checks the argument type makes sense. > > uint64_t token = s390_virtio_device_vq_token(dev, vector); > > S390CPU *cpu = s390_cpu_addr2state(0); > > CPUS390XState *env = &cpu->env; > > @@ -374,9 +382,9 @@ static void virtio_s390_notify(void *opaque, uint16_t vector) > > s390_virtio_irq(env, 0, token); > > } > > > > -static unsigned virtio_s390_get_features(void *opaque) > > +static unsigned virtio_s390_get_features(DeviceState *d) > > { > > - VirtIOS390Device *dev = (VirtIOS390Device*)opaque; > > + VirtIOS390Device *dev = VIRTIO_S390_DEVICE(d); > > return dev->host_features; > > } > > > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > > index 3ea4140..1e9566a 100644 > > --- a/hw/virtio-pci.c > > +++ b/hw/virtio-pci.c > > @@ -97,35 +97,42 @@ > > bool virtio_is_big_endian(void); > > > > /* virtio device */ > > +/* VirtIOPCIProxy to DeviceState. */ > > +#define VIRTIO_PCI_TO_QDEV(proxy) (&(proxy)->pci_dev.qdev) > > Unneeded. Same answer everywhere below. > > > > > -static void virtio_pci_notify(void *opaque, uint16_t vector) > > +/* DeviceState to VirtIOPCIProxy. Note: used on datapath, > > + * be careful and test performance if you change this. > > + */ > > +#define VIRTIO_PCI_PROXY(d) container_of(d, VirtIOPCIProxy, pci_dev.qdev) > > Same comment as for s390. > > + > > +static void virtio_pci_notify(DeviceState *d, uint16_t vector) > > { > > - VirtIOPCIProxy *proxy = opaque; > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > if (msix_enabled(&proxy->pci_dev)) > > msix_notify(&proxy->pci_dev, vector); > > else > > qemu_set_irq(proxy->pci_dev.irq[0], proxy->vdev->isr & 1); > > } > > > > -static void virtio_pci_save_config(void * opaque, QEMUFile *f) > > +static void virtio_pci_save_config(DeviceState *d, QEMUFile *f) > > { > > - VirtIOPCIProxy *proxy = opaque; > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > Is saving to a file seriously a hot path? Not at all but let's use same style in this file, consistently. > > pci_device_save(&proxy->pci_dev, f); > > msix_save(&proxy->pci_dev, f); > > if (msix_present(&proxy->pci_dev)) > > qemu_put_be16(f, proxy->vdev->config_vector); > > } > > > > -static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f) > > +static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f) > > { > > - VirtIOPCIProxy *proxy = opaque; > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > Ditto? > > > if (msix_present(&proxy->pci_dev)) > > qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n)); > > } > > > > -static int virtio_pci_load_config(void * opaque, QEMUFile *f) > > +static int virtio_pci_load_config(DeviceState *d, QEMUFile *f) > > { > > - VirtIOPCIProxy *proxy = opaque; > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > Same for loading from a file? > > > int ret; > > ret = pci_device_load(&proxy->pci_dev, f); > > if (ret) { > > @@ -144,9 +151,9 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f) > > return 0; > > } > > > > -static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f) > > +static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f) > > { > > - VirtIOPCIProxy *proxy = opaque; > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > Ditto? > > > uint16_t vector; > > if (msix_present(&proxy->pci_dev)) { > > qemu_get_be16s(f, &vector); > > @@ -244,7 +251,7 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy) > > > > void virtio_pci_reset(DeviceState *d) > > { > > - VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev); > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > Reset is not a hot path. > > > virtio_pci_stop_ioeventfd(proxy); > > virtio_reset(proxy->vdev); > > msix_unuse_all_vectors(&proxy->pci_dev); > > @@ -464,9 +471,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, > > } > > } > > > > -static unsigned virtio_pci_get_features(void *opaque) > > +static unsigned virtio_pci_get_features(DeviceState *d) > > { > > - VirtIOPCIProxy *proxy = opaque; > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > return proxy->host_features; > > } > > > > @@ -568,9 +575,9 @@ static void kvm_virtio_pci_vector_release(PCIDevice *dev, unsigned vector) > > } > > } > > > > -static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign) > > +static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign) > > { > > - VirtIOPCIProxy *proxy = opaque; > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > VirtQueue *vq = virtio_get_queue(proxy->vdev, n); > > EventNotifier *notifier = virtio_queue_get_guest_notifier(vq); > > > > @@ -588,15 +595,15 @@ static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign) > > return 0; > > } > > > > -static bool virtio_pci_query_guest_notifiers(void *opaque) > > +static bool virtio_pci_query_guest_notifiers(DeviceState *d) > > { > > - VirtIOPCIProxy *proxy = opaque; > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > return msix_enabled(&proxy->pci_dev); > > } > > > > -static int virtio_pci_set_guest_notifiers(void *opaque, bool assign) > > +static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign) > > { > > - VirtIOPCIProxy *proxy = opaque; > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > VirtIODevice *vdev = proxy->vdev; > > int r, n; > > > > @@ -612,7 +619,7 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign) > > break; > > } > > > > - r = virtio_pci_set_guest_notifier(opaque, n, assign); > > + r = virtio_pci_set_guest_notifier(d, n, assign); > > if (r < 0) { > > goto assign_error; > > } > > @@ -638,14 +645,14 @@ assign_error: > > /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */ > > assert(assign); > > while (--n >= 0) { > > - virtio_pci_set_guest_notifier(opaque, n, !assign); > > + virtio_pci_set_guest_notifier(d, n, !assign); > > } > > return r; > > } > > > > -static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign) > > +static int virtio_pci_set_host_notifier(DeviceState *d, int n, bool assign) > > { > > - VirtIOPCIProxy *proxy = opaque; > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > > > /* Stop using ioeventfd for virtqueue kick if the device starts using host > > * notifiers. This makes it easy to avoid stepping on each others' toes. > > @@ -661,9 +668,9 @@ static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign) > > return virtio_pci_set_host_notifier_internal(proxy, n, assign, false); > > } > > > > -static void virtio_pci_vmstate_change(void *opaque, bool running) > > +static void virtio_pci_vmstate_change(DeviceState *d, bool running) > > { > > - VirtIOPCIProxy *proxy = opaque; > > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); > > > > if (running) { > > /* Try to find out if the guest has bus master disabled, but is > > @@ -728,7 +735,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) > > proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; > > } > > > > - virtio_bind_device(vdev, &virtio_pci_bindings, proxy); > > + virtio_bind_device(vdev, &virtio_pci_bindings, VIRTIO_PCI_TO_QDEV(proxy)); > > DEVICE(proxy) - device init not a hot path. > > > proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY; > > proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE; > > proxy->host_features = vdev->get_features(vdev, proxy->host_features); > > diff --git a/hw/virtio.c b/hw/virtio.c > > index f40a8c5..e65d7c8 100644 > > --- a/hw/virtio.c > > +++ b/hw/virtio.c > > @@ -935,7 +943,7 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, > > } > > > > void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, > > - void *opaque) > > + DeviceState *opaque) > > { > > vdev->binding = binding; > > vdev->binding_opaque = opaque; > > diff --git a/hw/virtio.h b/hw/virtio.h > > index 7c17f7b..e2e57a4 100644 > > --- a/hw/virtio.h > > +++ b/hw/virtio.h > > @@ -91,17 +91,17 @@ typedef struct VirtQueueElement > > } VirtQueueElement; > > > > typedef struct { > > - void (*notify)(void * opaque, uint16_t vector); > > - void (*save_config)(void * opaque, QEMUFile *f); > > - void (*save_queue)(void * opaque, int n, QEMUFile *f); > > - int (*load_config)(void * opaque, QEMUFile *f); > > - int (*load_queue)(void * opaque, int n, QEMUFile *f); > > - int (*load_done)(void * opaque, QEMUFile *f); > > - unsigned (*get_features)(void * opaque); > > - bool (*query_guest_notifiers)(void * opaque); > > - int (*set_guest_notifiers)(void * opaque, bool assigned); > > - int (*set_host_notifier)(void * opaque, int n, bool assigned); > > - void (*vmstate_change)(void * opaque, bool running); > > + void (*notify)(DeviceState *d, uint16_t vector); > > + void (*save_config)(DeviceState *d, QEMUFile *f); > > + void (*save_queue)(DeviceState *d, int n, QEMUFile *f); > > + int (*load_config)(DeviceState *d, QEMUFile *f); > > + int (*load_queue)(DeviceState *d, int n, QEMUFile *f); > > + int (*load_done)(DeviceState *d, QEMUFile *f); > > + unsigned (*get_features)(DeviceState *d); > > + bool (*query_guest_notifiers)(DeviceState *d); > > + int (*set_guest_notifiers)(DeviceState *d, bool assigned); > > + int (*set_host_notifier)(DeviceState *d, int n, bool assigned); > > + void (*vmstate_change)(DeviceState *d, bool running); > > } VirtIOBindings; > > > > #define VIRTIO_PCI_QUEUE_MAX 64 > > @@ -128,7 +128,7 @@ struct VirtIODevice > > void (*set_status)(VirtIODevice *vdev, uint8_t val); > > VirtQueue *vq; > > const VirtIOBindings *binding; > > - void *binding_opaque; > > + DeviceState *binding_opaque; > > uint16_t device_id; > > bool vm_running; > > VMChangeStateEntry *vmstate; > > @@ -187,11 +187,11 @@ uint16_t virtio_queue_vector(VirtIODevice *vdev, int n); > > void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector); > > void virtio_set_status(VirtIODevice *vdev, uint8_t val); > > void virtio_reset(void *opaque); > > void virtio_update_irq(VirtIODevice *vdev); > > int virtio_set_features(VirtIODevice *vdev, uint32_t val); > > > > void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, > > - void *opaque); > > + DeviceState *opaque); > > > > /* Base devices. */ > > typedef struct VirtIOBlkConf VirtIOBlkConf; > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
"Michael S. Tsirkin" <mst@redhat.com> writes: > On Mon, Dec 17, 2012 at 11:59:15PM +0100, Andreas Färber wrote: >> Am 17.12.2012 22:40, schrieb Michael S. Tsirkin: >> > Move bindings from opaque to DeviceState. >> > This gives us better type safety with no performance cost. >> > Add macros to make future QOM work easier, document >> > which ones are data-path sensitive. >> > >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> > --- >> > >> > Changes from v1: >> > - Address comment by Anreas Färber: wrap container_of >> > macros to make future QOM work easier >> > - make a couple of bindings that v1 missed typesafe: >> > virtio doesn't use any void * now >> > >> > diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c >> > index e0ac2d1..8c693b4 100644 >> > --- a/hw/s390-virtio-bus.c >> > +++ b/hw/s390-virtio-bus.c >> > @@ -137,7 +137,7 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev) >> > >> > bus->dev_offs += dev_len; >> > >> > - virtio_bind_device(vdev, &virtio_s390_bindings, dev); >> > + virtio_bind_device(vdev, &virtio_s390_bindings, VIRTIO_S390_TO_QDEV(dev)); >> >> DEVICE(dev) exists for exactly that purpose, and device init is >> certainly no hot path. Please don't reinvent the wheel for virtio. > > OK. > Though my beef with DEVICE is that it ignores the type > passed in completely. You can give it int * and it will > happily cast to devicestate. Your only hope is to > catch the error at runtime. That's a feature. DEVICE can do upcasting and downcasting. There's no way to do compile time checking of upcasting when > It would be better if DEVICE got the name of the > qdev field, then we could check it's actually DeviceState > before casting. Yes it would mean a bit of churn if you rename the > field but it's very rare and trivial to change by a regexp. No, it would be much, much worse. You shouldn't have to know what the layout of the structure is to convert between types. > >> > dev->host_features = vdev->get_features(vdev, dev->host_features); >> > s390_virtio_device_sync(dev); >> > s390_virtio_reset_idx(dev); >> > @@ -364,9 +364,17 @@ VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus, ram_addr_t mem) >> > return NULL; >> > } >> > >> > -static void virtio_s390_notify(void *opaque, uint16_t vector) >> > +/* VirtIOS390Device to DeviceState */ >> > +#define VIRTIO_S390_TO_QDEV(dev) (&(dev)->qdev) >> >> Unneeded, and "QDEV" naming is not nice either. >> >> Definition after use. >> >> > + >> > +/* DeviceState to VirtIOS390Device. Note: used on datapath, >> > + * be careful and test performance if you change this. >> > + */ >> > +#define VIRTIO_S390_DEVICE(d) container_of(d, VirtIOS390Device, qdev) >> >> This leaves no name for a non-performance-critical macro we would expect >> under this name following the QOM naming conventions. >> >> Should be harmonized throughout the tree if we do this: > > Hey I only replaced one use of container_of macro with another. > It's fair enough to ask that my patch doesn't make your QOM work > harder but don't can't ask me to do all QOM work for you. What don't you just use a static inline and then you get even more type safety and don't confuse with QOM cast macros... Regards, Anthony Liguori > >> Maybe >> UNCHECKED_* or UNSAFE_*, but see below... > > Of course it's UNSAFE if you insist on doing C style macros. > > If you do this using container_of > it's not unchecked - it's compile-time checked. > > Then we could call it FAST_* > > >> > + >> > +static void virtio_s390_notify(DeviceState *d, uint16_t vector) >> > { >> > - VirtIOS390Device *dev = (VirtIOS390Device*)opaque; >> > + VirtIOS390Device *dev = VIRTIO_S390_DEVICE(d); >> >> Why not simply for the hot path: >> - VirtIOS390Device *dev = (VirtIOS390Device*)opaque; >> + VirtIOS390Device *dev = (VirtIOS390Device*)d; > > Because this throws out type checking which is exactly > what this patch is about: if d is changed to something > incompative there will be no error. Not pretty. > >> When doing so, the improvement of this DeviceState* patch is ensuring >> that a caller doesn't stuff something random into the opaque. The >> implementation side would remain unchanged; I don't see any change on >> the path calling these either, so no change in performance. >> >> Type safety is the very purpose of the QOM macros that you are trying to >> circumvent here. > > I am not circumventing anything. I am getting rid of void * > pointers. You can write a patch on top and patch the macros > to something else like QOM things. > You can not claim type safety with void * pointers so > let's apply the patch and you can add more type safety > with QOM or whatever. > >> Do you have any benchmark numbers justifying not using >> them? So far Anthony has told us to ignore that overhead, and I have >> merely been avoiding them on timer/interrupt void* opaques in favor of >> an old-fashioned C cast. > > If you care there you should care here these are called on each > interrupt. Anyway, old-fashioned C cast is evil, container_of > is better: it checks the argument type makes sense. > >> > uint64_t token = s390_virtio_device_vq_token(dev, vector); >> > S390CPU *cpu = s390_cpu_addr2state(0); >> > CPUS390XState *env = &cpu->env; >> > @@ -374,9 +382,9 @@ static void virtio_s390_notify(void *opaque, uint16_t vector) >> > s390_virtio_irq(env, 0, token); >> > } >> > >> > -static unsigned virtio_s390_get_features(void *opaque) >> > +static unsigned virtio_s390_get_features(DeviceState *d) >> > { >> > - VirtIOS390Device *dev = (VirtIOS390Device*)opaque; >> > + VirtIOS390Device *dev = VIRTIO_S390_DEVICE(d); >> > return dev->host_features; >> > } >> > >> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c >> > index 3ea4140..1e9566a 100644 >> > --- a/hw/virtio-pci.c >> > +++ b/hw/virtio-pci.c >> > @@ -97,35 +97,42 @@ >> > bool virtio_is_big_endian(void); >> > >> > /* virtio device */ >> > +/* VirtIOPCIProxy to DeviceState. */ >> > +#define VIRTIO_PCI_TO_QDEV(proxy) (&(proxy)->pci_dev.qdev) >> >> Unneeded. > > Same answer everywhere below. > >> >> > >> > -static void virtio_pci_notify(void *opaque, uint16_t vector) >> > +/* DeviceState to VirtIOPCIProxy. Note: used on datapath, >> > + * be careful and test performance if you change this. >> > + */ >> > +#define VIRTIO_PCI_PROXY(d) container_of(d, VirtIOPCIProxy, pci_dev.qdev) >> >> Same comment as for s390. >> > + >> > +static void virtio_pci_notify(DeviceState *d, uint16_t vector) >> > { >> > - VirtIOPCIProxy *proxy = opaque; >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> > if (msix_enabled(&proxy->pci_dev)) >> > msix_notify(&proxy->pci_dev, vector); >> > else >> > qemu_set_irq(proxy->pci_dev.irq[0], proxy->vdev->isr & 1); >> > } >> > >> > -static void virtio_pci_save_config(void * opaque, QEMUFile *f) >> > +static void virtio_pci_save_config(DeviceState *d, QEMUFile *f) >> > { >> > - VirtIOPCIProxy *proxy = opaque; >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> >> Is saving to a file seriously a hot path? > > Not at all but let's use same style in this file, consistently. > >> > pci_device_save(&proxy->pci_dev, f); >> > msix_save(&proxy->pci_dev, f); >> > if (msix_present(&proxy->pci_dev)) >> > qemu_put_be16(f, proxy->vdev->config_vector); >> > } >> > >> > -static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f) >> > +static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f) >> > { >> > - VirtIOPCIProxy *proxy = opaque; >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> >> Ditto? >> >> > if (msix_present(&proxy->pci_dev)) >> > qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n)); >> > } >> > >> > -static int virtio_pci_load_config(void * opaque, QEMUFile *f) >> > +static int virtio_pci_load_config(DeviceState *d, QEMUFile *f) >> > { >> > - VirtIOPCIProxy *proxy = opaque; >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> >> Same for loading from a file? >> >> > int ret; >> > ret = pci_device_load(&proxy->pci_dev, f); >> > if (ret) { >> > @@ -144,9 +151,9 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f) >> > return 0; >> > } >> > >> > -static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f) >> > +static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f) >> > { >> > - VirtIOPCIProxy *proxy = opaque; >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> >> Ditto? >> >> > uint16_t vector; >> > if (msix_present(&proxy->pci_dev)) { >> > qemu_get_be16s(f, &vector); >> > @@ -244,7 +251,7 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy) >> > >> > void virtio_pci_reset(DeviceState *d) >> > { >> > - VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev); >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> >> Reset is not a hot path. >> >> > virtio_pci_stop_ioeventfd(proxy); >> > virtio_reset(proxy->vdev); >> > msix_unuse_all_vectors(&proxy->pci_dev); >> > @@ -464,9 +471,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, >> > } >> > } >> > >> > -static unsigned virtio_pci_get_features(void *opaque) >> > +static unsigned virtio_pci_get_features(DeviceState *d) >> > { >> > - VirtIOPCIProxy *proxy = opaque; >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> > return proxy->host_features; >> > } >> > >> > @@ -568,9 +575,9 @@ static void kvm_virtio_pci_vector_release(PCIDevice *dev, unsigned vector) >> > } >> > } >> > >> > -static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign) >> > +static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign) >> > { >> > - VirtIOPCIProxy *proxy = opaque; >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> > VirtQueue *vq = virtio_get_queue(proxy->vdev, n); >> > EventNotifier *notifier = virtio_queue_get_guest_notifier(vq); >> > >> > @@ -588,15 +595,15 @@ static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign) >> > return 0; >> > } >> > >> > -static bool virtio_pci_query_guest_notifiers(void *opaque) >> > +static bool virtio_pci_query_guest_notifiers(DeviceState *d) >> > { >> > - VirtIOPCIProxy *proxy = opaque; >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> > return msix_enabled(&proxy->pci_dev); >> > } >> > >> > -static int virtio_pci_set_guest_notifiers(void *opaque, bool assign) >> > +static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign) >> > { >> > - VirtIOPCIProxy *proxy = opaque; >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> > VirtIODevice *vdev = proxy->vdev; >> > int r, n; >> > >> > @@ -612,7 +619,7 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign) >> > break; >> > } >> > >> > - r = virtio_pci_set_guest_notifier(opaque, n, assign); >> > + r = virtio_pci_set_guest_notifier(d, n, assign); >> > if (r < 0) { >> > goto assign_error; >> > } >> > @@ -638,14 +645,14 @@ assign_error: >> > /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */ >> > assert(assign); >> > while (--n >= 0) { >> > - virtio_pci_set_guest_notifier(opaque, n, !assign); >> > + virtio_pci_set_guest_notifier(d, n, !assign); >> > } >> > return r; >> > } >> > >> > -static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign) >> > +static int virtio_pci_set_host_notifier(DeviceState *d, int n, bool assign) >> > { >> > - VirtIOPCIProxy *proxy = opaque; >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> > >> > /* Stop using ioeventfd for virtqueue kick if the device starts using host >> > * notifiers. This makes it easy to avoid stepping on each others' toes. >> > @@ -661,9 +668,9 @@ static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign) >> > return virtio_pci_set_host_notifier_internal(proxy, n, assign, false); >> > } >> > >> > -static void virtio_pci_vmstate_change(void *opaque, bool running) >> > +static void virtio_pci_vmstate_change(DeviceState *d, bool running) >> > { >> > - VirtIOPCIProxy *proxy = opaque; >> > + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); >> > >> > if (running) { >> > /* Try to find out if the guest has bus master disabled, but is >> > @@ -728,7 +735,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) >> > proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; >> > } >> > >> > - virtio_bind_device(vdev, &virtio_pci_bindings, proxy); >> > + virtio_bind_device(vdev, &virtio_pci_bindings, VIRTIO_PCI_TO_QDEV(proxy)); >> >> DEVICE(proxy) - device init not a hot path. >> >> > proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY; >> > proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE; >> > proxy->host_features = vdev->get_features(vdev, proxy->host_features); >> > diff --git a/hw/virtio.c b/hw/virtio.c >> > index f40a8c5..e65d7c8 100644 >> > --- a/hw/virtio.c >> > +++ b/hw/virtio.c >> > @@ -935,7 +943,7 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, >> > } >> > >> > void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, >> > - void *opaque) >> > + DeviceState *opaque) >> > { >> > vdev->binding = binding; >> > vdev->binding_opaque = opaque; >> > diff --git a/hw/virtio.h b/hw/virtio.h >> > index 7c17f7b..e2e57a4 100644 >> > --- a/hw/virtio.h >> > +++ b/hw/virtio.h >> > @@ -91,17 +91,17 @@ typedef struct VirtQueueElement >> > } VirtQueueElement; >> > >> > typedef struct { >> > - void (*notify)(void * opaque, uint16_t vector); >> > - void (*save_config)(void * opaque, QEMUFile *f); >> > - void (*save_queue)(void * opaque, int n, QEMUFile *f); >> > - int (*load_config)(void * opaque, QEMUFile *f); >> > - int (*load_queue)(void * opaque, int n, QEMUFile *f); >> > - int (*load_done)(void * opaque, QEMUFile *f); >> > - unsigned (*get_features)(void * opaque); >> > - bool (*query_guest_notifiers)(void * opaque); >> > - int (*set_guest_notifiers)(void * opaque, bool assigned); >> > - int (*set_host_notifier)(void * opaque, int n, bool assigned); >> > - void (*vmstate_change)(void * opaque, bool running); >> > + void (*notify)(DeviceState *d, uint16_t vector); >> > + void (*save_config)(DeviceState *d, QEMUFile *f); >> > + void (*save_queue)(DeviceState *d, int n, QEMUFile *f); >> > + int (*load_config)(DeviceState *d, QEMUFile *f); >> > + int (*load_queue)(DeviceState *d, int n, QEMUFile *f); >> > + int (*load_done)(DeviceState *d, QEMUFile *f); >> > + unsigned (*get_features)(DeviceState *d); >> > + bool (*query_guest_notifiers)(DeviceState *d); >> > + int (*set_guest_notifiers)(DeviceState *d, bool assigned); >> > + int (*set_host_notifier)(DeviceState *d, int n, bool assigned); >> > + void (*vmstate_change)(DeviceState *d, bool running); >> > } VirtIOBindings; >> > >> > #define VIRTIO_PCI_QUEUE_MAX 64 >> > @@ -128,7 +128,7 @@ struct VirtIODevice >> > void (*set_status)(VirtIODevice *vdev, uint8_t val); >> > VirtQueue *vq; >> > const VirtIOBindings *binding; >> > - void *binding_opaque; >> > + DeviceState *binding_opaque; >> > uint16_t device_id; >> > bool vm_running; >> > VMChangeStateEntry *vmstate; >> > @@ -187,11 +187,11 @@ uint16_t virtio_queue_vector(VirtIODevice *vdev, int n); >> > void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector); >> > void virtio_set_status(VirtIODevice *vdev, uint8_t val); >> > void virtio_reset(void *opaque); >> > void virtio_update_irq(VirtIODevice *vdev); >> > int virtio_set_features(VirtIODevice *vdev, uint32_t val); >> > >> > void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, >> > - void *opaque); >> > + DeviceState *opaque); >> > >> > /* Base devices. */ >> > typedef struct VirtIOBlkConf VirtIOBlkConf; >> >> Andreas >> >> -- >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany >> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
On Mon, Dec 17, 2012 at 06:42:58PM -0600, Anthony Liguori wrote: > What don't you just use a static inline and then you get even more type > safety and don't confuse with QOM cast macros... > > Regards, > > Anthony Liguori OK.
On Mon, Dec 17, 2012 at 06:42:58PM -0600, Anthony Liguori wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > On Mon, Dec 17, 2012 at 11:59:15PM +0100, Andreas Färber wrote: > >> Am 17.12.2012 22:40, schrieb Michael S. Tsirkin: > >> > Move bindings from opaque to DeviceState. > >> > This gives us better type safety with no performance cost. > >> > Add macros to make future QOM work easier, document > >> > which ones are data-path sensitive. > >> > > >> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >> > --- > >> > > >> > Changes from v1: > >> > - Address comment by Anreas Färber: wrap container_of > >> > macros to make future QOM work easier > >> > - make a couple of bindings that v1 missed typesafe: > >> > virtio doesn't use any void * now > >> > > >> > diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c > >> > index e0ac2d1..8c693b4 100644 > >> > --- a/hw/s390-virtio-bus.c > >> > +++ b/hw/s390-virtio-bus.c > >> > @@ -137,7 +137,7 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev) > >> > > >> > bus->dev_offs += dev_len; > >> > > >> > - virtio_bind_device(vdev, &virtio_s390_bindings, dev); > >> > + virtio_bind_device(vdev, &virtio_s390_bindings, VIRTIO_S390_TO_QDEV(dev)); > >> > >> DEVICE(dev) exists for exactly that purpose, and device init is > >> certainly no hot path. Please don't reinvent the wheel for virtio. > > > > OK. > > Though my beef with DEVICE is that it ignores the type > > passed in completely. You can give it int * and it will > > happily cast to devicestate. Your only hope is to > > catch the error at runtime. > > That's a feature. DEVICE can do upcasting and downcasting. There's no > way to do compile time checking of upcasting when > > > It would be better if DEVICE got the name of the > > qdev field, then we could check it's actually DeviceState > > before casting. Yes it would mean a bit of churn if you rename the > > field but it's very rare and trivial to change by a regexp. > > No, it would be much, much worse. You shouldn't have to know what the > layout of the structure is to convert between types. Still I'm pointing out the problems, they are real. Illegal code like DEVICE("foobar") compiles fine and it shouldn't.
diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c index e0ac2d1..8c693b4 100644 --- a/hw/s390-virtio-bus.c +++ b/hw/s390-virtio-bus.c @@ -137,7 +137,7 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev) bus->dev_offs += dev_len; - virtio_bind_device(vdev, &virtio_s390_bindings, dev); + virtio_bind_device(vdev, &virtio_s390_bindings, VIRTIO_S390_TO_QDEV(dev)); dev->host_features = vdev->get_features(vdev, dev->host_features); s390_virtio_device_sync(dev); s390_virtio_reset_idx(dev); @@ -364,9 +364,17 @@ VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus, ram_addr_t mem) return NULL; } -static void virtio_s390_notify(void *opaque, uint16_t vector) +/* VirtIOS390Device to DeviceState */ +#define VIRTIO_S390_TO_QDEV(dev) (&(dev)->qdev) + +/* DeviceState to VirtIOS390Device. Note: used on datapath, + * be careful and test performance if you change this. + */ +#define VIRTIO_S390_DEVICE(d) container_of(d, VirtIOS390Device, qdev) + +static void virtio_s390_notify(DeviceState *d, uint16_t vector) { - VirtIOS390Device *dev = (VirtIOS390Device*)opaque; + VirtIOS390Device *dev = VIRTIO_S390_DEVICE(d); uint64_t token = s390_virtio_device_vq_token(dev, vector); S390CPU *cpu = s390_cpu_addr2state(0); CPUS390XState *env = &cpu->env; @@ -374,9 +382,9 @@ static void virtio_s390_notify(void *opaque, uint16_t vector) s390_virtio_irq(env, 0, token); } -static unsigned virtio_s390_get_features(void *opaque) +static unsigned virtio_s390_get_features(DeviceState *d) { - VirtIOS390Device *dev = (VirtIOS390Device*)opaque; + VirtIOS390Device *dev = VIRTIO_S390_DEVICE(d); return dev->host_features; } diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c index 3ea4140..1e9566a 100644 --- a/hw/virtio-pci.c +++ b/hw/virtio-pci.c @@ -97,35 +97,42 @@ bool virtio_is_big_endian(void); /* virtio device */ +/* VirtIOPCIProxy to DeviceState. */ +#define VIRTIO_PCI_TO_QDEV(proxy) (&(proxy)->pci_dev.qdev) -static void virtio_pci_notify(void *opaque, uint16_t vector) +/* DeviceState to VirtIOPCIProxy. Note: used on datapath, + * be careful and test performance if you change this. + */ +#define VIRTIO_PCI_PROXY(d) container_of(d, VirtIOPCIProxy, pci_dev.qdev) + +static void virtio_pci_notify(DeviceState *d, uint16_t vector) { - VirtIOPCIProxy *proxy = opaque; + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); if (msix_enabled(&proxy->pci_dev)) msix_notify(&proxy->pci_dev, vector); else qemu_set_irq(proxy->pci_dev.irq[0], proxy->vdev->isr & 1); } -static void virtio_pci_save_config(void * opaque, QEMUFile *f) +static void virtio_pci_save_config(DeviceState *d, QEMUFile *f) { - VirtIOPCIProxy *proxy = opaque; + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); pci_device_save(&proxy->pci_dev, f); msix_save(&proxy->pci_dev, f); if (msix_present(&proxy->pci_dev)) qemu_put_be16(f, proxy->vdev->config_vector); } -static void virtio_pci_save_queue(void * opaque, int n, QEMUFile *f) +static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f) { - VirtIOPCIProxy *proxy = opaque; + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); if (msix_present(&proxy->pci_dev)) qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n)); } -static int virtio_pci_load_config(void * opaque, QEMUFile *f) +static int virtio_pci_load_config(DeviceState *d, QEMUFile *f) { - VirtIOPCIProxy *proxy = opaque; + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); int ret; ret = pci_device_load(&proxy->pci_dev, f); if (ret) { @@ -144,9 +151,9 @@ static int virtio_pci_load_config(void * opaque, QEMUFile *f) return 0; } -static int virtio_pci_load_queue(void * opaque, int n, QEMUFile *f) +static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f) { - VirtIOPCIProxy *proxy = opaque; + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); uint16_t vector; if (msix_present(&proxy->pci_dev)) { qemu_get_be16s(f, &vector); @@ -244,7 +251,7 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy) void virtio_pci_reset(DeviceState *d) { - VirtIOPCIProxy *proxy = container_of(d, VirtIOPCIProxy, pci_dev.qdev); + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); virtio_pci_stop_ioeventfd(proxy); virtio_reset(proxy->vdev); msix_unuse_all_vectors(&proxy->pci_dev); @@ -464,9 +471,9 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address, } } -static unsigned virtio_pci_get_features(void *opaque) +static unsigned virtio_pci_get_features(DeviceState *d) { - VirtIOPCIProxy *proxy = opaque; + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); return proxy->host_features; } @@ -568,9 +575,9 @@ static void kvm_virtio_pci_vector_release(PCIDevice *dev, unsigned vector) } } -static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign) +static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign) { - VirtIOPCIProxy *proxy = opaque; + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); VirtQueue *vq = virtio_get_queue(proxy->vdev, n); EventNotifier *notifier = virtio_queue_get_guest_notifier(vq); @@ -588,15 +595,15 @@ static int virtio_pci_set_guest_notifier(void *opaque, int n, bool assign) return 0; } -static bool virtio_pci_query_guest_notifiers(void *opaque) +static bool virtio_pci_query_guest_notifiers(DeviceState *d) { - VirtIOPCIProxy *proxy = opaque; + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); return msix_enabled(&proxy->pci_dev); } -static int virtio_pci_set_guest_notifiers(void *opaque, bool assign) +static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign) { - VirtIOPCIProxy *proxy = opaque; + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); VirtIODevice *vdev = proxy->vdev; int r, n; @@ -612,7 +619,7 @@ static int virtio_pci_set_guest_notifiers(void *opaque, bool assign) break; } - r = virtio_pci_set_guest_notifier(opaque, n, assign); + r = virtio_pci_set_guest_notifier(d, n, assign); if (r < 0) { goto assign_error; } @@ -638,14 +645,14 @@ assign_error: /* We get here on assignment failure. Recover by undoing for VQs 0 .. n. */ assert(assign); while (--n >= 0) { - virtio_pci_set_guest_notifier(opaque, n, !assign); + virtio_pci_set_guest_notifier(d, n, !assign); } return r; } -static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign) +static int virtio_pci_set_host_notifier(DeviceState *d, int n, bool assign) { - VirtIOPCIProxy *proxy = opaque; + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); /* Stop using ioeventfd for virtqueue kick if the device starts using host * notifiers. This makes it easy to avoid stepping on each others' toes. @@ -661,9 +668,9 @@ static int virtio_pci_set_host_notifier(void *opaque, int n, bool assign) return virtio_pci_set_host_notifier_internal(proxy, n, assign, false); } -static void virtio_pci_vmstate_change(void *opaque, bool running) +static void virtio_pci_vmstate_change(DeviceState *d, bool running) { - VirtIOPCIProxy *proxy = opaque; + VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(d); if (running) { /* Try to find out if the guest has bus master disabled, but is @@ -728,7 +735,7 @@ void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) proxy->flags &= ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; } - virtio_bind_device(vdev, &virtio_pci_bindings, proxy); + virtio_bind_device(vdev, &virtio_pci_bindings, VIRTIO_PCI_TO_QDEV(proxy)); proxy->host_features |= 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY; proxy->host_features |= 0x1 << VIRTIO_F_BAD_FEATURE; proxy->host_features = vdev->get_features(vdev, proxy->host_features); diff --git a/hw/virtio.c b/hw/virtio.c index f40a8c5..e65d7c8 100644 --- a/hw/virtio.c +++ b/hw/virtio.c @@ -935,7 +943,7 @@ VirtIODevice *virtio_common_init(const char *name, uint16_t device_id, } void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, - void *opaque) + DeviceState *opaque) { vdev->binding = binding; vdev->binding_opaque = opaque; diff --git a/hw/virtio.h b/hw/virtio.h index 7c17f7b..e2e57a4 100644 --- a/hw/virtio.h +++ b/hw/virtio.h @@ -91,17 +91,17 @@ typedef struct VirtQueueElement } VirtQueueElement; typedef struct { - void (*notify)(void * opaque, uint16_t vector); - void (*save_config)(void * opaque, QEMUFile *f); - void (*save_queue)(void * opaque, int n, QEMUFile *f); - int (*load_config)(void * opaque, QEMUFile *f); - int (*load_queue)(void * opaque, int n, QEMUFile *f); - int (*load_done)(void * opaque, QEMUFile *f); - unsigned (*get_features)(void * opaque); - bool (*query_guest_notifiers)(void * opaque); - int (*set_guest_notifiers)(void * opaque, bool assigned); - int (*set_host_notifier)(void * opaque, int n, bool assigned); - void (*vmstate_change)(void * opaque, bool running); + void (*notify)(DeviceState *d, uint16_t vector); + void (*save_config)(DeviceState *d, QEMUFile *f); + void (*save_queue)(DeviceState *d, int n, QEMUFile *f); + int (*load_config)(DeviceState *d, QEMUFile *f); + int (*load_queue)(DeviceState *d, int n, QEMUFile *f); + int (*load_done)(DeviceState *d, QEMUFile *f); + unsigned (*get_features)(DeviceState *d); + bool (*query_guest_notifiers)(DeviceState *d); + int (*set_guest_notifiers)(DeviceState *d, bool assigned); + int (*set_host_notifier)(DeviceState *d, int n, bool assigned); + void (*vmstate_change)(DeviceState *d, bool running); } VirtIOBindings; #define VIRTIO_PCI_QUEUE_MAX 64 @@ -128,7 +128,7 @@ struct VirtIODevice void (*set_status)(VirtIODevice *vdev, uint8_t val); VirtQueue *vq; const VirtIOBindings *binding; - void *binding_opaque; + DeviceState *binding_opaque; uint16_t device_id; bool vm_running; VMChangeStateEntry *vmstate; @@ -187,11 +187,11 @@ uint16_t virtio_queue_vector(VirtIODevice *vdev, int n); void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector); void virtio_set_status(VirtIODevice *vdev, uint8_t val); void virtio_reset(void *opaque); void virtio_update_irq(VirtIODevice *vdev); int virtio_set_features(VirtIODevice *vdev, uint32_t val); void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, - void *opaque); + DeviceState *opaque); /* Base devices. */ typedef struct VirtIOBlkConf VirtIOBlkConf;
Move bindings from opaque to DeviceState. This gives us better type safety with no performance cost. Add macros to make future QOM work easier, document which ones are data-path sensitive. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Changes from v1: - Address comment by Anreas Färber: wrap container_of macros to make future QOM work easier - make a couple of bindings that v1 missed typesafe: virtio doesn't use any void * now