Re: [PATCH v2 1/6] KVM: arm64: Fix MDCR_EL2.HPMN reset value

[Marc Zyngier <maz@kernel.org>]

On Wed, 09 Apr 2025 21:21:33 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Wed, Apr 09, 2025 at 05:01:01PM +0100, Marc Zyngier wrote:
> > The MDCR_EL2 documentation indicates that the HPMN field has
> > the following behaviour:
> > 
> > "On a Warm reset, this field resets to the expression NUM_PMU_COUNTERS."
> > 
> > However, it appears we reset it to zero, which is not very useful.
> > 
> > Add a reset helper for MDCR_EL2, and handle the case where userspace
> > changes the target PMU, which may force us to change HPMN again.
> > 
> > Reported-by: Joey Gouly <joey.gouly@arm.com>
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/pmu-emul.c | 13 +++++++++++++
> >  arch/arm64/kvm/sys_regs.c |  8 +++++++-
> >  2 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> > index a1bc10d7116a5..4dc4f3a473c3f 100644
> > --- a/arch/arm64/kvm/pmu-emul.c
> > +++ b/arch/arm64/kvm/pmu-emul.c
> > @@ -1033,6 +1033,19 @@ static void kvm_arm_set_pmu(struct kvm *kvm, struct arm_pmu *arm_pmu)
> >  
> >  	kvm->arch.arm_pmu = arm_pmu;
> >  	kvm->arch.pmcr_n = kvm_arm_pmu_get_max_counters(kvm);
> 
> nit: Can we rename pmcr_n to nr_pmu_counters? The current name is misleading.

Fair enough.

> > +
> > +	/* Reset MDCR_EL2.HPMN behind the vcpus' back... */
> > +	if (test_bit(KVM_ARM_VCPU_HAS_EL2, kvm->arch.vcpu_features)) {
> > +		struct kvm_vcpu *vcpu;
> > +		unsigned long i;
> > +
> > +		kvm_for_each_vcpu(i, vcpu, kvm) {
> > +			u64 val = __vcpu_sys_reg(vcpu, MDCR_EL2);
> > +			val &= ~MDCR_EL2_HPMN;
> > +			val |= FIELD_PREP(MDCR_EL2_HPMN, kvm->arch.pmcr_n);
> > +			__vcpu_sys_reg(vcpu, MDCR_EL2) = val;
> > +		}
> 
> Shouldn't we be taking the vCPU mutex(es) here?

If we needed to, it shouldn't be here. We hold the config_lock at this
point, and taking a vcpu mutex would result in a locking inversion.

One option is to punt this to a request, but that makes the updated
HPMN un-observable from userspace until the vcpu has run. This already
affects the default PMU, btw, since it is only assigned on first run.

I'm also not convinced racing against userspace is a big problem here.

	M.

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