Re: [Qemu-devel] [PATCH v12 4/9] hw/vfio/platform: add capability to start IRQ propagation

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

Hi Alex,
On 04/17/2015 12:04 AM, Alex Williamson wrote:
> On Thu, 2015-03-19 at 17:16 +0000, Eric Auger wrote:
>> Add a reset notify function that enables to start the propagation of
>> interrupts to the guest.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>> v10 -> v11:
>> - comment reword
>>
>> v8 -> v9:
>> - handle failure in vfio_irq_starter
>> ---
>>  hw/vfio/platform.c              | 64 +++++++++++++++++++++++++++++++++++++++++
>>  include/hw/vfio/vfio-platform.h |  8 ++++++
>>  2 files changed, 72 insertions(+)
>>
>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>> index 31c2701..361e01b 100644
>> --- a/hw/vfio/platform.c
>> +++ b/hw/vfio/platform.c
>> @@ -572,6 +572,70 @@ static void vfio_platform_realize(DeviceState *dev, Error **errp)
>>      }
>>  }
>>  
>> +/**
>> + * vfio_irq_starter: Start IRQ forwarding for a specific VFIO device
>> + * @sbdev: Sysbus device handle
>> + * @opaque: DeviceState handle of the interrupt controller the device
>> + *          is attached to
>> + *
>> + * The function retrieves the absolute GSI number each VFIO IRQ
>> + * corresponds to and start forwarding.
>> + */
> 
> Using "forwarding" here is really making me look for your platform's EOI
> trick (IRQ Forwarding), which isn't here.
sure
> 
>> +static int vfio_irq_starter(SysBusDevice *sbdev, void *opaque)
>> +{
>> +    DeviceState *intc = (DeviceState *)opaque;
>> +    VFIOPlatformDevice *vdev;
>> +    VFIOINTp *intp;
>> +    qemu_irq irq;
>> +    int gsi, ret;
>> +
>> +    if (object_dynamic_cast(OBJECT(sbdev), TYPE_VFIO_PLATFORM)) {
>> +        vdev = VFIO_PLATFORM_DEVICE(sbdev);
>> +
>> +        QLIST_FOREACH(intp, &vdev->intp_list, next) {
>> +            gsi = 0;
>> +            while (1) {
>> +                irq = qdev_get_gpio_in(intc, gsi);
>> +                if (irq == intp->qemuirq) {
>> +                    break;
>> +                }
>> +                gsi++;
>> +            }
> 
> A for() loop would be more compact here, but is there some other exit
> condition we can add?  gsi < INT_MAX?
Currently I do not see any way to retrieve the number of input GPIOs.
This is static in core/qdev.c. either I leave as is or introduce getters.

> 
>> +            intp->virtualID = gsi;
>> +            ret = vdev->start_irq_fn(intp);
>> +            if (ret) {
>> +                error_report("%s unable to start propagation of IRQ index %d",
>> +                             vdev->vbasedev.name, intp->pin);
>> +                exit(1);
>> +            }
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +/**
>> + * vfio_kick_irqs: start IRQ forwarding for all the VFIO devices
>> + * attached to the platform bus
>> + * @data: Device state handle of the interrupt controller the VFIO IRQs
>> + *        must be bound to
>> + *
>> + * Binding to the platform bus IRQs happens on a machine init done
>> + * notifier registered by the platform bus. Only at that time the
>> + * absolute virtual IRQ = GSI is known and allows to setup IRQFD.
>> + * This function typically can be called in a reset notifier registered
>> + * by the machine file.
>> + */
> 
> So there's not actually an irqfd setup done here yet and what is
> currently done by the above starter function doesn't depend at all on
> the GSI.  Do you perhaps want to setup the vfio->eventfd signaling
> during your initfn and then only connect the eventfd->irqfd to KVM in
> the reset function?  Sort of how vfio-pci does the routing update.

Not sure I get what you mean here. In above starter I call start_irq_fn.
This latter starts the injection depending on the chosen technique, 1)
user side handling, 2) standalone irqfd, 2) irqfd + ARM forwarding.
For starting 2) and 3) I actually use the GSI set above by
intp->virtualID = gsi;
See vfio_start_irqfd_injection.

Indeed I noticed in VFIO-PCI you first started eventfd and then irqfd
but I thought I did not need to do that. What is the problem starting
VFIO signaling on reset (notifier) only? Besides, moving back to 2-step
settup would not fix my issue of being able to know the gsi to attach to
my irqfd. I need to further investigate this PCI INTx routing notifier...

Best Regards

Eric

> 
>> +void vfio_kick_irqs(void *data)
>> +{
>> +    DeviceState *intc = (DeviceState *)data;
>> +    DeviceState *dev =
>> +        qdev_find_recursive(sysbus_get_default(), TYPE_PLATFORM_BUS_DEVICE);
>> +    PlatformBusDevice *pbus = PLATFORM_BUS_DEVICE(dev);
>> +
>> +    assert(pbus->done_gathering);
>> +    foreach_dynamic_sysbus_device(vfio_irq_starter, intc);
>> +}
>> +
>>  static const VMStateDescription vfio_platform_vmstate = {
>>      .name = TYPE_VFIO_PLATFORM,
>>      .unmigratable = 1,
>> diff --git a/include/hw/vfio/vfio-platform.h b/include/hw/vfio/vfio-platform.h
>> index 31893a3..c9ee898 100644
>> --- a/include/hw/vfio/vfio-platform.h
>> +++ b/include/hw/vfio/vfio-platform.h
>> @@ -77,4 +77,12 @@ typedef struct VFIOPlatformDeviceClass {
>>  #define VFIO_PLATFORM_DEVICE_GET_CLASS(obj) \
>>       OBJECT_GET_CLASS(VFIOPlatformDeviceClass, (obj), TYPE_VFIO_PLATFORM)
>>  
>> +/**
>> + * vfio_kick_irqs - reset notifier that starts IRQ injection
>> + * for all VFIO dynamic sysbus devices attached to the platform bus.
>> + *
>> + * @opaque: handle to the interrupt controller DeviceState
>> + */
>> +void vfio_kick_irqs(void *opaque);
>> +
>>  #endif /*HW_VFIO_VFIO_PLATFORM_H*/
> 
> 
>