Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests

From: Marc Zyngier <maz@kernel.org>
To: Ricardo Koller <ricarkol@google.com>
Cc: drjones@redhat.com, kvm@vger.kernel.org, kvmarm@lists.cs.columbia.edu
Subject: Re: [kvm-unit-tests PATCH 3/3] arm: pmu: Remove checks for !overflow in chained counters tests
Date: Thu, 21 Jul 2022 14:43:50 +0100	[thread overview]
Message-ID: <871quezill.wl-maz@kernel.org> (raw)
In-Reply-To: <Ythy8XXN2rFytXdr@google.com>

On Wed, 20 Jul 2022 22:26:09 +0100,
Ricardo Koller <ricarkol@google.com> wrote:
> 
> On Wed, Jul 20, 2022 at 02:17:09PM -0700, Ricardo Koller wrote:
> > On Wed, Jul 20, 2022 at 10:45:20AM +0100, Marc Zyngier wrote:
> > > On Wed, 20 Jul 2022 09:40:01 +0100,
> > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > 
> > > > On Tue, Jul 19, 2022 at 12:34:05PM +0100, Marc Zyngier wrote:
> > > > > On Mon, 18 Jul 2022 16:49:10 +0100,
> > > > > Ricardo Koller <ricarkol@google.com> wrote:
> > > > > > 
> > > > > > A chained event overflowing on the low counter can set the overflow flag
> > > > > > in PMOVS.  KVM does not set it, but real HW and the fast-model seem to.
> > > > > > Moreover, the AArch64.IncrementEventCounter() pseudocode in the ARM ARM
> > > > > > (DDI 0487H.a, J1.1.1 "aarch64/debug") also sets the PMOVS bit on
> > > > > > overflow.
> > > > > 
> > > > > Isn't this indicative of a bug in the KVM emulation? To be honest, the
> > > > > pseudocode looks odd. It says:
> > > > > 
> > > > > <quote>
> > > > > 	if old_value<64:ovflw> != new_value<64:ovflw> then
> > > > > 	    PMOVSSET_EL0<idx> = '1';
> > > > > 	    PMOVSCLR_EL0<idx> = '1';
> > > > > </quote>
> > > > > 
> > > > > which I find remarkably ambiguous. Is this setting and clearing the
> > > > > overflow bit? Or setting it in the single register that backs the two
> > > > > accessors in whatever way it can?
> > > > > 
> > > > > If it is the second interpretation that is correct, then KVM
> > > > > definitely needs fixing
> > > > 
> > > > I think it's the second, as those two "= '1'" apply to the non-chained
> > > > counters case as well, which should definitely set the bit in PMOVSSET.
> > > > 
> > > > > (though this looks pretty involved for
> > > > > anything that isn't a SWINC event).
> > > > 
> > > > Ah, I see, there's a pretty convenient kvm_pmu_software_increment() for
> > > > SWINC, but a non-SWINC event is implemented as a single 64-bit perf
> > > > event.
> > > 
> > > Indeed. Which means we need to de-optimise chained counters to being
> > > 32bit events, which is pretty annoying (for rapidly firing events, the
> > > interrupt rate is going to be significantly higher).
> > > 
> > > I guess we should also investigate the support for FEAT_PMUv3p5 and
> > > native 64bit counters. Someone is bound to build it at some point.
> > 
> > The kernel perf event is implementing 64-bit counters using chained
> > counters. I assume this is already firing an interrupt for the low
> > counter overflow; we might need to just hook into that, investigating...

We probably only enable the overflow interrupt on the odd counter, and
not the even one (which is why you request chained counters the first
place).

And perf wouldn't call us back anyway, as we described the counter as
64bit.

> Additionally, given that the kernel is already emulating 64-bit
> counters, can KVM just expose FEAT_PMUv3p5? Assuming all the other new
> features can be emulated.

This is what I suggested above. Although it can only happen on a
system that already supports FEAT_PMU3p4, as PMMIR_EL1 is not defined
before that (and FEAT_PMU3p5 implies 3p4).

It also remains that we need to *properly* emulate chained counters,
which means not handling them as 64bit counters in perf, but as a
32bit counter and a carry (exactly like the pseudocode does).

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm