| Message ID | 20170209170904.5713-24-alex.bennee@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | MTTCG Base enabling patches with ARM enablement | expand |
On 9 February 2017 at 17:09, Alex Bennée <alex.bennee@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.bennee@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
Peter Maydell <peter.maydell@linaro.org> writes: > On 9 February 2017 at 17:09, Alex Bennée <alex.bennee@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.bennee@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
On 10 February 2017 at 15:19, Alex Bennée <alex.bennee@linaro.org> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: >> 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? In practice there will be only one but I don't think we should rely on that. malloc/free is probably better than overly convoluted code at this point. thanks -- PMM
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 { + IMX6SRCState *s; + unsigned long reset_bit; +}; + +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)); +} + + static void imx6_src_write(void *opaque, hwaddr offset, uint64_t value, unsigned size) { @@ -153,7 +193,7 @@ static void imx6_src_write(void *opaque, hwaddr offset, uint64_t value, arm_set_cpu_off(3); } /* We clear the reset bits as the processor changed state */ - clear_bit(CORE3_RST_SHIFT, ¤t_value); + imx6_defer_clear_reset_bit(3, s, CORE3_RST_SHIFT); clear_bit(CORE3_RST_SHIFT, &change_mask); } if (EXTRACT(change_mask, CORE2_ENABLE)) { @@ -162,11 +202,11 @@ static void imx6_src_write(void *opaque, hwaddr offset, uint64_t value, arm_set_cpu_on(2, s->regs[SRC_GPR5], s->regs[SRC_GPR6], 3, false); } else { - /* CORE 3 is shut down */ + /* CORE 2 is shut down */ arm_set_cpu_off(2); } /* We clear the reset bits as the processor changed state */ - clear_bit(CORE2_RST_SHIFT, ¤t_value); + imx6_defer_clear_reset_bit(2, s, CORE2_RST_SHIFT); clear_bit(CORE2_RST_SHIFT, &change_mask); } if (EXTRACT(change_mask, CORE1_ENABLE)) { @@ -175,28 +215,28 @@ static void imx6_src_write(void *opaque, hwaddr offset, uint64_t value, arm_set_cpu_on(1, s->regs[SRC_GPR3], s->regs[SRC_GPR4], 3, false); } else { - /* CORE 3 is shut down */ + /* CORE 1 is shut down */ arm_set_cpu_off(1); } /* We clear the reset bits as the processor changed state */ - clear_bit(CORE1_RST_SHIFT, ¤t_value); + imx6_defer_clear_reset_bit(1, s, CORE1_RST_SHIFT); clear_bit(CORE1_RST_SHIFT, &change_mask); } if (EXTRACT(change_mask, CORE0_RST)) { arm_reset_cpu(0); - clear_bit(CORE0_RST_SHIFT, ¤t_value); + imx6_defer_clear_reset_bit(0, s, CORE0_RST_SHIFT); } if (EXTRACT(change_mask, CORE1_RST)) { arm_reset_cpu(1); - clear_bit(CORE1_RST_SHIFT, ¤t_value); + imx6_defer_clear_reset_bit(1, s, CORE1_RST_SHIFT); } if (EXTRACT(change_mask, CORE2_RST)) { arm_reset_cpu(2); - clear_bit(CORE2_RST_SHIFT, ¤t_value); + imx6_defer_clear_reset_bit(2, s, CORE2_RST_SHIFT); } if (EXTRACT(change_mask, CORE3_RST)) { arm_reset_cpu(3); - clear_bit(CORE3_RST_SHIFT, ¤t_value); + imx6_defer_clear_reset_bit(3, s, CORE3_RST_SHIFT); } if (EXTRACT(change_mask, SW_IPU2_RST)) { /* We pretend the IPU2 is reset */
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.bennee@linaro.org> --- hw/misc/imx6_src.c | 58 +++++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 9 deletions(-) -- 2.11.0