KVM: arm64: More PMU/debug ID register fixes

---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/kvm/sys_regs.c +1 -1

index 3313dedfa505..0c0832472c4a 100644
@@ -1711,7 +1711,7 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,
 		p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) |
 			     (((dfr >> ID_AA64DFR0_BRPS_SHIFT) & 0xf) << 24) |
 			     (((dfr >> ID_AA64DFR0_CTX_CMPS_SHIFT) & 0xf) << 20)
-			     | (6 << 16) | (el3 << 14) | (el3 << 12));
+			     | (6 << 16) | (1 << 15) | (el3 << 14) | (el3 << 12));
 		return true;
 	}
 }

Signature · 6 lines

2.29.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Alexandru Elisei

Reviewed-by on v2 1/7

2021-01-26 17:33 UTC #2 lore raw

Re: [PATCH v2 1/7] KVM: arm64: Fix missing RES1 in emulation of DBGBIDR

Hi Marc,

The patch looks correct to me. Just to be on the safe side, I had a look at how the other fields are generated and they match the architecture:

Quoted 15 lines · Marc Zyngier

The AArch32 CP14 DBGDIDR has bit 15 set to RES1, which our current emulation doesn't set. Just add the missing bit.
Reported-by Peter Maydell <[email protected]>
Reviewed-by Eric Auger <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/kvm/sys_regs.c +0 -0
index 3313dedfa505..0c0832472c4a 100644

arch/arm64/kvm/sys_regs.c static bool trap_dbgidr(struct kvm_vcpu *vcpu, · line 1717

Quoted 8 lines

arch/arm64/kvm/sys_regs.c +1 -1

@@ -1711,7 +1711,7 @@ static bool trap_dbgidr(struct kvm_vcpu *vcpu,
 		p->regval = ((((dfr >> ID_AA64DFR0_WRPS_SHIFT) & 0xf) << 28) |
 			     (((dfr >> ID_AA64DFR0_BRPS_SHIFT) & 0xf) << 24) |
 			     (((dfr >> ID_AA64DFR0_CTX_CMPS_SHIFT) & 0xf) << 20)
-			     | (6 << 16) | (el3 << 14) | (el3 << 12));
+			     | (6 << 16) | (1 << 15) | (el3 << 14) | (el3 << 12));
 		return true;
 	}
 }

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-01-25 12:26 UTC #3 lore raw

PATCH v2 2/7 KVM: arm64: Fix AArch32 PMUv3 capping

We shouldn't expose *any* PMU capability when no PMU has been configured for this VM.

---
 arch/arm64/kvm/sys_regs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/arm64/kvm/sys_regs.c +2 -2

index 0c0832472c4a..ce08d28ab15c 100644
@@ -1048,8 +1048,8 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 	} else if (id == SYS_ID_DFR0_EL1) {
 		/* Limit guests to PMUv3 for ARMv8.1 */
 		val = cpuid_feature_cap_perfmon_field(val,
-						ID_DFR0_PERFMON_SHIFT,
-						ID_DFR0_PERFMON_8_1);
+						      ID_DFR0_PERFMON_SHIFT,
+						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_1 : 0);
 	}
 
 	return val;

Signature · 6 lines

2.29.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Alexandru Elisei

Reviewed-by on v2 2/7

2021-01-26 17:35 UTC #4 lore raw

Re: [PATCH v2 2/7] KVM: arm64: Fix AArch32 PMUv3 capping

Hi Marc,

This matches what we do for ID_AA64DFR0_EL1:

Quoted 14 lines · Marc Zyngier

We shouldn't expose *any* PMU capability when no PMU has been configured for this VM.
Reviewed-by Eric Auger <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/sys_regs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
arch/arm64/kvm/sys_regs.c +0 -0
index 0c0832472c4a..ce08d28ab15c 100644

arch/arm64/kvm/sys_regs.c static u64 read_id_reg(const struct kvm_vcpu *vcpu, · line 1055

Quoted 10 lines

arch/arm64/kvm/sys_regs.c +2 -2

@@ -1048,8 +1048,8 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 	} else if (id == SYS_ID_DFR0_EL1) {
 		/* Limit guests to PMUv3 for ARMv8.1 */
 		val = cpuid_feature_cap_perfmon_field(val,
-						ID_DFR0_PERFMON_SHIFT,
-						ID_DFR0_PERFMON_8_1);
+						      ID_DFR0_PERFMON_SHIFT,
+						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_1 : 0);
 	}
 
 	return val;

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-01-25 12:26 UTC #5 lore raw

PATCH v2 3/7 KVM: arm64: Add handling of AArch32 PCMEID{2,3} PMUv3 registers

Despite advertising support for AArch32 PMUv3p1, we fail to handle the PMCEID{2,3} registers, which conveniently alias with the top bits of PMCEID{0,1}_EL1.

Implement these registers with the usual AA32(HI/LO) aliasing mechanism.

---
 arch/arm64/kvm/sys_regs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

arch/arm64/kvm/sys_regs.c +9 -3

index ce08d28ab15c..2bea0494b81d 100644
@@ -685,14 +685,18 @@ static bool access_pmselr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 			  const struct sys_reg_desc *r)
 {
-	u64 pmceid;
+	u64 pmceid, mask, shift;
 
 	BUG_ON(p->is_write);
 
 	if (pmu_access_el0_disabled(vcpu))
 		return false;
 
+	get_access_mask(r, &mask, &shift);
+
 	pmceid = kvm_pmu_get_pmceid(vcpu, (p->Op2 & 1));
+	pmceid &= mask;
+	pmceid >>= shift;
 
 	p->regval = pmceid;

@@ -1895,8 +1899,8 @@ static const struct sys_reg_desc cp15_regs[] = {
 	{ Op1( 0), CRn( 9), CRm(12), Op2( 3), access_pmovs },
 	{ Op1( 0), CRn( 9), CRm(12), Op2( 4), access_pmswinc },
 	{ Op1( 0), CRn( 9), CRm(12), Op2( 5), access_pmselr },
-	{ Op1( 0), CRn( 9), CRm(12), Op2( 6), access_pmceid },
-	{ Op1( 0), CRn( 9), CRm(12), Op2( 7), access_pmceid },
+	{ AA32(LO), Op1( 0), CRn( 9), CRm(12), Op2( 6), access_pmceid },
+	{ AA32(LO), Op1( 0), CRn( 9), CRm(12), Op2( 7), access_pmceid },
 	{ Op1( 0), CRn( 9), CRm(13), Op2( 0), access_pmu_evcntr },
 	{ Op1( 0), CRn( 9), CRm(13), Op2( 1), access_pmu_evtyper },
 	{ Op1( 0), CRn( 9), CRm(13), Op2( 2), access_pmu_evcntr },

@@ -1904,6 +1908,8 @@ static const struct sys_reg_desc cp15_regs[] = {
 	{ Op1( 0), CRn( 9), CRm(14), Op2( 1), access_pminten },
 	{ Op1( 0), CRn( 9), CRm(14), Op2( 2), access_pminten },
 	{ Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs },
+	{ AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 4), access_pmceid },
+	{ AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 5), access_pmceid },
 
 	/* PRRR/MAIR0 */
 	{ AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, MAIR_EL1 },

Signature · 6 lines

2.29.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-01-25 12:26 UTC #6 lore raw

PATCH v2 4/7 KVM: arm64: Refactor filtering of ID registers

Our current ID register filtering is starting to be a mess of if() statements, and isn't going to get any saner.

Let's turn it into a switch(), which has a chance of being more readable, and introduce a FEATURE() macro that allows easy generation of feature masks.

No functionnal change intended.

---
 arch/arm64/kvm/sys_regs.c | 51 +++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

arch/arm64/kvm/sys_regs.c +28 ~~-23~~

index 2bea0494b81d..dda16d60197b 100644
@@ -9,6 +9,7 @@
  *          Christoffer Dall <[email protected]>
  */
 
+#include <linux/bitfield.h>
 #include <linux/bsearch.h>
 #include <linux/kvm_host.h>
 #include <linux/mm.h>

@@ -1016,6 +1017,8 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+#define FEATURE(x)	(GENMASK_ULL(x##_SHIFT + 3, x##_SHIFT))
+
 /* Read a sanitised cpufeature ID register by sys_reg_desc */
 static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 		struct sys_reg_desc const *r, bool raz)

@@ -1024,36 +1027,38 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
 	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
 
-	if (id == SYS_ID_AA64PFR0_EL1) {
+	switch (id) {
+	case SYS_ID_AA64PFR0_EL1:
 		if (!vcpu_has_sve(vcpu))
-			val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
-		val &= ~(0xfUL << ID_AA64PFR0_AMU_SHIFT);
-		val &= ~(0xfUL << ID_AA64PFR0_CSV2_SHIFT);
-		val |= ((u64)vcpu->kvm->arch.pfr0_csv2 << ID_AA64PFR0_CSV2_SHIFT);
-		val &= ~(0xfUL << ID_AA64PFR0_CSV3_SHIFT);
-		val |= ((u64)vcpu->kvm->arch.pfr0_csv3 << ID_AA64PFR0_CSV3_SHIFT);
-	} else if (id == SYS_ID_AA64PFR1_EL1) {
-		val &= ~(0xfUL << ID_AA64PFR1_MTE_SHIFT);
-	} else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
-		val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
-			 (0xfUL << ID_AA64ISAR1_API_SHIFT) |
-			 (0xfUL << ID_AA64ISAR1_GPA_SHIFT) |
-			 (0xfUL << ID_AA64ISAR1_GPI_SHIFT));
-	} else if (id == SYS_ID_AA64DFR0_EL1) {
-		u64 cap = 0;
-
+			val &= ~FEATURE(ID_AA64PFR0_SVE);
+		val &= ~FEATURE(ID_AA64PFR0_AMU);
+		val &= ~FEATURE(ID_AA64PFR0_CSV2);
+		val |= FIELD_PREP(FEATURE(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2);
+		val &= ~FEATURE(ID_AA64PFR0_CSV3);
+		val |= FIELD_PREP(FEATURE(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3);
+		break;
+	case SYS_ID_AA64PFR1_EL1:
+		val &= ~FEATURE(ID_AA64PFR1_MTE);
+		break;
+	case SYS_ID_AA64ISAR1_EL1:
+		if (!vcpu_has_ptrauth(vcpu))
+			val &= ~(FEATURE(ID_AA64ISAR1_APA) |
+				 FEATURE(ID_AA64ISAR1_API) |
+				 FEATURE(ID_AA64ISAR1_GPA) |
+				 FEATURE(ID_AA64ISAR1_GPI));
+		break;
+	case SYS_ID_AA64DFR0_EL1:
 		/* Limit guests to PMUv3 for ARMv8.1 */
-		if (kvm_vcpu_has_pmu(vcpu))
-			cap = ID_AA64DFR0_PMUVER_8_1;
-
 		val = cpuid_feature_cap_perfmon_field(val,
-						ID_AA64DFR0_PMUVER_SHIFT,
-						cap);
-	} else if (id == SYS_ID_DFR0_EL1) {
+						      ID_AA64DFR0_PMUVER_SHIFT,
+						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0);
+		break;
+	case SYS_ID_DFR0_EL1:
 		/* Limit guests to PMUv3 for ARMv8.1 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_DFR0_PERFMON_SHIFT,
 						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_1 : 0);
+		break;
 	}
 
 	return val;

Signature · 6 lines

2.29.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Alexandru Elisei

Reviewed-by on v2 4/7

2021-01-27 12:12 UTC #7 lore raw

Re: [PATCH v2 4/7] KVM: arm64: Refactor filtering of ID registers

Hi Marc,

Quoted 56 lines · Marc Zyngier

Our current ID register filtering is starting to be a mess of if() statements, and isn't going to get any saner.

Let's turn it into a switch(), which has a chance of being more readable, and introduce a FEATURE() macro that allows easy generation of feature masks.

No functionnal change intended.

---
 arch/arm64/kvm/sys_regs.c | 51 +++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

arch/arm64/kvm/sys_regs.c +5 ~~-13~~

index 2bea0494b81d..dda16d60197b 100644
@@ -9,6 +9,7 @@
  *          Christoffer Dall <[email protected]>
  */
 
+#include <linux/bitfield.h>
 #include <linux/bsearch.h>
 #include <linux/kvm_host.h>
 #include <linux/mm.h>

@@ -1016,6 +1017,8 @@ static bool access_arch_timer(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+#define FEATURE(x)	(GENMASK_ULL(x##_SHIFT + 3, x##_SHIFT))
+
 /* Read a sanitised cpufeature ID register by sys_reg_desc */
 static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 		struct sys_reg_desc const *r, bool raz)

@@ -1024,36 +1027,38 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 			 (u32)r->CRn, (u32)r->CRm, (u32)r->Op2);
 	u64 val = raz ? 0 : read_sanitised_ftr_reg(id);
 
-	if (id == SYS_ID_AA64PFR0_EL1) {
+	switch (id) {
+	case SYS_ID_AA64PFR0_EL1:
 		if (!vcpu_has_sve(vcpu))
-			val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
-		val &= ~(0xfUL << ID_AA64PFR0_AMU_SHIFT);
-		val &= ~(0xfUL << ID_AA64PFR0_CSV2_SHIFT);
-		val |= ((u64)vcpu->kvm->arch.pfr0_csv2 << ID_AA64PFR0_CSV2_SHIFT);
-		val &= ~(0xfUL << ID_AA64PFR0_CSV3_SHIFT);
-		val |= ((u64)vcpu->kvm->arch.pfr0_csv3 << ID_AA64PFR0_CSV3_SHIFT);
-	} else if (id == SYS_ID_AA64PFR1_EL1) {
-		val &= ~(0xfUL << ID_AA64PFR1_MTE_SHIFT);
-	} else if (id == SYS_ID_AA64ISAR1_EL1 && !vcpu_has_ptrauth(vcpu)) {
-		val &= ~((0xfUL << ID_AA64ISAR1_APA_SHIFT) |
-			 (0xfUL << ID_AA64ISAR1_API_SHIFT) |
-			 (0xfUL << ID_AA64ISAR1_GPA_SHIFT) |

arch/arm64/kvm/sys_regs.c static u64 read_id_reg(const struct kvm_vcpu *vcpu, · line 1038

Quoted 10 lines

arch/arm64/kvm/sys_regs.c +6 -4

-			 (0xfUL << ID_AA64ISAR1_GPI_SHIFT));
-	} else if (id == SYS_ID_AA64DFR0_EL1) {
-		u64 cap = 0;
-
+			val &= ~FEATURE(ID_AA64PFR0_SVE);
+		val &= ~FEATURE(ID_AA64PFR0_AMU);
+		val &= ~FEATURE(ID_AA64PFR0_CSV2);
+		val |= FIELD_PREP(FEATURE(ID_AA64PFR0_CSV2), (u64)vcpu->kvm->arch.pfr0_csv2);
+		val &= ~FEATURE(ID_AA64PFR0_CSV3);
+		val |= FIELD_PREP(FEATURE(ID_AA64PFR0_CSV3), (u64)vcpu->kvm->arch.pfr0_csv3);

The patch ends up looking really nice (checked that the previous behaviour is preserved). It also ends up making the code more robust because we make sure that we only do bitwise or on the first 4 bits of pfr0_csv2 and pfr0_csv3:

Quoted 22 lines

arch/arm64/kvm/sys_regs.c +14 -6

+		break;
+	case SYS_ID_AA64PFR1_EL1:
+		val &= ~FEATURE(ID_AA64PFR1_MTE);
+		break;
+	case SYS_ID_AA64ISAR1_EL1:
+		if (!vcpu_has_ptrauth(vcpu))
+			val &= ~(FEATURE(ID_AA64ISAR1_APA) |
+				 FEATURE(ID_AA64ISAR1_API) |
+				 FEATURE(ID_AA64ISAR1_GPA) |
+				 FEATURE(ID_AA64ISAR1_GPI));
+		break;
+	case SYS_ID_AA64DFR0_EL1:
 		/* Limit guests to PMUv3 for ARMv8.1 */
-		if (kvm_vcpu_has_pmu(vcpu))
-			cap = ID_AA64DFR0_PMUVER_8_1;
-
 		val = cpuid_feature_cap_perfmon_field(val,
-						ID_AA64DFR0_PMUVER_SHIFT,
-						cap);
-	} else if (id == SYS_ID_DFR0_EL1) {
+						      ID_AA64DFR0_PMUVER_SHIFT,
+						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0);

arch/arm64/kvm/sys_regs.c static u64 read_id_reg(const struct kvm_vcpu *vcpu, · line 1064

Quoted 10 lines

arch/arm64/kvm/sys_regs.c +3 -0

+		break;
+	case SYS_ID_DFR0_EL1:
 		/* Limit guests to PMUv3 for ARMv8.1 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_DFR0_PERFMON_SHIFT,
 						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_1 : 0);
+		break;
 	}
 
 	return val;

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-01-25 12:26 UTC #8 lore raw

PATCH v2 5/7 KVM: arm64: Limit the debug architecture to ARMv8.0

Let's not pretend we support anything but ARMv8.0 as far as the debug architecture is concerned.

---
 arch/arm64/kvm/sys_regs.c | 3 +++
 1 file changed, 3 insertions(+)

arch/arm64/kvm/sys_regs.c +3 -0

index dda16d60197b..8f79ec1fffa7 100644
@@ -1048,6 +1048,9 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 				 FEATURE(ID_AA64ISAR1_GPI));
 		break;
 	case SYS_ID_AA64DFR0_EL1:
+		/* Limit debug to ARMv8.0 */
+		val &= ~FEATURE(ID_AA64DFR0_DEBUGVER);
+		val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6);
 		/* Limit guests to PMUv3 for ARMv8.1 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_AA64DFR0_PMUVER_SHIFT,

Signature · 6 lines

2.29.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Alexandru Elisei

Reviewed-by on v2 5/7

2021-01-27 12:18 UTC #9 lore raw

Re: [PATCH v2 5/7] KVM: arm64: Limit the debug architecture to ARMv8.0

Hi Marc,

This looks correct, and it also matches what we report for aarch32 in trap_dbgidr():

Quoted 14 lines · Marc Zyngier

Let's not pretend we support anything but ARMv8.0 as far as the debug architecture is concerned.
Reviewed-by Eric Auger <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/sys_regs.c | 3 +++
 1 file changed, 3 insertions(+)
arch/arm64/kvm/sys_regs.c +0 -0
index dda16d60197b..8f79ec1fffa7 100644

arch/arm64/kvm/sys_regs.c static u64 read_id_reg(const struct kvm_vcpu *vcpu, · line 1056

Quoted 9 lines

arch/arm64/kvm/sys_regs.c +3 -0

@@ -1048,6 +1048,9 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 				 FEATURE(ID_AA64ISAR1_GPI));
 		break;
 	case SYS_ID_AA64DFR0_EL1:
+		/* Limit debug to ARMv8.0 */
+		val &= ~FEATURE(ID_AA64DFR0_DEBUGVER);
+		val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6);
 		/* Limit guests to PMUv3 for ARMv8.1 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_AA64DFR0_PMUVER_SHIFT,

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-01-25 12:26 UTC #10 lore raw

PATCH v2 6/7 KVM: arm64: Upgrade PMU support to ARMv8.4

Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.

Let's just do that and adjust what we return to the guest.

---
 arch/arm64/include/asm/sysreg.h |  3 +++
 arch/arm64/kvm/pmu-emul.c       |  6 ++++++
 arch/arm64/kvm/sys_regs.c       | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)

arch/arm64/include/asm/sysreg.h +3 -0

index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
 #define ID_DFR0_PERFMON_SHIFT		24
 
+#define ID_DFR0_PERFMON_8_0		0x3
 #define ID_DFR0_PERFMON_8_1		0x4
+#define ID_DFR0_PERFMON_8_4		0x5
+#define ID_DFR0_PERFMON_8_5		0x6
 
 #define ID_ISAR4_SWP_FRAC_SHIFT		28
 #define ID_ISAR4_PSR_M_SHIFT		24

arch/arm64/kvm/pmu-emul.c +6 -0

index 398f6df1bbe4..72cd704a8368 100644
@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
 		base = 0;
 	} else {
 		val = read_sysreg(pmceid1_el0);
+		/*
+		 * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+		 * as RAZ
+		 */
+		if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+			val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
 		base = 32;
 	}

arch/arm64/kvm/sys_regs.c +7 -4

index 8f79ec1fffa7..5da536ab738d 100644
@@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 		/* Limit debug to ARMv8.0 */
 		val &= ~FEATURE(ID_AA64DFR0_DEBUGVER);
 		val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6);
-		/* Limit guests to PMUv3 for ARMv8.1 */
+		/* Limit guests to PMUv3 for ARMv8.4 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_AA64DFR0_PMUVER_SHIFT,
-						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0);
+						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_4 : 0);
 		break;
 	case SYS_ID_DFR0_EL1:
-		/* Limit guests to PMUv3 for ARMv8.1 */
+		/* Limit guests to PMUv3 for ARMv8.4 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_DFR0_PERFMON_SHIFT,
-						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_1 : 0);
+						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
 		break;
 	}

@@ -1496,6 +1496,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 },
 	{ SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 },
+	{ SYS_DESC(SYS_PMMIR_EL1), trap_raz_wi },
 
 	{ SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
 	{ SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },

@@ -1918,6 +1919,8 @@ static const struct sys_reg_desc cp15_regs[] = {
 	{ Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs },
 	{ AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 4), access_pmceid },
 	{ AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 5), access_pmceid },
+	/* PMMIR */
+	{ Op1( 0), CRn( 9), CRm(14), Op2( 6), trap_raz_wi },
 
 	/* PRRR/MAIR0 */
 	{ AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, MAIR_EL1 },

Signature · 6 lines

2.29.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Alexandru Elisei

2021-01-27 14:09 UTC #11 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Hi Marc,

Quoted 4 lines · Marc Zyngier

Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.

According to ARM DDI 0487F.b, page D7-2743:

"If ARMv8.4-PMU is implemented:
- If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the PMMIR System registers are implemented.
- If STALL_SLOT is implemented, then the PMMIR System registers are implemented."

I tried to come up with a reason why PMMIR is emulated instead of being left undefined, but I couldn't figure it out. Would you mind adding a comment or changing the commit message to explain that?

Thanks,
Alex

Quoted 75 lines

Let's just do that and adjust what we return to the guest.

---
 arch/arm64/include/asm/sysreg.h |  3 +++
 arch/arm64/kvm/pmu-emul.c       |  6 ++++++
 arch/arm64/kvm/sys_regs.c       | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)

arch/arm64/include/asm/sysreg.h +3 -0

index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
 #define ID_DFR0_PERFMON_SHIFT		24
 
+#define ID_DFR0_PERFMON_8_0		0x3
 #define ID_DFR0_PERFMON_8_1		0x4
+#define ID_DFR0_PERFMON_8_4		0x5
+#define ID_DFR0_PERFMON_8_5		0x6
 
 #define ID_ISAR4_SWP_FRAC_SHIFT		28
 #define ID_ISAR4_PSR_M_SHIFT		24

arch/arm64/kvm/pmu-emul.c +6 -0

index 398f6df1bbe4..72cd704a8368 100644
@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
 		base = 0;
 	} else {
 		val = read_sysreg(pmceid1_el0);
+		/*
+		 * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+		 * as RAZ
+		 */
+		if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+			val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
 		base = 32;
 	}

arch/arm64/kvm/sys_regs.c +5 -4

index 8f79ec1fffa7..5da536ab738d 100644
@@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 		/* Limit debug to ARMv8.0 */
 		val &= ~FEATURE(ID_AA64DFR0_DEBUGVER);
 		val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6);
-		/* Limit guests to PMUv3 for ARMv8.1 */
+		/* Limit guests to PMUv3 for ARMv8.4 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_AA64DFR0_PMUVER_SHIFT,
-						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0);
+						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_4 : 0);
 		break;
 	case SYS_ID_DFR0_EL1:
-		/* Limit guests to PMUv3 for ARMv8.1 */
+		/* Limit guests to PMUv3 for ARMv8.4 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_DFR0_PERFMON_SHIFT,
-						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_1 : 0);
+						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
 		break;
 	}

@@ -1496,6 +1496,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 },
 	{ SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 },
+	{ SYS_DESC(SYS_PMMIR_EL1), trap_raz_wi },
 
 	{ SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
 	{ SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },

arch/arm64/kvm/sys_regs.c static const struct sys_reg_desc cp15_regs[] = { · line 1926

Quoted 8 lines

arch/arm64/kvm/sys_regs.c +2 -0

@@ -1918,6 +1919,8 @@ static const struct sys_reg_desc cp15_regs[] = {
 	{ Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs },
 	{ AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 4), access_pmceid },
 	{ AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 5), access_pmceid },
+	/* PMMIR */
+	{ Op1( 0), CRn( 9), CRm(14), Op2( 6), trap_raz_wi },
 
 	/* PRRR/MAIR0 */
 	{ AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, MAIR_EL1 },

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-01-27 14:35 UTC #12 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Hi Alex,

Quoted 17 lines · Alexandru Elisei

Hi Marc,
Quoted 4 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
According to ARM DDI 0487F.b, page D7-2743:
"If ARMv8.4-PMU is implemented:
- If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the PMMIR
System registers are implemented.
- If STALL_SLOT is implemented, then the PMMIR System registers are implemented."

Quoted 3 lines

I tried to come up with a reason why PMMIR is emulated instead of being left undefined, but I couldn't figure it out. Would you mind adding a comment or changing the commit message to explain that?

The main reason is that PMMIR gets new fields down the line, and doing the bare minimum in term of implementation allows us to gently ease into it.

We could also go for the full PMMIR reporting on homogeneous systems too, as a further improvement.

What do you think?

M.

Signature · 5 lines

Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Alexandru Elisei

2021-01-27 17:01 UTC #13 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Hi Marc,

Quoted 20 lines · Marc Zyngier

Hi Alex,
Quoted 17 lines · Alexandru Elisei
Hi Marc,
Quoted 4 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
According to ARM DDI 0487F.b, page D7-2743:
"If ARMv8.4-PMU is implemented:
- If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the PMMIR
System registers are implemented.
- If STALL_SLOT is implemented, then the PMMIR System registers are implemented."

Quoted 7 lines

Quoted 3 lines
I tried to come up with a reason why PMMIR is emulated instead of being left undefined, but I couldn't figure it out. Would you mind adding a comment or changing the commit message to explain that?
The main reason is that PMMIR gets new fields down the line, and doing the bare minimum in term of implementation allows us to gently ease into it.

I think I understand what you are saying - add a bare minimum emulation of the PMMIR register now so it's less work when we do decide to support the STALL_SLOT event for a guest.

Quoted 5 lines

We could also go for the full PMMIR reporting on homogeneous systems too, as a further improvement.
What do you think?

I don't have an opinion either way. But if you do decide to add full emulation for STALL_SLOT, I would like to help with reviewing the patches (I'm curious to see how KVM will detect that it's running on a homogeneous system).

Thanks,
Alex
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-01-27 17:24 UTC #14 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Quoted 37 lines · Alexandru Elisei

Hi Marc,
Quoted 20 lines · Marc Zyngier
Hi Alex,
Quoted 17 lines · Alexandru Elisei
Hi Marc,
Quoted 4 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
According to ARM DDI 0487F.b, page D7-2743:
"If ARMv8.4-PMU is implemented:
- If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the PMMIR
System registers are implemented.
- If STALL_SLOT is implemented, then the PMMIR System registers are implemented."
Quoted 7 lines
Quoted 3 lines
I tried to come up with a reason why PMMIR is emulated instead of being left undefined, but I couldn't figure it out. Would you mind adding a comment or changing the commit message to explain that?
The main reason is that PMMIR gets new fields down the line, and doing the bare minimum in term of implementation allows us to gently ease into it.
I think I understand what you are saying - add a bare minimum emulation of the PMMIR register now so it's less work when we do decide to support the STALL_SLOT event for a guest.
Quoted 4 lines
We could also go for the full PMMIR reporting on homogeneous systems too, as a further improvement.

Quoted 6 lines

Quoted 1 line
What do you think?
I don't have an opinion either way. But if you do decide to add full emulation for
STALL_SLOT, I would like to help with reviewing the patches (I'm curious to see how KVM will detect that it's running on a homogeneous system).

I'm not sure we can *detect* it. We'd need some more information from the core arch code and firmware. To be honest, PMU emulation is a joke on BL, so maybe we shouldn't really care and expose what we have.

M.

Signature · 5 lines

Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Alexandru Elisei

2021-01-27 17:42 UTC #15 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Hi Marc,

Had another look at the patch, comments below.

Quoted 34 lines · Marc Zyngier

Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
Let's just do that and adjust what we return to the guest.
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/include/asm/sysreg.h |  3 +++
 arch/arm64/kvm/pmu-emul.c       |  6 ++++++
 arch/arm64/kvm/sys_regs.c       | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
arch/arm64/include/asm/sysreg.h +3 -0
index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
 #define ID_DFR0_PERFMON_SHIFT		24
 
+#define ID_DFR0_PERFMON_8_0		0x3
 #define ID_DFR0_PERFMON_8_1		0x4
+#define ID_DFR0_PERFMON_8_4		0x5
+#define ID_DFR0_PERFMON_8_5		0x6
 
 #define ID_ISAR4_SWP_FRAC_SHIFT		28
 #define ID_ISAR4_PSR_M_SHIFT		24
arch/arm64/kvm/pmu-emul.c +0 -0
index 398f6df1bbe4..72cd704a8368 100644

arch/arm64/kvm/pmu-emul.c u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) · line 803

Quoted 9 lines

arch/arm64/kvm/pmu-emul.c +6 -0

@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
 		base = 0;
 	} else {
 		val = read_sysreg(pmceid1_el0);
+		/*
+		 * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+		 * as RAZ
+		 */
+		if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+			val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);

This is confusing the me. We have kvm->arch.pmuver set to the hardware PMU version (as set by __armv8pmu_probe_pmu()), but we ignore it when reporting the PMU version to the guest. Why do we do that? We limit the event number in kvm_pmu_event_mask() based on the hardware PMU version, so even if we advertise Armv8.4 PMU, support for all those extra events added by Arm8.1 PMU is missing (I hope I understood the code correctly).

I looked at commit c854188ea010 ("KVM: arm64: limit PMU version to PMUv3 for ARMv8.1") which changed read_id_reg() to report PMUv3 for Armv8.1 unconditionally, and there's no explanation why PMUv3 for Armv8.1 was chosen instead of plain PMUv3 (PMUVer = 0b0100).

Thanks,

Alex

Quoted 37 lines

arch/arm64/kvm/pmu-emul.c +0 -0

 		base = 32;
 	}

arch/arm64/kvm/sys_regs.c +5 -4

index 8f79ec1fffa7..5da536ab738d 100644
@@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 		/* Limit debug to ARMv8.0 */
 		val &= ~FEATURE(ID_AA64DFR0_DEBUGVER);
 		val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6);
-		/* Limit guests to PMUv3 for ARMv8.1 */
+		/* Limit guests to PMUv3 for ARMv8.4 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_AA64DFR0_PMUVER_SHIFT,
-						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0);
+						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_4 : 0);
 		break;
 	case SYS_ID_DFR0_EL1:
-		/* Limit guests to PMUv3 for ARMv8.1 */
+		/* Limit guests to PMUv3 for ARMv8.4 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_DFR0_PERFMON_SHIFT,
-						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_1 : 0);
+						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
 		break;
 	}

@@ -1496,6 +1496,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 },
 	{ SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 },
+	{ SYS_DESC(SYS_PMMIR_EL1), trap_raz_wi },
 
 	{ SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
 	{ SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },

arch/arm64/kvm/sys_regs.c static const struct sys_reg_desc cp15_regs[] = { · line 1926

Quoted 8 lines

arch/arm64/kvm/sys_regs.c +2 -0

@@ -1918,6 +1919,8 @@ static const struct sys_reg_desc cp15_regs[] = {
 	{ Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs },
 	{ AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 4), access_pmceid },
 	{ AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 5), access_pmceid },
+	/* PMMIR */
+	{ Op1( 0), CRn( 9), CRm(14), Op2( 6), trap_raz_wi },
 
 	/* PRRR/MAIR0 */
 	{ AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, MAIR_EL1 },

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-02-03 10:32 UTC #16 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Quoted 50 lines · Alexandru Elisei

Hi Marc,
Had another look at the patch, comments below.
Quoted 34 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
Let's just do that and adjust what we return to the guest.
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/include/asm/sysreg.h |  3 +++
 arch/arm64/kvm/pmu-emul.c       |  6 ++++++
 arch/arm64/kvm/sys_regs.c       | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
arch/arm64/include/asm/sysreg.h +3 -0
index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
 #define ID_DFR0_PERFMON_SHIFT		24
 
+#define ID_DFR0_PERFMON_8_0		0x3
 #define ID_DFR0_PERFMON_8_1		0x4
+#define ID_DFR0_PERFMON_8_4		0x5
+#define ID_DFR0_PERFMON_8_5		0x6
 
 #define ID_ISAR4_SWP_FRAC_SHIFT		28
 #define ID_ISAR4_PSR_M_SHIFT		24
arch/arm64/kvm/pmu-emul.c +0 -0
index 398f6df1bbe4..72cd704a8368 100644
arch/arm64/kvm/pmu-emul.c u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, · line 804
Quoted 10 lines
arch/arm64/kvm/pmu-emul.c +6 -0
@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, 
 bool pmceid1)
 		base = 0;
 	} else {
 		val = read_sysreg(pmceid1_el0);
+		/*
+		 * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+		 * as RAZ
+		 */
+		if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+			val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);

Quoted 7 lines

This is confusing the me. We have kvm->arch.pmuver set to the hardware PMU version
(as set by __armv8pmu_probe_pmu()), but we ignore it when reporting the PMU version to the guest. Why do we do that? We limit the event number in kvm_pmu_event_mask() based on the hardware PMU version, so even if we advertise Armv8.4 PMU, support for all those extra events added by Arm8.1 PMU is missing (I hope I understood the code correctly).

That's a bit of mess-up. We obtain ID_AA64DFR0_EL1 from the sanitised regs, but do most of our handling based on kvm->arch.pmuver. They really should be the same, because that's what the sanitised registers give you.

As for the events themselves, I don't get your drift. We do support all the ARMv8.1 PMU events as long as the HW supports it, and we don't lie to the guest about it either (cpuid_feature_cap_perfmon_field does *cap* the field to some value, it doesn't allow it to increase past what the HW supports).

Quoted 5 lines

I looked at commit c854188ea010 ("KVM: arm64: limit PMU version to PMUv3 for ARMv8.1") which changed read_id_reg() to report PMUv3 for Armv8.1 unconditionally,
and there's no explanation why PMUv3 for Armv8.1 was chosen instead of plain PMUv3 (PMUVer = 0b0100).

We picked ARMv8.1 because this is the first PMU revision that gives you events in the 0x4000 range, all of which are available on common ARMv8.2 HW.

Thanks,

M.

Signature · 5 lines

Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Alexandru Elisei

2021-02-03 17:29 UTC #17 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Hi Marc,

Quoted 63 lines · Marc Zyngier

Quoted 49 lines · Alexandru Elisei
Hi Marc,
Had another look at the patch, comments below.
Quoted 34 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
Let's just do that and adjust what we return to the guest.
Signed-off-by Marc Zyngier <[email protected]>
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/pmu-emul.c | 6 ++++++
arch/arm64/kvm/sys_regs.c | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
arch/arm64/include/asm/sysreg.h +3 -0
index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
  #define ID_DFR0_PERFMON_SHIFT        24
 
+#define ID_DFR0_PERFMON_8_0        0x3
  #define ID_DFR0_PERFMON_8_1        0x4
+#define ID_DFR0_PERFMON_8_4        0x5
+#define ID_DFR0_PERFMON_8_5        0x6
 
  #define ID_ISAR4_SWP_FRAC_SHIFT        28
  #define ID_ISAR4_PSR_M_SHIFT        24
arch/arm64/kvm/pmu-emul.c +0 -0
index 398f6df1bbe4..72cd704a8368 100644
arch/arm64/kvm/pmu-emul.c u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) · line 803
Quoted 9 lines
arch/arm64/kvm/pmu-emul.c +6 -0
@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
          base = 0;
      } else {
          val = read_sysreg(pmceid1_el0);
+        /*
+         * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+         * as RAZ
+         */
+        if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+            val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
Quoted 7 lines
This is confusing the me. We have kvm->arch.pmuver set to the hardware PMU version
(as set by __armv8pmu_probe_pmu()), but we ignore it when reporting the PMU version to the guest. Why do we do that? We limit the event number in kvm_pmu_event_mask() based on the hardware PMU version, so even if we advertise Armv8.4 PMU, support for all those extra events added by Arm8.1 PMU is missing (I hope I understood the code correctly).
That's a bit of mess-up. We obtain ID_AA64DFR0_EL1 from the sanitised regs, but do most of our handling based on kvm->arch.pmuver. They really should be the same, because that's what the sanitised registers give you.

Quoted 5 lines

As for the events themselves, I don't get your drift. We do support all the ARMv8.1 PMU events as long as the HW supports it, and we don't lie to the guest about it either (cpuid_feature_cap_perfmon_field does *cap* the field to some value, it doesn't allow it to increase past what the HW supports).

That's the piece that I was missing - I didn't realize that cpuid_feature_cap_perfmon_field() makes sure that the final version doesn't exceed what the hardware supports. Thanks for clearing it up!

Quoted 10 lines

Quoted 6 lines
I looked at commit c854188ea010 ("KVM: arm64: limit PMU version to PMUv3 for ARMv8.1") which changed read_id_reg() to report PMUv3 for Armv8.1 unconditionally,
and there's no explanation why PMUv3 for Armv8.1 was chosen instead of plain PMUv3 (PMUVer = 0b0100).
We picked ARMv8.1 because this is the first PMU revision that gives you events in the 0x4000 range, all of which are available on common ARMv8.2 HW.

Yes, makes sense in the context of cpuid_feature_cap_perfmon_field() capping PMUVer based on the hardware supported version.

Thanks,
Alex
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Auger Eric

2021-01-27 17:53 UTC #18 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Hi Marc,

Quoted 34 lines · Marc Zyngier

Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
Let's just do that and adjust what we return to the guest.
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/include/asm/sysreg.h |  3 +++
 arch/arm64/kvm/pmu-emul.c       |  6 ++++++
 arch/arm64/kvm/sys_regs.c       | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
arch/arm64/include/asm/sysreg.h +3 -0
index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
 #define ID_DFR0_PERFMON_SHIFT		24
 
+#define ID_DFR0_PERFMON_8_0		0x3
 #define ID_DFR0_PERFMON_8_1		0x4
+#define ID_DFR0_PERFMON_8_4		0x5
+#define ID_DFR0_PERFMON_8_5		0x6
 
 #define ID_ISAR4_SWP_FRAC_SHIFT		28
 #define ID_ISAR4_PSR_M_SHIFT		24
arch/arm64/kvm/pmu-emul.c +0 -0
index 398f6df1bbe4..72cd704a8368 100644

arch/arm64/kvm/pmu-emul.c u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) · line 803

Quoted 9 lines

arch/arm64/kvm/pmu-emul.c +6 -0

@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
 		base = 0;
 	} else {
 		val = read_sysreg(pmceid1_el0);
+		/*
+		 * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+		 * as RAZ
+		 */
+		if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+			val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);

what about the STALL_SLOT_BACKEND and FRONTEND events then?

Quoted 29 lines

arch/arm64/kvm/pmu-emul.c +0 -0

 		base = 32;
 	}

arch/arm64/kvm/sys_regs.c +4 -4

index 8f79ec1fffa7..5da536ab738d 100644
@@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
 		/* Limit debug to ARMv8.0 */
 		val &= ~FEATURE(ID_AA64DFR0_DEBUGVER);
 		val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6);
-		/* Limit guests to PMUv3 for ARMv8.1 */
+		/* Limit guests to PMUv3 for ARMv8.4 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_AA64DFR0_PMUVER_SHIFT,
-						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0);
+						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_4 : 0);
 		break;
 	case SYS_ID_DFR0_EL1:
-		/* Limit guests to PMUv3 for ARMv8.1 */
+		/* Limit guests to PMUv3 for ARMv8.4 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_DFR0_PERFMON_SHIFT,
-						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_1 : 0);
+						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
 		break;
 	}

arch/arm64/kvm/sys_regs.c static const struct sys_reg_desc sys_reg_descs[] = { · line 1498

Quoted 3 lines

arch/arm64/kvm/sys_regs.c +0 -0

@@ -1496,6 +1496,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 
 	{ SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 },
 	{ SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, PMINTENSET_EL1 },

"KVM: arm64: Hide PMU registers from userspace when not available" changed the above, doesn't it?

Quoted 6 lines

arch/arm64/kvm/sys_regs.c +1 -0

+	{ SYS_DESC(SYS_PMMIR_EL1), trap_raz_wi },
 
 	{ SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
 	{ SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },

arch/arm64/kvm/sys_regs.c static const struct sys_reg_desc cp15_regs[] = { · line 1926

Quoted 8 lines

arch/arm64/kvm/sys_regs.c +2 -0

@@ -1918,6 +1919,8 @@ static const struct sys_reg_desc cp15_regs[] = {
 	{ Op1( 0), CRn( 9), CRm(14), Op2( 3), access_pmovs },
 	{ AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 4), access_pmceid },
 	{ AA32(HI), Op1( 0), CRn( 9), CRm(14), Op2( 5), access_pmceid },
+	/* PMMIR */
+	{ Op1( 0), CRn( 9), CRm(14), Op2( 6), trap_raz_wi },
 
 	/* PRRR/MAIR0 */
 	{ AA32(LO), Op1( 0), CRn(10), CRm( 2), Op2( 0), access_vm_reg, NULL, MAIR_EL1 },

Thanks

Eric

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-02-03 10:37 UTC #19 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Hi Eric,

Quoted 40 lines · Auger Eric

Hi Marc,
Quoted 37 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
Let's just do that and adjust what we return to the guest.
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/include/asm/sysreg.h |  3 +++
 arch/arm64/kvm/pmu-emul.c       |  6 ++++++
 arch/arm64/kvm/sys_regs.c       | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
arch/arm64/include/asm/sysreg.h +3 -0
index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
 #define ID_DFR0_PERFMON_SHIFT		24
 
+#define ID_DFR0_PERFMON_8_0		0x3
 #define ID_DFR0_PERFMON_8_1		0x4
+#define ID_DFR0_PERFMON_8_4		0x5
+#define ID_DFR0_PERFMON_8_5		0x6
 
 #define ID_ISAR4_SWP_FRAC_SHIFT		28
 #define ID_ISAR4_PSR_M_SHIFT		24
arch/arm64/kvm/pmu-emul.c +0 -0
index 398f6df1bbe4..72cd704a8368 100644
@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, 
 bool pmceid1)
 		base = 0;
 	} else {

Quoted 8 lines

arch/arm64/kvm/pmu-emul.c u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) · line 803
Quoted 7 lines
arch/arm64/kvm/pmu-emul.c +6 -0
 		val = read_sysreg(pmceid1_el0);
+		/*
+		 * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+		 * as RAZ
+		 */
+		if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+			val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
what about the STALL_SLOT_BACKEND and FRONTEND events then?

Aren't these a mandatory ARMv8.1 feature? I don't see a reason to drop them.

Quoted 32 lines

arch/arm64/kvm/pmu-emul.c +0 -0

 		base = 32;
 	}

arch/arm64/kvm/sys_regs.c +4 -4

index 8f79ec1fffa7..5da536ab738d 100644
@@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu 
 *vcpu,
 		/* Limit debug to ARMv8.0 */
 		val &= ~FEATURE(ID_AA64DFR0_DEBUGVER);
 		val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6);
-		/* Limit guests to PMUv3 for ARMv8.1 */
+		/* Limit guests to PMUv3 for ARMv8.4 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_AA64DFR0_PMUVER_SHIFT,
-						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_1 : 0);
+						      kvm_vcpu_has_pmu(vcpu) ? ID_AA64DFR0_PMUVER_8_4 : 0);
 		break;
 	case SYS_ID_DFR0_EL1:
-		/* Limit guests to PMUv3 for ARMv8.1 */
+		/* Limit guests to PMUv3 for ARMv8.4 */
 		val = cpuid_feature_cap_perfmon_field(val,
 						      ID_DFR0_PERFMON_SHIFT,
-						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_1 : 0);
+						      kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
 		break;
 	}

@@ -1496,6 +1496,7 @@ static const struct sys_reg_desc sys_reg_descs[] 
 = {

Quoted 6 lines

arch/arm64/kvm/sys_regs.c static const struct sys_reg_desc sys_reg_descs[] · line 1501
Quoted 4 lines
arch/arm64/kvm/sys_regs.c +0 -0
 	{ SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown, 
 PMINTENSET_EL1 },
 	{ SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown, 
 PMINTENSET_EL1 },
"KVM: arm64: Hide PMU registers from userspace when not available" changed the above, doesn't it?

Yes, that's because the fix didn't make it in mainline before 5.11-rc5, and I based this on -rc4. I'll fix it at merge time.

Thanks,

M.

Signature · 5 lines

Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Auger Eric

2021-02-03 11:07 UTC #20 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Hi Marc,

Quoted 53 lines · Marc Zyngier

Hi Eric,
Quoted 41 lines · Auger Eric
Hi Marc,
Quoted 38 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
Let's just do that and adjust what we return to the guest.
Signed-off-by Marc Zyngier <[email protected]>
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/pmu-emul.c | 6 ++++++
arch/arm64/kvm/sys_regs.c | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
arch/arm64/include/asm/sysreg.h +3 -0
index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
  #define ID_DFR0_PERFMON_SHIFT        24
 
+#define ID_DFR0_PERFMON_8_0        0x3
  #define ID_DFR0_PERFMON_8_1        0x4
+#define ID_DFR0_PERFMON_8_4        0x5
+#define ID_DFR0_PERFMON_8_5        0x6
 
  #define ID_ISAR4_SWP_FRAC_SHIFT        28
  #define ID_ISAR4_PSR_M_SHIFT        24
arch/arm64/kvm/pmu-emul.c +0 -0
index 398f6df1bbe4..72cd704a8368 100644
@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu,
 bool pmceid1)
          base = 0;
      } else {
Quoted 8 lines
arch/arm64/kvm/pmu-emul.c u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) · line 803
Quoted 7 lines
arch/arm64/kvm/pmu-emul.c +6 -0
          val = read_sysreg(pmceid1_el0);
+        /*
+         * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+         * as RAZ
+         */
+        if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+            val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
what about the STALL_SLOT_BACKEND and FRONTEND events then?

Quoted 2 lines

Aren't these a mandatory ARMv8.1 feature? I don't see a reason to drop them.

I understand the 3 are linked together.

In D7.11 it is said
"
When any of the following common events are implemented, all three of them are implemented:
0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a Slot due to the backend,
0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a Slot due to the frontend.
0x003F , STALL_SLOT, No operation sent for execution on a Slot. "

Thanks

Eric

Quoted 44 lines

Quoted 37 lines

arch/arm64/kvm/pmu-emul.c +0 -0

          base = 32;
      }

arch/arm64/kvm/sys_regs.c +2 -2

index 8f79ec1fffa7..5da536ab738d 100644
@@ -1051,16 +1051,16 @@ static u64 read_id_reg(const struct kvm_vcpu
 *vcpu,
          /* Limit debug to ARMv8.0 */
          val &= ~FEATURE(ID_AA64DFR0_DEBUGVER);
          val |= FIELD_PREP(FEATURE(ID_AA64DFR0_DEBUGVER), 6);
-        /* Limit guests to PMUv3 for ARMv8.1 */
+        /* Limit guests to PMUv3 for ARMv8.4 */
          val = cpuid_feature_cap_perfmon_field(val,
                                ID_AA64DFR0_PMUVER_SHIFT,
-                              kvm_vcpu_has_pmu(vcpu) ?
 ID_AA64DFR0_PMUVER_8_1 : 0);
+                              kvm_vcpu_has_pmu(vcpu) ?
 ID_AA64DFR0_PMUVER_8_4 : 0);
          break;

     case SYS_ID_DFR0_EL1:
-        /* Limit guests to PMUv3 for ARMv8.1 */ +        /* Limit guests to PMUv3 for ARMv8.4 */ val = cpuid_feature_cap_perfmon_field(val, ID_DFR0_PERFMON_SHIFT, -                              kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_1 : 0);
+                              kvm_vcpu_has_pmu(vcpu) ? ID_DFR0_PERFMON_8_4 : 0);
         break;
     }

arch/arm64/kvm/sys_regs.c +0 -0

@@ -1496,6 +1496,7 @@ static const struct sys_reg_desc
 sys_reg_descs[] = {

Quoted 6 lines

arch/arm64/kvm/sys_regs.c static const struct sys_reg_desc · line 1501
Quoted 4 lines
arch/arm64/kvm/sys_regs.c +0 -0
      { SYS_DESC(SYS_PMINTENSET_EL1), access_pminten, reset_unknown,
 PMINTENSET_EL1 },
      { SYS_DESC(SYS_PMINTENCLR_EL1), access_pminten, reset_unknown,
 PMINTENSET_EL1 },
"KVM: arm64: Hide PMU registers from userspace when not available" changed the above, doesn't it?

Quoted 6 lines

Yes, that's because the fix didn't make it in mainline before 5.11-rc5, and I based this on -rc4. I'll fix it at merge time.
Thanks,
M.

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-02-03 11:20 UTC #21 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Quoted 60 lines · Auger Eric

Hi Marc,
Quoted 53 lines · Marc Zyngier
Hi Eric,
Quoted 41 lines · Auger Eric
Hi Marc,
Quoted 38 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
Let's just do that and adjust what we return to the guest.
Signed-off-by Marc Zyngier <[email protected]>
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/pmu-emul.c | 6 ++++++
arch/arm64/kvm/sys_regs.c | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
arch/arm64/include/asm/sysreg.h +3 -0
index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
  #define ID_DFR0_PERFMON_SHIFT        24
 
+#define ID_DFR0_PERFMON_8_0        0x3
  #define ID_DFR0_PERFMON_8_1        0x4
+#define ID_DFR0_PERFMON_8_4        0x5
+#define ID_DFR0_PERFMON_8_5        0x6
 
  #define ID_ISAR4_SWP_FRAC_SHIFT        28
  #define ID_ISAR4_PSR_M_SHIFT        24
arch/arm64/kvm/pmu-emul.c +0 -0
index 398f6df1bbe4..72cd704a8368 100644
@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu,
 bool pmceid1)
          base = 0;
      } else {
Quoted 8 lines
arch/arm64/kvm/pmu-emul.c u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) · line 803
Quoted 7 lines
arch/arm64/kvm/pmu-emul.c +6 -0
          val = read_sysreg(pmceid1_el0);
+        /*
+         * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+         * as RAZ
+         */
+        if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+            val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
what about the STALL_SLOT_BACKEND and FRONTEND events then?
Quoted 2 lines
Aren't these a mandatory ARMv8.1 feature? I don't see a reason to drop them.
I understand the 3 are linked together.

Quoted 10 lines

In D7.11 it is said
"
When any of the following common events are implemented, all three of them are implemented:
0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a Slot due to the backend,
0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a Slot due to the frontend.
0x003F , STALL_SLOT, No operation sent for execution on a Slot. "

They are linked in the sense that they report related events, but they don't have to be implemented in the same level of the architecure, if only because BACKEND/FRONTEND were introducedway before ARMv8.4.

What the architecture says is:

- For FEAT_PMUv3p1 (ARMv8.1):
"The STALL_FRONTEND and STALL_BACKEND events are required to be
implemented." (A2.4.1, DDI0487G.a)

- For FEAT_PMUv3p4 (ARMv8.4):
"If FEAT_PMUv3p4 is implemented:
- If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED
whether the PMMIR System registers are implemented.
- If STALL_SLOT is implemented, then the PMMIR System registers are
implemented." (D7-2873, DDI0487G.a)

So while BACKEND/FRONTEND are required in an ARMv8.4 implementation by virtue of being mandatory in ARMv8.1, STALL_SLOT isn't at any point.

Thanks,

M.

Signature · 5 lines

Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Auger Eric

2021-02-03 12:40 UTC #22 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Hi,

Quoted 76 lines · Marc Zyngier

Quoted 60 lines · Auger Eric
Hi Marc,
Quoted 53 lines · Marc Zyngier
Hi Eric,
Quoted 41 lines · Auger Eric
Hi Marc,
Quoted 38 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
Let's just do that and adjust what we return to the guest.
Signed-off-by Marc Zyngier <[email protected]>
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/pmu-emul.c | 6 ++++++
arch/arm64/kvm/sys_regs.c | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
arch/arm64/include/asm/sysreg.h +3 -0
index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
  #define ID_DFR0_PERFMON_SHIFT        24
 
+#define ID_DFR0_PERFMON_8_0        0x3
  #define ID_DFR0_PERFMON_8_1        0x4
+#define ID_DFR0_PERFMON_8_4        0x5
+#define ID_DFR0_PERFMON_8_5        0x6
 
  #define ID_ISAR4_SWP_FRAC_SHIFT        28
  #define ID_ISAR4_PSR_M_SHIFT        24
arch/arm64/kvm/pmu-emul.c +0 -0
index 398f6df1bbe4..72cd704a8368 100644
@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu,
 bool pmceid1)
          base = 0;
      } else {
Quoted 8 lines
arch/arm64/kvm/pmu-emul.c u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) · line 803
Quoted 7 lines
arch/arm64/kvm/pmu-emul.c +6 -0
          val = read_sysreg(pmceid1_el0);
+        /*
+         * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+         * as RAZ
+         */
+        if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+            val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
what about the STALL_SLOT_BACKEND and FRONTEND events then?
Quoted 2 lines
Aren't these a mandatory ARMv8.1 feature? I don't see a reason to drop them.
I understand the 3 are linked together.
Quoted 10 lines
In D7.11 it is said
"
When any of the following common events are implemented, all three of them are implemented:
0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a Slot due to the backend,
0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a Slot due to the frontend.
0x003F , STALL_SLOT, No operation sent for execution on a Slot. "
They are linked in the sense that they report related events, but they don't have to be implemented in the same level of the architecure, if only because BACKEND/FRONTEND were introducedway before ARMv8.4.

Quoted 5 lines

What the architecture says is:
- For FEAT_PMUv3p1 (ARMv8.1):
"The STALL_FRONTEND and STALL_BACKEND events are required to be implemented." (A2.4.1, DDI0487G.a)

Quoted 10 lines

- For FEAT_PMUv3p4 (ARMv8.4):
"If FEAT_PMUv3p4 is implemented:
- If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the PMMIR System registers are implemented.
- If STALL_SLOT is implemented, then the PMMIR System registers are implemented." (D7-2873, DDI0487G.a)
So while BACKEND/FRONTEND are required in an ARMv8.4 implementation by virtue of being mandatory in ARMv8.1, STALL_SLOT isn't at any point.

But then how do you understand "When any of the following common events are implemented, all three of them are implemented"?

Eric

Quoted 4 lines

Thanks,
M.

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-02-03 13:29 UTC #23 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Quoted 93 lines · Auger Eric

Hi,
Quoted 76 lines · Marc Zyngier
Quoted 60 lines · Auger Eric
Hi Marc,
Quoted 53 lines · Marc Zyngier
Hi Eric,
Quoted 41 lines · Auger Eric
Hi Marc,
Quoted 38 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
Let's just do that and adjust what we return to the guest.
Signed-off-by Marc Zyngier <[email protected]>
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/pmu-emul.c | 6 ++++++
arch/arm64/kvm/sys_regs.c | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
arch/arm64/include/asm/sysreg.h +3 -0
index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
  #define ID_DFR0_PERFMON_SHIFT        24
 
+#define ID_DFR0_PERFMON_8_0        0x3
  #define ID_DFR0_PERFMON_8_1        0x4
+#define ID_DFR0_PERFMON_8_4        0x5
+#define ID_DFR0_PERFMON_8_5        0x6
 
  #define ID_ISAR4_SWP_FRAC_SHIFT        28
  #define ID_ISAR4_PSR_M_SHIFT        24
arch/arm64/kvm/pmu-emul.c +0 -0
index 398f6df1bbe4..72cd704a8368 100644
@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu,
 bool pmceid1)
          base = 0;
      } else {
Quoted 8 lines
arch/arm64/kvm/pmu-emul.c u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1) · line 803
Quoted 7 lines
arch/arm64/kvm/pmu-emul.c +6 -0
          val = read_sysreg(pmceid1_el0);
+        /*
+         * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+         * as RAZ
+         */
+        if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+            val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
what about the STALL_SLOT_BACKEND and FRONTEND events then?
Quoted 2 lines
Aren't these a mandatory ARMv8.1 feature? I don't see a reason to drop them.
I understand the 3 are linked together.
Quoted 10 lines
In D7.11 it is said
"
When any of the following common events are implemented, all three of them are implemented:
0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a Slot due to the backend,
0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a Slot due to the frontend.
0x003F , STALL_SLOT, No operation sent for execution on a Slot. "
They are linked in the sense that they report related events, but they don't have to be implemented in the same level of the architecure, if only because BACKEND/FRONTEND were introducedway before ARMv8.4.
Quoted 5 lines
What the architecture says is:
- For FEAT_PMUv3p1 (ARMv8.1):
"The STALL_FRONTEND and STALL_BACKEND events are required to be implemented." (A2.4.1, DDI0487G.a)
OK
Quoted 8 lines
- For FEAT_PMUv3p4 (ARMv8.4):
"If FEAT_PMUv3p4 is implemented:
- If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the PMMIR System registers are implemented.
- If STALL_SLOT is implemented, then the PMMIR System registers are implemented." (D7-2873, DDI0487G.a)

Quoted 4 lines

Quoted 2 lines
So while BACKEND/FRONTEND are required in an ARMv8.4 implementation by virtue of being mandatory in ARMv8.1, STALL_SLOT isn't at any point.
But then how do you understand "When any of the following common events are implemented, all three of them are implemented"?

I think that's wholly inconsistent, because it would mean that STALL_SLOT isn't optional on ARMv8.4, and would make PMMIR mandatory.

I'm starting to think that dropping this patch may be the best thing to do...

Thanks,

M.

Signature · 5 lines

Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Alexandru Elisei

2021-02-04 12:32 UTC #24 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Hi,

Quoted 94 lines · Marc Zyngier

Quoted 93 lines · Auger Eric
Hi,
Quoted 76 lines · Marc Zyngier
Quoted 60 lines · Auger Eric
Hi Marc,
Quoted 53 lines · Marc Zyngier
Hi Eric,
Quoted 41 lines · Auger Eric
Hi Marc,
Quoted 38 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
Let's just do that and adjust what we return to the guest.
Signed-off-by Marc Zyngier <[email protected]>
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/pmu-emul.c | 6 ++++++
arch/arm64/kvm/sys_regs.c | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
arch/arm64/include/asm/sysreg.h +3 -0
index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
  #define ID_DFR0_PERFMON_SHIFT        24
 
+#define ID_DFR0_PERFMON_8_0        0x3
  #define ID_DFR0_PERFMON_8_1        0x4
+#define ID_DFR0_PERFMON_8_4        0x5
+#define ID_DFR0_PERFMON_8_5        0x6
 
  #define ID_ISAR4_SWP_FRAC_SHIFT        28
  #define ID_ISAR4_PSR_M_SHIFT        24
arch/arm64/kvm/pmu-emul.c +0 -0
index 398f6df1bbe4..72cd704a8368 100644
@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu,
 bool pmceid1)
          base = 0;
      } else {
Quoted 8 lines
arch/arm64/kvm/pmu-emul.c u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, · line 804
Quoted 7 lines
arch/arm64/kvm/pmu-emul.c +6 -0
          val = read_sysreg(pmceid1_el0);
+        /*
+         * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+         * as RAZ
+         */
+        if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+            val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
what about the STALL_SLOT_BACKEND and FRONTEND events then?
Quoted 2 lines
Aren't these a mandatory ARMv8.1 feature? I don't see a reason to drop them.
I understand the 3 are linked together.
Quoted 10 lines
In D7.11 it is said
"
When any of the following common events are implemented, all three of them are implemented:
0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a Slot due to the backend,
0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a Slot due to the frontend.
0x003F , STALL_SLOT, No operation sent for execution on a Slot. "
They are linked in the sense that they report related events, but they don't have to be implemented in the same level of the architecure, if only because BACKEND/FRONTEND were introducedway before ARMv8.4.
Quoted 5 lines
What the architecture says is:
- For FEAT_PMUv3p1 (ARMv8.1):
"The STALL_FRONTEND and STALL_BACKEND events are required to be implemented." (A2.4.1, DDI0487G.a)
OK
Quoted 8 lines
- For FEAT_PMUv3p4 (ARMv8.4):
"If FEAT_PMUv3p4 is implemented:
- If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the PMMIR System registers are implemented.
- If STALL_SLOT is implemented, then the PMMIR System registers are implemented." (D7-2873, DDI0487G.a)

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
So while BACKEND/FRONTEND are required in an ARMv8.4 implementation by virtue of being mandatory in ARMv8.1, STALL_SLOT isn't at any point.
But then how do you understand "When any of the following common events are implemented, all three of them are implemented"?
I think that's wholly inconsistent, because it would mean that STALL_SLOT isn't optional on ARMv8.4, and would make PMMIR mandatory.

I think there's some confusion regarding the event names. From my reading of the architecture, STALL != STALL_SLOT, STALL_BACKEND != STALL_SLOT_BACKEND, STALL_FRONTEND != STALL_SLOT_FRONTEND.

STALL{, _BACKEND, _FRONTEND} count the number of CPU cycles where no instructions are being executed on the PE (page D7-2872), STALL_SLOT{, _BACKEND, _FRONTEND} count the number of slots where no instructions are being executed (page D7-2873).

STALL_{BACKEND, FRONTEND} are required by ARMv8.1-PMU (pages A2-76, D7-2913); STALL is required by ARMv8.4-PMU (page D7-2914).

STALL_SLOT{, _BACKEND, _FRONTEND} are optional in ARMv8.4-PMU (pages D7-2913, D7-2914), but if one of them is implemented, all of them must be implemented (page D7-2914).

The problem I see with this patch is that it doesn't clear the STALL_SLOT_{BACKEND, FRONTEND} event bits along with the STALL_SLOT bit from PMCEID1_EL0.

Thanks

Alex

Quoted 6 lines

I'm starting to think that dropping this patch may be the best thing to do...
Thanks,
M.

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-02-04 14:21 UTC #25 lore raw

Re: [PATCH v2 6/7] KVM: arm64: Upgrade PMU support to ARMv8.4

Quoted 104 lines · Alexandru Elisei

Hi,
Quoted 101 lines · Marc Zyngier
Quoted 99 lines · Auger Eric
Hi,
Quoted 78 lines · Marc Zyngier
Quoted 62 lines · Auger Eric
Hi Marc,
Quoted 55 lines · Marc Zyngier
Hi Eric,
Quoted 43 lines · Auger Eric
Hi Marc,
Quoted 40 lines · Marc Zyngier
Upgrading the PMU code from ARMv8.1 to ARMv8.4 turns out to be pretty easy. All that is required is support for PMMIR_EL1, which is read-only, and for which returning 0 is a valid option as long as we don't advertise STALL_SLOT as an implemented event.
Let's just do that and adjust what we return to the guest.
Signed-off-by Marc Zyngier <[email protected]>
---
arch/arm64/include/asm/sysreg.h | 3 +++
arch/arm64/kvm/pmu-emul.c | 6 ++++++
arch/arm64/kvm/sys_regs.c | 11 +++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
arch/arm64/include/asm/sysreg.h +3 -0
index 8b5e7e5c3cc8..2fb3f386588c 100644
@@ -846,7 +846,10 @@
 
  #define ID_DFR0_PERFMON_SHIFT        24
 
+#define ID_DFR0_PERFMON_8_0        0x3
  #define ID_DFR0_PERFMON_8_1        0x4
+#define ID_DFR0_PERFMON_8_4        0x5
+#define ID_DFR0_PERFMON_8_5        0x6
 
  #define ID_ISAR4_SWP_FRAC_SHIFT        28
  #define ID_ISAR4_PSR_M_SHIFT        24
arch/arm64/kvm/pmu-emul.c +0 -0
index 398f6df1bbe4..72cd704a8368 100644
@@ -795,6 +795,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu 
 *vcpu,
 bool pmceid1)
          base = 0;
      } else {
          val = read_sysreg(pmceid1_el0);
Quoted 8 lines
arch/arm64/kvm/pmu-emul.c u64 kvm_pmu_get_pmceid(struct kvm_vcpu · line 806
Quoted 7 lines
arch/arm64/kvm/pmu-emul.c +6 -0
+        /*
+         * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+         * as RAZ
+         */
+        if (vcpu->kvm->arch.pmuver >= ID_AA64DFR0_PMUVER_8_4)
+            val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 
 32);
what about the STALL_SLOT_BACKEND and FRONTEND events then?
Quoted 2 lines
Aren't these a mandatory ARMv8.1 feature? I don't see a reason to drop them.
I understand the 3 are linked together.
Quoted 10 lines
In D7.11 it is said
"
When any of the following common events are implemented, all three of them are implemented:
0x003D , STALL_SLOT_BACKEND, No operation sent for execution on a Slot due to the backend,
0x003E , STALL_SLOT_FRONTEND, No operation sent for execution on a Slot due to the frontend.
0x003F , STALL_SLOT, No operation sent for execution on a Slot. "
They are linked in the sense that they report related events, but they don't have to be implemented in the same level of the architecure, if only because BACKEND/FRONTEND were introducedway before ARMv8.4.
Quoted 5 lines
What the architecture says is:
- For FEAT_PMUv3p1 (ARMv8.1):
"The STALL_FRONTEND and STALL_BACKEND events are required to be implemented." (A2.4.1, DDI0487G.a)
OK
Quoted 10 lines
- For FEAT_PMUv3p4 (ARMv8.4):
"If FEAT_PMUv3p4 is implemented:
- If STALL_SLOT is not implemented, it is IMPLEMENTATION DEFINED whether the PMMIR System registers are implemented.
- If STALL_SLOT is implemented, then the PMMIR System registers are implemented." (D7-2873, DDI0487G.a)
So while BACKEND/FRONTEND are required in an ARMv8.4 implementation by virtue of being mandatory in ARMv8.1, STALL_SLOT isn't at any point.
But then how do you understand "When any of the following common events are implemented, all three of them are implemented"?

Quoted 6 lines

Quoted 2 lines
I think that's wholly inconsistent, because it would mean that STALL_SLOT isn't optional on ARMv8.4, and would make PMMIR mandatory.
I think there's some confusion regarding the event names. From my reading of the architecture, STALL != STALL_SLOT, STALL_BACKEND != STALL_SLOT_BACKEND, STALL_FRONTEND != STALL_SLOT_FRONTEND.

Dammit, I hadn't realised that at all. Thanks for putting me right.

Quoted 10 lines

STALL{, _BACKEND, _FRONTEND} count the number of CPU cycles where no instructions
are being executed on the PE (page D7-2872), STALL_SLOT{, _BACKEND, _FRONTEND} count the number of slots where no instructions are being executed (page D7-2873).
STALL_{BACKEND, FRONTEND} are required by ARMv8.1-PMU (pages A2-76, D7-2913); STALL is required by ARMv8.4-PMU (page D7-2914).

Quoted 7 lines

STALL_SLOT{, _BACKEND, _FRONTEND} are optional in ARMv8.4-PMU (pages D7-2913, D7-2914), but if one of them is implemented, all of them must be implemented (page D7-2914).
The problem I see with this patch is that it doesn't clear the STALL_SLOT_{BACKEND, FRONTEND} event bits along with the STALL_SLOT bit from PMCEID1_EL0.

OK, so Eric was right, and I'm an illiterate idiot! I'll fix the patch and repost the whole thing.

Thanks,

M.

Signature · 5 lines

Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Marc Zyngier

author maintainer

2021-01-25 12:26 UTC #26 lore raw

PATCH v2 7/7 KVM: arm64: Use symbolic names for the PMU versions

Instead of using a bunch of magic numbers, use the existing definitions that have been added since 8673e02e58410 ("arm64: perf: Add support for ARMv8.5-PMU 64-bit counters")

---
 arch/arm64/kvm/pmu-emul.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/arm64/kvm/pmu-emul.c +4 -4

index 72cd704a8368..cb16ca2eee92 100644
@@ -23,11 +23,11 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc);
 static u32 kvm_pmu_event_mask(struct kvm *kvm)
 {
 	switch (kvm->arch.pmuver) {
-	case 1:			/* ARMv8.0 */
+	case ID_AA64DFR0_PMUVER_8_0:
 		return GENMASK(9, 0);
-	case 4:			/* ARMv8.1 */
-	case 5:			/* ARMv8.4 */
-	case 6:			/* ARMv8.5 */
+	case ID_AA64DFR0_PMUVER_8_1:
+	case ID_AA64DFR0_PMUVER_8_4:
+	case ID_AA64DFR0_PMUVER_8_5:
 		return GENMASK(15, 0);
 	default:		/* Shouldn't be here, just for sanity */
 		WARN_ONCE(1, "Unknown PMU version %d\n", kvm->arch.pmuver);

Signature · 6 lines

2.29.2

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Alexandru Elisei

Reviewed-by on v2 7/7

2021-01-27 14:28 UTC #27 lore raw

Re: [PATCH v2 7/7] KVM: arm64: Use symbolic names for the PMU versions

Hi Marc,

This is a nice cleanup. Checked that the defines have the same value as the constants they are replacing:

Quoted 19 lines · Marc Zyngier

Instead of using a bunch of magic numbers, use the existing definitions that have been added since 8673e02e58410 ("arm64: perf: Add support for ARMv8.5-PMU 64-bit counters")
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/pmu-emul.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm64/kvm/pmu-emul.c +1 -1
index 72cd704a8368..cb16ca2eee92 100644
@@ -23,11 +23,11 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc);
 static u32 kvm_pmu_event_mask(struct kvm *kvm)
 {
 	switch (kvm->arch.pmuver) {
-	case 1:			/* ARMv8.0 */
+	case ID_AA64DFR0_PMUVER_8_0:

arch/arm64/kvm/pmu-emul.c static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc); · line 33

Quoted 10 lines

arch/arm64/kvm/pmu-emul.c +3 -3

 		return GENMASK(9, 0);
-	case 4:			/* ARMv8.1 */
-	case 5:			/* ARMv8.4 */
-	case 6:			/* ARMv8.5 */
+	case ID_AA64DFR0_PMUVER_8_1:
+	case ID_AA64DFR0_PMUVER_8_4:
+	case ID_AA64DFR0_PMUVER_8_5:
 		return GENMASK(15, 0);
 	default:		/* Shouldn't be here, just for sanity */
 		WARN_ONCE(1, "Unknown PMU version %d\n", kvm->arch.pmuver);

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Auger Eric

Reviewed-by on v2 7/7

2021-01-27 17:56 UTC #28 lore raw

Re: [PATCH v2 7/7] KVM: arm64: Use symbolic names for the PMU versions

Hi Marc,

Quoted 20 lines · Marc Zyngier

Instead of using a bunch of magic numbers, use the existing definitions that have been added since 8673e02e58410 ("arm64: perf: Add support for ARMv8.5-PMU 64-bit counters")
Signed-off-by Marc Zyngier <[email protected]>
---
 arch/arm64/kvm/pmu-emul.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
arch/arm64/kvm/pmu-emul.c +1 -1
index 72cd704a8368..cb16ca2eee92 100644
@@ -23,11 +23,11 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc);
 static u32 kvm_pmu_event_mask(struct kvm *kvm)
 {
 	switch (kvm->arch.pmuver) {
-	case 1:			/* ARMv8.0 */
+	case ID_AA64DFR0_PMUVER_8_0:

arch/arm64/kvm/pmu-emul.c static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc); · line 33

Quoted 10 lines

arch/arm64/kvm/pmu-emul.c +3 -3

 		return GENMASK(9, 0);
-	case 4:			/* ARMv8.1 */
-	case 5:			/* ARMv8.4 */
-	case 6:			/* ARMv8.5 */
+	case ID_AA64DFR0_PMUVER_8_1:
+	case ID_AA64DFR0_PMUVER_8_4:
+	case ID_AA64DFR0_PMUVER_8_5:
 		return GENMASK(15, 0);
 	default:		/* Shouldn't be here, just for sanity */
 		WARN_ONCE(1, "Unknown PMU version %d\n", kvm->arch.pmuver);