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

[Ricardo Koller <ricarkol@google.com>]

On Thu, Jul 21, 2022 at 02:43:50PM +0100, Marc Zyngier wrote:
> 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).
> 

Got it, thanks.

Which brings me to what to do with this test. Should it be fixed for
bare-metal by ignoring the PMOVSSET check? or should it actually check
for PMOVSSET=1 and fail on KVM until KVM gets fixed?

Thanks,
Ricardo

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