Re: [PATCH v5 03/13] hw/core: create Resettable QOM interface

[Damien Hedde <damien.hedde@greensocs.com>]

On 11/29/19 7:32 PM, Peter Maydell wrote:
> On Fri, 18 Oct 2019 at 16:07, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> This commit defines an interface allowing multi-phase reset. This aims
>> to solve a problem of the actual single-phase reset (built in
>> DeviceClass and BusClass): reset behavior is dependent on the order
>> in which reset handlers are called. In particular doing external
>> side-effect (like setting an qemu_irq) is problematic because receiving
>> object may not be reset yet.
>>
>> The Resettable interface divides the reset in 3 well defined phases.
>> To reset an object tree, all 1st phases are executed then all 2nd then
>> all 3rd. See the comments in include/hw/resettable.h for a more complete
>> description. The interface defines 3 phases to let the future
>> possibility of holding an object into reset for some time.
>>
>> The qdev/qbus reset in DeviceClass and BusClass will be modified in
>> following commits to use this interface. A mechanism is provided
>> to allow executing a transitional reset handler in place of the 2nd
>> phase which is executed in children-then-parent order inside a tree.
>> This will allow to transition devices and buses smoothly while
>> keeping the exact current qdev/qbus reset behavior for now.
>>
>> Documentation will be added in a following commit.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>
>> In this patch only a single reset type is supported, but the interface
>> allows for more to be defined.
>>
>> I had some thought about problems which may arise when having multiple
>> reset types:
> 
> [snip]
> 
> Yeah, these all seem right. We clearly need to think a bit
> more before we add multiple reset types. Let's get this basic
> just-cold-reset in for now and come back to the rest later.
> 
> 
> Almost all of my comments below are just grammar/typo fixes
> for comments. The only substantives are:
>  * globals
>  * copyright/licensing comment needed in the .h file
> and they're pretty minor items.
> 
>> +/**
>> + * enter_phase_in_progress:
>> + * Flag telling whether we are currently in an enter phase where side
>> + * effects are forbidden. This flag allows us to catch if reset is called
>> + * again during during this phase.
>> + */
>> +static bool enter_phase_in_progress;
> 
> This looks weird -- I don't think a global for this works,
> because you might have several distinct subtrees of
> devices, and be doing reset on them both at once.
> I think that we only use this for an assert, though -- is
> that right? If so, we could just drop this.

We say that we need to own the iothread mutex for any reset, so global
should be ok. Thought, I just checked, it's only mentioned in the
documentation not in the header file. I should probably add a comment
there too along with the link to the documentation file.

If we want to drop the iothread mutex constraint. I think we need to
carefully check there is no hidden problem. In particular in hold and
exit phases we allow to have external effects like setting gpios and we
have no way to control what it provokes.

You're right it is just for assert: to avoid any miss-use of the api
which could lead to being in bad reset state. So we can indeed drop it.

> 
>> +void resettable_assert_reset(Object *obj, ResetType type)
>> +{
>> +    assert(!enter_phase_in_progress);
>> +    /* TODO: change that assert when adding support for other reset types */
> 
> I'm not sure which assert this is referring to -- the one above
> the comment, or the one below ?

It refers to the assert(type == RESET_TYPE_COLD).
I added theses because we cannot just add items in the enum to have
working multiple reset types.
A comment like this will be more clear:
/*
 * TODO: Additional reset types need support in phases handling
 * functions (resettable_phase_enter/hold/exit()) before allowing more
 * enum entries. Remove the following assert when it is done.
 */

> 
>> +    assert(type == RESET_TYPE_COLD);
>> +    trace_resettable_reset_assert_begin(obj, type);
>> +    enter_phase_in_progress = true;
>> +    resettable_phase_enter(obj, NULL, type);
>> +    enter_phase_in_progress = false;
>> +    resettable_phase_hold(obj, NULL, type);
>> +    trace_resettable_reset_assert_end(obj);
>> +}
>> +
>> +void resettable_release_reset(Object *obj, ResetType type)
>> +{
>> +    assert(!enter_phase_in_progress);
>> +    /* TODO: change that assert when adding support for other reset types */
> 
> Ditto.
> 
> 
> 
>> --- /dev/null
>> +++ b/include/hw/resettable.h
>> @@ -0,0 +1,199 @@
>> +#ifndef HW_RESETTABLE_H
>> +#define HW_RESETTABLE_H
>> +
> 
> All new files, including even small header files, should have
> the usual copyright-and-license comment at the top. (Can you
> check also whether this needs adding for any other new files the
> patchset creates, please?)

I'll do that and fix all the typos

Thanks for the review,
--
Damien