Re: [RFC PATCH v2 2/4] perf: arm_pmuv3: Introduce module param to partition the PMU

[Colton Lewis]

Hi Oliver, thanks for the review.

Oliver Upton <oliver.upton@xxxxxxxxx> writes:

> Hi Colton,

> On Sat, Feb 08, 2025 at 02:01:09AM +0000, Colton Lewis wrote:
> > For PMUv3, the register MDCR_EL2.HPMN partitiones the PMU counters
> > into two ranges where counters 0..HPMN-1 are accessible by EL1 and, if
> > allowed, EL0 while counters HPMN..N are only accessible by EL2.

> > Introduce a module parameter in the PMUv3 driver to set this
> > register. The name reserved_host_counters reflects the intent to
> > reserve some counters for the host so the guest may eventually be
> > allowed direct access to a subset of PMU functionality for increased
> > performance.

> > Track HPMN and whether the pmu is partitioned in struct arm_pmu
> > because KVM will need to know that to handle guests correctly.

> > While FEAT_HPMN0 does allow HPMN to be set to 0, this patch
> > specifically disallows that case because it's not useful given the
> > intention to allow guests access to their own counters.

> Quite the contrary.

> FEAT_HPMN0 is useful if userspace wants to provide a vPMU that has a
> fixed cycle counter w/o event counters. Certain OSes refuse to boot
> without it...

Cool. I'll make sure writing 0 is allowed if we have FEAT_HPMN0.

> > static inline u32 read_mdcr(void)
> > {
> > 	return read_sysreg(HDCR);
> > }

> > static inline void write_mdcr(u32 val)
> > {
> > 	write_sysreg(val, HDCR);
> > }


> Hmm... While this fixes the 32bit compilation issues, it opens a new can
> of worms.

> VHE is a 64bit only feature, so you're *guaranteed* that these accessors
> will undef (running at EL1).

True. I mentioned in the cover letter they aren't intended to actually
run in the PMU driver because I guard for VHE there.

> > +static void armv8pmu_partition(u8 hpmn)
> > +{
> > +	u64 mdcr = armv8pmu_mdcr_read();
> > +
> > +	mdcr &= ~ARMV8_PMU_MDCR_HPMN;
> > +	mdcr |= FIELD_PREP(ARMV8_PMU_MDCR_HPMN, hpmn);
> > +	/* Prevent guest counters counting at EL2 */
> > +	mdcr |= ARMV8_PMU_MDCR_HPMD;
> > +
> > +	armv8pmu_mdcr_write(mdcr);
> > +}
> > +

> After giving this a read, I don't think the host PMU driver should care
> about MDCR_EL2 at all. The only time that 'guest' events are loaded into
> the PMU is between vcpu_load() / vcpu_put(), at which point *KVM* has
> reconfigured MDCR_EL2.

> I'm not sure if there's much involvement with the host PMU driver beyond
> it pinky-swearing to only use the specified counters. KVM is what
> actually will program HPMN.

> That'd alleviate the PMU driver from having VHE awareness or caring
> about MDCR_EL2.

That does seem like a cleaner solution. I can lift that handling into
KVM code, but I will still need the variables I introduced in struct
arm_pmu to be populated so the driver knows what counters to use.

Without looking, I'm concerned there might be an initilization ordering
issue with that where KVM will get the value for HPMN first because it's
initialized first but then can't store them somewhere accessible to the
driver because the driver hasn't been initialized yet. Then the driver
is initialized and used without knowing HPMN and is now using counters
we wanted the guest to have.

There is probably an easy way around this, but I'll need to do some
digging.