[PATCH v2 0/6] KVM: arm64: EL2 PMU handling fixes

[PATCH v2 0/6] KVM: arm64: EL2 PMU handling fixes

[Marc Zyngier]

Joey reports that some of his PMU tests do not behave quite as
expected:

- MDCR_EL2.HPMN is set to 0 out of reset

- PMCR_EL0.P should reset all the counters when written from EL2

Oliver points out that setting PMCR_EL0.N from userspace by writing to
the register is silly with NV, and that we need a new PMU attribute
instead.

On top of that, I figured out that we had a number of little gotchas:

- It is possible for a guest to write an HPMN value that is out of
  bound, and it seems valuable to limit it

- PMCR_EL0.N should be the maximum number of counters when read from
  EL2, and MDCR_EL2.HPMN when read from EL0/EL1

- Prevent userspace from updating PMCR_EL0.N when EL2 is available

I haven't added any Cc stable, as NV is not functional upstream yet.

* From v1 [1]:

  - Added patch for the new PMU attribute
  - Added fix for HPMN limit
  - Prevent update of PMCR_EL0.N with NV
  - Fix kvm_vcpu_read_pmcr()
  - Rebased on 6.15-rc1

[1] https://lore.kernel.org/r/20250217112412.3963324-1-maz@kernel.org

Marc Zyngier (6):
  KVM: arm64: Fix MDCR_EL2.HPMN reset value
  KVM: arm64: Contextualise the handling of PMCR_EL0.P writes
  KVM: arm64: Allow userspace to limit the number of PMU counters for
    EL2 VMs
  KVM: arm64: Don't let userspace write to PMCR_EL0.N when the vcpu has
    EL2
  KVM: arm64: Handle out-of-bound write to HDCR_EL2.HPMN
  KVM: arm64: Let kvm_vcpu_read_pmcr() return an EL-dependent value for
    PMCR_EL0.N

 Documentation/virt/kvm/devices/vcpu.rst | 24 ++++++++++++
 arch/arm64/include/uapi/asm/kvm.h       |  9 +++--
 arch/arm64/kvm/pmu-emul.c               | 51 ++++++++++++++++++++++---
 arch/arm64/kvm/sys_regs.c               | 43 ++++++++++++++++-----
 4 files changed, 107 insertions(+), 20 deletions(-)

-- 
2.39.2

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

[Marc Zyngier]

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);
+
+	/* 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),
-- 
2.39.2

[PATCH v2 2/6] KVM: arm64: Contextualise the handling of PMCR_EL0.P writes

[Marc Zyngier]

Contrary to what the comment says in kvm_pmu_handle_pmcr(),
writing PMCR_EL0.P==1 has the following effects:

<quote>
The event counters affected by this field are:
  * All event counters in the first range.
  * If any of the following are true, all event counters in the second
    range:
    - EL2 is disabled or not implemented in the current Security state.
    - The PE is executing at EL2 or EL3.
</quote>

where the "first range" represent the counters in the [0..HPMN-1]
range, and the "second range" the counters in the [HPMN..MAX] range.

It so appears that writing P from EL2 should nuke all counters,
and not just the "guest" view. Just do that, and nuke the misleading
comment.

Reported-by: Joey Gouly <joey.gouly@arm.com>
Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/pmu-emul.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 4dc4f3a473c3f..f3650f2f2d468 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -608,14 +608,12 @@ void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val)
 		kvm_pmu_set_counter_value(vcpu, ARMV8_PMU_CYCLE_IDX, 0);
 
 	if (val & ARMV8_PMU_PMCR_P) {
-		/*
-		 * Unlike other PMU sysregs, the controls in PMCR_EL0 always apply
-		 * to the 'guest' range of counters and never the 'hyp' range.
-		 */
 		unsigned long mask = kvm_pmu_implemented_counter_mask(vcpu) &
-				     ~kvm_pmu_hyp_counter_mask(vcpu) &
 				     ~BIT(ARMV8_PMU_CYCLE_IDX);
 
+		if (!vcpu_is_el2(vcpu))
+			mask &= ~kvm_pmu_hyp_counter_mask(vcpu);
+
 		for_each_set_bit(i, &mask, 32)
 			kvm_pmu_set_pmc_value(kvm_vcpu_idx_to_pmc(vcpu, i), 0, true);
 	}
-- 
2.39.2

[PATCH v2 3/6] KVM: arm64: Allow userspace to limit the number of PMU counters for EL2 VMs

[Marc Zyngier]

As long as we had purely EL1 VMs, we could easily update the number
of guest-visible counters by letting userspace write to PMCR_EL0.N.

With VMs started at EL2, PMCR_EL1.N only reflects MDCR_EL2.HPMN,
and we don't have a good way to limit it.

For this purpose, introduce a new PMUv3 attribute that allows
limiting the maximum number of counters. This requires the explicit
selection of a PMU.

Suggested-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 Documentation/virt/kvm/devices/vcpu.rst | 24 ++++++++++++++++++++++++
 arch/arm64/include/uapi/asm/kvm.h       |  9 +++++----
 arch/arm64/kvm/pmu-emul.c               | 24 ++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
index 31a9576c07afa..6eef154a2e396 100644
--- a/Documentation/virt/kvm/devices/vcpu.rst
+++ b/Documentation/virt/kvm/devices/vcpu.rst
@@ -137,6 +137,30 @@ exit_reason = KVM_EXIT_FAIL_ENTRY and populate the fail_entry struct by setting
 hardare_entry_failure_reason field to KVM_EXIT_FAIL_ENTRY_CPU_UNSUPPORTED and
 the cpu field to the processor id.
 
+1.5 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_SET_PMCR_N
+---------------------------------------------
+
+:Parameters: in kvm_device_attr.addr the address to an unsigned int
+	     representing the maximum value taken by PMCR_EL0.N
+
+:Returns:
+
+	 =======  ====================================================
+	 -EBUSY   PMUv3 already initialized, a VCPU has already run or
+                  an event filter has already been set
+	 -EFAULT  Error accessing the value pointed to by addr
+	 -ENODEV  PMUv3 not supported or GIC not initialized
+	 -EINVAL  No PMUv3 explicitly selected, or value of N out of
+	 	  range
+	 =======  ====================================================
+
+Update the maximum value allowed in PMCR_EL0.N, defining the number of
+counters visible to the guest. This mandates that a PMU has
+explicitly been selected via KVM_ARM_VCPU_PMU_V3_SET_PMU, and will
+fail when no PMU has been explicitly selected, or the number of
+counters is out of range for the selected PMU. Selecting a new PMU
+cancels the effect of setting this attribute.
+
 2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
 =================================
 
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index af9d9acaf9975..a1b106780cf9d 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -431,10 +431,11 @@ enum {
 
 /* Device Control API on vcpu fd */
 #define KVM_ARM_VCPU_PMU_V3_CTRL	0
-#define   KVM_ARM_VCPU_PMU_V3_IRQ	0
-#define   KVM_ARM_VCPU_PMU_V3_INIT	1
-#define   KVM_ARM_VCPU_PMU_V3_FILTER	2
-#define   KVM_ARM_VCPU_PMU_V3_SET_PMU	3
+#define   KVM_ARM_VCPU_PMU_V3_IRQ		0
+#define   KVM_ARM_VCPU_PMU_V3_INIT		1
+#define   KVM_ARM_VCPU_PMU_V3_FILTER		2
+#define   KVM_ARM_VCPU_PMU_V3_SET_PMU		3
+#define   KVM_ARM_VCPU_PMU_V3_SET_PMCR_N	4
 #define KVM_ARM_VCPU_TIMER_CTRL		1
 #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
 #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index f3650f2f2d468..01fda19b0c825 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -1099,6 +1099,20 @@ static int kvm_arm_pmu_v3_set_pmu(struct kvm_vcpu *vcpu, int pmu_id)
 	return ret;
 }
 
+static int kvm_arm_pmu_v3_set_pmcr_n(struct kvm_vcpu *vcpu, unsigned int n)
+{
+	struct kvm *kvm = vcpu->kvm;
+
+	if (!kvm->arch.arm_pmu)
+		return -EINVAL;
+
+	if (n > kvm_arm_pmu_get_max_counters(kvm))
+		return -EINVAL;
+
+	kvm->arch.pmcr_n = n;
+	return 0;
+}
+
 int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 {
 	struct kvm *kvm = vcpu->kvm;
@@ -1195,6 +1209,15 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 
 		return kvm_arm_pmu_v3_set_pmu(vcpu, pmu_id);
 	}
+	case KVM_ARM_VCPU_PMU_V3_SET_PMCR_N: {
+		unsigned int __user *uaddr = (unsigned int __user *)(long)attr->addr;
+		unsigned int n;
+
+		if (get_user(n, uaddr))
+			return -EFAULT;
+
+		return kvm_arm_pmu_v3_set_pmcr_n(vcpu, n);
+	}
 	case KVM_ARM_VCPU_PMU_V3_INIT:
 		return kvm_arm_pmu_v3_init(vcpu);
 	}
@@ -1233,6 +1256,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 	case KVM_ARM_VCPU_PMU_V3_INIT:
 	case KVM_ARM_VCPU_PMU_V3_FILTER:
 	case KVM_ARM_VCPU_PMU_V3_SET_PMU:
+	case KVM_ARM_VCPU_PMU_V3_SET_PMCR_N:
 		if (kvm_vcpu_has_pmu(vcpu))
 			return 0;
 	}
-- 
2.39.2

[PATCH v2 4/6] KVM: arm64: Don't let userspace write to PMCR_EL0.N when the vcpu has EL2

[Marc Zyngier]

Now that userspace can provide its limit for hte maximum number of
counters, prevent it from writing to PMCR_EL0.N, as the value should
be derived from MDCR_EL2.HPMN in that case.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 73d68ea37ac21..00b5396492d51 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1216,6 +1216,7 @@ static int set_pmcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r,
 	 * with the existing KVM behavior.
 	 */
 	if (!kvm_vm_has_ran_once(kvm) &&
+	    !vcpu_has_nv(vcpu)	      &&
 	    new_n <= kvm_arm_pmu_get_max_counters(kvm))
 		kvm->arch.pmcr_n = new_n;
 
-- 
2.39.2

[PATCH v2 5/6] KVM: arm64: Handle out-of-bound write to HDCR_EL2.HPMN

[Marc Zyngier]

We don't really pay attention to what gets written to MDCR_EL2.HPMN,
and funky guests could play ugly games on us.

Restrict what gets written there, and limit the number of counters
to what the PMU is allowed to have.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 00b5396492d51..e53b8f82ca7f8 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2571,17 +2571,33 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,
 			struct sys_reg_params *p,
 			const struct sys_reg_desc *r)
 {
-	u64 old = __vcpu_sys_reg(vcpu, MDCR_EL2);
+	if (!p->is_write) {
+		p->regval = __vcpu_sys_reg(vcpu, MDCR_EL2);
+	} else {
+		u64 hpmn = FIELD_GET(MDCR_EL2_HPMN, p->regval);
+		u64 old = __vcpu_sys_reg(vcpu, MDCR_EL2);
+		u64 val = p->regval;
 
-	if (!access_rw(vcpu, p, r))
-		return false;
+		/*
+		 * If HPMN is out of bounds, limit it to what we actually
+		 * support. This matches the UNKNOWN definition of the field
+		 * in that case, and keeps the emulation simple. Sort of.
+		 */
+		if (hpmn > vcpu->kvm->arch.pmcr_n) {
+			hpmn = vcpu->kvm->arch.pmcr_n;
+			u64_replace_bits(val, hpmn, MDCR_EL2_HPMN);
+		}
 
-	/*
-	 * Request a reload of the PMU to enable/disable the counters affected
-	 * by HPME.
-	 */
-	if ((old ^ __vcpu_sys_reg(vcpu, MDCR_EL2)) & MDCR_EL2_HPME)
-		kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
+		vcpu_write_sys_reg(vcpu, val, r->reg);
+
+		/*
+		 * Request a reload of the PMU to enable/disable the
+		 * counters affected by HPME.
+		 */
+
+		if ((old ^ __vcpu_sys_reg(vcpu, MDCR_EL2)) & MDCR_EL2_HPME)
+			kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
+	}
 
 	return true;
 }
-- 
2.39.2

[PATCH v2 6/6] KVM: arm64: Let kvm_vcpu_read_pmcr() return an EL-dependent value for PMCR_EL0.N

[Marc Zyngier]

When EL2 is present, PMCR_EL0.N is the effective value of MDCR_EL2.HPMN
when accessed from EL1 or EL0.

Make sure we honor this requirement.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/pmu-emul.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 01fda19b0c825..f9edd0a2427c2 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -1295,8 +1295,12 @@ u8 kvm_arm_pmu_get_pmuver_limit(void)
 u64 kvm_vcpu_read_pmcr(struct kvm_vcpu *vcpu)
 {
 	u64 pmcr = __vcpu_sys_reg(vcpu, PMCR_EL0);
+	u64 n = vcpu->kvm->arch.pmcr_n;
 
-	return u64_replace_bits(pmcr, vcpu->kvm->arch.pmcr_n, ARMV8_PMU_PMCR_N);
+	if (vcpu_has_nv(vcpu) && !vcpu_is_el2(vcpu))
+		n = FIELD_GET(MDCR_EL2_HPMN, __vcpu_sys_reg(vcpu, MDCR_EL2));
+
+	return u64_replace_bits(pmcr, n, ARMV8_PMU_PMCR_N);
 }
 
 void kvm_pmu_nested_transition(struct kvm_vcpu *vcpu)
-- 
2.39.2

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

[Oliver Upton]

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

Re: [PATCH v2 3/6] KVM: arm64: Allow userspace to limit the number of PMU counters for EL2 VMs

[Oliver Upton]

On Wed, Apr 09, 2025 at 05:01:03PM +0100, Marc Zyngier wrote:
> As long as we had purely EL1 VMs, we could easily update the number
> of guest-visible counters by letting userspace write to PMCR_EL0.N.
> 
> With VMs started at EL2, PMCR_EL1.N only reflects MDCR_EL2.HPMN,
> and we don't have a good way to limit it.
> 
> For this purpose, introduce a new PMUv3 attribute that allows
> limiting the maximum number of counters. This requires the explicit
> selection of a PMU.
> 
> Suggested-by: Oliver Upton <oliver.upton@linux.dev>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  Documentation/virt/kvm/devices/vcpu.rst | 24 ++++++++++++++++++++++++
>  arch/arm64/include/uapi/asm/kvm.h       |  9 +++++----
>  arch/arm64/kvm/pmu-emul.c               | 24 ++++++++++++++++++++++++
>  3 files changed, 53 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index 31a9576c07afa..6eef154a2e396 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> @@ -137,6 +137,30 @@ exit_reason = KVM_EXIT_FAIL_ENTRY and populate the fail_entry struct by setting
>  hardare_entry_failure_reason field to KVM_EXIT_FAIL_ENTRY_CPU_UNSUPPORTED and
>  the cpu field to the processor id.
>  
> +1.5 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_SET_PMCR_N

Similar to my comment on the kvm_arch field, this should be renamed to
something other than PMCR_N.

Otherwise the implementation of the ioctl itself LGTM.

> +---------------------------------------------
> +
> +:Parameters: in kvm_device_attr.addr the address to an unsigned int
> +	     representing the maximum value taken by PMCR_EL0.N
> +
> +:Returns:
> +
> +	 =======  ====================================================
> +	 -EBUSY   PMUv3 already initialized, a VCPU has already run or
> +                  an event filter has already been set
> +	 -EFAULT  Error accessing the value pointed to by addr
> +	 -ENODEV  PMUv3 not supported or GIC not initialized
> +	 -EINVAL  No PMUv3 explicitly selected, or value of N out of
> +	 	  range
> +	 =======  ====================================================
> +
> +Update the maximum value allowed in PMCR_EL0.N, defining the number of
> +counters visible to the guest.

Set the number of implemented event counters in the vPMU

Thanks,
Oliver

Re: [PATCH v2 5/6] KVM: arm64: Handle out-of-bound write to HDCR_EL2.HPMN

[Oliver Upton]

HDCR? I thought you gave up on 32-bit a loooong time ago ;-)

On Wed, Apr 09, 2025 at 05:01:05PM +0100, Marc Zyngier wrote:
> We don't really pay attention to what gets written to MDCR_EL2.HPMN,
> and funky guests could play ugly games on us.
> 
> Restrict what gets written there, and limit the number of counters
> to what the PMU is allowed to have.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/sys_regs.c | 34 +++++++++++++++++++++++++---------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 00b5396492d51..e53b8f82ca7f8 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2571,17 +2571,33 @@ static bool access_mdcr(struct kvm_vcpu *vcpu,
>  			struct sys_reg_params *p,
>  			const struct sys_reg_desc *r)
>  {
> -	u64 old = __vcpu_sys_reg(vcpu, MDCR_EL2);
> +	if (!p->is_write) {
> +		p->regval = __vcpu_sys_reg(vcpu, MDCR_EL2);
> +	} else {

nit: you can do an early return for an emulated read and get rid of a
level of indentation for the write case.

> +		u64 hpmn = FIELD_GET(MDCR_EL2_HPMN, p->regval);
> +		u64 old = __vcpu_sys_reg(vcpu, MDCR_EL2);
> +		u64 val = p->regval;
>  
> -	if (!access_rw(vcpu, p, r))
> -		return false;
> +		/*
> +		 * If HPMN is out of bounds, limit it to what we actually
> +		 * support. This matches the UNKNOWN definition of the field
> +		 * in that case, and keeps the emulation simple. Sort of.
> +		 */
> +		if (hpmn > vcpu->kvm->arch.pmcr_n) {
> +			hpmn = vcpu->kvm->arch.pmcr_n;
> +			u64_replace_bits(val, hpmn, MDCR_EL2_HPMN);
> +		}
>  
> -	/*
> -	 * Request a reload of the PMU to enable/disable the counters affected
> -	 * by HPME.
> -	 */
> -	if ((old ^ __vcpu_sys_reg(vcpu, MDCR_EL2)) & MDCR_EL2_HPME)
> -		kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> +		vcpu_write_sys_reg(vcpu, val, r->reg);
> +
> +		/*
> +		 * Request a reload of the PMU to enable/disable the
> +		 * counters affected by HPME.
> +		 */
> +
> +		if ((old ^ __vcpu_sys_reg(vcpu, MDCR_EL2)) & MDCR_EL2_HPME)
> +			kvm_make_request(KVM_REQ_RELOAD_PMU, vcpu);
> +	}
>  
>  	return true;
>  }
> -- 
> 2.39.2
> 

Thanks,
Oliver

Re: [PATCH v2 0/6] KVM: arm64: EL2 PMU handling fixes

[Oliver Upton]

On Wed, Apr 09, 2025 at 05:01:00PM +0100, Marc Zyngier wrote:
> Joey reports that some of his PMU tests do not behave quite as
> expected:
> 
> - MDCR_EL2.HPMN is set to 0 out of reset
> 
> - PMCR_EL0.P should reset all the counters when written from EL2
> 
> Oliver points out that setting PMCR_EL0.N from userspace by writing to
> the register is silly with NV, and that we need a new PMU attribute
> instead.
> 
> On top of that, I figured out that we had a number of little gotchas:
> 
> - It is possible for a guest to write an HPMN value that is out of
>   bound, and it seems valuable to limit it
> 
> - PMCR_EL0.N should be the maximum number of counters when read from
>   EL2, and MDCR_EL2.HPMN when read from EL0/EL1
> 
> - Prevent userspace from updating PMCR_EL0.N when EL2 is available
> 
> I haven't added any Cc stable, as NV is not functional upstream yet.

Similarly, I don't see a compelling reason for this to go in as a fix
for 6.15, especially since you're adding new UAPI. Do you mind grabbing
it for the next merge window?

With the comments addressed:

Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thanks,
Oliver

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

[Marc Zyngier]

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.

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

[Oliver Upton]

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

Re: [PATCH v2 0/6] KVM: arm64: EL2 PMU handling fixes

[Marc Zyngier]

On Wed, 09 Apr 2025 21:31:31 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Wed, Apr 09, 2025 at 05:01:00PM +0100, Marc Zyngier wrote:
> > Joey reports that some of his PMU tests do not behave quite as
> > expected:
> > 
> > - MDCR_EL2.HPMN is set to 0 out of reset
> > 
> > - PMCR_EL0.P should reset all the counters when written from EL2
> > 
> > Oliver points out that setting PMCR_EL0.N from userspace by writing to
> > the register is silly with NV, and that we need a new PMU attribute
> > instead.
> > 
> > On top of that, I figured out that we had a number of little gotchas:
> > 
> > - It is possible for a guest to write an HPMN value that is out of
> >   bound, and it seems valuable to limit it
> > 
> > - PMCR_EL0.N should be the maximum number of counters when read from
> >   EL2, and MDCR_EL2.HPMN when read from EL0/EL1
> > 
> > - Prevent userspace from updating PMCR_EL0.N when EL2 is available
> > 
> > I haven't added any Cc stable, as NV is not functional upstream yet.
> 
> Similarly, I don't see a compelling reason for this to go in as a fix
> for 6.15, especially since you're adding new UAPI. Do you mind grabbing
> it for the next merge window?
> 
> With the comments addressed:
> 
> Reviewed-by: Oliver Upton <oliver.upton@linux.dev>

Thanks. I'll repost the whole thing next week and queue it for 6.16.

	M.

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