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

[Alexander Graf <agraf@suse.de>]

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.

I would also do the lookup the other way around. The GPIO / IRQ number
mapping is reasonably local to the GIC device, so I'd rather call a GIC
function to find the ID:

  kvm_gic_get_irq_gsi(s->gic_link, qdev_get_gpio_in(s, i));

> 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.

Not sure I understand what you're asking for :).


Alex