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

[Andreas Färber]

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

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