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

[Marc Zyngier <maz@kernel.org>]

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.

Thanks,

	M.

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