[PATCH v2 0/2] KVM: arm64: PMU event filtering fixes

[PATCH v2 0/2] KVM: arm64: PMU event filtering fixes

[Oliver Upton]

Set of fixes to KVM's handling of the exception level event filtering in
the PMU event type registers.

I dropped the PMU+NV disablement this time around as we need a complete
fix for that problem. At the same time, I want to get a rework of our
sysreg masks upstream soon to avoid any negative interaction with new
PMU features going in on the driver side of things.

Additionally, I added a fix for the non-secure filtering bits that
Suzuki had spotted (thanks!)

Oliver Upton (2):
  KVM: arm64: Treat PMEVTYPER<n>_EL0.NSH as RES0
  KVM: arm64: Virtualise PMEVTYPER<n>_EL1.{NSU,NSK}

 arch/arm64/kvm/pmu-emul.c      | 26 +++++++++++++++++---------
 arch/arm64/kvm/sys_regs.c      |  8 ++++++--
 include/kvm/arm_pmu.h          |  5 +++++
 include/linux/perf/arm_pmuv3.h |  8 +++++---
 4 files changed, 33 insertions(+), 14 deletions(-)


base-commit: 6465e260f48790807eef06b583b38ca9789b6072
-- 
2.42.0.655.g421f12c284-goog

[PATCH v2 1/2] KVM: arm64: Treat PMEVTYPER<n>_EL0.NSH as RES0

[Oliver Upton]

Prevent the guest from setting the NSH bit, which enables event counting
while the PE is in EL2. kvm_pmu_create_perf_event() never wired up the
bit, nor does it make any sense in the context of a guest without NV.

While at it, build the event type mask using explicit field definitions
instead of relying on ARMV8_PMU_EVTYPE_MASK. KVM probably should've been
doing this in the first place, as it avoids changes to the
aforementioned mask affecting sysreg emulation.

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/pmu-emul.c | 15 ++++++++-------
 arch/arm64/kvm/sys_regs.c |  8 ++++++--
 include/kvm/arm_pmu.h     |  5 +++++
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 6b066e04dc5d..f0d4a9ace5ad 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -60,6 +60,12 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
 	return __kvm_pmu_event_mask(pmuver);
 }
 
+u64 kvm_pmu_evtyper_mask(struct kvm *kvm)
+{
+	return ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0 |
+	       kvm_pmu_event_mask(kvm);
+}
+
 /**
  * kvm_pmc_is_64bit - determine if counter is 64bit
  * @pmc: counter context
@@ -657,18 +663,13 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
 				    u64 select_idx)
 {
 	struct kvm_pmc *pmc = kvm_vcpu_idx_to_pmc(vcpu, select_idx);
-	u64 reg, mask;
+	u64 reg;
 
 	if (!kvm_vcpu_has_pmu(vcpu))
 		return;
 
-	mask  =  ARMV8_PMU_EVTYPE_MASK;
-	mask &= ~ARMV8_PMU_EVTYPE_EVENT;
-	mask |= kvm_pmu_event_mask(vcpu->kvm);
-
 	reg = counter_index_to_evtreg(pmc->idx);
-
-	__vcpu_sys_reg(vcpu, reg) = data & mask;
+	__vcpu_sys_reg(vcpu, reg) = data & kvm_pmu_evtyper_mask(vcpu->kvm);
 
 	kvm_pmu_create_perf_event(pmc);
 }
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index e92ec810d449..78720c373904 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -746,8 +746,12 @@ static u64 reset_pmevcntr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 
 static u64 reset_pmevtyper(struct kvm_vcpu *vcpu, const struct sys_reg_desc *r)
 {
+	/* This thing will UNDEF, who cares about the reset value? */
+	if (!kvm_vcpu_has_pmu(vcpu))
+		return 0;
+
 	reset_unknown(vcpu, r);
-	__vcpu_sys_reg(vcpu, r->reg) &= ARMV8_PMU_EVTYPE_MASK;
+	__vcpu_sys_reg(vcpu, r->reg) &= kvm_pmu_evtyper_mask(vcpu->kvm);
 
 	return __vcpu_sys_reg(vcpu, r->reg);
 }
@@ -988,7 +992,7 @@ static bool access_pmu_evtyper(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 		kvm_pmu_set_counter_event_type(vcpu, p->regval, idx);
 		kvm_vcpu_pmu_restore_guest(vcpu);
 	} else {
-		p->regval = __vcpu_sys_reg(vcpu, reg) & ARMV8_PMU_EVTYPE_MASK;
+		p->regval = __vcpu_sys_reg(vcpu, reg);
 	}
 
 	return true;
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 31029f4f7be8..e0bcf447a2ab 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -101,6 +101,7 @@ void kvm_vcpu_pmu_resync_el0(void);
 })
 
 u8 kvm_arm_pmu_get_pmuver_limit(void);
+u64 kvm_pmu_evtyper_mask(struct kvm *kvm);
 
 #else
 struct kvm_pmu {
@@ -172,6 +173,10 @@ static inline u8 kvm_arm_pmu_get_pmuver_limit(void)
 {
 	return 0;
 }
+static inline u64 kvm_pmu_evtyper_mask(void)
+{
+	return 0;
+}
 static inline void kvm_vcpu_pmu_resync_el0(void) {}
 
 #endif
-- 
2.42.0.655.g421f12c284-goog

[PATCH v2 2/2] KVM: arm64: Virtualise PMEVTYPER<n>_EL1.{NSU,NSK}

[Oliver Upton]

Suzuki noticed that KVM's PMU emulation is oblivious to the NSU and NSK
event filter bits. On systems that have EL3 these bits modify the
filter behavior in non-secure EL0 and EL1, respectively. Even though the
kernel doesn't use these bits, it is entirely possible some other guest
OS does.

Implement the behavior of NSU and NSK as it appears in the pseudocode.

Reported-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/kvm/pmu-emul.c      | 11 +++++++++--
 include/linux/perf/arm_pmuv3.h |  8 +++++---
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index f0d4a9ace5ad..d28e0e989c98 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -63,6 +63,7 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
 u64 kvm_pmu_evtyper_mask(struct kvm *kvm)
 {
 	return ARMV8_PMU_EXCLUDE_EL1 | ARMV8_PMU_EXCLUDE_EL0 |
+	       ARMV8_PMU_EXCLUDE_NS_EL1 | ARMV8_PMU_EXCLUDE_NS_EL0 |
 	       kvm_pmu_event_mask(kvm);
 }
 
@@ -590,6 +591,7 @@ static void kvm_pmu_create_perf_event(struct kvm_pmc *pmc)
 	struct perf_event *event;
 	struct perf_event_attr attr;
 	u64 eventsel, reg, data;
+	bool p, u, nsk, nsu;
 
 	reg = counter_index_to_evtreg(pmc->idx);
 	data = __vcpu_sys_reg(vcpu, reg);
@@ -616,13 +618,18 @@ static void kvm_pmu_create_perf_event(struct kvm_pmc *pmc)
 	    !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
 		return;
 
+	p = data & ARMV8_PMU_EXCLUDE_EL1;
+	u = data & ARMV8_PMU_EXCLUDE_EL0;
+	nsk = data & ARMV8_PMU_EXCLUDE_NS_EL1;
+	nsu = data & ARMV8_PMU_EXCLUDE_NS_EL0;
+
 	memset(&attr, 0, sizeof(struct perf_event_attr));
 	attr.type = arm_pmu->pmu.type;
 	attr.size = sizeof(attr);
 	attr.pinned = 1;
 	attr.disabled = !kvm_pmu_counter_is_enabled(pmc);
-	attr.exclude_user = data & ARMV8_PMU_EXCLUDE_EL0 ? 1 : 0;
-	attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
+	attr.exclude_user = (u != nsu);
+	attr.exclude_kernel = (p != nsk);
 	attr.exclude_hv = 1; /* Don't count EL2 events */
 	attr.exclude_host = 1; /* Don't count host events */
 	attr.config = eventsel;
diff --git a/include/linux/perf/arm_pmuv3.h b/include/linux/perf/arm_pmuv3.h
index e3899bd77f5c..b74e71da1fa7 100644
--- a/include/linux/perf/arm_pmuv3.h
+++ b/include/linux/perf/arm_pmuv3.h
@@ -234,9 +234,11 @@
 /*
  * Event filters for PMUv3
  */
-#define ARMV8_PMU_EXCLUDE_EL1	(1U << 31)
-#define ARMV8_PMU_EXCLUDE_EL0	(1U << 30)
-#define ARMV8_PMU_INCLUDE_EL2	(1U << 27)
+#define ARMV8_PMU_EXCLUDE_EL1		(1U << 31)
+#define ARMV8_PMU_EXCLUDE_EL0		(1U << 30)
+#define ARMV8_PMU_EXCLUDE_NS_EL1	(1U << 29)
+#define ARMV8_PMU_EXCLUDE_NS_EL0	(1U << 28)
+#define ARMV8_PMU_INCLUDE_EL2		(1U << 27)
 
 /*
  * PMUSERENR: user enable reg
-- 
2.42.0.655.g421f12c284-goog

Re: [PATCH v2 2/2] KVM: arm64: Virtualise PMEVTYPER<n>_EL1.{NSU,NSK}

[Oliver Upton]

On Fri, Oct 13, 2023 at 05:29:01AM +0000, Oliver Upton wrote:
> Suzuki noticed that KVM's PMU emulation is oblivious to the NSU and NSK
> event filter bits. On systems that have EL3 these bits modify the
> filter behavior in non-secure EL0 and EL1, respectively. Even though the
> kernel doesn't use these bits, it is entirely possible some other guest
> OS does.
> 
> Implement the behavior of NSU and NSK as it appears in the pseudocode.
> 
> Reported-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>

This is obviously EL0, not EL1. Assuing there's no respin, I intend to
fix up the shortlog after applying the series.

-- 
Thanks,
Oliver

Re: [PATCH v2 0/2] KVM: arm64: PMU event filtering fixes

[Suzuki K Poulose]

On 13/10/2023 06:28, Oliver Upton wrote:
> Set of fixes to KVM's handling of the exception level event filtering in
> the PMU event type registers.
> 
> I dropped the PMU+NV disablement this time around as we need a complete
> fix for that problem. At the same time, I want to get a rework of our
> sysreg masks upstream soon to avoid any negative interaction with new
> PMU features going in on the driver side of things.
> 
> Additionally, I added a fix for the non-secure filtering bits that
> Suzuki had spotted (thanks!)
> 
> Oliver Upton (2):
>    KVM: arm64: Treat PMEVTYPER<n>_EL0.NSH as RES0
>    KVM: arm64: Virtualise PMEVTYPER<n>_EL1.{NSU,NSK}
> 
>   arch/arm64/kvm/pmu-emul.c      | 26 +++++++++++++++++---------
>   arch/arm64/kvm/sys_regs.c      |  8 ++++++--
>   include/kvm/arm_pmu.h          |  5 +++++
>   include/linux/perf/arm_pmuv3.h |  8 +++++---
>   4 files changed, 33 insertions(+), 14 deletions(-)
> 
> 
> base-commit: 6465e260f48790807eef06b583b38ca9789b6072


For the series:

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

Re: [PATCH v2 2/2] KVM: arm64: Virtualise PMEVTYPER<n>_EL1.{NSU,NSK}

[Marc Zyngier]

On Fri, 13 Oct 2023 06:29:01 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Suzuki noticed that KVM's PMU emulation is oblivious to the NSU and NSK
> event filter bits. On systems that have EL3 these bits modify the
> filter behavior in non-secure EL0 and EL1, respectively. Even though the
> kernel doesn't use these bits, it is entirely possible some other guest
> OS does.

But what does it mean for KVM itself? We have no EL3 to speak of as
far as a guest is concerned. And the moment we allow things like
NSU/NSK to be set, why don't we allow M as well?

Thanks,

	M.

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

Re: [PATCH v2 2/2] KVM: arm64: Virtualise PMEVTYPER<n>_EL1.{NSU,NSK}

[Oliver Upton]

On Wed, Oct 18, 2023 at 02:31:11PM +0100, Marc Zyngier wrote:
> On Fri, 13 Oct 2023 06:29:01 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > Suzuki noticed that KVM's PMU emulation is oblivious to the NSU and NSK
> > event filter bits. On systems that have EL3 these bits modify the
> > filter behavior in non-secure EL0 and EL1, respectively. Even though the
> > kernel doesn't use these bits, it is entirely possible some other guest
> > OS does.
> 
> But what does it mean for KVM itself? We have no EL3 to speak of as
> far as a guest is concerned. And the moment we allow things like
> NSU/NSK to be set, why don't we allow M as well?

Yeah, we need to have a think about all these extra bits TBH.

KVM doesn't filter the advertised ELs in PFR0, so from the guest POV
both EL2 and EL3 could potentially be implemented by the vCPU. Based
on that I think the bits at least need to be stateful, even though KVM's
emulation will never let the guest count events in a higher EL.

My patches aren't even consistent with the above statement, as NSH gets
RES0 treatment and the NS{U,K} bits do not. So how about this:

 - If EL3 is advertised in the guest's ID registers NS{U,K}, and M can
   be set. NS{U,K} work as proposed, M is ignored in KVM emulation.

 - If EL2 is advertised in the guest's ID registers NSH can be set but
   is ignored in KVM emulation.

Thoughts?

-- 
Thanks,
Oliver

Re: [PATCH v2 2/2] KVM: arm64: Virtualise PMEVTYPER<n>_EL1.{NSU,NSK}

[Marc Zyngier]

On Wed, 18 Oct 2023 19:24:11 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> On Wed, Oct 18, 2023 at 02:31:11PM +0100, Marc Zyngier wrote:
> > On Fri, 13 Oct 2023 06:29:01 +0100,
> > Oliver Upton <oliver.upton@linux.dev> wrote:
> > > 
> > > Suzuki noticed that KVM's PMU emulation is oblivious to the NSU and NSK
> > > event filter bits. On systems that have EL3 these bits modify the
> > > filter behavior in non-secure EL0 and EL1, respectively. Even though the
> > > kernel doesn't use these bits, it is entirely possible some other guest
> > > OS does.
> > 
> > But what does it mean for KVM itself? We have no EL3 to speak of as
> > far as a guest is concerned. And the moment we allow things like
> > NSU/NSK to be set, why don't we allow M as well?
> 
> Yeah, we need to have a think about all these extra bits TBH.
> 
> KVM doesn't filter the advertised ELs in PFR0, so from the guest POV
> both EL2 and EL3 could potentially be implemented by the vCPU. Based
> on that I think the bits at least need to be stateful, even though KVM's
> emulation will never let the guest count events in a higher EL.
> 
> My patches aren't even consistent with the above statement, as NSH gets
> RES0 treatment and the NS{U,K} bits do not. So how about this:
> 
>  - If EL3 is advertised in the guest's ID registers NS{U,K}, and M can
>    be set. NS{U,K} work as proposed, M is ignored in KVM emulation.
> 
>  - If EL2 is advertised in the guest's ID registers NSH can be set but
>    is ignored in KVM emulation.
> 
> Thoughts?

This would be consistent with the pseudocode (and what KVM can
reasonably achieve at this stage).

Care to respin it?

Thanks,

	M.

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