From: Will Deacon <will@kernel.org> To: Wei Li <liwei391@huawei.com> Cc: Catalin Marinas <catalin.marinas@arm.com>, Mark Rutland <mark.rutland@arm.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Anshuman Khandual <anshuman.khandual@arm.com>, Vincenzo Frascino <vincenzo.frascino@arm.com>, Marc Zyngier <maz@kernel.org>, Ionela Voinescu <ionela.voinescu@arm.com>, Ard Biesheuvel <ardb@kernel.org>, Amit Daniel Kachhap <amit.kachhap@arm.com>, Vladimir Murzin <vladimir.murzin@arm.com>, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, guohanjun@huawei.com Subject: Re: [PATCH v3] drivers/perf: Add support for ARMv8.3-SPE Date: Mon, 30 Nov 2020 10:06:28 +0000 [thread overview] Message-ID: <20201130100628.GB24098@willie-the-truck> (raw) In-Reply-To: <20201127060322.29025-1-liwei391@huawei.com> On Fri, Nov 27, 2020 at 02:03:22PM +0800, Wei Li wrote: > Armv8.3 extends the SPE by adding: > - Alignment field in the Events packet, and filtering on this event > using PMSEVFR_EL1. > - Support for the Scalable Vector Extension (SVE). > > The main additions for SVE are: > - Recording the vector length for SVE operations in the Operation Type > packet. It is not possible to filter on vector length. > - Incomplete predicate and empty predicate fields in the Events packet, > and filtering on these events using PMSEVFR_EL1. > > Update the check of pmsevfr for empty/partial predicated SVE and > alignment event in SPE driver. For adaption by the version of SPE, > expose 'pmsver' as cap attribute to userspace. > > Signed-off-by: Wei Li <liwei391@huawei.com> > --- > v2 -> v3: > - Make the definition of 'pmsevfr_res0' progressive and easy to check. > (Suggested by Will.) > --- > v1 -> v2: > - Rename 'pmuver' to 'pmsver', change it's type to 'u16' from 'int'. > (Suggested by Will and Leo.) > - Expose 'pmsver' as cap attribute through sysfs, instead of printing. > (Suggested by Will.) > --- > arch/arm64/include/asm/sysreg.h | 9 ++++++++- > drivers/perf/arm_spe_pmu.c | 21 +++++++++++++++++++-- > 2 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index d52c1b3ce589..57e5aee6f7e6 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -287,7 +287,11 @@ > #define SYS_PMSFCR_EL1_ST_SHIFT 18 > > #define SYS_PMSEVFR_EL1 sys_reg(3, 0, 9, 9, 5) > -#define SYS_PMSEVFR_EL1_RES0 0x0000ffff00ff0f55UL > +#define SYS_PMSEVFR_EL1_RES0_8_2 \ > + (GENMASK_ULL(47, 32) | GENMASK_ULL(23, 16) | GENMASK_ULL(11, 8) |\ > + BIT_ULL(6) | BIT_ULL(4) | BIT_ULL(2) | BIT_ULL(0)) > +#define SYS_PMSEVFR_EL1_RES0_8_3 \ > + (SYS_PMSEVFR_EL1_RES0_8_2 & ~(BIT_ULL(18) | BIT_ULL(17) | BIT_ULL(11))) > > #define SYS_PMSLATFR_EL1 sys_reg(3, 0, 9, 9, 6) > #define SYS_PMSLATFR_EL1_MINLAT_SHIFT 0 > @@ -829,6 +833,9 @@ > #define ID_AA64DFR0_PMUVER_8_5 0x6 > #define ID_AA64DFR0_PMUVER_IMP_DEF 0xf > > +#define ID_AA64DFR0_PMSVER_8_2 0x1 > +#define ID_AA64DFR0_PMSVER_8_3 0x2 > + > #define ID_DFR0_PERFMON_SHIFT 24 > > #define ID_DFR0_PERFMON_8_1 0x4 > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c > index cc00915ad6d1..515c51271d7f 100644 > --- a/drivers/perf/arm_spe_pmu.c > +++ b/drivers/perf/arm_spe_pmu.c > @@ -54,7 +54,7 @@ struct arm_spe_pmu { > struct hlist_node hotplug_node; > > int irq; /* PPI */ > - > + u16 pmsver; > u16 min_period; > u16 counter_sz; > > @@ -93,6 +93,7 @@ enum arm_spe_pmu_capabilities { > SPE_PMU_CAP_FEAT_MAX, > SPE_PMU_CAP_CNT_SZ = SPE_PMU_CAP_FEAT_MAX, > SPE_PMU_CAP_MIN_IVAL, > + SPE_PMU_CAP_PMSVER, > }; > > static int arm_spe_pmu_feat_caps[SPE_PMU_CAP_FEAT_MAX] = { > @@ -110,6 +111,8 @@ static u32 arm_spe_pmu_cap_get(struct arm_spe_pmu *spe_pmu, int cap) > return spe_pmu->counter_sz; > case SPE_PMU_CAP_MIN_IVAL: > return spe_pmu->min_period; > + case SPE_PMU_CAP_PMSVER: > + return spe_pmu->pmsver; > default: > WARN(1, "unknown cap %d\n", cap); > } > @@ -143,6 +146,7 @@ static struct attribute *arm_spe_pmu_cap_attr[] = { > SPE_CAP_EXT_ATTR_ENTRY(ernd, SPE_PMU_CAP_ERND), > SPE_CAP_EXT_ATTR_ENTRY(count_size, SPE_PMU_CAP_CNT_SZ), > SPE_CAP_EXT_ATTR_ENTRY(min_interval, SPE_PMU_CAP_MIN_IVAL), > + SPE_CAP_EXT_ATTR_ENTRY(pmsver, SPE_PMU_CAP_PMSVER), > NULL, > }; > > @@ -655,6 +659,18 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev) > return IRQ_HANDLED; > } > > +static u64 arm_spe_pmsevfr_res0(u16 pmsver) > +{ > + switch (pmsver) { > + case ID_AA64DFR0_PMSVER_8_2: > + return SYS_PMSEVFR_EL1_RES0_8_2; > + case ID_AA64DFR0_PMSVER_8_3: > + return SYS_PMSEVFR_EL1_RES0_8_3; > + default: > + return -1; > + } I don't think -1 is the right choice here if we don't recognise the version, as it means that old kernels won't work on new hardware. I think it would be better to return the highest architecture version we know about in that case... > +} > + > /* Perf callbacks */ > static int arm_spe_pmu_event_init(struct perf_event *event) > { > @@ -670,7 +686,7 @@ static int arm_spe_pmu_event_init(struct perf_event *event) > !cpumask_test_cpu(event->cpu, &spe_pmu->supported_cpus)) > return -ENOENT; > > - if (arm_spe_event_to_pmsevfr(event) & SYS_PMSEVFR_EL1_RES0) > + if (arm_spe_event_to_pmsevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver)) > return -EOPNOTSUPP; > > if (attr->exclude_idle) > @@ -937,6 +953,7 @@ static void __arm_spe_pmu_dev_probe(void *info) > fld, smp_processor_id()); > return; > } > + spe_pmu->pmsver = (u16)fld; ... which also means we should clamp this value now that we expose it to userspace. Otherwise, userspace can't rely on this field for anything. That said -- please can you tell me what userspace intends to do with this version number? Will
WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will@kernel.org> To: Wei Li <liwei391@huawei.com> Cc: Mark Rutland <mark.rutland@arm.com>, Vladimir Murzin <vladimir.murzin@arm.com>, Anshuman Khandual <anshuman.khandual@arm.com>, Catalin Marinas <catalin.marinas@arm.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, guohanjun@huawei.com, linux-kernel@vger.kernel.org, Marc Zyngier <maz@kernel.org>, Amit Daniel Kachhap <amit.kachhap@arm.com>, Vincenzo Frascino <vincenzo.frascino@arm.com>, Ionela Voinescu <ionela.voinescu@arm.com>, Ard Biesheuvel <ardb@kernel.org>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3] drivers/perf: Add support for ARMv8.3-SPE Date: Mon, 30 Nov 2020 10:06:28 +0000 [thread overview] Message-ID: <20201130100628.GB24098@willie-the-truck> (raw) In-Reply-To: <20201127060322.29025-1-liwei391@huawei.com> On Fri, Nov 27, 2020 at 02:03:22PM +0800, Wei Li wrote: > Armv8.3 extends the SPE by adding: > - Alignment field in the Events packet, and filtering on this event > using PMSEVFR_EL1. > - Support for the Scalable Vector Extension (SVE). > > The main additions for SVE are: > - Recording the vector length for SVE operations in the Operation Type > packet. It is not possible to filter on vector length. > - Incomplete predicate and empty predicate fields in the Events packet, > and filtering on these events using PMSEVFR_EL1. > > Update the check of pmsevfr for empty/partial predicated SVE and > alignment event in SPE driver. For adaption by the version of SPE, > expose 'pmsver' as cap attribute to userspace. > > Signed-off-by: Wei Li <liwei391@huawei.com> > --- > v2 -> v3: > - Make the definition of 'pmsevfr_res0' progressive and easy to check. > (Suggested by Will.) > --- > v1 -> v2: > - Rename 'pmuver' to 'pmsver', change it's type to 'u16' from 'int'. > (Suggested by Will and Leo.) > - Expose 'pmsver' as cap attribute through sysfs, instead of printing. > (Suggested by Will.) > --- > arch/arm64/include/asm/sysreg.h | 9 ++++++++- > drivers/perf/arm_spe_pmu.c | 21 +++++++++++++++++++-- > 2 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h > index d52c1b3ce589..57e5aee6f7e6 100644 > --- a/arch/arm64/include/asm/sysreg.h > +++ b/arch/arm64/include/asm/sysreg.h > @@ -287,7 +287,11 @@ > #define SYS_PMSFCR_EL1_ST_SHIFT 18 > > #define SYS_PMSEVFR_EL1 sys_reg(3, 0, 9, 9, 5) > -#define SYS_PMSEVFR_EL1_RES0 0x0000ffff00ff0f55UL > +#define SYS_PMSEVFR_EL1_RES0_8_2 \ > + (GENMASK_ULL(47, 32) | GENMASK_ULL(23, 16) | GENMASK_ULL(11, 8) |\ > + BIT_ULL(6) | BIT_ULL(4) | BIT_ULL(2) | BIT_ULL(0)) > +#define SYS_PMSEVFR_EL1_RES0_8_3 \ > + (SYS_PMSEVFR_EL1_RES0_8_2 & ~(BIT_ULL(18) | BIT_ULL(17) | BIT_ULL(11))) > > #define SYS_PMSLATFR_EL1 sys_reg(3, 0, 9, 9, 6) > #define SYS_PMSLATFR_EL1_MINLAT_SHIFT 0 > @@ -829,6 +833,9 @@ > #define ID_AA64DFR0_PMUVER_8_5 0x6 > #define ID_AA64DFR0_PMUVER_IMP_DEF 0xf > > +#define ID_AA64DFR0_PMSVER_8_2 0x1 > +#define ID_AA64DFR0_PMSVER_8_3 0x2 > + > #define ID_DFR0_PERFMON_SHIFT 24 > > #define ID_DFR0_PERFMON_8_1 0x4 > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c > index cc00915ad6d1..515c51271d7f 100644 > --- a/drivers/perf/arm_spe_pmu.c > +++ b/drivers/perf/arm_spe_pmu.c > @@ -54,7 +54,7 @@ struct arm_spe_pmu { > struct hlist_node hotplug_node; > > int irq; /* PPI */ > - > + u16 pmsver; > u16 min_period; > u16 counter_sz; > > @@ -93,6 +93,7 @@ enum arm_spe_pmu_capabilities { > SPE_PMU_CAP_FEAT_MAX, > SPE_PMU_CAP_CNT_SZ = SPE_PMU_CAP_FEAT_MAX, > SPE_PMU_CAP_MIN_IVAL, > + SPE_PMU_CAP_PMSVER, > }; > > static int arm_spe_pmu_feat_caps[SPE_PMU_CAP_FEAT_MAX] = { > @@ -110,6 +111,8 @@ static u32 arm_spe_pmu_cap_get(struct arm_spe_pmu *spe_pmu, int cap) > return spe_pmu->counter_sz; > case SPE_PMU_CAP_MIN_IVAL: > return spe_pmu->min_period; > + case SPE_PMU_CAP_PMSVER: > + return spe_pmu->pmsver; > default: > WARN(1, "unknown cap %d\n", cap); > } > @@ -143,6 +146,7 @@ static struct attribute *arm_spe_pmu_cap_attr[] = { > SPE_CAP_EXT_ATTR_ENTRY(ernd, SPE_PMU_CAP_ERND), > SPE_CAP_EXT_ATTR_ENTRY(count_size, SPE_PMU_CAP_CNT_SZ), > SPE_CAP_EXT_ATTR_ENTRY(min_interval, SPE_PMU_CAP_MIN_IVAL), > + SPE_CAP_EXT_ATTR_ENTRY(pmsver, SPE_PMU_CAP_PMSVER), > NULL, > }; > > @@ -655,6 +659,18 @@ static irqreturn_t arm_spe_pmu_irq_handler(int irq, void *dev) > return IRQ_HANDLED; > } > > +static u64 arm_spe_pmsevfr_res0(u16 pmsver) > +{ > + switch (pmsver) { > + case ID_AA64DFR0_PMSVER_8_2: > + return SYS_PMSEVFR_EL1_RES0_8_2; > + case ID_AA64DFR0_PMSVER_8_3: > + return SYS_PMSEVFR_EL1_RES0_8_3; > + default: > + return -1; > + } I don't think -1 is the right choice here if we don't recognise the version, as it means that old kernels won't work on new hardware. I think it would be better to return the highest architecture version we know about in that case... > +} > + > /* Perf callbacks */ > static int arm_spe_pmu_event_init(struct perf_event *event) > { > @@ -670,7 +686,7 @@ static int arm_spe_pmu_event_init(struct perf_event *event) > !cpumask_test_cpu(event->cpu, &spe_pmu->supported_cpus)) > return -ENOENT; > > - if (arm_spe_event_to_pmsevfr(event) & SYS_PMSEVFR_EL1_RES0) > + if (arm_spe_event_to_pmsevfr(event) & arm_spe_pmsevfr_res0(spe_pmu->pmsver)) > return -EOPNOTSUPP; > > if (attr->exclude_idle) > @@ -937,6 +953,7 @@ static void __arm_spe_pmu_dev_probe(void *info) > fld, smp_processor_id()); > return; > } > + spe_pmu->pmsver = (u16)fld; ... which also means we should clamp this value now that we expose it to userspace. Otherwise, userspace can't rely on this field for anything. That said -- please can you tell me what userspace intends to do with this version number? Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-11-30 10:07 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-27 6:03 [PATCH v3] drivers/perf: Add support for ARMv8.3-SPE Wei Li 2020-11-27 6:03 ` Wei Li 2020-11-27 12:03 ` Suzuki K Poulose 2020-11-27 12:03 ` Suzuki K Poulose 2020-11-30 9:55 ` Will Deacon 2020-11-30 9:55 ` Will Deacon 2020-11-30 10:06 ` Will Deacon [this message] 2020-11-30 10:06 ` Will Deacon 2020-12-03 8:42 ` liwei (GF) 2020-12-03 8:42 ` liwei (GF) 2020-12-03 9:19 ` Will Deacon 2020-12-03 9:19 ` Will Deacon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201130100628.GB24098@willie-the-truck \ --to=will@kernel.org \ --cc=amit.kachhap@arm.com \ --cc=anshuman.khandual@arm.com \ --cc=ardb@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=guohanjun@huawei.com \ --cc=ionela.voinescu@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=liwei391@huawei.com \ --cc=mark.rutland@arm.com \ --cc=maz@kernel.org \ --cc=suzuki.poulose@arm.com \ --cc=vincenzo.frascino@arm.com \ --cc=vladimir.murzin@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.