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

[Eric Auger]

On 11/26/2014 12:20 PM, Alexander Graf wrote:
> 
> 
> 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.
Hi Alex,

ok I work on 2)

Thanks for your guidance

Eric
> 
> 
> Alex
>