Re: [PATCH v7 10/11] vl: replace deprecated qbus_reset_all registration

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

On 1/15/20 1:36 PM, Damien Hedde wrote:
> Replace deprecated qbus_reset_all by resettable_cold_reset_fn for
> the sysbus reset registration.
> 
> Apart for the raspi machines, this does not impact the behavior
> because:
> + at this point resettable just calls the old reset methods of devices
>    and buses in the same order as qdev/qbus.
> + resettable handlers registered with qemu_register_reset are
>    serialized; there is no interleaving.
> + eventual explicit calls to legacy reset API (device_reset or
>    qdev/qbus_reset) inside this reset handler will not be masked out
>    by resettable mechanism; they do not go through resettable api.
> 
> For the raspi machines, during the sysbus reset the sd-card is not
> reset twice anymore but only once. This is a consequence of switching
> both sysbus reset and changing parent to resettable; it detects the
> second reset is not needed. This has no impact on the state after
> reset; the sd-card reset method only reset local state and query
> information from the block backend.
> 
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> The raspi reset change can be observed by using the following command
> (reset will occurs, then do Ctrl-C to end qemu; no firmware is
> given here).
> qemu-system-aarch64 -M raspi3 \
>      -trace resettable_phase_hold_exec \
>      -trace qdev_update_parent_bus \
>      -trace resettable_change_parent \
>      -trace qdev_reset -trace qbus_reset
> 
> Before the patch, the qdev/qbus_reset traces show when reset method are
> called. After the patch, the resettable_phase_hold_exec show when reset
> method are called.
> 
> The traced reset order of the raspi3 is listed below. I've added empty
> lines and the tree structure.
> 
>   +->bcm2835-peripherals reset
>   |
>   |       +->sd-card reset
>   |   +->sd-bus reset
>   +->bcm2835_gpio reset
>   |      -> dev_update_parent_bus (move the sd-card on the sdhci-bus)
>   |      -> resettable_change_parent
>   |
>   +->bcm2835-dma reset
>   |
>   |   +->bcm2835-sdhost-bus reset
>   +->bcm2835-sdhost reset
>   |
>   |       +->sd-card (reset ONLY BEFORE BEFORE THE PATCH)
>   |   +->sdhci-bus reset
>   +->generic-sdhci reset
>   |
>   +->bcm2835-rng reset
>   +->bcm2835-property reset
>   +->bcm2835-fb reset
>   +->bcm2835-mbox reset
>   +->bcm2835-aux reset
>   +->pl011 reset
>   +->bcm2835-ic reset
>   +->bcm2836-control reset
> System reset
> 
> In both case, the sd-card is reset (being on bcm2835_gpio/sd-bus) then moved
> to generic-sdhci/sdhci-bus by the bcm2835_gpio reset method.
> 
> Before the patch, it is then reset again being part of generic-sdhci/sdhci-bus.
> After the patch, it considered again for reset but its reset method is not
> called because it is already flagged as reset.

I find this information helpful, have you considered including it in the 
description?

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

> ---
>   vl.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/vl.c b/vl.c
> index 751401214c..e5a537d4f9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4362,7 +4362,15 @@ int main(int argc, char **argv, char **envp)
>   
>       /* TODO: once all bus devices are qdevified, this should be done
>        * when bus is created by qdev.c */
> -    qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
> +    /*
> +     * TODO: If we had a main 'reset container' that the whole system
> +     * lived in, we could reset that using the multi-phase reset
> +     * APIs. For the moment, we just reset the sysbus, which will cause
> +     * all devices hanging off it (and all their child buses, recursively)
> +     * to be reset. Note that this will *not* reset any Device objects
> +     * which are not attached to some part of the qbus tree!
> +     */
> +    qemu_register_reset(resettable_cold_reset_fn, sysbus_get_default());
>       qemu_run_machine_init_done_notifiers();
>   
>       if (rom_check_and_register_reset() != 0) {
>