[Qemu-devel] [PATCH 0/3] Extend check callback usability for irq connect notifier

[Qemu-devel] [PATCH 0/3] Extend check callback usability for irq connect notifier

[Eric Auger]

The VFIO platform device gets connected to the platform bus
on a machine init done notifier. Only at that point irqfd can be
setup. An irq connect notifier would be helpful to do that job.
Instead of adding a new callback at sysbus or qdev level, this
series proposes to use the property check() callback (credit to
Paolo).

- the callback is passed to qdev_init_gpio_out_named
- sysbus class now holds a irq_set_hook method usable as check
  callback
- sysbus_init_irq initializes the callback to the class method
- object_set_link_property is modified so that
  * the target object is populated before the check
  * check takes an Object ** enabling to access the object
    container.

Then The VFIO platform device would override the irq_set_hook
method in a separate patch.

Please let me know if those modifications are acceptable. Else
I will follow Peter's proposal. 

Best Regards

Eric

Eric Auger (3):
  qdev: pass the check callback to qdev_init_gpio_out_named
  qdev: check callback takes Object **target as third argument
  sysbus: add irq_set_hook

 hw/core/qdev-properties.c    |  2 +-
 hw/core/qdev.c               |  8 +++++---
 hw/core/sysbus.c             |  8 +++++++-
 include/hw/qdev-core.h       |  3 ++-
 include/hw/qdev-properties.h |  2 +-
 include/hw/sysbus.h          |  1 +
 include/qom/object.h         |  8 +++++---
 qom/object.c                 | 15 +++++++++------
 8 files changed, 31 insertions(+), 16 deletions(-)

-- 
1.8.3.2

[Qemu-devel] [PATCH 1/3] qdev: pass the check callback to qdev_init_gpio_out_named

[Eric Auger]

qdev_init_gpio_out_named takes a new argument corresponding to the
check callback passed to object_property_add_link. In qdev_init_gpio_out
this callback is set to the dummy object_property_allow_set_link.

This will allow qdev_init_gpio_out_named callers to specialize this
callback, typically sysbus.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/core/qdev.c         | 8 +++++---
 include/hw/qdev-core.h | 3 ++-
 include/qom/object.h   | 3 +++
 3 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 7fa58be..dc82df4 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -452,7 +452,8 @@ void qdev_init_gpio_in(DeviceState *dev, qemu_irq_handler handler, int n)
 }
 
 void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
-                              const char *name, int n)
+                              const char *name, int n,
+                              object_property_set_link_t check_cb)
 {
     int i;
     NamedGPIOList *gpio_list = qdev_get_named_gpio_list(dev, name);
@@ -465,7 +466,7 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
         memset(&pins[i], 0, sizeof(*pins));
         object_property_add_link(OBJECT(dev), propname, TYPE_IRQ,
                                  (Object **)&pins[i],
-                                 object_property_allow_set_link,
+                                 check_cb,
                                  OBJ_PROP_LINK_UNREF_ON_RELEASE,
                                  &error_abort);
     }
@@ -474,7 +475,8 @@ void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
 
 void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n)
 {
-    qdev_init_gpio_out_named(dev, pins, NULL, n);
+    qdev_init_gpio_out_named(dev, pins, NULL, n,
+                             object_property_allow_set_link);
 }
 
 qemu_irq qdev_get_gpio_in_named(DeviceState *dev, const char *name, int n)
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 24c973d..9d500a3 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -294,7 +294,8 @@ void qdev_init_gpio_out(DeviceState *dev, qemu_irq *pins, int n);
 void qdev_init_gpio_in_named(DeviceState *dev, qemu_irq_handler handler,
                              const char *name, int n);
 void qdev_init_gpio_out_named(DeviceState *dev, qemu_irq *pins,
-                              const char *name, int n);
+                              const char *name, int n,
+                              object_property_set_link_t fn);
 
 void qdev_pass_gpios(DeviceState *dev, DeviceState *container,
                      const char *name);
diff --git a/include/qom/object.h b/include/qom/object.h
index d2d7748..4687fa1 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -33,6 +33,9 @@ typedef struct TypeInfo TypeInfo;
 typedef struct InterfaceClass InterfaceClass;
 typedef struct InterfaceInfo InterfaceInfo;
 
+typedef void (*object_property_set_link_t)(Object *, const char *,
+                                           Object *, Error **);
+
 #define TYPE_OBJECT "object"
 
 /**
-- 
1.8.3.2

[Qemu-devel] [PATCH 2/3] qdev: check callback takes Object **target as third argument

[Eric Auger]

Check callback now takes as third argument an Object **. In
object_set_link_property, we pass the property child as argument.
We also assign the *child before the check call so that enhanced
check can be performed in the callback. In case the check fails,
the old value is restored and ref count is left unchanged.

This typically makes possible to do checks both on the *child
content (for instance a qemu_irq) and also perform some actions/
checks on its container, which was not possible before.

This is typically useful for starting irqfd setup in vfio platform
use case.

Signed-off-by: Eric Auger <eric.auger@linaro.org>
---
 hw/core/qdev-properties.c    |  2 +-
 include/hw/qdev-properties.h |  2 +-
 include/qom/object.h         |  7 +++----
 qom/object.c                 | 15 +++++++++------
 4 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index 570d5f0..a42f9d4 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -22,7 +22,7 @@ void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
 }
 
 void qdev_prop_allow_set_link_before_realize(Object *obj, const char *name,
-                                             Object *val, Error **errp)
+                                             Object **target, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
 
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index d67dad5..b2868be 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -213,6 +213,6 @@ void qdev_prop_set_after_realize(DeviceState *dev, const char *name,
  * object_property_add_link().
  */
 void qdev_prop_allow_set_link_before_realize(Object *obj, const char *name,
-                                             Object *val, Error **errp);
+                                             Object **target, Error **errp);
 
 #endif
diff --git a/include/qom/object.h b/include/qom/object.h
index 4687fa1..0a7daff 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -34,7 +34,7 @@ typedef struct InterfaceClass InterfaceClass;
 typedef struct InterfaceInfo InterfaceInfo;
 
 typedef void (*object_property_set_link_t)(Object *, const char *,
-                                           Object *, Error **);
+                                           Object **, Error **);
 
 #define TYPE_OBJECT "object"
 
@@ -1136,7 +1136,7 @@ typedef enum {
  * an error.
  */
 void object_property_allow_set_link(Object *, const char *,
-                                    Object *, Error **);
+                                    Object **, Error **);
 
 /**
  * object_property_add_link:
@@ -1168,8 +1168,7 @@ void object_property_allow_set_link(Object *, const char *,
  */
 void object_property_add_link(Object *obj, const char *name,
                               const char *type, Object **child,
-                              void (*check)(Object *obj, const char *name,
-                                            Object *val, Error **errp),
+                              object_property_set_link_t check,
                               ObjectPropertyLinkFlags flags,
                               Error **errp);
 
diff --git a/qom/object.c b/qom/object.c
index b8dff43..cc9ed87 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1112,14 +1112,14 @@ out:
 }
 
 void object_property_allow_set_link(Object *obj, const char *name,
-                                    Object *val, Error **errp)
+                                    Object **target, Error **errp)
 {
     /* Allow the link to be set, always */
 }
 
 typedef struct {
     Object **child;
-    void (*check)(Object *, const char *, Object *, Error **);
+    void (*check)(Object *, const char *, Object **, Error **);
     ObjectPropertyLinkFlags flags;
 } LinkProperty;
 
@@ -1201,14 +1201,17 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
         return;
     }
 
-    prop->check(obj, name, new_target, &local_err);
+    object_ref(new_target);
+    *child = new_target;
+
+    prop->check(obj, name, child, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
+        *child = old_target;
+        object_ref(new_target);
         return;
     }
 
-    object_ref(new_target);
-    *child = new_target;
     object_unref(old_target);
 }
 
@@ -1233,7 +1236,7 @@ static void object_release_link_property(Object *obj, const char *name,
 void object_property_add_link(Object *obj, const char *name,
                               const char *type, Object **child,
                               void (*check)(Object *, const char *,
-                                            Object *, Error **),
+                                            Object **, Error **),
                               ObjectPropertyLinkFlags flags,
                               Error **errp)
 {
-- 
1.8.3.2

[Qemu-devel] [PATCH 3/3] sysbus: add irq_set_hook

[Eric Auger]

Add a new callback in the SysBusDeviceClass. This callback now can
be overriden by devices inheriting from sysbus. By default the callback
is set to the dummy object_property_allow_set_link callback.

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

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index 8553a6f..e2c4899 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -165,7 +165,10 @@ void sysbus_mmio_map_overlap(SysBusDevice *dev, int n, hwaddr addr,
 /* Request an IRQ source.  The actual IRQ object may be populated later.  */
 void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p)
 {
-    qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1);
+    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_GET_CLASS(dev);
+
+    qdev_init_gpio_out_named(DEVICE(dev), p, SYSBUS_DEVICE_GPIO_IRQ, 1,
+                             sbc->irq_set_hook);
 }
 
 /* Pass IRQs from a target device.  */
@@ -316,8 +319,11 @@ MemoryRegion *sysbus_address_space(SysBusDevice *dev)
 static void sysbus_device_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
+    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
+
     k->init = sysbus_device_init;
     k->bus_type = TYPE_SYSTEM_BUS;
+    sbc->irq_set_hook = object_property_allow_set_link;
 }
 
 static const TypeInfo sysbus_device_type_info = {
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index dbf3f0f..26ffb2e 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -42,6 +42,7 @@ typedef struct SysBusDeviceClass {
 
     int (*init)(SysBusDevice *dev);
     void (*irq_routing_notifier)(SysBusDevice *dev, qemu_irq irq);
+    object_property_set_link_t irq_set_hook;
 } SysBusDeviceClass;
 
 struct SysBusDevice {
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH 2/3] qdev: check callback takes Object **target as third argument

[Paolo Bonzini]

On 28/04/2015 11:18, Eric Auger wrote:
> Check callback now takes as third argument an Object **. In
> object_set_link_property, we pass the property child as argument.
> We also assign the *child before the check call so that enhanced
> check can be performed in the callback. In case the check fails,
> the old value is restored and ref count is left unchanged.
> 
> This typically makes possible to do checks both on the *child
> content (for instance a qemu_irq) and also perform some actions/
> checks on its container, which was not possible before.
> 
> This is typically useful for starting irqfd setup in vfio platform
> use case.

s/typically/for example/

I can't say that "starting irqfd setup in vfio platform" is typical. :)

> diff --git a/include/qom/object.h b/include/qom/object.h
> index 4687fa1..0a7daff 100644
> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -34,7 +34,7 @@ typedef struct InterfaceClass InterfaceClass;
>  typedef struct InterfaceInfo InterfaceInfo;
>  
>  typedef void (*object_property_set_link_t)(Object *, const char *,
> -                                           Object *, Error **);
> +                                           Object **, Error **);

Let's make the new argument "Object * const*", and rename the typedef to
LinkPropertySetter.

Ok with that change.

Paolo


>  
>  #define TYPE_OBJECT "object"
>  
> @@ -1136,7 +1136,7 @@ typedef enum {
>   * an error.
>   */
>  void object_property_allow_set_link(Object *, const char *,
> -                                    Object *, Error **);
> +                                    Object **, Error **);
>  
>  /**
>   * object_property_add_link:
> @@ -1168,8 +1168,7 @@ void object_property_allow_set_link(Object *, const char *,
>   */
>  void object_property_add_link(Object *obj, const char *name,
>                                const char *type, Object **child,
> -                              void (*check)(Object *obj, const char *name,
> -                                            Object *val, Error **errp),
> +                              object_property_set_link_t check,
>                                ObjectPropertyLinkFlags flags,
>                                Error **errp);
>  
> diff --git a/qom/object.c b/qom/object.c
> index b8dff43..cc9ed87 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1112,14 +1112,14 @@ out:
>  }
>  
>  void object_property_allow_set_link(Object *obj, const char *name,
> -                                    Object *val, Error **errp)
> +                                    Object **target, Error **errp)
>  {
>      /* Allow the link to be set, always */
>  }
>  
>  typedef struct {
>      Object **child;
> -    void (*check)(Object *, const char *, Object *, Error **);
> +    void (*check)(Object *, const char *, Object **, Error **);
>      ObjectPropertyLinkFlags flags;
>  } LinkProperty;
>  
> @@ -1201,14 +1201,17 @@ static void object_set_link_property(Object *obj, Visitor *v, void *opaque,
>          return;
>      }
>  
> -    prop->check(obj, name, new_target, &local_err);
> +    object_ref(new_target);
> +    *child = new_target;
> +
> +    prop->check(obj, name, child, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> +        *child = old_target;
> +        object_ref(new_target);
>          return;
>      }
>  
> -    object_ref(new_target);
> -    *child = new_target;
>      object_unref(old_target);
>  }
>  
> @@ -1233,7 +1236,7 @@ static void object_release_link_property(Object *obj, const char *name,
>  void object_property_add_link(Object *obj, const char *name,
>                                const char *type, Object **child,
>                                void (*check)(Object *, const char *,
> -                                            Object *, Error **),
> +                                            Object **, Error **),
>                                ObjectPropertyLinkFlags flags,
>                                Error **errp)
>  {
> 

Re: [Qemu-devel] [PATCH 0/3] Extend check callback usability for irq connect notifier

[Peter Crosthwaite]

On Tue, Apr 28, 2015 at 2:18 AM, Eric Auger <eric.auger@linaro.org> wrote:
> The VFIO platform device gets connected to the platform bus
> on a machine init done notifier. Only at that point irqfd can be
> setup. An irq connect notifier would be helpful to do that job.
> Instead of adding a new callback at sysbus or qdev level, this
> series proposes to use the property check() callback (credit to
> Paolo).
>
> - the callback is passed to qdev_init_gpio_out_named
> - sysbus class now holds a irq_set_hook method usable as check
>   callback
> - sysbus_init_irq initializes the callback to the class method
> - object_set_link_property is modified so that
>   * the target object is populated before the check
>   * check takes an Object ** enabling to access the object
>     container.
>
> Then The VFIO platform device would override the irq_set_hook
> method in a separate patch.
>
> Please let me know if those modifications are acceptable. Else
> I will follow Peter's proposal.
>
> Best Regards
>
> Eric
>
> Eric Auger (3):
>   qdev: pass the check callback to qdev_init_gpio_out_named
>   qdev: check callback takes Object **target as third argument
>   sysbus: add irq_set_hook
>
>  hw/core/qdev-properties.c    |  2 +-
>  hw/core/qdev.c               |  8 +++++---
>  hw/core/sysbus.c             |  8 +++++++-
>  include/hw/qdev-core.h       |  3 ++-
>  include/hw/qdev-properties.h |  2 +-
>  include/hw/sysbus.h          |  1 +
>  include/qom/object.h         |  8 +++++---
>  qom/object.c                 | 15 +++++++++------

I think following the latest reply, Paolo is ok for my proposal but it
is higher effort and to be done as follow up work. This means we
should just merge the sysbus variant of your patch and fix it all
later.

This avoids these temporary changes to the QOM core.

Sorry for the back and forth. Ill have a look at the RYO link approach
later this week.

Regards,
Peter

>  8 files changed, 31 insertions(+), 16 deletions(-)
>
> --
> 1.8.3.2
>
>

Re: [Qemu-devel] [PATCH 0/3] Extend check callback usability for irq connect notifier

[Eric Auger]

Hi Peter,
On 04/28/2015 08:21 PM, Peter Crosthwaite wrote:
> On Tue, Apr 28, 2015 at 2:18 AM, Eric Auger <eric.auger@linaro.org> wrote:
>> The VFIO platform device gets connected to the platform bus
>> on a machine init done notifier. Only at that point irqfd can be
>> setup. An irq connect notifier would be helpful to do that job.
>> Instead of adding a new callback at sysbus or qdev level, this
>> series proposes to use the property check() callback (credit to
>> Paolo).
>>
>> - the callback is passed to qdev_init_gpio_out_named
>> - sysbus class now holds a irq_set_hook method usable as check
>>   callback
>> - sysbus_init_irq initializes the callback to the class method
>> - object_set_link_property is modified so that
>>   * the target object is populated before the check
>>   * check takes an Object ** enabling to access the object
>>     container.
>>
>> Then The VFIO platform device would override the irq_set_hook
>> method in a separate patch.
>>
>> Please let me know if those modifications are acceptable. Else
>> I will follow Peter's proposal.
>>
>> Best Regards
>>
>> Eric
>>
>> Eric Auger (3):
>>   qdev: pass the check callback to qdev_init_gpio_out_named
>>   qdev: check callback takes Object **target as third argument
>>   sysbus: add irq_set_hook
>>
>>  hw/core/qdev-properties.c    |  2 +-
>>  hw/core/qdev.c               |  8 +++++---
>>  hw/core/sysbus.c             |  8 +++++++-
>>  include/hw/qdev-core.h       |  3 ++-
>>  include/hw/qdev-properties.h |  2 +-
>>  include/hw/sysbus.h          |  1 +
>>  include/qom/object.h         |  8 +++++---
>>  qom/object.c                 | 15 +++++++++------
> 
> I think following the latest reply, Paolo is ok for my proposal but it
> is higher effort and to be done as follow up work. This means we
> should just merge the sysbus variant of your patch and fix it all
> later.

Well Paolo did a 2d reply to this series saying he was OK with the
change with few modifications
(https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03783.html)
which I posted yesterday included in my VFIO series v13
(https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg03943.html). I
thought it was OK, sorry.

Now I would not insist on pushing that series. Was is important for me
is to unlock my VFIO platform series for 2.4 application.

So if both of you say, let's use the temporary sysbus irq connect
notifier, I will repost the VFIO series with that single patch file.

Anyway for the VFIO platform device code this is not a big change.

Waiting for your confirmation.

Best Regards

Eric

> 
> This avoids these temporary changes to the QOM core.
> 
> Sorry for the back and forth. Ill have a look at the RYO link approach
> later this week.
> 
> Regards,
> Peter
> 
>>  8 files changed, 31 insertions(+), 16 deletions(-)
>>
>> --
>> 1.8.3.2
>>
>>