"Michael S. Tsirkin" <m...@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 <m...@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
