Re: [PATCH 2/9] KVM: Expose a version 2 architectural PMU to a guests

[Avi Kivity <avi@redhat.com>]

On 10/30/2011 06:53 PM, Gleb Natapov wrote:
> From: Avi Kivity <avi@redhat.com>

This has changed significantly, so please update the authorship.  You
can say 'based on original patch by ...' to provide due credit.

> Use perf_events to emulate an architectural PMU, version 2.

> +
> +/* mapping between fixed pmc index and arch_events array */
> +int fixed_pmc_events[] = {1, 0, 2};
> +
> +static bool pmc_is_gp(struct kvm_pmc *pmc)
> +{
> +	return pmc->type == KVM_PMC_GP;
> +}
> +
> +static inline u64 pmc_bitmask(struct kvm_pmc *pmc)
> +{
> +	struct kvm_pmu *pmu = &pmc->vcpu->arch.pmu;
> +
> +	return pmc_is_gp(pmc) ? pmu->gp_counter_bitmask :
> +		pmu->fixed_counter_bitmask;
> +}

Nicer to just push the bitmask (or bitwidth) into the counter itself.

> +
> +static inline int pmc_to_global_idx(struct kvm_pmc *pmc)
> +{
> +	struct kvm_pmu *pmu = &pmc->vcpu->arch.pmu;
> +	struct kvm_pmc *counters;
> +	int shift;
> +
> +	if (pmc_is_gp(pmc)) {
> +		counters = pmu->gp_counters;
> +		shift = X86_PMC_IDX_GENERIC;
> +	} else {
> +		counters = pmu->fixed_counters;
> +		shift = X86_PMC_IDX_FIXED;
> +	}
> +
> +	return pmc - counters + shift;
> +}

Again, push the global index into struct kvm_pmc.

> +
> +static void kvm_perf_overflow(struct perf_event *perf_event,
> +			      struct perf_sample_data *data,
> +			      struct pt_regs *regs)
> +{
> +	struct kvm_pmc *pmc = perf_event->overflow_handler_context;
> +	struct kvm_pmu *pmu = &pmc->vcpu->arch.pmu;
> +	__set_bit(pmc_to_global_idx(pmc),
> +			(unsigned long *)&pmu->global_status);
> +}
> +
> +static void kvm_perf_overflow_intr(struct perf_event *perf_event,
> +		struct perf_sample_data *data, struct pt_regs *regs)
> +{
> +	struct kvm_pmc *pmc = perf_event->overflow_handler_context;
> +	struct kvm_pmu *pmu = &pmc->vcpu->arch.pmu;
> +	if (!__test_and_set_bit(pmc_to_global_idx(pmc),
> +				(unsigned long *)&pmu->reprogram_pmi)) {
> +		kvm_perf_overflow(perf_event, data, regs);
> +		kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
> +	}
> +}

Is it safe to use the __ versions here?

Do we need to follow kvm_make_request() with kvm_vcpu_kick()?  If there
is a skew between the overflow and the host PMI, the guest might have
executed a HLT.

> +
> +static void reprogram_fixed_counter(struct kvm_pmc *pmc, u8 en_pmi, int idx)
> +{
> +	unsigned en = en_pmi & 0x3;
> +	bool pmi = en_pmi & 0x8;
> +
> +	stop_counter(pmc);
> +
> +	if (!en || !pmc_enabled(pmc))
> +		return;
> +
> +	reprogram_counter(pmc, PERF_TYPE_HARDWARE,
> +			arch_events[fixed_pmc_events[idx]].event_type,
> +			!(en & 0x2), /* exclude user */
> +			!(en & 0x1), /* exclude kernel */
> +			pmi);

Are there no #defines for those constants?

> +}
> +
> +#define FIXED_EN_PMI(R, I) (((R) >> ((I) * 4)) & 0xf)

function

> +	default:
> +		if ((pmc = get_gp_pmc(pmu, index, MSR_IA32_PERFCTR0)) ||
> +				(pmc = get_fixed_pmc(pmu, index))) {
> +			data = (s64)(s32)data;
> +			pmc->counter += data - read_pmc(pmc);
> +			return 0;
> +		} else if ((pmc = get_gp_pmc(pmu, index, MSR_P6_EVNTSEL0))) {
> +			if (data == pmc->eventsel)
> +				return 0;
> +			if (!(data & 0xffffffff00200000ull)) {
> +				reprogram_gp_counter(pmc, data);
> +				return 0;
> +			}
> +		}
> +	}
> +	return 1;
> +}
> +
> +
> +void kvm_pmu_cpuid_update(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_pmu *pmu = &vcpu->arch.pmu;
> +	struct kvm_cpuid_entry2 *entry;
> +	unsigned bitmap_len;
> +
> +	pmu->nr_arch_gp_counters = 0;
> +	pmu->nr_arch_fixed_counters = 0;
> +	pmu->fixed_counter_bitmask = 0;
> +	pmu->version = 0;
> +
> +	entry = kvm_find_cpuid_entry(vcpu, 0xa, 0);
> +	if (!entry)
> +		return;
> +
> +	pmu->version = entry->eax & 0xff;
> +	if (!pmu->version)
> +		return;
> +
> +	pmu->nr_arch_gp_counters = min((int)(entry->eax >> 8) & 0xff,
> +			X86_PMC_MAX_GENERIC);
> +	pmu->gp_counter_bitmask = ((u64)1 << ((entry->eax >> 16) & 0xff)) - 1;
> +	bitmap_len = (entry->eax >> 24) & 0xff;
> +	pmu->available_event_types = ~entry->ebx & ((1ull << bitmap_len) - 1);
> +
> +	if (pmu->version > 1) {
> +		pmu->nr_arch_fixed_counters = min((int)(entry->edx) & 0x1f,
> +				X86_PMC_MAX_FIXED);

Misplaced parentheses (though no effect on generated code).

> +		pmu->fixed_counter_bitmask =
> +			((u64)1 << ((entry->edx >> 5) & 0xff)) - 1;

The user can cause this to be very small (even zero).  Can this cause an
NMI storm?

> +		pmu->global_ctrl_mask = ~(((1 << pmu->nr_arch_gp_counters) - 1)
> +				| (((1ull << pmu->nr_arch_fixed_counters) - 1)
> +					<< X86_PMC_IDX_FIXED));
> +	} else
> +		pmu->global_ctrl = (1 << pmu->nr_arch_gp_counters) - 1;
> +}

Nicer to just return early if version < 2; less indentation and easier
to prepare for version 3.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.