Peter Maydell <peter.mayd...@linaro.org> writes:
> 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".
Yeah that's a hangover from the original code that was using unsigned
long for holding bitfields. I'll drop it down to 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?
Yes they run in the order they are queued.
>
> 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.
Previous patches had expanded the run_on_cpu code to have things like
CPUState and a single field to avoid malloc where we can. However I need
the IMX6SRCState and I don't know if I can get that in the work
function. Will there only ever be one on the system?
>
> Otherwise this looks OK.
>
> thanks
> -- PMM
--
Alex Bennée
