virtio: make bindings typesafe

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 <[email protected]>

---

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

hw/s390-virtio-bus.c +0 -0

index e0ac2d1..8c693b4 100644

hw/s390-virtio-bus.c +2 -2

     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;
 }
 

hw/virtio-pci.c +0 -0

index 3ea4140..1e9566a 100644

hw/virtio-pci.c +3 -1

+
+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))

hw/virtio-pci.c +14 -14

     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

hw/virtio-pci.c +0 -0

     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);

hw/virtio.c +1 -1

index f40a8c5..e65d7c8 100644
@@ -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;

hw/virtio.h +12 -12

index 7c17f7b..e2e57a4 100644
@@ -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);

Am 17.12.2012 22:40, schrieb Michael S. Tsirkin:

Quoted 20 lines

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 <[email protected]>

---

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

hw/s390-virtio-bus.c +0 -0

index e0ac2d1..8c693b4 100644
@@ -137,7 +137,7 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev)
 

hw/s390-virtio-bus.c static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev) · line 140

Quoted 4 lines

hw/s390-virtio-bus.c +1 -1

     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.

hw/s390-virtio-bus.c VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus, ram_addr_t mem) · line 368

Quoted 10 lines

hw/s390-virtio-bus.c +2 -1

     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.

hw/s390-virtio-bus.c VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus, ram_addr_t mem) · line 373

Quoted 5 lines

hw/s390-virtio-bus.c +5 -0

+
+/* 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...

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.

Quoted 22 lines

hw/s390-virtio-bus.c +2 -2

     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;
 }
 

hw/virtio-pci.c +0 -0

index 3ea4140..1e9566a 100644
@@ -97,35 +97,42 @@
 bool virtio_is_big_endian(void);
 

hw/virtio-pci.c line 101

Quoted 3 lines

hw/virtio-pci.c +2 -0

 /* virtio device */
+/* VirtIOPCIProxy to DeviceState. */
+#define VIRTIO_PCI_TO_QDEV(proxy) (&(proxy)->pci_dev.qdev)

Unneeded.

hw/virtio-pci.c line 106

Quoted 7 lines

hw/virtio-pci.c +4 -1

-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.

Quoted 11 lines

hw/virtio-pci.c +3 -1

+
+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);
 }
 

hw/virtio-pci.c line 119

Quoted 5 lines

hw/virtio-pci.c +2 -2

-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?

hw/virtio-pci.c line 128

Quoted 11 lines

hw/virtio-pci.c +2 -2

     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?

hw/virtio-pci.c line 135

Quoted 9 lines

hw/virtio-pci.c +2 -2

     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?

hw/virtio-pci.c static int virtio_pci_load_config(void * opaque, QEMUFile *f) · line 156

Quoted 12 lines

hw/virtio-pci.c +2 -2

     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?

hw/virtio-pci.c static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy) · line 254

Quoted 9 lines

hw/virtio-pci.c +1 -1

     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.

Quoted 87 lines

hw/virtio-pci.c +14 -14

     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

hw/virtio-pci.c void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) · line 738

Quoted 5 lines

hw/virtio-pci.c +1 -1

@@ -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.

Quoted 61 lines

hw/virtio-pci.c +0 -0

     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);

hw/virtio.c +1 -1

index f40a8c5..e65d7c8 100644
@@ -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;

hw/virtio.h +12 -12

index 7c17f7b..e2e57a4 100644
@@ -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);

hw/virtio.h uint16_t virtio_queue_vector(VirtIODevice *vdev, int n); · line 197

Quoted 10 lines

hw/virtio.h +1 -1

 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

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Quoted 21 lines · Andreas Färber

Am 17.12.2012 22:40, schrieb Michael S. Tsirkin:

Quoted 20 lines

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 <[email protected]>

---

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

hw/s390-virtio-bus.c +0 -0

index e0ac2d1..8c693b4 100644
@@ -137,7 +137,7 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev)
 

Quoted 7 lines

hw/s390-virtio-bus.c static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev) · line 140

Quoted 4 lines

hw/s390-virtio-bus.c +1 -1

     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.

Quoted 25 lines

hw/s390-virtio-bus.c VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus, ram_addr_t mem) · line 368

Quoted 11 lines

hw/s390-virtio-bus.c +2 -1

     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.

hw/s390-virtio-bus.c VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus, ram_addr_t mem) · line 373

Quoted 5 lines

hw/s390-virtio-bus.c +5 -0

+
+/* 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.

Quoted 1 line

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.

Quoted 3 lines

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_*

Review

Quoted 9 lines

hw/s390-virtio-bus.c VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus, ram_addr_t mem) · line 377

Quoted 5 lines

hw/s390-virtio-bus.c +3 -1

+
+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:

(quoted diff) +1 -1

-    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.

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 22 lines

Quoted 22 lines

hw/s390-virtio-bus.c +2 -2

     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;
 }
 

hw/virtio-pci.c +0 -0

index 3ea4140..1e9566a 100644
@@ -97,35 +97,42 @@
 bool virtio_is_big_endian(void);
 

Quoted 5 lines

hw/virtio-pci.c line 101

Quoted 3 lines

hw/virtio-pci.c +2 -0

 /* virtio device */
+/* VirtIOPCIProxy to DeviceState. */
+#define VIRTIO_PCI_TO_QDEV(proxy) (&(proxy)->pci_dev.qdev)

Unneeded.

Same answer everywhere below.

Quoted 20 lines

hw/virtio-pci.c line 106

Quoted 7 lines

hw/virtio-pci.c +4 -1

-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.

Quoted 11 lines

hw/virtio-pci.c +3 -1

+
+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);
 }
 

Quoted 7 lines

hw/virtio-pci.c line 119

Quoted 5 lines

hw/virtio-pci.c +2 -2

-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.

Quoted 225 lines

hw/virtio-pci.c line 128

Quoted 11 lines

hw/virtio-pci.c +2 -2

     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?

hw/virtio-pci.c line 135

Quoted 9 lines

hw/virtio-pci.c +2 -2

     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?

hw/virtio-pci.c static int virtio_pci_load_config(void * opaque, QEMUFile *f) · line 156

Quoted 12 lines

hw/virtio-pci.c +2 -2

     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?

hw/virtio-pci.c static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy) · line 254

Quoted 9 lines

hw/virtio-pci.c +1 -1

     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.

Quoted 87 lines

hw/virtio-pci.c +14 -14

     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

hw/virtio-pci.c void virtio_init_pci(VirtIOPCIProxy *proxy, VirtIODevice *vdev) · line 738

Quoted 5 lines

hw/virtio-pci.c +1 -1

@@ -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.

Quoted 61 lines

hw/virtio-pci.c +0 -0

     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);

hw/virtio.c +1 -1

index f40a8c5..e65d7c8 100644
@@ -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;

hw/virtio.h +12 -12

index 7c17f7b..e2e57a4 100644
@@ -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);

hw/virtio.h uint16_t virtio_queue_vector(VirtIODevice *vdev, int n); · line 197

Quoted 10 lines

hw/virtio.h +1 -1

 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

Signature · 2 lines

SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

Quoted 30 lines · Michael S. Tsirkin

Quoted 21 lines · Andreas Färber

Am 17.12.2012 22:40, schrieb Michael S. Tsirkin:

Quoted 20 lines

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 <[email protected]>

---

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

hw/s390-virtio-bus.c +0 -0

index e0ac2d1..8c693b4 100644
@@ -137,7 +137,7 @@ static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev)
 

Quoted 7 lines

hw/s390-virtio-bus.c static int s390_virtio_device_init(VirtIOS390Device *dev, VirtIODevice *vdev) · line 140

Quoted 4 lines

hw/s390-virtio-bus.c +1 -1

     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.

Quoted 5 lines

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

Quoted 4 lines

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.