Re: [Qemu-devel] [PATCHv2] virtio: make bindings typesafe

[Michael S. Tsirkin]

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 <address@hidden>
> > ---
> > 
> > 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