[Qemu-devel] [PATCH for-4.1] target/arm: Avoid bogus NSACR traps on M-profile without Security Extension

[Qemu-devel] [PATCH for-4.1] target/arm: Avoid bogus NSACR traps on M-profile without Security Extension

[Peter Maydell]

In Arm v8.0 M-profile CPUs without the Security Extension and also in
v7M CPUs, there is no NSACR register. However, the code we have to handle
the FPU does not always check whether the ARM_FEATURE_M_SECURITY bit
is set before testing whether env->v7m.nsacr permits access to the
FPU. This means that for a CPU with an FPU but without the Security
Extension we would always take a bogus fault when trying to stack
the FPU registers on an exception entry.

We could fix this by adding extra feature bit checks for all uses,
but it is simpler to just make the internal value of nsacr 0x3ff
("all non-secure accesses allowed"), since this is not guest
visible when the Security Extension is not present. This allows
us to continue to follow the Arm ARM pseudocode which takes a
similar approach. (In particular, in the v8.1 Arm ARM the register
is documented as reading as 0xcff in this configuration.)

Fixes: https://bugs.launchpad.net/qemu/+bug/1838475
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I've marked this as for-4.1 because support for M-profile FPU
is new in this release and this is a pretty bad bug for it.
It probably doesn't qualify as so bad we need an rc4 but I
think we need an rc4 anyway and it probably does merit putting
in at that point.

 target/arm/cpu.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 9eb40ff755f..ec2ab95dbeb 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -266,6 +266,14 @@ static void arm_cpu_reset(CPUState *s)
              * on ARM_FEATURE_V8 (we don't let the guest see the bit).
              */
             env->v7m.aircr = R_V7M_AIRCR_BFHFNMINS_MASK;
+            /*
+             * Set NSACR to indicate "NS access permitted to everything";
+             * this avoids having to have all the tests of it being
+             * conditional on ARM_FEATURE_M_SECURITY. Note also that from
+             * v8.1M the guest-visible value of NSACR in a CPU without the
+             * Security Extension is 0xcff.
+             */
+            env->v7m.nsacr = 0xcff;
         }
 
         /* In v7M the reset value of this bit is IMPDEF, but ARM recommends
-- 
2.20.1

Re: [Qemu-devel] [PATCH for-4.1] target/arm: Avoid bogus NSACR traps on M-profile without Security Extension

[Damien Hedde]

On 8/1/19 12:57 PM, Peter Maydell wrote:
> In Arm v8.0 M-profile CPUs without the Security Extension and also in
> v7M CPUs, there is no NSACR register. However, the code we have to handle
> the FPU does not always check whether the ARM_FEATURE_M_SECURITY bit
> is set before testing whether env->v7m.nsacr permits access to the
> FPU. This means that for a CPU with an FPU but without the Security
> Extension we would always take a bogus fault when trying to stack
> the FPU registers on an exception entry.
> 
> We could fix this by adding extra feature bit checks for all uses,
> but it is simpler to just make the internal value of nsacr 0x3ff

s/0x3ff/0xcff/ I think, given you put 0xcff after and in the code

> ("all non-secure accesses allowed"), since this is not guest
> visible when the Security Extension is not present. This allows
> us to continue to follow the Arm ARM pseudocode which takes a
> similar approach. (In particular, in the v8.1 Arm ARM the register
> is documented as reading as 0xcff in this configuration.)
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1838475
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I've marked this as for-4.1 because support for M-profile FPU
> is new in this release and this is a pretty bad bug for it.
> It probably doesn't qualify as so bad we need an rc4 but I
> think we need an rc4 anyway and it probably does merit putting
> in at that point.
> 
>  target/arm/cpu.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 9eb40ff755f..ec2ab95dbeb 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -266,6 +266,14 @@ static void arm_cpu_reset(CPUState *s)
>               * on ARM_FEATURE_V8 (we don't let the guest see the bit).
>               */
>              env->v7m.aircr = R_V7M_AIRCR_BFHFNMINS_MASK;
> +            /*
> +             * Set NSACR to indicate "NS access permitted to everything";
> +             * this avoids having to have all the tests of it being
> +             * conditional on ARM_FEATURE_M_SECURITY. Note also that from
> +             * v8.1M the guest-visible value of NSACR in a CPU without the
> +             * Security Extension is 0xcff.
> +             */
> +            env->v7m.nsacr = 0xcff;
>          }
>  
>          /* In v7M the reset value of this bit is IMPDEF, but ARM recommends
> 

Re: [Qemu-devel] [PATCH for-4.1] target/arm: Avoid bogus NSACR traps on M-profile without Security Extension

[Peter Maydell]

On Thu, 1 Aug 2019 at 15:20, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
> On 8/1/19 12:57 PM, Peter Maydell wrote:
> > In Arm v8.0 M-profile CPUs without the Security Extension and also in
> > v7M CPUs, there is no NSACR register. However, the code we have to handle
> > the FPU does not always check whether the ARM_FEATURE_M_SECURITY bit
> > is set before testing whether env->v7m.nsacr permits access to the
> > FPU. This means that for a CPU with an FPU but without the Security
> > Extension we would always take a bogus fault when trying to stack
> > the FPU registers on an exception entry.
> >
> > We could fix this by adding extra feature bit checks for all uses,
> > but it is simpler to just make the internal value of nsacr 0x3ff
>
> s/0x3ff/0xcff/ I think, given you put 0xcff after and in the code

Yes, 0xcff is correct and the commit message is wrong. (Bits 8 and 9
of the NSACR are RES0 in all situations.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-4.1] target/arm: Avoid bogus NSACR traps on M-profile without Security Extension

[Damien Hedde]

On 8/1/19 4:38 PM, Peter Maydell wrote:
> On Thu, 1 Aug 2019 at 15:20, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>>
>> On 8/1/19 12:57 PM, Peter Maydell wrote:
>>> In Arm v8.0 M-profile CPUs without the Security Extension and also in
>>> v7M CPUs, there is no NSACR register. However, the code we have to handle
>>> the FPU does not always check whether the ARM_FEATURE_M_SECURITY bit
>>> is set before testing whether env->v7m.nsacr permits access to the
>>> FPU. This means that for a CPU with an FPU but without the Security
>>> Extension we would always take a bogus fault when trying to stack
>>> the FPU registers on an exception entry.
>>>
>>> We could fix this by adding extra feature bit checks for all uses,
>>> but it is simpler to just make the internal value of nsacr 0x3ff
>>
>> s/0x3ff/0xcff/ I think, given you put 0xcff after and in the code
> 
> Yes, 0xcff is correct and the commit message is wrong. (Bits 8 and 9
> of the NSACR are RES0 in all situations.)
> 

Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>

Re: [Qemu-devel] [PATCH for-4.1] target/arm: Avoid bogus NSACR traps on M-profile without Security Extension

[Peter Maydell]

On Fri, 2 Aug 2019 at 08:51, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
> On 8/1/19 4:38 PM, Peter Maydell wrote:
> > On Thu, 1 Aug 2019 at 15:20, Damien Hedde <damien.hedde@greensocs.com> wrote:
> >>
> >>
> >> On 8/1/19 12:57 PM, Peter Maydell wrote:
> >>> In Arm v8.0 M-profile CPUs without the Security Extension and also in
> >>> v7M CPUs, there is no NSACR register. However, the code we have to handle
> >>> the FPU does not always check whether the ARM_FEATURE_M_SECURITY bit
> >>> is set before testing whether env->v7m.nsacr permits access to the
> >>> FPU. This means that for a CPU with an FPU but without the Security
> >>> Extension we would always take a bogus fault when trying to stack
> >>> the FPU registers on an exception entry.
> >>>
> >>> We could fix this by adding extra feature bit checks for all uses,
> >>> but it is simpler to just make the internal value of nsacr 0x3ff
> >>
> >> s/0x3ff/0xcff/ I think, given you put 0xcff after and in the code
> >
> > Yes, 0xcff is correct and the commit message is wrong. (Bits 8 and 9
> > of the NSACR are RES0 in all situations.)
> >
>
> Reviewed-by: Damien Hedde <damien.hedde@greensocs.com>

Thanks; since we need an rc4 anyway I have pushed this to master,
with the commit message typo fixed.

-- PMM