Re: [PATCH 1/1] KVM: arm64: PMU: Avoid inappropriate use of host's PMUVer

[Oliver Upton <oliver.upton@linux.dev>]

On Sat, Jun 10, 2023 at 09:54:30PM -0700, Reiji Watanabe wrote:
> On Sat, Jun 10, 2023 at 05:57:34PM -0700, Oliver Upton wrote:
> > Hi Reiji,
> > 
> > On Sat, Jun 10, 2023 at 12:45:10PM -0700, Reiji Watanabe wrote:
> > > @@ -735,7 +736,7 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
> > >  		 * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
> > >  		 * as RAZ
> > >  		 */
> > > -		if (vcpu->kvm->arch.arm_pmu->pmuver >= ID_AA64DFR0_EL1_PMUVer_V3P4)
> > > +		if (vcpu->kvm->arch.dfr0_pmuver.imp >= ID_AA64DFR0_EL1_PMUVer_V3P4)
> > >  			val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
> > 
> > I don't think this conditional masking is correct in the first place,
> 
> I'm not sure why this conditional masking is correct.
> Could you please elaborate ?

On second thought, the original code works, but for a rather non-obvious
reason. I was concerned about the case where kvm->arch.arm_pmu->pmuver does
not match the current CPU, but as you say we hide PMU from the guest in this
case.

My concern remains, though, for the proposed fix.

> > and this change would only make it worse.
> > 
> > We emulate reads of PMCEID1_EL0 using the literal value of the CPU. The
> > _advertised_ PMU version has no bearing on the core PMU version. So,
> > assuming we hit this on a v3p5+ part with userspace (stupidly)
> > advertising an older implementation level, we never clear the bit for
> > STALL_SLOT.
> 
> I'm not sure if I understand this comment correctly.
> When the guest's PMUVer is older than v3p4, I don't think we need
> to clear the bit for STALL_SLOT, as PMMIR_EL1 is not implemented
> for the guest (PMMIR_EL1 is implemented only on v3p4 or newer).
> Or am I missing something ?

The guest's PMU version has no influence on the *hardware* value of
PMCEID1_EL0.

Suppose KVM is running on a v3p5+ implementation, but userspace has set
ID_AA64DFR0_EL1.PMUVer to v3p0. In this case the read of PMCEID1_EL0 on
the preceding line would advertise the STALL_SLOT event, and KVM fails
to mask it due to the ID register value. The fact we do not support the
event is an invariant, and in the worst case we wind up clearing a bit
that's already 0.

This is why I'd suggested just unconditionally clearing the bit. While
we're on the topic, doesn't the same reasoning hold for
STALL_SLOT_{FRONTEND,BACKEND}? We probably want to hide those too.

--
Thanks,
Oliver