[PATCH v2 0/7] KVM: arm64: More PMU/debug ID register fixes

[PATCH v2 0/7] KVM: arm64: More PMU/debug ID register fixes

[Marc Zyngier]

This is a respin of a series I posted in the 5.7 time frame, and
completely forgot about it until I noticed again that a few things
where not what I remembered... Given how long it has been, I'm
pretending this is all new work.

Anyway, this covers a few gaps in our ID register handling for PMU and
Debug, plus the upgrade of the PMU support to 8.4 when possible.

I don't plan to take this into 5.11, but this is a likely candidate
for 5.12. Note that this conflicts with fixes that are on their way to
5.11. I'll probably end-up dragging the fixes into 5.12 to solve it.

* From v1 [1]:
  - Upgrade AArch32 to PMUv8.4 if possible
  - Don't advertise STALL_SLOT when advertising PMU 8.4
  - Add an extra patch replacing raw values for the PMU version with
    the already existing symbolaic values

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

Marc Zyngier (7):
  KVM: arm64: Fix missing RES1 in emulation of DBGBIDR
  KVM: arm64: Fix AArch32 PMUv3 capping
  KVM: arm64: Add handling of AArch32 PCMEID{2,3} PMUv3 registers
  KVM: arm64: Refactor filtering of ID registers
  KVM: arm64: Limit the debug architecture to ARMv8.0
  KVM: arm64: Upgrade PMU support to ARMv8.4
  KVM: arm64: Use symbolic names for the PMU versions

 arch/arm64/include/asm/sysreg.h |  3 ++
 arch/arm64/kvm/pmu-emul.c       | 14 ++++--
 arch/arm64/kvm/sys_regs.c       | 79 ++++++++++++++++++++-------------
 3 files changed, 61 insertions(+), 35 deletions(-)

-- 
2.29.2

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

[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 <peter.maydell@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 3313dedfa505..0c0832472c4a 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -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;
 	}
 }
-- 
2.29.2

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

[Marc Zyngier]

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

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 0c0832472c4a..ce08d28ab15c 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -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;
-- 
2.29.2

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

[Marc Zyngier]

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.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index ce08d28ab15c..2bea0494b81d 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -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 },
-- 
2.29.2

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

[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.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 51 +++++++++++++++++++++------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2bea0494b81d..dda16d60197b 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -9,6 +9,7 @@
  *          Christoffer Dall <c.dall@virtualopensystems.com>
  */
 
+#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;
-- 
2.29.2

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

[Marc Zyngier]

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

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/sys_regs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index dda16d60197b..8f79ec1fffa7 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -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,
-- 
2.29.2

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

[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 <maz@kernel.org>
---
 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(-)

diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 8b5e7e5c3cc8..2fb3f386588c 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -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
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 398f6df1bbe4..72cd704a8368 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -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;
 	}
 
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 8f79ec1fffa7..5da536ab738d 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -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 },
-- 
2.29.2

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

[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 <maz@kernel.org>
---
 arch/arm64/kvm/pmu-emul.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 72cd704a8368..cb16ca2eee92 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -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);
-- 
2.29.2

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

[Alexandru Elisei]

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:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

On 1/25/21 12:26 PM, Marc Zyngier wrote:
> 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 <peter.maydell@linaro.org>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/sys_regs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 3313dedfa505..0c0832472c4a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -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;
>  	}
>  }

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

[Alexandru Elisei]

Hi Marc,

This matches what we do for ID_AA64DFR0_EL1:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

On 1/25/21 12:26 PM, Marc Zyngier wrote:
> We shouldn't expose *any* PMU capability when no PMU has been
> configured for this VM.
>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/sys_regs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 0c0832472c4a..ce08d28ab15c 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -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;

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

[Alexandru Elisei]

Hi Marc,

On 1/25/21 12:26 PM, Marc Zyngier wrote:
> 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.
>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/sys_regs.c | 51 +++++++++++++++++++++------------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2bea0494b81d..dda16d60197b 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -9,6 +9,7 @@
>   *          Christoffer Dall <c.dall@virtualopensystems.com>
>   */
>  
> +#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);

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:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

> +		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;

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

[Alexandru Elisei]

Hi Marc,

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

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

On 1/25/21 12:26 PM, Marc Zyngier wrote:
> Let's not pretend we support anything but ARMv8.0 as far as the
> debug architecture is concerned.
>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/sys_regs.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index dda16d60197b..8f79ec1fffa7 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -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,

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

[Alexandru Elisei]

Hi Marc,

On 1/25/21 12:26 PM, Marc Zyngier wrote:
> 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
> Let's just do that and adjust what we return to the guest.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  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(-)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 8b5e7e5c3cc8..2fb3f386588c 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -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
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 398f6df1bbe4..72cd704a8368 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -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;
>  	}
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 8f79ec1fffa7..5da536ab738d 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -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 },

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

[Alexandru Elisei]

Hi Marc,

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

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

On 1/25/21 12:26 PM, Marc Zyngier wrote:
> 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 <maz@kernel.org>
> ---
>  arch/arm64/kvm/pmu-emul.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 72cd704a8368..cb16ca2eee92 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -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);

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

[Marc Zyngier]

Hi Alex,

On 2021-01-27 14:09, Alexandru Elisei wrote:
> Hi Marc,
> 
> On 1/25/21 12:26 PM, Marc Zyngier wrote:
>> 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?

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.
-- 
Jazz is not dead. It just smells funny...

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

[Alexandru Elisei]

Hi Marc,

On 1/27/21 2:35 PM, Marc Zyngier wrote:
> Hi Alex,
>
> On 2021-01-27 14:09, Alexandru Elisei wrote:
>> Hi Marc,
>>
>> On 1/25/21 12:26 PM, Marc Zyngier wrote:
>>> 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?
>
> 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.
>
> 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

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

[Marc Zyngier]

On 2021-01-27 17:00, Alexandru Elisei wrote:
> Hi Marc,
> 
> On 1/27/21 2:35 PM, Marc Zyngier wrote:
>> Hi Alex,
>> 
>> On 2021-01-27 14:09, Alexandru Elisei wrote:
>>> Hi Marc,
>>> 
>>> On 1/25/21 12:26 PM, Marc Zyngier wrote:
>>>> 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?
>> 
>> 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.
>> 
>> 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).

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.
-- 
Jazz is not dead. It just smells funny...

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

[Alexandru Elisei]

Hi Marc,

Had another look at the patch, comments below.

On 1/25/21 12:26 PM, Marc Zyngier wrote:
> 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 <maz@kernel.org>
> ---
>  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(-)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 8b5e7e5c3cc8..2fb3f386588c 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -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
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 398f6df1bbe4..72cd704a8368 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -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

>  		base = 32;
>  	}
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 8f79ec1fffa7..5da536ab738d 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -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 },

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

[Auger Eric]

Hi Marc,

On 1/25/21 1:26 PM, Marc Zyngier wrote:
> 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 <maz@kernel.org>
> ---
>  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(-)
> 
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 8b5e7e5c3cc8..2fb3f386588c 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -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
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 398f6df1bbe4..72cd704a8368 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -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?
>  		base = 32;
>  	}
>  
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 8f79ec1fffa7..5da536ab738d 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -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 },
"KVM: arm64: Hide PMU registers from userspace when not available"
changed the above, doesn't it?
> +	{ 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 },
> 
Thanks

Eric

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

[Auger Eric]

Hi Marc,

On 1/25/21 1:26 PM, Marc Zyngier wrote:
> 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 <maz@kernel.org>
> ---
>  arch/arm64/kvm/pmu-emul.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 72cd704a8368..cb16ca2eee92 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -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);
> 
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

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

[Marc Zyngier]

On 2021-01-27 17:41, Alexandru Elisei wrote:
> Hi Marc,
> 
> Had another look at the patch, comments below.
> 
> On 1/25/21 12:26 PM, Marc Zyngier wrote:
>> 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 <maz@kernel.org>
>> ---
>>  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(-)
>> 
>> diff --git a/arch/arm64/include/asm/sysreg.h 
>> b/arch/arm64/include/asm/sysreg.h
>> index 8b5e7e5c3cc8..2fb3f386588c 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -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
>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>> index 398f6df1bbe4..72cd704a8368 100644
>> --- a/arch/arm64/kvm/pmu-emul.c
>> +++ b/arch/arm64/kvm/pmu-emul.c
>> @@ -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).

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).

> 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.
-- 
Jazz is not dead. It just smells funny...

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

[Marc Zyngier]

Hi Eric,

On 2021-01-27 17:53, Auger Eric wrote:
> Hi Marc,
> 
> On 1/25/21 1:26 PM, Marc Zyngier wrote:
>> 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 <maz@kernel.org>
>> ---
>>  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(-)
>> 
>> diff --git a/arch/arm64/include/asm/sysreg.h 
>> b/arch/arm64/include/asm/sysreg.h
>> index 8b5e7e5c3cc8..2fb3f386588c 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -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
>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>> index 398f6df1bbe4..72cd704a8368 100644
>> --- a/arch/arm64/kvm/pmu-emul.c
>> +++ b/arch/arm64/kvm/pmu-emul.c
>> @@ -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?

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

>>  		base = 32;
>>  	}
>> 
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index 8f79ec1fffa7..5da536ab738d 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -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 },
> "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.
-- 
Jazz is not dead. It just smells funny...

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

[Auger Eric]

Hi Marc,
On 2/3/21 11:36 AM, Marc Zyngier wrote:
> Hi Eric,
> 
> On 2021-01-27 17:53, Auger Eric wrote:
>> Hi Marc,
>>
>> On 1/25/21 1:26 PM, Marc Zyngier wrote:
>>> 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 <maz@kernel.org>
>>> ---
>>>  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(-)
>>>
>>> diff --git a/arch/arm64/include/asm/sysreg.h
>>> b/arch/arm64/include/asm/sysreg.h
>>> index 8b5e7e5c3cc8..2fb3f386588c 100644
>>> --- a/arch/arm64/include/asm/sysreg.h
>>> +++ b/arch/arm64/include/asm/sysreg.h
>>> @@ -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
>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>>> index 398f6df1bbe4..72cd704a8368 100644
>>> --- a/arch/arm64/kvm/pmu-emul.c
>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>> @@ -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?
> 
> 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

> 
>>>          base = 32;
>>>      }
>>>
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>>> index 8f79ec1fffa7..5da536ab738d 100644
>>> --- a/arch/arm64/kvm/sys_regs.c
>>> +++ b/arch/arm64/kvm/sys_regs.c
>>> @@ -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 },
>> "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.

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

[Marc Zyngier]

On 2021-02-03 11:07, Auger Eric wrote:
> Hi Marc,
> On 2/3/21 11:36 AM, Marc Zyngier wrote:
>> Hi Eric,
>> 
>> On 2021-01-27 17:53, Auger Eric wrote:
>>> Hi Marc,
>>> 
>>> On 1/25/21 1:26 PM, Marc Zyngier wrote:
>>>> 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 <maz@kernel.org>
>>>> ---
>>>>  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(-)
>>>> 
>>>> diff --git a/arch/arm64/include/asm/sysreg.h
>>>> b/arch/arm64/include/asm/sysreg.h
>>>> index 8b5e7e5c3cc8..2fb3f386588c 100644
>>>> --- a/arch/arm64/include/asm/sysreg.h
>>>> +++ b/arch/arm64/include/asm/sysreg.h
>>>> @@ -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
>>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>>>> index 398f6df1bbe4..72cd704a8368 100644
>>>> --- a/arch/arm64/kvm/pmu-emul.c
>>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>>> @@ -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?
>> 
>> 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.
> "

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.
-- 
Jazz is not dead. It just smells funny...

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

[Auger Eric]

Hi,

On 2/3/21 12:20 PM, Marc Zyngier wrote:
> On 2021-02-03 11:07, Auger Eric wrote:
>> Hi Marc,
>> On 2/3/21 11:36 AM, Marc Zyngier wrote:
>>> Hi Eric,
>>>
>>> On 2021-01-27 17:53, Auger Eric wrote:
>>>> Hi Marc,
>>>>
>>>> On 1/25/21 1:26 PM, Marc Zyngier wrote:
>>>>> 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 <maz@kernel.org>
>>>>> ---
>>>>>  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(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/sysreg.h
>>>>> b/arch/arm64/include/asm/sysreg.h
>>>>> index 8b5e7e5c3cc8..2fb3f386588c 100644
>>>>> --- a/arch/arm64/include/asm/sysreg.h
>>>>> +++ b/arch/arm64/include/asm/sysreg.h
>>>>> @@ -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
>>>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>>>>> index 398f6df1bbe4..72cd704a8368 100644
>>>>> --- a/arch/arm64/kvm/pmu-emul.c
>>>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>>>> @@ -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?
>>>
>>> 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.
>> "
> 
> 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)
OK
> 
> - 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
> 
> Thanks,
> 
>          M.

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

[Marc Zyngier]

On 2021-02-03 12:39, Auger Eric wrote:
> Hi,
> 
> On 2/3/21 12:20 PM, Marc Zyngier wrote:
>> On 2021-02-03 11:07, Auger Eric wrote:
>>> Hi Marc,
>>> On 2/3/21 11:36 AM, Marc Zyngier wrote:
>>>> Hi Eric,
>>>> 
>>>> On 2021-01-27 17:53, Auger Eric wrote:
>>>>> Hi Marc,
>>>>> 
>>>>> On 1/25/21 1:26 PM, Marc Zyngier wrote:
>>>>>> 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 <maz@kernel.org>
>>>>>> ---
>>>>>>  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(-)
>>>>>> 
>>>>>> diff --git a/arch/arm64/include/asm/sysreg.h
>>>>>> b/arch/arm64/include/asm/sysreg.h
>>>>>> index 8b5e7e5c3cc8..2fb3f386588c 100644
>>>>>> --- a/arch/arm64/include/asm/sysreg.h
>>>>>> +++ b/arch/arm64/include/asm/sysreg.h
>>>>>> @@ -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
>>>>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>>>>>> index 398f6df1bbe4..72cd704a8368 100644
>>>>>> --- a/arch/arm64/kvm/pmu-emul.c
>>>>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>>>>> @@ -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?
>>>> 
>>>> 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.
>>> "
>> 
>> 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)
> OK
>> 
>> - 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"?

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.
-- 
Jazz is not dead. It just smells funny...

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

[Alexandru Elisei]

Hi Marc,

On 2/3/21 10:32 AM, Marc Zyngier wrote:
> On 2021-01-27 17:41, Alexandru Elisei wrote:
>> Hi Marc,
>>
>> Had another look at the patch, comments below.
>>
>> On 1/25/21 12:26 PM, Marc Zyngier wrote:
>>> 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 <maz@kernel.org>
>>> ---
>>>  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(-)
>>>
>>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
>>> index 8b5e7e5c3cc8..2fb3f386588c 100644
>>> --- a/arch/arm64/include/asm/sysreg.h
>>> +++ b/arch/arm64/include/asm/sysreg.h
>>> @@ -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
>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>>> index 398f6df1bbe4..72cd704a8368 100644
>>> --- a/arch/arm64/kvm/pmu-emul.c
>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>> @@ -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).
>
> 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).
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!
>
>> 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

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

[Alexandru Elisei]

Hi,

On 2/3/21 1:28 PM, Marc Zyngier wrote:
> On 2021-02-03 12:39, Auger Eric wrote:
>> Hi,
>>
>> On 2/3/21 12:20 PM, Marc Zyngier wrote:
>>> On 2021-02-03 11:07, Auger Eric wrote:
>>>> Hi Marc,
>>>> On 2/3/21 11:36 AM, Marc Zyngier wrote:
>>>>> Hi Eric,
>>>>>
>>>>> On 2021-01-27 17:53, Auger Eric wrote:
>>>>>> Hi Marc,
>>>>>>
>>>>>> On 1/25/21 1:26 PM, Marc Zyngier wrote:
>>>>>>> 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 <maz@kernel.org>
>>>>>>> ---
>>>>>>>  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(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/include/asm/sysreg.h
>>>>>>> b/arch/arm64/include/asm/sysreg.h
>>>>>>> index 8b5e7e5c3cc8..2fb3f386588c 100644
>>>>>>> --- a/arch/arm64/include/asm/sysreg.h
>>>>>>> +++ b/arch/arm64/include/asm/sysreg.h
>>>>>>> @@ -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
>>>>>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>>>>>>> index 398f6df1bbe4..72cd704a8368 100644
>>>>>>> --- a/arch/arm64/kvm/pmu-emul.c
>>>>>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>>>>>> @@ -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?
>>>>>
>>>>> 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.
>>>> "
>>>
>>> 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)
>> OK
>>>
>>> - 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"?
>
> 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

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

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

[Marc Zyngier]

On 2021-02-04 12:32, Alexandru Elisei wrote:
> Hi,
> 
> On 2/3/21 1:28 PM, Marc Zyngier wrote:
>> On 2021-02-03 12:39, Auger Eric wrote:
>>> Hi,
>>> 
>>> On 2/3/21 12:20 PM, Marc Zyngier wrote:
>>>> On 2021-02-03 11:07, Auger Eric wrote:
>>>>> Hi Marc,
>>>>> On 2/3/21 11:36 AM, Marc Zyngier wrote:
>>>>>> Hi Eric,
>>>>>> 
>>>>>> On 2021-01-27 17:53, Auger Eric wrote:
>>>>>>> Hi Marc,
>>>>>>> 
>>>>>>> On 1/25/21 1:26 PM, Marc Zyngier wrote:
>>>>>>>> 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 <maz@kernel.org>
>>>>>>>> ---
>>>>>>>>  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(-)
>>>>>>>> 
>>>>>>>> diff --git a/arch/arm64/include/asm/sysreg.h
>>>>>>>> b/arch/arm64/include/asm/sysreg.h
>>>>>>>> index 8b5e7e5c3cc8..2fb3f386588c 100644
>>>>>>>> --- a/arch/arm64/include/asm/sysreg.h
>>>>>>>> +++ b/arch/arm64/include/asm/sysreg.h
>>>>>>>> @@ -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
>>>>>>>> diff --git a/arch/arm64/kvm/pmu-emul.c 
>>>>>>>> b/arch/arm64/kvm/pmu-emul.c
>>>>>>>> index 398f6df1bbe4..72cd704a8368 100644
>>>>>>>> --- a/arch/arm64/kvm/pmu-emul.c
>>>>>>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>>>>>>> @@ -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?
>>>>>> 
>>>>>> 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.
>>>>> "
>>>> 
>>>> 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)
>>> OK
>>>> 
>>>> - 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"?
>> 
>> 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.

> 
> 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.

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

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...