[Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Eric Auger]

Add a new irq_routing_notifier notifier in the SysBusDeviceClass. This
notifier, if populated, is called after sysbus_connect_irq.

This mechanism is used to setup VFIO signaling once VFIO platform
devices get attached to their platform bus, on a machine init done
notifier.

Signed-off-by: Eric Auger <eric.auger@linaro.org>

---

v1 -> v2:
- duly put the notifier in the class and not in the device
---
 hw/core/sysbus.c    | 6 ++++++
 include/hw/sysbus.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
index b53c351..8553a6f 100644
--- a/hw/core/sysbus.c
+++ b/hw/core/sysbus.c
@@ -109,7 +109,13 @@ qemu_irq sysbus_get_connected_irq(SysBusDevice *dev, int n)
 
 void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
 {
+    SysBusDeviceClass *sbd = SYS_BUS_DEVICE_GET_CLASS(dev);
+
     qdev_connect_gpio_out_named(DEVICE(dev), SYSBUS_DEVICE_GPIO_IRQ, n, irq);
+
+    if (sbd->irq_routing_notifier) {
+        sbd->irq_routing_notifier(dev, irq);
+    }
 }
 
 /* Check whether an MMIO region exists */
diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
index d1f3f00..dbf3f0f 100644
--- a/include/hw/sysbus.h
+++ b/include/hw/sysbus.h
@@ -41,6 +41,7 @@ typedef struct SysBusDeviceClass {
     /*< public >*/
 
     int (*init)(SysBusDevice *dev);
+    void (*irq_routing_notifier)(SysBusDevice *dev, qemu_irq irq);
 } SysBusDeviceClass;
 
 struct SysBusDevice {
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Peter Crosthwaite]

On Fri, Apr 24, 2015 at 5:39 AM, Eric Auger <eric.auger@linaro.org> wrote:
> Add a new irq_routing_notifier notifier in the SysBusDeviceClass. This
> notifier, if populated, is called after sysbus_connect_irq.
>
> This mechanism is used to setup VFIO signaling once VFIO platform
> devices get attached to their platform bus, on a machine init done
> notifier.
>
> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>
> ---
>
> v1 -> v2:
> - duly put the notifier in the class and not in the device
> ---
>  hw/core/sysbus.c    | 6 ++++++
>  include/hw/sysbus.h | 1 +
>  2 files changed, 7 insertions(+)
>
> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
> index b53c351..8553a6f 100644
> --- a/hw/core/sysbus.c
> +++ b/hw/core/sysbus.c
> @@ -109,7 +109,13 @@ qemu_irq sysbus_get_connected_irq(SysBusDevice *dev, int n)
>
>  void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
>  {
> +    SysBusDeviceClass *sbd = SYS_BUS_DEVICE_GET_CLASS(dev);
> +
>      qdev_connect_gpio_out_named(DEVICE(dev), SYSBUS_DEVICE_GPIO_IRQ, n, irq);

One of my long term goals is to try and get rid of sysbus IRQ
abstraction completely in favor of just qdev gpios. This means
features that apply to GPIOs automatically apply to IRQs and vice
versa. Can your notifier hook be pushed up to the qdev GPIO level to
make it more globally usable and avoid a new feature to sysbus IRQs?

> +
> +    if (sbd->irq_routing_notifier) {
> +        sbd->irq_routing_notifier(dev, irq);
> +    }
>  }
>
>  /* Check whether an MMIO region exists */
> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
> index d1f3f00..dbf3f0f 100644
> --- a/include/hw/sysbus.h
> +++ b/include/hw/sysbus.h
> @@ -41,6 +41,7 @@ typedef struct SysBusDeviceClass {
>      /*< public >*/
>
>      int (*init)(SysBusDevice *dev);
> +    void (*irq_routing_notifier)(SysBusDevice *dev, qemu_irq irq);

Is it better to make the name more matched to sysbus_connect_irq?
Perhaps connect_irq_notifier. But with the qdev approach this would be
connect_gpio_out_notifier or something along those lines.

Regards,
Peter

>  } SysBusDeviceClass;
>
>  struct SysBusDevice {
> --
> 1.8.3.2
>
>

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Eric Auger]

Hi Peter,
On 04/27/2015 06:09 AM, Peter Crosthwaite wrote:
> On Fri, Apr 24, 2015 at 5:39 AM, Eric Auger <eric.auger@linaro.org> wrote:
>> Add a new irq_routing_notifier notifier in the SysBusDeviceClass. This
>> notifier, if populated, is called after sysbus_connect_irq.
>>
>> This mechanism is used to setup VFIO signaling once VFIO platform
>> devices get attached to their platform bus, on a machine init done
>> notifier.
>>
>> Signed-off-by: Eric Auger <eric.auger@linaro.org>
>>
>> ---
>>
>> v1 -> v2:
>> - duly put the notifier in the class and not in the device
>> ---
>>  hw/core/sysbus.c    | 6 ++++++
>>  include/hw/sysbus.h | 1 +
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/hw/core/sysbus.c b/hw/core/sysbus.c
>> index b53c351..8553a6f 100644
>> --- a/hw/core/sysbus.c
>> +++ b/hw/core/sysbus.c
>> @@ -109,7 +109,13 @@ qemu_irq sysbus_get_connected_irq(SysBusDevice *dev, int n)
>>
>>  void sysbus_connect_irq(SysBusDevice *dev, int n, qemu_irq irq)
>>  {
>> +    SysBusDeviceClass *sbd = SYS_BUS_DEVICE_GET_CLASS(dev);
>> +
>>      qdev_connect_gpio_out_named(DEVICE(dev), SYSBUS_DEVICE_GPIO_IRQ, n, irq);
> 
> One of my long term goals is to try and get rid of sysbus IRQ
> abstraction completely in favor of just qdev gpios. This means
> features that apply to GPIOs automatically apply to IRQs and vice
> versa. Can your notifier hook be pushed up to the qdev GPIO level to
> make it more globally usable and avoid a new feature to sysbus IRQs?
Yes sure, I am going to put the notifier in DeviceClass then.
> 
>> +
>> +    if (sbd->irq_routing_notifier) {
>> +        sbd->irq_routing_notifier(dev, irq);
>> +    }
>>  }
>>
>>  /* Check whether an MMIO region exists */
>> diff --git a/include/hw/sysbus.h b/include/hw/sysbus.h
>> index d1f3f00..dbf3f0f 100644
>> --- a/include/hw/sysbus.h
>> +++ b/include/hw/sysbus.h
>> @@ -41,6 +41,7 @@ typedef struct SysBusDeviceClass {
>>      /*< public >*/
>>
>>      int (*init)(SysBusDevice *dev);
>> +    void (*irq_routing_notifier)(SysBusDevice *dev, qemu_irq irq);
> 
> Is it better to make the name more matched to sysbus_connect_irq?
> Perhaps connect_irq_notifier. But with the qdev approach this would be
> connect_gpio_out_notifier or something along those lines.
OK for that naming

Thanks

Eric
> 
> Regards,
> Peter
> 
>>  } SysBusDeviceClass;
>>
>>  struct SysBusDevice {
>> --
>> 1.8.3.2
>>
>>

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Paolo Bonzini]

On 27/04/2015 10:26, Eric Auger wrote:
>> > One of my long term goals is to try and get rid of sysbus IRQ
>> > abstraction completely in favor of just qdev gpios. This means
>> > features that apply to GPIOs automatically apply to IRQs and vice
>> > versa. Can your notifier hook be pushed up to the qdev GPIO level to
>> > make it more globally usable and avoid a new feature to sysbus IRQs?
> Yes sure, I am going to put the notifier in DeviceClass then.

I've thought too about this, and I'm not sure about it.

It would mean you have to pass the gpio name (e.g.
SYSBUS_DEVICE_GPIO_IRQ) to the hook, and in the case of sysbus IRQs this
would leak the SYSBUS_DEVICE_GPIO_IRQ abstraction to the implementors of
the hook.

Paolo

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Eric Auger]

On 04/27/2015 12:39 PM, Paolo Bonzini wrote:
> 
> 
> On 27/04/2015 10:26, Eric Auger wrote:
>>>> One of my long term goals is to try and get rid of sysbus IRQ
>>>> abstraction completely in favor of just qdev gpios. This means
>>>> features that apply to GPIOs automatically apply to IRQs and vice
>>>> versa. Can your notifier hook be pushed up to the qdev GPIO level to
>>>> make it more globally usable and avoid a new feature to sysbus IRQs?
>> Yes sure, I am going to put the notifier in DeviceClass then.
> 
> I've thought too about this, and I'm not sure about it.
> 
> It would mean you have to pass the gpio name (e.g.
> SYSBUS_DEVICE_GPIO_IRQ) to the hook, and in the case of sysbus IRQs this
> would leak the SYSBUS_DEVICE_GPIO_IRQ abstraction to the implementors of
> the hook.
Hi Paolo,

Currently my notifier has the following proto:
    void (*connect_gpio_out_notifier)(DeviceState *dev, qemu_irq irq);

It is sufficient for my need.

is it really mandated to pass other qdev_connect_gpio_out_named args,
ie. name & n?

Best Regards

Eric
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Paolo Bonzini]

On 27/04/2015 14:20, Eric Auger wrote:
> On 04/27/2015 12:39 PM, Paolo Bonzini wrote:
>>
>>
>> On 27/04/2015 10:26, Eric Auger wrote:
>>>>> One of my long term goals is to try and get rid of sysbus IRQ
>>>>> abstraction completely in favor of just qdev gpios. This means
>>>>> features that apply to GPIOs automatically apply to IRQs and vice
>>>>> versa. Can your notifier hook be pushed up to the qdev GPIO level to
>>>>> make it more globally usable and avoid a new feature to sysbus IRQs?
>>> Yes sure, I am going to put the notifier in DeviceClass then.
>>
>> I've thought too about this, and I'm not sure about it.
>>
>> It would mean you have to pass the gpio name (e.g.
>> SYSBUS_DEVICE_GPIO_IRQ) to the hook, and in the case of sysbus IRQs this
>> would leak the SYSBUS_DEVICE_GPIO_IRQ abstraction to the implementors of
>> the hook.
> Hi Paolo,
> 
> Currently my notifier has the following proto:
>     void (*connect_gpio_out_notifier)(DeviceState *dev, qemu_irq irq);
> 
> It is sufficient for my need.
> 
> is it really mandated to pass other qdev_connect_gpio_out_named args,
> ie. name & n?

It's an ugly situation.  If you look at qdev_connect_gpio_out_named, it
is really a thin wrapper around object_property_set_link.  Just like
Peter wasn't too happy with changing sysbus_connect_irq, the same
objection would apply here.  Callers of object_property_set_link should
call the notifiers, not just those that use qdev_connect_gpio_out_named.

This is why I originally asked you to look into using the check callback
instead.

This is why I think it's better to keep the sysbus patch.

Paolo

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Peter Crosthwaite]

On Mon, Apr 27, 2015 at 6:37 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 27/04/2015 14:20, Eric Auger wrote:
>> On 04/27/2015 12:39 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 27/04/2015 10:26, Eric Auger wrote:
>>>>>> One of my long term goals is to try and get rid of sysbus IRQ
>>>>>> abstraction completely in favor of just qdev gpios. This means
>>>>>> features that apply to GPIOs automatically apply to IRQs and vice
>>>>>> versa. Can your notifier hook be pushed up to the qdev GPIO level to
>>>>>> make it more globally usable and avoid a new feature to sysbus IRQs?
>>>> Yes sure, I am going to put the notifier in DeviceClass then.
>>>
>>> I've thought too about this, and I'm not sure about it.
>>>
>>> It would mean you have to pass the gpio name (e.g.
>>> SYSBUS_DEVICE_GPIO_IRQ) to the hook, and in the case of sysbus IRQs this
>>> would leak the SYSBUS_DEVICE_GPIO_IRQ abstraction to the implementors of
>>> the hook.

That's OK IMO. SYSBUS_DEVICE_GPIO_IRQ was never intended to be
private. The semantics of it are something like "If you don't have
anything better to name your IRQ pin use this".

This adds the requirement on machine level code that you can't
consistently use sysbus_connect_irq for intc connection. But machines
should be able to connect any wires between any cores without having
to special case interrupts or chip-selects, resets or whatever. So the
name of the GPIO has to be exposed to the callback hook registration
if we want to break down this GPIO special casing. Names of interrupt
pins are system-level knowledge so I think this is all OK.

>> Hi Paolo,
>>
>> Currently my notifier has the following proto:
>>     void (*connect_gpio_out_notifier)(DeviceState *dev, qemu_irq irq);
>>
>> It is sufficient for my need.
>>
>> is it really mandated to pass other qdev_connect_gpio_out_named args,
>> ie. name & n?
>
> It's an ugly situation.  If you look at qdev_connect_gpio_out_named, it
> is really a thin wrapper around object_property_set_link.  Just like
> Peter wasn't too happy with changing sysbus_connect_irq, the same
> objection would apply here.  Callers of object_property_set_link should
> call the notifiers, not just those that use qdev_connect_gpio_out_named.
>
> This is why I originally asked you to look into using the check callback
> instead.
>

Is this still feasible? Pushing it up to the higher again to the QOM
level? I think this would be an ideal backend to the problem even if
we still go with a code-friendly sysbus frontend.

Regards,
Peter

> This is why I think it's better to keep the sysbus patch.
>
> Paolo
>

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Eric Auger]

On 04/27/2015 04:39 PM, Peter Crosthwaite wrote:
> On Mon, Apr 27, 2015 at 6:37 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 27/04/2015 14:20, Eric Auger wrote:
>>> On 04/27/2015 12:39 PM, Paolo Bonzini wrote:
>>>>
>>>>
>>>> On 27/04/2015 10:26, Eric Auger wrote:
>>>>>>> One of my long term goals is to try and get rid of sysbus IRQ
>>>>>>> abstraction completely in favor of just qdev gpios. This means
>>>>>>> features that apply to GPIOs automatically apply to IRQs and vice
>>>>>>> versa. Can your notifier hook be pushed up to the qdev GPIO level to
>>>>>>> make it more globally usable and avoid a new feature to sysbus IRQs?
>>>>> Yes sure, I am going to put the notifier in DeviceClass then.
>>>>
>>>> I've thought too about this, and I'm not sure about it.
>>>>
>>>> It would mean you have to pass the gpio name (e.g.
>>>> SYSBUS_DEVICE_GPIO_IRQ) to the hook, and in the case of sysbus IRQs this
>>>> would leak the SYSBUS_DEVICE_GPIO_IRQ abstraction to the implementors of
>>>> the hook.
> 
> That's OK IMO. SYSBUS_DEVICE_GPIO_IRQ was never intended to be
> private. The semantics of it are something like "If you don't have
> anything better to name your IRQ pin use this".
> 
> This adds the requirement on machine level code that you can't
> consistently use sysbus_connect_irq for intc connection. But machines
> should be able to connect any wires between any cores without having
> to special case interrupts or chip-selects, resets or whatever. So the
> name of the GPIO has to be exposed to the callback hook registration
> if we want to break down this GPIO special casing. Names of interrupt
> pins are system-level knowledge so I think this is all OK.
> 
>>> Hi Paolo,
>>>
>>> Currently my notifier has the following proto:
>>>     void (*connect_gpio_out_notifier)(DeviceState *dev, qemu_irq irq);
>>>
>>> It is sufficient for my need.
>>>
>>> is it really mandated to pass other qdev_connect_gpio_out_named args,
>>> ie. name & n?
>>
>> It's an ugly situation.  If you look at qdev_connect_gpio_out_named, it
>> is really a thin wrapper around object_property_set_link.  Just like
>> Peter wasn't too happy with changing sysbus_connect_irq, the same
>> objection would apply here.  Callers of object_property_set_link should
>> call the notifiers, not just those that use qdev_connect_gpio_out_named.
>>
>> This is why I originally asked you to look into using the check callback
>> instead.
>>
> 
> Is this still feasible? Pushing it up to the higher again to the QOM
> level? I think this would be an ideal backend to the problem even if
> we still go with a code-friendly sysbus frontend.

Peter, Paolo,

After your feedbacks, I feel I need to spend some more time on the
original check() track. I would prefer not to introduce any patch that
will make issue in the future.

Thanks

Eric

> 
> Regards,
> Peter
> 
>> This is why I think it's better to keep the sysbus patch.
>>
>> Paolo
>>

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Paolo Bonzini]

On 27/04/2015 16:56, Eric Auger wrote:
> Peter, Paolo,
> 
> After your feedbacks, I feel I need to spend some more time on the
> original check() track. I would prefer not to introduce any patch that
> will make issue in the future.

Peter, see the other threads between me and Eric.  See in particular
http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02749.html
starting at "The notifier actually is not even necessary" and the
replies from there.

If you have any idea, please help.

Paolo

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Peter Crosthwaite]

On Mon, Apr 27, 2015 at 8:01 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 27/04/2015 16:56, Eric Auger wrote:
>> Peter, Paolo,
>>
>> After your feedbacks, I feel I need to spend some more time on the
>> original check() track. I would prefer not to introduce any patch that
>> will make issue in the future.
>
> Peter, see the other threads between me and Eric.  See in particular
> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02749.html
> starting at "The notifier actually is not even necessary" and the
> replies from there.
>

Thanks,

I see the problem with check. In this reply

http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02956.html

Eric says that the problem with the check hook is it happens before
the setting. I think this can be solved with a RYO link setter for
GPIOs. We almost have an in-tree precedent with MemoryRegion and the
container property (memory.c):

 960     op = object_property_add(OBJECT(mr), "container",
 961                              "link<" TYPE_MEMORY_REGION ">",
 962                              memory_region_get_container,
 963                              NULL, /* memory_region_set_container */
 964                              NULL, NULL, &error_abort);

Now in reality we could have done this link normal style as it is only
a trivial getter, but the reason the link was done this way in the
first place, is because I have a follow up patch to memory.c that adds
a customer Link setter:

+static void memory_region_set_container(Object *obj, Visitor *v, void *opaque,
+                                        const char *name, Error **errp)
+{
+    MemoryRegion *mr = MEMORY_REGION(obj);
+    Error *local_err = NULL;
+    MemoryRegion *old_container = mr->container;
+    MemoryRegion *new_container = NULL;
+    char *path = NULL;
+
+    visit_type_str(v, &path, name, &local_err);
+
+    if (!local_err && strcmp(path, "") != 0) {
+        new_container = MEMORY_REGION(object_resolve_link(obj, name, path,
+                                      &local_err));
+        while (new_container->alias) {
+            new_container = new_container->alias;
+        }
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    object_ref(OBJECT(new_container));
+
+    memory_region_transaction_begin();
+    memory_region_ref(mr);
+    if (old_container) {
+        memory_region_del_subregion(old_container, mr);
+    }
+    mr->container = new_container;
+    if (new_container) {
+        memory_region_update_container_subregions(mr);
+    }
+    memory_region_unref(mr);
+    memory_region_transaction_commit();
+
+    object_unref(OBJECT(old_container));
+}
+

     op = object_property_add(OBJECT(mr), "container",
                              "link<" TYPE_MEMORY_REGION ">",
                              memory_region_get_container,
-                             NULL, /* memory_region_set_container */
+                             memory_region_set_container,
                              NULL, NULL, &error_abort);


The function does the normal link setting - similar to
object_set_link_property (not to be confused with
object_property_set_link!) but is surrounded by class specific side
effects. Specifically in this case, it does
memory_region_transaction_begin/ref/unref/commit etc for the MR.

For this GPIO case, you could create a custom setter that does the
normal set, then calls the DeviceClass installed hook (or you can
install the hook to the object and init it at qdev_init_gpio_out_named
time as suggested in the eariler thread). The callback will happen
after the link is populated.

To reduce verbosity, I suggest making object_set_link_property() a
visible API, then RYO link setters can call it surrounded by custom
behavior e.g:

foo_object_set_bar_property(...)
{
    pre_set_link_side_effects();
    object_set_link_property();
    post_set_link_side_effects();
}

object_set_link_property() would need to be coreified and wrapped to
remove it's awareness of LinkProperty type (as that doesn't exist in
RYO properties) in this case.

Regards,
Peter

> If you have any idea, please help.
>
> Paolo
>

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Eric Auger]

Hi Peter,

On 04/27/2015 07:43 PM, Peter Crosthwaite wrote:
> On Mon, Apr 27, 2015 at 8:01 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 27/04/2015 16:56, Eric Auger wrote:
>>> Peter, Paolo,
>>>
>>> After your feedbacks, I feel I need to spend some more time on the
>>> original check() track. I would prefer not to introduce any patch that
>>> will make issue in the future.
>>
>> Peter, see the other threads between me and Eric.  See in particular
>> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02749.html
>> starting at "The notifier actually is not even necessary" and the
>> replies from there.
>>
> 
> Thanks,
> 
> I see the problem with check. In this reply
> 
> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02956.html
> 
> Eric says that the problem with the check hook is it happens before
> the setting. I think this can be solved with a RYO link setter for
> GPIOs. We almost have an in-tree precedent with MemoryRegion and the
> container property (memory.c):
> 
>  960     op = object_property_add(OBJECT(mr), "container",
>  961                              "link<" TYPE_MEMORY_REGION ">",
>  962                              memory_region_get_container,
>  963                              NULL, /* memory_region_set_container */
>  964                              NULL, NULL, &error_abort);
> 
> Now in reality we could have done this link normal style as it is only
> a trivial getter, but the reason the link was done this way in the
> first place, is because I have a follow up patch to memory.c that adds
> a customer Link setter:
> 
> +static void memory_region_set_container(Object *obj, Visitor *v, void *opaque,
> +                                        const char *name, Error **errp)
> +{
> +    MemoryRegion *mr = MEMORY_REGION(obj);
> +    Error *local_err = NULL;
> +    MemoryRegion *old_container = mr->container;
> +    MemoryRegion *new_container = NULL;
> +    char *path = NULL;
> +
> +    visit_type_str(v, &path, name, &local_err);
> +
> +    if (!local_err && strcmp(path, "") != 0) {
> +        new_container = MEMORY_REGION(object_resolve_link(obj, name, path,
> +                                      &local_err));
> +        while (new_container->alias) {
> +            new_container = new_container->alias;
> +        }
> +    }
> +
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
> +    object_ref(OBJECT(new_container));
> +
> +    memory_region_transaction_begin();
> +    memory_region_ref(mr);
> +    if (old_container) {
> +        memory_region_del_subregion(old_container, mr);
> +    }
> +    mr->container = new_container;
> +    if (new_container) {
> +        memory_region_update_container_subregions(mr);
> +    }
> +    memory_region_unref(mr);
> +    memory_region_transaction_commit();
> +
> +    object_unref(OBJECT(old_container));
> +}
> +
> 
>      op = object_property_add(OBJECT(mr), "container",
>                               "link<" TYPE_MEMORY_REGION ">",
>                               memory_region_get_container,
> -                             NULL, /* memory_region_set_container */
> +                             memory_region_set_container,
>                               NULL, NULL, &error_abort);
> 
> 
> The function does the normal link setting - similar to
> object_set_link_property (not to be confused with
> object_property_set_link!) but is surrounded by class specific side
> effects. Specifically in this case, it does
> memory_region_transaction_begin/ref/unref/commit etc for the MR.
> 
> For this GPIO case, you could create a custom setter that does the
> normal set, then calls the DeviceClass installed hook (or you can
> install the hook to the object and init it at qdev_init_gpio_out_named
> time as suggested in the eariler thread). The callback will happen
> after the link is populated.
> 
> To reduce verbosity, I suggest making object_set_link_property() a
> visible API, then RYO link setters can call it surrounded by custom
> behavior e.g:
> 
> foo_object_set_bar_property(...)
> {
>     pre_set_link_side_effects();
>     object_set_link_property();
>     post_set_link_side_effects();
> }
> 
> object_set_link_property() would need to be coreified and wrapped to
> remove it's awareness of LinkProperty type (as that doesn't exist in
> RYO properties) in this case.

Thank you Peter for detailing this.

Yesterday I re-worked on the solution based on the check() method where
- check would take a Object **child as a 3d parameter
- we would assign *child before the call and in case the check fails set
the *child back to NULL in object_set_link_property.

I need to do some more testing.

I don't know if this solution would be acceptable too. If not I will
implement according to your guidelines.

Best Regards

Eric
> 
> Regards,
> Peter
> 
>> If you have any idea, please help.
>>
>> Paolo
>>

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Peter Crosthwaite]

On Mon, Apr 27, 2015 at 11:46 PM, Eric Auger <eric.auger@linaro.org> wrote:
> Hi Peter,
>
> On 04/27/2015 07:43 PM, Peter Crosthwaite wrote:
>> On Mon, Apr 27, 2015 at 8:01 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>
>>> On 27/04/2015 16:56, Eric Auger wrote:
>>>> Peter, Paolo,
>>>>
>>>> After your feedbacks, I feel I need to spend some more time on the
>>>> original check() track. I would prefer not to introduce any patch that
>>>> will make issue in the future.
>>>
>>> Peter, see the other threads between me and Eric.  See in particular
>>> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02749.html
>>> starting at "The notifier actually is not even necessary" and the
>>> replies from there.
>>>
>>
>> Thanks,
>>
>> I see the problem with check. In this reply
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02956.html
>>
>> Eric says that the problem with the check hook is it happens before
>> the setting. I think this can be solved with a RYO link setter for
>> GPIOs. We almost have an in-tree precedent with MemoryRegion and the
>> container property (memory.c):
>>
>>  960     op = object_property_add(OBJECT(mr), "container",
>>  961                              "link<" TYPE_MEMORY_REGION ">",
>>  962                              memory_region_get_container,
>>  963                              NULL, /* memory_region_set_container */
>>  964                              NULL, NULL, &error_abort);
>>
>> Now in reality we could have done this link normal style as it is only
>> a trivial getter, but the reason the link was done this way in the
>> first place, is because I have a follow up patch to memory.c that adds
>> a customer Link setter:
>>
>> +static void memory_region_set_container(Object *obj, Visitor *v, void *opaque,
>> +                                        const char *name, Error **errp)
>> +{
>> +    MemoryRegion *mr = MEMORY_REGION(obj);
>> +    Error *local_err = NULL;
>> +    MemoryRegion *old_container = mr->container;
>> +    MemoryRegion *new_container = NULL;
>> +    char *path = NULL;
>> +
>> +    visit_type_str(v, &path, name, &local_err);
>> +
>> +    if (!local_err && strcmp(path, "") != 0) {
>> +        new_container = MEMORY_REGION(object_resolve_link(obj, name, path,
>> +                                      &local_err));
>> +        while (new_container->alias) {
>> +            new_container = new_container->alias;
>> +        }
>> +    }
>> +
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +
>> +    object_ref(OBJECT(new_container));
>> +
>> +    memory_region_transaction_begin();
>> +    memory_region_ref(mr);
>> +    if (old_container) {
>> +        memory_region_del_subregion(old_container, mr);
>> +    }
>> +    mr->container = new_container;
>> +    if (new_container) {
>> +        memory_region_update_container_subregions(mr);
>> +    }
>> +    memory_region_unref(mr);
>> +    memory_region_transaction_commit();
>> +
>> +    object_unref(OBJECT(old_container));
>> +}
>> +
>>
>>      op = object_property_add(OBJECT(mr), "container",
>>                               "link<" TYPE_MEMORY_REGION ">",
>>                               memory_region_get_container,
>> -                             NULL, /* memory_region_set_container */
>> +                             memory_region_set_container,
>>                               NULL, NULL, &error_abort);
>>
>>
>> The function does the normal link setting - similar to
>> object_set_link_property (not to be confused with
>> object_property_set_link!) but is surrounded by class specific side
>> effects. Specifically in this case, it does
>> memory_region_transaction_begin/ref/unref/commit etc for the MR.
>>
>> For this GPIO case, you could create a custom setter that does the
>> normal set, then calls the DeviceClass installed hook (or you can
>> install the hook to the object and init it at qdev_init_gpio_out_named
>> time as suggested in the eariler thread). The callback will happen
>> after the link is populated.
>>
>> To reduce verbosity, I suggest making object_set_link_property() a
>> visible API, then RYO link setters can call it surrounded by custom
>> behavior e.g:
>>
>> foo_object_set_bar_property(...)
>> {
>>     pre_set_link_side_effects();
>>     object_set_link_property();
>>     post_set_link_side_effects();
>> }
>>
>> object_set_link_property() would need to be coreified and wrapped to
>> remove it's awareness of LinkProperty type (as that doesn't exist in
>> RYO properties) in this case.
>
> Thank you Peter for detailing this.
>
> Yesterday I re-worked on the solution based on the check() method where
> - check would take a Object **child as a 3d parameter
> - we would assign *child before the call and in case the check fails set
> the *child back to NULL in object_set_link_property.
>

I think this is starting to reach outside the design intent of the
check, letting it be an API that takes over the responsibility of
->set. Ideally check should be boolean and side-effectless.

> I need to do some more testing.
>
> I don't know if this solution would be acceptable too. If not I will
> implement according to your guidelines.
>

Lets see what Paolo says before another rework.

Regards,
Peter

> Best Regards
>
> Eric
>>
>> Regards,
>> Peter
>>
>>> If you have any idea, please help.
>>>
>>> Paolo
>>>
>
>

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Eric Auger]

On 04/28/2015 08:57 AM, Peter Crosthwaite wrote:
> On Mon, Apr 27, 2015 at 11:46 PM, Eric Auger <eric.auger@linaro.org> wrote:
>> Hi Peter,
>>
>> On 04/27/2015 07:43 PM, Peter Crosthwaite wrote:
>>> On Mon, Apr 27, 2015 at 8:01 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>>
>>>> On 27/04/2015 16:56, Eric Auger wrote:
>>>>> Peter, Paolo,
>>>>>
>>>>> After your feedbacks, I feel I need to spend some more time on the
>>>>> original check() track. I would prefer not to introduce any patch that
>>>>> will make issue in the future.
>>>>
>>>> Peter, see the other threads between me and Eric.  See in particular
>>>> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02749.html
>>>> starting at "The notifier actually is not even necessary" and the
>>>> replies from there.
>>>>
>>>
>>> Thanks,
>>>
>>> I see the problem with check. In this reply
>>>
>>> http://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02956.html
>>>
>>> Eric says that the problem with the check hook is it happens before
>>> the setting. I think this can be solved with a RYO link setter for
>>> GPIOs. We almost have an in-tree precedent with MemoryRegion and the
>>> container property (memory.c):
>>>
>>>  960     op = object_property_add(OBJECT(mr), "container",
>>>  961                              "link<" TYPE_MEMORY_REGION ">",
>>>  962                              memory_region_get_container,
>>>  963                              NULL, /* memory_region_set_container */
>>>  964                              NULL, NULL, &error_abort);
>>>
>>> Now in reality we could have done this link normal style as it is only
>>> a trivial getter, but the reason the link was done this way in the
>>> first place, is because I have a follow up patch to memory.c that adds
>>> a customer Link setter:
>>>
>>> +static void memory_region_set_container(Object *obj, Visitor *v, void *opaque,
>>> +                                        const char *name, Error **errp)
>>> +{
>>> +    MemoryRegion *mr = MEMORY_REGION(obj);
>>> +    Error *local_err = NULL;
>>> +    MemoryRegion *old_container = mr->container;
>>> +    MemoryRegion *new_container = NULL;
>>> +    char *path = NULL;
>>> +
>>> +    visit_type_str(v, &path, name, &local_err);
>>> +
>>> +    if (!local_err && strcmp(path, "") != 0) {
>>> +        new_container = MEMORY_REGION(object_resolve_link(obj, name, path,
>>> +                                      &local_err));
>>> +        while (new_container->alias) {
>>> +            new_container = new_container->alias;
>>> +        }
>>> +    }
>>> +
>>> +    if (local_err) {
>>> +        error_propagate(errp, local_err);
>>> +        return;
>>> +    }
>>> +
>>> +    object_ref(OBJECT(new_container));
>>> +
>>> +    memory_region_transaction_begin();
>>> +    memory_region_ref(mr);
>>> +    if (old_container) {
>>> +        memory_region_del_subregion(old_container, mr);
>>> +    }
>>> +    mr->container = new_container;
>>> +    if (new_container) {
>>> +        memory_region_update_container_subregions(mr);
>>> +    }
>>> +    memory_region_unref(mr);
>>> +    memory_region_transaction_commit();
>>> +
>>> +    object_unref(OBJECT(old_container));
>>> +}
>>> +
>>>
>>>      op = object_property_add(OBJECT(mr), "container",
>>>                               "link<" TYPE_MEMORY_REGION ">",
>>>                               memory_region_get_container,
>>> -                             NULL, /* memory_region_set_container */
>>> +                             memory_region_set_container,
>>>                               NULL, NULL, &error_abort);
>>>
>>>
>>> The function does the normal link setting - similar to
>>> object_set_link_property (not to be confused with
>>> object_property_set_link!) but is surrounded by class specific side
>>> effects. Specifically in this case, it does
>>> memory_region_transaction_begin/ref/unref/commit etc for the MR.
>>>
>>> For this GPIO case, you could create a custom setter that does the
>>> normal set, then calls the DeviceClass installed hook (or you can
>>> install the hook to the object and init it at qdev_init_gpio_out_named
>>> time as suggested in the eariler thread). The callback will happen
>>> after the link is populated.
>>>
>>> To reduce verbosity, I suggest making object_set_link_property() a
>>> visible API, then RYO link setters can call it surrounded by custom
>>> behavior e.g:
>>>
>>> foo_object_set_bar_property(...)
>>> {
>>>     pre_set_link_side_effects();
>>>     object_set_link_property();
>>>     post_set_link_side_effects();
>>> }
>>>
>>> object_set_link_property() would need to be coreified and wrapped to
>>> remove it's awareness of LinkProperty type (as that doesn't exist in
>>> RYO properties) in this case.
>>
>> Thank you Peter for detailing this.
>>
>> Yesterday I re-worked on the solution based on the check() method where
>> - check would take a Object **child as a 3d parameter
>> - we would assign *child before the call and in case the check fails set
>> the *child back to NULL in object_set_link_property.
>>
> 
> I think this is starting to reach outside the design intent of the
> check, letting it be an API that takes over the responsibility of
> ->set. Ideally check should be boolean and side-effectless.
I acknowledge ;-)
> 
>> I need to do some more testing.
>>
>> I don't know if this solution would be acceptable too. If not I will
>> implement according to your guidelines.
>>
> 
> Lets see what Paolo says before another rework.
sure

Thanks

Eric
> 
> Regards,
> Peter
> 
>> Best Regards
>>
>> Eric
>>>
>>> Regards,
>>> Peter
>>>
>>>> If you have any idea, please help.
>>>>
>>>> Paolo
>>>>
>>
>>

Re: [Qemu-devel] [PATCH v2] sysbus: add irq_routing_notifier

[Paolo Bonzini]

On 27/04/2015 19:43, Peter Crosthwaite wrote:
> To reduce verbosity, I suggest making object_set_link_property() a
> visible API, then RYO link setters can call it surrounded by custom
> behavior e.g:
> 
> foo_object_set_bar_property(...)
> {
>     pre_set_link_side_effects();
>     object_set_link_property();
>     post_set_link_side_effects();
> }
> 
> object_set_link_property() would need to be coreified and wrapped to
> remove it's awareness of LinkProperty type (as that doesn't exist in
> RYO properties) in this case.

I agree with this, but I think it can be done separately from Eric's work.

Paolo