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 <address@hidden> wrote:
>> Hi Peter,
>>
>> On 04/27/2015 07:43 PM, Peter Crosthwaite wrote:
>>> 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.
>>
>> 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
>>>>
>>
>>