Re: [PATCH V2 7/7] arm64/boot: Enable EL2 requirements for FEAT_PMUv3p9

[Rob Herring <robh@kernel.org>]

On Tue, Feb 25, 2025 at 12:17 AM Anshuman Khandual
<anshuman.khandual@arm.com> wrote:
>
>
>
> On 2/24/25 19:41, Mark Rutland wrote:
> > On Mon, Feb 03, 2025 at 10:38:28AM +0530, Anshuman Khandual wrote:
> >> FEAT_PMUv3p9 registers such as PMICNTR_EL0, PMICFILTR_EL0, and PMUACR_EL1
> >> access from EL1 requires appropriate EL2 fine grained trap configuration
> >> via FEAT_FGT2 based trap control registers HDFGRTR2_EL2 and HDFGWTR2_EL2.
> >> Otherwise such register accesses will result in traps into EL2.
> >>
> >> Add a new helper __init_el2_fgt2() which initializes FEAT_FGT2 based fine
> >> grained trap control registers HDFGRTR2_EL2 and HDFGWTR2_EL2 (setting the
> >> bits nPMICNTR_EL0, nPMICFILTR_EL0 and nPMUACR_EL1) to enable access into
> >> PMICNTR_EL0, PMICFILTR_EL0, and PMUACR_EL1 registers.
> >>
> >> Also update booting.rst with SCR_EL3.FGTEn2 requirement for all FEAT_FGT2
> >> based registers to be accessible in EL2.
> >>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Cc: Will Deacon <will@kernel.org>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Rob Herring <robh@kernel.org>
> >> Cc: Jonathan Corbet <corbet@lwn.net>
> >> Cc: Marc Zyngier <maz@kernel.org>
> >> Cc: Oliver Upton <oliver.upton@linux.dev>
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Cc: linux-doc@vger.kernel.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Cc: kvmarm@lists.linux.dev
> >> Tested-by: Rob Herring (Arm) <robh@kernel.org>
> >> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >> ---
> >>  Documentation/arch/arm64/booting.rst | 18 ++++++++++++++++++
> >>  arch/arm64/include/asm/el2_setup.h   | 25 +++++++++++++++++++++++++
> >>  2 files changed, 43 insertions(+)
> >
> > Three things to note here:
> >
> > (1) I think this is missing some other necessary register configuration.
> >
> >     From a quick scan, we also require MDCR_EL3.EnPM2 (bit [7]) to be
> >     configured, which is not described in mainline nor here. If that
>
> Will update the Documentation/arch/arm64/booting.rst.
>
> >     resets to 0, then EL{2,1,0} accesses to a number of registers such
> >     as PMUACR_EL1 may trap to EL3>
> >     AFAICT the boot-wrapper resets that bit to 0, so have we actually
> >     tested all of this with the boot-wrapper? Does TF-A set this bit?
>
> Right, boot wrapper resets the bit to 0. We will need the following changes
> to set that up when PMUv3p9 is available. MDCR_EL3.EnPM2 also needs to be
> set when FEAT_SPMU, FEAT_EBEP, FEAT_PMUv3_SS or FEAT_SPMU2 are implemented.
> Should those features be checked here as well ?

From Arm ARM:
If FEAT_SPMU is implemented, then FEAT_PMUv3p9 is implemented.
If FEAT_SPMU2 is implemented, then FEAT_SPMU is implemented.
If FEAT_PMUv3_SS is implemented, then FEAT_PMUv3p9 is implemented.

FEAT_EBEP is v9.3 and later. if FEAT_PMUv3p9 is implemented, FEAT_EBEP
is implemented.

So I don't think we need to check anything else.

>
> --- a/arch/aarch64/include/asm/cpu.h
> +++ b/arch/aarch64/include/asm/cpu.h
> @@ -56,6 +56,7 @@
>  #define MDCR_EL3_SBRBE_NOTRAP_NOPROHIBIT       (UL(3) << 32)
>  #define MDCR_EL3_ENPMSN                                BIT(36)
>  #define MDCR_EL3_EBWE                          BIT(43)
> +#define MDCR_EL3_EnPM2                         BIT(7)
>
>  #define SCR_EL3_RES1                   BITS(5, 4)
>  #define SCR_EL3_NS                     BIT(0)
> @@ -87,6 +88,7 @@
>  #define ID_AA64DFR0_EL1_PMSVER         BITS(35, 32)
>  #define ID_AA64DFR0_EL1_TRACEBUFFER    BITS(47, 44)
>  #define ID_AA64DFR0_EL1_BRBE           BITS(55, 52)
> +#define ID_AA64DFR0_EL1_PMUVER         BITS(11, 8)
>  #define ID_AA64DFR0_EL1_DEBUGVER       BITS(3, 0)
>
>  #define ID_AA64ISAR0_EL1_TME           BITS(27, 24)
> diff --git a/arch/aarch64/init.c b/arch/aarch64/init.c
> index 54e4cc4..fe7ed5f 100644
> --- a/arch/aarch64/init.c
> +++ b/arch/aarch64/init.c
> @@ -152,6 +152,9 @@ static void cpu_init_el3(void)
>         if (mrs_field(ID_AA64DFR0_EL1, DEBUGVER) >= 11)
>                 mdcr |= MDCR_EL3_EBWE;
>
> +       if (mrs_field(ID_AA64DFR0_EL1, PMUVER) >= 0b1001)
> +               mdcr |= MDCR_EL3_EnPM2;
> +
>         msr(MDCR_EL3, mdcr);
>
>         if (mrs_field(ID_AA64PFR0_EL1, SVE)) {
>
> MDCR_EL2.EnPM2 does not seem to be set on TFA either, will double check and
> get it enabled.
>
> >
> >     Are we sure we've cpatured *all* requirements for FEAT_PMUv3p9? i.e.
> >     is there anything else that we've missed?

For FEAT_PMUv3_SS, there's MDCR_EL3.EnPMSS, MDCR_EL3.PMSSE, and MDCR_EL2.PMSSE.

For FEAT_EBEP (and SEBEP), there's  MDCR_EL3.PMEE and MDCR_EL2.PMEE

I would assume those become requirements when and if the kernel uses
those features.

>
> >
> > (2) This is a fix for !VHE support for PMUACR and ICNTR, where the host
> >     may run at EL1 and consequently will be affected by fine grained
> >     traps.
> >
> >     So this probably needs a CC stable and/or fixes tag, and backport.
>
> Fixes: 0bbff9ed8165 ("perf/arm_pmuv3: Add PMUv3.9 per counter EL0 access control")
> Fixes: d8226d8cfbaf ("perf: arm_pmuv3: Add support for Armv9.4 PMU instruction counter")
> Cc: stable@vger.kernel.org
>
> But is there a particular stable tree this patch should be addressed ?

It's 6.12+, but the Fixes tags are enough for the tooling to decide.

>
> >
> > (3) As there's no KVM changes, this is only safe provided that the
> >     registers affected by these fine grained traps are already
> >     unconditionally trapped by other traps when running a vCPU.
> >
> >     It looks like PMICNTR_EL0, PMICFILTR_EL0, and PMUACR_EL1 are all
> >     trapped by MDCR_EL2.TPM, so that should work as long as we emulate
> >     the PMU. For direct access, FGT2 support will be a prerequisite.
> >
> > Ideally, we'd have added support for FGT2 before the PMU functionality
> > that implicitly depends upon it. We should pay more attention to trap
> > controls in future.
> >
> > Given (1) and (2) I think someone needs to look into this a bit more and
> > figure out if this needs a fixup or a respin.
>
> To summarize
>
> - Update arm64/booting.rst regarding MDCR_EL3.EnPM2
> - Add above mentioned "Fixes:" tag and "CC: stable"
> - But should respin this patch or send a fix up instead ?

It is not applied yet, right? So respin.

Rob