On 9 February 2017 at 17:09, Alex Bennée <alex.ben...@linaro.org> 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 <alex.ben...@linaro.org>
> ---
> 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
