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

[Alexander Graf <agraf@suse.de>]

On 27.11.14 16:28, Alexander Graf wrote:
> 
> 
> On 27.11.14 16:14, Eric Auger wrote:
>> 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 ;-)
> 
> That should be easy - make it a link property. In fact, this would be
> one of those cases where not generalizing the code would've been a good
> idea.
> 
> If device creation would live in the machine file, the machine could
> automatically set the link. Maybe you can still get there somehow? You
> could add a machine callback in the device allocation function.

If this gets too messy, I think doing a machine attribute would work as
well here. Check out the way we pass the e500-ccsr object on e500:


http://git.qemu.org/?p=qemu.git;a=blob;f=hw/pci-host/ppce500.c;h=1b4c0f00236e8005c261da527d416fe6a053b353;hb=HEAD#l337


http://git.qemu.org/?p=qemu.git;a=blob;f=hw/ppc/e500.c;h=2832fc0da444d89737768f7c4dcb0638e2625750;hb=HEAD#l873

I think doing an actual link would be cleaner, but at least the above
gets you to an acceptable state that can still be improved with links
later - the basic idea is the same :).


Alex