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

[Peter Crosthwaite]

On Mon, Apr 27, 2015 at 8:01 AM, Paolo Bonzini <address@hidden> 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
>