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

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

On 11/27/2014 06:24 PM, Alexander Graf wrote:
> 
> 
> On 27.11.14 18:13, Eric Auger wrote:
>> On 11/27/2014 04:55 PM, Alexander Graf wrote:
>>>
>>>
>>> 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.
>> In that case the machine (init done) callback would be used to pass the
>> vgic handle to each vfio device. Registered by the machine file, isn't
>> it. Aren't we exactly at the same state you wanted to improve initially
>> where the notifier is registered by the machine file, not belonging to
>> the VFIO device, just replacing first_irq param by vgic_handle which
>> eventually ends up as a link.
>>
>> This notifier still cannot be registered by the VFIO device finalize fn
>> since the VFIO device has no handle to the interrupt controller. kind of
>> chicken & egg problem.
>>>>
>>>> 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
>>
>> looks OK indeed
>>>
>>> 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 :).
>>
>>
>> and why not "simply" a qemu_register_reset passing the vgic handle as
>> opaque.
> 
> Who would register this reset callback? It'd have to be someone who
> knows both the VFIO device as well as the vGIC device.
the machine file would. reset callback implemented in vfio-platform.c,
looping on all instances. ~ as today for the notifier but without the
dangling pointer. not sure you will like it though ;-)
> 
> The reset idea could work as replacement for the notifier though. So you
> could have the VFIO device register a reset callback in which it asks
> the vgic for the number and registers the IRQ with KVM.
arghh, still the problem of passing the vgic handle. I used the reset cb
registration by the machine file to do that. Of course if we use your
machine property trick we can do the registration by the VFIO driver
itself.

Eric
> 
> 
> Alex
>