Re: [PATCH][boot-wrapper] aarch64: Enable ECV to allow access to CNTPOFF_EL2

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Aug 11, 2021 at 03:45:21PM +0100, Marc Zyngier wrote:
> On Wed, 11 Aug 2021 14:55:58 +0100,
> Mark Rutland <mark.rutland@arm.com> wrote:
> > 
> > On Wed, Aug 11, 2021 at 10:22:26AM +0100, Marc Zyngier wrote:
> > > If the implemnentation supports ID_AA64MMFR0_EL1.ECV==2,
> > > set SCR_EL3.ECVEn to allow EL2 access to CNTPOFF_EL2.
> > 
> > I was about to ask if that was a typo (why is this in an MMFR?), but
> > that is what the ARM ARM says!
> 
> I was just as surprised. PFR would have made a lot more sense, but I
> guess someone wanted to use these last four bits as quickly as
> possible... ;-)
> 
> > 
> > > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > > ---
> > >  arch/aarch64/boot.S | 11 +++++++++--
> > >  1 file changed, 9 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
> > > index 7f208b5..f0aa3cb 100644
> > > --- a/arch/aarch64/boot.S
> > > +++ b/arch/aarch64/boot.S
> > > @@ -54,10 +54,17 @@ ASM_FUNC(_start)
> > >  1:
> > >  	/* Enable FGT if present */
> > >  	mrs	x1, id_aa64mmfr0_el1
> > > -	ubfx	x1, x1, #56, #4
> > > -	cbz	x1, 1f
> > > +	ubfx	x2, x1, #56, #4
> > > +	cbz	x2, 1f
> > >  
> > >  	orr	x0, x0, #(1 << 27)		// FGT enable
> > > +1:
> > > +	/* Enable ECV2 if present (allows CNTPOFF_EL2) */
> > > +	ubfx	x2, x1, #60, #4
> > > +	cmp	x2, #2
> > > +	bne	1f
> > 
> > We need to check ID_AA64MMFR0_EL1.ECV >= 2 (to handle any futrue
> > variants) so the conditional branch needs to be something like `b.lt`.
> 
> Good point. I'm surprised ECV3 hasn't hit yet! :D
> 
> > > +
> > > +	orr	x0, x0, #(1 << 28)		// ECV enable
> > 
> > Aside from the above, this looks good to me, and I plan to apply this
> > with the below tweaks (updated patch below):
> > 
> > * Use b.lt, as above
> > 
> > * To keep each check self-contained, the ECV check will read
> >   id_aa64mmfr0_el1 itself. That removes the need to keep x1 around, and
> >   so we can leave the FGT check as-is.
> > 
> >   As part of some cleanup, I'm planning to move this to C, where feature
> >   checks will read ID fields via a helepr that implicitly reads the
> >   register, like:
> > 
> >   if (read_reg_field(ID_AA64MMFR0_EL1, ECV) >= 2)
> >   	scr |= SCR_EL3_ECVEN;
> 
> If you are actively rewriting this, maybe don't bother with the patch
> and just add this as part of your rewrite.

There's a bunch of preparatory stuff to fix first, so I've applied this
patch (with those alterations) for now.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel