[PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[Fuad Tabba]

On a CPU with FGT2 and an IMPLEMENTATION DEFINED PMU (PMUVer 0b1111),
__init_el2_fgt2 sets the PMUv3p9 bits in HDFGRTR2_EL2 and HDFGWTR2_EL2,
which are RES0 there. The macro only checks that PMUVer is at least
PMUv3p9, and 0b1111 passes.

Exclude 0b1111 before the compare, as __init_el2_debug does.

Fixes: 858c7bfcb35e1 ("arm64/boot: Enable EL2 requirements for FEAT_PMUv3p9")
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 arch/arm64/include/asm/el2_setup.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
index 87560d8b254e6..1da277baacf78 100644
--- a/arch/arm64/include/asm/el2_setup.h
+++ b/arch/arm64/include/asm/el2_setup.h
@@ -421,8 +421,9 @@
 	mov	x2, xzr
 	mrs	x1, id_aa64dfr0_el1
 	ubfx	x1, x1, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
-	cmp	x1, #ID_AA64DFR0_EL1_PMUVer_V3P9
-	b.lt	.Lskip_pmuv3p9_\@
+	cmp	x1, #ID_AA64DFR0_EL1_PMUVer_IMP_DEF
+	ccmp	x1, #ID_AA64DFR0_EL1_PMUVer_V3P9, #8, ne
+	b.lt	.Lskip_pmuv3p9_\@		// Skip if < PMUv3p9 or IMP_DEF
 
 	orr	x0, x0, #HDFGRTR2_EL2_nPMICNTR_EL0
 	orr	x0, x0, #HDFGRTR2_EL2_nPMICFILTR_EL0

base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
-- 
2.39.5

Re: [PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[Anshuman Khandual]

On Wed, Sep 30, 2026 at 01:12:12PM +0100, Fuad Tabba wrote:
> On a CPU with FGT2 and an IMPLEMENTATION DEFINED PMU (PMUVer 0b1111),
> __init_el2_fgt2 sets the PMUv3p9 bits in HDFGRTR2_EL2 and HDFGWTR2_EL2,
> which are RES0 there. The macro only checks that PMUVer is at least
> PMUv3p9, and 0b1111 passes.

Just curious - has this caused any real world problem on IMP defined PMUs ?

> 
> Exclude 0b1111 before the compare, as __init_el2_debug does.
> 
> Fixes: 858c7bfcb35e1 ("arm64/boot: Enable EL2 requirements for FEAT_PMUv3p9")
> Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
> ---
>  arch/arm64/include/asm/el2_setup.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> index 87560d8b254e6..1da277baacf78 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h
> @@ -421,8 +421,9 @@
>  	mov	x2, xzr
>  	mrs	x1, id_aa64dfr0_el1
>  	ubfx	x1, x1, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
> -	cmp	x1, #ID_AA64DFR0_EL1_PMUVer_V3P9
> -	b.lt	.Lskip_pmuv3p9_\@
> +	cmp	x1, #ID_AA64DFR0_EL1_PMUVer_IMP_DEF
> +	ccmp	x1, #ID_AA64DFR0_EL1_PMUVer_V3P9, #8, ne
> +	b.lt	.Lskip_pmuv3p9_\@		// Skip if < PMUv3p9 or IMP_DEF
>  
>  	orr	x0, x0, #HDFGRTR2_EL2_nPMICNTR_EL0
>  	orr	x0, x0, #HDFGRTR2_EL2_nPMICFILTR_EL0
> 
> base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
> -- 
> 2.39.5
> 

Re: [PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[Fuad Tabba]

Hi Anshuman,

On Wed, 30 Sept 2026 at 14:26, Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
> On Wed, Sep 30, 2026 at 01:12:12PM +0100, Fuad Tabba wrote:
> > On a CPU with FGT2 and an IMPLEMENTATION DEFINED PMU (PMUVer 0b1111),
> > __init_el2_fgt2 sets the PMUv3p9 bits in HDFGRTR2_EL2 and HDFGWTR2_EL2,
> > which are RES0 there. The macro only checks that PMUVer is at least
> > PMUv3p9, and 0b1111 passes.
>
> Just curious - has this caused any real world problem on IMP defined PMUs ?

Not as far as I know. I found it while auditing these and related bits for pKVM.

Cheers,
/fuad

Re: [PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[Anshuman Khandual]

On Wed, Sep 30, 2026 at 01:12:12PM +0100, Fuad Tabba wrote:
> On a CPU with FGT2 and an IMPLEMENTATION DEFINED PMU (PMUVer 0b1111),
> __init_el2_fgt2 sets the PMUv3p9 bits in HDFGRTR2_EL2 and HDFGWTR2_EL2,
> which are RES0 there. The macro only checks that PMUVer is at least
> PMUv3p9, and 0b1111 passes.
> 
> Exclude 0b1111 before the compare, as __init_el2_debug does.
> 
> Fixes: 858c7bfcb35e1 ("arm64/boot: Enable EL2 requirements for FEAT_PMUv3p9")
> Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>

This fixes a semantics error during feature detection while also preventing
undesirable writes into RES0 fields in IMPLEMENTATION DEFINED PMU instances.

Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>

> ---
>  arch/arm64/include/asm/el2_setup.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> index 87560d8b254e6..1da277baacf78 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h
> @@ -421,8 +421,9 @@
>  	mov	x2, xzr
>  	mrs	x1, id_aa64dfr0_el1
>  	ubfx	x1, x1, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
> -	cmp	x1, #ID_AA64DFR0_EL1_PMUVer_V3P9
> -	b.lt	.Lskip_pmuv3p9_\@
> +	cmp	x1, #ID_AA64DFR0_EL1_PMUVer_IMP_DEF
> +	ccmp	x1, #ID_AA64DFR0_EL1_PMUVer_V3P9, #8, ne
> +	b.lt	.Lskip_pmuv3p9_\@		// Skip if < PMUv3p9 or IMP_DEF
>  
>  	orr	x0, x0, #HDFGRTR2_EL2_nPMICNTR_EL0
>  	orr	x0, x0, #HDFGRTR2_EL2_nPMICFILTR_EL0
> 
> base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
> -- 
> 2.39.5
> 

Re: [PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[Will Deacon]

On Wed, Sep 30, 2026 at 01:12:12PM +0100, Fuad Tabba wrote:
> On a CPU with FGT2 and an IMPLEMENTATION DEFINED PMU (PMUVer 0b1111),
> __init_el2_fgt2 sets the PMUv3p9 bits in HDFGRTR2_EL2 and HDFGWTR2_EL2,
> which are RES0 there. The macro only checks that PMUVer is at least
> PMUv3p9, and 0b1111 passes.
> 
> Exclude 0b1111 before the compare, as __init_el2_debug does.
> 
> Fixes: 858c7bfcb35e1 ("arm64/boot: Enable EL2 requirements for FEAT_PMUv3p9")
> Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
> ---
>  arch/arm64/include/asm/el2_setup.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
> index 87560d8b254e6..1da277baacf78 100644
> --- a/arch/arm64/include/asm/el2_setup.h
> +++ b/arch/arm64/include/asm/el2_setup.h
> @@ -421,8 +421,9 @@
>  	mov	x2, xzr
>  	mrs	x1, id_aa64dfr0_el1
>  	ubfx	x1, x1, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
> -	cmp	x1, #ID_AA64DFR0_EL1_PMUVer_V3P9
> -	b.lt	.Lskip_pmuv3p9_\@
> +	cmp	x1, #ID_AA64DFR0_EL1_PMUVer_IMP_DEF
> +	ccmp	x1, #ID_AA64DFR0_EL1_PMUVer_V3P9, #8, ne
> +	b.lt	.Lskip_pmuv3p9_\@		// Skip if < PMUv3p9 or IMP_DEF

Are you sure #8 is the correct immediate for the ccmp? My reading of the
pseudocode is that it should be #9, but this instruction has always confused
me and I hate the fact that it doesn't take another condition code mnemonic
instead of a raw immediate value.

Will

Re: [PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[Fuad Tabba]

On Fri, Oct 02, 2026 at 02:29:26PM +0100, Will Deacon wrote:
> Are you sure #8 is the correct immediate for the ccmp? My reading of the
> pseudocode is that it should be #9, but this instruction has always confused
> me and I hate the fact that it doesn't take another condition code mnemonic
> instead of a raw immediate value.

My read is that #8 is right. From the CCMP (immediate) pseudocode in the
Arm ARM (DDI 0487 M.d, C6.2.81), flags start out as the nzcv immediate
and are only overwritten by the compare when the condition holds:

      var flags : bits(4) = nzcv;
      ...
      if ConditionHolds(condition) then
          ...
          (-, flags) = AddWithCarry{datasize}(operand1, NOT operand2, '1');
      end;
      PSTATE.[N,Z,C,V] = flags;

So when PMUVer == IMP_DEF the ne fails and NZCV = 0b1000, i.e. N=1, V=0.
LT is N != V (Table C1-1), so b.lt is taken and we skip. #9 would give
N=1, V=1, so we'd fall through and set the bits on an IMP_DEF PMU.

Agreed on the raw immediate, it's horrible.

Cheers,
/fuad

Re: [PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[Will Deacon]

On Fri, Oct 02, 2026 at 02:54:47PM +0100, Fuad Tabba wrote:
> On Fri, Oct 02, 2026 at 02:29:26PM +0100, Will Deacon wrote:
> > Are you sure #8 is the correct immediate for the ccmp? My reading of the
> > pseudocode is that it should be #9, but this instruction has always confused
> > me and I hate the fact that it doesn't take another condition code mnemonic
> > instead of a raw immediate value.
> 
> My read is that #8 is right. From the CCMP (immediate) pseudocode in the
> Arm ARM (DDI 0487 M.d, C6.2.81), flags start out as the nzcv immediate
> and are only overwritten by the compare when the condition holds:
> 
>       var flags : bits(4) = nzcv;
>       ...
>       if ConditionHolds(condition) then
>           ...
>           (-, flags) = AddWithCarry{datasize}(operand1, NOT operand2, '1');
>       end;
>       PSTATE.[N,Z,C,V] = flags;
> 
> So when PMUVer == IMP_DEF the ne fails and NZCV = 0b1000, i.e. N=1, V=0.
> LT is N != V (Table C1-1), so b.lt is taken and we skip. #9 would give
> N=1, V=1, so we'd fall through and set the bits on an IMP_DEF PMU.

Aha, that table is pretty helpful, thanks.

I think you're right -- I missed the inversion at the end of
ConditionHolds().

I'll pick this up next week.

Will

Re: [PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[Catalin Marinas]

On Fri, Oct 02, 2026 at 03:00:42PM +0100, Will Deacon wrote:
> On Fri, Oct 02, 2026 at 02:54:47PM +0100, Fuad Tabba wrote:
> > On Fri, Oct 02, 2026 at 02:29:26PM +0100, Will Deacon wrote:
> > > Are you sure #8 is the correct immediate for the ccmp? My reading of the
> > > pseudocode is that it should be #9, but this instruction has always confused
> > > me and I hate the fact that it doesn't take another condition code mnemonic
> > > instead of a raw immediate value.
> > 
> > My read is that #8 is right. From the CCMP (immediate) pseudocode in the
> > Arm ARM (DDI 0487 M.d, C6.2.81), flags start out as the nzcv immediate
> > and are only overwritten by the compare when the condition holds:
> > 
> >       var flags : bits(4) = nzcv;
> >       ...
> >       if ConditionHolds(condition) then
> >           ...
> >           (-, flags) = AddWithCarry{datasize}(operand1, NOT operand2, '1');
> >       end;
> >       PSTATE.[N,Z,C,V] = flags;
> > 
> > So when PMUVer == IMP_DEF the ne fails and NZCV = 0b1000, i.e. N=1, V=0.
> > LT is N != V (Table C1-1), so b.lt is taken and we skip. #9 would give
> > N=1, V=1, so we'd fall through and set the bits on an IMP_DEF PMU.
> 
> Aha, that table is pretty helpful, thanks.
> 
> I think you're right -- I missed the inversion at the end of
> ConditionHolds().
> 
> I'll pick this up next week.

I couldn't figure out the #8 either. I wonder whether it's more readable
as (untested):

	sub	x1, x1, #ID_AA64DFR0_EL1_PMUVer_V3P9
	cmp	x1, #(ID_AA64DFR0_EL1_PMUVer_IMP_DEF - ID_AA64DFR0_EL1_PMUVer_V3P9)
	b.hs	.Lskip_pmuv3p9_\@	// Skip unless V3P9 <= PMUVer < IMP_DEF

The first sub either gives us a large number (negative but we do the
unsigned comparison with b.hs) or something between 0 and (15-9). The
cmp and the 'same' part of b.hs skip the (15-9) case.

-- 
Catalin

Re: [PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[Fuad Tabba]

On Fri, Oct 02, 2026 at 03:51:16PM +0100, Catalin Marinas wrote:
> I couldn't figure out the #8 either. I wonder whether it's more readable
> as (untested):
>
>     sub    x1, x1, #ID_AA64DFR0_EL1_PMUVer_V3P9
>     cmp    x1, #(ID_AA64DFR0_EL1_PMUVer_IMP_DEF - ID_AA64DFR0_EL1_PMUVer_V3P9)
>     b.hs    .Lskip_pmuv3p9_\@    // Skip unless V3P9 <= PMUVer < IMP_DEF

This looks correct to me.

That said, if readability is the goal, I think the plainest is two
compares and two branches. It's one more instruction, but this runs
once at boot:

      cmp     x1, #ID_AA64DFR0_EL1_PMUVer_IMP_DEF
      b.eq    .Lskip_pmuv3p9_\@
      cmp     x1, #ID_AA64DFR0_EL1_PMUVer_V3P9
      b.lt    .Lskip_pmuv3p9_\@

I used ccmp to match __init_el2_debug in the same file, which does the
same NI/IMP_DEF check with a raw #4, as does reset_pmuserenr_el0 in
assembler.h (and hyp-entry.S uses the same pattern for HVC64/HVC32).

Cheers,
/fuad

Re: [PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[Catalin Marinas]

On Fri, Oct 02, 2026 at 04:24:06PM +0100, Fuad Tabba wrote:
> On Fri, Oct 02, 2026 at 03:51:16PM +0100, Catalin Marinas wrote:
> > I couldn't figure out the #8 either. I wonder whether it's more readable
> > as (untested):
> >
> >     sub    x1, x1, #ID_AA64DFR0_EL1_PMUVer_V3P9
> >     cmp    x1, #(ID_AA64DFR0_EL1_PMUVer_IMP_DEF - ID_AA64DFR0_EL1_PMUVer_V3P9)
> >     b.hs    .Lskip_pmuv3p9_\@    // Skip unless V3P9 <= PMUVer < IMP_DEF
> 
> This looks correct to me.
> 
> That said, if readability is the goal, I think the plainest is two
> compares and two branches. It's one more instruction, but this runs
> once at boot:
> 
>       cmp     x1, #ID_AA64DFR0_EL1_PMUVer_IMP_DEF
>       b.eq    .Lskip_pmuv3p9_\@
>       cmp     x1, #ID_AA64DFR0_EL1_PMUVer_V3P9
>       b.lt    .Lskip_pmuv3p9_\@
> 
> I used ccmp to match __init_el2_debug in the same file, which does the
> same NI/IMP_DEF check with a raw #4, as does reset_pmuserenr_el0 in
> assembler.h (and hyp-entry.S uses the same pattern for HVC64/HVC32).

I'll leave it to Will. My preference is whatever is more readable, no
need to optimise another cycle or two here.

-- 
Catalin

Re: [PATCH] arm64/boot: Don't set PMUv3p9 FGT2 bits without PMUv3

[Bradley Morgan]

On 30 September 2026 13:12:12 BST, Fuad Tabba <fuad.tabba@linux.dev> wrote:
>On a CPU with FGT2 and an IMPLEMENTATION DEFINED PMU (PMUVer 0b1111),
>__init_el2_fgt2 sets the PMUv3p9 bits in HDFGRTR2_EL2 and HDFGWTR2_EL2,
>which are RES0 there. The macro only checks that PMUVer is at least
>PMUv3p9, and 0b1111 passes.
>
>Exclude 0b1111 before the compare, as __init_el2_debug does.
>
>Fixes: 858c7bfcb35e1 ("arm64/boot: Enable EL2 requirements for FEAT_PMUv3p9")

I see, I see.

Reviewed-by: Bradley Morgan <brads@mainlining.org>


>Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
>---
> arch/arm64/include/asm/el2_setup.h | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
>diff --git a/arch/arm64/include/asm/el2_setup.h b/arch/arm64/include/asm/el2_setup.h
>index 87560d8b254e6..1da277baacf78 100644
>--- a/arch/arm64/include/asm/el2_setup.h
>+++ b/arch/arm64/include/asm/el2_setup.h
>@@ -421,8 +421,9 @@
> 	mov	x2, xzr
> 	mrs	x1, id_aa64dfr0_el1
> 	ubfx	x1, x1, #ID_AA64DFR0_EL1_PMUVer_SHIFT, #4
>-	cmp	x1, #ID_AA64DFR0_EL1_PMUVer_V3P9
>-	b.lt	.Lskip_pmuv3p9_\@
>+	cmp	x1, #ID_AA64DFR0_EL1_PMUVer_IMP_DEF
>+	ccmp	x1, #ID_AA64DFR0_EL1_PMUVer_V3P9, #8, ne
>+	b.lt	.Lskip_pmuv3p9_\@		// Skip if < PMUv3p9 or IMP_DEF
> 
> 	orr	x0, x0, #HDFGRTR2_EL2_nPMICNTR_EL0
> 	orr	x0, x0, #HDFGRTR2_EL2_nPMICFILTR_EL0
>
>base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
>

--- Thanks!
"I'm not a very positive person" - Linus torvalds