From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from zen.linaro.local ([81.128.185.34]) by smtp.gmail.com with ESMTPSA id e71sm2022340wma.8.2017.02.10.07.19.02 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 10 Feb 2017 07:19:02 -0800 (PST) Received: from zen (localhost [127.0.0.1]) by zen.linaro.local (Postfix) with ESMTPS id 9D8973E014D; Fri, 10 Feb 2017 15:19:17 +0000 (GMT) References: <20170209170904.5713-1-alex.bennee@linaro.org> <20170209170904.5713-24-alex.bennee@linaro.org> User-agent: mu4e 0.9.19; emacs 25.2.1 From: Alex =?utf-8?Q?Benn=C3=A9e?= To: Peter Maydell Cc: Richard Henderson , MTTCG Devel , QEMU Developers , KONRAD =?utf-8?B?RnLDqWTDqXJpYw==?= , Alvise Rigo , "Emilio G. Cota" , Pranith Kumar , Nikunj A Dadhania , Mark Burton , Paolo Bonzini , Jan Kiszka , Fedorov Sergey , Bamvor Zhang Jian , Peter Chubb , "open list\:i.MX31" Subject: Re: [PATCH v11 23/24] hw/misc/imx6_src: defer clearing of SRC_SCR reset bits In-reply-to: Date: Fri, 10 Feb 2017 15:19:17 +0000 Message-ID: <87tw82vzt6.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-TUID: WLfmyyCXABMQ Peter Maydell writes: > On 9 February 2017 at 17:09, Alex Bennée 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 >> --- >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56651) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ccCya-00008V-BS for qemu-devel@nongnu.org; Fri, 10 Feb 2017 10:19:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ccCyX-0006RE-6Q for qemu-devel@nongnu.org; Fri, 10 Feb 2017 10:19:08 -0500 Received: from mail-wm0-x22e.google.com ([2a00:1450:400c:c09::22e]:36122) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1ccCyW-0006Qy-Uh for qemu-devel@nongnu.org; Fri, 10 Feb 2017 10:19:05 -0500 Received: by mail-wm0-x22e.google.com with SMTP id c85so263028247wmi.1 for ; Fri, 10 Feb 2017 07:19:04 -0800 (PST) References: <20170209170904.5713-1-alex.bennee@linaro.org> <20170209170904.5713-24-alex.bennee@linaro.org> From: Alex =?utf-8?Q?Benn=C3=A9e?= In-reply-to: Date: Fri, 10 Feb 2017 15:19:17 +0000 Message-ID: <87tw82vzt6.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v11 23/24] hw/misc/imx6_src: defer clearing of SRC_SCR reset bits List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Richard Henderson , MTTCG Devel , QEMU Developers , KONRAD =?utf-8?B?RnLDqWTDqXJpYw==?= , Alvise Rigo , "Emilio G. Cota" , Pranith Kumar , Nikunj A Dadhania , Mark Burton , Paolo Bonzini , Jan Kiszka , Fedorov Sergey , Bamvor Zhang Jian , Peter Chubb , "open list:i.MX31" Peter Maydell writes: > On 9 February 2017 at 17:09, Alex Bennée 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 >> --- >> 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