Re: [Qemu-devel] [PATCH v7 09/16] hw/vfio/platform: add vfio-platform support

[Eric Auger]

On 11/26/2014 11:24 AM, Alexander Graf wrote:
> 
> 
> On 26.11.14 10:45, Eric Auger wrote:
>> On 11/05/2014 11:29 AM, Alexander Graf wrote:
>>>
>>>
>>> On 31.10.14 15:05, Eric Auger wrote:
>>>> Minimal VFIO platform implementation supporting
>>>> - register space user mapping,
>>>> - IRQ assignment based on eventfds handled on qemu side.
>>>>
>>>> irqfd kernel acceleration comes in a subsequent patch.
>>>>
>>>> Signed-off-by: Kim Phillips <address@hidden>
>>>> Signed-off-by: Eric Auger <address@hidden>
> 
> [...]
> 
>>>> +/*
>>>> + * Mechanics to program/start irq injection on machine init done notifier:
>>>> + * this is needed since at finalize time, the device IRQ are not yet
>>>> + * bound to the platform bus IRQ. It is assumed here dynamic instantiation
>>>> + * always is used. Binding to the platform bus IRQ happens on a machine
>>>> + * init done notifier registered by the machine file. After its execution
>>>> + * we execute a new notifier that actually starts the injection. When 
>>>> using
>>>> + * irqfd, programming the injection consists in associating eventfds to
>>>> + * GSI number,ie. virtual IRQ number
>>>> + */
>>>> +
>>>> +typedef struct VfioIrqStarterNotifierParams {
>>>> +    unsigned int platform_bus_first_irq;
>>>> +    Notifier notifier;
>>>> +} VfioIrqStarterNotifierParams;
>>>> +
>>>> +typedef struct VfioIrqStartParams {
>>>> +    PlatformBusDevice *pbus;
>>>> +    int platform_bus_first_irq;
>>>> +} VfioIrqStartParams;
>>>> +
>>>> +/* Start injection of IRQ for a specific VFIO device */
>>>> +static int vfio_irq_starter(SysBusDevice *sbdev, void *opaque)
>>>> +{
>>>> +    int i;
>>>> +    VfioIrqStartParams *p = opaque;
>>>> +    VFIOPlatformDevice *vdev;
>>>> +    VFIODevice *vbasedev;
>>>> +    uint64_t irq_number;
>>>> +    PlatformBusDevice *pbus = p->pbus;
>>>> +    int platform_bus_first_irq = p->platform_bus_first_irq;
>>>> +
>>>> +    if (object_dynamic_cast(OBJECT(sbdev), TYPE_VFIO_PLATFORM)) {
>>>> +        vdev = VFIO_PLATFORM_DEVICE(sbdev);
>>>> +        vbasedev = &vdev->vbasedev;
>>>> +        for (i = 0; i < vbasedev->num_irqs; i++) {
>>>> +            irq_number = platform_bus_get_irqn(pbus, sbdev, i)
>>>> +                             + platform_bus_first_irq;
>>>> +            vfio_start_irq_injection(sbdev, i, irq_number);
>>>> +        }
>>>> +    }
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +/* loop on all VFIO platform devices and start their IRQ injection */
>>>> +static void vfio_irq_starter_notify(Notifier *notifier, void *data)
>>>> +{
>>>> +    VfioIrqStarterNotifierParams *p =
>>>> +        container_of(notifier, VfioIrqStarterNotifierParams, notifier);
>>>> +    DeviceState *dev =
>>>> +        qdev_find_recursive(sysbus_get_default(), 
>>>> TYPE_PLATFORM_BUS_DEVICE);
>>>> +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(dev);
>>>> +
>>>> +    if (pbus->done_gathering) {
>>>> +        VfioIrqStartParams data = {
>>>> +            .pbus = pbus,
>>>> +            .platform_bus_first_irq = p->platform_bus_first_irq,
>>>> +        };
>>>> +
>>>> +        foreach_dynamic_sysbus_device(vfio_irq_starter, &data);
>>>> +    }
>>>> +}
>>>> +
>>>> +/* registers the machine init done notifier that will start VFIO IRQ */
>>>> +void vfio_register_irq_starter(int platform_bus_first_irq)
>>>> +{
>>>> +    VfioIrqStarterNotifierParams *p = g_new(VfioIrqStarterNotifierParams, 
>>>> 1);
>>>> +
>>>> +    p->platform_bus_first_irq = platform_bus_first_irq;
>>>> +    p->notifier.notify = vfio_irq_starter_notify;
>>>> +    qemu_add_machine_init_done_notifier(&p->notifier);
>>>
>>> Could you add a notifier for each device instead? Then the notifier
>>> would be part of the vfio device struct and not some dangling random
>>> pointer :).
>>>
>>> Of course instead of foreach_dynamic_sysbus_device() you would directly
>>> know the device you're dealing with and only handle a single device per
>>> notifier.
>>
>> Hi Alex,
>>
>> I don't see how to practically follow your request:
>>
>> - at machine init time, VFIO devices are not yet instantiated so I
>> cannot call foreach_dynamic_sysbus_device() there - I was definitively
>> wrong in my first reply :-().
>>
>> - I can't register a per VFIO device notifier in the VFIO device
>> finalize function because this latter is called after the platform bus
>> instantiation. So the IRQ binding notifier (registered in platform bus
>> finalize fn) would be called after the IRQ starter notifier.
>>
>> - then to simplify things a bit I could use a qemu_register_reset in
>> place of a machine init done notifier (would relax the call order
>> constraint) but the problem consists in passing the platform bus first
>> irq (all the more so you requested it became part of a const struct)
>>
>> Do I miss something?
> 
> So the basic idea is that the device itself calls
> qemu_add_machine_init_done_notifier() in its realize function. The
> Notifier struct would be part of the device state which means you can
> cast yourself into the VFIO device state.

humm, the vfio device is instantiated in the cmd line so after the
machine init. This means 1st the platform bus binding notifier is
registered (in platform bus realize) and then VFIO irq starter notifiers
are registered (in VFIO realize). Notifiers beeing executed in the
reverse order of their registration, this would fail. Am I wrong?
> 
> At that point the IRQ allocation should have already happened, so your
> IRQ objects are populated. You can then ask the KVM GIC to convert that
> qemu_irq object to a GIC IRQ ID that you can then use in your ioctl I
> suppose.
> 
> 
> Alex
>