| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAL_Jsq+dJiRLNM_QE_r6HOg7hEkJCPR6j-TGBpiJZLd+RDBbAA@mail.gmail.com>
Date: Wed, 26 Feb 2025 11:14:57 -0600
From: Rob Herring <robh@...nel.org>
To: Anshuman Khandual <anshuman.khandual@....com>
Cc: Mark Rutland <mark.rutland@....com>, linux-arm-kernel@...ts.infradead.org,
Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>, Marc Zyngier <maz@...nel.org>,
Ryan Roberts <ryan.roberts@....com>, Oliver Upton <oliver.upton@...ux.dev>,
Jonathan Corbet <corbet@....net>, Eric Auger <eric.auger@...hat.com>, kvmarm@...ts.linux.dev,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 7/7] arm64/boot: Enable EL2 requirements for FEAT_PMUv3p9
On Tue, Feb 25, 2025 at 12:17 AM Anshuman Khandual
<anshuman.khandual@....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@....com>
> >> Cc: Will Deacon <will@...nel.org>
> >> Cc: Mark Rutland <mark.rutland@....com>
> >> Cc: Rob Herring <robh@...nel.org>
> >> Cc: Jonathan Corbet <corbet@....net>
> >> Cc: Marc Zyngier <maz@...nel.org>
> >> Cc: Oliver Upton <oliver.upton@...ux.dev>
> >> Cc: linux-arm-kernel@...ts.infradead.org
> >> Cc: linux-doc@...r.kernel.org
> >> Cc: linux-kernel@...r.kernel.org
> >> Cc: kvmarm@...ts.linux.dev
> >> Tested-by: Rob Herring (Arm) <robh@...nel.org>
> >> Reviewed-by: Rob Herring (Arm) <robh@...nel.org>
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@....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@...r.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
Powered by blists - more mailing lists