Re: [Qemu-devel] [PATCH v6 3/4] vfio: vfio-pci device assignment driver

[Blue Swirl]

On Fri, Oct 5, 2012 at 5:33 PM, Alex Williamson
<address@hidden> wrote:
> On Fri, 2012-10-05 at 17:22 +0000, Blue Swirl wrote:
>> On Fri, Oct 5, 2012 at 5:11 PM, Alex Williamson
>> <address@hidden> wrote:
>> > On Fri, 2012-10-05 at 16:54 +0000, Blue Swirl wrote:
>> >> On Wed, Sep 26, 2012 at 5:19 PM, Alex Williamson
>> >> <address@hidden> wrote:
>> >> > +
>> >> > +typedef struct QEMU_PACKED VFIOIRQSetFD {
>> >> > +    struct vfio_irq_set irq_set;
>> >> > +    int32_t fd;
>> >> > +} VFIOIRQSetFD;
>> >>
>> >> I'm now getting this error from Clang:
>> >>
>> >> /src/qemu/hw/vfio_pci.c:126:25: error: field 'irq_set' with variable
>> >> sized type 'struct vfio_irq_set' not at the end of a struct or class
>> >> is a GNU extension [-Werror,-Wgnu]
>> >>     struct vfio_irq_set irq_set;
>> >>
>> >> Does the kernel really use the fd field, isn't it implicit from the
>> >> ioctl fd or are they different?
>> >
>> > The kernel side is defined as:
>> >
>> > struct vfio_irq_set {
>> >         __u32   argsz;
>> >         __u32   flags;
>> >         __u32   index;
>> >         __u32   start;
>> >         __u32   count;
>> >         __u8    data[];
>> > };
>>
>> Then the kernel only expects vfio_irq_set structure, not VFIOIRQSetFD,
>> so you should use &irq_set_fd.irq_set instead of &irq_set_fd for the
>> ioctl(). Then VFIOIRQSetFD can be rearranged to have fd field first,
>> also QEMU_PACKED is not necessary.
>
> Sorry, I was unclear.  The kernel sees fd as data[0], that's the point
> of the structure, so re-arranging it makes it useless.  Thanks,

I see. The example in GCC shows how to statically initialize flexible
array members properly but it does not seem to work:
http://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html

Also, Clang does not like that syntax either.

Maybe it's best to use g_malloc with room for the extra int.

>
> Alex
>
>> > Where data is the start of a variable sized array.  The data type of the
>> > array depends on the flags.  The purpose of VFIOIRQSetFD is simply to
>> > make a data type that I don't need to dynamically allocate.  You can
>> > find other cases for MSI and MSIX where we don't know the array size and
>> > do malloc the whole structure.  For this interrupt type we know there's
>> > only one entry.  If there's a better way to do this, let me know.  VFIO
>> > is only available on Linux hosts, so I have no particular reason to
>> > avoid GNU extensions.
>> >
>> >> > +
>> >> > +static int vfio_enable_intx(VFIODevice *vdev)
>> >> > +{
>> >> > +    VFIOIRQSetFD irq_set_fd = {
>> >> > +        .irq_set = {
>> >> > +            .argsz = sizeof(irq_set_fd),
>> >> > +            .flags = VFIO_IRQ_SET_DATA_EVENTFD | 
>> >> > VFIO_IRQ_SET_ACTION_TRIGGER,
>> >> > +            .index = VFIO_PCI_INTX_IRQ_INDEX,
>> >> > +            .start = 0,
>> >> > +            .count = 1,
>> >> > +        },
>> >>
>> >> Here the field is not even initialized.
>> >
>> > It's initialized later...
>> >
>> >> > +    };
>> >> > +    uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 
>> >> > 1);
>> >> > +    int ret;
>> >> > +
>> >> > +    if (vdev->intx.disabled || !pin) {
>> >> > +        return 0;
>> >> > +    }
>> >> > +
>> >> > +    vfio_disable_interrupts(vdev);
>> >> > +
>> >> > +    vdev->intx.pin = pin - 1; /* Pin A (1) -> irq[0] */
>> >> > +    ret = event_notifier_init(&vdev->intx.interrupt, 0);
>> >> > +    if (ret) {
>> >> > +        error_report("vfio: Error: event_notifier_init failed\n");
>> >> > +        return ret;
>> >> > +    }
>> >> > +
>> >> > +    irq_set_fd.fd = event_notifier_get_fd(&vdev->intx.interrupt);
>> >
>> > Here.
>> >
>> > Thanks,
>> > Alex
>> >
>
>
>