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

[Marc Zyngier <maz@kernel.org>]

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.