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

[Peter Crosthwaite]

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