Patchwork virtio: make bindings typesafe

login
register
mail settings
Submitter Michael S. Tsirkin
Date Dec. 17, 2012, 3:45 p.m.
Message ID <20121217154521.GA28674@redhat.com>
Download mbox | patch
Permalink /patch/206898/
State New
Headers show

Comments

Michael S. Tsirkin - Dec. 17, 2012, 3:45 p.m.
Move bindings from opaque to DeviceState.
This gives us better type safety with no performance cost.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/s390-virtio-bus.c |  8 ++++----
 hw/virtio-pci.c      | 48 ++++++++++++++++++++++++------------------------
 hw/virtio.h          | 24 ++++++++++++------------
 3 files changed, 40 insertions(+), 40 deletions(-)
Andreas Färber - Dec. 17, 2012, 5:55 p.m.
Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 3ea4140..63ae888 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -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
Paolo Bonzini - Dec. 17, 2012, 6:21 p.m.
Il 17/12/2012 18:55, Andreas Färber ha scritto:
> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>> index 3ea4140..63ae888 100644
>> --- a/hw/virtio-pci.c
>> +++ b/hw/virtio-pci.c
>> @@ -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!

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

> 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
>
Andreas Färber - Dec. 17, 2012, 6:25 p.m.
Am 17.12.2012 19:21, schrieb Paolo Bonzini:
> Il 17/12/2012 18:55, Andreas Färber ha scritto:
>> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>> index 3ea4140..63ae888 100644
>>> --- a/hw/virtio-pci.c
>>> +++ b/hw/virtio-pci.c
>>> @@ -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!
> 
> 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
Michael S. Tsirkin - Dec. 17, 2012, 8:48 p.m.
On Mon, Dec 17, 2012 at 07:25:08PM +0100, Andreas Färber wrote:
> Am 17.12.2012 19:21, schrieb Paolo Bonzini:
> > Il 17/12/2012 18:55, Andreas Färber ha scritto:
> >> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
> >>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >>> index 3ea4140..63ae888 100644
> >>> --- a/hw/virtio-pci.c
> >>> +++ b/hw/virtio-pci.c
> >>> @@ -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!
> > 
> > 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.


> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Michael S. Tsirkin - Dec. 17, 2012, 8:50 p.m.
On Mon, Dec 17, 2012 at 06:55:46PM +0100, Andreas Färber wrote:
> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > index 3ea4140..63ae888 100644
> > --- a/hw/virtio-pci.c
> > +++ b/hw/virtio-pci.c
> > @@ -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.

No, the core issue is unsafe void * use.

> 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

Me neither just get rid of void*

> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber - Dec. 17, 2012, 9:13 p.m.
Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
> On Mon, Dec 17, 2012 at 07:25:08PM +0100, Andreas Färber wrote:
>> Am 17.12.2012 19:21, schrieb Paolo Bonzini:
>>> Il 17/12/2012 18:55, Andreas Färber ha scritto:
>>>> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
>>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>>>> index 3ea4140..63ae888 100644
>>>>> --- a/hw/virtio-pci.c
>>>>> +++ b/hw/virtio-pci.c
>>>>> @@ -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!
>>>
>>> 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
Michael S. Tsirkin - Dec. 17, 2012, 9:18 p.m.
On Mon, Dec 17, 2012 at 10:13:11PM +0100, Andreas Färber wrote:
> Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
> > On Mon, Dec 17, 2012 at 07:25:08PM +0100, Andreas Färber wrote:
> >> Am 17.12.2012 19:21, schrieb Paolo Bonzini:
> >>> Il 17/12/2012 18:55, Andreas Färber ha scritto:
> >>>> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
> >>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >>>>> index 3ea4140..63ae888 100644
> >>>>> --- a/hw/virtio-pci.c
> >>>>> +++ b/hw/virtio-pci.c
> >>>>> @@ -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!
> >>>
> >>> 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

OK so just

#define VIRTIO_PCI_PROXY(d) container_of(d, VirtIOPCIProxy, pci_dev.qdev)

is OK with you?


> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber - Dec. 17, 2012, 10:08 p.m.
Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
> On Mon, Dec 17, 2012 at 10:13:11PM +0100, Andreas Färber wrote:
>> Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
>>> On Mon, Dec 17, 2012 at 07:25:08PM +0100, Andreas Färber wrote:
>>>> Am 17.12.2012 19:21, schrieb Paolo Bonzini:
>>>>> Il 17/12/2012 18:55, Andreas Färber ha scritto:
>>>>>> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
>>>>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>>>>>> index 3ea4140..63ae888 100644
>>>>>>> --- a/hw/virtio-pci.c
>>>>>>> +++ b/hw/virtio-pci.c
>>>>>>> @@ -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!
>>>>>
>>>>> 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.
> 
> 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
Michael S. Tsirkin - Dec. 17, 2012, 10:58 p.m.
On Mon, Dec 17, 2012 at 11:08:43PM +0100, Andreas Färber wrote:
> Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
> > On Mon, Dec 17, 2012 at 10:13:11PM +0100, Andreas Färber wrote:
> >> Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
> >>> On Mon, Dec 17, 2012 at 07:25:08PM +0100, Andreas Färber wrote:
> >>>> Am 17.12.2012 19:21, schrieb Paolo Bonzini:
> >>>>> Il 17/12/2012 18:55, Andreas Färber ha scritto:
> >>>>>> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
> >>>>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >>>>>>> index 3ea4140..63ae888 100644
> >>>>>>> --- a/hw/virtio-pci.c
> >>>>>>> +++ b/hw/virtio-pci.c
> >>>>>>> @@ -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!
> >>>>>
> >>>>> 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.
> > 
> > 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

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.

> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Andreas Färber - Dec. 18, 2012, 12:13 a.m.
Am 17.12.2012 23:58, schrieb Michael S. Tsirkin:
> On Mon, Dec 17, 2012 at 11:08:43PM +0100, Andreas Färber wrote:
>> Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
>>> On Mon, Dec 17, 2012 at 10:13:11PM +0100, Andreas Färber wrote:
>>>> Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
>>>>> On Mon, Dec 17, 2012 at 07:25:08PM +0100, Andreas Färber wrote:
>>>>>> Am 17.12.2012 19:21, schrieb Paolo Bonzini:
>>>>>>> Il 17/12/2012 18:55, Andreas Färber ha scritto:
>>>>>>>> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
>>>>>>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>>>>>>>> index 3ea4140..63ae888 100644
>>>>>>>>> --- a/hw/virtio-pci.c
>>>>>>>>> +++ b/hw/virtio-pci.c
>>>>>>>>> @@ -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!
>>>>>>>
>>>>>>> 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.
>>>
>>> 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.
> 
> 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.

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
Michael S. Tsirkin - Dec. 18, 2012, 12:30 a.m.
On Tue, Dec 18, 2012 at 01:13:18AM +0100, Andreas Färber wrote:
> Am 17.12.2012 23:58, schrieb Michael S. Tsirkin:
> > On Mon, Dec 17, 2012 at 11:08:43PM +0100, Andreas Färber wrote:
> >> Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
> >>> On Mon, Dec 17, 2012 at 10:13:11PM +0100, Andreas Färber wrote:
> >>>> Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
> >>>>> On Mon, Dec 17, 2012 at 07:25:08PM +0100, Andreas Färber wrote:
> >>>>>> Am 17.12.2012 19:21, schrieb Paolo Bonzini:
> >>>>>>> Il 17/12/2012 18:55, Andreas Färber ha scritto:
> >>>>>>>> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
> >>>>>>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >>>>>>>>> index 3ea4140..63ae888 100644
> >>>>>>>>> --- a/hw/virtio-pci.c
> >>>>>>>>> +++ b/hw/virtio-pci.c
> >>>>>>>>> @@ -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!
> >>>>>>>
> >>>>>>> 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.
> >>>
> >>> 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.
> > 
> > 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.
> 
> 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.
Andreas Färber - Dec. 18, 2012, 12:48 a.m.
Am 18.12.2012 01:30, schrieb Michael S. Tsirkin:
> On Tue, Dec 18, 2012 at 01:13:18AM +0100, Andreas Färber wrote:
>> Am 17.12.2012 23:58, schrieb Michael S. Tsirkin:
>>> On Mon, Dec 17, 2012 at 11:08:43PM +0100, Andreas Färber wrote:
>>>> Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
>>>>> On Mon, Dec 17, 2012 at 10:13:11PM +0100, Andreas Färber wrote:
>>>>>> Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
>>>>>>> On Mon, Dec 17, 2012 at 07:25:08PM +0100, Andreas Färber wrote:
>>>>>>>> Am 17.12.2012 19:21, schrieb Paolo Bonzini:
>>>>>>>>> Il 17/12/2012 18:55, Andreas Färber ha scritto:
>>>>>>>>>> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
>>>>>>>>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
>>>>>>>>>>> index 3ea4140..63ae888 100644
>>>>>>>>>>> --- a/hw/virtio-pci.c
>>>>>>>>>>> +++ b/hw/virtio-pci.c
>>>>>>>>>>> @@ -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!
>>>>>>>>>
>>>>>>>>> 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.
>>>>>
>>>>> 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.
>>>
>>> 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.
>>
>> 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
Michael S. Tsirkin - Dec. 18, 2012, 8:38 a.m.
On Tue, Dec 18, 2012 at 01:48:44AM +0100, Andreas Färber wrote:
> Am 18.12.2012 01:30, schrieb Michael S. Tsirkin:
> > On Tue, Dec 18, 2012 at 01:13:18AM +0100, Andreas Färber wrote:
> >> Am 17.12.2012 23:58, schrieb Michael S. Tsirkin:
> >>> On Mon, Dec 17, 2012 at 11:08:43PM +0100, Andreas Färber wrote:
> >>>> Am 17.12.2012 22:18, schrieb Michael S. Tsirkin:
> >>>>> On Mon, Dec 17, 2012 at 10:13:11PM +0100, Andreas Färber wrote:
> >>>>>> Am 17.12.2012 21:48, schrieb Michael S. Tsirkin:
> >>>>>>> On Mon, Dec 17, 2012 at 07:25:08PM +0100, Andreas Färber wrote:
> >>>>>>>> Am 17.12.2012 19:21, schrieb Paolo Bonzini:
> >>>>>>>>> Il 17/12/2012 18:55, Andreas Färber ha scritto:
> >>>>>>>>>> Am 17.12.2012 16:45, schrieb Michael S. Tsirkin:
> >>>>>>>>>>> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> >>>>>>>>>>> index 3ea4140..63ae888 100644
> >>>>>>>>>>> --- a/hw/virtio-pci.c
> >>>>>>>>>>> +++ b/hw/virtio-pci.c
> >>>>>>>>>>> @@ -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!
> >>>>>>>>>
> >>>>>>>>> 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.
> >>>>>
> >>>>> 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.
> >>>
> >>> 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.
> >>
> >> 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.

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.

> 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
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Patch

diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
index e0ac2d1..6c7c7e5 100644
--- a/hw/s390-virtio-bus.c
+++ b/hw/s390-virtio-bus.c
@@ -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;
 }
 
diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 3ea4140..63ae888 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -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);
     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 = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     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 = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     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 = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     int ret;
     ret = pci_device_load(&proxy->pci_dev, f);
     if (ret) {
@@ -144,9 +144,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 = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     uint16_t vector;
     if (msix_present(&proxy->pci_dev)) {
         qemu_get_be16s(f, &vector);
@@ -464,9 +464,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 = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     return proxy->host_features;
 }
 
@@ -568,9 +568,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 = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     VirtQueue *vq = virtio_get_queue(proxy->vdev, n);
     EventNotifier *notifier = virtio_queue_get_guest_notifier(vq);
 
@@ -588,15 +588,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 = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     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 = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
     VirtIODevice *vdev = proxy->vdev;
     int r, n;
 
@@ -612,7 +612,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 +638,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 = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
 
     /* 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 +661,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 = container_of(d, VirtIOPCIProxy, pci_dev.qdev);
 
     if (running) {
         /* Try to find out if the guest has bus master disabled, but is
diff --git a/hw/virtio.h b/hw/virtio.h
index 7c17f7b..b9eeaa5 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;