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

Eric Auger
2015-04-28T06:40:16Z

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
>>