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

[Alexander Graf]

On 26.11.14 11:48, Eric Auger wrote:
> 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?

Bleks. Ok, I see 2 ways out of this:

  1) Create a TailNotifier and convert the machine_init_done notifiers
to this

  2) Add an "irq now populated" notifier function callback in a new
PlatformBusDeviceClass struct that you use to describe the
PlatformBusDevice class. Call all children's notifiers from the
machine_init notifier in the platform bus.

The more I think about it, the more I prefer option 2 I think.


Alex