Re: [PATCH] sunxi: psci: remove redundant initialization from psci_arch_init

Re: [PATCH] sunxi: psci: remove redundant initialization from psci_arch_init

[Andre Przywara]

On Wed, 31 May 2023 14:15:20 -0600
Sam Edwards <cfsworks@gmail.com> wrote:

Hi,

(CC:ing Marc and Chen-Yu as the original authors)

sorry for the delay, found that mouldering in my Drafts folder.

> The nonsec code overrides/handles these:
> - Setting PMR to 0xFF happens earlier, in _nonsec_init, so this is
>   redundant.
> - The NS bit is not yet set: it gets set later in _secure_monitor.
>   Trying to clear it here is pointless and misleading.

So superficially this looks alright, but I am still somewhat reluctant
to apply this, see below ...

> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> ---
>  arch/arm/cpu/armv7/sunxi/psci.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> index e1d3638b5c..f866025c37 100644
> --- a/arch/arm/cpu/armv7/sunxi/psci.c
> +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> @@ -300,14 +300,10 @@ void __secure psci_arch_init(void)
>  	/* Set SGI15 priority to 0 */
>  	writeb(0, GICD_BASE + GICD_IPRIORITYRn + 15);
>  
> -	/* Be cool with non-secure */
> -	writel(0xff, GICC_BASE + GICC_PMR);
> -
>  	/* Switch FIQEn on */
>  	setbits_le32(GICC_BASE + GICC_CTLR, BIT(3));
>  
>  	reg = cp15_read_scr();
>  	reg |= BIT(2);  /* Enable FIQ in monitor mode */
> -	reg &= ~BIT(0); /* Secure mode */

I am scratching my head on this one: I see it deliberately being done in
the original patch by Marc[1], and wonder why. If I read the code and
the architecture correctly, this routine is called only(?) from the SMC
handler, which means we are in monitor mode, that only exists in secure
state. So the NS bit is irrelevant until we switch to another mode. And
we indeed set the bit later, before dropping back to non-secure. So I
agree that clearing the bit is pointless. Was it put in to be more
robust, for other potential callers of this function, for instance in
the boot path?

Does anyone remember?

Cheers,
Andre

[1]
https://source.denx.de/u-boot/u-boot/-/commit/d5db7024aafc5ea603f3a34f83bb29a1eaa3cbe7#f377950449a5dba076796d9e48fbd21f5d6ac545_0_65

>  	cp15_write_scr(reg);
>  }

Re: [PATCH] sunxi: psci: remove redundant initialization from psci_arch_init

[Marc Zyngier]

On Mon, 14 Aug 2023 21:39:10 +0100,
Andre Przywara <andre.przywara@arm.com> wrote:
> 
> On Wed, 31 May 2023 14:15:20 -0600
> Sam Edwards <cfsworks@gmail.com> wrote:
> 
> Hi,
> 
> (CC:ing Marc and Chen-Yu as the original authors)
> 
> sorry for the delay, found that mouldering in my Drafts folder.
> 
> > The nonsec code overrides/handles these:
> > - Setting PMR to 0xFF happens earlier, in _nonsec_init, so this is
> >   redundant.
> > - The NS bit is not yet set: it gets set later in _secure_monitor.
> >   Trying to clear it here is pointless and misleading.
> 
> So superficially this looks alright, but I am still somewhat reluctant
> to apply this, see below ...
> 
> > Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> > ---
> >  arch/arm/cpu/armv7/sunxi/psci.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> > index e1d3638b5c..f866025c37 100644
> > --- a/arch/arm/cpu/armv7/sunxi/psci.c
> > +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> > @@ -300,14 +300,10 @@ void __secure psci_arch_init(void)
> >  	/* Set SGI15 priority to 0 */
> >  	writeb(0, GICD_BASE + GICD_IPRIORITYRn + 15);
> >  
> > -	/* Be cool with non-secure */
> > -	writel(0xff, GICC_BASE + GICC_PMR);
> > -
> >  	/* Switch FIQEn on */
> >  	setbits_le32(GICC_BASE + GICC_CTLR, BIT(3));
> >  
> >  	reg = cp15_read_scr();
> >  	reg |= BIT(2);  /* Enable FIQ in monitor mode */
> > -	reg &= ~BIT(0); /* Secure mode */
> 
> I am scratching my head on this one: I see it deliberately being done in
> the original patch by Marc[1], and wonder why. If I read the code and
> the architecture correctly, this routine is called only(?) from the SMC
> handler, which means we are in monitor mode, that only exists in secure
> state. So the NS bit is irrelevant until we switch to another mode. And
> we indeed set the bit later, before dropping back to non-secure. So I
> agree that clearing the bit is pointless. Was it put in to be more
> robust, for other potential callers of this function, for instance in
> the boot path?
> 
> Does anyone remember?

I don't remember much about this, but setting PMR to include the NS
priorities (PMR[7]) is definitely significant when SCR.NS==0.
Otherwise, NS cannot write to PMR, which basically kills NS any use.

Now, and without the full context, it is hard to figure out what is
going on. But a lot of that crap was written with mandate to be able
to support the stupid old AW BSP which ran in secure mode, so there
were a number of convoluted paths to get there.

If this nonsense can now be dropped, I'm sure the whole thing can be
simplified. But someone who still care about 32bit platforms (i.e. not
me) should have a look.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] sunxi: psci: remove redundant initialization from psci_arch_init

[Chen-Yu Tsai]

On Tue, Aug 15, 2023 at 4:40 AM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Wed, 31 May 2023 14:15:20 -0600
> Sam Edwards <cfsworks@gmail.com> wrote:
>
> Hi,
>
> (CC:ing Marc and Chen-Yu as the original authors)
>
> sorry for the delay, found that mouldering in my Drafts folder.
>
> > The nonsec code overrides/handles these:
> > - Setting PMR to 0xFF happens earlier, in _nonsec_init, so this is
> >   redundant.
> > - The NS bit is not yet set: it gets set later in _secure_monitor.
> >   Trying to clear it here is pointless and misleading.
>
> So superficially this looks alright, but I am still somewhat reluctant
> to apply this, see below ...
>
> > Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> > ---
> >  arch/arm/cpu/armv7/sunxi/psci.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c
> > index e1d3638b5c..f866025c37 100644
> > --- a/arch/arm/cpu/armv7/sunxi/psci.c
> > +++ b/arch/arm/cpu/armv7/sunxi/psci.c
> > @@ -300,14 +300,10 @@ void __secure psci_arch_init(void)
> >       /* Set SGI15 priority to 0 */
> >       writeb(0, GICD_BASE + GICD_IPRIORITYRn + 15);
> >
> > -     /* Be cool with non-secure */
> > -     writel(0xff, GICC_BASE + GICC_PMR);
> > -
> >       /* Switch FIQEn on */
> >       setbits_le32(GICC_BASE + GICC_CTLR, BIT(3));
> >
> >       reg = cp15_read_scr();
> >       reg |= BIT(2);  /* Enable FIQ in monitor mode */
> > -     reg &= ~BIT(0); /* Secure mode */
>
> I am scratching my head on this one: I see it deliberately being done in
> the original patch by Marc[1], and wonder why. If I read the code and
> the architecture correctly, this routine is called only(?) from the SMC
> handler, which means we are in monitor mode, that only exists in secure
> state. So the NS bit is irrelevant until we switch to another mode. And
> we indeed set the bit later, before dropping back to non-secure. So I
> agree that clearing the bit is pointless. Was it put in to be more
> robust, for other potential callers of this function, for instance in
> the boot path?

IIRC the GIC manual says that the secure bit is set or cleared to select
which bank of registers is accessed.

And I suppose it is here to be more robust.

ChenYu

> Does anyone remember?
>
> Cheers,
> Andre
>
> [1]
> https://source.denx.de/u-boot/u-boot/-/commit/d5db7024aafc5ea603f3a34f83bb29a1eaa3cbe7#f377950449a5dba076796d9e48fbd21f5d6ac545_0_65
>
> >       cp15_write_scr(reg);
> >  }
>

Re: [PATCH] sunxi: psci: remove redundant initialization from psci_arch_init

[Sam Edwards]

On 8/25/23 00:20, Chen-Yu Tsai wrote:

Hi Chen-Yu,

> IIRC the GIC manual says that the secure bit is set or cleared to select
> which bank of registers is accessed.

Which secure bit are we talking about here? Do we mean the *configured* 
secure bit (SCR.NS, what the code is attempting to clear) or the 
*effective* secure bit (AWPROT[1], et al)? The distinction is important 
in monitor mode (where this function runs) since there (and only there) 
the CPU core ignores the configured setting and runs in the secure world 
unconditionally.

I'm guessing it's most likely the latter since the former isn't exposed 
outside of the CPU core, unless the GIC has some special signal going to 
it...

> And I suppose it is here to be more robust.

...but if it is the former (i.e. SCR.NS is significant in this function) 
the code should be retained, but moved *before* the GIC register 
accesses, and the old value of SCR.NS should be restored *after*.

Either way: I don't think this line should be kept in its current form, 
because it's written in a way that strongly suggests that we want to run 
in secure mode after exiting monitor mode, which is flatly not the case.

Happy Friday all,
Sam

Re: [PATCH] sunxi: psci: remove redundant initialization from psci_arch_init

[Marc Zyngier]

On Fri, 25 Aug 2023 19:05:32 +0100,
Sam Edwards <cfsworks@gmail.com> wrote:
> 
> On 8/25/23 00:20, Chen-Yu Tsai wrote:
> 
> Hi Chen-Yu,
> 
> > IIRC the GIC manual says that the secure bit is set or cleared to select
> > which bank of registers is accessed.
> 
> Which secure bit are we talking about here? Do we mean the
> *configured* secure bit (SCR.NS, what the code is attempting to clear)
> or the *effective* secure bit (AWPROT[1], et al)? The distinction is
> important in monitor mode (where this function runs) since there (and
> only there) the CPU core ignores the configured setting and runs in
> the secure world unconditionally.
> 
> I'm guessing it's most likely the latter since the former isn't
> exposed outside of the CPU core, unless the GIC has some special
> signal going to it...

The GIC definitely has the NS bit routed to it. Otherwise, the secure
configuration would just be an utter joke. Just try it.

> 
> > And I suppose it is here to be more robust.
> 
> ...but if it is the former (i.e. SCR.NS is significant in this
> function) the code should be retained, but moved *before* the GIC
> register accesses, and the old value of SCR.NS should be restored
> *after*.
> 
> Either way: I don't think this line should be kept in its current
> form, because it's written in a way that strongly suggests that we
> want to run in secure mode after exiting monitor mode, which is flatly
> not the case.

Well, history is unfortunately against you on that front. Running on
the secure side definitely was a requirement when this code was
initially written, as the AW BSP *required* to run on the secure side.

If that requirement is no more, great. But I don't think you can
decide that unilaterally.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH] sunxi: psci: remove redundant initialization from psci_arch_init

[Sam Edwards]

On 8/26/23 04:22, Marc Zyngier wrote:

Hi Marc!

> The GIC definitely has the NS bit routed to it. Otherwise, the secure
> configuration would just be an utter joke. Just try it.

Thank you for your response. I'd like to revisit my prior point about 
the distinction between the NS bit and AxPROT[1] bits in the context of 
monitor mode: in monitor mode, the NS bit does not determine the 
security state of the CPU core (monitor mode is always secure). But even 
then, the NS bit is still significant for other purposes, such as to 
bank accesses to certain CP15 registers -- and if I understand Chen-Yu 
correctly, some GIC registers also. That would require a special NS bit 
signal routed to the GIC so that it can distinguish between "secure, 
NS=0" and "secure, NS=1" accesses, which is why I asked if such a thing 
exists.

I understand that the GIC is designed to be aware of the security state 
(using the existing AxPROT[1] signals) so that it can protect the 
sensitive registers. And unless I misunderstand, this seems to be the 
point that you made here (my interpretation -- correct me if I'm wrong 
-- is that you are using "NS bit" as a metonym for "security state"). 
However I must clarify that my question was to seek further information 
from Chen-Yu about the possibility that NS is significant when accessing 
the GIC, even in monitor mode. Alternatively, his point might be merely 
highlighting that the GIC permits different types of access depending on 
the CPU's security state, which aligns with the viewpoint you've reiterated.

I apologize if my previous message didn't convey this context clearly 
enough. My goal was to unravel this nuanced aspect of the NS bit when in 
monitor mode, and to determine if NS needs to be getting set/cleared 
during GIC setup to maneuver around the banking, or if the value of the 
NS bit when in psci_arch_init() is truly of no consequence due to 
monitor mode.

> Well, history is unfortunately against you on that front. Running on
> the secure side definitely was a requirement when this code was
> initially written, as the AW BSP *required* to run on the secure side.
> 
> If that requirement is no more, great. But I don't think you can
> decide that unilaterally.

I have no idea when/if this requirement was changed. It might have never 
happened "formally": perhaps at some point, the SCR.NS=1 code got added 
after the call to psci_arch_init(), breaking (that version of) the AW 
BSP, and nobody ever complained or changed it back... so it stayed.

But we're starting to digress from what _this_ patch does. The intent 
here is only to remove two lines of code that (we're discussing to 
confirm) have no effect. I'm not touching the code that *actually* 
determines the world into which monitor mode exits.

Cheers,
Sam

Re: [PATCH] sunxi: psci: remove redundant initialization from psci_arch_init

[Chen-Yu Tsai]

On Tue, Aug 29, 2023 at 5:49 AM Sam Edwards <cfsworks@gmail.com> wrote:
>
> On 8/26/23 04:22, Marc Zyngier wrote:
>
> Hi Marc!
>
> > The GIC definitely has the NS bit routed to it. Otherwise, the secure
> > configuration would just be an utter joke. Just try it.
>
> Thank you for your response. I'd like to revisit my prior point about
> the distinction between the NS bit and AxPROT[1] bits in the context of
> monitor mode: in monitor mode, the NS bit does not determine the
> security state of the CPU core (monitor mode is always secure). But even
> then, the NS bit is still significant for other purposes, such as to
> bank accesses to certain CP15 registers -- and if I understand Chen-Yu
> correctly, some GIC registers also. That would require a special NS bit
> signal routed to the GIC so that it can distinguish between "secure,
> NS=0" and "secure, NS=1" accesses, which is why I asked if such a thing
> exists.
>
> I understand that the GIC is designed to be aware of the security state
> (using the existing AxPROT[1] signals) so that it can protect the
> sensitive registers. And unless I misunderstand, this seems to be the
> point that you made here (my interpretation -- correct me if I'm wrong
> -- is that you are using "NS bit" as a metonym for "security state").
> However I must clarify that my question was to seek further information
> from Chen-Yu about the possibility that NS is significant when accessing
> the GIC, even in monitor mode. Alternatively, his point might be merely
> highlighting that the GIC permits different types of access depending on
> the CPU's security state, which aligns with the viewpoint you've reiterated.
>
> I apologize if my previous message didn't convey this context clearly
> enough. My goal was to unravel this nuanced aspect of the NS bit when in
> monitor mode, and to determine if NS needs to be getting set/cleared
> during GIC setup to maneuver around the banking, or if the value of the
> NS bit when in psci_arch_init() is truly of no consequence due to
> monitor mode.

Sorry for my previous misleading comment. The banked registers refer to
the CP15 registers. From ARM's documentation [1]:

When the processor is in Monitor mode, the processor is in Secure state
regardless of the value of the SCR.NS bit. In Monitor mode, the SCR.NS
bit determines whether the Secure Banked CP15 registers or Non-secure
Banked CP15 registers are read or written using MRC or MCR instructions.

[1] https://developer.arm.com/documentation/ddi0406/b/System-Level-Architecture/Virtual-Memory-System-Architecture--VMSA-/CP15-registers-for-a-VMSA-implementation/Effect-of-the-Security-Extensions-on-the-CP15-registers?lang=en#Cbbdffad

> > Well, history is unfortunately against you on that front. Running on
> > the secure side definitely was a requirement when this code was
> > initially written, as the AW BSP *required* to run on the secure side.
> >
> > If that requirement is no more, great. But I don't think you can
> > decide that unilaterally.
>
> I have no idea when/if this requirement was changed. It might have never
> happened "formally": perhaps at some point, the SCR.NS=1 code got added
> after the call to psci_arch_init(), breaking (that version of) the AW
> BSP, and nobody ever complained or changed it back... so it stayed.

There was never any BSP code for PSCI before this. This code was written
by Marc in ARM assembly. I merely translated most of it to C code.
The snippet to clear SCR.NS is there in the first commit,

    d5db7024aafc sunxi: HYP/non-sec: add sun7i PSCI backend

ChenYu

> But we're starting to digress from what _this_ patch does. The intent
> here is only to remove two lines of code that (we're discussing to
> confirm) have no effect. I'm not touching the code that *actually*
> determines the world into which monitor mode exits.
>
> Cheers,
> Sam

Re: [PATCH] sunxi: psci: remove redundant initialization from psci_arch_init

[Chen-Yu Tsai]

(resent from kernel.org address)

On Tue, Aug 29, 2023 at 5:49 AM Sam Edwards <cfsworks@gmail.com> wrote:
>
> On 8/26/23 04:22, Marc Zyngier wrote:
>
> Hi Marc!
>
> > The GIC definitely has the NS bit routed to it. Otherwise, the secure
> > configuration would just be an utter joke. Just try it.
>
> Thank you for your response. I'd like to revisit my prior point about
> the distinction between the NS bit and AxPROT[1] bits in the context of
> monitor mode: in monitor mode, the NS bit does not determine the
> security state of the CPU core (monitor mode is always secure). But even
> then, the NS bit is still significant for other purposes, such as to
> bank accesses to certain CP15 registers -- and if I understand Chen-Yu
> correctly, some GIC registers also. That would require a special NS bit
> signal routed to the GIC so that it can distinguish between "secure,
> NS=0" and "secure, NS=1" accesses, which is why I asked if such a thing
> exists.
>
> I understand that the GIC is designed to be aware of the security state
> (using the existing AxPROT[1] signals) so that it can protect the
> sensitive registers. And unless I misunderstand, this seems to be the
> point that you made here (my interpretation -- correct me if I'm wrong
> -- is that you are using "NS bit" as a metonym for "security state").
> However I must clarify that my question was to seek further information
> from Chen-Yu about the possibility that NS is significant when accessing
> the GIC, even in monitor mode. Alternatively, his point might be merely
> highlighting that the GIC permits different types of access depending on
> the CPU's security state, which aligns with the viewpoint you've reiterated.
>
> I apologize if my previous message didn't convey this context clearly
> enough. My goal was to unravel this nuanced aspect of the NS bit when in
> monitor mode, and to determine if NS needs to be getting set/cleared
> during GIC setup to maneuver around the banking, or if the value of the
> NS bit when in psci_arch_init() is truly of no consequence due to
> monitor mode.

Sorry for my previous misleading comment. The banked registers refer to
the CP15 registers. From ARM's documentation [1]:

When the processor is in Monitor mode, the processor is in Secure state
regardless of the value of the SCR.NS bit. In Monitor mode, the SCR.NS
bit determines whether the Secure Banked CP15 registers or Non-secure
Banked CP15 registers are read or written using MRC or MCR instructions.

[1] https://developer.arm.com/documentation/ddi0406/b/System-Level-Architecture/Virtual-Memory-System-Architecture--VMSA-/CP15-registers-for-a-VMSA-implementation/Effect-of-the-Security-Extensions-on-the-CP15-registers?lang=en#Cbbdffad

> > Well, history is unfortunately against you on that front. Running on
> > the secure side definitely was a requirement when this code was
> > initially written, as the AW BSP *required* to run on the secure side.
> >
> > If that requirement is no more, great. But I don't think you can
> > decide that unilaterally.
>
> I have no idea when/if this requirement was changed. It might have never
> happened "formally": perhaps at some point, the SCR.NS=1 code got added
> after the call to psci_arch_init(), breaking (that version of) the AW
> BSP, and nobody ever complained or changed it back... so it stayed.

There was never any BSP code for PSCI before this. This code was written
by Marc in ARM assembly. I merely translated most of it to C code.
The snippet to clear SCR.NS is there in the first commit,

    d5db7024aafc sunxi: HYP/non-sec: add sun7i PSCI backend

ChenYu

> But we're starting to digress from what _this_ patch does. The intent
> here is only to remove two lines of code that (we're discussing to
> confirm) have no effect. I'm not touching the code that *actually*
> determines the world into which monitor mode exits.
>
> Cheers,
> Sam