| Message ID | 20121217154521.GA28674@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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
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
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
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
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
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
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
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.
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
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
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;
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(-)