Re: [PATCH v7 07/11] hw/core/qdev: update hotplug reset regarding resettable

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 1/15/20 1:36 PM, Damien Hedde wrote:
> This commit make use of the resettable API to reset the device being
> hotplugged when it is realized. Also it ensures it is put in a reset
> state coherent with the parent it is plugged into.
> 
> Note that there is a difference in the reset. Instead of resetting
> only the hotplugged device, we reset also its subtree (switch to
> resettable API). This is not expected to be a problem because
> sub-buses are just realized too. If a hotplugged device has any
> sub-buses it is logical to reset them too at this point.
> 
> The recently added should_be_hidden and PCI's partially_hotplugged
> mechanisms do not interfere with realize operation:
> + In the should_be_hidden use case, device creation is
> delayed.
> + The partially_hotplugged mechanism prevents a device to be
> unplugged and unrealized from qdev POV and unrealized.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> v7 update: inline resettable_state_clear()
> 
> v6 update: clear the reset state before doing any reset. Although it
> is apparently highly improbable that a device be realized again after
> being unrealized. See here for a discussion about this point and the
> partially_hotplugged/shoud_be_hidden cases.
> https://lists.nongnu.org/archive/html/qemu-devel/2019-11/msg04897.html
> ---
>   include/hw/resettable.h | 11 +++++++++++
>   hw/core/qdev.c          | 15 ++++++++++++++-
>   2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/include/hw/resettable.h b/include/hw/resettable.h
> index 3c87ab86c7..3f90da5b5b 100644
> --- a/include/hw/resettable.h
> +++ b/include/hw/resettable.h
> @@ -153,6 +153,17 @@ struct ResettableState {
>       bool exit_phase_in_progress;
>   };
>   
> +/**
> + * resettable_state_clear:
> + * Clear the state. It puts the state to the initial (zeroed) state required
> + * to reuse an object. Typically used in realize step of base classes
> + * implementing the interface.
> + */
> +static inline void resettable_state_clear(ResettableState *state)
> +{
> +    memset(state, 0, sizeof(ResettableState));
> +}
> +
>   /**
>    * resettable_reset:
>    * Trigger a reset on an object @obj of type @type. @obj must implement
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index 310b87e25a..5fac4cd8fc 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -937,6 +937,12 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>               }
>           }
>   
> +        /*
> +         * Clear the reset state, in case the object was previously unrealized
> +         * with a dirty state.
> +         */
> +        resettable_state_clear(&dev->reset);
> +
>           QLIST_FOREACH(bus, &dev->child_bus, sibling) {
>               object_property_set_bool(OBJECT(bus), true, "realized",
>                                            &local_err);
> @@ -945,7 +951,14 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>               }
>           }
>           if (dev->hotplugged) {
> -            device_legacy_reset(dev);
> +            /*
> +             * Reset the device, as well as its subtree which, at this point,
> +             * should be realized too.
> +             */
> +            resettable_assert_reset(OBJECT(dev), RESET_TYPE_COLD);
> +            resettable_change_parent(OBJECT(dev), OBJECT(dev->parent_bus),
> +                                     NULL);
> +            resettable_release_reset(OBJECT(dev), RESET_TYPE_COLD);
>           }
>           dev->pending_deleted_event = false;
>   
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>