Re: [Qemu-devel] [PATCH v11 23/24] hw/misc/imx6_src: defer clearing of SRC_SCR reset bits

[Peter Maydell]

On 9 February 2017 at 17:09, Alex Bennée <address@hidden> wrote:
> The arm_reset_cpu/set_cpu_on/set_cpu_off() functions do their work
> asynchronously in the target vCPUs context. As a result we need to
> ensure the SRC_SCR reset bits correctly report the reset status at the
> right time. To do this we defer the clearing of the bit with an async
> job which will run after the work queued by ARM powerctl functions.
>
> Signed-off-by: Alex Bennée <address@hidden>
> ---
>  hw/misc/imx6_src.c | 58 
> +++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 49 insertions(+), 9 deletions(-)
>
> diff --git a/hw/misc/imx6_src.c b/hw/misc/imx6_src.c
> index 55b817b8d7..f7c1d94a3e 100644
> --- a/hw/misc/imx6_src.c
> +++ b/hw/misc/imx6_src.c
> @@ -14,6 +14,7 @@
>  #include "qemu/bitops.h"
>  #include "qemu/log.h"
>  #include "arm-powerctl.h"
> +#include "qom/cpu.h"
>
>  #ifndef DEBUG_IMX6_SRC
>  #define DEBUG_IMX6_SRC 0
> @@ -113,6 +114,45 @@ static uint64_t imx6_src_read(void *opaque, hwaddr 
> offset, unsigned size)
>      return value;
>  }
>
> +
> +/* The reset is asynchronous so we need to defer clearing the reset
> + * bit until the work is completed.
> + */
> +
> +struct src_scr_reset_info {

Struct names should be CamelCase.

> +    IMX6SRCState *s;
> +    unsigned long reset_bit;

Unsigned long ? That's always a bit of a red flag because it's
variable in size from host to host. I think you want "int".

> +};
> +
> +static void imx6_clear_reset_bit(CPUState *cpu, run_on_cpu_data data)
> +{
> +    struct src_scr_reset_info *ri = data.host_ptr;
> +    IMX6SRCState *s = ri->s;
> +
> +    assert(qemu_mutex_iothread_locked());
> +
> +    s->regs[SRC_SCR] = deposit32(s->regs[SRC_SCR], ri->reset_bit, 1, 0);
> +    DPRINTF("reg[%s] <= 0x%" PRIx32 "\n",
> +            imx6_src_reg_name(SRC_SCR), s->regs[SRC_SCR]);
> +
> +    g_free(ri);
> +}
> +
> +static void imx6_defer_clear_reset_bit(int cpuid,
> +                                       IMX6SRCState *s,
> +                                       unsigned long reset_shift)
> +{
> +    struct src_scr_reset_info *ri;
> +
> +    ri = g_malloc(sizeof(struct src_scr_reset_info));
> +    ri->s = s;
> +    ri->reset_bit = reset_shift;
> +
> +    async_run_on_cpu(arm_get_cpu_by_id(cpuid), imx6_clear_reset_bit,
> +                     RUN_ON_CPU_HOST_PTR(ri));
> +}

What happens if we do a system reset between calling this
and the async hook running? Do all the outstanding async
run hooks guarantee to run first?

I guess a malloc-and-free is OK since the guest isn't going to be
bouncing CPUs through reset very often, though it's a bit ugly to
see in device code.

Otherwise this looks OK.

thanks
-- PMM