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

[Anthony Liguori <anthony@codemonkey.ws>]

"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