Re: [RFC PATCH v7 05/28] perf: arm_spe_pmu: Add PMBIDR_EL1 and PMSIDR_EL1 to struct arm_spe_pmu

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi,

On Thu, Oct 15, 2026 at 04:12:27PM +0100, Lorenzo Stoakes (ARM) wrote:
> I think probably some kind of commit message would be good even for a
> trivial change :)

Of course, I'll add something here.

> 
> On Thu, Sep 03, 2026 at 05:06:00PM +0100, Alexandru Elisei wrote:
> > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> > ---
> >  drivers/perf/arm_spe_pmu.c       | 3 +++
> >  include/linux/perf/arm_spe_pmu.h | 2 ++
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> > index 79c571a649d5..42ba0924bd02 100644
> > --- a/drivers/perf/arm_spe_pmu.c
> > +++ b/drivers/perf/arm_spe_pmu.c
> > @@ -1095,6 +1095,7 @@ static void __arm_spe_pmu_dev_probe(void *info)
> >  			"profiling buffer owned by higher exception level\n");
> >  		return;
> >  	}
> > +	spe_pmu->pmbidr_el1 = reg;
> 
> This doesn't seem to be used anywhere in the series?

Indeed, that's a leftover from the previous iterations, will remove.

> 
> >
> >  	/* Minimum alignment. If it's out-of-range, then fail the probe */
> >  	fld = FIELD_GET(PMBIDR_EL1_ALIGN, reg);
> > @@ -1107,6 +1108,8 @@ static void __arm_spe_pmu_dev_probe(void *info)
> >
> >  	/* It's now safe to read PMSIDR and figure out what we've got */
> >  	reg = read_sysreg_s(SYS_PMSIDR_EL1);
> > +	spe_pmu->pmsidr_el1 = reg;
> > +
> >  	if (FIELD_GET(PMSIDR_EL1_FE, reg))
> >  		spe_pmu->features |= SPE_PMU_FEAT_FILT_EVT;
> >
> > diff --git a/include/linux/perf/arm_spe_pmu.h b/include/linux/perf/arm_spe_pmu.h
> > index af86fd184050..97b14e236fcd 100644
> > --- a/include/linux/perf/arm_spe_pmu.h
> > +++ b/include/linux/perf/arm_spe_pmu.h
> > @@ -21,6 +21,8 @@ struct arm_spe_pmu {
> >  	cpumask_t				supported_cpus;
> >  	struct hlist_node			hotplug_node;
> >
> > +	u64					pmbidr_el1;
> 
> Equally, this doesn't seem to be used anywhere in the series, maybe drop?

Fixed.

> 
> > +	u64					pmsidr_el1;
> 
> This field is used in kvm_spe_has_feat_spe_fds() and
> kvm_spe_has_feat_spe_fne():
> 
> bool kvm_spe_has_feat_spe_fds(struct kvm *kvm)
> {
> 	...
> 	return kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSVer, V1P4) &&
> 	       FIELD_GET(PMSIDR_EL1_FDS, spe_pmu->pmsidr_el1);
> }
> 
> bool kvm_spe_has_feat_spe_fne(struct kvm *kvm)
> {
> 	...
> 	return kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSVer, V1P2) &&
> 	       FIELD_GET(PMSIDR_EL1_FnE, spe_pmu->pmsidr_el1);
> }
> 
> So checking for FDS and FnE, but these are available as feature flags as
> __arm_spe_pmu_dev_probe() gets these:
> 
> static void __arm_spe_pmu_dev_probe(void *info)
> {
> 	...
> 	if (FIELD_GET(PMSIDR_EL1_FnE, reg))
> 		spe_pmu->features |= SPE_PMU_FEAT_INV_FILT_EVT;
> 	...
> 	if (FIELD_GET(PMSIDR_EL1_FDS, reg))
> 		spe_pmu->features |= SPE_PMU_FEAT_FDS;
> 	...
> }
> 
> And then kvm_spe_has_feat_spe_fds() and kvm_spe_has_feat_spe_fne() can be:
> 
> bool kvm_spe_has_feat_spe_fds(struct kvm *kvm)
> {
> 	...
> 	return kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSVer, V1P4) &&
> 	       (spe_pmu->features & SPE_PMU_FEAT_FDS);
> }
> 
> bool kvm_spe_has_feat_spe_fne(struct kvm *kvm)
> {
> 	...
> 	return kvm_has_feat(kvm, ID_AA64DFR0_EL1, PMSVer, V1P2) &&
> 	       (spe_pmu->features & SPE_PMU_FEAT_INV_FILT_EVT);
> }
> 
> Right?

In this use case they are functionally the same, yes, but conceptually, they are
not the same thing. spe_pmu->features advertises the features that the SPE
*driver* supports, PMSIDR_EL1 advertises the features that the *hardware*
implements. The two are not always equal.  For example, KVM added handling for
the PMSNEVFR_EL1 register (added by FEAT_SPE_FnE), a few years before the SPE
driver supported FEAT_SPE_FnE.

I can see KVM, in the future, having to virtualize hardware features not
supported by the host driver yet, so unless you have a strong objection, I would
rather keep the use of the architectural register instead of relying on the
driver.

> 
> So it seems you could drop this patch from the series in general? Is there
> another reason to get these fields?
> 
> Also - there seems to be a divergence in the naming there between FnE and
> INV_FILT_EVT, is that intentional?

I think that's a question for the person who added FEAT_PMU_FEAT_INV_FILT_EVT?

Thanks,
Alex