Re: [Qemu-devel] [PATCH] virtio: make bindings typesafe

["Michael S. Tsirkin" <mst@redhat.com>]

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