From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:41306) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TklGz-00075Y-Od for qemu-devel@nongnu.org; Mon, 17 Dec 2012 19:43:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TklGx-0002y4-4c for qemu-devel@nongnu.org; Mon, 17 Dec 2012 19:43:05 -0500 Received: from mail-ob0-f171.google.com ([209.85.214.171]:34279) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TklGw-0002xb-Tl for qemu-devel@nongnu.org; Mon, 17 Dec 2012 19:43:03 -0500 Received: by mail-ob0-f171.google.com with SMTP id dn14so26859obc.2 for ; Mon, 17 Dec 2012 16:43:01 -0800 (PST) From: Anthony Liguori In-Reply-To: <20121217232725.GA12878@redhat.com> References: <20121217214042.GA11926@redhat.com> <50CFA3C3.4030707@suse.de> <20121217232725.GA12878@redhat.com> Date: Mon, 17 Dec 2012 18:42:58 -0600 Message-ID: <878v8w5gx9.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCHv2] virtio: make bindings typesafe List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , Andreas =?utf-8?Q?F=C3=A4rber?= Cc: Stefan Hajnoczi , Jan Kiszka , qemu-devel@nongnu.org, Alexander Graf , Christian Borntraeger , pbonzini@redhat.com "Michael S. Tsirkin" writes: > On Mon, Dec 17, 2012 at 11:59:15PM +0100, Andreas F=C3=A4rber 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. >> >=20 >> > Signed-off-by: Michael S. Tsirkin >> > --- >> >=20 >> > Changes from v1: >> > - Address comment by Anreas F=C3=A4rber: 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 >> >=20 >> > 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(VirtIOS390Devic= e *dev, VirtIODevice *vdev) >> >=20=20 >> > bus->dev_offs +=3D dev_len; >> >=20=20 >> > - virtio_bind_device(vdev, &virtio_s390_bindings, dev); >> > + virtio_bind_device(vdev, &virtio_s390_bindings, VIRTIO_S390_TO_QD= EV(dev)); >>=20 >> 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 =3D vdev->get_features(vdev, dev->host_feature= s); >> > s390_virtio_device_sync(dev); >> > s390_virtio_reset_idx(dev); >> > @@ -364,9 +364,17 @@ VirtIOS390Device *s390_virtio_bus_find_mem(VirtIO= S390Bus *bus, ram_addr_t mem) >> > return NULL; >> > } >> >=20=20 >> > -static void virtio_s390_notify(void *opaque, uint16_t vector) >> > +/* VirtIOS390Device to DeviceState */ >> > +#define VIRTIO_S390_TO_QDEV(dev) (&(dev)->qdev) >>=20 >> Unneeded, and "QDEV" naming is not nice either. >>=20 >> Definition after use. >>=20 >> > + >> > +/* 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) >>=20 >> This leaves no name for a non-performance-critical macro we would expect >> under this name following the QOM naming conventions. >>=20 >> 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=20 > 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 =3D (VirtIOS390Device*)opaque; >> > + VirtIOS390Device *dev =3D VIRTIO_S390_DEVICE(d); >>=20 >> Why not simply for the hot path: >> - VirtIOS390Device *dev =3D (VirtIOS390Device*)opaque; >> + VirtIOS390Device *dev =3D (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. >>=20 >> 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 =3D s390_virtio_device_vq_token(dev, vector); >> > S390CPU *cpu =3D s390_cpu_addr2state(0); >> > CPUS390XState *env =3D &cpu->env; >> > @@ -374,9 +382,9 @@ static void virtio_s390_notify(void *opaque, uint1= 6_t vector) >> > s390_virtio_irq(env, 0, token); >> > } >> >=20=20 >> > -static unsigned virtio_s390_get_features(void *opaque) >> > +static unsigned virtio_s390_get_features(DeviceState *d) >> > { >> > - VirtIOS390Device *dev =3D (VirtIOS390Device*)opaque; >> > + VirtIOS390Device *dev =3D VIRTIO_S390_DEVICE(d); >> > return dev->host_features; >> > } >> >=20=20 >> > 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); >> >=20=20 >> > /* virtio device */ >> > +/* VirtIOPCIProxy to DeviceState. */ >> > +#define VIRTIO_PCI_TO_QDEV(proxy) (&(proxy)->pci_dev.qdev) >>=20 >> Unneeded. > > Same answer everywhere below. > >>=20 >> >=20=20 >> > -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.q= dev) >>=20 >> Same comment as for s390. >> > + >> > +static void virtio_pci_notify(DeviceState *d, uint16_t vector) >> > { >> > - VirtIOPCIProxy *proxy =3D opaque; >> > + VirtIOPCIProxy *proxy =3D 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); >> > } >> >=20=20 >> > -static void virtio_pci_save_config(void * opaque, QEMUFile *f) >> > +static void virtio_pci_save_config(DeviceState *d, QEMUFile *f) >> > { >> > - VirtIOPCIProxy *proxy =3D opaque; >> > + VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(d); >>=20 >> 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); >> > } >> >=20=20 >> > -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 =3D opaque; >> > + VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(d); >>=20 >> Ditto? >>=20 >> > if (msix_present(&proxy->pci_dev)) >> > qemu_put_be16(f, virtio_queue_vector(proxy->vdev, n)); >> > } >> >=20=20 >> > -static int virtio_pci_load_config(void * opaque, QEMUFile *f) >> > +static int virtio_pci_load_config(DeviceState *d, QEMUFile *f) >> > { >> > - VirtIOPCIProxy *proxy =3D opaque; >> > + VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(d); >>=20 >> Same for loading from a file? >>=20 >> > int ret; >> > ret =3D pci_device_load(&proxy->pci_dev, f); >> > if (ret) { >> > @@ -144,9 +151,9 @@ static int virtio_pci_load_config(void * opaque, Q= EMUFile *f) >> > return 0; >> > } >> >=20=20 >> > -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 =3D opaque; >> > + VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(d); >>=20 >> Ditto? >>=20 >> > uint16_t vector; >> > if (msix_present(&proxy->pci_dev)) { >> > qemu_get_be16s(f, &vector); >> > @@ -244,7 +251,7 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIPro= xy *proxy) >> >=20=20 >> > void virtio_pci_reset(DeviceState *d) >> > { >> > - VirtIOPCIProxy *proxy =3D container_of(d, VirtIOPCIProxy, pci_dev= .qdev); >> > + VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(d); >>=20 >> Reset is not a hot path. >>=20 >> > 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, >> > } >> > } >> >=20=20 >> > -static unsigned virtio_pci_get_features(void *opaque) >> > +static unsigned virtio_pci_get_features(DeviceState *d) >> > { >> > - VirtIOPCIProxy *proxy =3D opaque; >> > + VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(d); >> > return proxy->host_features; >> > } >> >=20=20 >> > @@ -568,9 +575,9 @@ static void kvm_virtio_pci_vector_release(PCIDevic= e *dev, unsigned vector) >> > } >> > } >> >=20=20 >> > -static int virtio_pci_set_guest_notifier(void *opaque, int n, bool as= sign) >> > +static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool = assign) >> > { >> > - VirtIOPCIProxy *proxy =3D opaque; >> > + VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(d); >> > VirtQueue *vq =3D virtio_get_queue(proxy->vdev, n); >> > EventNotifier *notifier =3D virtio_queue_get_guest_notifier(vq); >> >=20=20 >> > @@ -588,15 +595,15 @@ static int virtio_pci_set_guest_notifier(void *o= paque, int n, bool assign) >> > return 0; >> > } >> >=20=20 >> > -static bool virtio_pci_query_guest_notifiers(void *opaque) >> > +static bool virtio_pci_query_guest_notifiers(DeviceState *d) >> > { >> > - VirtIOPCIProxy *proxy =3D opaque; >> > + VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(d); >> > return msix_enabled(&proxy->pci_dev); >> > } >> >=20=20 >> > -static int virtio_pci_set_guest_notifiers(void *opaque, bool assign) >> > +static int virtio_pci_set_guest_notifiers(DeviceState *d, bool assign) >> > { >> > - VirtIOPCIProxy *proxy =3D opaque; >> > + VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(d); >> > VirtIODevice *vdev =3D proxy->vdev; >> > int r, n; >> >=20=20 >> > @@ -612,7 +619,7 @@ static int virtio_pci_set_guest_notifiers(void *op= aque, bool assign) >> > break; >> > } >> >=20=20 >> > - r =3D virtio_pci_set_guest_notifier(opaque, n, assign); >> > + r =3D 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 >=3D 0) { >> > - virtio_pci_set_guest_notifier(opaque, n, !assign); >> > + virtio_pci_set_guest_notifier(d, n, !assign); >> > } >> > return r; >> > } >> >=20=20 >> > -static int virtio_pci_set_host_notifier(void *opaque, int n, bool ass= ign) >> > +static int virtio_pci_set_host_notifier(DeviceState *d, int n, bool a= ssign) >> > { >> > - VirtIOPCIProxy *proxy =3D opaque; >> > + VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(d); >> >=20=20 >> > /* Stop using ioeventfd for virtqueue kick if the device starts u= sing host >> > * notifiers. This makes it easy to avoid stepping on each other= s' toes. >> > @@ -661,9 +668,9 @@ static int virtio_pci_set_host_notifier(void *opaq= ue, int n, bool assign) >> > return virtio_pci_set_host_notifier_internal(proxy, n, assign, fa= lse); >> > } >> >=20=20 >> > -static void virtio_pci_vmstate_change(void *opaque, bool running) >> > +static void virtio_pci_vmstate_change(DeviceState *d, bool running) >> > { >> > - VirtIOPCIProxy *proxy =3D opaque; >> > + VirtIOPCIProxy *proxy =3D VIRTIO_PCI_PROXY(d); >> >=20=20 >> > 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, VirtIO= Device *vdev) >> > proxy->flags &=3D ~VIRTIO_PCI_FLAG_USE_IOEVENTFD; >> > } >> >=20=20 >> > - virtio_bind_device(vdev, &virtio_pci_bindings, proxy); >> > + virtio_bind_device(vdev, &virtio_pci_bindings, VIRTIO_PCI_TO_QDEV= (proxy)); >>=20 >> DEVICE(proxy) - device init not a hot path. >>=20 >> > proxy->host_features |=3D 0x1 << VIRTIO_F_NOTIFY_ON_EMPTY; >> > proxy->host_features |=3D 0x1 << VIRTIO_F_BAD_FEATURE; >> > proxy->host_features =3D vdev->get_features(vdev, proxy->host_fea= tures); >> > 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, >> > } >> >=20=20 >> > void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *bin= ding, >> > - void *opaque) >> > + DeviceState *opaque) >> > { >> > vdev->binding =3D binding; >> > vdev->binding_opaque =3D 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; >> >=20=20 >> > 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; >> >=20=20 >> > #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 vect= or); >> > 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); >> >=20=20 >> > void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *bin= ding, >> > - void *opaque); >> > + DeviceState *opaque); >> >=20=20 >> > /* Base devices. */ >> > typedef struct VirtIOBlkConf VirtIOBlkConf; >>=20 >> Andreas >>=20 >> --=20 >> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany >> GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3= =BCrnberg