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

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

On Thu, Apr 10, 2025 at 11:54:59AM +0100, Marc Zyngier wrote:
> 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.

Ah, right. Default PMU is set at KVM_ARM_VCPU_INIT, so any race that
comes afterwards would be the fault of userspace.

Fine with it as is then.

Thanks,
Oliver