Re: [PATCH 07/18] KVM: ARM64: PMU: Add perf event map and introduce perf event creating function

[Shannon Zhao <shannon.zhao@linaro.org>]

On 2015/7/17 22:30, Christoffer Dall wrote:
> On Mon, Jul 06, 2015 at 10:17:37AM +0800, shannon.zhao@linaro.org wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> 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.

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

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


>> 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 <shannon.zhao@linaro.org>
>> ---
>>  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.

> Is this because perf map multiple generic perf events to the same
> hardware event? 
I think so.

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


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

>> +}
>> +
>>  /**
>>   * 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
>> +}
>> +
>> +/**
>> + * 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?
> 
>> +
>> +	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.
> 
>> + */
>> +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
> otherwise, how do you ensure that for example exclude_guest is always
> clear?
> 
>> +
>> +	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.

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

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

>> +		return;
>> +	}
>> +	pmc->perf_event = event;
>> +}
>> +
>> +/**
>>   * kvm_pmu_init - Initialize global PMU state for per vcpu
>>   * @vcpu: The vcpu pointer
>>   *
>> -- 
>> 2.1.0
>>

-- 
Shannon