KVM: arm64: Filtering PMU events

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 <[email protected]>

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

arch/arm64/include/asm/kvm_host.h +6 -0

index 57fd46acd058..8e63c618688d 100644
@@ -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

arch/arm64/include/uapi/asm/kvm.h +16 -0

index ba85bb23f060..7b1511d6ce44 100644
@@ -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

virt/kvm/arm/arm.c +2 -0

index eda7b624eab8..8d849ac88a44 100644
@@ -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]);

virt/kvm/arm/pmu.c +0 -0

index f0d0312c0a55..9f0fd0224d5b 100644

virt/kvm/arm/pmu.c +9 -2

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

virt/kvm/arm/pmu.c +0 -9

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

virt/kvm/arm/pmu.c +4 -3

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

virt/kvm/arm/pmu.c +31 -0

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

Hi Marc,

Quoted 95 lines · 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 <[email protected]>

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

arch/arm64/include/asm/kvm_host.h +6 -0

index 57fd46acd058..8e63c618688d 100644
@@ -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

arch/arm64/include/uapi/asm/kvm.h +16 -0

index ba85bb23f060..7b1511d6ce44 100644
@@ -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

virt/kvm/arm/arm.c +2 -0

index eda7b624eab8..8d849ac88a44 100644
@@ -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]);

virt/kvm/arm/pmu.c +0 -0

index f0d0312c0a55..9f0fd0224d5b 100644

virt/kvm/arm/pmu.c static void kvm_pmu_create_perf_event(struct · line 584

Quoted 6 lines

virt/kvm/arm/pmu.c +2 -0

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

virt/kvm/arm/pmu.c static void kvm_pmu_create_perf_event(struct · line 586

Quoted 3 lines

virt/kvm/arm/pmu.c +0 -0

 	/* Software increment event does't need to be backed by a perf event 
 */

nit: while wer are at it fix the does't typo

Quoted 13 lines

virt/kvm/arm/pmu.c +9 -2

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

virt/kvm/arm/pmu.c static void kvm_pmu_create_perf_event(struct · line 607

Quoted 9 lines

virt/kvm/arm/pmu.c +1 -2

 	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?

Quoted 24 lines

virt/kvm/arm/pmu.c +0 -9

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

virt/kvm/arm/pmu.c static bool pmu_irq_is_valid(struct kvm *kvm, int irq) · line 804

Quoted 10 lines

virt/kvm/arm/pmu.c +7 -0

+#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?

Quoted 12 lines

virt/kvm/arm/pmu.c +4 -0

+
+	if (vcpu->arch.pmu.created)
+		return -EBUSY;
+
 	switch (attr->attr) {

virt/kvm/arm/pmu.c +0 -0

 	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;
 

virt/kvm/arm/pmu.c int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) · line 809

Quoted 3 lines

virt/kvm/arm/pmu.c +0 -3

-		if (!test_bit(KVM_ARM_VCPU_PMU_V3, vcpu->arch.features))
-			return -ENODEV;
-

Quoted 8 lines

virt/kvm/arm/pmu.c +0 -0

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

virt/kvm/arm/pmu.c int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu · line 844

Quoted 10 lines

virt/kvm/arm/pmu.c +10 -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 >= ?

virt/kvm/arm/pmu.c int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) · line 846

Quoted 3 lines

virt/kvm/arm/pmu.c +3 -0

+		    (filter.action != KVM_PMU_EVENT_ALLOW &&
+		     filter.action != KVM_PMU_EVENT_DENY))
+			return -EINVAL;

-EINVAL is not documented in the API doc.

Quoted 34 lines

virt/kvm/arm/pmu.c +31 -0

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

virt/kvm/arm/pmu.c int kvm_arm_pmu_v3_has_attr(struct kvm_vcpu *vcpu, · line 913

Quoted 6 lines

virt/kvm/arm/pmu.c +0 -0

 		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?

Quoted 5 lines

virt/kvm/arm/pmu.c +1 -0

 	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;

Hi Eric,

Quoted 101 lines · Auger Eric

Hi Marc,

Quoted 98 lines · 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 <[email protected]>

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

arch/arm64/include/asm/kvm_host.h +6 -0

index 57fd46acd058..8e63c618688d 100644
@@ -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

arch/arm64/include/uapi/asm/kvm.h +16 -0

index ba85bb23f060..7b1511d6ce44 100644
@@ -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

virt/kvm/arm/arm.c +2 -0

index eda7b624eab8..8d849ac88a44 100644
@@ -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]);

virt/kvm/arm/pmu.c +0 -0

index f0d0312c0a55..9f0fd0224d5b 100644
@@ -579,10 +579,19 @@ static void kvm_pmu_create_perf_event(struct
 kvm_vcpu *vcpu, u64 select_idx)
 

Quoted 9 lines

virt/kvm/arm/pmu.c static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx) · line 583

Quoted 4 lines

virt/kvm/arm/pmu.c +2 -0

      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? ;-)

Quoted 19 lines

Quoted 4 lines

virt/kvm/arm/pmu.c +0 -0

      /* Software increment event does't need to be backed by a perf

event */

nit: while wer are at it fix the does't typo

Quoted 14 lines

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

Quoted 12 lines

Quoted 9 lines

     memset(&attr, 0, sizeof(struct perf_event_attr));

virt/kvm/arm/pmu.c +0 -0

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

Quoted 25 lines

Quoted 25 lines

     counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);

virt/kvm/arm/pmu.c +0 -9

@@ -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;
  }
 

Quoted 11 lines

Quoted 10 lines

virt/kvm/arm/pmu.c +2 -0

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

Quoted 12 lines

Quoted 12 lines

virt/kvm/arm/pmu.c +4 -0

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

virt/kvm/arm/pmu.c +0 -0

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

Quoted 3 lines

virt/kvm/arm/pmu.c int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) · line 809

Quoted 3 lines

virt/kvm/arm/pmu.c +0 -3

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

Quoted 4 lines

Quoted 4 lines

virt/kvm/arm/pmu.c +0 -0

          if (get_user(irq, uaddr))
              return -EFAULT;

Quoted 16 lines

Quoted 5 lines

virt/kvm/arm/pmu.c +0 -0

@@ -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;
      }

virt/kvm/arm/pmu.c int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu · line 844

Quoted 10 lines

virt/kvm/arm/pmu.c +10 -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.

Quoted 5 lines

virt/kvm/arm/pmu.c int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) · line 846

Quoted 4 lines

virt/kvm/arm/pmu.c +3 -0

+            (filter.action != KVM_PMU_EVENT_ALLOW &&
+             filter.action != KVM_PMU_EVENT_DENY))
+            return -EINVAL;

-EINVAL is not documented in the API doc.

Good point.

Quoted 35 lines

Quoted 35 lines

virt/kvm/arm/pmu.c +26 -0

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

virt/kvm/arm/pmu.c +5 -0

+
+        mutex_unlock(&vcpu->kvm->lock);
+
+        return 0;
+    }

     case KVM_ARM_VCPU_PMU_V3_INIT:

Quoted 8 lines

Quoted 6 lines

         return kvm_arm_pmu_v3_init(vcpu);
     }

virt/kvm/arm/pmu.c +0 -0

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

Quoted 6 lines

Quoted 6 lines

     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.

Hi Marc,

Quoted 116 lines · Marc Zyngier

Hi Eric,

Quoted 112 lines · Auger Eric

Hi Marc,

Quoted 98 lines · 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 <[email protected]>

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

arch/arm64/include/asm/kvm_host.h +6 -0

index 57fd46acd058..8e63c618688d 100644
@@ -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

arch/arm64/include/uapi/asm/kvm.h +2 -0

index ba85bb23f060..7b1511d6ce44 100644
@@ -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.
+ */

arch/arm64/include/uapi/asm/kvm.h +11 -0

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

arch/arm64/include/uapi/asm/kvm.h +1 -0

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

virt/kvm/arm/arm.c +2 -0

index eda7b624eab8..8d849ac88a44 100644
@@ -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]);

virt/kvm/arm/pmu.c +0 -0

index f0d0312c0a55..9f0fd0224d5b 100644

virt/kvm/arm/pmu.c static void kvm_pmu_create_perf_event(struct · line 584

Quoted 6 lines

virt/kvm/arm/pmu.c +2 -0

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

Quoted 1 line

You don't like it? ;-)

? eventset set only once instead of 2 times

Quoted 32 lines

Quoted 19 lines

Quoted 4 lines

virt/kvm/arm/pmu.c +0 -0

      /* Software increment event does't need to be backed by a perf

event */

nit: while wer are at it fix the does't typo

Quoted 14 lines

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

Quoted 12 lines

Quoted 9 lines

     memset(&attr, 0, sizeof(struct perf_event_attr));

virt/kvm/arm/pmu.c +0 -0

@@ -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?

Quoted 4 lines

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.

Quoted 52 lines

Quoted 26 lines

Quoted 26 lines

     counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);

virt/kvm/arm/pmu.c +0 -9

@@ -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;
  }
 

Quoted 11 lines

Quoted 10 lines

virt/kvm/arm/pmu.c +2 -0

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

Quoted 12 lines

Quoted 12 lines

virt/kvm/arm/pmu.c +4 -0

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

virt/kvm/arm/pmu.c +0 -0

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

Quoted 7 lines

Quoted 3 lines

virt/kvm/arm/pmu.c int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) · line 809

Quoted 3 lines

virt/kvm/arm/pmu.c +0 -3

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

Quoted 5 lines

Quoted 2 lines

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;

Quoted 120 lines · Auger Eric

Hi Marc,

Quoted 117 lines · Marc Zyngier

Hi Eric,

Quoted 113 lines · Auger Eric

Hi Marc,

Quoted 99 lines · 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 <[email protected]>

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

arch/arm64/include/asm/kvm_host.h +6 -0

index 57fd46acd058..8e63c618688d 100644
@@ -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

arch/arm64/include/uapi/asm/kvm.h +16 -0

index ba85bb23f060..7b1511d6ce44 100644
@@ -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

virt/kvm/arm/arm.c +2 -0

index eda7b624eab8..8d849ac88a44 100644
@@ -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]);

virt/kvm/arm/pmu.c +0 -0

index f0d0312c0a55..9f0fd0224d5b 100644

virt/kvm/arm/pmu.c static void kvm_pmu_create_perf_event(struct · line 584

Quoted 6 lines

virt/kvm/arm/pmu.c +2 -0

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

Quoted 2 lines

Quoted 1 line

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.

Quoted 37 lines

Quoted 32 lines

Quoted 19 lines

Quoted 4 lines

virt/kvm/arm/pmu.c +0 -0

      /* Software increment event does't need to be backed by a perf

event */

nit: while wer are at it fix the does't typo

Quoted 14 lines

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

Quoted 12 lines

Quoted 9 lines

     memset(&attr, 0, sizeof(struct perf_event_attr));

virt/kvm/arm/pmu.c +0 -0

@@ -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?

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 52 lines

Quoted 52 lines

Quoted 26 lines

Quoted 26 lines

     counter = kvm_pmu_get_pair_counter_value(vcpu, pmc);

virt/kvm/arm/pmu.c +0 -9

@@ -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;
  }
 

Quoted 11 lines

Quoted 10 lines

virt/kvm/arm/pmu.c +2 -0

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

Quoted 12 lines

Quoted 12 lines

virt/kvm/arm/pmu.c +4 -0

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

virt/kvm/arm/pmu.c +0 -0

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

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

virt/kvm/arm/pmu.c int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr) · line 809

Quoted 3 lines

virt/kvm/arm/pmu.c +0 -3

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

[...]

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 118 lines · Auger Eric

Hi Marc,

Quoted 115 lines · Marc Zyngier

Hi Eric,

Quoted 111 lines · Auger Eric

Hi Marc,

Quoted 97 lines · 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 <[email protected]>

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

arch/arm64/include/asm/kvm_host.h +6 -0

index 57fd46acd058..8e63c618688d 100644
@@ -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

arch/arm64/include/uapi/asm/kvm.h +16 -0

index ba85bb23f060..7b1511d6ce44 100644
@@ -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

virt/kvm/arm/arm.c +2 -0

index eda7b624eab8..8d849ac88a44 100644
@@ -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]);

virt/kvm/arm/pmu.c +0 -0

index f0d0312c0a55..9f0fd0224d5b 100644

virt/kvm/arm/pmu.c static void kvm_pmu_create_perf_event(struct · line 584

Quoted 6 lines

virt/kvm/arm/pmu.c +2 -0

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

Quoted 2 lines

Quoted 1 line

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.

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?

Quoted 1 line · Alexander Graf

Hi Marc,

[...]

Quoted 3 lines

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.

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

Signed-off-by Marc Zyngier <[email protected]>

---
 Documentation/virt/kvm/devices/vcpu.rst | 40 +++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Documentation/virt/kvm/devices/vcpu.rst +3 -0

index 9963e680770a..7262c0469856 100644
@@ -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
+---------------------------------------
+

Documentation/virt/kvm/devices/vcpu.rst +8 -0

+
+struct kvm_pmu_event_filter {
+	__u16	base_event;
+	__u16	nevents;
+
+#define KVM_PMU_EVENT_ALLOW	0
+#define KVM_PMU_EVENT_DENY	1
+

Documentation/virt/kvm/devices/vcpu.rst +1 -0

+
 
 2. GROUP: KVM_ARM_VCPU_TIMER_CTRL
 =================================

Hi Marc,

Quoted 18 lines · Marc Zyngier

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

Signed-off-by Marc Zyngier <[email protected]>

---
 Documentation/virt/kvm/devices/vcpu.rst | 40 
+++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Documentation/virt/kvm/devices/vcpu.rst +0 -0

index 9963e680770a..7262c0469856 100644
@@ -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.

Quoted 5 lines

+1.3 ATTRIBUTE: KVM_ARM_VCPU_PMU_V3_FILTER
+---------------------------------------
+
+:Parameters: in kvm_device_attr.addr the address for a PMU event filter is a

Documentation/virt/kvm/devices/vcpu.rst Request the initialization of the PMUv3. If using the PMUv3 with an in-kernel · line 70

Quoted 9 lines

Documentation/virt/kvm/devices/vcpu.rst +9 -0

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

Documentation/virt/kvm/devices/vcpu.rst Request the initialization of the PMUv3. If using the PMUv3 with an in-kernel · line 73

Quoted 3 lines

Documentation/virt/kvm/devices/vcpu.rst +3 -0

+	 =======  ======================================================
+
+Request the installation of a PMU event filter describe as follows:

s/describe/described

Quoted 17 lines

Documentation/virt/kvm/devices/vcpu.rst +13 -0

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

Quoted 6 lines

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?

Quoted 21 lines · Auger Eric

Hi Marc,

Quoted 18 lines · Marc Zyngier

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

Signed-off-by Marc Zyngier <[email protected]>

---
 Documentation/virt/kvm/devices/vcpu.rst | 40 +++++++++++++++++++++++++

 1 file changed, 40 insertions(+)

Documentation/virt/kvm/devices/vcpu.rst +0 -0

index 9963e680770a..7262c0469856 100644
@@ -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.

Quoted 15 lines

Quoted 6 lines

+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

Documentation/virt/kvm/devices/vcpu.rst Request the initialization of the PMUv3. If using the PMUv3 with an in-kernel · line 70

Quoted 8 lines

Documentation/virt/kvm/devices/vcpu.rst +8 -0

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

Quoted 21 lines

Documentation/virt/kvm/devices/vcpu.rst

Quoted 3 lines

+     =======  ======================================================

Documentation/virt/kvm/devices/vcpu.rst +2 -0

+
+Request the installation of a PMU event filter describe as follows:

s/describe/described

Quoted 17 lines

Documentation/virt/kvm/devices/vcpu.rst +13 -0

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

Quoted 8 lines

Quoted 6 lines

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.

Quoted 4 lines

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.

Hi Marc,

Quoted 83 lines · Marc Zyngier

Quoted 32 lines · Auger Eric

Hi Marc,

Quoted 29 lines · Marc Zyngier

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

Signed-off-by Marc Zyngier <[email protected]>

---
 Documentation/virt/kvm/devices/vcpu.rst | 40
+++++++++++++++++++++++++

 1 file changed, 40 insertions(+)

Documentation/virt/kvm/devices/vcpu.rst +0 -0

index 9963e680770a..7262c0469856 100644
@@ -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

Documentation/virt/kvm/devices/vcpu.rst +3 -0

+
+:Returns:
+

Quoted 8 lines

Quoted 7 lines

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

Quoted 22 lines

Documentation/virt/kvm/devices/vcpu.rst

Quoted 4 lines

+     =======
======================================================

Documentation/virt/kvm/devices/vcpu.rst +2 -0

+
+Request the installation of a PMU event filter describe as follows:

s/describe/described

Quoted 17 lines

Documentation/virt/kvm/devices/vcpu.rst +13 -0

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

Quoted 8 lines

Quoted 6 lines

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.

Quoted 4 lines

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?

Quoted 2 lines

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.