[Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

This series introduces a new callback function in IRQState, named
get_gsi_cb. It is supposed to be populated by the interrupt controller
and its role is to convert the interrupt controller pin number into
the global system interrupt (gsi) number. The gsi is used when setting
irqfd up.

With PCI there is a PCIINTxRoute bus lookup mechanism that enables to
retrieve the gsi from the PCI host controller/bridge pin. The conversion
is implemented by the PCI host controller. With platform devices, this
conversion function is implemented by the interrupt controller.

Besides the callback member, a setter is introduced. First user is
arm_gic_kvm. A public function wraps the callback, qemu_irq_get_gsi.

The first user of qemu_irq_get_gsi might be the VFIO platform device.
This will come in the "KVM platform device passthrough" series.

Eric Auger (2):
  irq: add get_gsi callback
  intc: arm_gic_kvm: set the get_gsi callback

 hw/core/irq.c         | 20 ++++++++++++++++++++
 hw/intc/arm_gic_kvm.c | 10 ++++++++++
 include/hw/irq.h      |  8 ++++++++
 3 files changed, 38 insertions(+)

-- 
1.8.3.2

[PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

This series introduces a new callback function in IRQState, named
get_gsi_cb. It is supposed to be populated by the interrupt controller
and its role is to convert the interrupt controller pin number into
the global system interrupt (gsi) number. The gsi is used when setting
irqfd up.

With PCI there is a PCIINTxRoute bus lookup mechanism that enables to
retrieve the gsi from the PCI host controller/bridge pin. The conversion
is implemented by the PCI host controller. With platform devices, this
conversion function is implemented by the interrupt controller.

Besides the callback member, a setter is introduced. First user is
arm_gic_kvm. A public function wraps the callback, qemu_irq_get_gsi.

The first user of qemu_irq_get_gsi might be the VFIO platform device.
This will come in the "KVM platform device passthrough" series.

Eric Auger (2):
  irq: add get_gsi callback
  intc: arm_gic_kvm: set the get_gsi callback

 hw/core/irq.c         | 20 ++++++++++++++++++++
 hw/intc/arm_gic_kvm.c | 10 ++++++++++
 include/hw/irq.h      |  8 ++++++++
 3 files changed, 38 insertions(+)

-- 
1.8.3.2

[Qemu-devel] [PATCH 1/2] irq: add get_gsi callback

[Eric Auger]

Add a new IRQState get_gsi_cb field, corresponding to a callback
that transforms the interrupt controller pin number into a global
system interrupt number.

A setter is introduced for the interrupt controller to populate the
callback. Also add a public function that calls this callback.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/core/irq.c    | 20 ++++++++++++++++++++
 include/hw/irq.h |  8 ++++++++
 2 files changed, 28 insertions(+)

diff --git a/hw/core/irq.c b/hw/core/irq.c
index 8a62a36..7eeb742 100644
--- a/hw/core/irq.c
+++ b/hw/core/irq.c
@@ -31,6 +31,7 @@ struct IRQState {
     Object parent_obj;
 
     qemu_irq_handler handler;
+    qemu_irq_get_gsi_t get_gsi_cb;
     void *opaque;
     int n;
 };
@@ -144,6 +145,25 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n)
     }
 }
 
+int qemu_irq_set_get_gsi_cb(qemu_irq irq, qemu_irq_get_gsi_t cb)
+{
+    if (!irq) {
+        return -ENXIO;
+    }
+
+    irq->get_gsi_cb = cb;
+    return 0;
+}
+
+int qemu_irq_get_gsi(qemu_irq irq)
+{
+    if (!irq) {
+        return -ENXIO;
+    }
+
+    return irq->get_gsi_cb(irq->n);
+}
+
 static const TypeInfo irq_type_info = {
    .name = TYPE_IRQ,
    .parent = TYPE_OBJECT,
diff --git a/include/hw/irq.h b/include/hw/irq.h
index 4c4c2ea..658b8f2 100644
--- a/include/hw/irq.h
+++ b/include/hw/irq.h
@@ -8,6 +8,7 @@
 typedef struct IRQState *qemu_irq;
 
 typedef void (*qemu_irq_handler)(void *opaque, int n, int level);
+typedef int (*qemu_irq_get_gsi_t)(int pin);
 
 void qemu_set_irq(qemu_irq irq, int level);
 
@@ -62,4 +63,11 @@ qemu_irq *qemu_irq_proxy(qemu_irq **target, int n);
    on an existing vector of qemu_irq.  */
 void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n);
 
+/* sets the get_gsi callback (function that converts the interrupt
+   controller pin into a global system interrupt number */
+int qemu_irq_set_get_gsi_cb(qemu_irq irq,  qemu_irq_get_gsi_t cb);
+
+/* returns the gsi associated to a qemu_irq handle */
+int qemu_irq_get_gsi(qemu_irq irq);
+
 #endif
-- 
1.8.3.2

[PATCH 1/2] irq: add get_gsi callback

[Eric Auger]

Add a new IRQState get_gsi_cb field, corresponding to a callback
that transforms the interrupt controller pin number into a global
system interrupt number.

A setter is introduced for the interrupt controller to populate the
callback. Also add a public function that calls this callback.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/core/irq.c    | 20 ++++++++++++++++++++
 include/hw/irq.h |  8 ++++++++
 2 files changed, 28 insertions(+)

diff --git a/hw/core/irq.c b/hw/core/irq.c
index 8a62a36..7eeb742 100644
--- a/hw/core/irq.c
+++ b/hw/core/irq.c
@@ -31,6 +31,7 @@ struct IRQState {
     Object parent_obj;
 
     qemu_irq_handler handler;
+    qemu_irq_get_gsi_t get_gsi_cb;
     void *opaque;
     int n;
 };
@@ -144,6 +145,25 @@ void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n)
     }
 }
 
+int qemu_irq_set_get_gsi_cb(qemu_irq irq, qemu_irq_get_gsi_t cb)
+{
+    if (!irq) {
+        return -ENXIO;
+    }
+
+    irq->get_gsi_cb = cb;
+    return 0;
+}
+
+int qemu_irq_get_gsi(qemu_irq irq)
+{
+    if (!irq) {
+        return -ENXIO;
+    }
+
+    return irq->get_gsi_cb(irq->n);
+}
+
 static const TypeInfo irq_type_info = {
    .name = TYPE_IRQ,
    .parent = TYPE_OBJECT,
diff --git a/include/hw/irq.h b/include/hw/irq.h
index 4c4c2ea..658b8f2 100644
--- a/include/hw/irq.h
+++ b/include/hw/irq.h
@@ -8,6 +8,7 @@
 typedef struct IRQState *qemu_irq;
 
 typedef void (*qemu_irq_handler)(void *opaque, int n, int level);
+typedef int (*qemu_irq_get_gsi_t)(int pin);
 
 void qemu_set_irq(qemu_irq irq, int level);
 
@@ -62,4 +63,11 @@ qemu_irq *qemu_irq_proxy(qemu_irq **target, int n);
    on an existing vector of qemu_irq.  */
 void qemu_irq_intercept_in(qemu_irq *gpio_in, qemu_irq_handler handler, int n);
 
+/* sets the get_gsi callback (function that converts the interrupt
+   controller pin into a global system interrupt number */
+int qemu_irq_set_get_gsi_cb(qemu_irq irq,  qemu_irq_get_gsi_t cb);
+
+/* returns the gsi associated to a qemu_irq handle */
+int qemu_irq_get_gsi(qemu_irq irq);
+
 #endif
-- 
1.8.3.2

[Qemu-devel] [PATCH 2/2] intc: arm_gic_kvm: set the get_gsi callback

[Eric Auger]

The arm_gic_kvm now sets the get_gsi_cb callback so that the
global system interrupt of a qemu_irq can be retrieved. This enables
VFIO platform signaling to be setup.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/intc/arm_gic_kvm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index e1952ad..8a97a5b 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -87,6 +87,11 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
     kvm_set_irq(kvm_state, kvm_irq, !!level);
 }
 
+static int kvm_gic_get_gsi(int pin)
+{
+    return pin;
+}
+
 static bool kvm_arm_gic_can_save_restore(GICState *s)
 {
     return s->dev_fd >= 0;
@@ -554,6 +559,11 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
      */
     i += (GIC_INTERNAL * s->num_cpu);
     qdev_init_gpio_in(dev, kvm_arm_gic_set_irq, i);
+
+    for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
+        qemu_irq irq = qdev_get_gpio_in(dev, i);
+        qemu_irq_set_get_gsi_cb(irq, kvm_gic_get_gsi);
+    }
     /* We never use our outbound IRQ lines but provide them so that
      * we maintain the same interface as the non-KVM GIC.
      */
-- 
1.8.3.2

[PATCH 2/2] intc: arm_gic_kvm: set the get_gsi callback

[Eric Auger]

The arm_gic_kvm now sets the get_gsi_cb callback so that the
global system interrupt of a qemu_irq can be retrieved. This enables
VFIO platform signaling to be setup.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/intc/arm_gic_kvm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/hw/intc/arm_gic_kvm.c b/hw/intc/arm_gic_kvm.c
index e1952ad..8a97a5b 100644
--- a/hw/intc/arm_gic_kvm.c
+++ b/hw/intc/arm_gic_kvm.c
@@ -87,6 +87,11 @@ static void kvm_arm_gic_set_irq(void *opaque, int irq, int level)
     kvm_set_irq(kvm_state, kvm_irq, !!level);
 }
 
+static int kvm_gic_get_gsi(int pin)
+{
+    return pin;
+}
+
 static bool kvm_arm_gic_can_save_restore(GICState *s)
 {
     return s->dev_fd >= 0;
@@ -554,6 +559,11 @@ static void kvm_arm_gic_realize(DeviceState *dev, Error **errp)
      */
     i += (GIC_INTERNAL * s->num_cpu);
     qdev_init_gpio_in(dev, kvm_arm_gic_set_irq, i);
+
+    for (i = 0; i < s->num_irq - GIC_INTERNAL; i++) {
+        qemu_irq irq = qdev_get_gpio_in(dev, i);
+        qemu_irq_set_get_gsi_cb(irq, kvm_gic_get_gsi);
+    }
     /* We never use our outbound IRQ lines but provide them so that
      * we maintain the same interface as the non-KVM GIC.
      */
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 23/04/2015 10:49, Eric Auger wrote:
> This series introduces a new callback function in IRQState, named
> get_gsi_cb. It is supposed to be populated by the interrupt controller
> and its role is to convert the interrupt controller pin number into
> the global system interrupt (gsi) number. The gsi is used when setting
> irqfd up.
> 
> With PCI there is a PCIINTxRoute bus lookup mechanism that enables to
> retrieve the gsi from the PCI host controller/bridge pin. The conversion
> is implemented by the PCI host controller. With platform devices, this
> conversion function is implemented by the interrupt controller.
> 
> Besides the callback member, a setter is introduced. First user is
> arm_gic_kvm. A public function wraps the callback, qemu_irq_get_gsi.
> 
> The first user of qemu_irq_get_gsi might be the VFIO platform device.
> This will come in the "KVM platform device passthrough" series.

I'm sorry, I cannot understand the point of this without seeing a user.

Why can't you just use a GHashTable?  This mustn't be a hot path, since
VFIO does all the signalling in the kernel via irqfds.

Paolo

Re: [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 23/04/2015 10:49, Eric Auger wrote:
> This series introduces a new callback function in IRQState, named
> get_gsi_cb. It is supposed to be populated by the interrupt controller
> and its role is to convert the interrupt controller pin number into
> the global system interrupt (gsi) number. The gsi is used when setting
> irqfd up.
> 
> With PCI there is a PCIINTxRoute bus lookup mechanism that enables to
> retrieve the gsi from the PCI host controller/bridge pin. The conversion
> is implemented by the PCI host controller. With platform devices, this
> conversion function is implemented by the interrupt controller.
> 
> Besides the callback member, a setter is introduced. First user is
> arm_gic_kvm. A public function wraps the callback, qemu_irq_get_gsi.
> 
> The first user of qemu_irq_get_gsi might be the VFIO platform device.
> This will come in the "KVM platform device passthrough" series.

I'm sorry, I cannot understand the point of this without seeing a user.

Why can't you just use a GHashTable?  This mustn't be a hot path, since
VFIO does all the signalling in the kernel via irqfds.

Paolo

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

Hi Paolo,
On 04/23/2015 11:30 AM, Paolo Bonzini wrote:
> 
> 
> On 23/04/2015 10:49, Eric Auger wrote:
>> This series introduces a new callback function in IRQState, named
>> get_gsi_cb. It is supposed to be populated by the interrupt controller
>> and its role is to convert the interrupt controller pin number into
>> the global system interrupt (gsi) number. The gsi is used when setting
>> irqfd up.
>>
>> With PCI there is a PCIINTxRoute bus lookup mechanism that enables to
>> retrieve the gsi from the PCI host controller/bridge pin. The conversion
>> is implemented by the PCI host controller. With platform devices, this
>> conversion function is implemented by the interrupt controller.
>>
>> Besides the callback member, a setter is introduced. First user is
>> arm_gic_kvm. A public function wraps the callback, qemu_irq_get_gsi.
>>
>> The first user of qemu_irq_get_gsi might be the VFIO platform device.
>> This will come in the "KVM platform device passthrough" series.
> 
> I'm sorry, I cannot understand the point of this without seeing a user.
no problem, I intend to send the KVM platform device passthrough v13
today which will illustrate the usage.
> 
> Why can't you just use a GHashTable?

You mean implementing this hash table in the interrupt controller?

The problem is my VFIO device currently has no link to the interrupt
controller object. Besides it has a platform bus in between. Previously
I devised a reset notifier called in the machine file to pass this link
but Alex was not keen about this method. So the idea here is when the
VFIO sysbus device qemuirq is getting connected, a notifier is called,
VFIO device can retrieve the gsi of this qemu_irq and call KVM_IRQFD
with that gsi.

Best Regards

Eric
  This mustn't be a hot path, since
> VFIO does all the signalling in the kernel via irqfds.
> 
> Paolo
> 

Re: [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

Hi Paolo,
On 04/23/2015 11:30 AM, Paolo Bonzini wrote:
> 
> 
> On 23/04/2015 10:49, Eric Auger wrote:
>> This series introduces a new callback function in IRQState, named
>> get_gsi_cb. It is supposed to be populated by the interrupt controller
>> and its role is to convert the interrupt controller pin number into
>> the global system interrupt (gsi) number. The gsi is used when setting
>> irqfd up.
>>
>> With PCI there is a PCIINTxRoute bus lookup mechanism that enables to
>> retrieve the gsi from the PCI host controller/bridge pin. The conversion
>> is implemented by the PCI host controller. With platform devices, this
>> conversion function is implemented by the interrupt controller.
>>
>> Besides the callback member, a setter is introduced. First user is
>> arm_gic_kvm. A public function wraps the callback, qemu_irq_get_gsi.
>>
>> The first user of qemu_irq_get_gsi might be the VFIO platform device.
>> This will come in the "KVM platform device passthrough" series.
> 
> I'm sorry, I cannot understand the point of this without seeing a user.
no problem, I intend to send the KVM platform device passthrough v13
today which will illustrate the usage.
> 
> Why can't you just use a GHashTable?

You mean implementing this hash table in the interrupt controller?

The problem is my VFIO device currently has no link to the interrupt
controller object. Besides it has a platform bus in between. Previously
I devised a reset notifier called in the machine file to pass this link
but Alex was not keen about this method. So the idea here is when the
VFIO sysbus device qemuirq is getting connected, a notifier is called,
VFIO device can retrieve the gsi of this qemu_irq and call KVM_IRQFD
with that gsi.

Best Regards

Eric
  This mustn't be a hot path, since
> VFIO does all the signalling in the kernel via irqfds.
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 23/04/2015 11:40, Eric Auger wrote:
>> > Why can't you just use a GHashTable?
> You mean implementing this hash table in the interrupt controller?

No, in KVM.  Basically the kvm-vgic interrupt controller registers its 
qemu_irqs with kvm-all.c, passing the gsi number for KVM_IRQFD along 
the way.  Then VFIO device can pass the qemu_irq and ask for an irqfd 
for that qemu_irq, instead of having to know the gsi number.  kvm-all.c
acts as a mediator.

The notifier actually is not even necessary, because we already have
a very similar concept in QOM.  But qdev_init_gpio_out_named always
passes a dummy "notifier":

void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
                              const char *name, int n)
{
    int i;
    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");

    assert(gpio_list->num_in == 0 || !name);
    gpio_list->num_out += n;

    for (i = 0; i < n; ++i) {
        memset(&pins[i], 0, sizeof(*pins));
        object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
                                 (Object **)&pins[i],
                                 object_property_allow_set_link,
                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
                                 &error_abort);
    } 
    g_free(propname);
}

So you need to add a method to DeviceClass, with the same prototype
as object_property_allow_set_link, and set the default value to
object_property_allow_set_link in device_class_init.  Then
qdev_init_gpio_out_named can do

    DeviceClass *dc = DEVICE_GET_CLASS(dev);
    ...
        object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
                                 (Object **)&pins[i],
                                 dc->gpio_set_hook,
                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
                                 &error_abort);

or something like that.

Alternatively, and probably better, add the argument to
qdev_init_gpio_out_named; there aren't many calls.  Then
you can add the method only to SysbusDeviceClass rather than
to all devices, and you can change sysbus_init_irq like this:

void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
{
    SysBusDeviceClass *sdc = SYSBUS_DEVICE_GET_CLASS(dev);
    qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1,
                             sdc->irq_set_hook);
}


Paolo

> The problem is my VFIO device currently has no link to the interrupt
> controller object. Besides it has a platform bus in between. Previously
> I devised a reset notifier called in the machine file to pass this link
> but Alex was not keen about this method. So the idea here is when the
> VFIO sysbus device qemuirq is getting connected, a notifier is called,
> VFIO device can retrieve the gsi of this qemu_irq and call KVM_IRQFD
> with that gsi.

Re: [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 23/04/2015 11:40, Eric Auger wrote:
>> > Why can't you just use a GHashTable?
> You mean implementing this hash table in the interrupt controller?

No, in KVM.  Basically the kvm-vgic interrupt controller registers its 
qemu_irqs with kvm-all.c, passing the gsi number for KVM_IRQFD along 
the way.  Then VFIO device can pass the qemu_irq and ask for an irqfd 
for that qemu_irq, instead of having to know the gsi number.  kvm-all.c
acts as a mediator.

The notifier actually is not even necessary, because we already have
a very similar concept in QOM.  But qdev_init_gpio_out_named always
passes a dummy "notifier":

void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
                              const char *name, int n)
{
    int i;
    NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
    char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");

    assert(gpio_list->num_in == 0 || !name);
    gpio_list->num_out += n;

    for (i = 0; i < n; ++i) {
        memset(&pins[i], 0, sizeof(*pins));
        object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
                                 (Object **)&pins[i],
                                 object_property_allow_set_link,
                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
                                 &error_abort);
    } 
    g_free(propname);
}

So you need to add a method to DeviceClass, with the same prototype
as object_property_allow_set_link, and set the default value to
object_property_allow_set_link in device_class_init.  Then
qdev_init_gpio_out_named can do

    DeviceClass *dc = DEVICE_GET_CLASS(dev);
    ...
        object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
                                 (Object **)&pins[i],
                                 dc->gpio_set_hook,
                                 OBJ_PROP_LINK_UNREF_ON_RELEASE,
                                 &error_abort);

or something like that.

Alternatively, and probably better, add the argument to
qdev_init_gpio_out_named; there aren't many calls.  Then
you can add the method only to SysbusDeviceClass rather than
to all devices, and you can change sysbus_init_irq like this:

void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
{
    SysBusDeviceClass *sdc = SYSBUS_DEVICE_GET_CLASS(dev);
    qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1,
                             sdc->irq_set_hook);
}


Paolo

> The problem is my VFIO device currently has no link to the interrupt
> controller object. Besides it has a platform bus in between. Previously
> I devised a reset notifier called in the machine file to pass this link
> but Alex was not keen about this method. So the idea here is when the
> VFIO sysbus device qemuirq is getting connected, a notifier is called,
> VFIO device can retrieve the gsi of this qemu_irq and call KVM_IRQFD
> with that gsi.

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

Hi Paolo,

thanks for taking the time to explain. I will rewrite accordingly.

Best Regards

Eric

On 04/23/2015 11:58 AM, Paolo Bonzini wrote:
> 
> 
> On 23/04/2015 11:40, Eric Auger wrote:
>>>> Why can't you just use a GHashTable?
>> You mean implementing this hash table in the interrupt controller?
> 
> No, in KVM.  Basically the kvm-vgic interrupt controller registers its 
> qemu_irqs with kvm-all.c, passing the gsi number for KVM_IRQFD along 
> the way.  Then VFIO device can pass the qemu_irq and ask for an irqfd 
> for that qemu_irq, instead of having to know the gsi number.  kvm-all.c
> acts as a mediator.
> 
> The notifier actually is not even necessary, because we already have
> a very similar concept in QOM.  But qdev_init_gpio_out_named always
> passes a dummy "notifier":
> 
> void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>                               const char *name, int n)
> {
>     int i;
>     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>     char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");
> 
>     assert(gpio_list->num_in == 0 || !name);
>     gpio_list->num_out += n;
> 
>     for (i = 0; i < n; ++i) {
>         memset(&pins[i], 0, sizeof(*pins));
>         object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
>                                  (Object **)&pins[i],
>                                  object_property_allow_set_link,
>                                  OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                                  &error_abort);
>     } 
>     g_free(propname);
> }
> 
> So you need to add a method to DeviceClass, with the same prototype
> as object_property_allow_set_link, and set the default value to
> object_property_allow_set_link in device_class_init.  Then
> qdev_init_gpio_out_named can do
> 
>     DeviceClass *dc = DEVICE_GET_CLASS(dev);
>     ...
>         object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
>                                  (Object **)&pins[i],
>                                  dc->gpio_set_hook,
>                                  OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                                  &error_abort);
> 
> or something like that.
> 
> Alternatively, and probably better, add the argument to
> qdev_init_gpio_out_named; there aren't many calls.  Then
> you can add the method only to SysbusDeviceClass rather than
> to all devices, and you can change sysbus_init_irq like this:
> 
> void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
> {
>     SysBusDeviceClass *sdc = SYSBUS_DEVICE_GET_CLASS(dev);
>     qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1,
>                              sdc->irq_set_hook);
> }
> 
> 
> Paolo
> 
>> The problem is my VFIO device currently has no link to the interrupt
>> controller object. Besides it has a platform bus in between. Previously
>> I devised a reset notifier called in the machine file to pass this link
>> but Alex was not keen about this method. So the idea here is when the
>> VFIO sysbus device qemuirq is getting connected, a notifier is called,
>> VFIO device can retrieve the gsi of this qemu_irq and call KVM_IRQFD
>> with that gsi.

Re: [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

Hi Paolo,

thanks for taking the time to explain. I will rewrite accordingly.

Best Regards

Eric

On 04/23/2015 11:58 AM, Paolo Bonzini wrote:
> 
> 
> On 23/04/2015 11:40, Eric Auger wrote:
>>>> Why can't you just use a GHashTable?
>> You mean implementing this hash table in the interrupt controller?
> 
> No, in KVM.  Basically the kvm-vgic interrupt controller registers its 
> qemu_irqs with kvm-all.c, passing the gsi number for KVM_IRQFD along 
> the way.  Then VFIO device can pass the qemu_irq and ask for an irqfd 
> for that qemu_irq, instead of having to know the gsi number.  kvm-all.c
> acts as a mediator.
> 
> The notifier actually is not even necessary, because we already have
> a very similar concept in QOM.  But qdev_init_gpio_out_named always
> passes a dummy "notifier":
> 
> void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>                               const char *name, int n)
> {
>     int i;
>     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>     char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");
> 
>     assert(gpio_list->num_in == 0 || !name);
>     gpio_list->num_out += n;
> 
>     for (i = 0; i < n; ++i) {
>         memset(&pins[i], 0, sizeof(*pins));
>         object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
>                                  (Object **)&pins[i],
>                                  object_property_allow_set_link,
>                                  OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                                  &error_abort);
>     } 
>     g_free(propname);
> }
> 
> So you need to add a method to DeviceClass, with the same prototype
> as object_property_allow_set_link, and set the default value to
> object_property_allow_set_link in device_class_init.  Then
> qdev_init_gpio_out_named can do
> 
>     DeviceClass *dc = DEVICE_GET_CLASS(dev);
>     ...
>         object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
>                                  (Object **)&pins[i],
>                                  dc->gpio_set_hook,
>                                  OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                                  &error_abort);
> 
> or something like that.
> 
> Alternatively, and probably better, add the argument to
> qdev_init_gpio_out_named; there aren't many calls.  Then
> you can add the method only to SysbusDeviceClass rather than
> to all devices, and you can change sysbus_init_irq like this:
> 
> void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
> {
>     SysBusDeviceClass *sdc = SYSBUS_DEVICE_GET_CLASS(dev);
>     qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1,
>                              sdc->irq_set_hook);
> }
> 
> 
> Paolo
> 
>> The problem is my VFIO device currently has no link to the interrupt
>> controller object. Besides it has a platform bus in between. Previously
>> I devised a reset notifier called in the machine file to pass this link
>> but Alex was not keen about this method. So the idea here is when the
>> VFIO sysbus device qemuirq is getting connected, a notifier is called,
>> VFIO device can retrieve the gsi of this qemu_irq and call KVM_IRQFD
>> with that gsi.

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

On 04/23/2015 11:58 AM, Paolo Bonzini wrote:
> 
> 
> On 23/04/2015 11:40, Eric Auger wrote:
>>>> Why can't you just use a GHashTable?
>> You mean implementing this hash table in the interrupt controller?
> 
> No, in KVM.  Basically the kvm-vgic interrupt controller registers its 
> qemu_irqs with kvm-all.c, passing the gsi number for KVM_IRQFD along 
> the way.  Then VFIO device can pass the qemu_irq and ask for an irqfd 
> for that qemu_irq, instead of having to know the gsi number.  kvm-all.c
> acts as a mediator.
> 
> The notifier actually is not even necessary, because we already have
> a very similar concept in QOM.  But qdev_init_gpio_out_named always
> passes a dummy "notifier":
> 
> void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>                               const char *name, int n)
> {
>     int i;
>     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>     char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");
> 
>     assert(gpio_list->num_in == 0 || !name);
>     gpio_list->num_out += n;
> 
>     for (i = 0; i < n; ++i) {
>         memset(&pins[i], 0, sizeof(*pins));
>         object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
>                                  (Object **)&pins[i],
>                                  object_property_allow_set_link,
>                                  OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                                  &error_abort);
>     } 
>     g_free(propname);
> }
> 
> So you need to add a method to DeviceClass, with the same prototype
> as object_property_allow_set_link, and set the default value to
> object_property_allow_set_link in device_class_init.  Then
> qdev_init_gpio_out_named can do
> 
>     DeviceClass *dc = DEVICE_GET_CLASS(dev);
>     ...
>         object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
>                                  (Object **)&pins[i],
>                                  dc->gpio_set_hook,
>                                  OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                                  &error_abort);
> 
> or something like that.
> 
> Alternatively, and probably better, add the argument to
> qdev_init_gpio_out_named; there aren't many calls.  Then
> you can add the method only to SysbusDeviceClass rather than
> to all devices, and you can change sysbus_init_irq like this:
> 
> void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
> {
>     SysBusDeviceClass *sdc = SYSBUS_DEVICE_GET_CLASS(dev);
>     qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1,
>                              sdc->irq_set_hook);
> }

Hi Paolo

I implemented this alternative but my concern is the check method is
called before the qemu_irq setting. So on this callback I cannot
retrieve the qemu_irq VFIOINTp struct container object needed to setup
irqfd hence does not work for me. I would need a post_check cb. Do you
think it it sensible to add another cb?

Thanks

Eric
> 
> 
> Paolo
> 
>> The problem is my VFIO device currently has no link to the interrupt
>> controller object. Besides it has a platform bus in between. Previously
>> I devised a reset notifier called in the machine file to pass this link
>> but Alex was not keen about this method. So the idea here is when the
>> VFIO sysbus device qemuirq is getting connected, a notifier is called,
>> VFIO device can retrieve the gsi of this qemu_irq and call KVM_IRQFD
>> with that gsi.

Re: [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

On 04/23/2015 11:58 AM, Paolo Bonzini wrote:
> 
> 
> On 23/04/2015 11:40, Eric Auger wrote:
>>>> Why can't you just use a GHashTable?
>> You mean implementing this hash table in the interrupt controller?
> 
> No, in KVM.  Basically the kvm-vgic interrupt controller registers its 
> qemu_irqs with kvm-all.c, passing the gsi number for KVM_IRQFD along 
> the way.  Then VFIO device can pass the qemu_irq and ask for an irqfd 
> for that qemu_irq, instead of having to know the gsi number.  kvm-all.c
> acts as a mediator.
> 
> The notifier actually is not even necessary, because we already have
> a very similar concept in QOM.  But qdev_init_gpio_out_named always
> passes a dummy "notifier":
> 
> void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
>                               const char *name, int n)
> {
>     int i;
>     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
>     char *propname = g_strdup_printf("%s[*]", name ? name : "unnamed-gpio-out");
> 
>     assert(gpio_list->num_in == 0 || !name);
>     gpio_list->num_out += n;
> 
>     for (i = 0; i < n; ++i) {
>         memset(&pins[i], 0, sizeof(*pins));
>         object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
>                                  (Object **)&pins[i],
>                                  object_property_allow_set_link,
>                                  OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                                  &error_abort);
>     } 
>     g_free(propname);
> }
> 
> So you need to add a method to DeviceClass, with the same prototype
> as object_property_allow_set_link, and set the default value to
> object_property_allow_set_link in device_class_init.  Then
> qdev_init_gpio_out_named can do
> 
>     DeviceClass *dc = DEVICE_GET_CLASS(dev);
>     ...
>         object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
>                                  (Object **)&pins[i],
>                                  dc->gpio_set_hook,
>                                  OBJ_PROP_LINK_UNREF_ON_RELEASE,
>                                  &error_abort);
> 
> or something like that.
> 
> Alternatively, and probably better, add the argument to
> qdev_init_gpio_out_named; there aren't many calls.  Then
> you can add the method only to SysbusDeviceClass rather than
> to all devices, and you can change sysbus_init_irq like this:
> 
> void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
> {
>     SysBusDeviceClass *sdc = SYSBUS_DEVICE_GET_CLASS(dev);
>     qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1,
>                              sdc->irq_set_hook);
> }

Hi Paolo

I implemented this alternative but my concern is the check method is
called before the qemu_irq setting. So on this callback I cannot
retrieve the qemu_irq VFIOINTp struct container object needed to setup
irqfd hence does not work for me. I would need a post_check cb. Do you
think it it sensible to add another cb?

Thanks

Eric
> 
> 
> Paolo
> 
>> The problem is my VFIO device currently has no link to the interrupt
>> controller object. Besides it has a platform bus in between. Previously
>> I devised a reset notifier called in the machine file to pass this link
>> but Alex was not keen about this method. So the idea here is when the
>> VFIO sysbus device qemuirq is getting connected, a notifier is called,
>> VFIO device can retrieve the gsi of this qemu_irq and call KVM_IRQFD
>> with that gsi.

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 24/04/2015 11:01, Eric Auger wrote:
>> > void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
>> > {
>> >     SysBusDeviceClass *sdc = SYSBUS_DEVICE_GET_CLASS(dev);
>> >     qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1,
>> >                              sdc->irq_set_hook);
>> > }
> Hi Paolo
> 
> I implemented this alternative but my concern is the check method is
> called before the qemu_irq setting. So on this callback I cannot
> retrieve the qemu_irq VFIOINTp struct container object needed to setup
> irqfd hence does not work for me.

Isn't the qemu_irq passed as the third argument to the callback?  I
thought this solution was fine because you weren't passing the "int n"
from sysbus_connect_irq to your notifier.

If you really cannot make it work, I guess your "sysbus: add
irq_routing_notifier" patch would be okay.  I would only ask you to move
the function pointer from SysBusDevice to SysBusDeviceClass.

Thanks,

Paolo

> I would need a post_check cb. Do you
> think it it sensible to add another cb?

Re: [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 24/04/2015 11:01, Eric Auger wrote:
>> > void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
>> > {
>> >     SysBusDeviceClass *sdc = SYSBUS_DEVICE_GET_CLASS(dev);
>> >     qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1,
>> >                              sdc->irq_set_hook);
>> > }
> Hi Paolo
> 
> I implemented this alternative but my concern is the check method is
> called before the qemu_irq setting. So on this callback I cannot
> retrieve the qemu_irq VFIOINTp struct container object needed to setup
> irqfd hence does not work for me.

Isn't the qemu_irq passed as the third argument to the callback?  I
thought this solution was fine because you weren't passing the "int n"
from sysbus_connect_irq to your notifier.

If you really cannot make it work, I guess your "sysbus: add
irq_routing_notifier" patch would be okay.  I would only ask you to move
the function pointer from SysBusDevice to SysBusDeviceClass.

Thanks,

Paolo

> I would need a post_check cb. Do you
> think it it sensible to add another cb?

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

On 04/24/2015 11:11 AM, Paolo Bonzini wrote:
> 
> 
> On 24/04/2015 11:01, Eric Auger wrote:
>>>> void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
>>>> {
>>>>     SysBusDeviceClass *sdc = SYSBUS_DEVICE_GET_CLASS(dev);
>>>>     qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1,
>>>>                              sdc->irq_set_hook);
>>>> }
>> Hi Paolo
>>
>> I implemented this alternative but my concern is the check method is
>> called before the qemu_irq setting. So on this callback I cannot
>> retrieve the qemu_irq VFIOINTp struct container object needed to setup
>> irqfd hence does not work for me.
> 
> Isn't the qemu_irq passed as the third argument to the callback?  I
> thought this solution was fine because you weren't passing the "int n"
> from sysbus_connect_irq to your notifier.
Yes it is. But I need to access the EventNotifiers for trigger &
resample which were initialized before for eventfd trigger and stored in
the container object. Will study if I can restructure the code ...

Thanks

Eric

> 
> If you really cannot make it work, I guess your "sysbus: add
> irq_routing_notifier" patch would be okay.  I would only ask you to move
> the function pointer from SysBusDevice to SysBusDeviceClass.
> 
> Thanks,
> 
> Paolo
> 
>> I would need a post_check cb. Do you
>> think it it sensible to add another cb?

Re: [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

On 04/24/2015 11:11 AM, Paolo Bonzini wrote:
> 
> 
> On 24/04/2015 11:01, Eric Auger wrote:
>>>> void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
>>>> {
>>>>     SysBusDeviceClass *sdc = SYSBUS_DEVICE_GET_CLASS(dev);
>>>>     qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1,
>>>>                              sdc->irq_set_hook);
>>>> }
>> Hi Paolo
>>
>> I implemented this alternative but my concern is the check method is
>> called before the qemu_irq setting. So on this callback I cannot
>> retrieve the qemu_irq VFIOINTp struct container object needed to setup
>> irqfd hence does not work for me.
> 
> Isn't the qemu_irq passed as the third argument to the callback?  I
> thought this solution was fine because you weren't passing the "int n"
> from sysbus_connect_irq to your notifier.
Yes it is. But I need to access the EventNotifiers for trigger &
resample which were initialized before for eventfd trigger and stored in
the container object. Will study if I can restructure the code ...

Thanks

Eric

> 
> If you really cannot make it work, I guess your "sysbus: add
> irq_routing_notifier" patch would be okay.  I would only ask you to move
> the function pointer from SysBusDevice to SysBusDeviceClass.
> 
> Thanks,
> 
> Paolo
> 
>> I would need a post_check cb. Do you
>> think it it sensible to add another cb?

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 24/04/2015 11:18, Eric Auger wrote:
>>> >> I implemented this alternative but my concern is the check method is
>>> >> called before the qemu_irq setting. So on this callback I cannot
>>> >> retrieve the qemu_irq VFIOINTp struct container object needed to setup
>>> >> irqfd hence does not work for me.
>> > 
>> > Isn't the qemu_irq passed as the third argument to the callback?  I
>> > thought this solution was fine because you weren't passing the "int n"
>> > from sysbus_connect_irq to your notifier.
> Yes it is. But I need to access the EventNotifiers for trigger &
> resample which were initialized before for eventfd trigger and stored in
> the container object. Will study if I can restructure the code ...

What did the notifier code look like with your patch?

Paolo

Re: [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 24/04/2015 11:18, Eric Auger wrote:
>>> >> I implemented this alternative but my concern is the check method is
>>> >> called before the qemu_irq setting. So on this callback I cannot
>>> >> retrieve the qemu_irq VFIOINTp struct container object needed to setup
>>> >> irqfd hence does not work for me.
>> > 
>> > Isn't the qemu_irq passed as the third argument to the callback?  I
>> > thought this solution was fine because you weren't passing the "int n"
>> > from sysbus_connect_irq to your notifier.
> Yes it is. But I need to access the EventNotifiers for trigger &
> resample which were initialized before for eventfd trigger and stored in
> the container object. Will study if I can restructure the code ...

What did the notifier code look like with your patch?

Paolo

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

On 04/24/2015 11:29 AM, Paolo Bonzini wrote:
> 
> 
> On 24/04/2015 11:18, Eric Auger wrote:
>>>>>> I implemented this alternative but my concern is the check method is
>>>>>> called before the qemu_irq setting. So on this callback I cannot
>>>>>> retrieve the qemu_irq VFIOINTp struct container object needed to setup
>>>>>> irqfd hence does not work for me.
>>>>
>>>> Isn't the qemu_irq passed as the third argument to the callback?  I
>>>> thought this solution was fine because you weren't passing the "int n"
>>>> from sysbus_connect_irq to your notifier.
>> Yes it is. But I need to access the EventNotifiers for trigger &
>> resample which were initialized before for eventfd trigger and stored in
>> the container object. Will study if I can restructure the code ...
> 
> What did the notifier code look like with your patch?

Currently both notifiers are stored in the VFIOINTp struct. They are
initialized in vfio_init_intp. VFIO platform device holds a list of
VFIOINTp struct.

When the vfio_start_irqfd callback is called the qemuirq is not yet
initialized so I cannot retrieve the EventNotifiers by container_of.

I can allocate and initialize them in the callback but I need a place to
store them to close the eventfd.

Also at the moment I start irqfd I must take some actions to stop (user
side) eventfd trigger; this also urges me to get access to VFIOINTp
struct. Some extracted pieces below (under work).

Eric

typedef struct VFIOINTp {
    QLIST_ENTRY(VFIOINTp) next; /* entry for IRQ list */
    QSIMPLEQ_ENTRY(VFIOINTp) pqnext; /* entry for pending IRQ queue */
    EventNotifier interrupt; /* eventfd triggered on interrupt */
    EventNotifier unmask; /* eventfd for unmask on QEMU bypass */
    qemu_irq qemuirq;
    struct VFIOPlatformDevice *vdev; /* back pointer to device */
    int state; /* inactive, pending, active */
    uint8_t pin; /* index */
    uint32_t flags; /* IRQ info flags */
    bool kvm_accel; /* set when QEMU bypass through KVM enabled */
} VFIOINTp;


static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
                                struct vfio_irq_info info)
{
    int ret;
    VFIOPlatformDevice *vdev =
        container_of(vbasedev, VFIOPlatformDevice, vbasedev);
    SysBusDevice *sbdev = SYS_BUS_DEVICE(vdev);
    VFIOINTp *intp;

    intp = g_malloc0(sizeof(*intp));
    intp->vdev = vdev;
    intp->pin = info.index;
    intp->flags = info.flags;
    intp->state = VFIO_IRQ_INACTIVE;
    intp->kvm_accel = false;

    sysbus_init_irq(sbdev, &intp->qemuirq);

    /* Get an eventfd for trigger */
    ret = event_notifier_init(&intp->interrupt, 0);
    if (ret) {
        g_free(intp);
        error_report("vfio: Error: trigger event_notifier_init failed ");
        return NULL;
    }
    /* Get an eventfd for resample/unmask */
    ret = event_notifier_init(&intp->unmask, 0);
    if (ret) {
        g_free(intp);
        error_report("vfio: Error: resample event_notifier_init failed ");
        return NULL;
    }


static void vfio_start_irqfd(Object *obj, const char *name,
                             Object *child, Error ** err)
{
    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(obj);
    qemu_irq irq = OBJECT_CHECK(struct IRQState, (child), TYPE_IRQ);
    struct VFIOINTp *tmp, *intp = container_of(&irq, struct VFIOINTp,
qemuirq);
    EventNotifier *interrupt, *unmask;

    vfio_stop_vfio_signaling(intp);

    interrupt = g_new(EventNotifier, 1);
    unmask = g_new(EventNotifier, 1);

    /* Get an eventfd for trigger */
    ret = event_notifier_init(interrupt, 0);
    if (ret) {
        return NULL;
    }
    /* Get an eventfd for resample/unmask */
    ret = event_notifier_init(unmask, 0);
    if (ret) {
        return NULL;
    }

    kvm_irqchip_add_qemuirq_irqfd_notifier(kvm_state,
                                           interrupt,
                                           unmask,
                                           irq);

../..
}

> 
> Paolo
> 

Re: [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

On 04/24/2015 11:29 AM, Paolo Bonzini wrote:
> 
> 
> On 24/04/2015 11:18, Eric Auger wrote:
>>>>>> I implemented this alternative but my concern is the check method is
>>>>>> called before the qemu_irq setting. So on this callback I cannot
>>>>>> retrieve the qemu_irq VFIOINTp struct container object needed to setup
>>>>>> irqfd hence does not work for me.
>>>>
>>>> Isn't the qemu_irq passed as the third argument to the callback?  I
>>>> thought this solution was fine because you weren't passing the "int n"
>>>> from sysbus_connect_irq to your notifier.
>> Yes it is. But I need to access the EventNotifiers for trigger &
>> resample which were initialized before for eventfd trigger and stored in
>> the container object. Will study if I can restructure the code ...
> 
> What did the notifier code look like with your patch?

Currently both notifiers are stored in the VFIOINTp struct. They are
initialized in vfio_init_intp. VFIO platform device holds a list of
VFIOINTp struct.

When the vfio_start_irqfd callback is called the qemuirq is not yet
initialized so I cannot retrieve the EventNotifiers by container_of.

I can allocate and initialize them in the callback but I need a place to
store them to close the eventfd.

Also at the moment I start irqfd I must take some actions to stop (user
side) eventfd trigger; this also urges me to get access to VFIOINTp
struct. Some extracted pieces below (under work).

Eric

typedef struct VFIOINTp {
    QLIST_ENTRY(VFIOINTp) next; /* entry for IRQ list */
    QSIMPLEQ_ENTRY(VFIOINTp) pqnext; /* entry for pending IRQ queue */
    EventNotifier interrupt; /* eventfd triggered on interrupt */
    EventNotifier unmask; /* eventfd for unmask on QEMU bypass */
    qemu_irq qemuirq;
    struct VFIOPlatformDevice *vdev; /* back pointer to device */
    int state; /* inactive, pending, active */
    uint8_t pin; /* index */
    uint32_t flags; /* IRQ info flags */
    bool kvm_accel; /* set when QEMU bypass through KVM enabled */
} VFIOINTp;


static VFIOINTp *vfio_init_intp(VFIODevice *vbasedev,
                                struct vfio_irq_info info)
{
    int ret;
    VFIOPlatformDevice *vdev =
        container_of(vbasedev, VFIOPlatformDevice, vbasedev);
    SysBusDevice *sbdev = SYS_BUS_DEVICE(vdev);
    VFIOINTp *intp;

    intp = g_malloc0(sizeof(*intp));
    intp->vdev = vdev;
    intp->pin = info.index;
    intp->flags = info.flags;
    intp->state = VFIO_IRQ_INACTIVE;
    intp->kvm_accel = false;

    sysbus_init_irq(sbdev, &intp->qemuirq);

    /* Get an eventfd for trigger */
    ret = event_notifier_init(&intp->interrupt, 0);
    if (ret) {
        g_free(intp);
        error_report("vfio: Error: trigger event_notifier_init failed ");
        return NULL;
    }
    /* Get an eventfd for resample/unmask */
    ret = event_notifier_init(&intp->unmask, 0);
    if (ret) {
        g_free(intp);
        error_report("vfio: Error: resample event_notifier_init failed ");
        return NULL;
    }


static void vfio_start_irqfd(Object *obj, const char *name,
                             Object *child, Error ** err)
{
    VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(obj);
    qemu_irq irq = OBJECT_CHECK(struct IRQState, (child), TYPE_IRQ);
    struct VFIOINTp *tmp, *intp = container_of(&irq, struct VFIOINTp,
qemuirq);
    EventNotifier *interrupt, *unmask;

    vfio_stop_vfio_signaling(intp);

    interrupt = g_new(EventNotifier, 1);
    unmask = g_new(EventNotifier, 1);

    /* Get an eventfd for trigger */
    ret = event_notifier_init(interrupt, 0);
    if (ret) {
        return NULL;
    }
    /* Get an eventfd for resample/unmask */
    ret = event_notifier_init(unmask, 0);
    if (ret) {
        return NULL;
    }

    kvm_irqchip_add_qemuirq_irqfd_notifier(kvm_state,
                                           interrupt,
                                           unmask,
                                           irq);

../..
}

> 
> Paolo
> 

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 24/04/2015 11:48, Eric Auger wrote:
>> > What did the notifier code look like with your patch?
> Currently both notifiers are stored in the VFIOINTp struct. They are
> initialized in vfio_init_intp. VFIO platform device holds a list of
> VFIOINTp struct.
> 
> When the vfio_start_irqfd callback is called the qemuirq is not yet
> initialized so I cannot retrieve the EventNotifiers by container_of.

Understood better now.  I would pass the "Object **child" variable of
object_set_link_property to the callback then (in qom/object.c).

Then I would go for the other solution (the notifier, just adding the
callback to SysbusDeviceClass).  But I still do not understand why you
didn't have exactly the same problem :) unless you were walking the list
to find the relevant VFIOINTp.

Paolo

> I can allocate and initialize them in the callback but I need a place to
> store them to close the eventfd.
> 
> Also at the moment I start irqfd I must take some actions to stop (user
> side) eventfd trigger; this also urges me to get access to VFIOINTp
> struct. Some extracted pieces below (under work).

Re: [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 24/04/2015 11:48, Eric Auger wrote:
>> > What did the notifier code look like with your patch?
> Currently both notifiers are stored in the VFIOINTp struct. They are
> initialized in vfio_init_intp. VFIO platform device holds a list of
> VFIOINTp struct.
> 
> When the vfio_start_irqfd callback is called the qemuirq is not yet
> initialized so I cannot retrieve the EventNotifiers by container_of.

Understood better now.  I would pass the "Object **child" variable of
object_set_link_property to the callback then (in qom/object.c).

Then I would go for the other solution (the notifier, just adding the
callback to SysbusDeviceClass).  But I still do not understand why you
didn't have exactly the same problem :) unless you were walking the list
to find the relevant VFIOINTp.

Paolo

> I can allocate and initialize them in the callback but I need a place to
> store them to close the eventfd.
> 
> Also at the moment I start irqfd I must take some actions to stop (user
> side) eventfd trigger; this also urges me to get access to VFIOINTp
> struct. Some extracted pieces below (under work).

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

On 04/24/2015 12:02 PM, Paolo Bonzini wrote:
> 
> 
> On 24/04/2015 11:48, Eric Auger wrote:
>>>> What did the notifier code look like with your patch?
>> Currently both notifiers are stored in the VFIOINTp struct. They are
>> initialized in vfio_init_intp. VFIO platform device holds a list of
>> VFIOINTp struct.
>>
>> When the vfio_start_irqfd callback is called the qemuirq is not yet
>> initialized so I cannot retrieve the EventNotifiers by container_of.
> 
> Understood better now.  I would pass the "Object **child" variable of
> object_set_link_property to the callback then (in qom/object.c).
> 
> Then I would go for the other solution (the notifier, just adding the
> callback to SysbusDeviceClass).  But I still do not understand why you
> didn't have exactly the same problem :) unless you were walking the list
> to find the relevant VFIOINTp.
Hi Paolo,

Yes I was walking the list ;-)

So is my understanding coherent: revert to first patch but moving
notifier in class

Thanks

Eric
> 
> Paolo
> 
>> I can allocate and initialize them in the callback but I need a place to
>> store them to close the eventfd.
>>
>> Also at the moment I start irqfd I must take some actions to stop (user
>> side) eventfd trigger; this also urges me to get access to VFIOINTp
>> struct. Some extracted pieces below (under work).

Re: [PATCH 0/2] irq: add get_gsi callback

[Eric Auger]

On 04/24/2015 12:02 PM, Paolo Bonzini wrote:
> 
> 
> On 24/04/2015 11:48, Eric Auger wrote:
>>>> What did the notifier code look like with your patch?
>> Currently both notifiers are stored in the VFIOINTp struct. They are
>> initialized in vfio_init_intp. VFIO platform device holds a list of
>> VFIOINTp struct.
>>
>> When the vfio_start_irqfd callback is called the qemuirq is not yet
>> initialized so I cannot retrieve the EventNotifiers by container_of.
> 
> Understood better now.  I would pass the "Object **child" variable of
> object_set_link_property to the callback then (in qom/object.c).
> 
> Then I would go for the other solution (the notifier, just adding the
> callback to SysbusDeviceClass).  But I still do not understand why you
> didn't have exactly the same problem :) unless you were walking the list
> to find the relevant VFIOINTp.
Hi Paolo,

Yes I was walking the list ;-)

So is my understanding coherent: revert to first patch but moving
notifier in class

Thanks

Eric
> 
> Paolo
> 
>> I can allocate and initialize them in the callback but I need a place to
>> store them to close the eventfd.
>>
>> Also at the moment I start irqfd I must take some actions to stop (user
>> side) eventfd trigger; this also urges me to get access to VFIOINTp
>> struct. Some extracted pieces below (under work).

Re: [Qemu-devel] [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 24/04/2015 13:59, Eric Auger wrote:
> > Then I would go for the other solution (the notifier, just adding the
> > callback to SysbusDeviceClass).  But I still do not understand why you
> > didn't have exactly the same problem :) unless you were walking the list
> > to find the relevant VFIOINTp.
> 
> Yes I was walking the list ;-)
> 
> So is my understanding coherent: revert to first patch but moving
> notifier in class

Yes.

Paolo

Re: [PATCH 0/2] irq: add get_gsi callback

[Paolo Bonzini]

On 24/04/2015 13:59, Eric Auger wrote:
> > Then I would go for the other solution (the notifier, just adding the
> > callback to SysbusDeviceClass).  But I still do not understand why you
> > didn't have exactly the same problem :) unless you were walking the list
> > to find the relevant VFIOINTp.
> 
> Yes I was walking the list ;-)
> 
> So is my understanding coherent: revert to first patch but moving
> notifier in class

Yes.

Paolo