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

[Eric Auger <eric.auger@linaro.org>]

On 11/27/2014 03:35 PM, Alexander Graf wrote:
> 
> 
> On 27.11.14 15:05, Eric Auger wrote:
>> On 11/26/2014 03:46 PM, Eric Auger wrote:
>>> 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 <kim.phillips@linaro.org>
>>>>>>>>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>>> +/*
>>>>>>>>> + * 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)
>>
>> Hi Alex,
>>
>> I believe I understand your proposal but the issue is to pass the
>> platform bus first_irq parameter which is needed to compute the absolute
>> IRQ number (=irqfd GSI). VFIO device does not have this info. Platform
>> bus doesn't have it either. Only machine file has the info.
> 
> Well, the GIC should have this info as well. That's why I was trying to
> point out that you want to ask the GIC about the absolute IRQ number on
> its own number space.
> 
> You need to make the connection with the GIC anyway, no? So you need to
> somehow get awareness of the GIC device. Or are you hijacking the global
> GSI number space?

Hi Alex,

Well OK I believe I understand your idea: in vfio device, loop on all
gic gpios using   qdev_get_gpio_in(gicdev, i) and identify i that
matches the qemu_irq I want to kick off. That would be feasible if VFIO
has a handle to the GIC DeviceState (gicdev), which is not curently the
case. so me move the problem to passing the gicdev to vfio ;-)

VFIO being mostly generic we could only do that in the derived VFIO
device (the famous calxeda xgmac device) or some intermediate vfio arm
device - let's be crazy!? ;-) - . GIC derives from std sysbus device (no
kind of generic interrupt controller device I could recognize) when
parsing the qom tree stuff so I don't see any other solution to retrieve
the intc handle after machine creation.

I can try that. In that case do you agree with adding/removing sysbus
binding_done notifiers in platform bus and drop callback in platform bus
class. I would call all registered notifiers at the end of
platform_bus_init_notify.

Thanks

Best Regards

Eric

> 
>>
>> The "irq now populated" notifier function callback would be called in
>> platform bus platform_bus_init_notify or link_sysbus_device I guess,
>> already executed in a machine-init-done notifier. The callback would
>> need to be called with sbdev and first_irq param to fulfill its task
>> (check of VFIO type, IRQFD setup). So I need to pass first_irq to
>> platform_bus. Do you agree? Can I add an API?
>>
>> Besides there would be a single callback per platform bus. Wouldn't it
>> be worth to add an infrastructure to add/remove misc "binding_done"
>> notifiers and call all registered functions in link_sysbus_device?
> 
> Usually the "realize" function is good enough for 99% of the devices out
> there. We're just special because we do lazy binding of IRQs on the
> platform bus :).
> 
> 
> 
> Alex
>