Re: [PATCH v2 8/8] drivers/perf: Add Apple icestorm/firestorm CPU PMU driver

[Marc Zyngier <maz@kernel.org>]

On Thu, 02 Dec 2021 16:14:01 +0000,
Mark Rutland <mark.rutland@arm.com> wrote:
> 
> On Thu, Dec 02, 2021 at 03:39:46PM +0000, Marc Zyngier wrote:
> > On Wed, 01 Dec 2021 16:58:10 +0000,
> > Mark Rutland <mark.rutland@arm.com> wrote:
> > > 
> > > On Wed, Dec 01, 2021 at 01:49:09PM +0000, Marc Zyngier wrote:
> > > > Add a new, weird and wonderful driver for the equally weird Apple
> > > > PMU HW. Although the PMU itself is functional, we don't know much
> > > > about the events yet, so this can be considered as yet another
> > > > random number generator...
> > > 
> > > It's really frustrating that Apple built this rather than the
> > > architected PMU, because we've generally pushed back on
> > > IMPLEMENTATION DEFINED junk in this area, and supporting this makes
> > > it harder to push back on other vendors going the same route, which
> > > I'm not keen on. That, and the usual state of IMP-DEF stuff making
> > > this stupidly painful to reason about.
> > 
> > As much as I agree with you on the stinking aspect of an IMPDEF PMU,
> > this doesn't contradicts the architecture. To avoid the spread of this
> > madness, forbidding an IMPDEF implementation in the architecture would
> > be the right thing to do.
> 
> Yeah; I'll see what I can do. ;)
> 
> > > I can see that we can get this working bare-metal with DT, but I
> > > really don't want to try to support this in other cases (e.g. in a
> > > VM, potentially with ACPI), or this IMP-DEFness is going to spread
> > > more throughout the arm_pmu code.
> > 
> > Well, an alternative would be to sidestep the arm_pmu framework
> > altogether.  Which would probably suck even more.
> > 
> > > How does this interact with PMU emulation for a KVM guest?
> > 
> > It doesn't. No non-architected PMU will get exposed to a KVM guest,
> > and the usual "inject an UNDEF exception on IMPDEF access" applies. As
> > far as I am concerned, KVM is purely architectural and doesn't need to
> > be encumbered with this.
> 
> Cool; I think not exposing this into a VM rules out the other issues I
> was concerned with, so as long as we're ruling that out I think we're
> agreed (and I see no reason for us to try to force this platform to work
> with ACPI on bare-metal).

Nah. This is a tortuous enough system.

> > No, there is a single, per-counter control for EL0 and EL2. I couldn't
> > get the counters to report anything useful while a guest was running,
> > but that doesn't mean such control doesn't exist.
> 
> Ok. We might need to require the exclude_guest flag for now, assuming
> the perf tool automatically sets that.

OK.

> 
> [...]
> 
> > > > +	state = read_sysreg_s(SYS_IMP_APL_PMCR0_EL1);
> > > > +	overflow = read_sysreg_s(SYS_IMP_APL_PMSR_EL1);
> > > 
> > > I assume the overflow behaviour is free-running rather than stopping?
> > 
> > Configurable, apparently. At the moment, I set it to stop on overflow.
> > Happy to change the behaviour though.
> 
> The architected PMU continues counting upon overflow (which prevents
> losing counts around the overlflow occurring), so I'd prefer that.
> 
> Is that behaviour per-counter, or for the PMU as a whole?

It is global. This will probably require some additional rework to
clear bit 47 in overflowing counters, which we can't do atomically.

> 
> [...]
> 
> > > > +static int m1_pmu_device_probe(struct platform_device *pdev)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = arm_pmu_device_probe(pdev, m1_pmu_of_device_ids, NULL);
> > > > +	if (!ret) {
> > > > +		/*
> > > > +		 * If probe succeeds, taint the kernel as this is all
> > > > +		 * undocumented, implementation defined black magic.
> > > > +		 */
> > > > +		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
> > > > +	}
> > > > +
> > > > +	return ret;
> > > > +}
> > > 
> > > Hmmm... that means we're always going to TAINT on this HW with an appropriate
> > > DT, which could mask other reasons TAINT_CPU_OUT_OF_SPEC would be set, even
> > > where the user isn't using the PMU.
> > > 
> > > Maybe we should have a cmdline option to opt-in to using the IMP-DEF PMU (and
> > > only tainting in that case)?
> > 
> > I'd rather taint on first use. Requiring a command-line argument for
> > this seems a bit over the top...
> 
> That does sound nicer.
> 
> That said, if we've probed the thing, we're going to be poking it to
> reset it (including out of idle), even if the user hasn't tried to use
> it, so I'm not sure what's best after all...

Yup, there is that. I'll have another look.

	M.

-- 
Without deviation from the norm, progress is not possible.