[PATCH v2 0/2] KVM: arm64: Filtering PMU events

[PATCH v2 0/2] KVM: arm64: Filtering PMU events

[Marc Zyngier]

It is at times necessary to prevent a guest from being able to sample
certain events if multiple CPUs share resources such as a cache level. In
this case, it would be interesting if the VMM could simply prevent certain
events from being counted instead of hiding the PMU.

Given that most events are not architected, there is no easy way to
designate which events shouldn't be counted other than specifying the raw
event number.

Since I have no idea whether it is better to use an event whitelist or
blacklist, the proposed API takes a cue from the x86 version and allows
either allowing or denying counting of ranges of events. The event space
being pretty large (16bits on ARMv8.1), the default policy is set by the
first filter that gets installed (default deny if we first allow, default
allow if we first deny).

The filter state is global to the guest, despite the PMU being per CPU. I'm
not sure whether it would be worth it making it CPU-private.

As an example of what can be done in userspace, I have the corresponding
kvmtool hack here[1].

* From v1:
  - Cleaned up handling of the cycle counter
  - Documented restrictions on SW_INC, CHAIN and CPU_CYCLES events

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/kvmtool.git/commit/?h=pmu-filter

Marc Zyngier (2):
  KVM: arm64: Add PMU event filtering infrastructure
  KVM: arm64: Document PMU filtering API

 Documentation/virt/kvm/devices/vcpu.rst | 40 ++++++++++++
 arch/arm64/include/asm/kvm_host.h       |  6 ++
 arch/arm64/include/uapi/asm/kvm.h       | 16 +++++
 virt/kvm/arm/arm.c                      |  2 +
 virt/kvm/arm/pmu.c                      | 84 ++++++++++++++++++++-----
 5 files changed, 132 insertions(+), 16 deletions(-)

-- 
2.20.1

[PATCH v2 0/2] KVM: arm64: Filtering PMU events

[Marc Zyngier]

It is at times necessary to prevent a guest from being able to sample
certain events if multiple CPUs share resources such as a cache level. In
this case, it would be interesting if the VMM could simply prevent certain
events from being counted instead of hiding the PMU.

Given that most events are not architected, there is no easy way to
designate which events shouldn't be counted other than specifying the raw
event number.

Since I have no idea whether it is better to use an event whitelist or
blacklist, the proposed API takes a cue from the x86 version and allows
either allowing or denying counting of ranges of events. The event space
being pretty large (16bits on ARMv8.1), the default policy is set by the
first filter that gets installed (default deny if we first allow, default
allow if we first deny).

The filter state is global to the guest, despite the PMU being per CPU. I'm
not sure whether it would be worth it making it CPU-private.

As an example of what can be done in userspace, I have the corresponding
kvmtool hack here[1].

* From v1:
  - Cleaned up handling of the cycle counter
  - Documented restrictions on SW_INC, CHAIN and CPU_CYCLES events

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/kvmtool.git/commit/?h=pmu-filter

Marc Zyngier (2):
  KVM: arm64: Add PMU event filtering infrastructure
  KVM: arm64: Document PMU filtering API

 Documentation/virt/kvm/devices/vcpu.rst | 40 ++++++++++++
 arch/arm64/include/asm/kvm_host.h       |  6 ++
 arch/arm64/include/uapi/asm/kvm.h       | 16 +++++
 virt/kvm/arm/arm.c                      |  2 +
 virt/kvm/arm/pmu.c                      | 84 ++++++++++++++++++++-----
 5 files changed, 132 insertions(+), 16 deletions(-)

-- 
2.20.1


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

[PATCH v2 0/2] KVM: arm64: Filtering PMU events

[Marc Zyngier]

It is at times necessary to prevent a guest from being able to sample
certain events if multiple CPUs share resources such as a cache level. In
this case, it would be interesting if the VMM could simply prevent certain
events from being counted instead of hiding the PMU.

Given that most events are not architected, there is no easy way to
designate which events shouldn't be counted other than specifying the raw
event number.

Since I have no idea whether it is better to use an event whitelist or
blacklist, the proposed API takes a cue from the x86 version and allows
either allowing or denying counting of ranges of events. The event space
being pretty large (16bits on ARMv8.1), the default policy is set by the
first filter that gets installed (default deny if we first allow, default
allow if we first deny).

The filter state is global to the guest, despite the PMU being per CPU. I'm
not sure whether it would be worth it making it CPU-private.

As an example of what can be done in userspace, I have the corresponding
kvmtool hack here[1].

* From v1:
  - Cleaned up handling of the cycle counter
  - Documented restrictions on SW_INC, CHAIN and CPU_CYCLES events

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/kvmtool.git/commit/?h=pmu-filter

Marc Zyngier (2):
  KVM: arm64: Add PMU event filtering infrastructure
  KVM: arm64: Document PMU filtering API

 Documentation/virt/kvm/devices/vcpu.rst | 40 ++++++++++++
 arch/arm64/include/asm/kvm_host.h       |  6 ++
 arch/arm64/include/uapi/asm/kvm.h       | 16 +++++
 virt/kvm/arm/arm.c                      |  2 +
 virt/kvm/arm/pmu.c                      | 84 ++++++++++++++++++++-----
 5 files changed, 132 insertions(+), 16 deletions(-)

-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

It can be desirable to expose a PMU to a guest, and yet not want the
guest to be able to count some of the implemented events (because this
would give information on shared resources, for example.

For this, let's extend the PMUv3 device API, and offer a way to setup a
bitmap of the allowed events (the default being no bitmap, and thus no
filtering).

Userspace can thus allow/deny ranges of event. The default policy
depends on the "polarity" of the first filter setup (default deny if the
filter allows events, and default allow if the filter denies events).
This allows to setup exactly what is allowed for a given guest.

Note that although the ioctl is per-vcpu, the map of allowed events is
global to the VM (it can be setup from any vcpu until the vcpu PMU is
initialized).

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h |  6 +++
 arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
 virt/kvm/arm/arm.c                |  2 +
 virt/kvm/arm/pmu.c                | 84 +++++++++++++++++++++++++------
 4 files changed, 92 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 57fd46acd058..8e63c618688d 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -91,6 +91,12 @@ struct kvm_arch {
 	 * supported.
 	 */
 	bool return_nisv_io_abort_to_user;
+
+	/*
+	 * VM-wide PMU filter, implemented as a bitmap and big enough
+	 * for up to 65536 events
+	 */
+	unsigned long *pmu_filter;
 };
 
 #define KVM_NR_MEM_OBJS     40
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index ba85bb23f060..7b1511d6ce44 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -159,6 +159,21 @@ struct kvm_sync_regs {
 struct kvm_arch_memory_slot {
 };
 
+/*
+ * PMU filter structure. Describe a range of events with a particular
+ * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
+ */
+struct kvm_pmu_event_filter {
+	__u16	base_event;
+	__u16	nevents;
+
+#define KVM_PMU_EVENT_ALLOW	0
+#define KVM_PMU_EVENT_DENY	1
+
+	__u8	action;
+	__u8	pad[3];
+};
+
 /* for KVM_GET/SET_VCPU_EVENTS */
 struct kvm_vcpu_events {
 	struct {
@@ -329,6 +344,7 @@ struct kvm_vcpu_events {
 #define KVM_ARM_VCPU_PMU_V3_CTRL	0
 #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
 #define   KVM_ARM_VCPU_PMU_V3_INIT	1
+#define   KVM_ARM_VCPU_PMU_V3_FILTER	2
 #define KVM_ARM_VCPU_TIMER_CTRL		1
 #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
 #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index eda7b624eab8..8d849ac88a44 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 	free_percpu(kvm->arch.last_vcpu_ran);
 	kvm->arch.last_vcpu_ran = NULL;
 
+	bitmap_free(kvm->arch.pmu_filter);
+
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		if (kvm->vcpus[i]) {
 			kvm_vcpu_destroy(kvm->vcpus[i]);
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index f0d0312c0a55..9f0fd0224d5b 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
 
 	kvm_pmu_stop_counter(vcpu, pmc);
 	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
+	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
+		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
 
 	/* Software increment event does't need to be backed by a perf event */
-	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
-	    pmc->idx != ARMV8_PMU_CYCLE_IDX)
+	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
+		return;
+
+	/*
+	 * If we have a filter in place and that the event isn't allowed, do
+	 * not install a perf event either.
+	 */
+	if (vcpu->kvm->arch.pmu_filter &&
+	    !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
 		return;
 
 	memset(&attr, 0, sizeof(struct perf_event_attr));
@@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
 	attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
 	attr.exclude_hv = 1; /* Don't count EL2 events */
 	attr.exclude_host = 1; /* Don't count host events */
-	attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
-		ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
+	attr.config = eventsel;
 
 	counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
 
@@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 
 static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
 {
-	if (!kvm_arm_support_pmu_v3())
-		return -ENODEV;
-
-	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
-		return -ENXIO;
-
-	if (vcpu->arch.pmu.created)
-		return -EBUSY;
-
 	if (irqchip_in_kernel(vcpu->kvm)) {
 		int ret;
 
@@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
 	return true;
 }
 
+#define NR_EVENTS	(ARMV8_PMU_EVTYPE_EVENT + 1)
+
 int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 {
+	if (!kvm_arm_support_pmu_v3())
+		return -ENODEV;
+
+	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
+		return -ENODEV;
+
+	if (vcpu->arch.pmu.created)
+		return -EBUSY;
+
 	switch (attr->attr) {
 	case KVM_ARM_VCPU_PMU_V3_IRQ: {
 		int __user *uaddr = (int __user *)(long)attr->addr;
@@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		if (!irqchip_in_kernel(vcpu->kvm))
 			return -EINVAL;
 
-		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
-			return -ENODEV;
-
 		if (get_user(irq, uaddr))
 			return -EFAULT;
 
@@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		vcpu->arch.pmu.irq_num = irq;
 		return 0;
 	}
+	case KVM_ARM_VCPU_PMU_V3_FILTER: {
+		struct kvm_pmu_event_filter __user *uaddr;
+		struct kvm_pmu_event_filter filter;
+
+		uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
+
+		if (copy_from_user(&filter, uaddr, sizeof(filter)))
+			return -EFAULT;
+
+		if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
+		    (filter.action != KVM_PMU_EVENT_ALLOW &&
+		     filter.action != KVM_PMU_EVENT_DENY))
+			return -EINVAL;
+
+		mutex_lock(&vcpu->kvm->lock);
+
+		if (!vcpu->kvm->arch.pmu_filter) {
+			vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS, GFP_KERNEL);
+			if (!vcpu->kvm->arch.pmu_filter) {
+				mutex_unlock(&vcpu->kvm->lock);
+				return -ENOMEM;
+			}
+
+			/*
+			 * The default depends on the first applied filter.
+			 * If it allows events, the default is to deny.
+			 * Conversely, if the first filter denies a set of
+			 * events, the default is to allow.
+			 */
+			if (filter.action == KVM_PMU_EVENT_ALLOW)
+				bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
+			else
+				bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
+		}
+
+		if (filter.action == KVM_PMU_EVENT_ALLOW)
+			bitmap_set(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
+		else
+			bitmap_clear(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
+
+		mutex_unlock(&vcpu->kvm->lock);
+
+		return 0;
+	}
 	case KVM_ARM_VCPU_PMU_V3_INIT:
 		return kvm_arm_pmu_v3_init(vcpu);
 	}
@@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 	switch (attr->attr) {
 	case KVM_ARM_VCPU_PMU_V3_IRQ:
 	case KVM_ARM_VCPU_PMU_V3_INIT:
+	case KVM_ARM_VCPU_PMU_V3_FILTER:
 		if (kvm_arm_support_pmu_v3() &&
 		    test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
 			return 0;
-- 
2.20.1

[PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

It can be desirable to expose a PMU to a guest, and yet not want the
guest to be able to count some of the implemented events (because this
would give information on shared resources, for example.

For this, let's extend the PMUv3 device API, and offer a way to setup a
bitmap of the allowed events (the default being no bitmap, and thus no
filtering).

Userspace can thus allow/deny ranges of event. The default policy
depends on the "polarity" of the first filter setup (default deny if the
filter allows events, and default allow if the filter denies events).
This allows to setup exactly what is allowed for a given guest.

Note that although the ioctl is per-vcpu, the map of allowed events is
global to the VM (it can be setup from any vcpu until the vcpu PMU is
initialized).

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h |  6 +++
 arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
 virt/kvm/arm/arm.c                |  2 +
 virt/kvm/arm/pmu.c                | 84 +++++++++++++++++++++++++------
 4 files changed, 92 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 57fd46acd058..8e63c618688d 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -91,6 +91,12 @@ struct kvm_arch {
 	 * supported.
 	 */
 	bool return_nisv_io_abort_to_user;
+
+	/*
+	 * VM-wide PMU filter, implemented as a bitmap and big enough
+	 * for up to 65536 events
+	 */
+	unsigned long *pmu_filter;
 };
 
 #define KVM_NR_MEM_OBJS     40
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index ba85bb23f060..7b1511d6ce44 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -159,6 +159,21 @@ struct kvm_sync_regs {
 struct kvm_arch_memory_slot {
 };
 
+/*
+ * PMU filter structure. Describe a range of events with a particular
+ * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
+ */
+struct kvm_pmu_event_filter {
+	__u16	base_event;
+	__u16	nevents;
+
+#define KVM_PMU_EVENT_ALLOW	0
+#define KVM_PMU_EVENT_DENY	1
+
+	__u8	action;
+	__u8	pad[3];
+};
+
 /* for KVM_GET/SET_VCPU_EVENTS */
 struct kvm_vcpu_events {
 	struct {
@@ -329,6 +344,7 @@ struct kvm_vcpu_events {
 #define KVM_ARM_VCPU_PMU_V3_CTRL	0
 #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
 #define   KVM_ARM_VCPU_PMU_V3_INIT	1
+#define   KVM_ARM_VCPU_PMU_V3_FILTER	2
 #define KVM_ARM_VCPU_TIMER_CTRL		1
 #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
 #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index eda7b624eab8..8d849ac88a44 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 	free_percpu(kvm->arch.last_vcpu_ran);
 	kvm->arch.last_vcpu_ran = NULL;
 
+	bitmap_free(kvm->arch.pmu_filter);
+
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		if (kvm->vcpus[i]) {
 			kvm_vcpu_destroy(kvm->vcpus[i]);
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index f0d0312c0a55..9f0fd0224d5b 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
 
 	kvm_pmu_stop_counter(vcpu, pmc);
 	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
+	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
+		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
 
 	/* Software increment event does't need to be backed by a perf event */
-	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
-	    pmc->idx != ARMV8_PMU_CYCLE_IDX)
+	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
+		return;
+
+	/*
+	 * If we have a filter in place and that the event isn't allowed, do
+	 * not install a perf event either.
+	 */
+	if (vcpu->kvm->arch.pmu_filter &&
+	    !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
 		return;
 
 	memset(&attr, 0, sizeof(struct perf_event_attr));
@@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
 	attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
 	attr.exclude_hv = 1; /* Don't count EL2 events */
 	attr.exclude_host = 1; /* Don't count host events */
-	attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
-		ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
+	attr.config = eventsel;
 
 	counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
 
@@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 
 static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
 {
-	if (!kvm_arm_support_pmu_v3())
-		return -ENODEV;
-
-	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
-		return -ENXIO;
-
-	if (vcpu->arch.pmu.created)
-		return -EBUSY;
-
 	if (irqchip_in_kernel(vcpu->kvm)) {
 		int ret;
 
@@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
 	return true;
 }
 
+#define NR_EVENTS	(ARMV8_PMU_EVTYPE_EVENT + 1)
+
 int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 {
+	if (!kvm_arm_support_pmu_v3())
+		return -ENODEV;
+
+	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
+		return -ENODEV;
+
+	if (vcpu->arch.pmu.created)
+		return -EBUSY;
+
 	switch (attr->attr) {
 	case KVM_ARM_VCPU_PMU_V3_IRQ: {
 		int __user *uaddr = (int __user *)(long)attr->addr;
@@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		if (!irqchip_in_kernel(vcpu->kvm))
 			return -EINVAL;
 
-		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
-			return -ENODEV;
-
 		if (get_user(irq, uaddr))
 			return -EFAULT;
 
@@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		vcpu->arch.pmu.irq_num = irq;
 		return 0;
 	}
+	case KVM_ARM_VCPU_PMU_V3_FILTER: {
+		struct kvm_pmu_event_filter __user *uaddr;
+		struct kvm_pmu_event_filter filter;
+
+		uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
+
+		if (copy_from_user(&filter, uaddr, sizeof(filter)))
+			return -EFAULT;
+
+		if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
+		    (filter.action != KVM_PMU_EVENT_ALLOW &&
+		     filter.action != KVM_PMU_EVENT_DENY))
+			return -EINVAL;
+
+		mutex_lock(&vcpu->kvm->lock);
+
+		if (!vcpu->kvm->arch.pmu_filter) {
+			vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS, GFP_KERNEL);
+			if (!vcpu->kvm->arch.pmu_filter) {
+				mutex_unlock(&vcpu->kvm->lock);
+				return -ENOMEM;
+			}
+
+			/*
+			 * The default depends on the first applied filter.
+			 * If it allows events, the default is to deny.
+			 * Conversely, if the first filter denies a set of
+			 * events, the default is to allow.
+			 */
+			if (filter.action == KVM_PMU_EVENT_ALLOW)
+				bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
+			else
+				bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
+		}
+
+		if (filter.action == KVM_PMU_EVENT_ALLOW)
+			bitmap_set(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
+		else
+			bitmap_clear(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
+
+		mutex_unlock(&vcpu->kvm->lock);
+
+		return 0;
+	}
 	case KVM_ARM_VCPU_PMU_V3_INIT:
 		return kvm_arm_pmu_v3_init(vcpu);
 	}
@@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 	switch (attr->attr) {
 	case KVM_ARM_VCPU_PMU_V3_IRQ:
 	case KVM_ARM_VCPU_PMU_V3_INIT:
+	case KVM_ARM_VCPU_PMU_V3_FILTER:
 		if (kvm_arm_support_pmu_v3() &&
 		    test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
 			return 0;
-- 
2.20.1


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

[PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

It can be desirable to expose a PMU to a guest, and yet not want the
guest to be able to count some of the implemented events (because this
would give information on shared resources, for example.

For this, let's extend the PMUv3 device API, and offer a way to setup a
bitmap of the allowed events (the default being no bitmap, and thus no
filtering).

Userspace can thus allow/deny ranges of event. The default policy
depends on the "polarity" of the first filter setup (default deny if the
filter allows events, and default allow if the filter denies events).
This allows to setup exactly what is allowed for a given guest.

Note that although the ioctl is per-vcpu, the map of allowed events is
global to the VM (it can be setup from any vcpu until the vcpu PMU is
initialized).

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h |  6 +++
 arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
 virt/kvm/arm/arm.c                |  2 +
 virt/kvm/arm/pmu.c                | 84 +++++++++++++++++++++++++------
 4 files changed, 92 insertions(+), 16 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 57fd46acd058..8e63c618688d 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -91,6 +91,12 @@ struct kvm_arch {
 	 * supported.
 	 */
 	bool return_nisv_io_abort_to_user;
+
+	/*
+	 * VM-wide PMU filter, implemented as a bitmap and big enough
+	 * for up to 65536 events
+	 */
+	unsigned long *pmu_filter;
 };
 
 #define KVM_NR_MEM_OBJS     40
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index ba85bb23f060..7b1511d6ce44 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -159,6 +159,21 @@ struct kvm_sync_regs {
 struct kvm_arch_memory_slot {
 };
 
+/*
+ * PMU filter structure. Describe a range of events with a particular
+ * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
+ */
+struct kvm_pmu_event_filter {
+	__u16	base_event;
+	__u16	nevents;
+
+#define KVM_PMU_EVENT_ALLOW	0
+#define KVM_PMU_EVENT_DENY	1
+
+	__u8	action;
+	__u8	pad[3];
+};
+
 /* for KVM_GET/SET_VCPU_EVENTS */
 struct kvm_vcpu_events {
 	struct {
@@ -329,6 +344,7 @@ struct kvm_vcpu_events {
 #define KVM_ARM_VCPU_PMU_V3_CTRL	0
 #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
 #define   KVM_ARM_VCPU_PMU_V3_INIT	1
+#define   KVM_ARM_VCPU_PMU_V3_FILTER	2
 #define KVM_ARM_VCPU_TIMER_CTRL		1
 #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
 #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index eda7b624eab8..8d849ac88a44 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 	free_percpu(kvm->arch.last_vcpu_ran);
 	kvm->arch.last_vcpu_ran = NULL;
 
+	bitmap_free(kvm->arch.pmu_filter);
+
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
 		if (kvm->vcpus[i]) {
 			kvm_vcpu_destroy(kvm->vcpus[i]);
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index f0d0312c0a55..9f0fd0224d5b 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
 
 	kvm_pmu_stop_counter(vcpu, pmc);
 	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
+	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
+		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
 
 	/* Software increment event does't need to be backed by a perf event */
-	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
-	    pmc->idx != ARMV8_PMU_CYCLE_IDX)
+	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
+		return;
+
+	/*
+	 * If we have a filter in place and that the event isn't allowed, do
+	 * not install a perf event either.
+	 */
+	if (vcpu->kvm->arch.pmu_filter &&
+	    !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
 		return;
 
 	memset(&attr, 0, sizeof(struct perf_event_attr));
@@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
 	attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
 	attr.exclude_hv = 1; /* Don't count EL2 events */
 	attr.exclude_host = 1; /* Don't count host events */
-	attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
-		ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
+	attr.config = eventsel;
 
 	counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
 
@@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 
 static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
 {
-	if (!kvm_arm_support_pmu_v3())
-		return -ENODEV;
-
-	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
-		return -ENXIO;
-
-	if (vcpu->arch.pmu.created)
-		return -EBUSY;
-
 	if (irqchip_in_kernel(vcpu->kvm)) {
 		int ret;
 
@@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
 	return true;
 }
 
+#define NR_EVENTS	(ARMV8_PMU_EVTYPE_EVENT + 1)
+
 int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 {
+	if (!kvm_arm_support_pmu_v3())
+		return -ENODEV;
+
+	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
+		return -ENODEV;
+
+	if (vcpu->arch.pmu.created)
+		return -EBUSY;
+
 	switch (attr->attr) {
 	case KVM_ARM_VCPU_PMU_V3_IRQ: {
 		int __user *uaddr = (int __user *)(long)attr->addr;
@@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		if (!irqchip_in_kernel(vcpu->kvm))
 			return -EINVAL;
 
-		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
-			return -ENODEV;
-
 		if (get_user(irq, uaddr))
 			return -EFAULT;
 
@@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 		vcpu->arch.pmu.irq_num = irq;
 		return 0;
 	}
+	case KVM_ARM_VCPU_PMU_V3_FILTER: {
+		struct kvm_pmu_event_filter __user *uaddr;
+		struct kvm_pmu_event_filter filter;
+
+		uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
+
+		if (copy_from_user(&filter, uaddr, sizeof(filter)))
+			return -EFAULT;
+
+		if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
+		    (filter.action != KVM_PMU_EVENT_ALLOW &&
+		     filter.action != KVM_PMU_EVENT_DENY))
+			return -EINVAL;
+
+		mutex_lock(&vcpu->kvm->lock);
+
+		if (!vcpu->kvm->arch.pmu_filter) {
+			vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS, GFP_KERNEL);
+			if (!vcpu->kvm->arch.pmu_filter) {
+				mutex_unlock(&vcpu->kvm->lock);
+				return -ENOMEM;
+			}
+
+			/*
+			 * The default depends on the first applied filter.
+			 * If it allows events, the default is to deny.
+			 * Conversely, if the first filter denies a set of
+			 * events, the default is to allow.
+			 */
+			if (filter.action == KVM_PMU_EVENT_ALLOW)
+				bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
+			else
+				bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
+		}
+
+		if (filter.action == KVM_PMU_EVENT_ALLOW)
+			bitmap_set(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
+		else
+			bitmap_clear(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
+
+		mutex_unlock(&vcpu->kvm->lock);
+
+		return 0;
+	}
 	case KVM_ARM_VCPU_PMU_V3_INIT:
 		return kvm_arm_pmu_v3_init(vcpu);
 	}
@@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 	switch (attr->attr) {
 	case KVM_ARM_VCPU_PMU_V3_IRQ:
 	case KVM_ARM_VCPU_PMU_V3_INIT:
+	case KVM_ARM_VCPU_PMU_V3_FILTER:
 		if (kvm_arm_support_pmu_v3() &&
 		    test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
 			return 0;
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Auger Eric]

Hi Marc,

On 3/9/20 1:48 PM, Marc Zyngier wrote:
> It can be desirable to expose a PMU to a guest, and yet not want the
> guest to be able to count some of the implemented events (because this
> would give information on shared resources, for example.
> 
> For this, let's extend the PMUv3 device API, and offer a way to setup a
> bitmap of the allowed events (the default being no bitmap, and thus no
> filtering).
> 
> Userspace can thus allow/deny ranges of event. The default policy
> depends on the "polarity" of the first filter setup (default deny if the
> filter allows events, and default allow if the filter denies events).
> This allows to setup exactly what is allowed for a given guest.
> 
> Note that although the ioctl is per-vcpu, the map of allowed events is
> global to the VM (it can be setup from any vcpu until the vcpu PMU is
> initialized).
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h |  6 +++
>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>  virt/kvm/arm/arm.c                |  2 +
>  virt/kvm/arm/pmu.c                | 84 +++++++++++++++++++++++++------
>  4 files changed, 92 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 57fd46acd058..8e63c618688d 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -91,6 +91,12 @@ struct kvm_arch {
>  	 * supported.
>  	 */
>  	bool return_nisv_io_abort_to_user;
> +
> +	/*
> +	 * VM-wide PMU filter, implemented as a bitmap and big enough
> +	 * for up to 65536 events
> +	 */
> +	unsigned long *pmu_filter;
>  };
>  
>  #define KVM_NR_MEM_OBJS     40
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index ba85bb23f060..7b1511d6ce44 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>  struct kvm_arch_memory_slot {
>  };
>  
> +/*
> + * PMU filter structure. Describe a range of events with a particular
> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
> + */
> +struct kvm_pmu_event_filter {
> +	__u16	base_event;
> +	__u16	nevents;
> +
> +#define KVM_PMU_EVENT_ALLOW	0
> +#define KVM_PMU_EVENT_DENY	1
> +
> +	__u8	action;
> +	__u8	pad[3];
> +};
> +
>  /* for KVM_GET/SET_VCPU_EVENTS */
>  struct kvm_vcpu_events {
>  	struct {
> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
>  #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
>  #define   KVM_ARM_VCPU_PMU_V3_INIT	1
> +#define   KVM_ARM_VCPU_PMU_V3_FILTER	2
>  #define KVM_ARM_VCPU_TIMER_CTRL		1
>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index eda7b624eab8..8d849ac88a44 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>  	free_percpu(kvm->arch.last_vcpu_ran);
>  	kvm->arch.last_vcpu_ran = NULL;
>  
> +	bitmap_free(kvm->arch.pmu_filter);
> +
>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>  		if (kvm->vcpus[i]) {
>  			kvm_vcpu_destroy(kvm->vcpus[i]);
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index f0d0312c0a55..9f0fd0224d5b 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  
>  	kvm_pmu_stop_counter(vcpu, pmc);
>  	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
> +	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> +		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
nit:
	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
	else
		eventsel = data & ARMV8_PMU_EVTYPE_EVENT;

>  
>  	/* Software increment event does't need to be backed by a perf event */
nit: while wer are at it fix the does't typo
> -	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
> -	    pmc->idx != ARMV8_PMU_CYCLE_IDX)
> +	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
> +		return;
> +
> +	/*
> +	 * If we have a filter in place and that the event isn't allowed, do
> +	 * not install a perf event either.
> +	 */
> +	if (vcpu->kvm->arch.pmu_filter &&
> +	    !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>  		return;
>  
>  	memset(&attr, 0, sizeof(struct perf_event_attr));
> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  	attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>  	attr.exclude_hv = 1; /* Don't count EL2 events */
>  	attr.exclude_host = 1; /* Don't count host events */
> -	attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
> -		ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
> +	attr.config = eventsel;
So in that case the guest counter will not increment but the guest does
not know the counter is not implemented. Can't this lead to bad user
experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?
>  
>  	counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>  
> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>  
>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>  {
> -	if (!kvm_arm_support_pmu_v3())
> -		return -ENODEV;
> -
> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> -		return -ENXIO;
> -
> -	if (vcpu->arch.pmu.created)
> -		return -EBUSY;
> -
>  	if (irqchip_in_kernel(vcpu->kvm)) {
>  		int ret;
>  
> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
>  	return true;
>  }
>  
> +#define NR_EVENTS	(ARMV8_PMU_EVTYPE_EVENT + 1)
> +
>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  {
> +	if (!kvm_arm_support_pmu_v3())
> +		return -ENODEV;
> +
> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> +		return -ENODEV;
I see you changed -ENXIO into -ENODEV. wanted?
> +
> +	if (vcpu->arch.pmu.created)
> +		return -EBUSY;
> +
>  	switch (attr->attr) {
>  	case KVM_ARM_VCPU_PMU_V3_IRQ: {
>  		int __user *uaddr = (int __user *)(long)attr->addr;
> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  		if (!irqchip_in_kernel(vcpu->kvm))
>  			return -EINVAL;
>  
> -		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> -			return -ENODEV;
> -
>  		if (get_user(irq, uaddr))
>  			return -EFAULT;
>  
> @@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  		vcpu->arch.pmu.irq_num = irq;
>  		return 0;
>  	}
> +	case KVM_ARM_VCPU_PMU_V3_FILTER: {
> +		struct kvm_pmu_event_filter __user *uaddr;
> +		struct kvm_pmu_event_filter filter;
> +
> +		uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
> +
> +		if (copy_from_user(&filter, uaddr, sizeof(filter)))
> +			return -EFAULT;
> +
> +		if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
isnt't it >= ?
> +		    (filter.action != KVM_PMU_EVENT_ALLOW &&
> +		     filter.action != KVM_PMU_EVENT_DENY))
> +			return -EINVAL;
-EINVAL is not documented in the API doc.
> +
> +		mutex_lock(&vcpu->kvm->lock);
> +
> +		if (!vcpu->kvm->arch.pmu_filter) {
> +			vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS, GFP_KERNEL);
> +			if (!vcpu->kvm->arch.pmu_filter) {
> +				mutex_unlock(&vcpu->kvm->lock);
> +				return -ENOMEM;
> +			}
> +
> +			/*
> +			 * The default depends on the first applied filter.
> +			 * If it allows events, the default is to deny.
> +			 * Conversely, if the first filter denies a set of
> +			 * events, the default is to allow.
> +			 */
> +			if (filter.action == KVM_PMU_EVENT_ALLOW)
> +				bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
> +			else
> +				bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
> +		}
> +
> +		if (filter.action == KVM_PMU_EVENT_ALLOW)
> +			bitmap_set(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
> +		else
> +			bitmap_clear(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
> +
> +		mutex_unlock(&vcpu->kvm->lock);
> +
> +		return 0;
> +	}
>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>  		return kvm_arm_pmu_v3_init(vcpu);
>  	}
> @@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  	switch (attr->attr) {
>  	case KVM_ARM_VCPU_PMU_V3_IRQ:
not related to this patch but shouldn't we advertise this only with
in-kernel irqchip?
>  	case KVM_ARM_VCPU_PMU_V3_INIT:
> +	case KVM_ARM_VCPU_PMU_V3_FILTER:
>  		if (kvm_arm_support_pmu_v3() &&
>  		    test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>  			return 0;
> 
Thanks

Eric

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Auger Eric]

Hi Marc,

On 3/9/20 1:48 PM, Marc Zyngier wrote:
> It can be desirable to expose a PMU to a guest, and yet not want the
> guest to be able to count some of the implemented events (because this
> would give information on shared resources, for example.
> 
> For this, let's extend the PMUv3 device API, and offer a way to setup a
> bitmap of the allowed events (the default being no bitmap, and thus no
> filtering).
> 
> Userspace can thus allow/deny ranges of event. The default policy
> depends on the "polarity" of the first filter setup (default deny if the
> filter allows events, and default allow if the filter denies events).
> This allows to setup exactly what is allowed for a given guest.
> 
> Note that although the ioctl is per-vcpu, the map of allowed events is
> global to the VM (it can be setup from any vcpu until the vcpu PMU is
> initialized).
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h |  6 +++
>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>  virt/kvm/arm/arm.c                |  2 +
>  virt/kvm/arm/pmu.c                | 84 +++++++++++++++++++++++++------
>  4 files changed, 92 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 57fd46acd058..8e63c618688d 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -91,6 +91,12 @@ struct kvm_arch {
>  	 * supported.
>  	 */
>  	bool return_nisv_io_abort_to_user;
> +
> +	/*
> +	 * VM-wide PMU filter, implemented as a bitmap and big enough
> +	 * for up to 65536 events
> +	 */
> +	unsigned long *pmu_filter;
>  };
>  
>  #define KVM_NR_MEM_OBJS     40
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index ba85bb23f060..7b1511d6ce44 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>  struct kvm_arch_memory_slot {
>  };
>  
> +/*
> + * PMU filter structure. Describe a range of events with a particular
> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
> + */
> +struct kvm_pmu_event_filter {
> +	__u16	base_event;
> +	__u16	nevents;
> +
> +#define KVM_PMU_EVENT_ALLOW	0
> +#define KVM_PMU_EVENT_DENY	1
> +
> +	__u8	action;
> +	__u8	pad[3];
> +};
> +
>  /* for KVM_GET/SET_VCPU_EVENTS */
>  struct kvm_vcpu_events {
>  	struct {
> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
>  #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
>  #define   KVM_ARM_VCPU_PMU_V3_INIT	1
> +#define   KVM_ARM_VCPU_PMU_V3_FILTER	2
>  #define KVM_ARM_VCPU_TIMER_CTRL		1
>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index eda7b624eab8..8d849ac88a44 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>  	free_percpu(kvm->arch.last_vcpu_ran);
>  	kvm->arch.last_vcpu_ran = NULL;
>  
> +	bitmap_free(kvm->arch.pmu_filter);
> +
>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>  		if (kvm->vcpus[i]) {
>  			kvm_vcpu_destroy(kvm->vcpus[i]);
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index f0d0312c0a55..9f0fd0224d5b 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  
>  	kvm_pmu_stop_counter(vcpu, pmc);
>  	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
> +	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> +		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
nit:
	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
	else
		eventsel = data & ARMV8_PMU_EVTYPE_EVENT;

>  
>  	/* Software increment event does't need to be backed by a perf event */
nit: while wer are at it fix the does't typo
> -	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
> -	    pmc->idx != ARMV8_PMU_CYCLE_IDX)
> +	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
> +		return;
> +
> +	/*
> +	 * If we have a filter in place and that the event isn't allowed, do
> +	 * not install a perf event either.
> +	 */
> +	if (vcpu->kvm->arch.pmu_filter &&
> +	    !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>  		return;
>  
>  	memset(&attr, 0, sizeof(struct perf_event_attr));
> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  	attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>  	attr.exclude_hv = 1; /* Don't count EL2 events */
>  	attr.exclude_host = 1; /* Don't count host events */
> -	attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
> -		ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
> +	attr.config = eventsel;
So in that case the guest counter will not increment but the guest does
not know the counter is not implemented. Can't this lead to bad user
experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?
>  
>  	counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>  
> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>  
>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>  {
> -	if (!kvm_arm_support_pmu_v3())
> -		return -ENODEV;
> -
> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> -		return -ENXIO;
> -
> -	if (vcpu->arch.pmu.created)
> -		return -EBUSY;
> -
>  	if (irqchip_in_kernel(vcpu->kvm)) {
>  		int ret;
>  
> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
>  	return true;
>  }
>  
> +#define NR_EVENTS	(ARMV8_PMU_EVTYPE_EVENT + 1)
> +
>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  {
> +	if (!kvm_arm_support_pmu_v3())
> +		return -ENODEV;
> +
> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> +		return -ENODEV;
I see you changed -ENXIO into -ENODEV. wanted?
> +
> +	if (vcpu->arch.pmu.created)
> +		return -EBUSY;
> +
>  	switch (attr->attr) {
>  	case KVM_ARM_VCPU_PMU_V3_IRQ: {
>  		int __user *uaddr = (int __user *)(long)attr->addr;
> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  		if (!irqchip_in_kernel(vcpu->kvm))
>  			return -EINVAL;
>  
> -		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> -			return -ENODEV;
> -
>  		if (get_user(irq, uaddr))
>  			return -EFAULT;
>  
> @@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  		vcpu->arch.pmu.irq_num = irq;
>  		return 0;
>  	}
> +	case KVM_ARM_VCPU_PMU_V3_FILTER: {
> +		struct kvm_pmu_event_filter __user *uaddr;
> +		struct kvm_pmu_event_filter filter;
> +
> +		uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
> +
> +		if (copy_from_user(&filter, uaddr, sizeof(filter)))
> +			return -EFAULT;
> +
> +		if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
isnt't it >= ?
> +		    (filter.action != KVM_PMU_EVENT_ALLOW &&
> +		     filter.action != KVM_PMU_EVENT_DENY))
> +			return -EINVAL;
-EINVAL is not documented in the API doc.
> +
> +		mutex_lock(&vcpu->kvm->lock);
> +
> +		if (!vcpu->kvm->arch.pmu_filter) {
> +			vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS, GFP_KERNEL);
> +			if (!vcpu->kvm->arch.pmu_filter) {
> +				mutex_unlock(&vcpu->kvm->lock);
> +				return -ENOMEM;
> +			}
> +
> +			/*
> +			 * The default depends on the first applied filter.
> +			 * If it allows events, the default is to deny.
> +			 * Conversely, if the first filter denies a set of
> +			 * events, the default is to allow.
> +			 */
> +			if (filter.action == KVM_PMU_EVENT_ALLOW)
> +				bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
> +			else
> +				bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
> +		}
> +
> +		if (filter.action == KVM_PMU_EVENT_ALLOW)
> +			bitmap_set(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
> +		else
> +			bitmap_clear(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
> +
> +		mutex_unlock(&vcpu->kvm->lock);
> +
> +		return 0;
> +	}
>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>  		return kvm_arm_pmu_v3_init(vcpu);
>  	}
> @@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  	switch (attr->attr) {
>  	case KVM_ARM_VCPU_PMU_V3_IRQ:
not related to this patch but shouldn't we advertise this only with
in-kernel irqchip?
>  	case KVM_ARM_VCPU_PMU_V3_INIT:
> +	case KVM_ARM_VCPU_PMU_V3_FILTER:
>  		if (kvm_arm_support_pmu_v3() &&
>  		    test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>  			return 0;
> 
Thanks

Eric


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

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Auger Eric]

Hi Marc,

On 3/9/20 1:48 PM, Marc Zyngier wrote:
> It can be desirable to expose a PMU to a guest, and yet not want the
> guest to be able to count some of the implemented events (because this
> would give information on shared resources, for example.
> 
> For this, let's extend the PMUv3 device API, and offer a way to setup a
> bitmap of the allowed events (the default being no bitmap, and thus no
> filtering).
> 
> Userspace can thus allow/deny ranges of event. The default policy
> depends on the "polarity" of the first filter setup (default deny if the
> filter allows events, and default allow if the filter denies events).
> This allows to setup exactly what is allowed for a given guest.
> 
> Note that although the ioctl is per-vcpu, the map of allowed events is
> global to the VM (it can be setup from any vcpu until the vcpu PMU is
> initialized).
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h |  6 +++
>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>  virt/kvm/arm/arm.c                |  2 +
>  virt/kvm/arm/pmu.c                | 84 +++++++++++++++++++++++++------
>  4 files changed, 92 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 57fd46acd058..8e63c618688d 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -91,6 +91,12 @@ struct kvm_arch {
>  	 * supported.
>  	 */
>  	bool return_nisv_io_abort_to_user;
> +
> +	/*
> +	 * VM-wide PMU filter, implemented as a bitmap and big enough
> +	 * for up to 65536 events
> +	 */
> +	unsigned long *pmu_filter;
>  };
>  
>  #define KVM_NR_MEM_OBJS     40
> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> index ba85bb23f060..7b1511d6ce44 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>  struct kvm_arch_memory_slot {
>  };
>  
> +/*
> + * PMU filter structure. Describe a range of events with a particular
> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
> + */
> +struct kvm_pmu_event_filter {
> +	__u16	base_event;
> +	__u16	nevents;
> +
> +#define KVM_PMU_EVENT_ALLOW	0
> +#define KVM_PMU_EVENT_DENY	1
> +
> +	__u8	action;
> +	__u8	pad[3];
> +};
> +
>  /* for KVM_GET/SET_VCPU_EVENTS */
>  struct kvm_vcpu_events {
>  	struct {
> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
>  #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
>  #define   KVM_ARM_VCPU_PMU_V3_INIT	1
> +#define   KVM_ARM_VCPU_PMU_V3_FILTER	2
>  #define KVM_ARM_VCPU_TIMER_CTRL		1
>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index eda7b624eab8..8d849ac88a44 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>  	free_percpu(kvm->arch.last_vcpu_ran);
>  	kvm->arch.last_vcpu_ran = NULL;
>  
> +	bitmap_free(kvm->arch.pmu_filter);
> +
>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>  		if (kvm->vcpus[i]) {
>  			kvm_vcpu_destroy(kvm->vcpus[i]);
> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
> index f0d0312c0a55..9f0fd0224d5b 100644
> --- a/virt/kvm/arm/pmu.c
> +++ b/virt/kvm/arm/pmu.c
> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  
>  	kvm_pmu_stop_counter(vcpu, pmc);
>  	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
> +	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> +		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
nit:
	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
	else
		eventsel = data & ARMV8_PMU_EVTYPE_EVENT;

>  
>  	/* Software increment event does't need to be backed by a perf event */
nit: while wer are at it fix the does't typo
> -	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
> -	    pmc->idx != ARMV8_PMU_CYCLE_IDX)
> +	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
> +		return;
> +
> +	/*
> +	 * If we have a filter in place and that the event isn't allowed, do
> +	 * not install a perf event either.
> +	 */
> +	if (vcpu->kvm->arch.pmu_filter &&
> +	    !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>  		return;
>  
>  	memset(&attr, 0, sizeof(struct perf_event_attr));
> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  	attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>  	attr.exclude_hv = 1; /* Don't count EL2 events */
>  	attr.exclude_host = 1; /* Don't count host events */
> -	attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
> -		ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
> +	attr.config = eventsel;
So in that case the guest counter will not increment but the guest does
not know the counter is not implemented. Can't this lead to bad user
experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?
>  
>  	counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>  
> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>  
>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>  {
> -	if (!kvm_arm_support_pmu_v3())
> -		return -ENODEV;
> -
> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> -		return -ENXIO;
> -
> -	if (vcpu->arch.pmu.created)
> -		return -EBUSY;
> -
>  	if (irqchip_in_kernel(vcpu->kvm)) {
>  		int ret;
>  
> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
>  	return true;
>  }
>  
> +#define NR_EVENTS	(ARMV8_PMU_EVTYPE_EVENT + 1)
> +
>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  {
> +	if (!kvm_arm_support_pmu_v3())
> +		return -ENODEV;
> +
> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> +		return -ENODEV;
I see you changed -ENXIO into -ENODEV. wanted?
> +
> +	if (vcpu->arch.pmu.created)
> +		return -EBUSY;
> +
>  	switch (attr->attr) {
>  	case KVM_ARM_VCPU_PMU_V3_IRQ: {
>  		int __user *uaddr = (int __user *)(long)attr->addr;
> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  		if (!irqchip_in_kernel(vcpu->kvm))
>  			return -EINVAL;
>  
> -		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
> -			return -ENODEV;
> -
>  		if (get_user(irq, uaddr))
>  			return -EFAULT;
>  
> @@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  		vcpu->arch.pmu.irq_num = irq;
>  		return 0;
>  	}
> +	case KVM_ARM_VCPU_PMU_V3_FILTER: {
> +		struct kvm_pmu_event_filter __user *uaddr;
> +		struct kvm_pmu_event_filter filter;
> +
> +		uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
> +
> +		if (copy_from_user(&filter, uaddr, sizeof(filter)))
> +			return -EFAULT;
> +
> +		if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
isnt't it >= ?
> +		    (filter.action != KVM_PMU_EVENT_ALLOW &&
> +		     filter.action != KVM_PMU_EVENT_DENY))
> +			return -EINVAL;
-EINVAL is not documented in the API doc.
> +
> +		mutex_lock(&vcpu->kvm->lock);
> +
> +		if (!vcpu->kvm->arch.pmu_filter) {
> +			vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS, GFP_KERNEL);
> +			if (!vcpu->kvm->arch.pmu_filter) {
> +				mutex_unlock(&vcpu->kvm->lock);
> +				return -ENOMEM;
> +			}
> +
> +			/*
> +			 * The default depends on the first applied filter.
> +			 * If it allows events, the default is to deny.
> +			 * Conversely, if the first filter denies a set of
> +			 * events, the default is to allow.
> +			 */
> +			if (filter.action == KVM_PMU_EVENT_ALLOW)
> +				bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
> +			else
> +				bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
> +		}
> +
> +		if (filter.action == KVM_PMU_EVENT_ALLOW)
> +			bitmap_set(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
> +		else
> +			bitmap_clear(vcpu->kvm->arch.pmu_filter, filter.base_event, filter.nevents);
> +
> +		mutex_unlock(&vcpu->kvm->lock);
> +
> +		return 0;
> +	}
>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>  		return kvm_arm_pmu_v3_init(vcpu);
>  	}
> @@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  	switch (attr->attr) {
>  	case KVM_ARM_VCPU_PMU_V3_IRQ:
not related to this patch but shouldn't we advertise this only with
in-kernel irqchip?
>  	case KVM_ARM_VCPU_PMU_V3_INIT:
> +	case KVM_ARM_VCPU_PMU_V3_FILTER:
>  		if (kvm_arm_support_pmu_v3() &&
>  		    test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>  			return 0;
> 
Thanks

Eric

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

Hi Eric,

On 2020-03-09 18:05, Auger Eric wrote:
> Hi Marc,
> 
> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>> It can be desirable to expose a PMU to a guest, and yet not want the
>> guest to be able to count some of the implemented events (because this
>> would give information on shared resources, for example.
>> 
>> For this, let's extend the PMUv3 device API, and offer a way to setup 
>> a
>> bitmap of the allowed events (the default being no bitmap, and thus no
>> filtering).
>> 
>> Userspace can thus allow/deny ranges of event. The default policy
>> depends on the "polarity" of the first filter setup (default deny if 
>> the
>> filter allows events, and default allow if the filter denies events).
>> This allows to setup exactly what is allowed for a given guest.
>> 
>> Note that although the ioctl is per-vcpu, the map of allowed events is
>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>> initialized).
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>  virt/kvm/arm/arm.c                |  2 +
>>  virt/kvm/arm/pmu.c                | 84 
>> +++++++++++++++++++++++++------
>>  4 files changed, 92 insertions(+), 16 deletions(-)
>> 
>> diff --git a/arch/arm64/include/asm/kvm_host.h 
>> b/arch/arm64/include/asm/kvm_host.h
>> index 57fd46acd058..8e63c618688d 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>  	 * supported.
>>  	 */
>>  	bool return_nisv_io_abort_to_user;
>> +
>> +	/*
>> +	 * VM-wide PMU filter, implemented as a bitmap and big enough
>> +	 * for up to 65536 events
>> +	 */
>> +	unsigned long *pmu_filter;
>>  };
>> 
>>  #define KVM_NR_MEM_OBJS     40
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
>> b/arch/arm64/include/uapi/asm/kvm.h
>> index ba85bb23f060..7b1511d6ce44 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>  struct kvm_arch_memory_slot {
>>  };
>> 
>> +/*
>> + * PMU filter structure. Describe a range of events with a particular
>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>> + */
>> +struct kvm_pmu_event_filter {
>> +	__u16	base_event;
>> +	__u16	nevents;
>> +
>> +#define KVM_PMU_EVENT_ALLOW	0
>> +#define KVM_PMU_EVENT_DENY	1
>> +
>> +	__u8	action;
>> +	__u8	pad[3];
>> +};
>> +
>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>  struct kvm_vcpu_events {
>>  	struct {
>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
>>  #define   KVM_ARM_VCPU_PMU_V3_INIT	1
>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER	2
>>  #define KVM_ARM_VCPU_TIMER_CTRL		1
>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index eda7b624eab8..8d849ac88a44 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>  	free_percpu(kvm->arch.last_vcpu_ran);
>>  	kvm->arch.last_vcpu_ran = NULL;
>> 
>> +	bitmap_free(kvm->arch.pmu_filter);
>> +
>>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>  		if (kvm->vcpus[i]) {
>>  			kvm_vcpu_destroy(kvm->vcpus[i]);
>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>> index f0d0312c0a55..9f0fd0224d5b 100644
>> --- a/virt/kvm/arm/pmu.c
>> +++ b/virt/kvm/arm/pmu.c
>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct 
>> kvm_vcpu *vcpu, u64 select_idx)
>> 
>>  	kvm_pmu_stop_counter(vcpu, pmc);
>>  	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>> +	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>> +		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
> nit:
> 	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> 		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
> 	else
> 		eventsel = data & ARMV8_PMU_EVTYPE_EVENT;

You don't like it? ;-)

>> 
>>  	/* Software increment event does't need to be backed by a perf event 
>> */
> nit: while wer are at it fix the does't typo
>> -	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>> -	    pmc->idx != ARMV8_PMU_CYCLE_IDX)
>> +	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>> +		return;
>> +
>> +	/*
>> +	 * If we have a filter in place and that the event isn't allowed, do
>> +	 * not install a perf event either.
>> +	 */
>> +	if (vcpu->kvm->arch.pmu_filter &&
>> +	    !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>  		return;
>> 
>>  	memset(&attr, 0, sizeof(struct perf_event_attr));
>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct 
>> kvm_vcpu *vcpu, u64 select_idx)
>>  	attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>  	attr.exclude_hv = 1; /* Don't count EL2 events */
>>  	attr.exclude_host = 1; /* Don't count host events */
>> -	attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>> -		ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>> +	attr.config = eventsel;
> So in that case the guest counter will not increment but the guest does
> not know the counter is not implemented. Can't this lead to bad user
> experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?

The whole point is that we want to keep things hidden from the guest.
Also, PMCEID{0,1} only describe a small set of events (the architected
common events), and not the whole range of microarchitectural events
that the CPU implements.

>> 
>>  	counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>> 
>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>> 
>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>  {
>> -	if (!kvm_arm_support_pmu_v3())
>> -		return -ENODEV;
>> -
>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>> -		return -ENXIO;
>> -
>> -	if (vcpu->arch.pmu.created)
>> -		return -EBUSY;
>> -
>>  	if (irqchip_in_kernel(vcpu->kvm)) {
>>  		int ret;
>> 
>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int 
>> irq)
>>  	return true;
>>  }
>> 
>> +#define NR_EVENTS	(ARMV8_PMU_EVTYPE_EVENT + 1)
>> +
>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct 
>> kvm_device_attr *attr)
>>  {
>> +	if (!kvm_arm_support_pmu_v3())
>> +		return -ENODEV;
>> +
>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>> +		return -ENODEV;
> I see you changed -ENXIO into -ENODEV. wanted?

Probably not... but see below.

>> +
>> +	if (vcpu->arch.pmu.created)
>> +		return -EBUSY;
>> +
>>  	switch (attr->attr) {
>>  	case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>  		int __user *uaddr = (int __user *)(long)attr->addr;
>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, 
>> struct kvm_device_attr *attr)
>>  		if (!irqchip_in_kernel(vcpu->kvm))
>>  			return -EINVAL;
>> 
>> -		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>> -			return -ENODEV;
>> -

Here's why. I wonder if we already have a problem with the consistency 
of the
error codes returned to userspace.

>>  		if (get_user(irq, uaddr))
>>  			return -EFAULT;
>> 
>> @@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu 
>> *vcpu, struct kvm_device_attr *attr)
>>  		vcpu->arch.pmu.irq_num = irq;
>>  		return 0;
>>  	}
>> +	case KVM_ARM_VCPU_PMU_V3_FILTER: {
>> +		struct kvm_pmu_event_filter __user *uaddr;
>> +		struct kvm_pmu_event_filter filter;
>> +
>> +		uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
>> +
>> +		if (copy_from_user(&filter, uaddr, sizeof(filter)))
>> +			return -EFAULT;
>> +
>> +		if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
> isnt't it >= ?

No, I think this is correct. I want to be able to filter event 0xFFFF, 
for example,
so I have base_event=0xFFFF and nevents=1.

>> +		    (filter.action != KVM_PMU_EVENT_ALLOW &&
>> +		     filter.action != KVM_PMU_EVENT_DENY))
>> +			return -EINVAL;
> -EINVAL is not documented in the API doc.

Good point.

>> +
>> +		mutex_lock(&vcpu->kvm->lock);
>> +
>> +		if (!vcpu->kvm->arch.pmu_filter) {
>> +			vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS, GFP_KERNEL);
>> +			if (!vcpu->kvm->arch.pmu_filter) {
>> +				mutex_unlock(&vcpu->kvm->lock);
>> +				return -ENOMEM;
>> +			}
>> +
>> +			/*
>> +			 * The default depends on the first applied filter.
>> +			 * If it allows events, the default is to deny.
>> +			 * Conversely, if the first filter denies a set of
>> +			 * events, the default is to allow.
>> +			 */
>> +			if (filter.action == KVM_PMU_EVENT_ALLOW)
>> +				bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>> +			else
>> +				bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>> +		}
>> +
>> +		if (filter.action == KVM_PMU_EVENT_ALLOW)
>> +			bitmap_set(vcpu->kvm->arch.pmu_filter, filter.base_event, 
>> filter.nevents);
>> +		else
>> +			bitmap_clear(vcpu->kvm->arch.pmu_filter, filter.base_event, 
>> filter.nevents);
>> +
>> +		mutex_unlock(&vcpu->kvm->lock);
>> +
>> +		return 0;
>> +	}
>>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>>  		return kvm_arm_pmu_v3_init(vcpu);
>>  	}
>> @@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, 
>> struct kvm_device_attr *attr)
>>  	switch (attr->attr) {
>>  	case KVM_ARM_VCPU_PMU_V3_IRQ:
> not related to this patch but shouldn't we advertise this only with
> in-kernel irqchip?

We do support the PMU without the in-kernel chip, unfortunately... Yes,
supporting this feature was a big mistake.

>>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>> +	case KVM_ARM_VCPU_PMU_V3_FILTER:
>>  		if (kvm_arm_support_pmu_v3() &&
>>  		    test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>  			return 0;

Thanks for the review,

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

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

Hi Eric,

On 2020-03-09 18:05, Auger Eric wrote:
> Hi Marc,
> 
> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>> It can be desirable to expose a PMU to a guest, and yet not want the
>> guest to be able to count some of the implemented events (because this
>> would give information on shared resources, for example.
>> 
>> For this, let's extend the PMUv3 device API, and offer a way to setup 
>> a
>> bitmap of the allowed events (the default being no bitmap, and thus no
>> filtering).
>> 
>> Userspace can thus allow/deny ranges of event. The default policy
>> depends on the "polarity" of the first filter setup (default deny if 
>> the
>> filter allows events, and default allow if the filter denies events).
>> This allows to setup exactly what is allowed for a given guest.
>> 
>> Note that although the ioctl is per-vcpu, the map of allowed events is
>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>> initialized).
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>  virt/kvm/arm/arm.c                |  2 +
>>  virt/kvm/arm/pmu.c                | 84 
>> +++++++++++++++++++++++++------
>>  4 files changed, 92 insertions(+), 16 deletions(-)
>> 
>> diff --git a/arch/arm64/include/asm/kvm_host.h 
>> b/arch/arm64/include/asm/kvm_host.h
>> index 57fd46acd058..8e63c618688d 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>  	 * supported.
>>  	 */
>>  	bool return_nisv_io_abort_to_user;
>> +
>> +	/*
>> +	 * VM-wide PMU filter, implemented as a bitmap and big enough
>> +	 * for up to 65536 events
>> +	 */
>> +	unsigned long *pmu_filter;
>>  };
>> 
>>  #define KVM_NR_MEM_OBJS     40
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
>> b/arch/arm64/include/uapi/asm/kvm.h
>> index ba85bb23f060..7b1511d6ce44 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>  struct kvm_arch_memory_slot {
>>  };
>> 
>> +/*
>> + * PMU filter structure. Describe a range of events with a particular
>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>> + */
>> +struct kvm_pmu_event_filter {
>> +	__u16	base_event;
>> +	__u16	nevents;
>> +
>> +#define KVM_PMU_EVENT_ALLOW	0
>> +#define KVM_PMU_EVENT_DENY	1
>> +
>> +	__u8	action;
>> +	__u8	pad[3];
>> +};
>> +
>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>  struct kvm_vcpu_events {
>>  	struct {
>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
>>  #define   KVM_ARM_VCPU_PMU_V3_INIT	1
>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER	2
>>  #define KVM_ARM_VCPU_TIMER_CTRL		1
>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index eda7b624eab8..8d849ac88a44 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>  	free_percpu(kvm->arch.last_vcpu_ran);
>>  	kvm->arch.last_vcpu_ran = NULL;
>> 
>> +	bitmap_free(kvm->arch.pmu_filter);
>> +
>>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>  		if (kvm->vcpus[i]) {
>>  			kvm_vcpu_destroy(kvm->vcpus[i]);
>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>> index f0d0312c0a55..9f0fd0224d5b 100644
>> --- a/virt/kvm/arm/pmu.c
>> +++ b/virt/kvm/arm/pmu.c
>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct 
>> kvm_vcpu *vcpu, u64 select_idx)
>> 
>>  	kvm_pmu_stop_counter(vcpu, pmc);
>>  	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>> +	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>> +		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
> nit:
> 	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> 		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
> 	else
> 		eventsel = data & ARMV8_PMU_EVTYPE_EVENT;

You don't like it? ;-)

>> 
>>  	/* Software increment event does't need to be backed by a perf event 
>> */
> nit: while wer are at it fix the does't typo
>> -	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>> -	    pmc->idx != ARMV8_PMU_CYCLE_IDX)
>> +	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>> +		return;
>> +
>> +	/*
>> +	 * If we have a filter in place and that the event isn't allowed, do
>> +	 * not install a perf event either.
>> +	 */
>> +	if (vcpu->kvm->arch.pmu_filter &&
>> +	    !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>  		return;
>> 
>>  	memset(&attr, 0, sizeof(struct perf_event_attr));
>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct 
>> kvm_vcpu *vcpu, u64 select_idx)
>>  	attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>  	attr.exclude_hv = 1; /* Don't count EL2 events */
>>  	attr.exclude_host = 1; /* Don't count host events */
>> -	attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>> -		ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>> +	attr.config = eventsel;
> So in that case the guest counter will not increment but the guest does
> not know the counter is not implemented. Can't this lead to bad user
> experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?

The whole point is that we want to keep things hidden from the guest.
Also, PMCEID{0,1} only describe a small set of events (the architected
common events), and not the whole range of microarchitectural events
that the CPU implements.

>> 
>>  	counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>> 
>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>> 
>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>  {
>> -	if (!kvm_arm_support_pmu_v3())
>> -		return -ENODEV;
>> -
>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>> -		return -ENXIO;
>> -
>> -	if (vcpu->arch.pmu.created)
>> -		return -EBUSY;
>> -
>>  	if (irqchip_in_kernel(vcpu->kvm)) {
>>  		int ret;
>> 
>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int 
>> irq)
>>  	return true;
>>  }
>> 
>> +#define NR_EVENTS	(ARMV8_PMU_EVTYPE_EVENT + 1)
>> +
>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct 
>> kvm_device_attr *attr)
>>  {
>> +	if (!kvm_arm_support_pmu_v3())
>> +		return -ENODEV;
>> +
>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>> +		return -ENODEV;
> I see you changed -ENXIO into -ENODEV. wanted?

Probably not... but see below.

>> +
>> +	if (vcpu->arch.pmu.created)
>> +		return -EBUSY;
>> +
>>  	switch (attr->attr) {
>>  	case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>  		int __user *uaddr = (int __user *)(long)attr->addr;
>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, 
>> struct kvm_device_attr *attr)
>>  		if (!irqchip_in_kernel(vcpu->kvm))
>>  			return -EINVAL;
>> 
>> -		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>> -			return -ENODEV;
>> -

Here's why. I wonder if we already have a problem with the consistency 
of the
error codes returned to userspace.

>>  		if (get_user(irq, uaddr))
>>  			return -EFAULT;
>> 
>> @@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu 
>> *vcpu, struct kvm_device_attr *attr)
>>  		vcpu->arch.pmu.irq_num = irq;
>>  		return 0;
>>  	}
>> +	case KVM_ARM_VCPU_PMU_V3_FILTER: {
>> +		struct kvm_pmu_event_filter __user *uaddr;
>> +		struct kvm_pmu_event_filter filter;
>> +
>> +		uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
>> +
>> +		if (copy_from_user(&filter, uaddr, sizeof(filter)))
>> +			return -EFAULT;
>> +
>> +		if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
> isnt't it >= ?

No, I think this is correct. I want to be able to filter event 0xFFFF, 
for example,
so I have base_event=0xFFFF and nevents=1.

>> +		    (filter.action != KVM_PMU_EVENT_ALLOW &&
>> +		     filter.action != KVM_PMU_EVENT_DENY))
>> +			return -EINVAL;
> -EINVAL is not documented in the API doc.

Good point.

>> +
>> +		mutex_lock(&vcpu->kvm->lock);
>> +
>> +		if (!vcpu->kvm->arch.pmu_filter) {
>> +			vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS, GFP_KERNEL);
>> +			if (!vcpu->kvm->arch.pmu_filter) {
>> +				mutex_unlock(&vcpu->kvm->lock);
>> +				return -ENOMEM;
>> +			}
>> +
>> +			/*
>> +			 * The default depends on the first applied filter.
>> +			 * If it allows events, the default is to deny.
>> +			 * Conversely, if the first filter denies a set of
>> +			 * events, the default is to allow.
>> +			 */
>> +			if (filter.action == KVM_PMU_EVENT_ALLOW)
>> +				bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>> +			else
>> +				bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>> +		}
>> +
>> +		if (filter.action == KVM_PMU_EVENT_ALLOW)
>> +			bitmap_set(vcpu->kvm->arch.pmu_filter, filter.base_event, 
>> filter.nevents);
>> +		else
>> +			bitmap_clear(vcpu->kvm->arch.pmu_filter, filter.base_event, 
>> filter.nevents);
>> +
>> +		mutex_unlock(&vcpu->kvm->lock);
>> +
>> +		return 0;
>> +	}
>>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>>  		return kvm_arm_pmu_v3_init(vcpu);
>>  	}
>> @@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, 
>> struct kvm_device_attr *attr)
>>  	switch (attr->attr) {
>>  	case KVM_ARM_VCPU_PMU_V3_IRQ:
> not related to this patch but shouldn't we advertise this only with
> in-kernel irqchip?

We do support the PMU without the in-kernel chip, unfortunately... Yes,
supporting this feature was a big mistake.

>>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>> +	case KVM_ARM_VCPU_PMU_V3_FILTER:
>>  		if (kvm_arm_support_pmu_v3() &&
>>  		    test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>  			return 0;

Thanks for the review,

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

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

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

Hi Eric,

On 2020-03-09 18:05, Auger Eric wrote:
> Hi Marc,
> 
> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>> It can be desirable to expose a PMU to a guest, and yet not want the
>> guest to be able to count some of the implemented events (because this
>> would give information on shared resources, for example.
>> 
>> For this, let's extend the PMUv3 device API, and offer a way to setup 
>> a
>> bitmap of the allowed events (the default being no bitmap, and thus no
>> filtering).
>> 
>> Userspace can thus allow/deny ranges of event. The default policy
>> depends on the "polarity" of the first filter setup (default deny if 
>> the
>> filter allows events, and default allow if the filter denies events).
>> This allows to setup exactly what is allowed for a given guest.
>> 
>> Note that although the ioctl is per-vcpu, the map of allowed events is
>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>> initialized).
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>  virt/kvm/arm/arm.c                |  2 +
>>  virt/kvm/arm/pmu.c                | 84 
>> +++++++++++++++++++++++++------
>>  4 files changed, 92 insertions(+), 16 deletions(-)
>> 
>> diff --git a/arch/arm64/include/asm/kvm_host.h 
>> b/arch/arm64/include/asm/kvm_host.h
>> index 57fd46acd058..8e63c618688d 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>  	 * supported.
>>  	 */
>>  	bool return_nisv_io_abort_to_user;
>> +
>> +	/*
>> +	 * VM-wide PMU filter, implemented as a bitmap and big enough
>> +	 * for up to 65536 events
>> +	 */
>> +	unsigned long *pmu_filter;
>>  };
>> 
>>  #define KVM_NR_MEM_OBJS     40
>> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
>> b/arch/arm64/include/uapi/asm/kvm.h
>> index ba85bb23f060..7b1511d6ce44 100644
>> --- a/arch/arm64/include/uapi/asm/kvm.h
>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>  struct kvm_arch_memory_slot {
>>  };
>> 
>> +/*
>> + * PMU filter structure. Describe a range of events with a particular
>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>> + */
>> +struct kvm_pmu_event_filter {
>> +	__u16	base_event;
>> +	__u16	nevents;
>> +
>> +#define KVM_PMU_EVENT_ALLOW	0
>> +#define KVM_PMU_EVENT_DENY	1
>> +
>> +	__u8	action;
>> +	__u8	pad[3];
>> +};
>> +
>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>  struct kvm_vcpu_events {
>>  	struct {
>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>  #define KVM_ARM_VCPU_PMU_V3_CTRL	0
>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ	0
>>  #define   KVM_ARM_VCPU_PMU_V3_INIT	1
>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER	2
>>  #define KVM_ARM_VCPU_TIMER_CTRL		1
>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER		0
>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER		1
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index eda7b624eab8..8d849ac88a44 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>  	free_percpu(kvm->arch.last_vcpu_ran);
>>  	kvm->arch.last_vcpu_ran = NULL;
>> 
>> +	bitmap_free(kvm->arch.pmu_filter);
>> +
>>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>  		if (kvm->vcpus[i]) {
>>  			kvm_vcpu_destroy(kvm->vcpus[i]);
>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>> index f0d0312c0a55..9f0fd0224d5b 100644
>> --- a/virt/kvm/arm/pmu.c
>> +++ b/virt/kvm/arm/pmu.c
>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct 
>> kvm_vcpu *vcpu, u64 select_idx)
>> 
>>  	kvm_pmu_stop_counter(vcpu, pmc);
>>  	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>> +	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>> +		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
> nit:
> 	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> 		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
> 	else
> 		eventsel = data & ARMV8_PMU_EVTYPE_EVENT;

You don't like it? ;-)

>> 
>>  	/* Software increment event does't need to be backed by a perf event 
>> */
> nit: while wer are at it fix the does't typo
>> -	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>> -	    pmc->idx != ARMV8_PMU_CYCLE_IDX)
>> +	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>> +		return;
>> +
>> +	/*
>> +	 * If we have a filter in place and that the event isn't allowed, do
>> +	 * not install a perf event either.
>> +	 */
>> +	if (vcpu->kvm->arch.pmu_filter &&
>> +	    !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>  		return;
>> 
>>  	memset(&attr, 0, sizeof(struct perf_event_attr));
>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct 
>> kvm_vcpu *vcpu, u64 select_idx)
>>  	attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>  	attr.exclude_hv = 1; /* Don't count EL2 events */
>>  	attr.exclude_host = 1; /* Don't count host events */
>> -	attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>> -		ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>> +	attr.config = eventsel;
> So in that case the guest counter will not increment but the guest does
> not know the counter is not implemented. Can't this lead to bad user
> experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?

The whole point is that we want to keep things hidden from the guest.
Also, PMCEID{0,1} only describe a small set of events (the architected
common events), and not the whole range of microarchitectural events
that the CPU implements.

>> 
>>  	counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>> 
>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>> 
>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>  {
>> -	if (!kvm_arm_support_pmu_v3())
>> -		return -ENODEV;
>> -
>> -	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>> -		return -ENXIO;
>> -
>> -	if (vcpu->arch.pmu.created)
>> -		return -EBUSY;
>> -
>>  	if (irqchip_in_kernel(vcpu->kvm)) {
>>  		int ret;
>> 
>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int 
>> irq)
>>  	return true;
>>  }
>> 
>> +#define NR_EVENTS	(ARMV8_PMU_EVTYPE_EVENT + 1)
>> +
>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct 
>> kvm_device_attr *attr)
>>  {
>> +	if (!kvm_arm_support_pmu_v3())
>> +		return -ENODEV;
>> +
>> +	if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>> +		return -ENODEV;
> I see you changed -ENXIO into -ENODEV. wanted?

Probably not... but see below.

>> +
>> +	if (vcpu->arch.pmu.created)
>> +		return -EBUSY;
>> +
>>  	switch (attr->attr) {
>>  	case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>  		int __user *uaddr = (int __user *)(long)attr->addr;
>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, 
>> struct kvm_device_attr *attr)
>>  		if (!irqchip_in_kernel(vcpu->kvm))
>>  			return -EINVAL;
>> 
>> -		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>> -			return -ENODEV;
>> -

Here's why. I wonder if we already have a problem with the consistency 
of the
error codes returned to userspace.

>>  		if (get_user(irq, uaddr))
>>  			return -EFAULT;
>> 
>> @@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu 
>> *vcpu, struct kvm_device_attr *attr)
>>  		vcpu->arch.pmu.irq_num = irq;
>>  		return 0;
>>  	}
>> +	case KVM_ARM_VCPU_PMU_V3_FILTER: {
>> +		struct kvm_pmu_event_filter __user *uaddr;
>> +		struct kvm_pmu_event_filter filter;
>> +
>> +		uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
>> +
>> +		if (copy_from_user(&filter, uaddr, sizeof(filter)))
>> +			return -EFAULT;
>> +
>> +		if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
> isnt't it >= ?

No, I think this is correct. I want to be able to filter event 0xFFFF, 
for example,
so I have base_event=0xFFFF and nevents=1.

>> +		    (filter.action != KVM_PMU_EVENT_ALLOW &&
>> +		     filter.action != KVM_PMU_EVENT_DENY))
>> +			return -EINVAL;
> -EINVAL is not documented in the API doc.

Good point.

>> +
>> +		mutex_lock(&vcpu->kvm->lock);
>> +
>> +		if (!vcpu->kvm->arch.pmu_filter) {
>> +			vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS, GFP_KERNEL);
>> +			if (!vcpu->kvm->arch.pmu_filter) {
>> +				mutex_unlock(&vcpu->kvm->lock);
>> +				return -ENOMEM;
>> +			}
>> +
>> +			/*
>> +			 * The default depends on the first applied filter.
>> +			 * If it allows events, the default is to deny.
>> +			 * Conversely, if the first filter denies a set of
>> +			 * events, the default is to allow.
>> +			 */
>> +			if (filter.action == KVM_PMU_EVENT_ALLOW)
>> +				bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>> +			else
>> +				bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>> +		}
>> +
>> +		if (filter.action == KVM_PMU_EVENT_ALLOW)
>> +			bitmap_set(vcpu->kvm->arch.pmu_filter, filter.base_event, 
>> filter.nevents);
>> +		else
>> +			bitmap_clear(vcpu->kvm->arch.pmu_filter, filter.base_event, 
>> filter.nevents);
>> +
>> +		mutex_unlock(&vcpu->kvm->lock);
>> +
>> +		return 0;
>> +	}
>>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>>  		return kvm_arm_pmu_v3_init(vcpu);
>>  	}
>> @@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, 
>> struct kvm_device_attr *attr)
>>  	switch (attr->attr) {
>>  	case KVM_ARM_VCPU_PMU_V3_IRQ:
> not related to this patch but shouldn't we advertise this only with
> in-kernel irqchip?

We do support the PMU without the in-kernel chip, unfortunately... Yes,
supporting this feature was a big mistake.

>>  	case KVM_ARM_VCPU_PMU_V3_INIT:
>> +	case KVM_ARM_VCPU_PMU_V3_FILTER:
>>  		if (kvm_arm_support_pmu_v3() &&
>>  		    test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>  			return 0;

Thanks for the review,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Auger Eric]

Hi Marc,

On 3/10/20 12:03 PM, Marc Zyngier wrote:
> Hi Eric,
> 
> On 2020-03-09 18:05, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>> guest to be able to count some of the implemented events (because this
>>> would give information on shared resources, for example.
>>>
>>> For this, let's extend the PMUv3 device API, and offer a way to setup a
>>> bitmap of the allowed events (the default being no bitmap, and thus no
>>> filtering).
>>>
>>> Userspace can thus allow/deny ranges of event. The default policy
>>> depends on the "polarity" of the first filter setup (default deny if the
>>> filter allows events, and default allow if the filter denies events).
>>> This allows to setup exactly what is allowed for a given guest.
>>>
>>> Note that although the ioctl is per-vcpu, the map of allowed events is
>>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>>> initialized).
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>  virt/kvm/arm/arm.c                |  2 +
>>>  virt/kvm/arm/pmu.c                | 84 +++++++++++++++++++++++++------
>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>> b/arch/arm64/include/asm/kvm_host.h
>>> index 57fd46acd058..8e63c618688d 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>       * supported.
>>>       */
>>>      bool return_nisv_io_abort_to_user;
>>> +
>>> +    /*
>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>> +     * for up to 65536 events
>>> +     */
>>> +    unsigned long *pmu_filter;
>>>  };
>>>
>>>  #define KVM_NR_MEM_OBJS     40
>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>> b/arch/arm64/include/uapi/asm/kvm.h
>>> index ba85bb23f060..7b1511d6ce44 100644
>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>  struct kvm_arch_memory_slot {
>>>  };
>>>
>>> +/*
>>> + * PMU filter structure. Describe a range of events with a particular
>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>> + */
>>> +struct kvm_pmu_event_filter {
>>> +    __u16    base_event;
>>> +    __u16    nevents;
>>> +
>>> +#define KVM_PMU_EVENT_ALLOW    0
>>> +#define KVM_PMU_EVENT_DENY    1
>>> +
>>> +    __u8    action;
>>> +    __u8    pad[3];
>>> +};
>>> +
>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>  struct kvm_vcpu_events {
>>>      struct {
>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>> index eda7b624eab8..8d849ac88a44 100644
>>> --- a/virt/kvm/arm/arm.c
>>> +++ b/virt/kvm/arm/arm.c
>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>      kvm->arch.last_vcpu_ran = NULL;
>>>
>>> +    bitmap_free(kvm->arch.pmu_filter);
>>> +
>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>          if (kvm->vcpus[i]) {
>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>> --- a/virt/kvm/arm/pmu.c
>>> +++ b/virt/kvm/arm/pmu.c
>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>> kvm_vcpu *vcpu, u64 select_idx)
>>>
>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>> nit:
>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>     else
>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
> 
> You don't like it? ;-)
? eventset set only once instead of 2 times
> 
>>>
>>>      /* Software increment event does't need to be backed by a perf
>>> event */
>> nit: while wer are at it fix the does't typo
>>> -    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>>> -        pmc->idx != ARMV8_PMU_CYCLE_IDX)
>>> +    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>>> +        return;
>>> +
>>> +    /*
>>> +     * If we have a filter in place and that the event isn't
>>> allowed, do
>>> +     * not install a perf event either.
>>> +     */
>>> +    if (vcpu->kvm->arch.pmu_filter &&
>>> +        !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>>          return;
>>>
>>>      memset(&attr, 0, sizeof(struct perf_event_attr));
>>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct
>>> kvm_vcpu *vcpu, u64 select_idx)
>>>      attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>>      attr.exclude_hv = 1; /* Don't count EL2 events */
>>>      attr.exclude_host = 1; /* Don't count host events */
>>> -    attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>>> -        ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>>> +    attr.config = eventsel;
>> So in that case the guest counter will not increment but the guest does
>> not know the counter is not implemented. Can't this lead to bad user
>> experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?
> 
> The whole point is that we want to keep things hidden from the guest.
> Also, PMCEID{0,1} only describe a small set of events (the architected
> common events), and not the whole range of microarchitectural events
> that the CPU implements.

I am still not totally convinced. Things are not totally hidden to the
guest as the counter does not increment, right? So a guest may try to
use as it is advertised in PMCEID0/1 but not get the expected results
leading to potential support request. I agree not all the events are
described there but your API also allows to filter out some of the ones
that are advertised.
> 
>>>
>>>      counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>>>
>>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>>>
>>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>  {
>>> -    if (!kvm_arm_support_pmu_v3())
>>> -        return -ENODEV;
>>> -
>>> -    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>> -        return -ENXIO;
>>> -
>>> -    if (vcpu->arch.pmu.created)
>>> -        return -EBUSY;
>>> -
>>>      if (irqchip_in_kernel(vcpu->kvm)) {
>>>          int ret;
>>>
>>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm,
>>> int irq)
>>>      return true;
>>>  }
>>>
>>> +#define NR_EVENTS    (ARMV8_PMU_EVTYPE_EVENT + 1)
>>> +
>>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct
>>> kvm_device_attr *attr)
>>>  {
>>> +    if (!kvm_arm_support_pmu_v3())
>>> +        return -ENODEV;
>>> +
>>> +    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>> +        return -ENODEV;
>> I see you changed -ENXIO into -ENODEV. wanted?
> 
> Probably not... but see below.
> 
>>> +
>>> +    if (vcpu->arch.pmu.created)
>>> +        return -EBUSY;
>>> +
>>>      switch (attr->attr) {
>>>      case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>>          int __user *uaddr = (int __user *)(long)attr->addr;
>>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>> *vcpu, struct kvm_device_attr *attr)
>>>          if (!irqchip_in_kernel(vcpu->kvm))
>>>              return -EINVAL;
>>>
>>> -        if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>> -            return -ENODEV;
>>> -
> 
> Here's why. I wonder if we already have a problem with the consistency
> of the
> error codes returned to userspace.
OK. Then you may document it in the commit message?
> 
>>>          if (get_user(irq, uaddr))
>>>              return -EFAULT;
>>>
>>> @@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>> *vcpu, struct kvm_device_attr *attr)
>>>          vcpu->arch.pmu.irq_num = irq;
>>>          return 0;
>>>      }
>>> +    case KVM_ARM_VCPU_PMU_V3_FILTER: {
>>> +        struct kvm_pmu_event_filter __user *uaddr;
>>> +        struct kvm_pmu_event_filter filter;
>>> +
>>> +        uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
>>> +
>>> +        if (copy_from_user(&filter, uaddr, sizeof(filter)))
>>> +            return -EFAULT;
>>> +
>>> +        if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
>> isnt't it >= ?
> 
> No, I think this is correct. I want to be able to filter event 0xFFFF,
> for example,
> so I have base_event=0xFFFF and nevents=1.
hum my mistake. Sorry
> 
>>> +            (filter.action != KVM_PMU_EVENT_ALLOW &&
>>> +             filter.action != KVM_PMU_EVENT_DENY))
>>> +            return -EINVAL;
>> -EINVAL is not documented in the API doc.
> 
> Good point.
> 
>>> +
>>> +        mutex_lock(&vcpu->kvm->lock);
>>> +
>>> +        if (!vcpu->kvm->arch.pmu_filter) {
>>> +            vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS,
>>> GFP_KERNEL);
>>> +            if (!vcpu->kvm->arch.pmu_filter) {
>>> +                mutex_unlock(&vcpu->kvm->lock);
>>> +                return -ENOMEM;
>>> +            }
>>> +
>>> +            /*
>>> +             * The default depends on the first applied filter.
>>> +             * If it allows events, the default is to deny.
>>> +             * Conversely, if the first filter denies a set of
>>> +             * events, the default is to allow.
>>> +             */
>>> +            if (filter.action == KVM_PMU_EVENT_ALLOW)
>>> +                bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>>> +            else
>>> +                bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>>> +        }
>>> +
>>> +        if (filter.action == KVM_PMU_EVENT_ALLOW)
>>> +            bitmap_set(vcpu->kvm->arch.pmu_filter,
>>> filter.base_event, filter.nevents);
>>> +        else
>>> +            bitmap_clear(vcpu->kvm->arch.pmu_filter,
>>> filter.base_event, filter.nevents);
>>> +
>>> +        mutex_unlock(&vcpu->kvm->lock);
>>> +
>>> +        return 0;
>>> +    }
>>>      case KVM_ARM_VCPU_PMU_V3_INIT:
>>>          return kvm_arm_pmu_v3_init(vcpu);
>>>      }
>>> @@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu
>>> *vcpu, struct kvm_device_attr *attr)
>>>      switch (attr->attr) {
>>>      case KVM_ARM_VCPU_PMU_V3_IRQ:
>> not related to this patch but shouldn't we advertise this only with
>> in-kernel irqchip?
> 
> We do support the PMU without the in-kernel chip, unfortunately... Yes,
> supporting this feature was a big mistake.
But I see in kvm_arm_pmu_v3_set_attr:
case KVM_ARM_VCPU_PMU_V3_IRQ:
../..
                if (!irqchip_in_kernel(vcpu->kvm))
                        return -EINVAL;

Thanks

Eric



> 
>>>      case KVM_ARM_VCPU_PMU_V3_INIT:
>>> +    case KVM_ARM_VCPU_PMU_V3_FILTER:
>>>          if (kvm_arm_support_pmu_v3() &&
>>>              test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>              return 0;
> 
> Thanks for the review,
> 
>         M.

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Auger Eric]

Hi Marc,

On 3/10/20 12:03 PM, Marc Zyngier wrote:
> Hi Eric,
> 
> On 2020-03-09 18:05, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>> guest to be able to count some of the implemented events (because this
>>> would give information on shared resources, for example.
>>>
>>> For this, let's extend the PMUv3 device API, and offer a way to setup a
>>> bitmap of the allowed events (the default being no bitmap, and thus no
>>> filtering).
>>>
>>> Userspace can thus allow/deny ranges of event. The default policy
>>> depends on the "polarity" of the first filter setup (default deny if the
>>> filter allows events, and default allow if the filter denies events).
>>> This allows to setup exactly what is allowed for a given guest.
>>>
>>> Note that although the ioctl is per-vcpu, the map of allowed events is
>>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>>> initialized).
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>  virt/kvm/arm/arm.c                |  2 +
>>>  virt/kvm/arm/pmu.c                | 84 +++++++++++++++++++++++++------
>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>> b/arch/arm64/include/asm/kvm_host.h
>>> index 57fd46acd058..8e63c618688d 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>       * supported.
>>>       */
>>>      bool return_nisv_io_abort_to_user;
>>> +
>>> +    /*
>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>> +     * for up to 65536 events
>>> +     */
>>> +    unsigned long *pmu_filter;
>>>  };
>>>
>>>  #define KVM_NR_MEM_OBJS     40
>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>> b/arch/arm64/include/uapi/asm/kvm.h
>>> index ba85bb23f060..7b1511d6ce44 100644
>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>  struct kvm_arch_memory_slot {
>>>  };
>>>
>>> +/*
>>> + * PMU filter structure. Describe a range of events with a particular
>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>> + */
>>> +struct kvm_pmu_event_filter {
>>> +    __u16    base_event;
>>> +    __u16    nevents;
>>> +
>>> +#define KVM_PMU_EVENT_ALLOW    0
>>> +#define KVM_PMU_EVENT_DENY    1
>>> +
>>> +    __u8    action;
>>> +    __u8    pad[3];
>>> +};
>>> +
>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>  struct kvm_vcpu_events {
>>>      struct {
>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>> index eda7b624eab8..8d849ac88a44 100644
>>> --- a/virt/kvm/arm/arm.c
>>> +++ b/virt/kvm/arm/arm.c
>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>      kvm->arch.last_vcpu_ran = NULL;
>>>
>>> +    bitmap_free(kvm->arch.pmu_filter);
>>> +
>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>          if (kvm->vcpus[i]) {
>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>> --- a/virt/kvm/arm/pmu.c
>>> +++ b/virt/kvm/arm/pmu.c
>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>> kvm_vcpu *vcpu, u64 select_idx)
>>>
>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>> nit:
>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>     else
>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
> 
> You don't like it? ;-)
? eventset set only once instead of 2 times
> 
>>>
>>>      /* Software increment event does't need to be backed by a perf
>>> event */
>> nit: while wer are at it fix the does't typo
>>> -    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>>> -        pmc->idx != ARMV8_PMU_CYCLE_IDX)
>>> +    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>>> +        return;
>>> +
>>> +    /*
>>> +     * If we have a filter in place and that the event isn't
>>> allowed, do
>>> +     * not install a perf event either.
>>> +     */
>>> +    if (vcpu->kvm->arch.pmu_filter &&
>>> +        !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>>          return;
>>>
>>>      memset(&attr, 0, sizeof(struct perf_event_attr));
>>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct
>>> kvm_vcpu *vcpu, u64 select_idx)
>>>      attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>>      attr.exclude_hv = 1; /* Don't count EL2 events */
>>>      attr.exclude_host = 1; /* Don't count host events */
>>> -    attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>>> -        ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>>> +    attr.config = eventsel;
>> So in that case the guest counter will not increment but the guest does
>> not know the counter is not implemented. Can't this lead to bad user
>> experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?
> 
> The whole point is that we want to keep things hidden from the guest.
> Also, PMCEID{0,1} only describe a small set of events (the architected
> common events), and not the whole range of microarchitectural events
> that the CPU implements.

I am still not totally convinced. Things are not totally hidden to the
guest as the counter does not increment, right? So a guest may try to
use as it is advertised in PMCEID0/1 but not get the expected results
leading to potential support request. I agree not all the events are
described there but your API also allows to filter out some of the ones
that are advertised.
> 
>>>
>>>      counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>>>
>>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>>>
>>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>  {
>>> -    if (!kvm_arm_support_pmu_v3())
>>> -        return -ENODEV;
>>> -
>>> -    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>> -        return -ENXIO;
>>> -
>>> -    if (vcpu->arch.pmu.created)
>>> -        return -EBUSY;
>>> -
>>>      if (irqchip_in_kernel(vcpu->kvm)) {
>>>          int ret;
>>>
>>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm,
>>> int irq)
>>>      return true;
>>>  }
>>>
>>> +#define NR_EVENTS    (ARMV8_PMU_EVTYPE_EVENT + 1)
>>> +
>>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct
>>> kvm_device_attr *attr)
>>>  {
>>> +    if (!kvm_arm_support_pmu_v3())
>>> +        return -ENODEV;
>>> +
>>> +    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>> +        return -ENODEV;
>> I see you changed -ENXIO into -ENODEV. wanted?
> 
> Probably not... but see below.
> 
>>> +
>>> +    if (vcpu->arch.pmu.created)
>>> +        return -EBUSY;
>>> +
>>>      switch (attr->attr) {
>>>      case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>>          int __user *uaddr = (int __user *)(long)attr->addr;
>>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>> *vcpu, struct kvm_device_attr *attr)
>>>          if (!irqchip_in_kernel(vcpu->kvm))
>>>              return -EINVAL;
>>>
>>> -        if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>> -            return -ENODEV;
>>> -
> 
> Here's why. I wonder if we already have a problem with the consistency
> of the
> error codes returned to userspace.
OK. Then you may document it in the commit message?
> 
>>>          if (get_user(irq, uaddr))
>>>              return -EFAULT;
>>>
>>> @@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>> *vcpu, struct kvm_device_attr *attr)
>>>          vcpu->arch.pmu.irq_num = irq;
>>>          return 0;
>>>      }
>>> +    case KVM_ARM_VCPU_PMU_V3_FILTER: {
>>> +        struct kvm_pmu_event_filter __user *uaddr;
>>> +        struct kvm_pmu_event_filter filter;
>>> +
>>> +        uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
>>> +
>>> +        if (copy_from_user(&filter, uaddr, sizeof(filter)))
>>> +            return -EFAULT;
>>> +
>>> +        if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
>> isnt't it >= ?
> 
> No, I think this is correct. I want to be able to filter event 0xFFFF,
> for example,
> so I have base_event=0xFFFF and nevents=1.
hum my mistake. Sorry
> 
>>> +            (filter.action != KVM_PMU_EVENT_ALLOW &&
>>> +             filter.action != KVM_PMU_EVENT_DENY))
>>> +            return -EINVAL;
>> -EINVAL is not documented in the API doc.
> 
> Good point.
> 
>>> +
>>> +        mutex_lock(&vcpu->kvm->lock);
>>> +
>>> +        if (!vcpu->kvm->arch.pmu_filter) {
>>> +            vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS,
>>> GFP_KERNEL);
>>> +            if (!vcpu->kvm->arch.pmu_filter) {
>>> +                mutex_unlock(&vcpu->kvm->lock);
>>> +                return -ENOMEM;
>>> +            }
>>> +
>>> +            /*
>>> +             * The default depends on the first applied filter.
>>> +             * If it allows events, the default is to deny.
>>> +             * Conversely, if the first filter denies a set of
>>> +             * events, the default is to allow.
>>> +             */
>>> +            if (filter.action == KVM_PMU_EVENT_ALLOW)
>>> +                bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>>> +            else
>>> +                bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>>> +        }
>>> +
>>> +        if (filter.action == KVM_PMU_EVENT_ALLOW)
>>> +            bitmap_set(vcpu->kvm->arch.pmu_filter,
>>> filter.base_event, filter.nevents);
>>> +        else
>>> +            bitmap_clear(vcpu->kvm->arch.pmu_filter,
>>> filter.base_event, filter.nevents);
>>> +
>>> +        mutex_unlock(&vcpu->kvm->lock);
>>> +
>>> +        return 0;
>>> +    }
>>>      case KVM_ARM_VCPU_PMU_V3_INIT:
>>>          return kvm_arm_pmu_v3_init(vcpu);
>>>      }
>>> @@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu
>>> *vcpu, struct kvm_device_attr *attr)
>>>      switch (attr->attr) {
>>>      case KVM_ARM_VCPU_PMU_V3_IRQ:
>> not related to this patch but shouldn't we advertise this only with
>> in-kernel irqchip?
> 
> We do support the PMU without the in-kernel chip, unfortunately... Yes,
> supporting this feature was a big mistake.
But I see in kvm_arm_pmu_v3_set_attr:
case KVM_ARM_VCPU_PMU_V3_IRQ:
../..
                if (!irqchip_in_kernel(vcpu->kvm))
                        return -EINVAL;

Thanks

Eric



> 
>>>      case KVM_ARM_VCPU_PMU_V3_INIT:
>>> +    case KVM_ARM_VCPU_PMU_V3_FILTER:
>>>          if (kvm_arm_support_pmu_v3() &&
>>>              test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>              return 0;
> 
> Thanks for the review,
> 
>         M.


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

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Auger Eric]

Hi Marc,

On 3/10/20 12:03 PM, Marc Zyngier wrote:
> Hi Eric,
> 
> On 2020-03-09 18:05, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>> guest to be able to count some of the implemented events (because this
>>> would give information on shared resources, for example.
>>>
>>> For this, let's extend the PMUv3 device API, and offer a way to setup a
>>> bitmap of the allowed events (the default being no bitmap, and thus no
>>> filtering).
>>>
>>> Userspace can thus allow/deny ranges of event. The default policy
>>> depends on the "polarity" of the first filter setup (default deny if the
>>> filter allows events, and default allow if the filter denies events).
>>> This allows to setup exactly what is allowed for a given guest.
>>>
>>> Note that although the ioctl is per-vcpu, the map of allowed events is
>>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>>> initialized).
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>  virt/kvm/arm/arm.c                |  2 +
>>>  virt/kvm/arm/pmu.c                | 84 +++++++++++++++++++++++++------
>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>> b/arch/arm64/include/asm/kvm_host.h
>>> index 57fd46acd058..8e63c618688d 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>       * supported.
>>>       */
>>>      bool return_nisv_io_abort_to_user;
>>> +
>>> +    /*
>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>> +     * for up to 65536 events
>>> +     */
>>> +    unsigned long *pmu_filter;
>>>  };
>>>
>>>  #define KVM_NR_MEM_OBJS     40
>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>> b/arch/arm64/include/uapi/asm/kvm.h
>>> index ba85bb23f060..7b1511d6ce44 100644
>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>  struct kvm_arch_memory_slot {
>>>  };
>>>
>>> +/*
>>> + * PMU filter structure. Describe a range of events with a particular
>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>> + */
>>> +struct kvm_pmu_event_filter {
>>> +    __u16    base_event;
>>> +    __u16    nevents;
>>> +
>>> +#define KVM_PMU_EVENT_ALLOW    0
>>> +#define KVM_PMU_EVENT_DENY    1
>>> +
>>> +    __u8    action;
>>> +    __u8    pad[3];
>>> +};
>>> +
>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>  struct kvm_vcpu_events {
>>>      struct {
>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>> index eda7b624eab8..8d849ac88a44 100644
>>> --- a/virt/kvm/arm/arm.c
>>> +++ b/virt/kvm/arm/arm.c
>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>      kvm->arch.last_vcpu_ran = NULL;
>>>
>>> +    bitmap_free(kvm->arch.pmu_filter);
>>> +
>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>          if (kvm->vcpus[i]) {
>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>> --- a/virt/kvm/arm/pmu.c
>>> +++ b/virt/kvm/arm/pmu.c
>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>> kvm_vcpu *vcpu, u64 select_idx)
>>>
>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>> nit:
>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>     else
>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
> 
> You don't like it? ;-)
? eventset set only once instead of 2 times
> 
>>>
>>>      /* Software increment event does't need to be backed by a perf
>>> event */
>> nit: while wer are at it fix the does't typo
>>> -    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>>> -        pmc->idx != ARMV8_PMU_CYCLE_IDX)
>>> +    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>>> +        return;
>>> +
>>> +    /*
>>> +     * If we have a filter in place and that the event isn't
>>> allowed, do
>>> +     * not install a perf event either.
>>> +     */
>>> +    if (vcpu->kvm->arch.pmu_filter &&
>>> +        !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>>          return;
>>>
>>>      memset(&attr, 0, sizeof(struct perf_event_attr));
>>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct
>>> kvm_vcpu *vcpu, u64 select_idx)
>>>      attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>>      attr.exclude_hv = 1; /* Don't count EL2 events */
>>>      attr.exclude_host = 1; /* Don't count host events */
>>> -    attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>>> -        ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>>> +    attr.config = eventsel;
>> So in that case the guest counter will not increment but the guest does
>> not know the counter is not implemented. Can't this lead to bad user
>> experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?
> 
> The whole point is that we want to keep things hidden from the guest.
> Also, PMCEID{0,1} only describe a small set of events (the architected
> common events), and not the whole range of microarchitectural events
> that the CPU implements.

I am still not totally convinced. Things are not totally hidden to the
guest as the counter does not increment, right? So a guest may try to
use as it is advertised in PMCEID0/1 but not get the expected results
leading to potential support request. I agree not all the events are
described there but your API also allows to filter out some of the ones
that are advertised.
> 
>>>
>>>      counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>>>
>>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>>>
>>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>  {
>>> -    if (!kvm_arm_support_pmu_v3())
>>> -        return -ENODEV;
>>> -
>>> -    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>> -        return -ENXIO;
>>> -
>>> -    if (vcpu->arch.pmu.created)
>>> -        return -EBUSY;
>>> -
>>>      if (irqchip_in_kernel(vcpu->kvm)) {
>>>          int ret;
>>>
>>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm,
>>> int irq)
>>>      return true;
>>>  }
>>>
>>> +#define NR_EVENTS    (ARMV8_PMU_EVTYPE_EVENT + 1)
>>> +
>>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct
>>> kvm_device_attr *attr)
>>>  {
>>> +    if (!kvm_arm_support_pmu_v3())
>>> +        return -ENODEV;
>>> +
>>> +    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>> +        return -ENODEV;
>> I see you changed -ENXIO into -ENODEV. wanted?
> 
> Probably not... but see below.
> 
>>> +
>>> +    if (vcpu->arch.pmu.created)
>>> +        return -EBUSY;
>>> +
>>>      switch (attr->attr) {
>>>      case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>>          int __user *uaddr = (int __user *)(long)attr->addr;
>>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>> *vcpu, struct kvm_device_attr *attr)
>>>          if (!irqchip_in_kernel(vcpu->kvm))
>>>              return -EINVAL;
>>>
>>> -        if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>> -            return -ENODEV;
>>> -
> 
> Here's why. I wonder if we already have a problem with the consistency
> of the
> error codes returned to userspace.
OK. Then you may document it in the commit message?
> 
>>>          if (get_user(irq, uaddr))
>>>              return -EFAULT;
>>>
>>> @@ -824,6 +831,50 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>> *vcpu, struct kvm_device_attr *attr)
>>>          vcpu->arch.pmu.irq_num = irq;
>>>          return 0;
>>>      }
>>> +    case KVM_ARM_VCPU_PMU_V3_FILTER: {
>>> +        struct kvm_pmu_event_filter __user *uaddr;
>>> +        struct kvm_pmu_event_filter filter;
>>> +
>>> +        uaddr = (struct kvm_pmu_event_filter __user *)(long)attr->addr;
>>> +
>>> +        if (copy_from_user(&filter, uaddr, sizeof(filter)))
>>> +            return -EFAULT;
>>> +
>>> +        if (((u32)filter.base_event + filter.nevents) > NR_EVENTS ||
>> isnt't it >= ?
> 
> No, I think this is correct. I want to be able to filter event 0xFFFF,
> for example,
> so I have base_event=0xFFFF and nevents=1.
hum my mistake. Sorry
> 
>>> +            (filter.action != KVM_PMU_EVENT_ALLOW &&
>>> +             filter.action != KVM_PMU_EVENT_DENY))
>>> +            return -EINVAL;
>> -EINVAL is not documented in the API doc.
> 
> Good point.
> 
>>> +
>>> +        mutex_lock(&vcpu->kvm->lock);
>>> +
>>> +        if (!vcpu->kvm->arch.pmu_filter) {
>>> +            vcpu->kvm->arch.pmu_filter = bitmap_alloc(NR_EVENTS,
>>> GFP_KERNEL);
>>> +            if (!vcpu->kvm->arch.pmu_filter) {
>>> +                mutex_unlock(&vcpu->kvm->lock);
>>> +                return -ENOMEM;
>>> +            }
>>> +
>>> +            /*
>>> +             * The default depends on the first applied filter.
>>> +             * If it allows events, the default is to deny.
>>> +             * Conversely, if the first filter denies a set of
>>> +             * events, the default is to allow.
>>> +             */
>>> +            if (filter.action == KVM_PMU_EVENT_ALLOW)
>>> +                bitmap_zero(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>>> +            else
>>> +                bitmap_fill(vcpu->kvm->arch.pmu_filter, NR_EVENTS);
>>> +        }
>>> +
>>> +        if (filter.action == KVM_PMU_EVENT_ALLOW)
>>> +            bitmap_set(vcpu->kvm->arch.pmu_filter,
>>> filter.base_event, filter.nevents);
>>> +        else
>>> +            bitmap_clear(vcpu->kvm->arch.pmu_filter,
>>> filter.base_event, filter.nevents);
>>> +
>>> +        mutex_unlock(&vcpu->kvm->lock);
>>> +
>>> +        return 0;
>>> +    }
>>>      case KVM_ARM_VCPU_PMU_V3_INIT:
>>>          return kvm_arm_pmu_v3_init(vcpu);
>>>      }
>>> @@ -860,6 +911,7 @@ int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu
>>> *vcpu, struct kvm_device_attr *attr)
>>>      switch (attr->attr) {
>>>      case KVM_ARM_VCPU_PMU_V3_IRQ:
>> not related to this patch but shouldn't we advertise this only with
>> in-kernel irqchip?
> 
> We do support the PMU without the in-kernel chip, unfortunately... Yes,
> supporting this feature was a big mistake.
But I see in kvm_arm_pmu_v3_set_attr:
case KVM_ARM_VCPU_PMU_V3_IRQ:
../..
                if (!irqchip_in_kernel(vcpu->kvm))
                        return -EINVAL;

Thanks

Eric



> 
>>>      case KVM_ARM_VCPU_PMU_V3_INIT:
>>> +    case KVM_ARM_VCPU_PMU_V3_FILTER:
>>>          if (kvm_arm_support_pmu_v3() &&
>>>              test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>              return 0;
> 
> Thanks for the review,
> 
>         M.

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

On 2020-03-10 17:40, Auger Eric wrote:
> Hi Marc,
> 
> On 3/10/20 12:03 PM, Marc Zyngier wrote:
>> Hi Eric,
>> 
>> On 2020-03-09 18:05, Auger Eric wrote:
>>> Hi Marc,
>>> 
>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>>> guest to be able to count some of the implemented events (because 
>>>> this
>>>> would give information on shared resources, for example.
>>>> 
>>>> For this, let's extend the PMUv3 device API, and offer a way to 
>>>> setup a
>>>> bitmap of the allowed events (the default being no bitmap, and thus 
>>>> no
>>>> filtering).
>>>> 
>>>> Userspace can thus allow/deny ranges of event. The default policy
>>>> depends on the "polarity" of the first filter setup (default deny if 
>>>> the
>>>> filter allows events, and default allow if the filter denies 
>>>> events).
>>>> This allows to setup exactly what is allowed for a given guest.
>>>> 
>>>> Note that although the ioctl is per-vcpu, the map of allowed events 
>>>> is
>>>> global to the VM (it can be setup from any vcpu until the vcpu PMU 
>>>> is
>>>> initialized).
>>>> 
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> ---
>>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>>  virt/kvm/arm/arm.c                |  2 +
>>>>  virt/kvm/arm/pmu.c                | 84 
>>>> +++++++++++++++++++++++++------
>>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>> 
>>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>>> b/arch/arm64/include/asm/kvm_host.h
>>>> index 57fd46acd058..8e63c618688d 100644
>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>>       * supported.
>>>>       */
>>>>      bool return_nisv_io_abort_to_user;
>>>> +
>>>> +    /*
>>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>>> +     * for up to 65536 events
>>>> +     */
>>>> +    unsigned long *pmu_filter;
>>>>  };
>>>> 
>>>>  #define KVM_NR_MEM_OBJS     40
>>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>>> b/arch/arm64/include/uapi/asm/kvm.h
>>>> index ba85bb23f060..7b1511d6ce44 100644
>>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>>  struct kvm_arch_memory_slot {
>>>>  };
>>>> 
>>>> +/*
>>>> + * PMU filter structure. Describe a range of events with a 
>>>> particular
>>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>>> + */
>>>> +struct kvm_pmu_event_filter {
>>>> +    __u16    base_event;
>>>> +    __u16    nevents;
>>>> +
>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>> +#define KVM_PMU_EVENT_DENY    1
>>>> +
>>>> +    __u8    action;
>>>> +    __u8    pad[3];
>>>> +};
>>>> +
>>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>>  struct kvm_vcpu_events {
>>>>      struct {
>>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>> index eda7b624eab8..8d849ac88a44 100644
>>>> --- a/virt/kvm/arm/arm.c
>>>> +++ b/virt/kvm/arm/arm.c
>>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>>      kvm->arch.last_vcpu_ran = NULL;
>>>> 
>>>> +    bitmap_free(kvm->arch.pmu_filter);
>>>> +
>>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>>          if (kvm->vcpus[i]) {
>>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>>> --- a/virt/kvm/arm/pmu.c
>>>> +++ b/virt/kvm/arm/pmu.c
>>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>> 
>>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>> nit:
>>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>     else
>>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>> 
>> You don't like it? ;-)
> ? eventset set only once instead of 2 times

The compiler does the right thing, but sore, I'll change it.

>> 
>>>> 
>>>>      /* Software increment event does't need to be backed by a perf
>>>> event */
>>> nit: while wer are at it fix the does't typo
>>>> -    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>>>> -        pmc->idx != ARMV8_PMU_CYCLE_IDX)
>>>> +    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>>>> +        return;
>>>> +
>>>> +    /*
>>>> +     * If we have a filter in place and that the event isn't
>>>> allowed, do
>>>> +     * not install a perf event either.
>>>> +     */
>>>> +    if (vcpu->kvm->arch.pmu_filter &&
>>>> +        !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>>>          return;
>>>> 
>>>>      memset(&attr, 0, sizeof(struct perf_event_attr));
>>>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct
>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>      attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>>>      attr.exclude_hv = 1; /* Don't count EL2 events */
>>>>      attr.exclude_host = 1; /* Don't count host events */
>>>> -    attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>>>> -        ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>>>> +    attr.config = eventsel;
>>> So in that case the guest counter will not increment but the guest 
>>> does
>>> not know the counter is not implemented. Can't this lead to bad user
>>> experience. Shouldn't this disablement be reflected in PMCEID0/1 
>>> regs?
>> 
>> The whole point is that we want to keep things hidden from the guest.
>> Also, PMCEID{0,1} only describe a small set of events (the architected
>> common events), and not the whole range of microarchitectural events
>> that the CPU implements.
> 
> I am still not totally convinced. Things are not totally hidden to the
> guest as the counter does not increment, right? So a guest may try to
> use as it is advertised in PMCEID0/1 but not get the expected results
> leading to potential support request. I agree not all the events are
> described there but your API also allows to filter out some of the ones
> that are advertised.

I think we're at odds when it comes to the goal of this series. If you
read the CPU TRM, you will find that event X is implemented. You look
at PMCEIDx, and you find it is not. You still get a support request! ;-)

Dropping events from these registers is totally trivial, but I'm not
sure this will reduce the surprise effect. It doesn't hurt anyway, so
I'll implement that.

>> 
>>>> 
>>>>      counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>>>> 
>>>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu 
>>>> *vcpu)
>>>> 
>>>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>>  {
>>>> -    if (!kvm_arm_support_pmu_v3())
>>>> -        return -ENODEV;
>>>> -
>>>> -    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>> -        return -ENXIO;
>>>> -
>>>> -    if (vcpu->arch.pmu.created)
>>>> -        return -EBUSY;
>>>> -
>>>>      if (irqchip_in_kernel(vcpu->kvm)) {
>>>>          int ret;
>>>> 
>>>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm,
>>>> int irq)
>>>>      return true;
>>>>  }
>>>> 
>>>> +#define NR_EVENTS    (ARMV8_PMU_EVTYPE_EVENT + 1)
>>>> +
>>>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct
>>>> kvm_device_attr *attr)
>>>>  {
>>>> +    if (!kvm_arm_support_pmu_v3())
>>>> +        return -ENODEV;
>>>> +
>>>> +    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>> +        return -ENODEV;
>>> I see you changed -ENXIO into -ENODEV. wanted?
>> 
>> Probably not... but see below.
>> 
>>>> +
>>>> +    if (vcpu->arch.pmu.created)
>>>> +        return -EBUSY;
>>>> +
>>>>      switch (attr->attr) {
>>>>      case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>>>          int __user *uaddr = (int __user *)(long)attr->addr;
>>>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>>> *vcpu, struct kvm_device_attr *attr)
>>>>          if (!irqchip_in_kernel(vcpu->kvm))
>>>>              return -EINVAL;
>>>> 
>>>> -        if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>> -            return -ENODEV;
>>>> -
>> 
>> Here's why. I wonder if we already have a problem with the consistency
>> of the
>> error codes returned to userspace.
> OK. Then you may document it in the commit message?

I still need to work out whether we actually have an issue on that.

[...]

>>> not related to this patch but shouldn't we advertise this only with
>>> in-kernel irqchip?
>> 
>> We do support the PMU without the in-kernel chip, unfortunately... 
>> Yes,
>> supporting this feature was a big mistake.
> But I see in kvm_arm_pmu_v3_set_attr:
> case KVM_ARM_VCPU_PMU_V3_IRQ:
> ../..
>                 if (!irqchip_in_kernel(vcpu->kvm))
>                         return -EINVAL;

Ah, I see what you mean. Yes, we probably shouldn't report that the PMU
IRQ attribute is supported when we don't have an in-kernel irqchip.

Thanks,

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

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

On 2020-03-10 17:40, Auger Eric wrote:
> Hi Marc,
> 
> On 3/10/20 12:03 PM, Marc Zyngier wrote:
>> Hi Eric,
>> 
>> On 2020-03-09 18:05, Auger Eric wrote:
>>> Hi Marc,
>>> 
>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>>> guest to be able to count some of the implemented events (because 
>>>> this
>>>> would give information on shared resources, for example.
>>>> 
>>>> For this, let's extend the PMUv3 device API, and offer a way to 
>>>> setup a
>>>> bitmap of the allowed events (the default being no bitmap, and thus 
>>>> no
>>>> filtering).
>>>> 
>>>> Userspace can thus allow/deny ranges of event. The default policy
>>>> depends on the "polarity" of the first filter setup (default deny if 
>>>> the
>>>> filter allows events, and default allow if the filter denies 
>>>> events).
>>>> This allows to setup exactly what is allowed for a given guest.
>>>> 
>>>> Note that although the ioctl is per-vcpu, the map of allowed events 
>>>> is
>>>> global to the VM (it can be setup from any vcpu until the vcpu PMU 
>>>> is
>>>> initialized).
>>>> 
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> ---
>>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>>  virt/kvm/arm/arm.c                |  2 +
>>>>  virt/kvm/arm/pmu.c                | 84 
>>>> +++++++++++++++++++++++++------
>>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>> 
>>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>>> b/arch/arm64/include/asm/kvm_host.h
>>>> index 57fd46acd058..8e63c618688d 100644
>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>>       * supported.
>>>>       */
>>>>      bool return_nisv_io_abort_to_user;
>>>> +
>>>> +    /*
>>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>>> +     * for up to 65536 events
>>>> +     */
>>>> +    unsigned long *pmu_filter;
>>>>  };
>>>> 
>>>>  #define KVM_NR_MEM_OBJS     40
>>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>>> b/arch/arm64/include/uapi/asm/kvm.h
>>>> index ba85bb23f060..7b1511d6ce44 100644
>>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>>  struct kvm_arch_memory_slot {
>>>>  };
>>>> 
>>>> +/*
>>>> + * PMU filter structure. Describe a range of events with a 
>>>> particular
>>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>>> + */
>>>> +struct kvm_pmu_event_filter {
>>>> +    __u16    base_event;
>>>> +    __u16    nevents;
>>>> +
>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>> +#define KVM_PMU_EVENT_DENY    1
>>>> +
>>>> +    __u8    action;
>>>> +    __u8    pad[3];
>>>> +};
>>>> +
>>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>>  struct kvm_vcpu_events {
>>>>      struct {
>>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>> index eda7b624eab8..8d849ac88a44 100644
>>>> --- a/virt/kvm/arm/arm.c
>>>> +++ b/virt/kvm/arm/arm.c
>>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>>      kvm->arch.last_vcpu_ran = NULL;
>>>> 
>>>> +    bitmap_free(kvm->arch.pmu_filter);
>>>> +
>>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>>          if (kvm->vcpus[i]) {
>>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>>> --- a/virt/kvm/arm/pmu.c
>>>> +++ b/virt/kvm/arm/pmu.c
>>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>> 
>>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>> nit:
>>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>     else
>>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>> 
>> You don't like it? ;-)
> ? eventset set only once instead of 2 times

The compiler does the right thing, but sore, I'll change it.

>> 
>>>> 
>>>>      /* Software increment event does't need to be backed by a perf
>>>> event */
>>> nit: while wer are at it fix the does't typo
>>>> -    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>>>> -        pmc->idx != ARMV8_PMU_CYCLE_IDX)
>>>> +    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>>>> +        return;
>>>> +
>>>> +    /*
>>>> +     * If we have a filter in place and that the event isn't
>>>> allowed, do
>>>> +     * not install a perf event either.
>>>> +     */
>>>> +    if (vcpu->kvm->arch.pmu_filter &&
>>>> +        !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>>>          return;
>>>> 
>>>>      memset(&attr, 0, sizeof(struct perf_event_attr));
>>>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct
>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>      attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>>>      attr.exclude_hv = 1; /* Don't count EL2 events */
>>>>      attr.exclude_host = 1; /* Don't count host events */
>>>> -    attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>>>> -        ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>>>> +    attr.config = eventsel;
>>> So in that case the guest counter will not increment but the guest 
>>> does
>>> not know the counter is not implemented. Can't this lead to bad user
>>> experience. Shouldn't this disablement be reflected in PMCEID0/1 
>>> regs?
>> 
>> The whole point is that we want to keep things hidden from the guest.
>> Also, PMCEID{0,1} only describe a small set of events (the architected
>> common events), and not the whole range of microarchitectural events
>> that the CPU implements.
> 
> I am still not totally convinced. Things are not totally hidden to the
> guest as the counter does not increment, right? So a guest may try to
> use as it is advertised in PMCEID0/1 but not get the expected results
> leading to potential support request. I agree not all the events are
> described there but your API also allows to filter out some of the ones
> that are advertised.

I think we're at odds when it comes to the goal of this series. If you
read the CPU TRM, you will find that event X is implemented. You look
at PMCEIDx, and you find it is not. You still get a support request! ;-)

Dropping events from these registers is totally trivial, but I'm not
sure this will reduce the surprise effect. It doesn't hurt anyway, so
I'll implement that.

>> 
>>>> 
>>>>      counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>>>> 
>>>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu 
>>>> *vcpu)
>>>> 
>>>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>>  {
>>>> -    if (!kvm_arm_support_pmu_v3())
>>>> -        return -ENODEV;
>>>> -
>>>> -    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>> -        return -ENXIO;
>>>> -
>>>> -    if (vcpu->arch.pmu.created)
>>>> -        return -EBUSY;
>>>> -
>>>>      if (irqchip_in_kernel(vcpu->kvm)) {
>>>>          int ret;
>>>> 
>>>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm,
>>>> int irq)
>>>>      return true;
>>>>  }
>>>> 
>>>> +#define NR_EVENTS    (ARMV8_PMU_EVTYPE_EVENT + 1)
>>>> +
>>>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct
>>>> kvm_device_attr *attr)
>>>>  {
>>>> +    if (!kvm_arm_support_pmu_v3())
>>>> +        return -ENODEV;
>>>> +
>>>> +    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>> +        return -ENODEV;
>>> I see you changed -ENXIO into -ENODEV. wanted?
>> 
>> Probably not... but see below.
>> 
>>>> +
>>>> +    if (vcpu->arch.pmu.created)
>>>> +        return -EBUSY;
>>>> +
>>>>      switch (attr->attr) {
>>>>      case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>>>          int __user *uaddr = (int __user *)(long)attr->addr;
>>>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>>> *vcpu, struct kvm_device_attr *attr)
>>>>          if (!irqchip_in_kernel(vcpu->kvm))
>>>>              return -EINVAL;
>>>> 
>>>> -        if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>> -            return -ENODEV;
>>>> -
>> 
>> Here's why. I wonder if we already have a problem with the consistency
>> of the
>> error codes returned to userspace.
> OK. Then you may document it in the commit message?

I still need to work out whether we actually have an issue on that.

[...]

>>> not related to this patch but shouldn't we advertise this only with
>>> in-kernel irqchip?
>> 
>> We do support the PMU without the in-kernel chip, unfortunately... 
>> Yes,
>> supporting this feature was a big mistake.
> But I see in kvm_arm_pmu_v3_set_attr:
> case KVM_ARM_VCPU_PMU_V3_IRQ:
> ../..
>                 if (!irqchip_in_kernel(vcpu->kvm))
>                         return -EINVAL;

Ah, I see what you mean. Yes, we probably shouldn't report that the PMU
IRQ attribute is supported when we don't have an in-kernel irqchip.

Thanks,

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

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

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

On 2020-03-10 17:40, Auger Eric wrote:
> Hi Marc,
> 
> On 3/10/20 12:03 PM, Marc Zyngier wrote:
>> Hi Eric,
>> 
>> On 2020-03-09 18:05, Auger Eric wrote:
>>> Hi Marc,
>>> 
>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>>> guest to be able to count some of the implemented events (because 
>>>> this
>>>> would give information on shared resources, for example.
>>>> 
>>>> For this, let's extend the PMUv3 device API, and offer a way to 
>>>> setup a
>>>> bitmap of the allowed events (the default being no bitmap, and thus 
>>>> no
>>>> filtering).
>>>> 
>>>> Userspace can thus allow/deny ranges of event. The default policy
>>>> depends on the "polarity" of the first filter setup (default deny if 
>>>> the
>>>> filter allows events, and default allow if the filter denies 
>>>> events).
>>>> This allows to setup exactly what is allowed for a given guest.
>>>> 
>>>> Note that although the ioctl is per-vcpu, the map of allowed events 
>>>> is
>>>> global to the VM (it can be setup from any vcpu until the vcpu PMU 
>>>> is
>>>> initialized).
>>>> 
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> ---
>>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>>  virt/kvm/arm/arm.c                |  2 +
>>>>  virt/kvm/arm/pmu.c                | 84 
>>>> +++++++++++++++++++++++++------
>>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>> 
>>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>>> b/arch/arm64/include/asm/kvm_host.h
>>>> index 57fd46acd058..8e63c618688d 100644
>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>>       * supported.
>>>>       */
>>>>      bool return_nisv_io_abort_to_user;
>>>> +
>>>> +    /*
>>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>>> +     * for up to 65536 events
>>>> +     */
>>>> +    unsigned long *pmu_filter;
>>>>  };
>>>> 
>>>>  #define KVM_NR_MEM_OBJS     40
>>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>>> b/arch/arm64/include/uapi/asm/kvm.h
>>>> index ba85bb23f060..7b1511d6ce44 100644
>>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>>  struct kvm_arch_memory_slot {
>>>>  };
>>>> 
>>>> +/*
>>>> + * PMU filter structure. Describe a range of events with a 
>>>> particular
>>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>>> + */
>>>> +struct kvm_pmu_event_filter {
>>>> +    __u16    base_event;
>>>> +    __u16    nevents;
>>>> +
>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>> +#define KVM_PMU_EVENT_DENY    1
>>>> +
>>>> +    __u8    action;
>>>> +    __u8    pad[3];
>>>> +};
>>>> +
>>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>>  struct kvm_vcpu_events {
>>>>      struct {
>>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>> index eda7b624eab8..8d849ac88a44 100644
>>>> --- a/virt/kvm/arm/arm.c
>>>> +++ b/virt/kvm/arm/arm.c
>>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>>      kvm->arch.last_vcpu_ran = NULL;
>>>> 
>>>> +    bitmap_free(kvm->arch.pmu_filter);
>>>> +
>>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>>          if (kvm->vcpus[i]) {
>>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>>> --- a/virt/kvm/arm/pmu.c
>>>> +++ b/virt/kvm/arm/pmu.c
>>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>> 
>>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>> nit:
>>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>     else
>>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>> 
>> You don't like it? ;-)
> ? eventset set only once instead of 2 times

The compiler does the right thing, but sore, I'll change it.

>> 
>>>> 
>>>>      /* Software increment event does't need to be backed by a perf
>>>> event */
>>> nit: while wer are at it fix the does't typo
>>>> -    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>>>> -        pmc->idx != ARMV8_PMU_CYCLE_IDX)
>>>> +    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>>>> +        return;
>>>> +
>>>> +    /*
>>>> +     * If we have a filter in place and that the event isn't
>>>> allowed, do
>>>> +     * not install a perf event either.
>>>> +     */
>>>> +    if (vcpu->kvm->arch.pmu_filter &&
>>>> +        !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>>>          return;
>>>> 
>>>>      memset(&attr, 0, sizeof(struct perf_event_attr));
>>>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct
>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>      attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>>>      attr.exclude_hv = 1; /* Don't count EL2 events */
>>>>      attr.exclude_host = 1; /* Don't count host events */
>>>> -    attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>>>> -        ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>>>> +    attr.config = eventsel;
>>> So in that case the guest counter will not increment but the guest 
>>> does
>>> not know the counter is not implemented. Can't this lead to bad user
>>> experience. Shouldn't this disablement be reflected in PMCEID0/1 
>>> regs?
>> 
>> The whole point is that we want to keep things hidden from the guest.
>> Also, PMCEID{0,1} only describe a small set of events (the architected
>> common events), and not the whole range of microarchitectural events
>> that the CPU implements.
> 
> I am still not totally convinced. Things are not totally hidden to the
> guest as the counter does not increment, right? So a guest may try to
> use as it is advertised in PMCEID0/1 but not get the expected results
> leading to potential support request. I agree not all the events are
> described there but your API also allows to filter out some of the ones
> that are advertised.

I think we're at odds when it comes to the goal of this series. If you
read the CPU TRM, you will find that event X is implemented. You look
at PMCEIDx, and you find it is not. You still get a support request! ;-)

Dropping events from these registers is totally trivial, but I'm not
sure this will reduce the surprise effect. It doesn't hurt anyway, so
I'll implement that.

>> 
>>>> 
>>>>      counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>>>> 
>>>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu 
>>>> *vcpu)
>>>> 
>>>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>>  {
>>>> -    if (!kvm_arm_support_pmu_v3())
>>>> -        return -ENODEV;
>>>> -
>>>> -    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>> -        return -ENXIO;
>>>> -
>>>> -    if (vcpu->arch.pmu.created)
>>>> -        return -EBUSY;
>>>> -
>>>>      if (irqchip_in_kernel(vcpu->kvm)) {
>>>>          int ret;
>>>> 
>>>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm,
>>>> int irq)
>>>>      return true;
>>>>  }
>>>> 
>>>> +#define NR_EVENTS    (ARMV8_PMU_EVTYPE_EVENT + 1)
>>>> +
>>>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct
>>>> kvm_device_attr *attr)
>>>>  {
>>>> +    if (!kvm_arm_support_pmu_v3())
>>>> +        return -ENODEV;
>>>> +
>>>> +    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>> +        return -ENODEV;
>>> I see you changed -ENXIO into -ENODEV. wanted?
>> 
>> Probably not... but see below.
>> 
>>>> +
>>>> +    if (vcpu->arch.pmu.created)
>>>> +        return -EBUSY;
>>>> +
>>>>      switch (attr->attr) {
>>>>      case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>>>          int __user *uaddr = (int __user *)(long)attr->addr;
>>>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>>> *vcpu, struct kvm_device_attr *attr)
>>>>          if (!irqchip_in_kernel(vcpu->kvm))
>>>>              return -EINVAL;
>>>> 
>>>> -        if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>> -            return -ENODEV;
>>>> -
>> 
>> Here's why. I wonder if we already have a problem with the consistency
>> of the
>> error codes returned to userspace.
> OK. Then you may document it in the commit message?

I still need to work out whether we actually have an issue on that.

[...]

>>> not related to this patch but shouldn't we advertise this only with
>>> in-kernel irqchip?
>> 
>> We do support the PMU without the in-kernel chip, unfortunately... 
>> Yes,
>> supporting this feature was a big mistake.
> But I see in kvm_arm_pmu_v3_set_attr:
> case KVM_ARM_VCPU_PMU_V3_IRQ:
> ../..
>                 if (!irqchip_in_kernel(vcpu->kvm))
>                         return -EINVAL;

Ah, I see what you mean. Yes, we probably shouldn't report that the PMU
IRQ attribute is supported when we don't have an in-kernel irqchip.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Auger Eric]

Hi Marc,

On 3/10/20 7:00 PM, Marc Zyngier wrote:
> On 2020-03-10 17:40, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/10/20 12:03 PM, Marc Zyngier wrote:
>>> Hi Eric,
>>>
>>> On 2020-03-09 18:05, Auger Eric wrote:
>>>> Hi Marc,
>>>>
>>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>>>> guest to be able to count some of the implemented events (because this
>>>>> would give information on shared resources, for example.
>>>>>
>>>>> For this, let's extend the PMUv3 device API, and offer a way to
>>>>> setup a
>>>>> bitmap of the allowed events (the default being no bitmap, and thus no
>>>>> filtering).
>>>>>
>>>>> Userspace can thus allow/deny ranges of event. The default policy
>>>>> depends on the "polarity" of the first filter setup (default deny
>>>>> if the
>>>>> filter allows events, and default allow if the filter denies events).
>>>>> This allows to setup exactly what is allowed for a given guest.
>>>>>
>>>>> Note that although the ioctl is per-vcpu, the map of allowed events is
>>>>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>>>>> initialized).
>>>>>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>> ---
>>>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>>>  virt/kvm/arm/arm.c                |  2 +
>>>>>  virt/kvm/arm/pmu.c                | 84
>>>>> +++++++++++++++++++++++++------
>>>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>>>> b/arch/arm64/include/asm/kvm_host.h
>>>>> index 57fd46acd058..8e63c618688d 100644
>>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>>>       * supported.
>>>>>       */
>>>>>      bool return_nisv_io_abort_to_user;
>>>>> +
>>>>> +    /*
>>>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>>>> +     * for up to 65536 events
>>>>> +     */
>>>>> +    unsigned long *pmu_filter;
>>>>>  };
>>>>>
>>>>>  #define KVM_NR_MEM_OBJS     40
>>>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>>>> b/arch/arm64/include/uapi/asm/kvm.h
>>>>> index ba85bb23f060..7b1511d6ce44 100644
>>>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>>>  struct kvm_arch_memory_slot {
>>>>>  };
>>>>>
>>>>> +/*
>>>>> + * PMU filter structure. Describe a range of events with a particular
>>>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>>>> + */
>>>>> +struct kvm_pmu_event_filter {
>>>>> +    __u16    base_event;
>>>>> +    __u16    nevents;
>>>>> +
>>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>>> +#define KVM_PMU_EVENT_DENY    1
>>>>> +
>>>>> +    __u8    action;
>>>>> +    __u8    pad[3];
>>>>> +};
>>>>> +
>>>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>>>  struct kvm_vcpu_events {
>>>>>      struct {
>>>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>>> index eda7b624eab8..8d849ac88a44 100644
>>>>> --- a/virt/kvm/arm/arm.c
>>>>> +++ b/virt/kvm/arm/arm.c
>>>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>>>      kvm->arch.last_vcpu_ran = NULL;
>>>>>
>>>>> +    bitmap_free(kvm->arch.pmu_filter);
>>>>> +
>>>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>>>          if (kvm->vcpus[i]) {
>>>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>>>> --- a/virt/kvm/arm/pmu.c
>>>>> +++ b/virt/kvm/arm/pmu.c
>>>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>>
>>>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>> nit:
>>>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>>     else
>>>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>
>>> You don't like it? ;-)
>> ? eventset set only once instead of 2 times
> 
> The compiler does the right thing, but sore, I'll change it.
> 
>>>
>>>>>
>>>>>      /* Software increment event does't need to be backed by a perf
>>>>> event */
>>>> nit: while wer are at it fix the does't typo
>>>>> -    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>>>>> -        pmc->idx != ARMV8_PMU_CYCLE_IDX)
>>>>> +    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>>>>> +        return;
>>>>> +
>>>>> +    /*
>>>>> +     * If we have a filter in place and that the event isn't
>>>>> allowed, do
>>>>> +     * not install a perf event either.
>>>>> +     */
>>>>> +    if (vcpu->kvm->arch.pmu_filter &&
>>>>> +        !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>>>>          return;
>>>>>
>>>>>      memset(&attr, 0, sizeof(struct perf_event_attr));
>>>>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct
>>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>>      attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>>>>      attr.exclude_hv = 1; /* Don't count EL2 events */
>>>>>      attr.exclude_host = 1; /* Don't count host events */
>>>>> -    attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>>>>> -        ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>>>>> +    attr.config = eventsel;
>>>> So in that case the guest counter will not increment but the guest does
>>>> not know the counter is not implemented. Can't this lead to bad user
>>>> experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?
>>>
>>> The whole point is that we want to keep things hidden from the guest.
>>> Also, PMCEID{0,1} only describe a small set of events (the architected
>>> common events), and not the whole range of microarchitectural events
>>> that the CPU implements.
>>
>> I am still not totally convinced. Things are not totally hidden to the
>> guest as the counter does not increment, right? So a guest may try to
>> use as it is advertised in PMCEID0/1 but not get the expected results
>> leading to potential support request. I agree not all the events are
>> described there but your API also allows to filter out some of the ones
>> that are advertised.
> 
> I think we're at odds when it comes to the goal of this series. If you
> read the CPU TRM, you will find that event X is implemented. You look
> at PMCEIDx, and you find it is not. You still get a support request! ;-)
Yep that's a weird situation indeed, I haven't thought about the TRM.
> 
> Dropping events from these registers is totally trivial, but I'm not
> sure this will reduce the surprise effect. It doesn't hurt anyway, so
> I'll implement that.
Up to you. Or at least you can document it in the commit msg.

Thanks

Eric

> 
>>>
>>>>>
>>>>>      counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>>>>>
>>>>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>>>>>
>>>>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>>>  {
>>>>> -    if (!kvm_arm_support_pmu_v3())
>>>>> -        return -ENODEV;
>>>>> -
>>>>> -    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>>> -        return -ENXIO;
>>>>> -
>>>>> -    if (vcpu->arch.pmu.created)
>>>>> -        return -EBUSY;
>>>>> -
>>>>>      if (irqchip_in_kernel(vcpu->kvm)) {
>>>>>          int ret;
>>>>>
>>>>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm,
>>>>> int irq)
>>>>>      return true;
>>>>>  }
>>>>>
>>>>> +#define NR_EVENTS    (ARMV8_PMU_EVTYPE_EVENT + 1)
>>>>> +
>>>>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct
>>>>> kvm_device_attr *attr)
>>>>>  {
>>>>> +    if (!kvm_arm_support_pmu_v3())
>>>>> +        return -ENODEV;
>>>>> +
>>>>> +    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>>> +        return -ENODEV;
>>>> I see you changed -ENXIO into -ENODEV. wanted?
>>>
>>> Probably not... but see below.
>>>
>>>>> +
>>>>> +    if (vcpu->arch.pmu.created)
>>>>> +        return -EBUSY;
>>>>> +
>>>>>      switch (attr->attr) {
>>>>>      case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>>>>          int __user *uaddr = (int __user *)(long)attr->addr;
>>>>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>>>> *vcpu, struct kvm_device_attr *attr)
>>>>>          if (!irqchip_in_kernel(vcpu->kvm))
>>>>>              return -EINVAL;
>>>>>
>>>>> -        if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>>> -            return -ENODEV;
>>>>> -
>>>
>>> Here's why. I wonder if we already have a problem with the consistency
>>> of the
>>> error codes returned to userspace.
>> OK. Then you may document it in the commit message?
> 
> I still need to work out whether we actually have an issue on that.
> 
> [...]
> 
>>>> not related to this patch but shouldn't we advertise this only with
>>>> in-kernel irqchip?
>>>
>>> We do support the PMU without the in-kernel chip, unfortunately... Yes,
>>> supporting this feature was a big mistake.
>> But I see in kvm_arm_pmu_v3_set_attr:
>> case KVM_ARM_VCPU_PMU_V3_IRQ:
>> ../..
>>                 if (!irqchip_in_kernel(vcpu->kvm))
>>                         return -EINVAL;
> 
> Ah, I see what you mean. Yes, we probably shouldn't report that the PMU
> IRQ attribute is supported when we don't have an in-kernel irqchip.
> 
> Thanks,
> 
>         M.

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Auger Eric]

Hi Marc,

On 3/10/20 7:00 PM, Marc Zyngier wrote:
> On 2020-03-10 17:40, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/10/20 12:03 PM, Marc Zyngier wrote:
>>> Hi Eric,
>>>
>>> On 2020-03-09 18:05, Auger Eric wrote:
>>>> Hi Marc,
>>>>
>>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>>>> guest to be able to count some of the implemented events (because this
>>>>> would give information on shared resources, for example.
>>>>>
>>>>> For this, let's extend the PMUv3 device API, and offer a way to
>>>>> setup a
>>>>> bitmap of the allowed events (the default being no bitmap, and thus no
>>>>> filtering).
>>>>>
>>>>> Userspace can thus allow/deny ranges of event. The default policy
>>>>> depends on the "polarity" of the first filter setup (default deny
>>>>> if the
>>>>> filter allows events, and default allow if the filter denies events).
>>>>> This allows to setup exactly what is allowed for a given guest.
>>>>>
>>>>> Note that although the ioctl is per-vcpu, the map of allowed events is
>>>>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>>>>> initialized).
>>>>>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>> ---
>>>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>>>  virt/kvm/arm/arm.c                |  2 +
>>>>>  virt/kvm/arm/pmu.c                | 84
>>>>> +++++++++++++++++++++++++------
>>>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>>>> b/arch/arm64/include/asm/kvm_host.h
>>>>> index 57fd46acd058..8e63c618688d 100644
>>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>>>       * supported.
>>>>>       */
>>>>>      bool return_nisv_io_abort_to_user;
>>>>> +
>>>>> +    /*
>>>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>>>> +     * for up to 65536 events
>>>>> +     */
>>>>> +    unsigned long *pmu_filter;
>>>>>  };
>>>>>
>>>>>  #define KVM_NR_MEM_OBJS     40
>>>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>>>> b/arch/arm64/include/uapi/asm/kvm.h
>>>>> index ba85bb23f060..7b1511d6ce44 100644
>>>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>>>  struct kvm_arch_memory_slot {
>>>>>  };
>>>>>
>>>>> +/*
>>>>> + * PMU filter structure. Describe a range of events with a particular
>>>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>>>> + */
>>>>> +struct kvm_pmu_event_filter {
>>>>> +    __u16    base_event;
>>>>> +    __u16    nevents;
>>>>> +
>>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>>> +#define KVM_PMU_EVENT_DENY    1
>>>>> +
>>>>> +    __u8    action;
>>>>> +    __u8    pad[3];
>>>>> +};
>>>>> +
>>>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>>>  struct kvm_vcpu_events {
>>>>>      struct {
>>>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>>> index eda7b624eab8..8d849ac88a44 100644
>>>>> --- a/virt/kvm/arm/arm.c
>>>>> +++ b/virt/kvm/arm/arm.c
>>>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>>>      kvm->arch.last_vcpu_ran = NULL;
>>>>>
>>>>> +    bitmap_free(kvm->arch.pmu_filter);
>>>>> +
>>>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>>>          if (kvm->vcpus[i]) {
>>>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>>>> --- a/virt/kvm/arm/pmu.c
>>>>> +++ b/virt/kvm/arm/pmu.c
>>>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>>
>>>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>> nit:
>>>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>>     else
>>>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>
>>> You don't like it? ;-)
>> ? eventset set only once instead of 2 times
> 
> The compiler does the right thing, but sore, I'll change it.
> 
>>>
>>>>>
>>>>>      /* Software increment event does't need to be backed by a perf
>>>>> event */
>>>> nit: while wer are at it fix the does't typo
>>>>> -    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>>>>> -        pmc->idx != ARMV8_PMU_CYCLE_IDX)
>>>>> +    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>>>>> +        return;
>>>>> +
>>>>> +    /*
>>>>> +     * If we have a filter in place and that the event isn't
>>>>> allowed, do
>>>>> +     * not install a perf event either.
>>>>> +     */
>>>>> +    if (vcpu->kvm->arch.pmu_filter &&
>>>>> +        !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>>>>          return;
>>>>>
>>>>>      memset(&attr, 0, sizeof(struct perf_event_attr));
>>>>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct
>>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>>      attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>>>>      attr.exclude_hv = 1; /* Don't count EL2 events */
>>>>>      attr.exclude_host = 1; /* Don't count host events */
>>>>> -    attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>>>>> -        ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>>>>> +    attr.config = eventsel;
>>>> So in that case the guest counter will not increment but the guest does
>>>> not know the counter is not implemented. Can't this lead to bad user
>>>> experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?
>>>
>>> The whole point is that we want to keep things hidden from the guest.
>>> Also, PMCEID{0,1} only describe a small set of events (the architected
>>> common events), and not the whole range of microarchitectural events
>>> that the CPU implements.
>>
>> I am still not totally convinced. Things are not totally hidden to the
>> guest as the counter does not increment, right? So a guest may try to
>> use as it is advertised in PMCEID0/1 but not get the expected results
>> leading to potential support request. I agree not all the events are
>> described there but your API also allows to filter out some of the ones
>> that are advertised.
> 
> I think we're at odds when it comes to the goal of this series. If you
> read the CPU TRM, you will find that event X is implemented. You look
> at PMCEIDx, and you find it is not. You still get a support request! ;-)
Yep that's a weird situation indeed, I haven't thought about the TRM.
> 
> Dropping events from these registers is totally trivial, but I'm not
> sure this will reduce the surprise effect. It doesn't hurt anyway, so
> I'll implement that.
Up to you. Or at least you can document it in the commit msg.

Thanks

Eric

> 
>>>
>>>>>
>>>>>      counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>>>>>
>>>>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>>>>>
>>>>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>>>  {
>>>>> -    if (!kvm_arm_support_pmu_v3())
>>>>> -        return -ENODEV;
>>>>> -
>>>>> -    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>>> -        return -ENXIO;
>>>>> -
>>>>> -    if (vcpu->arch.pmu.created)
>>>>> -        return -EBUSY;
>>>>> -
>>>>>      if (irqchip_in_kernel(vcpu->kvm)) {
>>>>>          int ret;
>>>>>
>>>>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm,
>>>>> int irq)
>>>>>      return true;
>>>>>  }
>>>>>
>>>>> +#define NR_EVENTS    (ARMV8_PMU_EVTYPE_EVENT + 1)
>>>>> +
>>>>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct
>>>>> kvm_device_attr *attr)
>>>>>  {
>>>>> +    if (!kvm_arm_support_pmu_v3())
>>>>> +        return -ENODEV;
>>>>> +
>>>>> +    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>>> +        return -ENODEV;
>>>> I see you changed -ENXIO into -ENODEV. wanted?
>>>
>>> Probably not... but see below.
>>>
>>>>> +
>>>>> +    if (vcpu->arch.pmu.created)
>>>>> +        return -EBUSY;
>>>>> +
>>>>>      switch (attr->attr) {
>>>>>      case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>>>>          int __user *uaddr = (int __user *)(long)attr->addr;
>>>>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>>>> *vcpu, struct kvm_device_attr *attr)
>>>>>          if (!irqchip_in_kernel(vcpu->kvm))
>>>>>              return -EINVAL;
>>>>>
>>>>> -        if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>>> -            return -ENODEV;
>>>>> -
>>>
>>> Here's why. I wonder if we already have a problem with the consistency
>>> of the
>>> error codes returned to userspace.
>> OK. Then you may document it in the commit message?
> 
> I still need to work out whether we actually have an issue on that.
> 
> [...]
> 
>>>> not related to this patch but shouldn't we advertise this only with
>>>> in-kernel irqchip?
>>>
>>> We do support the PMU without the in-kernel chip, unfortunately... Yes,
>>> supporting this feature was a big mistake.
>> But I see in kvm_arm_pmu_v3_set_attr:
>> case KVM_ARM_VCPU_PMU_V3_IRQ:
>> ../..
>>                 if (!irqchip_in_kernel(vcpu->kvm))
>>                         return -EINVAL;
> 
> Ah, I see what you mean. Yes, we probably shouldn't report that the PMU
> IRQ attribute is supported when we don't have an in-kernel irqchip.
> 
> Thanks,
> 
>         M.


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

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Auger Eric]

Hi Marc,

On 3/10/20 7:00 PM, Marc Zyngier wrote:
> On 2020-03-10 17:40, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/10/20 12:03 PM, Marc Zyngier wrote:
>>> Hi Eric,
>>>
>>> On 2020-03-09 18:05, Auger Eric wrote:
>>>> Hi Marc,
>>>>
>>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>>>> guest to be able to count some of the implemented events (because this
>>>>> would give information on shared resources, for example.
>>>>>
>>>>> For this, let's extend the PMUv3 device API, and offer a way to
>>>>> setup a
>>>>> bitmap of the allowed events (the default being no bitmap, and thus no
>>>>> filtering).
>>>>>
>>>>> Userspace can thus allow/deny ranges of event. The default policy
>>>>> depends on the "polarity" of the first filter setup (default deny
>>>>> if the
>>>>> filter allows events, and default allow if the filter denies events).
>>>>> This allows to setup exactly what is allowed for a given guest.
>>>>>
>>>>> Note that although the ioctl is per-vcpu, the map of allowed events is
>>>>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>>>>> initialized).
>>>>>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>> ---
>>>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>>>  virt/kvm/arm/arm.c                |  2 +
>>>>>  virt/kvm/arm/pmu.c                | 84
>>>>> +++++++++++++++++++++++++------
>>>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>>>> b/arch/arm64/include/asm/kvm_host.h
>>>>> index 57fd46acd058..8e63c618688d 100644
>>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>>>       * supported.
>>>>>       */
>>>>>      bool return_nisv_io_abort_to_user;
>>>>> +
>>>>> +    /*
>>>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>>>> +     * for up to 65536 events
>>>>> +     */
>>>>> +    unsigned long *pmu_filter;
>>>>>  };
>>>>>
>>>>>  #define KVM_NR_MEM_OBJS     40
>>>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>>>> b/arch/arm64/include/uapi/asm/kvm.h
>>>>> index ba85bb23f060..7b1511d6ce44 100644
>>>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>>>  struct kvm_arch_memory_slot {
>>>>>  };
>>>>>
>>>>> +/*
>>>>> + * PMU filter structure. Describe a range of events with a particular
>>>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>>>> + */
>>>>> +struct kvm_pmu_event_filter {
>>>>> +    __u16    base_event;
>>>>> +    __u16    nevents;
>>>>> +
>>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>>> +#define KVM_PMU_EVENT_DENY    1
>>>>> +
>>>>> +    __u8    action;
>>>>> +    __u8    pad[3];
>>>>> +};
>>>>> +
>>>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>>>  struct kvm_vcpu_events {
>>>>>      struct {
>>>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>>> index eda7b624eab8..8d849ac88a44 100644
>>>>> --- a/virt/kvm/arm/arm.c
>>>>> +++ b/virt/kvm/arm/arm.c
>>>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>>>      kvm->arch.last_vcpu_ran = NULL;
>>>>>
>>>>> +    bitmap_free(kvm->arch.pmu_filter);
>>>>> +
>>>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>>>          if (kvm->vcpus[i]) {
>>>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>>>> --- a/virt/kvm/arm/pmu.c
>>>>> +++ b/virt/kvm/arm/pmu.c
>>>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>>
>>>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>> nit:
>>>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>>     else
>>>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>
>>> You don't like it? ;-)
>> ? eventset set only once instead of 2 times
> 
> The compiler does the right thing, but sore, I'll change it.
> 
>>>
>>>>>
>>>>>      /* Software increment event does't need to be backed by a perf
>>>>> event */
>>>> nit: while wer are at it fix the does't typo
>>>>> -    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>>>>> -        pmc->idx != ARMV8_PMU_CYCLE_IDX)
>>>>> +    if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
>>>>> +        return;
>>>>> +
>>>>> +    /*
>>>>> +     * If we have a filter in place and that the event isn't
>>>>> allowed, do
>>>>> +     * not install a perf event either.
>>>>> +     */
>>>>> +    if (vcpu->kvm->arch.pmu_filter &&
>>>>> +        !test_bit(eventsel, vcpu->kvm->arch.pmu_filter))
>>>>>          return;
>>>>>
>>>>>      memset(&attr, 0, sizeof(struct perf_event_attr));
>>>>> @@ -594,8 +603,7 @@ static void kvm_pmu_create_perf_event(struct
>>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>>      attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
>>>>>      attr.exclude_hv = 1; /* Don't count EL2 events */
>>>>>      attr.exclude_host = 1; /* Don't count host events */
>>>>> -    attr.config = (pmc->idx == ARMV8_PMU_CYCLE_IDX) ?
>>>>> -        ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
>>>>> +    attr.config = eventsel;
>>>> So in that case the guest counter will not increment but the guest does
>>>> not know the counter is not implemented. Can't this lead to bad user
>>>> experience. Shouldn't this disablement be reflected in PMCEID0/1 regs?
>>>
>>> The whole point is that we want to keep things hidden from the guest.
>>> Also, PMCEID{0,1} only describe a small set of events (the architected
>>> common events), and not the whole range of microarchitectural events
>>> that the CPU implements.
>>
>> I am still not totally convinced. Things are not totally hidden to the
>> guest as the counter does not increment, right? So a guest may try to
>> use as it is advertised in PMCEID0/1 but not get the expected results
>> leading to potential support request. I agree not all the events are
>> described there but your API also allows to filter out some of the ones
>> that are advertised.
> 
> I think we're at odds when it comes to the goal of this series. If you
> read the CPU TRM, you will find that event X is implemented. You look
> at PMCEIDx, and you find it is not. You still get a support request! ;-)
Yep that's a weird situation indeed, I haven't thought about the TRM.
> 
> Dropping events from these registers is totally trivial, but I'm not
> sure this will reduce the surprise effect. It doesn't hurt anyway, so
> I'll implement that.
Up to you. Or at least you can document it in the commit msg.

Thanks

Eric

> 
>>>
>>>>>
>>>>>      counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);
>>>>>
>>>>> @@ -735,15 +743,6 @@ int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>>>>>
>>>>>  static int kvm_arm_pmu_v3_init(struct kvm_vcpu *vcpu)
>>>>>  {
>>>>> -    if (!kvm_arm_support_pmu_v3())
>>>>> -        return -ENODEV;
>>>>> -
>>>>> -    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>>> -        return -ENXIO;
>>>>> -
>>>>> -    if (vcpu->arch.pmu.created)
>>>>> -        return -EBUSY;
>>>>> -
>>>>>      if (irqchip_in_kernel(vcpu->kvm)) {
>>>>>          int ret;
>>>>>
>>>>> @@ -794,8 +793,19 @@ static bool pmu_irq_is_valid(struct kvm *kvm,
>>>>> int irq)
>>>>>      return true;
>>>>>  }
>>>>>
>>>>> +#define NR_EVENTS    (ARMV8_PMU_EVTYPE_EVENT + 1)
>>>>> +
>>>>>  int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct
>>>>> kvm_device_attr *attr)
>>>>>  {
>>>>> +    if (!kvm_arm_support_pmu_v3())
>>>>> +        return -ENODEV;
>>>>> +
>>>>> +    if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>>> +        return -ENODEV;
>>>> I see you changed -ENXIO into -ENODEV. wanted?
>>>
>>> Probably not... but see below.
>>>
>>>>> +
>>>>> +    if (vcpu->arch.pmu.created)
>>>>> +        return -EBUSY;
>>>>> +
>>>>>      switch (attr->attr) {
>>>>>      case KVM_ARM_VCPU_PMU_V3_IRQ: {
>>>>>          int __user *uaddr = (int __user *)(long)attr->addr;
>>>>> @@ -804,9 +814,6 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu
>>>>> *vcpu, struct kvm_device_attr *attr)
>>>>>          if (!irqchip_in_kernel(vcpu->kvm))
>>>>>              return -EINVAL;
>>>>>
>>>>> -        if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
>>>>> -            return -ENODEV;
>>>>> -
>>>
>>> Here's why. I wonder if we already have a problem with the consistency
>>> of the
>>> error codes returned to userspace.
>> OK. Then you may document it in the commit message?
> 
> I still need to work out whether we actually have an issue on that.
> 
> [...]
> 
>>>> not related to this patch but shouldn't we advertise this only with
>>>> in-kernel irqchip?
>>>
>>> We do support the PMU without the in-kernel chip, unfortunately... Yes,
>>> supporting this feature was a big mistake.
>> But I see in kvm_arm_pmu_v3_set_attr:
>> case KVM_ARM_VCPU_PMU_V3_IRQ:
>> ../..
>>                 if (!irqchip_in_kernel(vcpu->kvm))
>>                         return -EINVAL;
> 
> Ah, I see what you mean. Yes, we probably shouldn't report that the PMU
> IRQ attribute is supported when we don't have an in-kernel irqchip.
> 
> Thanks,
> 
>         M.

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Alexander Graf]

Hi Marc,

On 10.03.20 19:00, Marc Zyngier wrote:
> On 2020-03-10 17:40, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/10/20 12:03 PM, Marc Zyngier wrote:
>>> Hi Eric,
>>>
>>> On 2020-03-09 18:05, Auger Eric wrote:
>>>> Hi Marc,
>>>>
>>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>>>> guest to be able to count some of the implemented events (because this
>>>>> would give information on shared resources, for example.
>>>>>
>>>>> For this, let's extend the PMUv3 device API, and offer a way to 
>>>>> setup a
>>>>> bitmap of the allowed events (the default being no bitmap, and thus no
>>>>> filtering).
>>>>>
>>>>> Userspace can thus allow/deny ranges of event. The default policy
>>>>> depends on the "polarity" of the first filter setup (default deny 
>>>>> if the
>>>>> filter allows events, and default allow if the filter denies events).
>>>>> This allows to setup exactly what is allowed for a given guest.
>>>>>
>>>>> Note that although the ioctl is per-vcpu, the map of allowed events is
>>>>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>>>>> initialized).
>>>>>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>> ---
>>>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>>>  virt/kvm/arm/arm.c                |  2 +
>>>>>  virt/kvm/arm/pmu.c                | 84 
>>>>> +++++++++++++++++++++++++------
>>>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>>>> b/arch/arm64/include/asm/kvm_host.h
>>>>> index 57fd46acd058..8e63c618688d 100644
>>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>>>       * supported.
>>>>>       */
>>>>>      bool return_nisv_io_abort_to_user;
>>>>> +
>>>>> +    /*
>>>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>>>> +     * for up to 65536 events
>>>>> +     */
>>>>> +    unsigned long *pmu_filter;
>>>>>  };
>>>>>
>>>>>  #define KVM_NR_MEM_OBJS     40
>>>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>>>> b/arch/arm64/include/uapi/asm/kvm.h
>>>>> index ba85bb23f060..7b1511d6ce44 100644
>>>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>>>  struct kvm_arch_memory_slot {
>>>>>  };
>>>>>
>>>>> +/*
>>>>> + * PMU filter structure. Describe a range of events with a particular
>>>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>>>> + */
>>>>> +struct kvm_pmu_event_filter {
>>>>> +    __u16    base_event;
>>>>> +    __u16    nevents;
>>>>> +
>>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>>> +#define KVM_PMU_EVENT_DENY    1
>>>>> +
>>>>> +    __u8    action;
>>>>> +    __u8    pad[3];
>>>>> +};
>>>>> +
>>>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>>>  struct kvm_vcpu_events {
>>>>>      struct {
>>>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>>> index eda7b624eab8..8d849ac88a44 100644
>>>>> --- a/virt/kvm/arm/arm.c
>>>>> +++ b/virt/kvm/arm/arm.c
>>>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>>>      kvm->arch.last_vcpu_ran = NULL;
>>>>>
>>>>> +    bitmap_free(kvm->arch.pmu_filter);
>>>>> +
>>>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>>>          if (kvm->vcpus[i]) {
>>>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>>>> --- a/virt/kvm/arm/pmu.c
>>>>> +++ b/virt/kvm/arm/pmu.c
>>>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>>
>>>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>> nit:
>>>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>>     else
>>>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>
>>> You don't like it? ;-)
>> ? eventset set only once instead of 2 times
> 
> The compiler does the right thing, but sore, I'll change it.

I haven't seen a v3 follow-up after this. Do you happen to have that 
somewhere in a local branch and just need to send it out or would you 
prefer if I pick up v2 and address the comments?


Thanks,

Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Alexander Graf]

Hi Marc,

On 10.03.20 19:00, Marc Zyngier wrote:
> On 2020-03-10 17:40, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/10/20 12:03 PM, Marc Zyngier wrote:
>>> Hi Eric,
>>>
>>> On 2020-03-09 18:05, Auger Eric wrote:
>>>> Hi Marc,
>>>>
>>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>>>> guest to be able to count some of the implemented events (because this
>>>>> would give information on shared resources, for example.
>>>>>
>>>>> For this, let's extend the PMUv3 device API, and offer a way to 
>>>>> setup a
>>>>> bitmap of the allowed events (the default being no bitmap, and thus no
>>>>> filtering).
>>>>>
>>>>> Userspace can thus allow/deny ranges of event. The default policy
>>>>> depends on the "polarity" of the first filter setup (default deny 
>>>>> if the
>>>>> filter allows events, and default allow if the filter denies events).
>>>>> This allows to setup exactly what is allowed for a given guest.
>>>>>
>>>>> Note that although the ioctl is per-vcpu, the map of allowed events is
>>>>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>>>>> initialized).
>>>>>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>> ---
>>>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>>>  virt/kvm/arm/arm.c                |  2 +
>>>>>  virt/kvm/arm/pmu.c                | 84 
>>>>> +++++++++++++++++++++++++------
>>>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>>>> b/arch/arm64/include/asm/kvm_host.h
>>>>> index 57fd46acd058..8e63c618688d 100644
>>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>>>       * supported.
>>>>>       */
>>>>>      bool return_nisv_io_abort_to_user;
>>>>> +
>>>>> +    /*
>>>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>>>> +     * for up to 65536 events
>>>>> +     */
>>>>> +    unsigned long *pmu_filter;
>>>>>  };
>>>>>
>>>>>  #define KVM_NR_MEM_OBJS     40
>>>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>>>> b/arch/arm64/include/uapi/asm/kvm.h
>>>>> index ba85bb23f060..7b1511d6ce44 100644
>>>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>>>  struct kvm_arch_memory_slot {
>>>>>  };
>>>>>
>>>>> +/*
>>>>> + * PMU filter structure. Describe a range of events with a particular
>>>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>>>> + */
>>>>> +struct kvm_pmu_event_filter {
>>>>> +    __u16    base_event;
>>>>> +    __u16    nevents;
>>>>> +
>>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>>> +#define KVM_PMU_EVENT_DENY    1
>>>>> +
>>>>> +    __u8    action;
>>>>> +    __u8    pad[3];
>>>>> +};
>>>>> +
>>>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>>>  struct kvm_vcpu_events {
>>>>>      struct {
>>>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>>> index eda7b624eab8..8d849ac88a44 100644
>>>>> --- a/virt/kvm/arm/arm.c
>>>>> +++ b/virt/kvm/arm/arm.c
>>>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>>>      kvm->arch.last_vcpu_ran = NULL;
>>>>>
>>>>> +    bitmap_free(kvm->arch.pmu_filter);
>>>>> +
>>>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>>>          if (kvm->vcpus[i]) {
>>>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>>>> --- a/virt/kvm/arm/pmu.c
>>>>> +++ b/virt/kvm/arm/pmu.c
>>>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>>
>>>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>> nit:
>>>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>>     else
>>>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>
>>> You don't like it? ;-)
>> ? eventset set only once instead of 2 times
> 
> The compiler does the right thing, but sore, I'll change it.

I haven't seen a v3 follow-up after this. Do you happen to have that 
somewhere in a local branch and just need to send it out or would you 
prefer if I pick up v2 and address the comments?


Thanks,

Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


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

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Alexander Graf]

Hi Marc,

On 10.03.20 19:00, Marc Zyngier wrote:
> On 2020-03-10 17:40, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/10/20 12:03 PM, Marc Zyngier wrote:
>>> Hi Eric,
>>>
>>> On 2020-03-09 18:05, Auger Eric wrote:
>>>> Hi Marc,
>>>>
>>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>>> It can be desirable to expose a PMU to a guest, and yet not want the
>>>>> guest to be able to count some of the implemented events (because this
>>>>> would give information on shared resources, for example.
>>>>>
>>>>> For this, let's extend the PMUv3 device API, and offer a way to 
>>>>> setup a
>>>>> bitmap of the allowed events (the default being no bitmap, and thus no
>>>>> filtering).
>>>>>
>>>>> Userspace can thus allow/deny ranges of event. The default policy
>>>>> depends on the "polarity" of the first filter setup (default deny 
>>>>> if the
>>>>> filter allows events, and default allow if the filter denies events).
>>>>> This allows to setup exactly what is allowed for a given guest.
>>>>>
>>>>> Note that although the ioctl is per-vcpu, the map of allowed events is
>>>>> global to the VM (it can be setup from any vcpu until the vcpu PMU is
>>>>> initialized).
>>>>>
>>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>>> ---
>>>>>  arch/arm64/include/asm/kvm_host.h |  6 +++
>>>>>  arch/arm64/include/uapi/asm/kvm.h | 16 ++++++
>>>>>  virt/kvm/arm/arm.c                |  2 +
>>>>>  virt/kvm/arm/pmu.c                | 84 
>>>>> +++++++++++++++++++++++++------
>>>>>  4 files changed, 92 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>>>> b/arch/arm64/include/asm/kvm_host.h
>>>>> index 57fd46acd058..8e63c618688d 100644
>>>>> --- a/arch/arm64/include/asm/kvm_host.h
>>>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>>>> @@ -91,6 +91,12 @@ struct kvm_arch {
>>>>>       * supported.
>>>>>       */
>>>>>      bool return_nisv_io_abort_to_user;
>>>>> +
>>>>> +    /*
>>>>> +     * VM-wide PMU filter, implemented as a bitmap and big enough
>>>>> +     * for up to 65536 events
>>>>> +     */
>>>>> +    unsigned long *pmu_filter;
>>>>>  };
>>>>>
>>>>>  #define KVM_NR_MEM_OBJS     40
>>>>> diff --git a/arch/arm64/include/uapi/asm/kvm.h
>>>>> b/arch/arm64/include/uapi/asm/kvm.h
>>>>> index ba85bb23f060..7b1511d6ce44 100644
>>>>> --- a/arch/arm64/include/uapi/asm/kvm.h
>>>>> +++ b/arch/arm64/include/uapi/asm/kvm.h
>>>>> @@ -159,6 +159,21 @@ struct kvm_sync_regs {
>>>>>  struct kvm_arch_memory_slot {
>>>>>  };
>>>>>
>>>>> +/*
>>>>> + * PMU filter structure. Describe a range of events with a particular
>>>>> + * action. To be used with KVM_ARM_VCPU_PMU_V3_FILTER.
>>>>> + */
>>>>> +struct kvm_pmu_event_filter {
>>>>> +    __u16    base_event;
>>>>> +    __u16    nevents;
>>>>> +
>>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>>> +#define KVM_PMU_EVENT_DENY    1
>>>>> +
>>>>> +    __u8    action;
>>>>> +    __u8    pad[3];
>>>>> +};
>>>>> +
>>>>>  /* for KVM_GET/SET_VCPU_EVENTS */
>>>>>  struct kvm_vcpu_events {
>>>>>      struct {
>>>>> @@ -329,6 +344,7 @@ struct kvm_vcpu_events {
>>>>>  #define KVM_ARM_VCPU_PMU_V3_CTRL    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_IRQ    0
>>>>>  #define   KVM_ARM_VCPU_PMU_V3_INIT    1
>>>>> +#define   KVM_ARM_VCPU_PMU_V3_FILTER    2
>>>>>  #define KVM_ARM_VCPU_TIMER_CTRL        1
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_VTIMER        0
>>>>>  #define   KVM_ARM_VCPU_TIMER_IRQ_PTIMER        1
>>>>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>>>>> index eda7b624eab8..8d849ac88a44 100644
>>>>> --- a/virt/kvm/arm/arm.c
>>>>> +++ b/virt/kvm/arm/arm.c
>>>>> @@ -164,6 +164,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>>>      free_percpu(kvm->arch.last_vcpu_ran);
>>>>>      kvm->arch.last_vcpu_ran = NULL;
>>>>>
>>>>> +    bitmap_free(kvm->arch.pmu_filter);
>>>>> +
>>>>>      for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>>>>>          if (kvm->vcpus[i]) {
>>>>>              kvm_vcpu_destroy(kvm->vcpus[i]);
>>>>> diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
>>>>> index f0d0312c0a55..9f0fd0224d5b 100644
>>>>> --- a/virt/kvm/arm/pmu.c
>>>>> +++ b/virt/kvm/arm/pmu.c
>>>>> @@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
>>>>> kvm_vcpu *vcpu, u64 select_idx)
>>>>>
>>>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>>>>      eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>>> +    if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>> +        eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>> nit:
>>>>     if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
>>>>         eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
>>>>     else
>>>>         eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>>
>>> You don't like it? ;-)
>> ? eventset set only once instead of 2 times
> 
> The compiler does the right thing, but sore, I'll change it.

I haven't seen a v3 follow-up after this. Do you happen to have that 
somewhere in a local branch and just need to send it out or would you 
prefer if I pick up v2 and address the comments?


Thanks,

Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

On 2020-08-19 00:24, Alexander Graf wrote:
> Hi Marc,

[...]

> I haven't seen a v3 follow-up after this. Do you happen to have that
> somewhere in a local branch and just need to send it out or would you
> prefer if I pick up v2 and address the comments?

I'll look into it.

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

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

On 2020-08-19 00:24, Alexander Graf wrote:
> Hi Marc,

[...]

> I haven't seen a v3 follow-up after this. Do you happen to have that
> somewhere in a local branch and just need to send it out or would you
> prefer if I pick up v2 and address the comments?

I'll look into it.

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

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

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Marc Zyngier]

On 2020-08-19 00:24, Alexander Graf wrote:
> Hi Marc,

[...]

> I haven't seen a v3 follow-up after this. Do you happen to have that
> somewhere in a local branch and just need to send it out or would you
> prefer if I pick up v2 and address the comments?

I'll look into it.

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Alexander Graf]

On 20.08.20 09:37, Marc Zyngier wrote:
> 
> On 2020-08-19 00:24, Alexander Graf wrote:
>> Hi Marc,
> 
> [...]
> 
>> I haven't seen a v3 follow-up after this. Do you happen to have that
>> somewhere in a local branch and just need to send it out or would you
>> prefer if I pick up v2 and address the comments?
> 
> I'll look into it.

Thank you :)


Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Alexander Graf]

On 20.08.20 09:37, Marc Zyngier wrote:
> 
> On 2020-08-19 00:24, Alexander Graf wrote:
>> Hi Marc,
> 
> [...]
> 
>> I haven't seen a v3 follow-up after this. Do you happen to have that
>> somewhere in a local branch and just need to send it out or would you
>> prefer if I pick up v2 and address the comments?
> 
> I'll look into it.

Thank you :)


Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879




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

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[Alexander Graf]

On 20.08.20 09:37, Marc Zyngier wrote:
> 
> On 2020-08-19 00:24, Alexander Graf wrote:
>> Hi Marc,
> 
> [...]
> 
>> I haven't seen a v3 follow-up after this. Do you happen to have that
>> somewhere in a local branch and just need to send it out or would you
>> prefer if I pick up v2 and address the comments?
> 
> I'll look into it.

Thank you :)


Alex



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879



_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 1/2] KVM: arm64: Add PMU event filtering infrastructure

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2213 bytes --]

Hi Marc,

I love your patch! Yet something to improve:

[auto build test ERROR on v5.6-rc5]
[also build test ERROR on next-20200320]
[cannot apply to kvmarm/next arm/for-next xlnx/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Marc-Zyngier/KVM-arm64-Filtering-PMU-events/20200310-025319
base:    2c523b344dfa65a3738e7039832044aa133c75fb
config: arm-axm55xx_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=9.2.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   arch/arm/kvm/../../../virt/kvm/arm/arm.c: In function 'kvm_arch_destroy_vm':
>> arch/arm/kvm/../../../virt/kvm/arm/arm.c:167:23: error: 'struct kvm_arch' has no member named 'pmu_filter'
     167 |  bitmap_free(kvm->arch.pmu_filter);
         |                       ^

vim +167 arch/arm/kvm/../../../virt/kvm/arm/arm.c

   152	
   153	
   154	/**
   155	 * kvm_arch_destroy_vm - destroy the VM data structure
   156	 * @kvm:	pointer to the KVM struct
   157	 */
   158	void kvm_arch_destroy_vm(struct kvm *kvm)
   159	{
   160		int i;
   161	
   162		kvm_vgic_destroy(kvm);
   163	
   164		free_percpu(kvm->arch.last_vcpu_ran);
   165		kvm->arch.last_vcpu_ran = NULL;
   166	
 > 167		bitmap_free(kvm->arch.pmu_filter);
   168	
   169		for (i = 0; i < KVM_MAX_VCPUS; ++i) {
   170			if (kvm->vcpus[i]) {
   171				kvm_vcpu_destroy(kvm->vcpus[i]);
   172				kvm->vcpus[i] = NULL;
   173			}
   174		}
   175		atomic_set(&kvm->online_vcpus, 0);
   176	}
   177	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 21629 bytes --]

[PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Marc Zyngier]

Add a small blurb describing how the event filtering API gets used.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 Documentation/virt/kvm/devices/vcpu.rst | 40 +++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
index 9963e680770a..7262c0469856 100644
--- a/Documentation/virt/kvm/devices/vcpu.rst
+++ b/Documentation/virt/kvm/devices/vcpu.rst
@@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
 virtual GIC implementation, this must be done after initializing the in-kernel
 irqchip.
 
+1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
+---------------------------------------
+
+:Parameters: in kvm_device_attr.addr the address for a PMU event filter is a
+             pointer to a struct kvm_pmu_event_filter
+
+:Returns:
+
+	 =======  ======================================================
+	 -ENODEV: PMUv3 not supported or GIC not initialized
+	 -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
+	 	  configured as required prior to calling this attribute
+	 -EBUSY:  PMUv3 already initialized
+	 =======  ======================================================
+
+Request the installation of a PMU event filter describe as follows:
+
+struct kvm_pmu_event_filter {
+	__u16	base_event;
+	__u16	nevents;
+
+#define KVM_PMU_EVENT_ALLOW	0
+#define KVM_PMU_EVENT_DENY	1
+
+	__u8	action;
+	__u8	pad[3];
+};
+
+A filter range is defined as the range [@base_event, @base_event + @nevents[,
+together with an @action (KVM_PMU_EVENT_ALLOW or KVM_PMU_EVENT_DENY). The
+first registered range defines the global policy (global ALLOW if the first
+@action is DENY, global DENY if the first @action is ALLOW). Multiple ranges
+can be programmed, and must fit within the 16bit space defined by the ARMv8.1
+PMU architecture.
+
+Restrictions: Event 0 (SW_INCR) is never filtered, as it doesn't count a
+hardware event. Filtering event 0x1E (CHAIN) has no effect either, as it
+isn't strictly speaking an event. Filtering the cycle counter is possible
+using event 0x11 (CPU_CYCLES).
+
 
 2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
 =================================
-- 
2.20.1

[PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Marc Zyngier]

Add a small blurb describing how the event filtering API gets used.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 Documentation/virt/kvm/devices/vcpu.rst | 40 +++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
index 9963e680770a..7262c0469856 100644
--- a/Documentation/virt/kvm/devices/vcpu.rst
+++ b/Documentation/virt/kvm/devices/vcpu.rst
@@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
 virtual GIC implementation, this must be done after initializing the in-kernel
 irqchip.
 
+1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
+---------------------------------------
+
+:Parameters: in kvm_device_attr.addr the address for a PMU event filter is a
+             pointer to a struct kvm_pmu_event_filter
+
+:Returns:
+
+	 =======  ======================================================
+	 -ENODEV: PMUv3 not supported or GIC not initialized
+	 -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
+	 	  configured as required prior to calling this attribute
+	 -EBUSY:  PMUv3 already initialized
+	 =======  ======================================================
+
+Request the installation of a PMU event filter describe as follows:
+
+struct kvm_pmu_event_filter {
+	__u16	base_event;
+	__u16	nevents;
+
+#define KVM_PMU_EVENT_ALLOW	0
+#define KVM_PMU_EVENT_DENY	1
+
+	__u8	action;
+	__u8	pad[3];
+};
+
+A filter range is defined as the range [@base_event, @base_event + @nevents[,
+together with an @action (KVM_PMU_EVENT_ALLOW or KVM_PMU_EVENT_DENY). The
+first registered range defines the global policy (global ALLOW if the first
+@action is DENY, global DENY if the first @action is ALLOW). Multiple ranges
+can be programmed, and must fit within the 16bit space defined by the ARMv8.1
+PMU architecture.
+
+Restrictions: Event 0 (SW_INCR) is never filtered, as it doesn't count a
+hardware event. Filtering event 0x1E (CHAIN) has no effect either, as it
+isn't strictly speaking an event. Filtering the cycle counter is possible
+using event 0x11 (CPU_CYCLES).
+
 
 2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
 =================================
-- 
2.20.1


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

[PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Marc Zyngier]

Add a small blurb describing how the event filtering API gets used.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 Documentation/virt/kvm/devices/vcpu.rst | 40 +++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
index 9963e680770a..7262c0469856 100644
--- a/Documentation/virt/kvm/devices/vcpu.rst
+++ b/Documentation/virt/kvm/devices/vcpu.rst
@@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
 virtual GIC implementation, this must be done after initializing the in-kernel
 irqchip.
 
+1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
+---------------------------------------
+
+:Parameters: in kvm_device_attr.addr the address for a PMU event filter is a
+             pointer to a struct kvm_pmu_event_filter
+
+:Returns:
+
+	 =======  ======================================================
+	 -ENODEV: PMUv3 not supported or GIC not initialized
+	 -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
+	 	  configured as required prior to calling this attribute
+	 -EBUSY:  PMUv3 already initialized
+	 =======  ======================================================
+
+Request the installation of a PMU event filter describe as follows:
+
+struct kvm_pmu_event_filter {
+	__u16	base_event;
+	__u16	nevents;
+
+#define KVM_PMU_EVENT_ALLOW	0
+#define KVM_PMU_EVENT_DENY	1
+
+	__u8	action;
+	__u8	pad[3];
+};
+
+A filter range is defined as the range [@base_event, @base_event + @nevents[,
+together with an @action (KVM_PMU_EVENT_ALLOW or KVM_PMU_EVENT_DENY). The
+first registered range defines the global policy (global ALLOW if the first
+@action is DENY, global DENY if the first @action is ALLOW). Multiple ranges
+can be programmed, and must fit within the 16bit space defined by the ARMv8.1
+PMU architecture.
+
+Restrictions: Event 0 (SW_INCR) is never filtered, as it doesn't count a
+hardware event. Filtering event 0x1E (CHAIN) has no effect either, as it
+isn't strictly speaking an event. Filtering the cycle counter is possible
+using event 0x11 (CPU_CYCLES).
+
 
 2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
 =================================
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Auger Eric]

Hi Marc,

On 3/9/20 1:48 PM, Marc Zyngier wrote:
> Add a small blurb describing how the event filtering API gets used.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  Documentation/virt/kvm/devices/vcpu.rst | 40 +++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index 9963e680770a..7262c0469856 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
>  virtual GIC implementation, this must be done after initializing the in-kernel
>  irqchip.
>  
> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
> +---------------------------------------
> +
> +:Parameters: in kvm_device_attr.addr the address for a PMU event filter is a
> +             pointer to a struct kvm_pmu_event_filter
> +
> +:Returns:
> +
> +	 =======  ======================================================
> +	 -ENODEV: PMUv3 not supported or GIC not initialized
> +	 -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
> +	 	  configured as required prior to calling this attribute
> +	 -EBUSY:  PMUv3 already initialized
maybe document -EINVAL?
> +	 =======  ======================================================
> +
> +Request the installation of a PMU event filter describe as follows:
s/describe/described
> +
> +struct kvm_pmu_event_filter {
> +	__u16	base_event;
> +	__u16	nevents;
> +
> +#define KVM_PMU_EVENT_ALLOW	0
> +#define KVM_PMU_EVENT_DENY	1
> +
> +	__u8	action;
> +	__u8	pad[3];
> +};
> +
> +A filter range is defined as the range [@base_event, @base_event + @nevents[,
> +together with an @action (KVM_PMU_EVENT_ALLOW or KVM_PMU_EVENT_DENY). The
> +first registered range defines the global policy (global ALLOW if the first
> +@action is DENY, global DENY if the first @action is ALLOW). Multiple ranges
> +can be programmed, and must fit within the 16bit space defined by the ARMv8.1
> +PMU architecture.
what about before 8.1 where the range was 10 bits? Should it be tested
in the code?

nitpicking: It is not totally obvious what does happen if the user space
sets a deny filter on a range and then an allow filter on the same
range. it is supported but may be worth telling so? Also explain the the
default filtering remains "allow" by default?
> +
> +Restrictions: Event 0 (SW_INCR) is never filtered, as it doesn't count a
> +hardware event. Filtering event 0x1E (CHAIN) has no effect either, as it
> +isn't strictly speaking an event. Filtering the cycle counter is possible
> +using event 0x11 (CPU_CYCLES).
Thanks

Eric
> +
>  
>  2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
>  =================================
> 

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Auger Eric]

Hi Marc,

On 3/9/20 1:48 PM, Marc Zyngier wrote:
> Add a small blurb describing how the event filtering API gets used.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  Documentation/virt/kvm/devices/vcpu.rst | 40 +++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index 9963e680770a..7262c0469856 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
>  virtual GIC implementation, this must be done after initializing the in-kernel
>  irqchip.
>  
> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
> +---------------------------------------
> +
> +:Parameters: in kvm_device_attr.addr the address for a PMU event filter is a
> +             pointer to a struct kvm_pmu_event_filter
> +
> +:Returns:
> +
> +	 =======  ======================================================
> +	 -ENODEV: PMUv3 not supported or GIC not initialized
> +	 -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
> +	 	  configured as required prior to calling this attribute
> +	 -EBUSY:  PMUv3 already initialized
maybe document -EINVAL?
> +	 =======  ======================================================
> +
> +Request the installation of a PMU event filter describe as follows:
s/describe/described
> +
> +struct kvm_pmu_event_filter {
> +	__u16	base_event;
> +	__u16	nevents;
> +
> +#define KVM_PMU_EVENT_ALLOW	0
> +#define KVM_PMU_EVENT_DENY	1
> +
> +	__u8	action;
> +	__u8	pad[3];
> +};
> +
> +A filter range is defined as the range [@base_event, @base_event + @nevents[,
> +together with an @action (KVM_PMU_EVENT_ALLOW or KVM_PMU_EVENT_DENY). The
> +first registered range defines the global policy (global ALLOW if the first
> +@action is DENY, global DENY if the first @action is ALLOW). Multiple ranges
> +can be programmed, and must fit within the 16bit space defined by the ARMv8.1
> +PMU architecture.
what about before 8.1 where the range was 10 bits? Should it be tested
in the code?

nitpicking: It is not totally obvious what does happen if the user space
sets a deny filter on a range and then an allow filter on the same
range. it is supported but may be worth telling so? Also explain the the
default filtering remains "allow" by default?
> +
> +Restrictions: Event 0 (SW_INCR) is never filtered, as it doesn't count a
> +hardware event. Filtering event 0x1E (CHAIN) has no effect either, as it
> +isn't strictly speaking an event. Filtering the cycle counter is possible
> +using event 0x11 (CPU_CYCLES).
Thanks

Eric
> +
>  
>  2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
>  =================================
> 


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

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Auger Eric]

Hi Marc,

On 3/9/20 1:48 PM, Marc Zyngier wrote:
> Add a small blurb describing how the event filtering API gets used.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  Documentation/virt/kvm/devices/vcpu.rst | 40 +++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index 9963e680770a..7262c0469856 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using the PMUv3 with an in-kernel
>  virtual GIC implementation, this must be done after initializing the in-kernel
>  irqchip.
>  
> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
> +---------------------------------------
> +
> +:Parameters: in kvm_device_attr.addr the address for a PMU event filter is a
> +             pointer to a struct kvm_pmu_event_filter
> +
> +:Returns:
> +
> +	 =======  ======================================================
> +	 -ENODEV: PMUv3 not supported or GIC not initialized
> +	 -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
> +	 	  configured as required prior to calling this attribute
> +	 -EBUSY:  PMUv3 already initialized
maybe document -EINVAL?
> +	 =======  ======================================================
> +
> +Request the installation of a PMU event filter describe as follows:
s/describe/described
> +
> +struct kvm_pmu_event_filter {
> +	__u16	base_event;
> +	__u16	nevents;
> +
> +#define KVM_PMU_EVENT_ALLOW	0
> +#define KVM_PMU_EVENT_DENY	1
> +
> +	__u8	action;
> +	__u8	pad[3];
> +};
> +
> +A filter range is defined as the range [@base_event, @base_event + @nevents[,
> +together with an @action (KVM_PMU_EVENT_ALLOW or KVM_PMU_EVENT_DENY). The
> +first registered range defines the global policy (global ALLOW if the first
> +@action is DENY, global DENY if the first @action is ALLOW). Multiple ranges
> +can be programmed, and must fit within the 16bit space defined by the ARMv8.1
> +PMU architecture.
what about before 8.1 where the range was 10 bits? Should it be tested
in the code?

nitpicking: It is not totally obvious what does happen if the user space
sets a deny filter on a range and then an allow filter on the same
range. it is supported but may be worth telling so? Also explain the the
default filtering remains "allow" by default?
> +
> +Restrictions: Event 0 (SW_INCR) is never filtered, as it doesn't count a
> +hardware event. Filtering event 0x1E (CHAIN) has no effect either, as it
> +isn't strictly speaking an event. Filtering the cycle counter is possible
> +using event 0x11 (CPU_CYCLES).
Thanks

Eric
> +
>  
>  2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
>  =================================
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Marc Zyngier]

On 2020-03-09 18:17, Auger Eric wrote:
> Hi Marc,
> 
> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>> Add a small blurb describing how the event filtering API gets used.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  Documentation/virt/kvm/devices/vcpu.rst | 40 
>> +++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>> 
>> diff --git a/Documentation/virt/kvm/devices/vcpu.rst 
>> b/Documentation/virt/kvm/devices/vcpu.rst
>> index 9963e680770a..7262c0469856 100644
>> --- a/Documentation/virt/kvm/devices/vcpu.rst
>> +++ b/Documentation/virt/kvm/devices/vcpu.rst
>> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using 
>> the PMUv3 with an in-kernel
>>  virtual GIC implementation, this must be done after initializing the 
>> in-kernel
>>  irqchip.
>> 
>> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
>> +---------------------------------------
>> +
>> +:Parameters: in kvm_device_attr.addr the address for a PMU event 
>> filter is a
>> +             pointer to a struct kvm_pmu_event_filter
>> +
>> +:Returns:
>> +
>> +	 =======  ======================================================
>> +	 -ENODEV: PMUv3 not supported or GIC not initialized
>> +	 -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
>> +	 	  configured as required prior to calling this attribute
>> +	 -EBUSY:  PMUv3 already initialized
> maybe document -EINVAL?

Yup, definitely.

>> +	 =======  ======================================================
>> +
>> +Request the installation of a PMU event filter describe as follows:
> s/describe/described
>> +
>> +struct kvm_pmu_event_filter {
>> +	__u16	base_event;
>> +	__u16	nevents;
>> +
>> +#define KVM_PMU_EVENT_ALLOW	0
>> +#define KVM_PMU_EVENT_DENY	1
>> +
>> +	__u8	action;
>> +	__u8	pad[3];
>> +};
>> +
>> +A filter range is defined as the range [@base_event, @base_event + 
>> @nevents[,
>> +together with an @action (KVM_PMU_EVENT_ALLOW or KVM_PMU_EVENT_DENY). 
>> The
>> +first registered range defines the global policy (global ALLOW if the 
>> first
>> +@action is DENY, global DENY if the first @action is ALLOW). Multiple 
>> ranges
>> +can be programmed, and must fit within the 16bit space defined by the 
>> ARMv8.1
>> +PMU architecture.
> what about before 8.1 where the range was 10 bits? Should it be tested
> in the code?

It's a good point. We could test that upon installing the filter and 
limit
the bitmap allocation to the minimum.

> nitpicking: It is not totally obvious what does happen if the user 
> space
> sets a deny filter on a range and then an allow filter on the same
> range. it is supported but may be worth telling so? Also explain the 
> the
> default filtering remains "allow" by default?

Overlapping filters are easy: the last one wins. And yes, no filter 
means
just that: no filter.

Thanks,

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

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Marc Zyngier]

On 2020-03-09 18:17, Auger Eric wrote:
> Hi Marc,
> 
> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>> Add a small blurb describing how the event filtering API gets used.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  Documentation/virt/kvm/devices/vcpu.rst | 40 
>> +++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>> 
>> diff --git a/Documentation/virt/kvm/devices/vcpu.rst 
>> b/Documentation/virt/kvm/devices/vcpu.rst
>> index 9963e680770a..7262c0469856 100644
>> --- a/Documentation/virt/kvm/devices/vcpu.rst
>> +++ b/Documentation/virt/kvm/devices/vcpu.rst
>> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using 
>> the PMUv3 with an in-kernel
>>  virtual GIC implementation, this must be done after initializing the 
>> in-kernel
>>  irqchip.
>> 
>> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
>> +---------------------------------------
>> +
>> +:Parameters: in kvm_device_attr.addr the address for a PMU event 
>> filter is a
>> +             pointer to a struct kvm_pmu_event_filter
>> +
>> +:Returns:
>> +
>> +	 =======  ======================================================
>> +	 -ENODEV: PMUv3 not supported or GIC not initialized
>> +	 -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
>> +	 	  configured as required prior to calling this attribute
>> +	 -EBUSY:  PMUv3 already initialized
> maybe document -EINVAL?

Yup, definitely.

>> +	 =======  ======================================================
>> +
>> +Request the installation of a PMU event filter describe as follows:
> s/describe/described
>> +
>> +struct kvm_pmu_event_filter {
>> +	__u16	base_event;
>> +	__u16	nevents;
>> +
>> +#define KVM_PMU_EVENT_ALLOW	0
>> +#define KVM_PMU_EVENT_DENY	1
>> +
>> +	__u8	action;
>> +	__u8	pad[3];
>> +};
>> +
>> +A filter range is defined as the range [@base_event, @base_event + 
>> @nevents[,
>> +together with an @action (KVM_PMU_EVENT_ALLOW or KVM_PMU_EVENT_DENY). 
>> The
>> +first registered range defines the global policy (global ALLOW if the 
>> first
>> +@action is DENY, global DENY if the first @action is ALLOW). Multiple 
>> ranges
>> +can be programmed, and must fit within the 16bit space defined by the 
>> ARMv8.1
>> +PMU architecture.
> what about before 8.1 where the range was 10 bits? Should it be tested
> in the code?

It's a good point. We could test that upon installing the filter and 
limit
the bitmap allocation to the minimum.

> nitpicking: It is not totally obvious what does happen if the user 
> space
> sets a deny filter on a range and then an allow filter on the same
> range. it is supported but may be worth telling so? Also explain the 
> the
> default filtering remains "allow" by default?

Overlapping filters are easy: the last one wins. And yes, no filter 
means
just that: no filter.

Thanks,

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

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

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Marc Zyngier]

On 2020-03-09 18:17, Auger Eric wrote:
> Hi Marc,
> 
> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>> Add a small blurb describing how the event filtering API gets used.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  Documentation/virt/kvm/devices/vcpu.rst | 40 
>> +++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>> 
>> diff --git a/Documentation/virt/kvm/devices/vcpu.rst 
>> b/Documentation/virt/kvm/devices/vcpu.rst
>> index 9963e680770a..7262c0469856 100644
>> --- a/Documentation/virt/kvm/devices/vcpu.rst
>> +++ b/Documentation/virt/kvm/devices/vcpu.rst
>> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using 
>> the PMUv3 with an in-kernel
>>  virtual GIC implementation, this must be done after initializing the 
>> in-kernel
>>  irqchip.
>> 
>> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
>> +---------------------------------------
>> +
>> +:Parameters: in kvm_device_attr.addr the address for a PMU event 
>> filter is a
>> +             pointer to a struct kvm_pmu_event_filter
>> +
>> +:Returns:
>> +
>> +	 =======  ======================================================
>> +	 -ENODEV: PMUv3 not supported or GIC not initialized
>> +	 -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
>> +	 	  configured as required prior to calling this attribute
>> +	 -EBUSY:  PMUv3 already initialized
> maybe document -EINVAL?

Yup, definitely.

>> +	 =======  ======================================================
>> +
>> +Request the installation of a PMU event filter describe as follows:
> s/describe/described
>> +
>> +struct kvm_pmu_event_filter {
>> +	__u16	base_event;
>> +	__u16	nevents;
>> +
>> +#define KVM_PMU_EVENT_ALLOW	0
>> +#define KVM_PMU_EVENT_DENY	1
>> +
>> +	__u8	action;
>> +	__u8	pad[3];
>> +};
>> +
>> +A filter range is defined as the range [@base_event, @base_event + 
>> @nevents[,
>> +together with an @action (KVM_PMU_EVENT_ALLOW or KVM_PMU_EVENT_DENY). 
>> The
>> +first registered range defines the global policy (global ALLOW if the 
>> first
>> +@action is DENY, global DENY if the first @action is ALLOW). Multiple 
>> ranges
>> +can be programmed, and must fit within the 16bit space defined by the 
>> ARMv8.1
>> +PMU architecture.
> what about before 8.1 where the range was 10 bits? Should it be tested
> in the code?

It's a good point. We could test that upon installing the filter and 
limit
the bitmap allocation to the minimum.

> nitpicking: It is not totally obvious what does happen if the user 
> space
> sets a deny filter on a range and then an allow filter on the same
> range. it is supported but may be worth telling so? Also explain the 
> the
> default filtering remains "allow" by default?

Overlapping filters are easy: the last one wins. And yes, no filter 
means
just that: no filter.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Auger Eric]

Hi Marc,

On 3/10/20 12:54 PM, Marc Zyngier wrote:
> On 2020-03-09 18:17, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>> Add a small blurb describing how the event filtering API gets used.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  Documentation/virt/kvm/devices/vcpu.rst | 40 +++++++++++++++++++++++++
>>>  1 file changed, 40 insertions(+)
>>>
>>> diff --git a/Documentation/virt/kvm/devices/vcpu.rst
>>> b/Documentation/virt/kvm/devices/vcpu.rst
>>> index 9963e680770a..7262c0469856 100644
>>> --- a/Documentation/virt/kvm/devices/vcpu.rst
>>> +++ b/Documentation/virt/kvm/devices/vcpu.rst
>>> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using
>>> the PMUv3 with an in-kernel
>>>  virtual GIC implementation, this must be done after initializing the
>>> in-kernel
>>>  irqchip.
>>>
>>> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
>>> +---------------------------------------
>>> +
>>> +:Parameters: in kvm_device_attr.addr the address for a PMU event
>>> filter is a
>>> +             pointer to a struct kvm_pmu_event_filter
>>> +
>>> +:Returns:
>>> +
>>> +     =======  ======================================================
>>> +     -ENODEV: PMUv3 not supported or GIC not initialized
>>> +     -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
>>> +           configured as required prior to calling this attribute
>>> +     -EBUSY:  PMUv3 already initialized
>> maybe document -EINVAL?
> 
> Yup, definitely.
> 
>>> +     =======  ======================================================
>>> +
>>> +Request the installation of a PMU event filter describe as follows:
>> s/describe/described
>>> +
>>> +struct kvm_pmu_event_filter {
>>> +    __u16    base_event;
>>> +    __u16    nevents;
>>> +
>>> +#define KVM_PMU_EVENT_ALLOW    0
>>> +#define KVM_PMU_EVENT_DENY    1
>>> +
>>> +    __u8    action;
>>> +    __u8    pad[3];
>>> +};
>>> +
>>> +A filter range is defined as the range [@base_event, @base_event +
>>> @nevents[,
>>> +together with an @action (KVM_PMU_EVENT_ALLOW or
>>> KVM_PMU_EVENT_DENY). The
>>> +first registered range defines the global policy (global ALLOW if
>>> the first
>>> +@action is DENY, global DENY if the first @action is ALLOW).
>>> Multiple ranges
>>> +can be programmed, and must fit within the 16bit space defined by
>>> the ARMv8.1
>>> +PMU architecture.
>> what about before 8.1 where the range was 10 bits? Should it be tested
>> in the code?
> 
> It's a good point. We could test that upon installing the filter and limit
> the bitmap allocation to the minimum.
> 
>> nitpicking: It is not totally obvious what does happen if the user space
>> sets a deny filter on a range and then an allow filter on the same
>> range. it is supported but may be worth telling so? Also explain the the
>> default filtering remains "allow" by default?
> 
> Overlapping filters are easy: the last one wins. And yes, no filter means
> just that: no filter.
Actually the point I wanted to put forward is
1) set allow filter on range [0-a] -> default setting is deny and allow
[0-a] only
2) deny deny filter on rang [0-a] -> there is no "real" active filtering
anymore but default behavior still is deny. ie. you do not destroy the
bitmap on the last filter removal but on the VM removal.

Thanks

Eric

> 
> Thanks,
> 
>         M.

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Auger Eric]

Hi Marc,

On 3/10/20 12:54 PM, Marc Zyngier wrote:
> On 2020-03-09 18:17, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>> Add a small blurb describing how the event filtering API gets used.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  Documentation/virt/kvm/devices/vcpu.rst | 40 +++++++++++++++++++++++++
>>>  1 file changed, 40 insertions(+)
>>>
>>> diff --git a/Documentation/virt/kvm/devices/vcpu.rst
>>> b/Documentation/virt/kvm/devices/vcpu.rst
>>> index 9963e680770a..7262c0469856 100644
>>> --- a/Documentation/virt/kvm/devices/vcpu.rst
>>> +++ b/Documentation/virt/kvm/devices/vcpu.rst
>>> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using
>>> the PMUv3 with an in-kernel
>>>  virtual GIC implementation, this must be done after initializing the
>>> in-kernel
>>>  irqchip.
>>>
>>> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
>>> +---------------------------------------
>>> +
>>> +:Parameters: in kvm_device_attr.addr the address for a PMU event
>>> filter is a
>>> +             pointer to a struct kvm_pmu_event_filter
>>> +
>>> +:Returns:
>>> +
>>> +     =======  ======================================================
>>> +     -ENODEV: PMUv3 not supported or GIC not initialized
>>> +     -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
>>> +           configured as required prior to calling this attribute
>>> +     -EBUSY:  PMUv3 already initialized
>> maybe document -EINVAL?
> 
> Yup, definitely.
> 
>>> +     =======  ======================================================
>>> +
>>> +Request the installation of a PMU event filter describe as follows:
>> s/describe/described
>>> +
>>> +struct kvm_pmu_event_filter {
>>> +    __u16    base_event;
>>> +    __u16    nevents;
>>> +
>>> +#define KVM_PMU_EVENT_ALLOW    0
>>> +#define KVM_PMU_EVENT_DENY    1
>>> +
>>> +    __u8    action;
>>> +    __u8    pad[3];
>>> +};
>>> +
>>> +A filter range is defined as the range [@base_event, @base_event +
>>> @nevents[,
>>> +together with an @action (KVM_PMU_EVENT_ALLOW or
>>> KVM_PMU_EVENT_DENY). The
>>> +first registered range defines the global policy (global ALLOW if
>>> the first
>>> +@action is DENY, global DENY if the first @action is ALLOW).
>>> Multiple ranges
>>> +can be programmed, and must fit within the 16bit space defined by
>>> the ARMv8.1
>>> +PMU architecture.
>> what about before 8.1 where the range was 10 bits? Should it be tested
>> in the code?
> 
> It's a good point. We could test that upon installing the filter and limit
> the bitmap allocation to the minimum.
> 
>> nitpicking: It is not totally obvious what does happen if the user space
>> sets a deny filter on a range and then an allow filter on the same
>> range. it is supported but may be worth telling so? Also explain the the
>> default filtering remains "allow" by default?
> 
> Overlapping filters are easy: the last one wins. And yes, no filter means
> just that: no filter.
Actually the point I wanted to put forward is
1) set allow filter on range [0-a] -> default setting is deny and allow
[0-a] only
2) deny deny filter on rang [0-a] -> there is no "real" active filtering
anymore but default behavior still is deny. ie. you do not destroy the
bitmap on the last filter removal but on the VM removal.

Thanks

Eric

> 
> Thanks,
> 
>         M.


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

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Auger Eric]

Hi Marc,

On 3/10/20 12:54 PM, Marc Zyngier wrote:
> On 2020-03-09 18:17, Auger Eric wrote:
>> Hi Marc,
>>
>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>> Add a small blurb describing how the event filtering API gets used.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  Documentation/virt/kvm/devices/vcpu.rst | 40 +++++++++++++++++++++++++
>>>  1 file changed, 40 insertions(+)
>>>
>>> diff --git a/Documentation/virt/kvm/devices/vcpu.rst
>>> b/Documentation/virt/kvm/devices/vcpu.rst
>>> index 9963e680770a..7262c0469856 100644
>>> --- a/Documentation/virt/kvm/devices/vcpu.rst
>>> +++ b/Documentation/virt/kvm/devices/vcpu.rst
>>> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If using
>>> the PMUv3 with an in-kernel
>>>  virtual GIC implementation, this must be done after initializing the
>>> in-kernel
>>>  irqchip.
>>>
>>> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
>>> +---------------------------------------
>>> +
>>> +:Parameters: in kvm_device_attr.addr the address for a PMU event
>>> filter is a
>>> +             pointer to a struct kvm_pmu_event_filter
>>> +
>>> +:Returns:
>>> +
>>> +     =======  ======================================================
>>> +     -ENODEV: PMUv3 not supported or GIC not initialized
>>> +     -ENXIO:  PMUv3 not properly configured or in-kernel irqchip not
>>> +           configured as required prior to calling this attribute
>>> +     -EBUSY:  PMUv3 already initialized
>> maybe document -EINVAL?
> 
> Yup, definitely.
> 
>>> +     =======  ======================================================
>>> +
>>> +Request the installation of a PMU event filter describe as follows:
>> s/describe/described
>>> +
>>> +struct kvm_pmu_event_filter {
>>> +    __u16    base_event;
>>> +    __u16    nevents;
>>> +
>>> +#define KVM_PMU_EVENT_ALLOW    0
>>> +#define KVM_PMU_EVENT_DENY    1
>>> +
>>> +    __u8    action;
>>> +    __u8    pad[3];
>>> +};
>>> +
>>> +A filter range is defined as the range [@base_event, @base_event +
>>> @nevents[,
>>> +together with an @action (KVM_PMU_EVENT_ALLOW or
>>> KVM_PMU_EVENT_DENY). The
>>> +first registered range defines the global policy (global ALLOW if
>>> the first
>>> +@action is DENY, global DENY if the first @action is ALLOW).
>>> Multiple ranges
>>> +can be programmed, and must fit within the 16bit space defined by
>>> the ARMv8.1
>>> +PMU architecture.
>> what about before 8.1 where the range was 10 bits? Should it be tested
>> in the code?
> 
> It's a good point. We could test that upon installing the filter and limit
> the bitmap allocation to the minimum.
> 
>> nitpicking: It is not totally obvious what does happen if the user space
>> sets a deny filter on a range and then an allow filter on the same
>> range. it is supported but may be worth telling so? Also explain the the
>> default filtering remains "allow" by default?
> 
> Overlapping filters are easy: the last one wins. And yes, no filter means
> just that: no filter.
Actually the point I wanted to put forward is
1) set allow filter on range [0-a] -> default setting is deny and allow
[0-a] only
2) deny deny filter on rang [0-a] -> there is no "real" active filtering
anymore but default behavior still is deny. ie. you do not destroy the
bitmap on the last filter removal but on the VM removal.

Thanks

Eric

> 
> Thanks,
> 
>         M.

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Marc Zyngier]

On 2020-03-10 17:30, Auger Eric wrote:
> Hi Marc,
> 
> On 3/10/20 12:54 PM, Marc Zyngier wrote:
>> On 2020-03-09 18:17, Auger Eric wrote:
>>> Hi Marc,
>>> 
>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>> Add a small blurb describing how the event filtering API gets used.
>>>> 
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> ---
>>>>  Documentation/virt/kvm/devices/vcpu.rst | 40 
>>>> +++++++++++++++++++++++++
>>>>  1 file changed, 40 insertions(+)
>>>> 
>>>> diff --git a/Documentation/virt/kvm/devices/vcpu.rst
>>>> b/Documentation/virt/kvm/devices/vcpu.rst
>>>> index 9963e680770a..7262c0469856 100644
>>>> --- a/Documentation/virt/kvm/devices/vcpu.rst
>>>> +++ b/Documentation/virt/kvm/devices/vcpu.rst
>>>> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If 
>>>> using
>>>> the PMUv3 with an in-kernel
>>>>  virtual GIC implementation, this must be done after initializing 
>>>> the
>>>> in-kernel
>>>>  irqchip.
>>>> 
>>>> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
>>>> +---------------------------------------
>>>> +
>>>> +:Parameters: in kvm_device_attr.addr the address for a PMU event
>>>> filter is a
>>>> +             pointer to a struct kvm_pmu_event_filter
>>>> +
>>>> +:Returns:
>>>> +
>>>> +     =======  
>>>> ======================================================
>>>> +     -ENODEV: PMUv3 not supported or GIC not initialized
>>>> +     -ENXIO:  PMUv3 not properly configured or in-kernel irqchip 
>>>> not
>>>> +           configured as required prior to calling this attribute
>>>> +     -EBUSY:  PMUv3 already initialized
>>> maybe document -EINVAL?
>> 
>> Yup, definitely.
>> 
>>>> +     =======  
>>>> ======================================================
>>>> +
>>>> +Request the installation of a PMU event filter describe as follows:
>>> s/describe/described
>>>> +
>>>> +struct kvm_pmu_event_filter {
>>>> +    __u16    base_event;
>>>> +    __u16    nevents;
>>>> +
>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>> +#define KVM_PMU_EVENT_DENY    1
>>>> +
>>>> +    __u8    action;
>>>> +    __u8    pad[3];
>>>> +};
>>>> +
>>>> +A filter range is defined as the range [@base_event, @base_event +
>>>> @nevents[,
>>>> +together with an @action (KVM_PMU_EVENT_ALLOW or
>>>> KVM_PMU_EVENT_DENY). The
>>>> +first registered range defines the global policy (global ALLOW if
>>>> the first
>>>> +@action is DENY, global DENY if the first @action is ALLOW).
>>>> Multiple ranges
>>>> +can be programmed, and must fit within the 16bit space defined by
>>>> the ARMv8.1
>>>> +PMU architecture.
>>> what about before 8.1 where the range was 10 bits? Should it be 
>>> tested
>>> in the code?
>> 
>> It's a good point. We could test that upon installing the filter and 
>> limit
>> the bitmap allocation to the minimum.
>> 
>>> nitpicking: It is not totally obvious what does happen if the user 
>>> space
>>> sets a deny filter on a range and then an allow filter on the same
>>> range. it is supported but may be worth telling so? Also explain the 
>>> the
>>> default filtering remains "allow" by default?
>> 
>> Overlapping filters are easy: the last one wins. And yes, no filter 
>> means
>> just that: no filter.
> Actually the point I wanted to put forward is
> 1) set allow filter on range [0-a] -> default setting is deny and allow
> [0-a] only
> 2) deny deny filter on rang [0-a] -> there is no "real" active 
> filtering
> anymore but default behavior still is deny. ie. you do not destroy the
> bitmap on the last filter removal but on the VM removal.

Ah, gotcha. Yes, this is odd. The solution to this is to re-apply a 
default
behaviour. But this needs documenting...

Thanks,

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

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Marc Zyngier]

On 2020-03-10 17:30, Auger Eric wrote:
> Hi Marc,
> 
> On 3/10/20 12:54 PM, Marc Zyngier wrote:
>> On 2020-03-09 18:17, Auger Eric wrote:
>>> Hi Marc,
>>> 
>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>> Add a small blurb describing how the event filtering API gets used.
>>>> 
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> ---
>>>>  Documentation/virt/kvm/devices/vcpu.rst | 40 
>>>> +++++++++++++++++++++++++
>>>>  1 file changed, 40 insertions(+)
>>>> 
>>>> diff --git a/Documentation/virt/kvm/devices/vcpu.rst
>>>> b/Documentation/virt/kvm/devices/vcpu.rst
>>>> index 9963e680770a..7262c0469856 100644
>>>> --- a/Documentation/virt/kvm/devices/vcpu.rst
>>>> +++ b/Documentation/virt/kvm/devices/vcpu.rst
>>>> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If 
>>>> using
>>>> the PMUv3 with an in-kernel
>>>>  virtual GIC implementation, this must be done after initializing 
>>>> the
>>>> in-kernel
>>>>  irqchip.
>>>> 
>>>> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
>>>> +---------------------------------------
>>>> +
>>>> +:Parameters: in kvm_device_attr.addr the address for a PMU event
>>>> filter is a
>>>> +             pointer to a struct kvm_pmu_event_filter
>>>> +
>>>> +:Returns:
>>>> +
>>>> +     =======  
>>>> ======================================================
>>>> +     -ENODEV: PMUv3 not supported or GIC not initialized
>>>> +     -ENXIO:  PMUv3 not properly configured or in-kernel irqchip 
>>>> not
>>>> +           configured as required prior to calling this attribute
>>>> +     -EBUSY:  PMUv3 already initialized
>>> maybe document -EINVAL?
>> 
>> Yup, definitely.
>> 
>>>> +     =======  
>>>> ======================================================
>>>> +
>>>> +Request the installation of a PMU event filter describe as follows:
>>> s/describe/described
>>>> +
>>>> +struct kvm_pmu_event_filter {
>>>> +    __u16    base_event;
>>>> +    __u16    nevents;
>>>> +
>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>> +#define KVM_PMU_EVENT_DENY    1
>>>> +
>>>> +    __u8    action;
>>>> +    __u8    pad[3];
>>>> +};
>>>> +
>>>> +A filter range is defined as the range [@base_event, @base_event +
>>>> @nevents[,
>>>> +together with an @action (KVM_PMU_EVENT_ALLOW or
>>>> KVM_PMU_EVENT_DENY). The
>>>> +first registered range defines the global policy (global ALLOW if
>>>> the first
>>>> +@action is DENY, global DENY if the first @action is ALLOW).
>>>> Multiple ranges
>>>> +can be programmed, and must fit within the 16bit space defined by
>>>> the ARMv8.1
>>>> +PMU architecture.
>>> what about before 8.1 where the range was 10 bits? Should it be 
>>> tested
>>> in the code?
>> 
>> It's a good point. We could test that upon installing the filter and 
>> limit
>> the bitmap allocation to the minimum.
>> 
>>> nitpicking: It is not totally obvious what does happen if the user 
>>> space
>>> sets a deny filter on a range and then an allow filter on the same
>>> range. it is supported but may be worth telling so? Also explain the 
>>> the
>>> default filtering remains "allow" by default?
>> 
>> Overlapping filters are easy: the last one wins. And yes, no filter 
>> means
>> just that: no filter.
> Actually the point I wanted to put forward is
> 1) set allow filter on range [0-a] -> default setting is deny and allow
> [0-a] only
> 2) deny deny filter on rang [0-a] -> there is no "real" active 
> filtering
> anymore but default behavior still is deny. ie. you do not destroy the
> bitmap on the last filter removal but on the VM removal.

Ah, gotcha. Yes, this is odd. The solution to this is to re-apply a 
default
behaviour. But this needs documenting...

Thanks,

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

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

Re: [PATCH v2 2/2] KVM: arm64: Document PMU filtering API

[Marc Zyngier]

On 2020-03-10 17:30, Auger Eric wrote:
> Hi Marc,
> 
> On 3/10/20 12:54 PM, Marc Zyngier wrote:
>> On 2020-03-09 18:17, Auger Eric wrote:
>>> Hi Marc,
>>> 
>>> On 3/9/20 1:48 PM, Marc Zyngier wrote:
>>>> Add a small blurb describing how the event filtering API gets used.
>>>> 
>>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>>> ---
>>>>  Documentation/virt/kvm/devices/vcpu.rst | 40 
>>>> +++++++++++++++++++++++++
>>>>  1 file changed, 40 insertions(+)
>>>> 
>>>> diff --git a/Documentation/virt/kvm/devices/vcpu.rst
>>>> b/Documentation/virt/kvm/devices/vcpu.rst
>>>> index 9963e680770a..7262c0469856 100644
>>>> --- a/Documentation/virt/kvm/devices/vcpu.rst
>>>> +++ b/Documentation/virt/kvm/devices/vcpu.rst
>>>> @@ -55,6 +55,46 @@ Request the initialization of the PMUv3.  If 
>>>> using
>>>> the PMUv3 with an in-kernel
>>>>  virtual GIC implementation, this must be done after initializing 
>>>> the
>>>> in-kernel
>>>>  irqchip.
>>>> 
>>>> +1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
>>>> +---------------------------------------
>>>> +
>>>> +:Parameters: in kvm_device_attr.addr the address for a PMU event
>>>> filter is a
>>>> +             pointer to a struct kvm_pmu_event_filter
>>>> +
>>>> +:Returns:
>>>> +
>>>> +     =======  
>>>> ======================================================
>>>> +     -ENODEV: PMUv3 not supported or GIC not initialized
>>>> +     -ENXIO:  PMUv3 not properly configured or in-kernel irqchip 
>>>> not
>>>> +           configured as required prior to calling this attribute
>>>> +     -EBUSY:  PMUv3 already initialized
>>> maybe document -EINVAL?
>> 
>> Yup, definitely.
>> 
>>>> +     =======  
>>>> ======================================================
>>>> +
>>>> +Request the installation of a PMU event filter describe as follows:
>>> s/describe/described
>>>> +
>>>> +struct kvm_pmu_event_filter {
>>>> +    __u16    base_event;
>>>> +    __u16    nevents;
>>>> +
>>>> +#define KVM_PMU_EVENT_ALLOW    0
>>>> +#define KVM_PMU_EVENT_DENY    1
>>>> +
>>>> +    __u8    action;
>>>> +    __u8    pad[3];
>>>> +};
>>>> +
>>>> +A filter range is defined as the range [@base_event, @base_event +
>>>> @nevents[,
>>>> +together with an @action (KVM_PMU_EVENT_ALLOW or
>>>> KVM_PMU_EVENT_DENY). The
>>>> +first registered range defines the global policy (global ALLOW if
>>>> the first
>>>> +@action is DENY, global DENY if the first @action is ALLOW).
>>>> Multiple ranges
>>>> +can be programmed, and must fit within the 16bit space defined by
>>>> the ARMv8.1
>>>> +PMU architecture.
>>> what about before 8.1 where the range was 10 bits? Should it be 
>>> tested
>>> in the code?
>> 
>> It's a good point. We could test that upon installing the filter and 
>> limit
>> the bitmap allocation to the minimum.
>> 
>>> nitpicking: It is not totally obvious what does happen if the user 
>>> space
>>> sets a deny filter on a range and then an allow filter on the same
>>> range. it is supported but may be worth telling so? Also explain the 
>>> the
>>> default filtering remains "allow" by default?
>> 
>> Overlapping filters are easy: the last one wins. And yes, no filter 
>> means
>> just that: no filter.
> Actually the point I wanted to put forward is
> 1) set allow filter on range [0-a] -> default setting is deny and allow
> [0-a] only
> 2) deny deny filter on rang [0-a] -> there is no "real" active 
> filtering
> anymore but default behavior still is deny. ie. you do not destroy the
> bitmap on the last filter removal but on the VM removal.

Ah, gotcha. Yes, this is odd. The solution to this is to re-apply a 
default
behaviour. But this needs documenting...

Thanks,

        M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm