| Message ID | 20250409160106.6445-2-maz@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: arm64: EL2 PMU handling fixes | expand |
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. > + > + /* 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? Thanks, Oliver
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.
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
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); + + /* 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; + } + } } /** diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 005ad28f73068..73d68ea37ac21 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -2698,6 +2698,12 @@ static int set_imp_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r, .set_user = set_imp_id_reg, \ .reset = reset_imp_id_reg, \ .val = mask, \ + } + +static u64 reset_mdcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r) +{ + __vcpu_sys_reg(vcpu, r->reg) = vcpu->kvm->arch.pmcr_n; + return vcpu->kvm->arch.pmcr_n; } /* @@ -3243,7 +3249,7 @@ static const struct sys_reg_desc sys_reg_descs[] = { EL2_REG(SCTLR_EL2, access_rw, reset_val, SCTLR_EL2_RES1), EL2_REG(ACTLR_EL2, access_rw, reset_val, 0), EL2_REG_VNCR(HCR_EL2, reset_hcr, 0), - EL2_REG(MDCR_EL2, access_mdcr, reset_val, 0), + EL2_REG(MDCR_EL2, access_mdcr, reset_mdcr, 0), EL2_REG(CPTR_EL2, access_rw, reset_val, CPTR_NVHE_EL2_RES1), EL2_REG_VNCR(HSTR_EL2, reset_val, 0), EL2_REG_VNCR(HFGRTR_EL2, reset_val, 0),
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(-)