Message ID | 20191216233519.29030-7-nieklinnenbank@gmail.com |
---|---|
State | New |
Headers | show |
Series | Add Allwinner H3 SoC and Orange Pi PC Machine | expand |
Hello Peter, In the previous version of this patch series I included the fix for setting CP10,CP11 bits in arm_set_cpu_on(), which is now in master (0c7f8c43daf65560). While that worked, I did not realize that setting those bits require rebuilding the flags. Philippe reported this [1] initially, later on during review we discussed [2] and attempted to correct it [3]. Could you please have a short look at this? Right now I don't see anymore issues, but I'm just not very familiar with this area of the code. Regards, Niek [1] https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg01920.html [2] https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg02784.html [3] https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg02785.html On Tue, Dec 17, 2019 at 12:36 AM Niek Linnenbank <nieklinnenbank@gmail.com> wrote: > After setting CP15 bits in arm_set_cpu_on() the cached hflags must > be rebuild to reflect the changed processor state. Without rebuilding, > the cached hflags would be inconsistent until the next call to > arm_rebuild_hflags(). When QEMU is compiled with debugging enabled > (--enable-debug), this problem is captured shortly after the first > call to arm_set_cpu_on() for CPUs running in ARM 32-bit non-secure mode: > > qemu-system-arm: target/arm/helper.c:11359: cpu_get_tb_cpu_state: > Assertion `flags == rebuild_hflags_internal(env)' failed. > Aborted (core dumped) > > Fixes: 0c7f8c43daf65 > Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com> > --- > target/arm/arm-powerctl.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c > index b064513d44..b75f813b40 100644 > --- a/target/arm/arm-powerctl.c > +++ b/target/arm/arm-powerctl.c > @@ -127,6 +127,9 @@ static void arm_set_cpu_on_async_work(CPUState > *target_cpu_state, > target_cpu->env.regs[0] = info->context_id; > } > > + /* CP15 update requires rebuilding hflags */ > + arm_rebuild_hflags(&target_cpu->env); > + > /* Start the new CPU at the requested address */ > cpu_set_pc(target_cpu_state, info->entry); > > -- > 2.17.1 > >
Cc'ing Richard : this is one for you I think... (surely we need to rebuild the hflags from scratch when we power up a CPU anyway?) thanks -- PMM On Mon, 16 Dec 2019 at 23:44, Niek Linnenbank <nieklinnenbank@gmail.com> wrote: > > Hello Peter, > > In the previous version of this patch series I included the fix for setting CP10,CP11 bits > in arm_set_cpu_on(), which is now in master (0c7f8c43daf65560). While that worked, I did not > realize that setting those bits require rebuilding the flags. Philippe reported this [1] initially, > later on during review we discussed [2] and attempted to correct it [3]. > > Could you please have a short look at this? Right now I don't see anymore > issues, but I'm just not very familiar with this area of the code. > > Regards, > Niek > > [1] https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg01920.html > [2] https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg02784.html > [3] https://lists.gnu.org/archive/html/qemu-devel/2019-12/msg02785.html > > > On Tue, Dec 17, 2019 at 12:36 AM Niek Linnenbank <nieklinnenbank@gmail.com> wrote: >> >> After setting CP15 bits in arm_set_cpu_on() the cached hflags must >> be rebuild to reflect the changed processor state. Without rebuilding, >> the cached hflags would be inconsistent until the next call to >> arm_rebuild_hflags(). When QEMU is compiled with debugging enabled >> (--enable-debug), this problem is captured shortly after the first >> call to arm_set_cpu_on() for CPUs running in ARM 32-bit non-secure mode: >> >> qemu-system-arm: target/arm/helper.c:11359: cpu_get_tb_cpu_state: >> Assertion `flags == rebuild_hflags_internal(env)' failed. >> Aborted (core dumped) >> >> Fixes: 0c7f8c43daf65 >> Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com> >> --- >> target/arm/arm-powerctl.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c >> index b064513d44..b75f813b40 100644 >> --- a/target/arm/arm-powerctl.c >> +++ b/target/arm/arm-powerctl.c >> @@ -127,6 +127,9 @@ static void arm_set_cpu_on_async_work(CPUState *target_cpu_state, >> target_cpu->env.regs[0] = info->context_id; >> } >> >> + /* CP15 update requires rebuilding hflags */ >> + arm_rebuild_hflags(&target_cpu->env); >> + >> /* Start the new CPU at the requested address */ >> cpu_set_pc(target_cpu_state, info->entry); >> >> -- >> 2.17.1 >> > > > -- > Niek Linnenbank
On 12/17/19 6:12 AM, Peter Maydell wrote: > Cc'ing Richard : this is one for you I think... (surely we > need to rebuild the hflags from scratch when we power up > a CPU anyway?) We do compute hflags from scratch in reset. It has also turned out that there were a few board models that poked at the contents of the cpu and needed special help. Some of that I would imagine would be fixed properly with the multi-phase reset patches, where we could rebuild hflags when *leaving* reset. In arm_set_cpu_on_async_work, we start by resetting the cpu and then start poking at the contents of some system registers. So, yes, we do need to rebuild after doing that. Also, I'm not sure how this function should fit into the multi-phase reset future. So: >> On Tue, Dec 17, 2019 at 12:36 AM Niek Linnenbank <nieklinnenbank@gmail.com> wrote: >>> >>> After setting CP15 bits in arm_set_cpu_on() the cached hflags must >>> be rebuild to reflect the changed processor state. Without rebuilding, >>> the cached hflags would be inconsistent until the next call to >>> arm_rebuild_hflags(). When QEMU is compiled with debugging enabled >>> (--enable-debug), this problem is captured shortly after the first >>> call to arm_set_cpu_on() for CPUs running in ARM 32-bit non-secure mode: >>> >>> qemu-system-arm: target/arm/helper.c:11359: cpu_get_tb_cpu_state: >>> Assertion `flags == rebuild_hflags_internal(env)' failed. >>> Aborted (core dumped) >>> >>> Fixes: 0c7f8c43daf65 >>> Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com> >>> --- >>> target/arm/arm-powerctl.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c >>> index b064513d44..b75f813b40 100644 >>> --- a/target/arm/arm-powerctl.c >>> +++ b/target/arm/arm-powerctl.c >>> @@ -127,6 +127,9 @@ static void arm_set_cpu_on_async_work(CPUState *target_cpu_state, >>> target_cpu->env.regs[0] = info->context_id; >>> } >>> >>> + /* CP15 update requires rebuilding hflags */ >>> + arm_rebuild_hflags(&target_cpu->env); >>> + >>> /* Start the new CPU at the requested address */ >>> cpu_set_pc(target_cpu_state, info->entry); >>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Tue, 17 Dec 2019 at 16:41, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 12/17/19 6:12 AM, Peter Maydell wrote: > > Cc'ing Richard : this is one for you I think... (surely we > > need to rebuild the hflags from scratch when we power up > > a CPU anyway?) > > We do compute hflags from scratch in reset. > > It has also turned out that there were a few board models that poked at the > contents of the cpu and needed special help. Some of that I would imagine > would be fixed properly with the multi-phase reset patches, where we could > rebuild hflags when *leaving* reset. > > In arm_set_cpu_on_async_work, we start by resetting the cpu and then start > poking at the contents of some system registers. So, yes, we do need to > rebuild after doing that. Also, I'm not sure how this function should fit into > the multi-phase reset future. > > So: > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Thanks; I've applied this patch to target-arm.next. -- PMM
Hello Richard, On Tue, Dec 17, 2019 at 5:41 PM Richard Henderson < richard.henderson@linaro.org> wrote: > On 12/17/19 6:12 AM, Peter Maydell wrote: > > Cc'ing Richard : this is one for you I think... (surely we > > need to rebuild the hflags from scratch when we power up > > a CPU anyway?) > > We do compute hflags from scratch in reset. > > It has also turned out that there were a few board models that poked at the > contents of the cpu and needed special help. Some of that I would imagine > would be fixed properly with the multi-phase reset patches, where we could > rebuild hflags when *leaving* reset. > > In arm_set_cpu_on_async_work, we start by resetting the cpu and then start > poking at the contents of some system registers. So, yes, we do need to > rebuild after doing that. Also, I'm not sure how this function should fit > into > the multi-phase reset future. > Great, thanks a lot for confirming and clarifying this! You mention the multi-phase reset feature, is that going to replace the arm_set_cpu_on() functionality? Currently I chose to use this function for implementing the CPU configuration module in the Allwinner H3 Soc. U-Boot needs the CPU configuration module to provide PSCI which Linux uses to bring up the secondary cores. And basically the CPU configuration module needs something to let the secondary CPUs power on, reset and start executing at some address. Would you suggest to keep using arm_set_cpu_on() for this, or should I instead use a different function? Regards, Niek > > So: > > >> On Tue, Dec 17, 2019 at 12:36 AM Niek Linnenbank < > nieklinnenbank@gmail.com> wrote: > >>> > >>> After setting CP15 bits in arm_set_cpu_on() the cached hflags must > >>> be rebuild to reflect the changed processor state. Without rebuilding, > >>> the cached hflags would be inconsistent until the next call to > >>> arm_rebuild_hflags(). When QEMU is compiled with debugging enabled > >>> (--enable-debug), this problem is captured shortly after the first > >>> call to arm_set_cpu_on() for CPUs running in ARM 32-bit non-secure > mode: > >>> > >>> qemu-system-arm: target/arm/helper.c:11359: cpu_get_tb_cpu_state: > >>> Assertion `flags == rebuild_hflags_internal(env)' failed. > >>> Aborted (core dumped) > >>> > >>> Fixes: 0c7f8c43daf65 > >>> Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com> > >>> --- > >>> target/arm/arm-powerctl.c | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c > >>> index b064513d44..b75f813b40 100644 > >>> --- a/target/arm/arm-powerctl.c > >>> +++ b/target/arm/arm-powerctl.c > >>> @@ -127,6 +127,9 @@ static void arm_set_cpu_on_async_work(CPUState > *target_cpu_state, > >>> target_cpu->env.regs[0] = info->context_id; > >>> } > >>> > >>> + /* CP15 update requires rebuilding hflags */ > >>> + arm_rebuild_hflags(&target_cpu->env); > >>> + > >>> /* Start the new CPU at the requested address */ > >>> cpu_set_pc(target_cpu_state, info->entry); > >>> > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> > > > r~ >
On 12/18/19 11:01 AM, Niek Linnenbank wrote: > Hello Richard, > > On Tue, Dec 17, 2019 at 5:41 PM Richard Henderson <richard.henderson@linaro.org > <mailto:richard.henderson@linaro.org>> wrote: > > On 12/17/19 6:12 AM, Peter Maydell wrote: > > Cc'ing Richard : this is one for you I think... (surely we > > need to rebuild the hflags from scratch when we power up > > a CPU anyway?) > > We do compute hflags from scratch in reset. > > It has also turned out that there were a few board models that poked at the > contents of the cpu and needed special help. Some of that I would imagine > would be fixed properly with the multi-phase reset patches, where we could > rebuild hflags when *leaving* reset. > > In arm_set_cpu_on_async_work, we start by resetting the cpu and then start > poking at the contents of some system registers. So, yes, we do need to > rebuild after doing that. Also, I'm not sure how this function should fit into > the multi-phase reset future. > > > Great, thanks a lot for confirming and clarifying this! > You mention the multi-phase reset feature, is that going to replace the > arm_set_cpu_on() functionality? I don't think so, but I'm not sure. As I said above, I don't immediately see how arm_set_cpu_on() will integrate. In any case, multi-phase reset is still pending, though I believe it is high on Peter's priority queue for the 5.0 development cycle. r~
diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c index b064513d44..b75f813b40 100644 --- a/target/arm/arm-powerctl.c +++ b/target/arm/arm-powerctl.c @@ -127,6 +127,9 @@ static void arm_set_cpu_on_async_work(CPUState *target_cpu_state, target_cpu->env.regs[0] = info->context_id; } + /* CP15 update requires rebuilding hflags */ + arm_rebuild_hflags(&target_cpu->env); + /* Start the new CPU at the requested address */ cpu_set_pc(target_cpu_state, info->entry);
After setting CP15 bits in arm_set_cpu_on() the cached hflags must be rebuild to reflect the changed processor state. Without rebuilding, the cached hflags would be inconsistent until the next call to arm_rebuild_hflags(). When QEMU is compiled with debugging enabled (--enable-debug), this problem is captured shortly after the first call to arm_set_cpu_on() for CPUs running in ARM 32-bit non-secure mode: qemu-system-arm: target/arm/helper.c:11359: cpu_get_tb_cpu_state: Assertion `flags == rebuild_hflags_internal(env)' failed. Aborted (core dumped) Fixes: 0c7f8c43daf65 Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com> --- target/arm/arm-powerctl.c | 3 +++ 1 file changed, 3 insertions(+)