[PATCH v3 0/5] KVM: arm64: Filtering PMU events

[PATCH v3 0/5] 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 v2:
  - Split out the error handling refactor for clarity
  - Added a terrible hack to fish out the PMU version, because BL is great
  - Update the guest's view of PCMEID{0,1}_EL1
  - General tidying up

* 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 (5):
  KVM: arm64: Refactor PMU attribute error handling
  KVM: arm64: Use event mask matching architecture revision
  KVM: arm64: Add PMU event filtering infrastructure
  KVM: arm64: Mask out filtered events in PCMEID{0,1}_EL1
  KVM: arm64: Document PMU filtering API

 Documentation/virt/kvm/devices/vcpu.rst |  46 ++++++
 arch/arm64/include/asm/kvm_host.h       |   7 +
 arch/arm64/include/uapi/asm/kvm.h       |  16 ++
 arch/arm64/kvm/arm.c                    |   2 +
 arch/arm64/kvm/pmu-emul.c               | 199 +++++++++++++++++++++---
 arch/arm64/kvm/sys_regs.c               |   5 +-
 include/kvm/arm_pmu.h                   |   5 +
 7 files changed, 254 insertions(+), 26 deletions(-)

-- 
2.28.0


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

[PATCH v3 1/5] KVM: arm64: Refactor PMU attribute error handling

[Marc Zyngier]

The PMU emulation error handling is pretty messy when dealing with
attributes. Let's refactor it so that we have less duplication,
and that it is easy to extend later on.

A functional change is that kvm_arm_pmu_v3_init() used to return
-ENXIO when the PMU feature wasn't set. The error is now reported
as -ENODEV, matching the documentation. -ENXIO is still returned
when the interrupt isn't properly configured.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/pmu-emul.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index f0d0312c0a55..93d797df42c6 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -735,15 +735,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;
 
@@ -796,6 +787,15 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
 
 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 +804,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;
 
-- 
2.28.0


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

[PATCH v3 2/5] KVM: arm64: Use event mask matching architecture revision

[Marc Zyngier]

The PMU code suffers from a small defect where we assume that the event
number provided by the guest is always 16 bit wide, even if the CPU only
implements the ARMv8.0 architecture. This isn't really problematic in
the sense that the event number ends up in a system register, cropping
it to the right width, but still this needs fixing.

In order to make it work, let's probe the version of the PMU that the
guest is going to use. This is done by temporarily creating a kernel
event and looking at the PMUVer field that has been saved at probe time
in the associated arm_pmu structure. This in turn gets saved in the kvm
structure, and subsequently used to compute the event mask that gets
used throughout the PMU code.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/include/asm/kvm_host.h |  2 +
 arch/arm64/kvm/pmu-emul.c         | 81 +++++++++++++++++++++++++++++--
 2 files changed, 78 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 65568b23868a..6cd60be69c28 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -110,6 +110,8 @@ struct kvm_arch {
 	 * supported.
 	 */
 	bool return_nisv_io_abort_to_user;
+
+	unsigned int pmuver;
 };
 
 struct kvm_vcpu_fault_info {
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 93d797df42c6..8a5f65763814 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -20,6 +20,20 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc);
 
 #define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1
 
+static u32 kvm_pmu_event_mask(struct kvm *kvm)
+{
+	switch (kvm->arch.pmuver) {
+	case 1:			/* ARMv8.0 */
+		return GENMASK(9, 0);
+	case 4:			/* ARMv8.1 */
+	case 5:			/* ARMv8.4 */
+	case 6:			/* ARMv8.5 */
+		return GENMASK(15, 0);
+	default:		/* Shouldn't be there, just for sanity */
+		return 0;
+	}
+}
+
 /**
  * kvm_pmu_idx_is_64bit - determine if select_idx is a 64bit counter
  * @vcpu: The vcpu pointer
@@ -100,7 +114,7 @@ static bool kvm_pmu_idx_has_chain_evtype(struct kvm_vcpu *vcpu, u64 select_idx)
 		return false;
 
 	reg = PMEVTYPER0_EL0 + select_idx;
-	eventsel = __vcpu_sys_reg(vcpu, reg) & ARMV8_PMU_EVTYPE_EVENT;
+	eventsel = __vcpu_sys_reg(vcpu, reg) & kvm_pmu_event_mask(vcpu->kvm);
 
 	return eventsel == ARMV8_PMUV3_PERFCTR_CHAIN;
 }
@@ -495,7 +509,7 @@ void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val)
 
 		/* PMSWINC only applies to ... SW_INC! */
 		type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
-		type &= ARMV8_PMU_EVTYPE_EVENT;
+		type &= kvm_pmu_event_mask(vcpu->kvm);
 		if (type != ARMV8_PMUV3_PERFCTR_SW_INCR)
 			continue;
 
@@ -578,7 +592,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
 	data = __vcpu_sys_reg(vcpu, reg);
 
 	kvm_pmu_stop_counter(vcpu, pmc);
-	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
+	eventsel = data & kvm_pmu_event_mask(vcpu->kvm);;
 
 	/* Software increment event does't need to be backed by a perf event */
 	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
@@ -679,17 +693,68 @@ static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx)
 void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
 				    u64 select_idx)
 {
-	u64 reg, event_type = data & ARMV8_PMU_EVTYPE_MASK;
+	u64 reg, mask;
+
+	mask  =  ARMV8_PMU_EVTYPE_MASK;
+	mask &= ~ARMV8_PMU_EVTYPE_EVENT;
+	mask |= kvm_pmu_event_mask(vcpu->kvm);
 
 	reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
 	      ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + select_idx;
 
-	__vcpu_sys_reg(vcpu, reg) = event_type;
+	__vcpu_sys_reg(vcpu, reg) = data & mask;
 
 	kvm_pmu_update_pmc_chained(vcpu, select_idx);
 	kvm_pmu_create_perf_event(vcpu, select_idx);
 }
 
+static int kvm_pmu_probe_pmuver(void)
+{
+	struct perf_event_attr attr = { };
+	struct perf_event *event;
+	struct arm_pmu *pmu;
+	int pmuver = 0xf;
+
+	/*
+	 * Create a dummy event that only counts user cycles. As we'll never
+	 * leave thing function with the event being live, it will never
+	 * count anything. But it allows us to probe some of the PMU
+	 * details. Yes, this is terrible.
+	 */
+	attr.type = PERF_TYPE_RAW;
+	attr.size = sizeof(attr);
+	attr.pinned = 1;
+	attr.disabled = 0;
+	attr.exclude_user = 0;
+	attr.exclude_kernel = 1;
+	attr.exclude_hv = 1;
+	attr.exclude_host = 1;
+	attr.config = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
+	attr.sample_period = GENMASK(63, 0);
+
+	event = perf_event_create_kernel_counter(&attr, -1, current,
+						 kvm_pmu_perf_overflow, &attr);
+
+	if (IS_ERR(event)) {
+		pr_err_once("kvm: pmu event creation failed %ld\n",
+			    PTR_ERR(event));
+		return 0xf;
+	}
+
+	if (event->pmu) {
+		pmu = to_arm_pmu(event->pmu);
+		if (pmu->pmuver)
+			pmuver = pmu->pmuver;
+		pr_debug("PMU on CPUs %*pbl version %x\n",
+			 cpumask_pr_args(&pmu->supported_cpus), pmuver);
+	}
+
+	perf_event_disable(event);
+	perf_event_release_kernel(event);
+
+	return pmuver;
+}
+
 bool kvm_arm_support_pmu_v3(void)
 {
 	/*
@@ -796,6 +861,12 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
 	if (vcpu->arch.pmu.created)
 		return -EBUSY;
 
+	if (!vcpu->kvm->arch.pmuver)
+		vcpu->kvm->arch.pmuver = kvm_pmu_probe_pmuver();
+
+	if (vcpu->kvm->arch.pmuver == 0xf)
+		return -ENODEV;
+
 	switch (attr->attr) {
 	case KVM_ARM_VCPU_PMU_V3_IRQ: {
 		int __user *uaddr = (int __user *)(long)attr->addr;
-- 
2.28.0


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

[PATCH v3 3/5] 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 |  5 +++
 arch/arm64/include/uapi/asm/kvm.h | 16 +++++++
 arch/arm64/kvm/arm.c              |  2 +
 arch/arm64/kvm/pmu-emul.c         | 70 ++++++++++++++++++++++++++++---
 4 files changed, 87 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 6cd60be69c28..1e64260b7e2b 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -111,6 +111,11 @@ struct kvm_arch {
 	 */
 	bool return_nisv_io_abort_to_user;
 
+	/*
+	 * VM-wide PMU filter, implemented as a bitmap and big enough for
+	 * up to 2^10 events (ARMv8.0) or 2^16 events (ARMv8.1+).
+	 */
+	unsigned long *pmu_filter;
 	unsigned int pmuver;
 };
 
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/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
index 691d21e4c717..0f11d0009c17 100644
--- a/arch/arm64/kvm/arm.c
+++ b/arch/arm64/kvm/arm.c
@@ -145,6 +145,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 {
 	int i;
 
+	bitmap_free(kvm->arch.pmu_filter);
+
 	kvm_vgic_destroy(kvm);
 
 	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 8a5f65763814..67a731bafbc9 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -30,6 +30,7 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
 	case 6:			/* ARMv8.5 */
 		return GENMASK(15, 0);
 	default:		/* Shouldn't be there, just for sanity */
+		WARN_ONCE(1, "Unknown PMU version %d\n", kvm->arch.pmuver);
 		return 0;
 	}
 }
@@ -592,11 +593,21 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
 	data = __vcpu_sys_reg(vcpu, reg);
 
 	kvm_pmu_stop_counter(vcpu, pmc);
-	eventsel = data & kvm_pmu_event_mask(vcpu->kvm);;
+	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
+		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
+	else
+		eventsel = data & kvm_pmu_event_mask(vcpu->kvm);
+
+	/* Software increment event doesn't need to be backed by a perf event */
+	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
+		return;
 
-	/* 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 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));
@@ -608,8 +619,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);
 
@@ -892,6 +902,53 @@ 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;
+		int nr_events;
+
+		nr_events = kvm_pmu_event_mask(vcpu->kvm) + 1;
+
+		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);
 	}
@@ -928,6 +985,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.28.0


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

[PATCH v3 4/5] KVM: arm64: Mask out filtered events in PCMEID{0, 1}_EL1

[Marc Zyngier]

As we can now hide events from the guest, let's also adjust its view of
PCMEID{0,1}_EL1 so that it can figure out why some common events are not
counting as they should.

The astute user can still look into the TRM for their CPU and find out
they've been cheated, though. Nobody's perfect.

Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 arch/arm64/kvm/pmu-emul.c | 29 +++++++++++++++++++++++++++++
 arch/arm64/kvm/sys_regs.c |  5 +----
 include/kvm/arm_pmu.h     |  5 +++++
 3 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
index 67a731bafbc9..0458860bade2 100644
--- a/arch/arm64/kvm/pmu-emul.c
+++ b/arch/arm64/kvm/pmu-emul.c
@@ -765,6 +765,35 @@ static int kvm_pmu_probe_pmuver(void)
 	return pmuver;
 }
 
+u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
+{
+	unsigned long *bmap = vcpu->kvm->arch.pmu_filter;
+	u64 val, mask = 0;
+	int base, i;
+
+	if (!pmceid1) {
+		val = read_sysreg(pmceid0_el0);
+		base = 0;
+	} else {
+		val = read_sysreg(pmceid1_el0);
+		base = 32;
+	}
+
+	if (!bmap)
+		return val;
+
+	for (i = 0; i < 32; i += 8) {
+		u64 byte;
+
+		byte = bitmap_get_value8(bmap, base + i);
+		mask |= byte << i;
+		byte = bitmap_get_value8(bmap, 0x4000 + base + i);
+		mask |= byte << (32 + i);
+	}
+
+	return val & mask;
+}
+
 bool kvm_arm_support_pmu_v3(void)
 {
 	/*
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 077293b5115f..20ab2a7d37ca 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -769,10 +769,7 @@ static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	if (pmu_access_el0_disabled(vcpu))
 		return false;
 
-	if (!(p->Op2 & 1))
-		pmceid = read_sysreg(pmceid0_el0);
-	else
-		pmceid = read_sysreg(pmceid1_el0);
+	pmceid = kvm_pmu_get_pmceid(vcpu, (p->Op2 & 1));
 
 	p->regval = pmceid;
 
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 6db030439e29..98cbfe885a53 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -34,6 +34,7 @@ struct kvm_pmu {
 u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx);
 void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
 u64 kvm_pmu_valid_counter_mask(struct kvm_vcpu *vcpu);
+u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1);
 void kvm_pmu_vcpu_init(struct kvm_vcpu *vcpu);
 void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu);
 void kvm_pmu_vcpu_destroy(struct kvm_vcpu *vcpu);
@@ -108,6 +109,10 @@ static inline int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
 {
 	return 0;
 }
+static inline u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
+{
+	return 0;
+}
 #endif
 
 #endif
-- 
2.28.0


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

[PATCH v3 5/5] 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 | 46 +++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
index ca374d3fe085..203b91e93151 100644
--- a/Documentation/virt/kvm/devices/vcpu.rst
+++ b/Documentation/virt/kvm/devices/vcpu.rst
@@ -55,6 +55,52 @@ 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
+	 -EINVAL: Invalid filter range
+	 =======  ======================================================
+
+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 event space defined by the PMU
+architecture (10 bits on ARMv8.0, 16 bits from ARMv8.1 onwards).
+
+Note: "Cancelling" a filter by registering the opposite action for the same
+range doesn't change the default action. For example, installing an ALLOW
+filter for event range [0:10] as the first filter and then applying a DENY
+action for the same range will leave the whole range as disabled.
+
+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.28.0


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

Re: [PATCH v3 1/5] KVM: arm64: Refactor PMU attribute error handling

[Andrew Jones]

Hi Marc,

On Tue, Sep 08, 2020 at 08:58:26AM +0100, Marc Zyngier wrote:
> The PMU emulation error handling is pretty messy when dealing with
> attributes. Let's refactor it so that we have less duplication,
> and that it is easy to extend later on.
> 
> A functional change is that kvm_arm_pmu_v3_init() used to return
> -ENXIO when the PMU feature wasn't set. The error is now reported
> as -ENODEV, matching the documentation.

Hmm, I didn't think we could make changes like that, since some userspace
somewhere may now depend on the buggy interface. That said, I'm not really
against the change, but maybe it should go as a separate patch.

> -ENXIO is still returned
> when the interrupt isn't properly configured.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/pmu-emul.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index f0d0312c0a55..93d797df42c6 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -735,15 +735,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;
>  
> @@ -796,6 +787,15 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int irq)
>  
>  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;

nit: could combine these two if's w/ an ||

> +
> +	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 +804,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;
>  
> -- 
> 2.28.0
> 

Thanks,
drew

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


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

Re: [PATCH v3 2/5] KVM: arm64: Use event mask matching architecture revision

[Andrew Jones]

On Tue, Sep 08, 2020 at 08:58:27AM +0100, Marc Zyngier wrote:
> The PMU code suffers from a small defect where we assume that the event
> number provided by the guest is always 16 bit wide, even if the CPU only
> implements the ARMv8.0 architecture. This isn't really problematic in
> the sense that the event number ends up in a system register, cropping
> it to the right width, but still this needs fixing.
> 
> In order to make it work, let's probe the version of the PMU that the
> guest is going to use. This is done by temporarily creating a kernel
> event and looking at the PMUVer field that has been saved at probe time
> in the associated arm_pmu structure. This in turn gets saved in the kvm
> structure, and subsequently used to compute the event mask that gets
> used throughout the PMU code.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h |  2 +
>  arch/arm64/kvm/pmu-emul.c         | 81 +++++++++++++++++++++++++++++--
>  2 files changed, 78 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 65568b23868a..6cd60be69c28 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -110,6 +110,8 @@ struct kvm_arch {
>  	 * supported.
>  	 */
>  	bool return_nisv_io_abort_to_user;
> +
> +	unsigned int pmuver;
>  };
>  
>  struct kvm_vcpu_fault_info {
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 93d797df42c6..8a5f65763814 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -20,6 +20,20 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc);
>  
>  #define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1
>  
> +static u32 kvm_pmu_event_mask(struct kvm *kvm)
> +{
> +	switch (kvm->arch.pmuver) {
> +	case 1:			/* ARMv8.0 */
> +		return GENMASK(9, 0);
> +	case 4:			/* ARMv8.1 */
> +	case 5:			/* ARMv8.4 */
> +	case 6:			/* ARMv8.5 */
> +		return GENMASK(15, 0);
> +	default:		/* Shouldn't be there, just for sanity */

s/there/here/

I see a warning was added here in a later patch. Wouldn't it make sense to
add the warning now?

> +		return 0;
> +	}
> +}
> +
>  /**
>   * kvm_pmu_idx_is_64bit - determine if select_idx is a 64bit counter
>   * @vcpu: The vcpu pointer
> @@ -100,7 +114,7 @@ static bool kvm_pmu_idx_has_chain_evtype(struct kvm_vcpu *vcpu, u64 select_idx)
>  		return false;
>  
>  	reg = PMEVTYPER0_EL0 + select_idx;
> -	eventsel = __vcpu_sys_reg(vcpu, reg) & ARMV8_PMU_EVTYPE_EVENT;
> +	eventsel = __vcpu_sys_reg(vcpu, reg) & kvm_pmu_event_mask(vcpu->kvm);
>  
>  	return eventsel == ARMV8_PMUV3_PERFCTR_CHAIN;
>  }
> @@ -495,7 +509,7 @@ void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val)
>  
>  		/* PMSWINC only applies to ... SW_INC! */
>  		type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
> -		type &= ARMV8_PMU_EVTYPE_EVENT;
> +		type &= kvm_pmu_event_mask(vcpu->kvm);
>  		if (type != ARMV8_PMUV3_PERFCTR_SW_INCR)
>  			continue;
>  
> @@ -578,7 +592,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  	data = __vcpu_sys_reg(vcpu, reg);
>  
>  	kvm_pmu_stop_counter(vcpu, pmc);
> -	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
> +	eventsel = data & kvm_pmu_event_mask(vcpu->kvm);;
>  
>  	/* Software increment event does't need to be backed by a perf event */
>  	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
> @@ -679,17 +693,68 @@ static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx)
>  void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
>  				    u64 select_idx)
>  {
> -	u64 reg, event_type = data & ARMV8_PMU_EVTYPE_MASK;
> +	u64 reg, mask;
> +
> +	mask  =  ARMV8_PMU_EVTYPE_MASK;
> +	mask &= ~ARMV8_PMU_EVTYPE_EVENT;
> +	mask |= kvm_pmu_event_mask(vcpu->kvm);
>  
>  	reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
>  	      ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + select_idx;
>  
> -	__vcpu_sys_reg(vcpu, reg) = event_type;
> +	__vcpu_sys_reg(vcpu, reg) = data & mask;
>  
>  	kvm_pmu_update_pmc_chained(vcpu, select_idx);
>  	kvm_pmu_create_perf_event(vcpu, select_idx);
>  }
>  
> +static int kvm_pmu_probe_pmuver(void)
> +{
> +	struct perf_event_attr attr = { };
> +	struct perf_event *event;
> +	struct arm_pmu *pmu;
> +	int pmuver = 0xf;
> +
> +	/*
> +	 * Create a dummy event that only counts user cycles. As we'll never
> +	 * leave thing function with the event being live, it will never

s/thing/this/

> +	 * count anything. But it allows us to probe some of the PMU
> +	 * details. Yes, this is terrible.
> +	 */
> +	attr.type = PERF_TYPE_RAW;
> +	attr.size = sizeof(attr);
> +	attr.pinned = 1;
> +	attr.disabled = 0;
> +	attr.exclude_user = 0;
> +	attr.exclude_kernel = 1;
> +	attr.exclude_hv = 1;
> +	attr.exclude_host = 1;
> +	attr.config = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
> +	attr.sample_period = GENMASK(63, 0);
> +
> +	event = perf_event_create_kernel_counter(&attr, -1, current,
> +						 kvm_pmu_perf_overflow, &attr);
> +
> +	if (IS_ERR(event)) {
> +		pr_err_once("kvm: pmu event creation failed %ld\n",
> +			    PTR_ERR(event));
> +		return 0xf;
> +	}
> +
> +	if (event->pmu) {
> +		pmu = to_arm_pmu(event->pmu);
> +		if (pmu->pmuver)
> +			pmuver = pmu->pmuver;
> +		pr_debug("PMU on CPUs %*pbl version %x\n",
> +			 cpumask_pr_args(&pmu->supported_cpus), pmuver);

Can't this potentially produce a super long output line? And should it
still output the same message when pmuver is 0xf?

> +	}
> +
> +	perf_event_disable(event);
> +	perf_event_release_kernel(event);
> +
> +	return pmuver;
> +}
> +
>  bool kvm_arm_support_pmu_v3(void)
>  {
>  	/*
> @@ -796,6 +861,12 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  	if (vcpu->arch.pmu.created)
>  		return -EBUSY;
>  
> +	if (!vcpu->kvm->arch.pmuver)
> +		vcpu->kvm->arch.pmuver = kvm_pmu_probe_pmuver();
> +
> +	if (vcpu->kvm->arch.pmuver == 0xf)
> +		return -ENODEV;
> +
>  	switch (attr->attr) {
>  	case KVM_ARM_VCPU_PMU_V3_IRQ: {
>  		int __user *uaddr = (int __user *)(long)attr->addr;
> -- 
> 2.28.0
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

Thanks,
drew


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

Re: [PATCH v3 1/5] KVM: arm64: Refactor PMU attribute error handling

[Marc Zyngier]

Hi Andrew,

On 2020-09-08 10:53, Andrew Jones wrote:
> Hi Marc,
> 
> On Tue, Sep 08, 2020 at 08:58:26AM +0100, Marc Zyngier wrote:
>> The PMU emulation error handling is pretty messy when dealing with
>> attributes. Let's refactor it so that we have less duplication,
>> and that it is easy to extend later on.
>> 
>> A functional change is that kvm_arm_pmu_v3_init() used to return
>> -ENXIO when the PMU feature wasn't set. The error is now reported
>> as -ENODEV, matching the documentation.
> 
> Hmm, I didn't think we could make changes like that, since some 
> userspace
> somewhere may now depend on the buggy interface.

Well, this is the whole point of this patch: discussing whether
this change is acceptable or whether existing VMMs are relying
on such behaviour. We *could* leave it as is, but I thought I'd
bring it up!

> That said, I'm not really
> against the change, but maybe it should go as a separate patch.

Sure, why not.

>> -ENXIO is still returned
>> when the interrupt isn't properly configured.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kvm/pmu-emul.c | 21 +++++++++------------
>>  1 file changed, 9 insertions(+), 12 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>> index f0d0312c0a55..93d797df42c6 100644
>> --- a/arch/arm64/kvm/pmu-emul.c
>> +++ b/arch/arm64/kvm/pmu-emul.c
>> @@ -735,15 +735,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;
>> 
>> @@ -796,6 +787,15 @@ static bool pmu_irq_is_valid(struct kvm *kvm, int 
>> irq)
>> 
>>  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;
> 
> nit: could combine these two if's w/ an ||

This was made to make the userspace visible change obvious to the
reviewer. Now that you have noticed it, I'm happy to merge these
two! ;-)

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 v3 3/5] KVM: arm64: Add PMU event filtering infrastructure

[Andrew Jones]

On Tue, Sep 08, 2020 at 08:58:28AM +0100, 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 |  5 +++
>  arch/arm64/include/uapi/asm/kvm.h | 16 +++++++
>  arch/arm64/kvm/arm.c              |  2 +
>  arch/arm64/kvm/pmu-emul.c         | 70 ++++++++++++++++++++++++++++---
>  4 files changed, 87 insertions(+), 6 deletions(-)
> 

Reviewed-by: Andrew Jones <drjones@redhat.com>


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

Re: [PATCH v3 5/5] KVM: arm64: Document PMU filtering API

[Andrew Jones]

On Tue, Sep 08, 2020 at 08:58:30AM +0100, 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 | 46 +++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index ca374d3fe085..203b91e93151 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> @@ -55,6 +55,52 @@ 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
> +---------------------------------------

Need a couple more '--'

> +
> +: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
> +	 -EINVAL: Invalid filter range
> +	 =======  ======================================================
> +
> +Request the installation of a PMU event filter describe as follows:

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

closing ] is reversed, and should it be [] or [) ?

> +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 event space defined by the PMU
> +architecture (10 bits on ARMv8.0, 16 bits from ARMv8.1 onwards).
> +
> +Note: "Cancelling" a filter by registering the opposite action for the same
> +range doesn't change the default action. For example, installing an ALLOW
> +filter for event range [0:10] as the first filter and then applying a DENY

[) ?

> +action for the same range will leave the whole range as disabled.
> +
> +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.28.0
> 
> _______________________________________________
> kvmarm mailing list
> kvmarm@lists.cs.columbia.edu
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
> 

Otherwise

Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew


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

Re: [PATCH v3 2/5] KVM: arm64: Use event mask matching architecture revision

[Auger Eric]

Hi Marc,

On 9/8/20 9:58 AM, Marc Zyngier wrote:
> The PMU code suffers from a small defect where we assume that the event
> number provided by the guest is always 16 bit wide, even if the CPU only
> implements the ARMv8.0 architecture. This isn't really problematic in
> the sense that the event number ends up in a system register, cropping
> it to the right width, but still this needs fixing.
> 
> In order to make it work, let's probe the version of the PMU that the
> guest is going to use. This is done by temporarily creating a kernel
> event and looking at the PMUVer field that has been saved at probe time
> in the associated arm_pmu structure. This in turn gets saved in the kvm
> structure, and subsequently used to compute the event mask that gets
> used throughout the PMU code.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h |  2 +
>  arch/arm64/kvm/pmu-emul.c         | 81 +++++++++++++++++++++++++++++--
>  2 files changed, 78 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 65568b23868a..6cd60be69c28 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -110,6 +110,8 @@ struct kvm_arch {
>  	 * supported.
>  	 */
>  	bool return_nisv_io_abort_to_user;
> +
> +	unsigned int pmuver;
>  };
>  
>  struct kvm_vcpu_fault_info {
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 93d797df42c6..8a5f65763814 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -20,6 +20,20 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu, struct kvm_pmc *pmc);
>  
>  #define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1
>  
> +static u32 kvm_pmu_event_mask(struct kvm *kvm)
> +{
> +	switch (kvm->arch.pmuver) {
> +	case 1:			/* ARMv8.0 */
> +		return GENMASK(9, 0);
> +	case 4:			/* ARMv8.1 */
> +	case 5:			/* ARMv8.4 */
> +	case 6:			/* ARMv8.5 */
> +		return GENMASK(15, 0);
> +	default:		/* Shouldn't be there, just for sanity */
> +		return 0;
> +	}
> +}
> +
>  /**
>   * kvm_pmu_idx_is_64bit - determine if select_idx is a 64bit counter
>   * @vcpu: The vcpu pointer
> @@ -100,7 +114,7 @@ static bool kvm_pmu_idx_has_chain_evtype(struct kvm_vcpu *vcpu, u64 select_idx)
>  		return false;
>  
>  	reg = PMEVTYPER0_EL0 + select_idx;
> -	eventsel = __vcpu_sys_reg(vcpu, reg) & ARMV8_PMU_EVTYPE_EVENT;
> +	eventsel = __vcpu_sys_reg(vcpu, reg) & kvm_pmu_event_mask(vcpu->kvm);
>  
>  	return eventsel == ARMV8_PMUV3_PERFCTR_CHAIN;
>  }
> @@ -495,7 +509,7 @@ void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val)
>  
>  		/* PMSWINC only applies to ... SW_INC! */
>  		type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
> -		type &= ARMV8_PMU_EVTYPE_EVENT;
> +		type &= kvm_pmu_event_mask(vcpu->kvm);
>  		if (type != ARMV8_PMUV3_PERFCTR_SW_INCR)
>  			continue;
>  
> @@ -578,7 +592,7 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  	data = __vcpu_sys_reg(vcpu, reg);
>  
>  	kvm_pmu_stop_counter(vcpu, pmc);
> -	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
> +	eventsel = data & kvm_pmu_event_mask(vcpu->kvm);;
>  
>  	/* Software increment event does't need to be backed by a perf event */
>  	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
> @@ -679,17 +693,68 @@ static void kvm_pmu_update_pmc_chained(struct kvm_vcpu *vcpu, u64 select_idx)
>  void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
>  				    u64 select_idx)
>  {
> -	u64 reg, event_type = data & ARMV8_PMU_EVTYPE_MASK;
> +	u64 reg, mask;
> +
> +	mask  =  ARMV8_PMU_EVTYPE_MASK;
> +	mask &= ~ARMV8_PMU_EVTYPE_EVENT;
> +	mask |= kvm_pmu_event_mask(vcpu->kvm);
>  
>  	reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
>  	      ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + select_idx;
>  
> -	__vcpu_sys_reg(vcpu, reg) = event_type;
> +	__vcpu_sys_reg(vcpu, reg) = data & mask;
>  
>  	kvm_pmu_update_pmc_chained(vcpu, select_idx);
>  	kvm_pmu_create_perf_event(vcpu, select_idx);
>  }
>  
> +static int kvm_pmu_probe_pmuver(void)
> +{
> +	struct perf_event_attr attr = { };
> +	struct perf_event *event;
> +	struct arm_pmu *pmu;
> +	int pmuver = 0xf;
> +
> +	/*
> +	 * Create a dummy event that only counts user cycles. As we'll never
> +	 * leave thing function with the event being live, it will never
> +	 * count anything. But it allows us to probe some of the PMU
> +	 * details. Yes, this is terrible.
I fail to understand why we can't directly read ID_DFR0_EL1.PerfMon?

Thanks

Eric
> +	 */
> +	attr.type = PERF_TYPE_RAW;
> +	attr.size = sizeof(attr);
> +	attr.pinned = 1;
> +	attr.disabled = 0;
> +	attr.exclude_user = 0;
> +	attr.exclude_kernel = 1;
> +	attr.exclude_hv = 1;
> +	attr.exclude_host = 1;
> +	attr.config = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
> +	attr.sample_period = GENMASK(63, 0);
> +
> +	event = perf_event_create_kernel_counter(&attr, -1, current,
> +						 kvm_pmu_perf_overflow, &attr);
> +
> +	if (IS_ERR(event)) {
> +		pr_err_once("kvm: pmu event creation failed %ld\n",
> +			    PTR_ERR(event));
> +		return 0xf;
> +	}
> +
> +	if (event->pmu) {
> +		pmu = to_arm_pmu(event->pmu);
> +		if (pmu->pmuver)
> +			pmuver = pmu->pmuver;
> +		pr_debug("PMU on CPUs %*pbl version %x\n",
> +			 cpumask_pr_args(&pmu->supported_cpus), pmuver);
> +	}
> +
> +	perf_event_disable(event);
> +	perf_event_release_kernel(event);
> +
> +	return pmuver;
> +}
> +
>  bool kvm_arm_support_pmu_v3(void)
>  {
>  	/*
> @@ -796,6 +861,12 @@ int kvm_arm_pmu_v3_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr)
>  	if (vcpu->arch.pmu.created)
>  		return -EBUSY;
>  
> +	if (!vcpu->kvm->arch.pmuver)
> +		vcpu->kvm->arch.pmuver = kvm_pmu_probe_pmuver();
> +
> +	if (vcpu->kvm->arch.pmuver == 0xf)
> +		return -ENODEV;
> +
>  	switch (attr->attr) {
>  	case KVM_ARM_VCPU_PMU_V3_IRQ: {
>  		int __user *uaddr = (int __user *)(long)attr->addr;
> 


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

Re: [PATCH v3 2/5] KVM: arm64: Use event mask matching architecture revision

[Marc Zyngier]

Hi Eric,

On 2020-09-09 10:38, Auger Eric wrote:
> Hi Marc,
> 
> On 9/8/20 9:58 AM, Marc Zyngier wrote:
>> The PMU code suffers from a small defect where we assume that the 
>> event
>> number provided by the guest is always 16 bit wide, even if the CPU 
>> only
>> implements the ARMv8.0 architecture. This isn't really problematic in
>> the sense that the event number ends up in a system register, cropping
>> it to the right width, but still this needs fixing.
>> 
>> In order to make it work, let's probe the version of the PMU that the
>> guest is going to use. This is done by temporarily creating a kernel
>> event and looking at the PMUVer field that has been saved at probe 
>> time
>> in the associated arm_pmu structure. This in turn gets saved in the 
>> kvm
>> structure, and subsequently used to compute the event mask that gets
>> used throughout the PMU code.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/include/asm/kvm_host.h |  2 +
>>  arch/arm64/kvm/pmu-emul.c         | 81 
>> +++++++++++++++++++++++++++++--
>>  2 files changed, 78 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/arm64/include/asm/kvm_host.h 
>> b/arch/arm64/include/asm/kvm_host.h
>> index 65568b23868a..6cd60be69c28 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -110,6 +110,8 @@ struct kvm_arch {
>>  	 * supported.
>>  	 */
>>  	bool return_nisv_io_abort_to_user;
>> +
>> +	unsigned int pmuver;
>>  };
>> 
>>  struct kvm_vcpu_fault_info {
>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>> index 93d797df42c6..8a5f65763814 100644
>> --- a/arch/arm64/kvm/pmu-emul.c
>> +++ b/arch/arm64/kvm/pmu-emul.c
>> @@ -20,6 +20,20 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu 
>> *vcpu, struct kvm_pmc *pmc);
>> 
>>  #define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1
>> 
>> +static u32 kvm_pmu_event_mask(struct kvm *kvm)
>> +{
>> +	switch (kvm->arch.pmuver) {
>> +	case 1:			/* ARMv8.0 */
>> +		return GENMASK(9, 0);
>> +	case 4:			/* ARMv8.1 */
>> +	case 5:			/* ARMv8.4 */
>> +	case 6:			/* ARMv8.5 */
>> +		return GENMASK(15, 0);
>> +	default:		/* Shouldn't be there, just for sanity */
>> +		return 0;
>> +	}
>> +}
>> +
>>  /**
>>   * kvm_pmu_idx_is_64bit - determine if select_idx is a 64bit counter
>>   * @vcpu: The vcpu pointer
>> @@ -100,7 +114,7 @@ static bool kvm_pmu_idx_has_chain_evtype(struct 
>> kvm_vcpu *vcpu, u64 select_idx)
>>  		return false;
>> 
>>  	reg = PMEVTYPER0_EL0 + select_idx;
>> -	eventsel = __vcpu_sys_reg(vcpu, reg) & ARMV8_PMU_EVTYPE_EVENT;
>> +	eventsel = __vcpu_sys_reg(vcpu, reg) & 
>> kvm_pmu_event_mask(vcpu->kvm);
>> 
>>  	return eventsel == ARMV8_PMUV3_PERFCTR_CHAIN;
>>  }
>> @@ -495,7 +509,7 @@ void kvm_pmu_software_increment(struct kvm_vcpu 
>> *vcpu, u64 val)
>> 
>>  		/* PMSWINC only applies to ... SW_INC! */
>>  		type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
>> -		type &= ARMV8_PMU_EVTYPE_EVENT;
>> +		type &= kvm_pmu_event_mask(vcpu->kvm);
>>  		if (type != ARMV8_PMUV3_PERFCTR_SW_INCR)
>>  			continue;
>> 
>> @@ -578,7 +592,7 @@ static void kvm_pmu_create_perf_event(struct 
>> kvm_vcpu *vcpu, u64 select_idx)
>>  	data = __vcpu_sys_reg(vcpu, reg);
>> 
>>  	kvm_pmu_stop_counter(vcpu, pmc);
>> -	eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>> +	eventsel = data & kvm_pmu_event_mask(vcpu->kvm);;
>> 
>>  	/* Software increment event does't need to be backed by a perf event 
>> */
>>  	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>> @@ -679,17 +693,68 @@ static void kvm_pmu_update_pmc_chained(struct 
>> kvm_vcpu *vcpu, u64 select_idx)
>>  void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
>>  				    u64 select_idx)
>>  {
>> -	u64 reg, event_type = data & ARMV8_PMU_EVTYPE_MASK;
>> +	u64 reg, mask;
>> +
>> +	mask  =  ARMV8_PMU_EVTYPE_MASK;
>> +	mask &= ~ARMV8_PMU_EVTYPE_EVENT;
>> +	mask |= kvm_pmu_event_mask(vcpu->kvm);
>> 
>>  	reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
>>  	      ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + select_idx;
>> 
>> -	__vcpu_sys_reg(vcpu, reg) = event_type;
>> +	__vcpu_sys_reg(vcpu, reg) = data & mask;
>> 
>>  	kvm_pmu_update_pmc_chained(vcpu, select_idx);
>>  	kvm_pmu_create_perf_event(vcpu, select_idx);
>>  }
>> 
>> +static int kvm_pmu_probe_pmuver(void)
>> +{
>> +	struct perf_event_attr attr = { };
>> +	struct perf_event *event;
>> +	struct arm_pmu *pmu;
>> +	int pmuver = 0xf;
>> +
>> +	/*
>> +	 * Create a dummy event that only counts user cycles. As we'll never
>> +	 * leave thing function with the event being live, it will never
>> +	 * count anything. But it allows us to probe some of the PMU
>> +	 * details. Yes, this is terrible.
> I fail to understand why we can't directly read ID_DFR0_EL1.PerfMon?

Because you're missing the big-little nightmare. What you read on the
current CPU is irrelevant. You want to read the PMU version on the PMU
that is going to actually be used (and that's the whatever perf decides
to use at this point).

That's also the reason why PMU in guests only work in BL systems
on one class of CPUs only.

Yes, all CPUs should have the same PMU version. Unfortunately,
that's not always the case...

         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 v3 2/5] KVM: arm64: Use event mask matching architecture revision

[Auger Eric]

Hi Marc,

On 9/9/20 11:54 AM, Marc Zyngier wrote:
> Hi Eric,
> 
> On 2020-09-09 10:38, Auger Eric wrote:
>> Hi Marc,
>>
>> On 9/8/20 9:58 AM, Marc Zyngier wrote:
>>> The PMU code suffers from a small defect where we assume that the event
>>> number provided by the guest is always 16 bit wide, even if the CPU only
>>> implements the ARMv8.0 architecture. This isn't really problematic in
>>> the sense that the event number ends up in a system register, cropping
>>> it to the right width, but still this needs fixing.
>>>
>>> In order to make it work, let's probe the version of the PMU that the
>>> guest is going to use. This is done by temporarily creating a kernel
>>> event and looking at the PMUVer field that has been saved at probe time
>>> in the associated arm_pmu structure. This in turn gets saved in the kvm
>>> structure, and subsequently used to compute the event mask that gets
>>> used throughout the PMU code.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/include/asm/kvm_host.h |  2 +
>>>  arch/arm64/kvm/pmu-emul.c         | 81 +++++++++++++++++++++++++++++--
>>>  2 files changed, 78 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>> b/arch/arm64/include/asm/kvm_host.h
>>> index 65568b23868a..6cd60be69c28 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -110,6 +110,8 @@ struct kvm_arch {
>>>       * supported.
>>>       */
>>>      bool return_nisv_io_abort_to_user;
>>> +
>>> +    unsigned int pmuver;
>>>  };
>>>
>>>  struct kvm_vcpu_fault_info {
>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>>> index 93d797df42c6..8a5f65763814 100644
>>> --- a/arch/arm64/kvm/pmu-emul.c
>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>> @@ -20,6 +20,20 @@ static void kvm_pmu_stop_counter(struct kvm_vcpu
>>> *vcpu, struct kvm_pmc *pmc);
>>>
>>>  #define PERF_ATTR_CFG1_KVM_PMU_CHAINED 0x1
>>>
>>> +static u32 kvm_pmu_event_mask(struct kvm *kvm)
>>> +{
>>> +    switch (kvm->arch.pmuver) {
>>> +    case 1:            /* ARMv8.0 */
>>> +        return GENMASK(9, 0);
>>> +    case 4:            /* ARMv8.1 */
>>> +    case 5:            /* ARMv8.4 */
>>> +    case 6:            /* ARMv8.5 */
>>> +        return GENMASK(15, 0);
>>> +    default:        /* Shouldn't be there, just for sanity */
>>> +        return 0;
>>> +    }
>>> +}
>>> +
>>>  /**
>>>   * kvm_pmu_idx_is_64bit - determine if select_idx is a 64bit counter
>>>   * @vcpu: The vcpu pointer
>>> @@ -100,7 +114,7 @@ static bool kvm_pmu_idx_has_chain_evtype(struct
>>> kvm_vcpu *vcpu, u64 select_idx)
>>>          return false;
>>>
>>>      reg = PMEVTYPER0_EL0 + select_idx;
>>> -    eventsel = __vcpu_sys_reg(vcpu, reg) & ARMV8_PMU_EVTYPE_EVENT;
>>> +    eventsel = __vcpu_sys_reg(vcpu, reg) &
>>> kvm_pmu_event_mask(vcpu->kvm);
>>>
>>>      return eventsel == ARMV8_PMUV3_PERFCTR_CHAIN;
>>>  }
>>> @@ -495,7 +509,7 @@ void kvm_pmu_software_increment(struct kvm_vcpu
>>> *vcpu, u64 val)
>>>
>>>          /* PMSWINC only applies to ... SW_INC! */
>>>          type = __vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i);
>>> -        type &= ARMV8_PMU_EVTYPE_EVENT;
>>> +        type &= kvm_pmu_event_mask(vcpu->kvm);
>>>          if (type != ARMV8_PMUV3_PERFCTR_SW_INCR)
>>>              continue;
>>>
>>> @@ -578,7 +592,7 @@ static void kvm_pmu_create_perf_event(struct
>>> kvm_vcpu *vcpu, u64 select_idx)
>>>      data = __vcpu_sys_reg(vcpu, reg);
>>>
>>>      kvm_pmu_stop_counter(vcpu, pmc);
>>> -    eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
>>> +    eventsel = data & kvm_pmu_event_mask(vcpu->kvm);;
>>>
>>>      /* Software increment event does't need to be backed by a perf
>>> event */
>>>      if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
>>> @@ -679,17 +693,68 @@ static void kvm_pmu_update_pmc_chained(struct
>>> kvm_vcpu *vcpu, u64 select_idx)
>>>  void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
>>>                      u64 select_idx)
>>>  {
>>> -    u64 reg, event_type = data & ARMV8_PMU_EVTYPE_MASK;
>>> +    u64 reg, mask;
>>> +
>>> +    mask  =  ARMV8_PMU_EVTYPE_MASK;
>>> +    mask &= ~ARMV8_PMU_EVTYPE_EVENT;
>>> +    mask |= kvm_pmu_event_mask(vcpu->kvm);
>>>
>>>      reg = (select_idx == ARMV8_PMU_CYCLE_IDX)
>>>            ? PMCCFILTR_EL0 : PMEVTYPER0_EL0 + select_idx;
>>>
>>> -    __vcpu_sys_reg(vcpu, reg) = event_type;
>>> +    __vcpu_sys_reg(vcpu, reg) = data & mask;
>>>
>>>      kvm_pmu_update_pmc_chained(vcpu, select_idx);
>>>      kvm_pmu_create_perf_event(vcpu, select_idx);
>>>  }
>>>
>>> +static int kvm_pmu_probe_pmuver(void)
>>> +{
>>> +    struct perf_event_attr attr = { };
>>> +    struct perf_event *event;
>>> +    struct arm_pmu *pmu;
>>> +    int pmuver = 0xf;
>>> +
>>> +    /*
>>> +     * Create a dummy event that only counts user cycles. As we'll
>>> never
>>> +     * leave thing function with the event being live, it will never
>>> +     * count anything. But it allows us to probe some of the PMU
>>> +     * details. Yes, this is terrible.
>> I fail to understand why we can't directly read ID_DFR0_EL1.PerfMon?
> 
> Because you're missing the big-little nightmare. What you read on the
> current CPU is irrelevant. You want to read the PMU version on the PMU
> that is going to actually be used (and that's the whatever perf decides
> to use at this point).
> 
> That's also the reason why PMU in guests only work in BL systems
> on one class of CPUs only.
> 
> Yes, all CPUs should have the same PMU version. Unfortunately,
> that's not always the case...

Ah OK. I Forgot this...

Thanks

Eric
> 
>         M.


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

Re: [PATCH v3 3/5] KVM: arm64: Add PMU event filtering infrastructure

[Auger Eric]

Hi Marc,

On 9/8/20 9:58 AM, 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 |  5 +++
>  arch/arm64/include/uapi/asm/kvm.h | 16 +++++++
>  arch/arm64/kvm/arm.c              |  2 +
>  arch/arm64/kvm/pmu-emul.c         | 70 ++++++++++++++++++++++++++++---
>  4 files changed, 87 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 6cd60be69c28..1e64260b7e2b 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -111,6 +111,11 @@ struct kvm_arch {
>  	 */
>  	bool return_nisv_io_abort_to_user;
>  
> +	/*
> +	 * VM-wide PMU filter, implemented as a bitmap and big enough for
> +	 * up to 2^10 events (ARMv8.0) or 2^16 events (ARMv8.1+).
> +	 */
> +	unsigned long *pmu_filter;
>  	unsigned int pmuver;
>  };
>  
> 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/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 691d21e4c717..0f11d0009c17 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -145,6 +145,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>  {
>  	int i;
>  
> +	bitmap_free(kvm->arch.pmu_filter);
> +
>  	kvm_vgic_destroy(kvm);
>  
>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 8a5f65763814..67a731bafbc9 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -30,6 +30,7 @@ static u32 kvm_pmu_event_mask(struct kvm *kvm)
>  	case 6:			/* ARMv8.5 */
>  		return GENMASK(15, 0);
>  	default:		/* Shouldn't be there, just for sanity */
> +		WARN_ONCE(1, "Unknown PMU version %d\n", kvm->arch.pmuver);
rather belongs to previous patch
>  		return 0;
>  	}
>  }
> @@ -592,11 +593,21 @@ static void kvm_pmu_create_perf_event(struct kvm_vcpu *vcpu, u64 select_idx)
>  	data = __vcpu_sys_reg(vcpu, reg);
>  
>  	kvm_pmu_stop_counter(vcpu, pmc);
> -	eventsel = data & kvm_pmu_event_mask(vcpu->kvm);;
> +	if (pmc->idx == ARMV8_PMU_CYCLE_IDX)
> +		eventsel = ARMV8_PMUV3_PERFCTR_CPU_CYCLES;
So from a filter pov the cycle counter is assimilated to the event
counter CPU_CYCLES event (0x11). I see there are some differences
between PMCCNTR and 0x11 counting (wrt PMCR, ...) though. Shouldn't we
mention the cycle counter is getting filtered as well
> +	else
> +		eventsel = data & kvm_pmu_event_mask(vcpu->kvm);
> +
> +	/* Software increment event doesn't need to be backed by a perf event */
> +	if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR)
> +		return;
>  
> -	/* 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 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));
> @@ -608,8 +619,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);
>  
> @@ -892,6 +902,53 @@ 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;
> +		int nr_events;
> +
> +		nr_events = kvm_pmu_event_mask(vcpu->kvm) + 1;
> +
> +		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);
>  	}
> @@ -928,6 +985,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;
> 
Thanks

Eric


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

Re: [PATCH v3 4/5] KVM: arm64: Mask out filtered events in PCMEID{0,1}_EL1

[Auger Eric]

Hi Marc,

On 9/8/20 9:58 AM, Marc Zyngier wrote:
> As we can now hide events from the guest, let's also adjust its view of
> PCMEID{0,1}_EL1 so that it can figure out why some common events are not
> counting as they should.
Referring to my previous comment should we filter the cycle counter out?
> 
> The astute user can still look into the TRM for their CPU and find out
> they've been cheated, though. Nobody's perfect.
> 
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  arch/arm64/kvm/pmu-emul.c | 29 +++++++++++++++++++++++++++++
>  arch/arm64/kvm/sys_regs.c |  5 +----
>  include/kvm/arm_pmu.h     |  5 +++++
>  3 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
> index 67a731bafbc9..0458860bade2 100644
> --- a/arch/arm64/kvm/pmu-emul.c
> +++ b/arch/arm64/kvm/pmu-emul.c
> @@ -765,6 +765,35 @@ static int kvm_pmu_probe_pmuver(void)
>  	return pmuver;
>  }
>  
> +u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
> +{
> +	unsigned long *bmap = vcpu->kvm->arch.pmu_filter;
> +	u64 val, mask = 0;
> +	int base, i;
> +
> +	if (!pmceid1) {
> +		val = read_sysreg(pmceid0_el0);
> +		base = 0;
> +	} else {
> +		val = read_sysreg(pmceid1_el0);
> +		base = 32;
> +	}
> +
> +	if (!bmap)
> +		return val;
> +
> +	for (i = 0; i < 32; i += 8) {
s/32/4?

Thanks

Eric
> +		u64 byte;
> +
> +		byte = bitmap_get_value8(bmap, base + i);
> +		mask |= byte << i;
> +		byte = bitmap_get_value8(bmap, 0x4000 + base + i);
> +		mask |= byte << (32 + i);
> +	}
> +
> +	return val & mask;
> +}
> +
>  bool kvm_arm_support_pmu_v3(void)
>  {
>  	/*
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 077293b5115f..20ab2a7d37ca 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -769,10 +769,7 @@ static bool access_pmceid(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
>  	if (pmu_access_el0_disabled(vcpu))
>  		return false;
>  
> -	if (!(p->Op2 & 1))
> -		pmceid = read_sysreg(pmceid0_el0);
> -	else
> -		pmceid = read_sysreg(pmceid1_el0);
> +	pmceid = kvm_pmu_get_pmceid(vcpu, (p->Op2 & 1));
>  
>  	p->regval = pmceid;
>  
> diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
> index 6db030439e29..98cbfe885a53 100644
> --- a/include/kvm/arm_pmu.h
> +++ b/include/kvm/arm_pmu.h
> @@ -34,6 +34,7 @@ struct kvm_pmu {
>  u64 kvm_pmu_get_counter_value(struct kvm_vcpu *vcpu, u64 select_idx);
>  void kvm_pmu_set_counter_value(struct kvm_vcpu *vcpu, u64 select_idx, u64 val);
>  u64 kvm_pmu_valid_counter_mask(struct kvm_vcpu *vcpu);
> +u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1);
>  void kvm_pmu_vcpu_init(struct kvm_vcpu *vcpu);
>  void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu);
>  void kvm_pmu_vcpu_destroy(struct kvm_vcpu *vcpu);
> @@ -108,6 +109,10 @@ static inline int kvm_arm_pmu_v3_enable(struct kvm_vcpu *vcpu)
>  {
>  	return 0;
>  }
> +static inline u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
> +{
> +	return 0;
> +}
>  #endif
>  
>  #endif
> 


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

Re: [PATCH v3 5/5] KVM: arm64: Document PMU filtering API

[Auger Eric]

Hi Marc,

On 9/8/20 9:58 AM, 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 | 46 +++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/Documentation/virt/kvm/devices/vcpu.rst b/Documentation/virt/kvm/devices/vcpu.rst
> index ca374d3fe085..203b91e93151 100644
> --- a/Documentation/virt/kvm/devices/vcpu.rst
> +++ b/Documentation/virt/kvm/devices/vcpu.rst
> @@ -55,6 +55,52 @@ 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
> +	 -EINVAL: Invalid filter range
> +	 =======  ======================================================
> +
> +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 event space defined by the PMU
> +architecture (10 bits on ARMv8.0, 16 bits from ARMv8.1 onwards).
> +
> +Note: "Cancelling" a filter by registering the opposite action for the same
> +range doesn't change the default action. For example, installing an ALLOW
> +filter for event range [0:10] as the first filter and then applying a DENY
> +action for the same range will leave the whole range as disabled.
> +
> +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).
Oh I see here you did comment it in the uapi.

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 v3 4/5] KVM: arm64: Mask out filtered events in PCMEID{0,1}_EL1

[Marc Zyngier]

Hi Eric,

On 2020-09-09 18:43, Auger Eric wrote:
> Hi Marc,
> 
> On 9/8/20 9:58 AM, Marc Zyngier wrote:
>> As we can now hide events from the guest, let's also adjust its view 
>> of
>> PCMEID{0,1}_EL1 so that it can figure out why some common events are 
>> not
>> counting as they should.
> Referring to my previous comment should we filter the cycle counter 
> out?
>> 
>> The astute user can still look into the TRM for their CPU and find out
>> they've been cheated, though. Nobody's perfect.
>> 
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  arch/arm64/kvm/pmu-emul.c | 29 +++++++++++++++++++++++++++++
>>  arch/arm64/kvm/sys_regs.c |  5 +----
>>  include/kvm/arm_pmu.h     |  5 +++++
>>  3 files changed, 35 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>> index 67a731bafbc9..0458860bade2 100644
>> --- a/arch/arm64/kvm/pmu-emul.c
>> +++ b/arch/arm64/kvm/pmu-emul.c
>> @@ -765,6 +765,35 @@ static int kvm_pmu_probe_pmuver(void)
>>  	return pmuver;
>>  }
>> 
>> +u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
>> +{
>> +	unsigned long *bmap = vcpu->kvm->arch.pmu_filter;
>> +	u64 val, mask = 0;
>> +	int base, i;
>> +
>> +	if (!pmceid1) {
>> +		val = read_sysreg(pmceid0_el0);
>> +		base = 0;
>> +	} else {
>> +		val = read_sysreg(pmceid1_el0);
>> +		base = 32;
>> +	}
>> +
>> +	if (!bmap)
>> +		return val;
>> +
>> +	for (i = 0; i < 32; i += 8) {
> s/32/4?

I don't think so, see below.

> 
> Thanks
> 
> Eric
>> +		u64 byte;
>> +
>> +		byte = bitmap_get_value8(bmap, base + i);
>> +		mask |= byte << i;

For each iteration of the loop, we read a byte from the bitmap
(hence the += 8 above), and orr it into the mask. This makes 4
iteration of the loop.

Or am I missing your point entirely?

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 v3 4/5] KVM: arm64: Mask out filtered events in PCMEID{0,1}_EL1

[Auger Eric]

Hi Marc,

On 9/9/20 7:50 PM, Marc Zyngier wrote:
> Hi Eric,
> 
> On 2020-09-09 18:43, Auger Eric wrote:
>> Hi Marc,
>>
>> On 9/8/20 9:58 AM, Marc Zyngier wrote:
>>> As we can now hide events from the guest, let's also adjust its view of
>>> PCMEID{0,1}_EL1 so that it can figure out why some common events are not
>>> counting as they should.
>> Referring to my previous comment should we filter the cycle counter out?
>>>
>>> The astute user can still look into the TRM for their CPU and find out
>>> they've been cheated, though. Nobody's perfect.
>>>
>>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>>> ---
>>>  arch/arm64/kvm/pmu-emul.c | 29 +++++++++++++++++++++++++++++
>>>  arch/arm64/kvm/sys_regs.c |  5 +----
>>>  include/kvm/arm_pmu.h     |  5 +++++
>>>  3 files changed, 35 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm64/kvm/pmu-emul.c b/arch/arm64/kvm/pmu-emul.c
>>> index 67a731bafbc9..0458860bade2 100644
>>> --- a/arch/arm64/kvm/pmu-emul.c
>>> +++ b/arch/arm64/kvm/pmu-emul.c
>>> @@ -765,6 +765,35 @@ static int kvm_pmu_probe_pmuver(void)
>>>      return pmuver;
>>>  }
>>>
>>> +u64 kvm_pmu_get_pmceid(struct kvm_vcpu *vcpu, bool pmceid1)
>>> +{
>>> +    unsigned long *bmap = vcpu->kvm->arch.pmu_filter;
>>> +    u64 val, mask = 0;
>>> +    int base, i;
>>> +
>>> +    if (!pmceid1) {
>>> +        val = read_sysreg(pmceid0_el0);
>>> +        base = 0;
>>> +    } else {
>>> +        val = read_sysreg(pmceid1_el0);
>>> +        base = 32;
>>> +    }
>>> +
>>> +    if (!bmap)
>>> +        return val;
>>> +
>>> +    for (i = 0; i < 32; i += 8) {
>> s/32/4?
> 
> I don't think so, see below.
> 
>>
>> Thanks
>>
>> Eric
>>> +        u64 byte;
>>> +
>>> +        byte = bitmap_get_value8(bmap, base + i);
>>> +        mask |= byte << i;
> 
> For each iteration of the loop, we read a byte from the bitmap
> (hence the += 8 above), and orr it into the mask. This makes 4
> iteration of the loop.
> 
> Or am I missing your point entirely?

Hum no you're right. Sorry for the noise.

Looks good to me:

Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric


> 
> Thanks,
> 
>         M.


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