On Tue, Jul 21, 2015 at 09:35:03AM +0800, Shannon Zhao wrote:

On 2015/7/17 22:30, Christoffer Dall wrote:

quoted

On Mon, Jul 06, 2015 at 10:17:37AM +0800, shannon.zhao at linaro.org wrote:

quoted

From: Shannon Zhao <redacted>

When we use tools like perf on host, perf passes the event type and the
id in this type category to kernel, then kernel will map them to event
number and write this number to PMU PMEVTYPER<n>_EL0 register. While
we're trapping and emulating guest accesses to PMU registers, we get the
event number and map it to the event type and the id reversely.

There's something with the nomenclature that makes this really hard to
read.

you mention here: "event type", "the id", and "event number".  The
former two I think are perf identifiers, and the latter is the hardware
event number, is this right?

Yeah, right.

ok, if we can clarify our nomenclature throughout here I think that
would make the patches easier to understand/read.

quoted

quoted

Check whether the event type is same with the one to be set.

when?

In function kvm_pmu_set_counter_event_type before create a new perf event.

do you mean when the guest configures some counter and we we create a
perf event accordingly?

If so, I think this could be clarified in the commit text.

quoted

quoted

+	if ((data & ARMV8_EVTYPE_EVENT) == pmc->eventsel)
+		return;

quoted

quoted

If not,
stop counter to monitor current event and find the event type map id.
According to the bits of data to configure this perf_event attr and
set exclude_host to 1 for guest. Then call perf_event API to create the
corresponding event and save the event pointer.

Signed-off-by: Shannon Zhao <redacted>
---
 include/kvm/arm_pmu.h |   4 ++
 virt/kvm/arm/pmu.c    | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 177 insertions(+)

diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 27d14ca..1050b24 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h

@@ -45,9 +45,13 @@ struct kvm_pmu {
 
 #ifdef CONFIG_KVM_ARM_PMU
 void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu);
+void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, unsigned long data,
+				    unsigned long select_idx);
 void kvm_pmu_init(struct kvm_vcpu *vcpu);
 #else
 static inline void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu) {}
+void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, unsigned long data,
+				    unsigned long select_idx) {}
 static inline void kvm_pmu_init(struct kvm_vcpu *vcpu) {}
 #endif
 

diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index dc252d0..50a3c82 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c

@@ -18,8 +18,68 @@
 #include <linux/cpu.h>
 #include <linux/kvm.h>
 #include <linux/kvm_host.h>
+#include <linux/perf_event.h>
 #include <kvm/arm_pmu.h>
 
+/* PMU HW events mapping. */
+static struct kvm_pmu_hw_event_map {
+	unsigned eventsel;
+	unsigned event_type;
+} kvm_pmu_hw_events[] = {
+	[0] = { 0x11, PERF_COUNT_HW_CPU_CYCLES },
+	[1] = { 0x08, PERF_COUNT_HW_INSTRUCTIONS },
+	[2] = { 0x04, PERF_COUNT_HW_CACHE_REFERENCES },
+	[3] = { 0x03, PERF_COUNT_HW_CACHE_MISSES },
+	[4] = { 0x10, PERF_COUNT_HW_BRANCH_MISSES },
+};
+
+/* PMU HW cache events mapping. */
+static struct kvm_pmu_hw_cache_event_map {
+	unsigned eventsel;
+	unsigned cache_type;
+	unsigned cache_op;
+	unsigned cache_result;
+} kvm_pmu_hw_cache_events[] = {
+	[0] = { 0x04, PERF_COUNT_HW_CACHE_L1D, PERF_COUNT_HW_CACHE_OP_READ,
+		      PERF_COUNT_HW_CACHE_RESULT_ACCESS },
+	[1] = { 0x03, PERF_COUNT_HW_CACHE_L1D, PERF_COUNT_HW_CACHE_OP_READ,
+		      PERF_COUNT_HW_CACHE_RESULT_MISS },
+	[2] = { 0x04, PERF_COUNT_HW_CACHE_L1D, PERF_COUNT_HW_CACHE_OP_WRITE,
+		      PERF_COUNT_HW_CACHE_RESULT_ACCESS },
+	[3] = { 0x03, PERF_COUNT_HW_CACHE_L1D, PERF_COUNT_HW_CACHE_OP_WRITE,
+		      PERF_COUNT_HW_CACHE_RESULT_MISS },

seems to me that the four entries above will never be used, because
you'll always match them in kvm_pmu_hw_events above?

Yes, I found this before, but for the completeness I list them.

hmm, having unused entries for completeness is a bit weird, IMHO.
Either way, a comment explaining why they're missing or that some are
never used would be helpful.

quoted

Is this because perf map multiple generic perf events to the same
hardware event? 

I think so.

quoted

Does it matter if we register this with perf as one or
the other then?

I think it's ok because the hardware event numbers are same.

ok

quoted

quoted

+	[4] = { 0x12, PERF_COUNT_HW_CACHE_BPU, PERF_COUNT_HW_CACHE_OP_READ,
+		      PERF_COUNT_HW_CACHE_RESULT_ACCESS },
+	[5] = { 0x10, PERF_COUNT_HW_CACHE_BPU, PERF_COUNT_HW_CACHE_OP_READ,
+		      PERF_COUNT_HW_CACHE_RESULT_MISS },
+	[6] = { 0x12, PERF_COUNT_HW_CACHE_BPU, PERF_COUNT_HW_CACHE_OP_WRITE,
+		      PERF_COUNT_HW_CACHE_RESULT_ACCESS },
+	[7] = { 0x10, PERF_COUNT_HW_CACHE_BPU, PERF_COUNT_HW_CACHE_OP_WRITE,
+		      PERF_COUNT_HW_CACHE_RESULT_MISS },
+};
+
+/**
+ * kvm_pmu_stop_counter - stop PMU counter for the selected counter
+ * @vcpu: The vcpu pointer
+ * @select_idx: The counter index
+ *
+ * If this counter has been configured to monitor some event, disable and
+ * release it.
+ */
+static void kvm_pmu_stop_counter(struct kvm_vcpu *vcpu,
+				 unsigned long select_idx)
+{
+	struct kvm_pmu *pmu = &vcpu->arch.pmu;
+	struct kvm_pmc *pmc = &pmu->pmc[select_idx];
+
+	if (pmc->perf_event) {
+		perf_event_disable(pmc->perf_event);
+		perf_event_release_kernel(pmc->perf_event);
+	}
+	pmc->perf_event = NULL;
+	pmc->eventsel = 0xff;

why is 0xff 'unused' or reserved?  If we're choosing this arbitrarily,
why is it not 0x3ff?  Should we create a define for this?

Yeah, 0x3ff may be better.

quoted

quoted

+}
+
 /**
  * kvm_pmu_vcpu_reset - reset pmu state for cpu
  * @vcpu: The vcpu pointer

@@ -27,12 +87,125 @@
  */
 void kvm_pmu_vcpu_reset(struct kvm_vcpu *vcpu)
 {
+	int i;
 	struct kvm_pmu *pmu = &vcpu->arch.pmu;
 
+	for (i = 0; i < ARMV8_MAX_COUNTERS; i++)
+		kvm_pmu_stop_counter(vcpu, i);
+	pmu->overflow_status = 0;
 	pmu->irq_pending = false;
 }
 
 /**
+ * kvm_pmu_find_hw_event - find hardware event
+ * @pmu: The pmu pointer
+ * @event_select: The number of selected event type
+ *
+ * Based on the number of selected event type, find out whether it belongs to
+ * PERF_TYPE_HARDWARE. If so, return the corresponding event id.
+ */
+static unsigned kvm_pmu_find_hw_event(struct kvm_pmu *pmu,
+				      unsigned long event_select)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(kvm_pmu_hw_events); i++)
+		if (kvm_pmu_hw_events[i].eventsel == event_select)
+			break;
+
+	if (i == ARRAY_SIZE(kvm_pmu_hw_events))
+		return PERF_COUNT_HW_MAX;
+
+	return kvm_pmu_hw_events[i].event_type;

you can just return this directly in the loop if you have a match and
unconditionally return PERF_COUNT_HW_MAX outside the loop without having
to check the loop condition.

ok

quoted

quoted

+}
+
+/**
+ * kvm_pmu_find_hw_cache_event - find hardware cache event
+ * @pmu: The pmu pointer
+ * @event_select: The number of selected event type
+ *
+ * Based on the number of selected event type, find out whether it belongs to
+ * PERF_TYPE_HW_CACHE. If so, return the corresponding event id.
+ */
+static unsigned kvm_pmu_find_hw_cache_event(struct kvm_pmu *pmu,
+					    unsigned long event_select)
+{
+	int i;
+	unsigned config;
+
+	for (i = 0; i < ARRAY_SIZE(kvm_pmu_hw_cache_events); i++)
+		if (kvm_pmu_hw_cache_events[i].eventsel == event_select)
+			break;
+
+	if (i == ARRAY_SIZE(kvm_pmu_hw_cache_events))
+		return PERF_COUNT_HW_CACHE_MAX;

I feel like I just read this code, can we reuse it with a pointer to the
array?

quoted

+
+	config = (kvm_pmu_hw_cache_events[i].cache_type & 0xff)
+		 | ((kvm_pmu_hw_cache_events[i].cache_op & 0xff) << 8)
+		 | ((kvm_pmu_hw_cache_events[i].cache_result & 0xff) << 16);
+
+	return config;
+}
+
+/**
+ * kvm_pmu_set_counter_event_type - set selected counter to monitor some event
+ * @vcpu: The vcpu pointer
+ * @data: The data guest writes to PMXEVTYPER_EL0
+ * @select_idx: The number of selected counter
+ *
+ * Firstly check whether the event type is same with the one to be set.
+ * If not, stop counter to monitor current event and find the event type map id.
+ * According to the bits of data to configure this perf_event attr and set
+ * exclude_host to 1 for guest. Then call perf_event API to create the
+ * corresponding event and save the event pointer.

This text seems to be describing more how the function does something,
as opposed to what it does and why.  I found it a little hard to read.

quoted

+ */
+void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, unsigned long data,
+				    unsigned long select_idx)
+{
+	struct kvm_pmu *pmu = &vcpu->arch.pmu;
+	struct kvm_pmc *pmc = &pmu->pmc[select_idx];
+	struct perf_event *event;
+	struct perf_event_attr attr;
+	unsigned config, type = PERF_TYPE_RAW;
+
+	if ((data & ARMV8_EVTYPE_EVENT) == pmc->eventsel)
+		return;
+
+	kvm_pmu_stop_counter(vcpu, select_idx);
+	pmc->eventsel = data & ARMV8_EVTYPE_EVENT;
+
+	config = kvm_pmu_find_hw_event(pmu, pmc->eventsel);
+	if (config != PERF_COUNT_HW_MAX) {
+		type = PERF_TYPE_HARDWARE;
+	} else {
+		config = kvm_pmu_find_hw_cache_event(pmu, pmc->eventsel);
+		if (config != PERF_COUNT_HW_CACHE_MAX)
+			type = PERF_TYPE_HW_CACHE;
+	}
+
+	if (type == PERF_TYPE_RAW)
+		config = pmc->eventsel;

don't you need to memset attr to 0 first?

ok

quoted

otherwise, how do you ensure that for example exclude_guest is always
clear?

quoted

+
+	attr.type = type;
+	attr.size = sizeof(attr);
+	attr.pinned = true;
+	attr.exclude_user = data & ARMV8_EXCLUDE_EL0 ? 1 : 0;
+	attr.exclude_kernel = data & ARMV8_EXCLUDE_EL1 ? 1 : 0;
+	attr.exclude_hv = data & ARMV8_INCLUDE_EL2 ? 0 : 1;

should the guest be able to see something counted in the hypervisor ever
or should that only be the host being able to see that?

my gut feeling is that the hypervisor should be hidden from the guest
and that exclude_hv = 0, is the right choice.  But this is a question
about the semantics of perf, I suppose.

This is what I'm not sure. I just thought about if guest has complete
ELs(EL3-EL0) and how to deal with nested virtualization.

nested virtualization is really not very likely to happen on arm64.  I
think the right way to look at this is that we're emulating a platform
without EL2 and therefore the guest should not see any events from EL2.

quoted

quoted

+	attr.exclude_host = 1;
+	attr.config = config;
+	attr.sample_period = (-pmc->counter) & (((u64)1 << 32) - 1);

whoa, what is this scary calculation?

definitely needs an explanation?

quoted

+
+	event = perf_event_create_kernel_counter(&attr, -1, current, NULL, pmc);
+	if (IS_ERR(event)) {
+		kvm_err("kvm: pmu event creation failed %ld\n",
+			  PTR_ERR(event));

doesn't this mean we'll spam the kernel log if the guest supplies
bogus/unsupported event numbers?

X86 uses printk_once, is that ok to us here?

yeah, that should be fine.

quoted

In that case it shoudl be kvm_debug and the guest should be able to see
this somehow (e.g. events don't count).

Yes, will think about this.

printk_once of kvm_debug would be fine.

Thanks,
-Christoffer