[PATCH v2 0/5] KVM: arm64: PMU: Fix PMUver related handling for vPMU support

[PATCH v2 0/5] KVM: arm64: PMU: Fix PMUver related handling for vPMU support

[Reiji Watanabe]

This is a consolidated v2 of two separate series, [1] and [2].

On systems where the PMUVer is not uniform across all PEs,
KVM currently does not advertise PMUv3 to the guest,
even if userspace successfully runs KVM_ARM_VCPU_INIT with
KVM_ARM_VCPU_PMU_V3.
The patch-1 and patch-2 will address this inconsistent
behavior by disallowing userspace from configuring vPMU,
as such systems would be extremely uncommon and unlikely
to even use KVM (according to Marc [3]).

The patch-3 will fix improper use of the host's PMUver to
determine a valid range of PMU events for the guest (the
guest's PMUver should be used instead).

The patch-4 and patch-5 will try to hide the STALL_SLOT*
events unconditionally per Oliver's suggestion [4].

Presently, KVM hides the STALL_SLOT event depending on the
host PMU version, instead of the guest's PMU version, which
doesn't seem to be accurate, as it appears that older PMU than
PMUv3p4 could implement the event according to the Arm ARM.
Exposing the STALL_SLOT event without PMMIR_EL1 (supported
from PMUv3p4) for the guest won't be very useful though.
The patch-4 stops advertising the event for guest unconditionally,
rather than fixing or keeping the inaccurate checking to advertise
the event for the case, where it is not very useful.
The patch-5 stops advertising the STALL_SLOT_{FRONT,BACK}END
events to the guest, similar to the STALL_SLOT event, as when any
of these three events are implemented, all three of them should
be implemented, according to the Arm ARM.

This series is based on 6.5-rc3.

[1] https://lore.kernel.org/all/20230610061520.3026530-1-reijiw@google.com/
[2] https://lore.kernel.org/all/20230610194510.4146549-1-reijiw@google.com/
[3] https://lore.kernel.org/all/874jnqp73o.wl-maz@kernel.org/
[4] https://lore.kernel.org/all/ZIm1kdFBfXYMdfbV@linux.dev/

Reiji Watanabe (5):
  KVM: arm64: PMU: Use of pmuv3_implemented() instead of open-coded
    version
  KVM: arm64: PMU: Disallow vPMU on non-uniform PMUVer systems
  KVM: arm64: PMU: Avoid inappropriate use of host's PMUVer
  KVM: arm64: PMU: Don't advertise the STALL_SLOT event
  KVM: arm64: PMU: Don't advertise STALL_SLOT_{FRONTEND,BACKEND}

 arch/arm64/kvm/arm.c      |  1 +
 arch/arm64/kvm/pmu-emul.c | 44 ++++++++++++++++++++++++++-------------
 include/kvm/arm_pmu.h     |  3 +++
 3 files changed, 34 insertions(+), 14 deletions(-)


base-commit: 6eaae198076080886b9e7d57f4ae06fa782f90ef
-- 
2.41.0.585.gd2178a4bd4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/5] KVM: arm64: PMU: Use of pmuv3_implemented() instead of open-coded version

[Reiji Watanabe]

Replace the open-coded PMU version check with pmuv3_implemented()
to simplify the code.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 arch/arm64/kvm/pmu-emul.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 560650972478..dee83119e112 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -14,6 +14,7 @@
 #include <asm/kvm_emulate.h>
 #include <kvm/arm_pmu.h>
 #include <kvm/arm_vgic.h>
+#include <asm/arm_pmuv3.h>
 
 #define PERF_ATTR_CFG1_COUNTER_64BIT	BIT(0)
 
@@ -672,8 +673,7 @@ void kvm_host_pmu_init(struct arm_pmu *pmu)
 {
 	struct arm_pmu_entry *entry;
 
-	if (pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_NI ||
-	    pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
+	if (!pmuv3_implemented(pmu->pmuver))
 		return;
 
 	mutex_lock(&arm_pmus_lock);
-- 
2.41.0.585.gd2178a4bd4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/5] KVM: arm64: PMU: Disallow vPMU on non-uniform PMUVer systems

[Reiji Watanabe]

Disallow userspace from configuring vPMU for guests on systems
where the PMUVer is not uniform across all PEs.
KVM has not been advertising PMUv3 to the guests with vPMU on
such systems anyway, and such systems would be extremely
uncommon and unlikely to even use KVM.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 arch/arm64/kvm/arm.c      |  1 +
 arch/arm64/kvm/pmu-emul.c | 11 ++++++++---
 include/kvm/arm_pmu.h     |  3 +++
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 72dc53a75d1c..1d410dea21ac 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -2053,6 +2053,7 @@ static int __init init_subsystems(void)
 		goto out;
 
 	kvm_register_perf_callbacks(NULL);
+	kvm_pmu_init();
 
 out:
 	if (err)
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index dee83119e112..6fb5c59948a8 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -685,9 +685,6 @@ void kvm_host_pmu_init(struct arm_pmu *pmu)
 	entry->arm_pmu = pmu;
 	list_add_tail(&entry->entry, &arm_pmus);
 
-	if (list_is_singular(&arm_pmus))
-		static_branch_enable(&kvm_arm_pmu_available);
-
 out_unlock:
 	mutex_unlock(&arm_pmus_lock);
 }
@@ -1057,3 +1054,11 @@ u8 kvm_arm_pmu_get_pmuver_limit(void)
 					      ID_AA64DFR0_EL1_PMUVer_V3P5);
 	return FIELD_GET(ARM64_FEATURE_MASK(ID_AA64DFR0_EL1_PMUVer), tmp);
 }
+
+void kvm_pmu_init(void)
+{
+	u8 pmuver = kvm_arm_pmu_get_pmuver_limit();
+
+	if (pmuv3_implemented(pmuver))
+		static_branch_enable(&kvm_arm_pmu_available);
+}
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 847da6fc2713..9cf50e16305a 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -74,6 +74,7 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu);
 struct kvm_pmu_events *kvm_get_pmu_events(void);
 void kvm_vcpu_pmu_restore_guest(struct kvm_vcpu *vcpu);
 void kvm_vcpu_pmu_restore_host(struct kvm_vcpu *vcpu);
+void kvm_pmu_init(void);
 
 #define kvm_vcpu_has_pmu(vcpu)					\
 	(test_bit(KVM_ARM_VCPU_PMU_V3, (vcpu)->arch.features))
@@ -110,6 +111,8 @@ static inline bool kvm_arm_support_pmu_v3(void)
 	return false;
 }
 
+static inline void kvm_pmu_init(void) {};
+
 #define kvm_arm_pmu_irq_initialized(v)	(false)
 static inline u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu,
 					    u64 select_idx)
-- 
2.41.0.585.gd2178a4bd4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] KVM: arm64: PMU: Disallow vPMU on non-uniform PMUVer systems

[Oliver Upton]

On Fri, Jul 28, 2023 at 11:19:04AM -0700, Reiji Watanabe wrote:
> Disallow userspace from configuring vPMU for guests on systems
> where the PMUVer is not uniform across all PEs.
> KVM has not been advertising PMUv3 to the guests with vPMU on
> such systems anyway, and such systems would be extremely
> uncommon and unlikely to even use KVM.

This doesn't actually disallow userspace from configuring a vPMU, it
only hides the KVM cap. kvm_host_pmu_init() will still insert the host
PMU instance in the list of valid PMUs, and there doesn't appear to be
any check against the static key anywhere on that path.

FWIW, this static key is actually responsible for indicating whether KVM
supports context switching the PMU between host/guest. While vPMU obviously
depends on that, the perf subsystem also allows the host to program events
to count while the guest is running.

I actually prefer where we flip the static key, as PMU context switching
depends on both KVM support as well as the PMU driver coming up successfully.
Instead, you could hoist the check against the sanitised PMU version into
kvm_host_pmu_init(), maybe something like:

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 560650972478..f6a0e558472f 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -672,8 +672,11 @@ void kvm_host_pmu_init(struct arm_pmu *pmu)
 {
 	struct arm_pmu_entry *entry;
 
-	if (pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_NI ||
-	    pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
+	/*
+	 * Check the sanitised PMU version for the system, as KVM does not
+	 * support implementations where PMUv3 exists on a subset of CPUs.
+	 */
+	if (!pmuv3_implemented(kvm_arm_pmu_get_pmuver_limit()))
 		return;
 
 	mutex_lock(&arm_pmus_lock);

-- 
Thanks,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] KVM: arm64: PMU: Disallow vPMU on non-uniform PMUVer systems

[Reiji Watanabe]

Hi Oliver,

> This doesn't actually disallow userspace from configuring a vPMU, it
> only hides the KVM cap. kvm_host_pmu_init() will still insert the host
> PMU instance in the list of valid PMUs, and there doesn't appear to be
> any check against the static key anywhere on that path.

In v6.5-rc3, which I used as the base, or even in v6.5-rc4,
it appears kvm_reset_vcpu() checks against the static key.
So, the initial KVM_ARM_VCPU_INIT with vPMU configured will
fail on the systems.  Or am I missing something ? (Or is that
going to be removed by other patches that are already queued?)

But, right, it still insert the host PMU instance in the list,
which is unnecessary.

> I actually prefer where we flip the static key, as PMU context switching
> depends on both KVM support as well as the PMU driver coming up successfully.
> Instead, you could hoist the check against the sanitised PMU version into
> kvm_host_pmu_init(), maybe something like:

Thank you, it looks better.  I will fix this in v3.

Thank you,
Reiji

> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 560650972478..f6a0e558472f 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -672,8 +672,11 @@ void kvm_host_pmu_init(struct arm_pmu *pmu)
>  {
>         struct arm_pmu_entry *entry;
>
> -       if (pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_NI ||
> -           pmu->pmuver == ID_AA64DFR0_EL1_PMUVer_IMP_DEF)
> +       /*
> +        * Check the sanitised PMU version for the system, as KVM does not
> +        * support implementations where PMUv3 exists on a subset of CPUs.
> +        */
> +       if (!pmuv3_implemented(kvm_arm_pmu_get_pmuver_limit()))
>                 return;
>
>         mutex_lock(&arm_pmus_lock);
>
> --
> Thanks,
> Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] KVM: arm64: PMU: Disallow vPMU on non-uniform PMUVer systems

[Oliver Upton]

On Mon, Jul 31, 2023 at 04:54:04AM -0700, Reiji Watanabe wrote:
> Hi Oliver,
> 
> > This doesn't actually disallow userspace from configuring a vPMU, it
> > only hides the KVM cap. kvm_host_pmu_init() will still insert the host
> > PMU instance in the list of valid PMUs, and there doesn't appear to be
> > any check against the static key anywhere on that path.
> 
> In v6.5-rc3, which I used as the base, or even in v6.5-rc4,
> it appears kvm_reset_vcpu() checks against the static key.
> So, the initial KVM_ARM_VCPU_INIT with vPMU configured will
> fail on the systems.  Or am I missing something ? (Or is that
> going to be removed by other patches that are already queued?)

No, I definitely missed something, sorry for the noise! I had only
checked the PMU attributes, forgot about feature flags for a moment.

> > I actually prefer where we flip the static key, as PMU context switching
> > depends on both KVM support as well as the PMU driver coming up successfully.
> > Instead, you could hoist the check against the sanitised PMU version into
> > kvm_host_pmu_init(), maybe something like:
> 
> Thank you, it looks better.  I will fix this in v3.

Sounds good, thanks!

--
Best,
Oliver

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/5] KVM: arm64: PMU: Avoid inappropriate use of host's PMUVer

[Reiji Watanabe]

Avoid using the PMUVer of the host's PMU hardware to determine
the PMU event mask, except in one case, as the value of host's
PMUVer may differ from the value of ID_AA64DFR0_EL1.PMUVer for
the guest.

The exception case is when using the PMUVer to determine the
valid range of events for KVM_ARM_VCPU_PMU_V3_FILTER, as it has
been allowing userspace to specify events that are valid for
the PMU hardware, regardless of the value of the guest's
ID_AA64DFR0_EL1.PMUVer.  KVM will use a valid range of events
based on the value of the guest's ID_AA64DFR0_EL1.PMUVer,
in order to effectively filter events that the guest attempts
to program though.

Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 arch/arm64/kvm/pmu-emul.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 6fb5c59948a8..f0cbc9024bb7 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -36,12 +36,8 @@ static struct kvm_pmc *kvm_vcpu_idx_to_pmc(struct kvm_vcpu *vcpu, int cnt_idx)
 	return &vcpu->arch.pmu.pmc[cnt_idx];
 }
 
-static u32 kvm_pmu_event_mask(struct kvm *kvm)
+static u32 __kvm_pmu_event_mask(unsigned int pmuver)
 {
-	unsigned int pmuver;
-
-	pmuver = kvm->arch.arm_pmu->pmuver;
-
 	switch (pmuver) {
 	case ID_AA64DFR0_EL1_PMUVer_IMP:
 		return GENMASK(9, 0);
@@ -56,6 +52,14 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
 	}
 }
 
+static u32 kvm_pmu_event_mask(struct kvm *kvm)
+{
+	u64 dfr0 = IDREG(kvm, SYS_ID_AA64DFR0_EL1);
+	u8 pmuver = SYS_FIELD_GET(ID_AA64DFR0_EL1, PMUVer, dfr0);
+
+	return __kvm_pmu_event_mask(pmuver);
+}
+
 /**
  * kvm_pmc_is_64bit - determine if counter is 64bit
  * @pmc: counter context
@@ -947,11 +951,17 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		return 0;
 	}
 	case KVM_ARM_VCPU_PMU_V3_FILTER: {
+		u8 pmuver = kvm_arm_pmu_get_pmuver_limit();
 		struct kvm_pmu_event_filter __user *uaddr;
 		struct kvm_pmu_event_filter filter;
 		int nr_events;
 
-		nr_events = kvm_pmu_event_mask(kvm) + 1;
+		/*
+		 * Allow userspace to specify an event filter for the entire
+		 * event range supported by PMUVer of the hardware, rather
+		 * than the guest's PMUVer for KVM backward compatibility.
+		 */
+		nr_events = __kvm_pmu_event_mask(pmuver) + 1;
 
 		uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
 
-- 
2.41.0.585.gd2178a4bd4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/5] KVM: arm64: PMU: Don't advertise the STALL_SLOT event

[Reiji Watanabe]

Currently, KVM hides the STALL_SLOT event for guests if the
host PMU version is PMUv3p4 or newer, as PMMIR_EL1 is handled
as RAZ for the guests. But, this should be based on the guests'
PMU version (instead of the host PMU version), as an older PMU
that doesn't support PMMIR_EL1 could support the STALL_SLOT
event, according to the Arm ARM. Exposing the STALL_SLOT event
without PMMIR_EL1 won't be very useful anyway though.

Stop advertising the STALL_SLOT event for guests unconditionally,
rather than fixing or keeping the inaccurate checking to
advertise the event for the case, where it is not very useful.

Suggested-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 arch/arm64/kvm/pmu-emul.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index f0cbc9024bb7..68f44f893b44 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -754,8 +754,7 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
 		 * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
 		 * as RAZ
 		 */
-		if (vcpu->kvm->arch.arm_pmu->pmuver >= ID_AA64DFR0_EL1_PMUVer_V3P4)
-			val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
+		val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
 		base = 32;
 	}
 
-- 
2.41.0.585.gd2178a4bd4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/5] KVM: arm64: PMU: Don't advertise STALL_SLOT_{FRONTEND,BACKEND}

[Reiji Watanabe]

Don't advertise STALL_SLOT_{FRONT,BACK}END events to the guest,
similar to STALL_SLOT event, as when any of these three events
are implemented, all three of them should be implemented,
according to the Arm ARM.

Suggested-by: Oliver Upton <oliver.upton@linux.dev>
Signed-off-by: Reiji Watanabe <reijiw@google.com>
---
 arch/arm64/kvm/pmu-emul.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 68f44f893b44..47a27941163c 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -751,10 +751,12 @@ u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
 	} else {
 		val = read_sysreg(pmceid1_el0);
 		/*
-		 * Don't advertise STALL_SLOT, as PMMIR_EL0 is handled
+		 * Don't advertise STALL_SLOT*, as PMMIR_EL0 is handled
 		 * as RAZ
 		 */
-		val &= ~BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32);
+		val &= ~(BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT - 32) |
+			 BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT_FRONTEND - 32) |
+			 BIT_ULL(ARMV8_PMUV3_PERFCTR_STALL_SLOT_BACKEND - 32));
 		base = 32;
 	}
 
-- 
2.41.0.585.gd2178a4bd4-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel