[06/10] arm/arm-powerctl: set NSACR.{CP11, CP10} bits in arm_set_cpu_on()
diff mbox series

Message ID 20191202210947.3603-7-nieklinnenbank@gmail.com
State New
Headers show
Series
  • Add Allwinner H3 SoC and Orange Pi PC Machine
Related show

Commit Message

Niek Linnenbank Dec. 2, 2019, 9:09 p.m. UTC
This change ensures that the FPU can be accessed in Non-Secure mode
when the CPU core is reset using the arm_set_cpu_on() function call.
The NSACR.{CP11,CP10} bits define the exception level required to
access the FPU in Non-Secure mode. Without these bits set, the CPU
will give an undefined exception trap on the first FPU access for the
secondary cores under Linux.

Fixes: fc1120a7f5
Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
---
 target/arm/arm-powerctl.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Peter Maydell Dec. 6, 2019, 2:24 p.m. UTC | #1
On Mon, 2 Dec 2019 at 21:10, Niek Linnenbank <nieklinnenbank@gmail.com> wrote:
>
> This change ensures that the FPU can be accessed in Non-Secure mode
> when the CPU core is reset using the arm_set_cpu_on() function call.
> The NSACR.{CP11,CP10} bits define the exception level required to
> access the FPU in Non-Secure mode. Without these bits set, the CPU
> will give an undefined exception trap on the first FPU access for the
> secondary cores under Linux.
>
> Fixes: fc1120a7f5
> Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
> ---

Oops, another place where we failed to realise the ramifications
of making NSACR actually do something.

Since this is a bugfix I'm going to fish it out of this patchset
and apply it to target-arm.next with a cc: stable.

Thanks for the catch!

-- PMM
Niek Linnenbank Dec. 6, 2019, 8:01 p.m. UTC | #2
Hey Peter,

On Fri, Dec 6, 2019 at 3:25 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Mon, 2 Dec 2019 at 21:10, Niek Linnenbank <nieklinnenbank@gmail.com>
> wrote:
> >
> > This change ensures that the FPU can be accessed in Non-Secure mode
> > when the CPU core is reset using the arm_set_cpu_on() function call.
> > The NSACR.{CP11,CP10} bits define the exception level required to
> > access the FPU in Non-Secure mode. Without these bits set, the CPU
> > will give an undefined exception trap on the first FPU access for the
> > secondary cores under Linux.
> >
> > Fixes: fc1120a7f5
> > Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
> > ---
>
> Oops, another place where we failed to realise the ramifications
> of making NSACR actually do something.
>
> Since this is a bugfix I'm going to fish it out of this patchset
> and apply it to target-arm.next with a cc: stable.
>
> Thanks for the catch!
>

Sure, I'm happy to help. Note that I only tested this fix with
the Allwinner H3 SoC patches that I'm working on.

OK, I'll keep an eye out for it. Once it is solved in master, I'll remove
this patch from the patch series.

Regards,
Niek

>
> -- PMM
>
Niek Linnenbank Dec. 13, 2019, 8:52 p.m. UTC | #3
Hi Peter,

Philippe discovered that this patch triggers an hflags assertion error when
building QEMU
with debugging enabled (--enable-debug and --extra-cflags=-ggdb).

See this thread for details:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg665049.html

What I added to resolve that is to call arm_rebuild_hflags() after setting
CP10,CP11.
However I'm not sure of any other side effects because I just don't know
this area of the code very well.

Regards,
Niek

diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
index f77a950db6..cf2f3d69ab 100644
--- a/target/arm/arm-powerctl.c
+++ b/target/arm/arm-powerctl.c
@@ -104,6 +104,9 @@ static void arm_set_cpu_on_async_work(CPUState
*target_cpu_state,
         /* Processor is not in secure mode */
         target_cpu->env.cp15.scr_el3 |= SCR_NS;

+        /* Set NSACR.{CP11,CP10} so NS can access the FPU */
+        target_cpu->env.cp15.nsacr |= 3 << 10;
+
         /*
          * If QEMU is providing the equivalent of EL3 firmware, then we need
          * to make sure a CPU targeting EL2 comes out of reset with a
@@ -124,6 +127,9 @@ static void arm_set_cpu_on_async_work(CPUState
*target_cpu_state,
         target_cpu->env.regs[0] = info->context_id;
     }

+    /* Ensure hflags is rebuild */
+    arm_rebuild_hflags(&target_cpu->env);
+
     /* Start the new CPU at the requested address */
     cpu_set_pc(target_cpu_state, info->entry);





On Fri, Dec 6, 2019 at 9:01 PM Niek Linnenbank <nieklinnenbank@gmail.com>
wrote:

> Hey Peter,
>
> On Fri, Dec 6, 2019 at 3:25 PM Peter Maydell <peter.maydell@linaro.org>
> wrote:
>
>> On Mon, 2 Dec 2019 at 21:10, Niek Linnenbank <nieklinnenbank@gmail.com>
>> wrote:
>> >
>> > This change ensures that the FPU can be accessed in Non-Secure mode
>> > when the CPU core is reset using the arm_set_cpu_on() function call.
>> > The NSACR.{CP11,CP10} bits define the exception level required to
>> > access the FPU in Non-Secure mode. Without these bits set, the CPU
>> > will give an undefined exception trap on the first FPU access for the
>> > secondary cores under Linux.
>> >
>> > Fixes: fc1120a7f5
>> > Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
>> > ---
>>
>> Oops, another place where we failed to realise the ramifications
>> of making NSACR actually do something.
>>
>> Since this is a bugfix I'm going to fish it out of this patchset
>> and apply it to target-arm.next with a cc: stable.
>>
>> Thanks for the catch!
>>
>
> Sure, I'm happy to help. Note that I only tested this fix with
> the Allwinner H3 SoC patches that I'm working on.
>
> OK, I'll keep an eye out for it. Once it is solved in master, I'll remove
> this patch from the patch series.
>
> Regards,
> Niek
>
>>
>> -- PMM
>>
>
>
> --
> Niek Linnenbank
>
>

Patch
diff mbox series

diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
index f77a950db6..b064513d44 100644
--- a/target/arm/arm-powerctl.c
+++ b/target/arm/arm-powerctl.c
@@ -104,6 +104,9 @@  static void arm_set_cpu_on_async_work(CPUState *target_cpu_state,
         /* Processor is not in secure mode */
         target_cpu->env.cp15.scr_el3 |= SCR_NS;
 
+        /* Set NSACR.{CP11,CP10} so NS can access the FPU */
+        target_cpu->env.cp15.nsacr |= 3 << 10;
+
         /*
          * If QEMU is providing the equivalent of EL3 firmware, then we need
          * to make sure a CPU targeting EL2 comes out of reset with a