virtio: make bindings typesafe
PATCH 1/1 virtio: make bindings typesafe
Move bindings from opaque to DeviceState.
This gives us better type safety with no performance cost.
--- hw/s390-virtio-bus.c | 8 ++++---- hw/virtio-pci.c | 48 ++++++++++++++++++++++++------------------------ hw/virtio.h | 24 ++++++++++++------------ 3 files changed, 40 insertions(+), 40 deletions(-)
hw/s390-virtio-bus.c +4 -4
@@ -364,9 +364,9 @@ VirtIOS390Device *s390_virtio_bus_find_mem(VirtIOS390Bus *bus, ram_addr_t mem)return NULL ; } -static void virtio_s390_notify (void *opaque,uint16_t vector) +static void virtio_s390_notify (DeviceState *d,uint16_t vector) { -VirtIOS390Device *dev = (VirtIOS390Device *)opaque; +VirtIOS390Device *dev=container_of (d, VirtIOS390Device, qdev);uint64_t token =s390_virtio_device_vq_token (dev, vector);S390CPU *cpu =s390_cpu_addr2state (0 );CPUS390XState *env = &cpu->env;
@@ -374,9 +374,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=container_of (d, VirtIOS390Device, qdev);return dev->host_features; }
hw/virtio-pci.c +24 -24
130 lines not on this page · raw message
hw/virtio.h +12 -12
@@ -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 VirtIODevicevoid (*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;
Signature · 1 line
MST
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 13 lines
hw/virtio-pci.c +2
-2@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ -static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);
Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
A core issue being addressed here is that virtio devices are modelled neither in the regular qdev way nor the QOM way. They don't inherit correctly and use their own set of common-init functions - one side effect of Fred's series was to make them first-class QOM citizens. QOM like qdev doesn't support multi-inheritence, so the discussed approach Fred is trying to implement is to have both a VirtioDevice as base class for Virtio{Block,...}Device and PCIDevice/SysBusDevice/... as base class for a virtio bridge device with a virtio-bus, on which only virtio is spoken and pure VirtioDevices can sit (which as you say have device IDs but not all PCI properties).
What remained under discussion AFAIU was how to expose this modelling construct to the user - Peter aiming to expose this to the user and me proposing to hide this (for PCI) as an internal implementation detail.
Whether DeviceState or a new VirtioDevice or something else is being used in the API is a different issue that I don't really mind.
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
Il 17/12/2012 18:55, Andreas Färber ha scritto:
Quoted 9 lines
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
Quoted 8 lines
hw/virtio-pci.c +0
-0@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */
Quoted 8 lines
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 5 lines
hw/virtio-pci.c +2
-2-static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Well, that's just a difference of VIRTIO_PCI_PROXY(d) vs. container_of.
But the patch is still an improvement. In fact it should a logical first step even in Peter's refactoring (where the devicestate is replaced by d->parent_bus or d->parent_bus->parent).
Paolo
Quoted 19 lines
A core issue being addressed here is that virtio devices are modelled neither in the regular qdev way nor the QOM way. They don't inherit correctly and use their own set of common-init functions - one side effect of Fred's series was to make them first-class QOM citizens. QOM like qdev doesn't support multi-inheritence, so the discussed approach Fred is trying to implement is to have both a VirtioDevice as base class for Virtio{Block,...}Device and PCIDevice/SysBusDevice/... as base class for a virtio bridge device with a virtio-bus, on which only virtio is spoken and pure VirtioDevices can sit (which as you say have device IDs but not all PCI properties).
What remained under discussion AFAIU was how to expose this modelling construct to the user - Peter aiming to expose this to the user and me proposing to hide this (for PCI) as an internal implementation detail.Whether DeviceState or a new VirtioDevice or something else is being used in the API is a different issue that I don't really mind.
Andreas
Am 17.12.2012 19:21, schrieb Paolo Bonzini:
Quoted 19 lines
Il 17/12/2012 18:55, Andreas Färber ha scritto:
Quoted 9 lines
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
Quoted 8 lines
hw/virtio-pci.c +0
-0@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ Quoted 8 lines
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 5 lines
hw/virtio-pci.c +2
-2-static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Quoted 1 line
Well, that's just a difference of
VIRTIO_PCI_PROXY(d)vs.container_of.
VIRTIO_PCI_PROXY(d) would be acceptable, sure. But as-is this patch just pushes unnecessary work on Fred, me, you or anyone else who works with QOM.
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 20 lines · Andreas Färber
Am 17.12.2012 19:21, schrieb Paolo Bonzini:
Quoted 19 lines
Il 17/12/2012 18:55, Andreas Färber ha scritto:
Quoted 17 lines
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 13 lines
hw/virtio-pci.c +2
-2@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ -static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Quoted 6 lines
Quoted 1 line
Well, that's just a difference of
VIRTIO_PCI_PROXY(d)vs.container_of.
VIRTIO_PCI_PROXY(d)would be acceptable, sure. But as-is this patch just pushes unnecessary work on Fred, me, you or anyone else who works with QOM.Andreas
What's VIRTIO_PCI_PROXY? Note this is data path we do not want extra code.
Quoted 3 lines
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
Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
Quoted 21 lines · Andreas Färber
Quoted 20 lines
Am 17.12.2012 19:21, schrieb Paolo Bonzini:
Quoted 19 lines
Il 17/12/2012 18:55, Andreas Färber ha scritto:
Quoted 17 lines
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 13 lines
hw/virtio-pci.c +2
-2@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ -static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Well, that's just a difference of
VIRTIO_PCI_PROXY(d)vs.container_of.
VIRTIO_PCI_PROXY(d)would be acceptable, sure. But as-is this patch just pushes unnecessary work on Fred, me, you or anyone else who works with QOM.What's
VIRTIO_PCI_PROXY? Note this is data path we do not want extra code.
My complaint is the direct access of pci_dev, qdev, etc. parent fields in many places as the main change of this patch. Those mean more places to touch in a future patch.
Use of any new-style macro hiding these - wherever the particular one suggested may be defined or whether it needs to be added - is better.
If performance of dynamic_cast is an issue - something I'd leave you to discuss with Anthony - you can just do a C cast directly. Just don't spread this qdev paradigm further please.
Regards,
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 37 lines · Andreas Färber
Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
Quoted 21 lines · Andreas Färber
Quoted 20 lines
Am 17.12.2012 19:21, schrieb Paolo Bonzini:
Quoted 19 lines
Il 17/12/2012 18:55, Andreas Färber ha scritto:
Quoted 17 lines
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 13 lines
hw/virtio-pci.c +2
-2@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ -static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Well, that's just a difference of
VIRTIO_PCI_PROXY(d)vs.container_of.
VIRTIO_PCI_PROXY(d)would be acceptable, sure. But as-is this patch just pushes unnecessary work on Fred, me, you or anyone else who works with QOM.What's
VIRTIO_PCI_PROXY? Note this is data path we do not want extra code.My complaint is the direct access of
pci_dev, qdev, etc. parent fields in many places as the main change of this patch. Those mean more places to touch in a future patch.Use of any new-style macro hiding these - wherever the particular one suggested may be defined or whether it needs to be added - is better.
Quoted 6 lines
If performance of
dynamic_castis an issue - something I'd leave you to discuss with Anthony - you can just do a C cast directly. Just don't spread this qdev paradigm further please.Regards,
Andreas
OK so just
#define VIRTIO_PCI_PROXY(d) container_of(d, VirtIOPCIProxy, pci_dev.qdev)
is OK with you?
Quoted 3 lines
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
Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
Quoted 42 lines · Andreas Färber
Quoted 34 lines
Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
Quoted 21 lines · Andreas Färber
Quoted 20 lines
Am 17.12.2012 19:21, schrieb Paolo Bonzini:
Quoted 19 lines
Il 17/12/2012 18:55, Andreas Färber ha scritto:
Quoted 17 lines
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 13 lines
hw/virtio-pci.c +2
-2@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ -static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Well, that's just a difference of
VIRTIO_PCI_PROXY(d)vs.container_of.
VIRTIO_PCI_PROXY(d)would be acceptable, sure. But as-is this patch just pushes unnecessary work on Fred, me, you or anyone else who works with QOM.What's
VIRTIO_PCI_PROXY? Note this is data path we do not want extra code.My complaint is the direct access of
pci_dev, qdev, etc. parent fields in many places as the main change of this patch. Those mean more places to touch in a future patch.Quoted 6 lines
Use of any new-style macro hiding these - wherever the particular one suggested may be defined or whether it needs to be added - is better.
If performance of
dynamic_castis an issue - something I'd leave you to discuss with Anthony - you can just do a C cast directly. Just don't spread this qdev paradigm further please.
Quoted 5 lines
OK so just
#define
VIRTIO_PCI_PROXY(d)container_of(d, VirtIOPCIProxy, pci_dev.qdev)is OK with you?
Well, at least it's better than inlining it...
I would've expected to see VIRTIO_PCI_PROXY(obj) defined as OBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_something) somewhere.
If, as you imply with "data path", this were a problem, you could just do VirtIOPCIProxy *proxy = (VirtIOPCIProxy *)d inline to allow for VIRTIO_PCI_PROXY() to be used in the QOM sense elsewhere.
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 51 lines · Andreas Färber
Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
Quoted 42 lines · Andreas Färber
Quoted 34 lines
Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
Quoted 21 lines · Andreas Färber
Quoted 20 lines
Am 17.12.2012 19:21, schrieb Paolo Bonzini:
Quoted 19 lines
Il 17/12/2012 18:55, Andreas Färber ha scritto:
Quoted 17 lines
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 13 lines
hw/virtio-pci.c +2
-2@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ -static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Well, that's just a difference of
VIRTIO_PCI_PROXY(d)vs.container_of.
VIRTIO_PCI_PROXY(d)would be acceptable, sure. But as-is this patch just pushes unnecessary work on Fred, me, you or anyone else who works with QOM.What's
VIRTIO_PCI_PROXY? Note this is data path we do not want extra code.My complaint is the direct access of
pci_dev, qdev, etc. parent fields in many places as the main change of this patch. Those mean more places to touch in a future patch.Quoted 6 lines
Use of any new-style macro hiding these - wherever the particular one suggested may be defined or whether it needs to be added - is better.
If performance of
dynamic_castis an issue - something I'd leave you to discuss with Anthony - you can just do a C cast directly. Just don't spread this qdev paradigm further please.Quoted 5 lines
OK so just
#define
VIRTIO_PCI_PROXY(d)container_of(d, VirtIOPCIProxy, pci_dev.qdev)is OK with you?
Well, at least it's better than inlining it...
Quoted 8 lines
I would've expected to see
VIRTIO_PCI_PROXY(obj)defined asOBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_something)somewhere.If, as you imply with "data path", this were a problem, you could just do VirtIOPCIProxy *proxy = (VirtIOPCIProxy *)d inline to allow for
VIRTIO_PCI_PROXY()to be used in the QOM sense elsewhere.Andreas
I don't get it - where?
Since we don't do runtime checks we need container_of - safer than a plain cast.
Anyway, when you start doing your QOM conversions it will be easy to do what you like.
Quoted 3 lines
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
Am 17.12.2012 23:58, schrieb Michael S. Tsirkin:
Quoted 59 lines · Andreas Färber
Quoted 49 lines
Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
Quoted 42 lines · Andreas Färber
Quoted 34 lines
Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
Quoted 21 lines · Andreas Färber
Quoted 20 lines
Am 17.12.2012 19:21, schrieb Paolo Bonzini:
Quoted 19 lines
Il 17/12/2012 18:55, Andreas Färber ha scritto:
Quoted 17 lines
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 13 lines
hw/virtio-pci.c +2
-2@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ -static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Well, that's just a difference of
VIRTIO_PCI_PROXY(d)vs.container_of.
VIRTIO_PCI_PROXY(d)would be acceptable, sure. But as-is this patch just pushes unnecessary work on Fred, me, you or anyone else who works with QOM.What's
VIRTIO_PCI_PROXY? Note this is data path we do not want extra code.My complaint is the direct access of
pci_dev, qdev, etc. parent fields in many places as the main change of this patch. Those mean more places to touch in a future patch.Quoted 6 lines
Use of any new-style macro hiding these - wherever the particular one suggested may be defined or whether it needs to be added - is better.
If performance of
dynamic_castis an issue - something I'd leave you to discuss with Anthony - you can just do a C cast directly. Just don't spread this qdev paradigm further please.Quoted 5 lines
OK so just
#define
VIRTIO_PCI_PROXY(d)container_of(d, VirtIOPCIProxy, pci_dev.qdev)is OK with you?
Quoted 8 lines
Well, at least it's better than inlining it...
I would've expected to see
VIRTIO_PCI_PROXY(obj)defined asOBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_something)somewhere.If, as you imply with "data path", this were a problem, you could just do VirtIOPCIProxy *proxy = (VirtIOPCIProxy *)d inline to allow for
VIRTIO_PCI_PROXY()to be used in the QOM sense elsewhere.
Quoted 6 lines
I don't get it - where?
Since we don't do runtime checks we needcontainer_of- safer than a plain cast.Anyway, when you start doing your QOM conversions it will be easy to do what you like.
I don't get what you don't get - QOM has been around for nearly a year now and it's neither my invention nor my conversion, it's ours! It's an object-orientation framework for C to avoid a flag day for rewriting everything in a new language, while allowing us to get a number of new features.
In object-oriented languages like C++, C# or Java there is no concept of accessing parent state by name with or without container_of(); that is a QOM implementation detail as long as we don't generate the parent's fields in the actual struct via some QIDL'ish preprocessing or by switching to a language that supports it. Does this explain better?
Every maintainer should review patches to conform to QOM, just like I review for Coding Style issues even if I don't personally agree to every bit of what became the consensus. That has exactly nothing to do with me, I may be many things but not QOM maintainer. Having Anthony or any single maintainer follow-up everyone's devices with fixes for QOM simply doesn't scale. And when I do refactorings to get something particular accomplished, I can't do any major benchmarking of each device, so I need an easy way to tell if someone was merely sloppy or utterly clever about the choice of macros.
Late rebellions against QOM are just as tiring as the constant Coding Style wars in the audio code. If you have an idea to improve QOM cast performance (maybe disable some checks for non-debug builds?) that would benefit everyone rather than just having PCI/virtio be different from the rest of the code base.
Having said that, yes, container_of() assures that VirtIOPCIProxy has a PCIDevice field somewhere, but when you pass (Device *)sth_not_a_device in the caller, that check becomes totally moot. Having a typed argument is not moot, of course (I believe Fred was proposing the more specific VirtioDevice* for that). But since there are only two callers passing in DeviceState* to the bind function today (maybe four soon) I fail to see the benefit of container_of() you are trying to sell me here...
Regards,
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 60 lines · Andreas Färber
Am 17.12.2012 23:58, schrieb Michael S. Tsirkin:
Quoted 59 lines · Andreas Färber
Quoted 57 lines
Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
Quoted 42 lines · Andreas Färber
Quoted 34 lines
Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
Quoted 21 lines · Andreas Färber
Quoted 20 lines
Am 17.12.2012 19:21, schrieb Paolo Bonzini:
Quoted 19 lines
Il 17/12/2012 18:55, Andreas Färber ha scritto:
Quoted 17 lines
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 13 lines
hw/virtio-pci.c +2
-2@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ -static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Well, that's just a difference of
VIRTIO_PCI_PROXY(d)vs.container_of.
VIRTIO_PCI_PROXY(d)would be acceptable, sure. But as-is this patch just pushes unnecessary work on Fred, me, you or anyone else who works with QOM.What's
VIRTIO_PCI_PROXY? Note this is data path we do not want extra code.My complaint is the direct access of
pci_dev, qdev, etc. parent fields in many places as the main change of this patch. Those mean more places to touch in a future patch.Quoted 6 lines
Use of any new-style macro hiding these - wherever the particular one suggested may be defined or whether it needs to be added - is better.
If performance of
dynamic_castis an issue - something I'd leave you to discuss with Anthony - you can just do a C cast directly. Just don't spread this qdev paradigm further please.Quoted 5 lines
OK so just
#define
VIRTIO_PCI_PROXY(d)container_of(d, VirtIOPCIProxy, pci_dev.qdev)is OK with you?
Well, at least it's better than inlining it...
I would've expected to see
VIRTIO_PCI_PROXY(obj)defined asOBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_something)somewhere.If, as you imply with "data path", this were a problem, you could just do VirtIOPCIProxy *proxy = (VirtIOPCIProxy *)d inline to allow for
VIRTIO_PCI_PROXY()to be used in the QOM sense elsewhere.
Quoted 8 lines
Quoted 6 lines
I don't get it - where?
Since we don't do runtime checks we needcontainer_of- safer than a plain cast.Anyway, when you start doing your QOM conversions it will be easy to do what you like.
I don't get what you don't get
Wha'ts the QOM way to get virtio pci proxy from
devicestate?
C cast is not what I am looking for.
Am 18.12.2012 01:30, schrieb Michael S. Tsirkin:
Quoted 65 lines · Andreas Färber
Quoted 64 lines
Am 17.12.2012 23:58, schrieb Michael S. Tsirkin:
Quoted 63 lines · Andreas Färber
Quoted 49 lines
Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
Quoted 42 lines · Andreas Färber
Quoted 34 lines
Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
Quoted 21 lines · Andreas Färber
Quoted 20 lines
Am 17.12.2012 19:21, schrieb Paolo Bonzini:
Quoted 19 lines
Il 17/12/2012 18:55, Andreas Färber ha scritto:
Quoted 17 lines
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 13 lines
hw/virtio-pci.c +2
-2@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ -static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Well, that's just a difference of
VIRTIO_PCI_PROXY(d)vs.container_of.
VIRTIO_PCI_PROXY(d)would be acceptable, sure. But as-is this patch just pushes unnecessary work on Fred, me, you or anyone else who works with QOM.What's
VIRTIO_PCI_PROXY? Note this is data path we do not want extra code.My complaint is the direct access of
pci_dev, qdev, etc. parent fields in many places as the main change of this patch. Those mean more places to touch in a future patch.Quoted 6 lines
Use of any new-style macro hiding these - wherever the particular one suggested may be defined or whether it needs to be added - is better.
If performance of
dynamic_castis an issue - something I'd leave you to discuss with Anthony - you can just do a C cast directly. Just don't spread this qdev paradigm further please.Quoted 5 lines
OK so just
#define
VIRTIO_PCI_PROXY(d)container_of(d, VirtIOPCIProxy, pci_dev.qdev)is OK with you?
Quoted 8 lines
Well, at least it's better than inlining it...
I would've expected to see
VIRTIO_PCI_PROXY(obj)defined asOBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_something)somewhere.If, as you imply with "data path", this were a problem, you could just do VirtIOPCIProxy *proxy = (VirtIOPCIProxy *)d inline to allow for
VIRTIO_PCI_PROXY()to be used in the QOM sense elsewhere.I don't get it - where?
Since we don't do runtime checks we needcontainer_of- safer than a plain cast.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Anyway, when you start doing your QOM conversions it will be easy to do what you like.
I don't get what you don't get
Wha'ts the QOM way to get virtio pci proxy from
devicestate?
C cast is not what I am looking for.
Looking into virtio-pci.c it looks like virtio has a similar deficiency as EHCI USB (my recent series): We lack an abstract intermediate type TYPE_VIRTIO_PCI_PROXY to match the struct VirtIOPCIProxy shared by its subtypes:
Object
- DeviceState
- PCIDevice
- VirtIOPCIProxy
- virtio-scsi-pci
- virtio-rng-pci
...
Not sure if that can be extracted from Fred's series already; otherwise I can send you a patch.
Then you can do the mentioned:
#define VIRTIO_PCI_PROXY(obj) \
OBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_VIRTIO_PCI_PROXY)
DeviceState *dev = ...;
VirtIOPCIProxy *proxy = VIRTIO_PCI_PROXY(dev);
where you consider it acceptable performance-wise and a FAST_VIRTIO_PCI_PROXY_FROM_DEVICE(dev) or so elsewhere.
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 88 lines · Andreas Färber
Am 18.12.2012 01:30, schrieb Michael S. Tsirkin:
Quoted 65 lines · Andreas Färber
Quoted 64 lines
Am 17.12.2012 23:58, schrieb Michael S. Tsirkin:
Quoted 63 lines · Andreas Färber
Quoted 49 lines
Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
Quoted 42 lines · Andreas Färber
Quoted 34 lines
Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
Quoted 21 lines · Andreas Färber
Quoted 20 lines
Am 17.12.2012 19:21, schrieb Paolo Bonzini:
Quoted 19 lines
Il 17/12/2012 18:55, Andreas Färber ha scritto:
Quoted 17 lines
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 13 lines
hw/virtio-pci.c +2
-2@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ -static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Well, that's just a difference of
VIRTIO_PCI_PROXY(d)vs.container_of.
VIRTIO_PCI_PROXY(d)would be acceptable, sure. But as-is this patch just pushes unnecessary work on Fred, me, you or anyone else who works with QOM.What's
VIRTIO_PCI_PROXY? Note this is data path we do not want extra code.My complaint is the direct access of
pci_dev, qdev, etc. parent fields in many places as the main change of this patch. Those mean more places to touch in a future patch.Quoted 6 lines
Use of any new-style macro hiding these - wherever the particular one suggested may be defined or whether it needs to be added - is better.
If performance of
dynamic_castis an issue - something I'd leave you to discuss with Anthony - you can just do a C cast directly. Just don't spread this qdev paradigm further please.Quoted 5 lines
OK so just
#define
VIRTIO_PCI_PROXY(d)container_of(d, VirtIOPCIProxy, pci_dev.qdev)is OK with you?
Quoted 8 lines
Well, at least it's better than inlining it...
I would've expected to see
VIRTIO_PCI_PROXY(obj)defined asOBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_something)somewhere.If, as you imply with "data path", this were a problem, you could just do VirtIOPCIProxy *proxy = (VirtIOPCIProxy *)d inline to allow for
VIRTIO_PCI_PROXY()to be used in the QOM sense elsewhere.I don't get it - where?
Since we don't do runtime checks we needcontainer_of- safer than a plain cast.Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Anyway, when you start doing your QOM conversions it will be easy to do what you like.
I don't get what you don't get
Wha'ts the QOM way to get virtio pci proxy from
devicestate?
C cast is not what I am looking for.Looking into
virtio-pci.cit looks like virtio has a similar deficiency as EHCI USB (my recent series): We lack an abstract intermediate typeTYPE_VIRTIO_PCI_PROXYto match the struct VirtIOPCIProxy shared by its subtypes:Object
- DeviceState
- PCIDevice
- VirtIOPCIProxy
- virtio-scsi-pci
- virtio-rng-pci
...
Quoted 2 lines
Not sure if that can be extracted from Fred's series already; otherwise I can send you a patch.
I'd like to avoid the dependency - let me do the rework using simple container_of meanwhile, then Fred's series can change the cast in a single place.
Quoted 16 lines
Then you can do the mentioned:
#define
VIRTIO_PCI_PROXY(obj)\
OBJECT_CHECK(VirtIOPCIProxy, (obj), TYPE_VIRTIO_PCI_PROXY)DeviceState *dev = ...;
VirtIOPCIProxy *proxy =VIRTIO_PCI_PROXY(dev);where you consider it acceptable performance-wise and a
FAST_VIRTIO_PCI_PROXY_FROM_DEVICE(dev)or so elsewhere.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 18 lines · Andreas Färber
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
hw/virtio-pci.c bool virtio_is_big_endian(void); · line 103
Quoted 13 lines
hw/virtio-pci.c +2
-2@@ -98,34 +98,34 @@ bool virtio_is_big_endian(void);/* virtio device */ -static void virtio_pci_notify (void *opaque,uint16_t vector) +static void virtio_pci_notify (DeviceState *d,uint16_t vector) { -VirtIOPCIProxy *proxy = opaque; +VirtIOPCIProxy *proxy =container_of (d, VirtIOPCIProxy, pci_dev.qdev);Nack. This is going the wrong direction QOM-wise and you among all others know that from PCI host bridges!
Quoted 2 lines
A core issue being addressed here is that virtio devices are modelled neither in the regular qdev way nor the QOM way.
No, the core issue is unsafe void * use.
Quoted 13 lines
They don't inherit
correctly and use their own set of common-init functions - one side effect of Fred's series was to make them first-class QOM citizens. QOM like qdev doesn't support multi-inheritence, so the discussed approach Fred is trying to implement is to have both a VirtioDevice as base class for Virtio{Block,...}Device and PCIDevice/SysBusDevice/... as base class for a virtio bridge device with a virtio-bus, on which only virtio is spoken and pure VirtioDevices can sit (which as you say have device IDs but not all PCI properties).
What remained under discussion AFAIU was how to expose this modelling construct to the user - Peter aiming to expose this to the user and me proposing to hide this (for PCI) as an internal implementation detail.
Quoted 4 lines
Whether DeviceState or a new VirtioDevice or something else is being used in the API is a different issue that I don't really mind.
Andreas
Me neither just get rid of void*
Quoted 3 lines
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