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

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

[Marc Zyngier]

If the implemnentation supports ID_AA64MMFR0_EL1.ECV==2,
set SCR_EL3.ECVEn to allow EL2 access to CNTPOFF_EL2.

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
+
+	orr	x0, x0, #(1 << 28)		// ECV enable
 1:
 	/* Enable MTE if present */
 	mrs	x10, id_aa64pfr1_el1
-- 
2.30.2


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

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

[Mark Rutland]

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!

> 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`.

> +
> +	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;

Thanks,
Mark.
---->8----
From bc6a9380eb3c5afc96735a54d455f2487df48700 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@kernel.org>
Date: Wed, 11 Aug 2021 10:22:26 +0100
Subject: [PATCH] aarch64: Enable ECV to allow access to CNTPOFF_EL2

If the implemnentation supports ID_AA64MMFR0_EL1.ECV==2,
set SCR_EL3.ECVEn to allow EL2 access to CNTPOFF_EL2.

Signed-off-by: Marc Zyngier <maz@kernel.org>
[Mark: read id_aa64mmfr0_el1 separately, s/bne/b.lt/]
Signed-off-by: Mark Rutland <mark.rutland@arm.com>
---
 arch/aarch64/boot.S | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
index 7f208b5..2215f7e 100644
--- a/arch/aarch64/boot.S
+++ b/arch/aarch64/boot.S
@@ -59,6 +59,14 @@ ASM_FUNC(_start)
 
 	orr	x0, x0, #(1 << 27)		// FGT enable
 1:
+	/* Enable ECV2 if present (allows CNTPOFF_EL2) */
+	mrs	x1, id_aa64mmfr0_el1
+	ubfx	x1, x1, #60, #4
+	cmp	x1, #2
+	b.lt	1f
+
+	orr	x0, x0, #(1 << 28)		// ECV enable
+1:
 	/* Enable MTE if present */
 	mrs	x10, id_aa64pfr1_el1
 	ubfx	x10, x10, #8, #4
-- 
2.11.0


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

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

[Marc Zyngier]

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.

> 
> Thanks,
> Mark.
> ---->8----
> From bc6a9380eb3c5afc96735a54d455f2487df48700 Mon Sep 17 00:00:00 2001
> From: Marc Zyngier <maz@kernel.org>
> Date: Wed, 11 Aug 2021 10:22:26 +0100
> Subject: [PATCH] aarch64: Enable ECV to allow access to CNTPOFF_EL2
> 
> If the implemnentation supports ID_AA64MMFR0_EL1.ECV==2,
> set SCR_EL3.ECVEn to allow EL2 access to CNTPOFF_EL2.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> [Mark: read id_aa64mmfr0_el1 separately, s/bne/b.lt/]
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> ---
>  arch/aarch64/boot.S | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/aarch64/boot.S b/arch/aarch64/boot.S
> index 7f208b5..2215f7e 100644
> --- a/arch/aarch64/boot.S
> +++ b/arch/aarch64/boot.S
> @@ -59,6 +59,14 @@ ASM_FUNC(_start)
>  
>  	orr	x0, x0, #(1 << 27)		// FGT enable
>  1:
> +	/* Enable ECV2 if present (allows CNTPOFF_EL2) */
> +	mrs	x1, id_aa64mmfr0_el1
> +	ubfx	x1, x1, #60, #4
> +	cmp	x1, #2
> +	b.lt	1f
> +
> +	orr	x0, x0, #(1 << 28)		// ECV enable
> +1:
>  	/* Enable MTE if present */
>  	mrs	x10, id_aa64pfr1_el1
>  	ubfx	x10, x10, #8, #4

Otherwise looks fine to me.

Thanks,

	M.

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

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

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

[Mark Rutland]

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