Skip to contentSkip to conversation

KVM in-guest performance monitoring

PATCH In review Gleb Natapov 8 participants 42 messages Oct 30, 2011 – Dec 26, 2011

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover KVM in-guest performance monitoring

This patchset exposes an emulated version 2 architectural performance monitoring unit to KVM guests. The PMU is emulated using perf_events, so the host kernel can multiplex host-wide, host-user, and the guest on available resources.

The patches should be applied on top of KVM patches from the patch series "[PATCH v2 0/9] perf support for x86 guest/host-only bits" [1]

If you want to try running perf in a guest you need to apply the patch below to qemu-kvm and use -cpu host on qemu command line. But DO NOT TRY those patches without applying [2][3] to the host kernel first. Don't tell me I didn't warn you!

[1] https://lkml.org/lkml/2011/10/5/153
[2] https://lkml.org/lkml/2011/10/18/390
[3] https://lkml.org/lkml/2011/10/23/163

Avi Kivity (8):
KVM: Expose kvm_lapic_local_deliver()
KVM: Expose a version 2 architectural PMU to a guests
KVM: Add generic RDPMC support
KVM: SVM: Intercept RDPMC
KVM: VMX: Intercept RDPMC
KVM: Expose the architectural performance monitoring CPUID leaf
KVM: x86 emulator: fix RDPMC privilege check
KVM: x86 emulator: implement RDPMC (0F 33)

Gleb Natapov (1):
perf: expose perf capability to other modules.

 arch/x86/include/asm/kvm_emulate.h     |    1 +
 arch/x86/include/asm/kvm_host.h        |   44 +++
 arch/x86/include/asm/perf_event.h      |   11 +
 arch/x86/kernel/cpu/perf_event.c       |   11 +
 arch/x86/kernel/cpu/perf_event.h       |    2 +
 arch/x86/kernel/cpu/perf_event_intel.c |    3 +
 arch/x86/kvm/Makefile                  |    2 +-
 arch/x86/kvm/emulate.c                 |   13 +-
 arch/x86/kvm/lapic.c                   |    2 +-
 arch/x86/kvm/lapic.h                   |    1 +
 arch/x86/kvm/pmu.c                     |  513 ++++++++++++++++++++++++++++++++
 arch/x86/kvm/svm.c                     |   15 +
 arch/x86/kvm/vmx.c                     |   15 +-
 arch/x86/kvm/x86.c                     |   65 ++++-
 include/linux/kvm_host.h               |    1 +
 15 files changed, 686 insertions(+), 13 deletions(-)
 create mode 100644 arch/x86/kvm/pmu.c
target-i386/cpuid.c +14 -5
index f179999..ff2a0ca 100644
@@ -1178,11 +1178,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *edx = 0;
         break;
     case 0xA:
-        /* Architectural Performance Monitoring Leaf */
-        *eax = 0;
-        *ebx = 0;
-        *ecx = 0;
-        *edx = 0;
+	if (kvm_enabled()) {
+            KVMState *s = env->kvm_state;
+
+            *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX);
+            *ebx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EBX);
+            *ecx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_ECX);
+            *edx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EDX);
+	} else {
+		/* Architectural Performance Monitoring Leaf */
+		*eax = 0; //0x07280402;
+		*ebx = 0;
+		*ecx = 0;
+		*edx = 0; //0x00000503;
+	}
         break;
     case 0xD:
         /* Processor Extended State */
Signature · 1 line
1.7.5.3
#1 lore raw

PATCH 1/9 KVM: Expose kvm_lapic_local_deliver()

From: Avi Kivity <[email protected]>

Needed to deliver performance monitoring interrupts.

---
 arch/x86/kvm/lapic.c |    2 +-
 arch/x86/kvm/lapic.h |    1 +
 2 files changed, 2 insertions(+), 1 deletions(-)
arch/x86/kvm/lapic.c +1 -1
index 54abb40..e87e43e 100644
@@ -1120,7 +1120,7 @@ int apic_has_pending_timer(struct kvm_vcpu *vcpu)
 	return 0;
 }
 
-static int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
+int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type)
 {
 	u32 reg = apic_get_reg(apic, lvt_type);
 	int vector, mode, trig_mode;
arch/x86/kvm/lapic.h +1 -0
index 138e8cc..6f4ce25 100644
@@ -34,6 +34,7 @@ void kvm_apic_set_version(struct kvm_vcpu *vcpu);
 int kvm_apic_match_physical_addr(struct kvm_lapic *apic, u16 dest);
 int kvm_apic_match_logical_addr(struct kvm_lapic *apic, u8 mda);
 int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq);
+int kvm_apic_local_deliver(struct kvm_lapic *apic, int lvt_type);
 
 u64 kvm_get_apic_base(struct kvm_vcpu *vcpu);
 void kvm_set_apic_base(struct kvm_vcpu *vcpu, u64 data);
Signature · 1 line
1.7.5.3
#2 lore raw

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

In reply to Gleb Natapov

From: Avi Kivity <[email protected]>

Use perf_events to emulate an architectural PMU, version 2.

---
 arch/x86/include/asm/kvm_host.h |   43 ++++
 arch/x86/kvm/Makefile           |    2 +-
 arch/x86/kvm/pmu.c              |  513 +++++++++++++++++++++++++++++++++++++++
 arch/x86/kvm/x86.c              |   20 +-
 include/linux/kvm_host.h        |    1 +
 5 files changed, 570 insertions(+), 9 deletions(-)
 create mode 100644 arch/x86/kvm/pmu.c
arch/x86/include/asm/kvm_host.h +43 -0
index c1f19de..53caa94 100644
@@ -20,6 +20,7 @@
 #include <linux/kvm.h>
 #include <linux/kvm_para.h>
 #include <linux/kvm_types.h>
+#include <linux/perf_event.h>
 
 #include <asm/pvclock-abi.h>
 #include <asm/desc.h>
@@ -296,6 +297,36 @@ struct kvm_mmu {
 	u64 pdptrs[4]; /* pae */
 };
 
+enum pmc_type {
+	KVM_PMC_GP,
+	KVM_PMC_FIXED,
+};
+
+struct kvm_pmc {
+	enum pmc_type type;
+	u64 counter;
+	u64 eventsel;
+	struct perf_event *perf_event;
+	struct kvm_vcpu *vcpu;
+};
+
+struct kvm_pmu {
+	unsigned nr_arch_gp_counters;
+	unsigned nr_arch_fixed_counters;
+	unsigned available_event_types;
+	u64 fixed_ctr_ctrl;
+	u64 global_ctrl;
+	u64 global_status;
+	u64 global_ovf_ctrl;
+	u64 gp_counter_bitmask;
+	u64 fixed_counter_bitmask;
+	u64 global_ctrl_mask;
+	u8 version;
+	struct kvm_pmc gp_counters[X86_PMC_MAX_GENERIC];
+	struct kvm_pmc fixed_counters[X86_PMC_MAX_FIXED];
+	u64 reprogram_pmi;
+};
+
 struct kvm_vcpu_arch {
 	/*
 	 * rip and regs accesses must go through
@@ -433,6 +464,8 @@ struct kvm_vcpu_arch {
 	unsigned access;
 	gfn_t mmio_gfn;
 
+	struct kvm_pmu pmu;
+
 	/* used for guest single stepping over the given code position */
 	unsigned long singlestep_rip;
 
@@ -894,4 +927,14 @@ extern bool kvm_find_async_pf_gfn(struct kvm_vcpu *vcpu, gfn_t gfn);
 
 void kvm_complete_insn_gp(struct kvm_vcpu *vcpu, int err);
 
+void kvm_pmu_init(struct kvm_vcpu *vcpu);
+void kvm_pmu_destroy(struct kvm_vcpu *vcpu);
+void kvm_pmu_reset(struct kvm_vcpu *vcpu);
+void kvm_pmu_cpuid_update(struct kvm_vcpu *vcpu);
+bool kvm_pmu_msr(struct kvm_vcpu *vcpu, u32 msr);
+int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *data);
+int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data);
+int kvm_pmu_read_pmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
+void kvm_handle_pmu_event(struct kvm_vcpu *vcpu);
+
 #endif /* _ASM_X86_KVM_HOST_H */
arch/x86/kvm/Makefile +1 -1
index f15501f..cfca03f 100644
@@ -12,7 +12,7 @@ kvm-$(CONFIG_IOMMU_API)	+= $(addprefix ../../../virt/kvm/, iommu.o)
 kvm-$(CONFIG_KVM_ASYNC_PF)	+= $(addprefix ../../../virt/kvm/, async_pf.o)
 
 kvm-y			+= x86.o mmu.o emulate.o i8259.o irq.o lapic.o \
-			   i8254.o timer.o
+			   i8254.o timer.o pmu.o
 kvm-intel-y		+= vmx.o
 kvm-amd-y		+= svm.o
 
arch/x86/kvm/pmu.c new file +513 -0

513 lines not on this page · raw message

arch/x86/kvm/x86.c +12 -8
index 2b1c526..eb60363 100644
@@ -623,6 +623,8 @@ static void update_cpuid(struct kvm_vcpu *vcpu)
 
 	if (apic)
 		apic->lapic_timer.timer_mode_mask = timer_mode_mask;
+
+	kvm_pmu_cpuid_update(vcpu);
 }
 
 int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
@@ -1655,8 +1657,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	 * which we perfectly emulate ;-). Any other value should be at least
 	 * reported, some guests depend on them.
 	 */
-	case MSR_P6_EVNTSEL0:
-	case MSR_P6_EVNTSEL1:
 	case MSR_K7_EVNTSEL0:
 	case MSR_K7_EVNTSEL1:
 	case MSR_K7_EVNTSEL2:
@@ -1668,8 +1668,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	/* at least RHEL 4 unconditionally writes to the perfctr registers,
 	 * so we ignore writes to make it happy.
 	 */
-	case MSR_P6_PERFCTR0:
-	case MSR_P6_PERFCTR1:
 	case MSR_K7_PERFCTR0:
 	case MSR_K7_PERFCTR1:
 	case MSR_K7_PERFCTR2:
@@ -1706,6 +1704,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	default:
 		if (msr && (msr == vcpu->kvm->arch.xen_hvm_config.msr))
 			return xen_hvm_config(vcpu, data);
+		if (kvm_pmu_msr(vcpu, msr))
+			return kvm_pmu_set_msr(vcpu, msr, data);
 		if (!ignore_msrs) {
 			pr_unimpl(vcpu, "unhandled wrmsr: 0x%x data %llx\n",
 				msr, data);
@@ -1868,10 +1868,6 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 	case MSR_K8_SYSCFG:
 	case MSR_K7_HWCR:
 	case MSR_VM_HSAVE_PA:
-	case MSR_P6_PERFCTR0:
-	case MSR_P6_PERFCTR1:
-	case MSR_P6_EVNTSEL0:
-	case MSR_P6_EVNTSEL1:
 	case MSR_K7_EVNTSEL0:
 	case MSR_K7_PERFCTR0:
 	case MSR_K8_INT_PENDING_MSG:
@@ -1982,6 +1978,8 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
 		data = 0xbe702111;
 		break;
 	default:
+		if (kvm_pmu_msr(vcpu, msr))
+			return kvm_pmu_get_msr(vcpu, msr, pdata);
 		if (!ignore_msrs) {
 			pr_unimpl(vcpu, "unhandled rdmsr: 0x%x\n", msr);
 			return 1;
@@ -5730,6 +5728,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 			process_nmi(vcpu);
 		req_immediate_exit =
 			kvm_check_request(KVM_REQ_IMMEDIATE_EXIT, vcpu);
+		if (kvm_check_request(KVM_REQ_PMU, vcpu))
+			kvm_handle_pmu_event(vcpu);
 	}
 
 	r = kvm_mmu_reload(vcpu);
@@ -6470,6 +6470,8 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu)
 	kvm_async_pf_hash_reset(vcpu);
 	vcpu->arch.apf.halted = false;
 
+	kvm_pmu_reset(vcpu);
+
 	return kvm_x86_ops->vcpu_reset(vcpu);
 }
 
@@ -6558,6 +6560,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 		goto fail_free_mce_banks;
 
 	kvm_async_pf_hash_reset(vcpu);
+	kvm_pmu_init(vcpu);
 
 	return 0;
 fail_free_mce_banks:
@@ -6576,6 +6579,7 @@ void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)
 {
 	int idx;
 
+	kvm_pmu_destroy(vcpu);
 	kfree(vcpu->arch.mce_banks);
 	kvm_free_lapic(vcpu);
 	idx = srcu_read_lock(&vcpu->kvm->srcu);
include/linux/kvm_host.h +1 -0
index c6a2ec9..7ad40d5 100644
@@ -52,6 +52,7 @@
 #define KVM_REQ_STEAL_UPDATE      13
 #define KVM_REQ_NMI               14
 #define KVM_REQ_IMMEDIATE_EXIT    15
+#define KVM_REQ_PMU               16
 
 #define KVM_USERSPACE_IRQ_SOURCE_ID	0
 
Signature · 1 line
1.7.5.3
#3 lore raw

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

Quoted 1 line · Gleb Natapov

From: Avi Kivity <[email protected]>

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

Quoted 8 lines

Use perf_events to emulate an architectural PMU, version 2.

arch/x86/kvm/pmu.c new file +6 -0
+
+/* 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)
+{

arch/x86/kvm/pmu.c line 51

Quoted 10 lines
arch/x86/kvm/pmu.c new file +10 -0
+	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.

Quoted 7 lines
arch/x86/kvm/pmu.c new file +7 -0
+
+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;
+

arch/x86/kvm/pmu.c line 68

Quoted 10 lines
arch/x86/kvm/pmu.c new file +10 -0
+	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.

Quoted 12 lines
arch/x86/kvm/pmu.c new file +12 -0
+
+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,

arch/x86/kvm/pmu.c line 122

Quoted 10 lines
arch/x86/kvm/pmu.c new file +10 -0
+		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.

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+
+static void reprogram_fixed_counter(struct kvm_pmc *pmc, u8 en_pmi, int idx)
+{
+	unsigned en = en_pmi & 0x3;
+	bool pmi = en_pmi & 0x8;
+

arch/x86/kvm/pmu.c line 242

Quoted 10 lines
arch/x86/kvm/pmu.c new file +10 -0
+	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?

arch/x86/kvm/pmu.c line 245

Quoted 3 lines
arch/x86/kvm/pmu.c new file +3 -0
+}
+
+#define FIXED_EN_PMI(R, I) (((R) >> ((I) * 4)) & 0xf)

function

Quoted 37 lines
arch/x86/kvm/pmu.c new file +37 -0
+	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;

arch/x86/kvm/pmu.c line 442

Quoted 10 lines
arch/x86/kvm/pmu.c new file +10 -0
+
+	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).

arch/x86/kvm/pmu.c line 444

Quoted 2 lines
arch/x86/kvm/pmu.c new file +2 -0
+		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?

arch/x86/kvm/pmu.c line 450

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+		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.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#4 lore raw

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

Quoted 18 lines · Avi Kivity
Quoted 1 line · Gleb Natapov

From: Avi Kivity <[email protected]>

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

Quoted 12 lines

Use perf_events to emulate an architectural PMU, version 2.

arch/x86/kvm/pmu.c new file +10 -0
+
+/* 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)
Quoted 9 lines

arch/x86/kvm/pmu.c line 51

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+{
+	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.

Hmm, is it really nicer to replicate the same information 35 times?

Quoted 11 lines
Quoted 11 lines
arch/x86/kvm/pmu.c new file +11 -0
+
+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 {
Quoted 8 lines

arch/x86/kvm/pmu.c line 68

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+		counters = pmu->fixed_counters;
+		shift = X86_PMC_IDX_FIXED;
+	}
+
+	return pmc - counters + shift;
+}

Again, push the global index into struct kvm_pmc.

OK.

Quoted 17 lines
Quoted 17 lines
arch/x86/kvm/pmu.c new file +16 -0
+
+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;
Quoted 9 lines

arch/x86/kvm/pmu.c line 122

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+	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?

It supposed to run in an NMI context on the same CPU that just ran the vcpu so simultaneous access to the same variable from different CPUs shouldn't be possible. But if your scenario below can happen then that assumption may not hold. The question is if PMI delivery can be so skewed as to be delivered long after vmexit (which switches perf msr values btw).

Quoted 3 lines

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.

Is kvm_vcpu_kick() safe for NMI context?

Quoted 11 lines
Quoted 11 lines
arch/x86/kvm/pmu.c new file +10 -0
+
+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;
Quoted 9 lines

arch/x86/kvm/pmu.c line 242

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+
+	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?

Nope. perf_event_intel.c open codes them too.

Quoted 56 lines

arch/x86/kvm/pmu.c line 245

Quoted 3 lines
arch/x86/kvm/pmu.c new file +3 -0
+}
+
+#define FIXED_EN_PMI(R, I) (((R) >> ((I) * 4)) & 0xf)

function

Quoted 37 lines
arch/x86/kvm/pmu.c new file +37 -0
+	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;

arch/x86/kvm/pmu.c line 442

Quoted 10 lines
arch/x86/kvm/pmu.c new file +10 -0
+
+	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).

Quoted 6 lines

arch/x86/kvm/pmu.c line 444

Quoted 2 lines
arch/x86/kvm/pmu.c new file +2 -0
+		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?

If user will set it to zero then attr.sample_period will always be 0 and perf will think that the event is non sampling and will use max_period instead. For a small value greater than zero how is it different from userspace creating an event with sample_period of 1?

Quoted 13 lines

arch/x86/kvm/pmu.c line 450

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+		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.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

--
Gleb.

#5 lore raw

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

Quoted 26 lines · Gleb Natapov
Quoted 17 lines
Quoted 17 lines
arch/x86/kvm/pmu.c new file +16 -0
+
+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;
Quoted 9 lines

arch/x86/kvm/pmu.c line 122

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+	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?

Quoted 7 lines

It supposed to run in an NMI context on the same CPU that just ran the vcpu so simultaneous access to the same variable from different CPUs shouldn't be possible. But if your scenario below can happen then that assumption may not hold. The question is if PMI delivery can be so skewed as to be delivered long after vmexit (which switches perf msr values btw).

Thinking about it some more since perf events we create are bound to a vcpu task it is impossible to have event callback called on a different CPU.

--
Gleb.

#6 lore raw

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

Quoted 34 lines · Gleb Natapov
Quoted 26 lines · Gleb Natapov
Quoted 17 lines
Quoted 17 lines
arch/x86/kvm/pmu.c new file +16 -0
+
+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;
Quoted 9 lines

arch/x86/kvm/pmu.c line 122

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+	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?

Quoted 7 lines

It supposed to run in an NMI context on the same CPU that just ran the vcpu so simultaneous access to the same variable from different CPUs shouldn't be possible. But if your scenario below can happen then that assumption may not hold. The question is if PMI delivery can be so skewed as to be delivered long after vmexit (which switches perf msr values btw).

Quoted 2 lines

Thinking about it some more since perf events we create are bound to a vcpu task it is impossible to have event callback called on a different CPU.

Ok. Maybe there is some theoretical skew value that gets the NMI delayed past the context switch, but not on any real cpu.

Signature · 1 line
error compiling committee.c: too many arguments to function
#7 lore raw

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

Quoted 35 lines · Avi Kivity
Quoted 34 lines · Gleb Natapov
Quoted 33 lines · Gleb Natapov
Quoted 17 lines
Quoted 17 lines
arch/x86/kvm/pmu.c new file +16 -0
+
+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;
Quoted 9 lines

arch/x86/kvm/pmu.c line 122

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+	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?

It supposed to run in an NMI context on the same CPU that just ran the vcpu so simultaneous access to the same variable from different CPUs shouldn't be possible. But if your scenario below can happen then that assumption may not hold. The question is if PMI delivery can be so skewed as to be delivered long after vmexit (which switches perf msr values btw).

Quoted 6 lines
Quoted 2 lines

Thinking about it some more since perf events we create are bound to a vcpu task it is impossible to have event callback called on a different CPU.

Ok. Maybe there is some theoretical skew value that gets the NMI delayed past the context switch, but not on any real cpu.

During a context switch perf events attached to a task will be schedules out and such skewed NMI will be ignored.

--
Gleb.

#8 lore raw

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

In reply to Gleb Natapov
Quoted 19 lines · Gleb Natapov
Quoted 10 lines
Quoted 10 lines
arch/x86/kvm/pmu.c new file +10 -0
+
+/* 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)
Quoted 9 lines

arch/x86/kvm/pmu.c line 51

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+{
+	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.

Quoted 1 line

Hmm, is it really nicer to replicate the same information 35 times?

If it were 35 times, you could do pmu->type->bitmask. But it's just 5 or 6 times.

Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+
+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;
Quoted 9 lines

arch/x86/kvm/pmu.c line 122

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+	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?

Quoted 6 lines

It supposed to run in an NMI context on the same CPU that just ran the vcpu so simultaneous access to the same variable from different CPUs shouldn't be possible. But if your scenario below can happen then that assumption may not hold. The question is if PMI delivery can be so skewed as to be delivered long after vmexit (which switches perf msr values btw).

The compiler/runtime is allowed to implement __test_and_set_bit() as multiple instructions, no? Do we have any similar sequences outside nmi context?

Quoted 4 lines
Quoted 3 lines

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.

Is kvm_vcpu_kick() safe for NMI context?

No. There is irq_work_queue() for that. Would be good to avoid it if we know that it's safe to (for example if we have PF_VCPU set).

Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
arch/x86/kvm/pmu.c new file +10 -0
+
+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;
Quoted 9 lines

arch/x86/kvm/pmu.c line 242

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+
+	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?

Quoted 1 line

Nope. perf_event_intel.c open codes them too.

Okay.

Quoted 8 lines
Quoted 4 lines

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

If user will set it to zero then attr.sample_period will always be 0 and perf will think that the event is non sampling and will use max_period instead. For a small value greater than zero how is it different from userspace creating an event with sample_period of 1?

I don't know. Does the kernel survive it?

Signature · 1 line
error compiling committee.c: too many arguments to function
#9 lore raw

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

Quoted 20 lines · Avi Kivity
Quoted 19 lines · Gleb Natapov
Quoted 19 lines
Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+
+/* 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)
+{

arch/x86/kvm/pmu.c line 51

Quoted 10 lines
arch/x86/kvm/pmu.c new file +10 -0
+	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.

Quoted 5 lines
Quoted 1 line

Hmm, is it really nicer to replicate the same information 35 times?

If it were 35 times, you could do pmu->type->bitmask. But it's just 5 or 6 times.

It is 35. Perf defines X86_PMC_MAX_GENERIC to be 32 and X86_PMC_MAX_FIXED to be 3. I can do pmu->type->bitmask if you think it is better.

Quoted 22 lines
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+
+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;
Quoted 9 lines

arch/x86/kvm/pmu.c line 122

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+	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?

Quoted 6 lines

It supposed to run in an NMI context on the same CPU that just ran the vcpu so simultaneous access to the same variable from different CPUs shouldn't be possible. But if your scenario below can happen then that assumption may not hold. The question is if PMI delivery can be so skewed as to be delivered long after vmexit (which switches perf msr values btw).

Quoted 4 lines

The compiler/runtime is allowed to implement __test_and_set_bit() as multiple instructions, no? Do we have any similar sequences outside nmi context?

Yes we do. On handling PMU event during guest entry and during event reprogramming. On x86 __ version is different from non __ version only by lock prefix. It would be pity to use locked functions here though. We need local_ functions for bitops.

Quoted 8 lines
Quoted 4 lines
Quoted 3 lines

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.

Is kvm_vcpu_kick() safe for NMI context?

No. There is irq_work_queue() for that. Would be good to avoid it if we know that it's safe to (for example if we have PF_VCPU set).

Checking PF_VCPU will not tell us that vcpu is going to reenter guest mode again.

Quoted 28 lines
Quoted 20 lines
Quoted 11 lines
Quoted 11 lines
arch/x86/kvm/pmu.c new file +10 -0
+
+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;
Quoted 9 lines

arch/x86/kvm/pmu.c line 242

Quoted 6 lines
arch/x86/kvm/pmu.c new file +6 -0
+
+	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?

Quoted 1 line

Nope. perf_event_intel.c open codes them too.

Okay.

Quoted 4 lines
Quoted 4 lines

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

Quoted 7 lines
Quoted 4 lines

If user will set it to zero then attr.sample_period will always be 0 and perf will think that the event is non sampling and will use max_period instead. For a small value greater than zero how is it different from userspace creating an event with sample_period of 1?

I don't know. Does the kernel survive it?

Need to test, but I do not see anything in the kernel that prevent userspace from setting it to any value.

--
Gleb.

#10 lore raw

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

Quoted 7 lines · Gleb Natapov
Quoted 3 lines · Avi Kivity

No. There is irq_work_queue() for that. Would be good to avoid it if we know that it's safe to (for example if we have PF_VCPU set).

Checking PF_VCPU will not tell us that vcpu is going to reenter guest mode again.

Looks like we can check percpu_read(current_vcpu) to see if last vmexit was due to NMI we are processing and decide according to that.

--
Gleb.

#11 lore raw

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

In reply to Gleb Natapov
Quoted 8 lines · Gleb Natapov
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines

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

If user will set it to zero then attr.sample_period will always be 0 and perf will think that the event is non sampling and will use max_period instead. For a small value greater than zero how is it different from userspace creating an event with sample_period of 1?

Quoted 5 lines
Quoted 2 lines

I don't know. Does the kernel survive it?

Need to test, but I do not see anything in the kernel that prevent userspace from setting it to any value.

You can set sample_period to 1 by providing -c 1 to perf top/record. Kernel survives it easily since perf subsystem kindly throttles interrupts for you. You can set max interrupt per event per time tick in /proc/sys/kernel/perf_event_max_sample_rate.

--
Gleb.

#12 lore raw

PATCH 1/1 KVM: x86: Fix build breakage due to anonymous field initialization

In reply to Gleb Natapov

Not all compilers accept anonymous field initialization, e.g. gcc 4.5.1.

---
 arch/x86/kvm/pmu.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
arch/x86/kvm/pmu.c +2 -1
index 061a03f..7aad544 100644
@@ -170,10 +170,11 @@ static void reprogram_counter(struct kvm_pmc *pmc, u32 type,
 		.exclude_host = 1,
 		.exclude_user = exclude_user,
 		.exclude_kernel = exclude_kernel,
-		.sample_period = (-pmc->counter) & pmc_bitmask(pmc),
 		.config = config,
 	};
 
+	attr.sample_period = (-pmc->counter) & pmc_bitmask(pmc);
+
 	event = perf_event_create_kernel_counter(&attr, -1, current,
 						 intr ? kvm_perf_overflow_intr :
 						 kvm_perf_overflow, pmc);
#13 lore raw

Re: [PATCH] KVM: x86: Fix build breakage due to anonymous field initialization

Quoted 13 lines · Jan Kiszka

Not all compilers accept anonymous field initialization, e.g. gcc 4.5.1.

---
 arch/x86/kvm/pmu.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
arch/x86/kvm/pmu.c +0 -0
index 061a03f..7aad544 100644

arch/x86/kvm/pmu.c static void reprogram_counter(struct kvm_pmc *pmc, u32 type, · line 173

Quoted 4 lines
arch/x86/kvm/pmu.c +0 -1
@@ -170,10 +170,11 @@ static void reprogram_counter(struct kvm_pmc *pmc, u32 type,
 		.exclude_host = 1,
 		.exclude_user = exclude_user,
 		.exclude_kernel = exclude_kernel,
-		.sample_period = (-pmc->counter) & pmc_bitmask(pmc),

+ { .sample_period = ... },

Does that work?

Quoted 2 lines
arch/x86/kvm/pmu.c +0 -0
 		.config = config,
 	};
#14 lore raw

Re: [PATCH] KVM: x86: Fix build breakage due to anonymous field initialization

Quoted 20 lines · Peter Zijlstra
Quoted 17 lines · Jan Kiszka

Not all compilers accept anonymous field initialization, e.g. gcc 4.5.1.

---
 arch/x86/kvm/pmu.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
arch/x86/kvm/pmu.c +0 -1
index 061a03f..7aad544 100644
@@ -170,10 +170,11 @@ static void reprogram_counter(struct kvm_pmc *pmc, u32 type,
 		.exclude_host = 1,
 		.exclude_user = exclude_user,
 		.exclude_kernel = exclude_kernel,
-		.sample_period = (-pmc->counter) & pmc_bitmask(pmc),
(quoted diff) +1 -0
+		{ .sample_period = ... },
Quoted 1 line

Does that work?

Nope:

CC [M] arch/x86/kvm/pmu.o
/data/linux-kvm/arch/x86/kvm/pmu.c: In function ‘reprogram_counter’:
/data/linux-kvm/arch/x86/kvm/pmu.c:173:3: warning: braces around scalar initializer /data/linux-kvm/arch/x86/kvm/pmu.c:173:3: warning: (near initialization for ‘attr.exclude_hv’) /data/linux-kvm/arch/x86/kvm/pmu.c:173:3: error: field name not in record or union initializer /data/linux-kvm/arch/x86/kvm/pmu.c:173:3: error: (near initialization for ‘attr.exclude_hv’)

Jan

Signature · 2 lines
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
Applied
#15 lore raw

Re: [PATCH] KVM: x86: Fix build breakage due to anonymous field initialization

In reply to Jan Kiszka
Quoted 3 lines · Jan Kiszka

Not all compilers accept anonymous field initialization, e.g. gcc 4.5.1.

Thanks, applied.

Signature · 1 line
error compiling committee.c: too many arguments to function
#16 lore raw

PATCH 3/9 KVM: Add generic RDPMC support

In reply to Gleb Natapov

From: Avi Kivity <[email protected]>

Add a helper function that emulates the RDPMC instruction operation.

---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/x86.c              |   15 +++++++++++++++
 2 files changed, 16 insertions(+), 0 deletions(-)
arch/x86/include/asm/kvm_host.h +1 -0
index 53caa94..729b3b7 100644
@@ -767,6 +767,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data);
 
 unsigned long kvm_get_rflags(struct kvm_vcpu *vcpu);
 void kvm_set_rflags(struct kvm_vcpu *vcpu, unsigned long rflags);
+bool kvm_rdpmc(struct kvm_vcpu *vcpu);
 
 void kvm_queue_exception(struct kvm_vcpu *vcpu, unsigned nr);
 void kvm_queue_exception_e(struct kvm_vcpu *vcpu, unsigned nr, u32 error_code);
arch/x86/kvm/x86.c +15 -0
index eb60363..5ea4cb8 100644
@@ -814,6 +814,21 @@ int kvm_get_dr(struct kvm_vcpu *vcpu, int dr, unsigned long *val)
 }
 EXPORT_SYMBOL_GPL(kvm_get_dr);
 
+bool kvm_rdpmc(struct kvm_vcpu *vcpu)
+{
+	u32 ecx = kvm_register_read(vcpu, VCPU_REGS_RCX);
+	u64 data;
+	int err;
+
+	err = kvm_pmu_read_pmc(vcpu, ecx, &data);
+	if (err)
+		return err;
+	kvm_register_write(vcpu, VCPU_REGS_RAX, (u32)data);
+	kvm_register_write(vcpu, VCPU_REGS_RDX, data >> 32);
+	return err;
+}
+EXPORT_SYMBOL_GPL(kvm_rdpmc);
+
 /*
  * List of msr numbers which we expose to userspace through KVM_GET_MSRS
  * and KVM_SET_MSRS, and KVM_GET_MSR_INDEX_LIST.
Signature · 1 line
1.7.5.3
#17 lore raw

PATCH 4/9 KVM: SVM: Intercept RDPMC

In reply to Gleb Natapov

From: Avi Kivity <[email protected]>

Intercept RDPMC and forward it to the PMU emulation code.

---
 arch/x86/kvm/svm.c |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)
arch/x86/kvm/svm.c +15 -0
index e32243e..5fa553b 100644
@@ -1014,6 +1014,7 @@ static void init_vmcb(struct vcpu_svm *svm)
 	set_intercept(svm, INTERCEPT_NMI);
 	set_intercept(svm, INTERCEPT_SMI);
 	set_intercept(svm, INTERCEPT_SELECTIVE_CR0);
+	set_intercept(svm, INTERCEPT_RDPMC);
 	set_intercept(svm, INTERCEPT_CPUID);
 	set_intercept(svm, INTERCEPT_INVD);
 	set_intercept(svm, INTERCEPT_HLT);
@@ -2770,6 +2771,19 @@ static int emulate_on_interception(struct vcpu_svm *svm)
 	return emulate_instruction(&svm->vcpu, 0) == EMULATE_DONE;
 }
 
+static int rdpmc_interception(struct vcpu_svm *svm)
+{
+	int err;
+
+	if (!static_cpu_has(X86_FEATURE_NRIPS))
+		return emulate_on_interception(svm);
+
+	err = kvm_rdpmc(&svm->vcpu);
+	kvm_complete_insn_gp(&svm->vcpu, err);
+
+	return 1;
+}
+
 bool check_selective_cr0_intercepted(struct vcpu_svm *svm, unsigned long val)
 {
 	unsigned long cr0 = svm->vcpu.arch.cr0;
@@ -3190,6 +3204,7 @@ static int (*svm_exit_handlers[])(struct vcpu_svm *svm) = {
 	[SVM_EXIT_SMI]				= nop_on_interception,
 	[SVM_EXIT_INIT]				= nop_on_interception,
 	[SVM_EXIT_VINTR]			= interrupt_window_interception,
+	[SVM_EXIT_RDPMC]			= rdpmc_interception,
 	[SVM_EXIT_CPUID]			= cpuid_interception,
 	[SVM_EXIT_IRET]                         = iret_interception,
 	[SVM_EXIT_INVD]                         = emulate_on_interception,
Signature · 1 line
1.7.5.3
#18 lore raw

PATCH 5/9 KVM: VMX: Intercept RDPMC

In reply to Gleb Natapov

From: Avi Kivity <[email protected]>

Intercept RDPMC and forward it to the PMU emulation code.

---
 arch/x86/kvm/vmx.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)
arch/x86/kvm/vmx.c +14 -1
index 6e28d58..a6535ba 100644
@@ -1956,6 +1956,7 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 #endif
 		CPU_BASED_MOV_DR_EXITING | CPU_BASED_UNCOND_IO_EXITING |
 		CPU_BASED_USE_IO_BITMAPS | CPU_BASED_MONITOR_EXITING |
+		CPU_BASED_RDPMC_EXITING |
 		CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
 	/*
 	 * We can allow some features even when not supported by the
@@ -2414,7 +2415,8 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
 	      CPU_BASED_USE_TSC_OFFSETING |
 	      CPU_BASED_MWAIT_EXITING |
 	      CPU_BASED_MONITOR_EXITING |
-	      CPU_BASED_INVLPG_EXITING;
+	      CPU_BASED_INVLPG_EXITING |
+	      CPU_BASED_RDPMC_EXITING;
 
 	if (yield_on_hlt)
 		min |= CPU_BASED_HLT_EXITING;
@@ -4615,6 +4617,16 @@ static int handle_invlpg(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
+static int handle_rdpmc(struct kvm_vcpu *vcpu)
+{
+	int err;
+
+	err = kvm_rdpmc(vcpu);
+	kvm_complete_insn_gp(vcpu, err);
+
+	return 1;
+}
+
 static int handle_wbinvd(struct kvm_vcpu *vcpu)
 {
 	skip_emulated_instruction(vcpu);
@@ -5565,6 +5577,7 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
 	[EXIT_REASON_HLT]                     = handle_halt,
 	[EXIT_REASON_INVD]		      = handle_invd,
 	[EXIT_REASON_INVLPG]		      = handle_invlpg,
+	[EXIT_REASON_RDPMC]                   = handle_rdpmc,
 	[EXIT_REASON_VMCALL]                  = handle_vmcall,
 	[EXIT_REASON_VMCLEAR]	              = handle_vmclear,
 	[EXIT_REASON_VMLAUNCH]                = handle_vmlaunch,
Signature · 1 line
1.7.5.3
#19 lore raw

PATCH 6/9 perf: expose perf capability to other modules.

In reply to Gleb Natapov

KVM needs to know perf capability to decide which PMU it can expose to a guest.

---
 arch/x86/include/asm/perf_event.h      |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.c       |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.h       |    2 ++
 arch/x86/kernel/cpu/perf_event_intel.c |    3 +++
 4 files changed, 27 insertions(+), 0 deletions(-)
arch/x86/include/asm/perf_event.h +11 -0
index f61c62f..7d7e57f 100644
@@ -201,7 +201,18 @@ struct perf_guest_switch_msr {
 	u64 host, guest;
 };
 
+struct x86_pmu_capability {
+	int version;
+	int num_counters_gp;
+	int num_counters_fixed;
+	int bit_width_gp;
+	int bit_width_fixed;
+	unsigned int events_mask;
+	int events_mask_len;
+};
+
 extern struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr);
+extern void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap);
 #else
 static inline perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
 {
arch/x86/kernel/cpu/perf_event.c +11 -0
index 6408910..94ac9ca 100644
@@ -1570,3 +1570,14 @@ unsigned long perf_misc_flags(struct pt_regs *regs)
 
 	return misc;
 }
+
+void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap)
+{
+	cap->version = x86_pmu.version;
+	cap->num_counters_gp = x86_pmu.num_counters;
+	cap->num_counters_fixed = x86_pmu.num_counters_fixed;
+	cap->bit_width_gp = cap->bit_width_fixed = x86_pmu.cntval_bits;
+	cap->events_mask = x86_pmu.events_mask;
+	cap->events_mask_len = x86_pmu.events_mask_len;
+}
+EXPORT_SYMBOL_GPL(perf_get_x86_pmu_capability);
arch/x86/kernel/cpu/perf_event.h +2 -0
index b9698d4..e9ed238 100644
@@ -259,6 +259,8 @@ struct x86_pmu {
 	int		num_counters_fixed;
 	int		cntval_bits;
 	u64		cntval_mask;
+	u32		events_mask;
+	int		events_mask_len;
 	int		apic;
 	u64		max_period;
 	struct event_constraint *
arch/x86/kernel/cpu/perf_event_intel.c +3 -0
index e09ca20..64e5f35 100644
@@ -1580,6 +1580,8 @@ __init int intel_pmu_init(void)
 	x86_pmu.num_counters		= eax.split.num_counters;
 	x86_pmu.cntval_bits		= eax.split.bit_width;
 	x86_pmu.cntval_mask		= (1ULL << eax.split.bit_width) - 1;
+	x86_pmu.events_mask		= ebx;
+	x86_pmu.events_mask_len		= eax.split.mask_length;
 
 	/*
 	 * Quirk: v2 perfmon does not report fixed-purpose events, so
@@ -1651,6 +1653,7 @@ __init int intel_pmu_init(void)
 			 * architectural event which is often completely bogus:
 			 */
 			intel_perfmon_event_map[PERF_COUNT_HW_BRANCH_MISSES] = 0x7f89;
+			x86_pmu.events_mask &= ~0x40;
 
 			pr_cont("erratum AAJ80 worked around, ");
 		}
Signature · 1 line
1.7.5.3
#20 lore raw

Re: [PATCH 6/9] perf: expose perf capability to other modules.

Quoted 12 lines · Gleb Natapov

KVM needs to know perf capability to decide which PMU it can expose to a guest.

---
 arch/x86/include/asm/perf_event.h      |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.c       |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.h       |    2 ++
 arch/x86/kernel/cpu/perf_event_intel.c |    3 +++
 4 files changed, 27 insertions(+), 0 deletions(-)

Peter, can you please review this, and if all is well, either apply or ack?

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#21 lore raw

Re: [PATCH 6/9] perf: expose perf capability to other modules.

In reply to Gleb Natapov
Quoted 24 lines · Gleb Natapov

KVM needs to know perf capability to decide which PMU it can expose to a guest.

---
 arch/x86/include/asm/perf_event.h      |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.c       |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.h       |    2 ++
 arch/x86/kernel/cpu/perf_event_intel.c |    3 +++
 4 files changed, 27 insertions(+), 0 deletions(-)
arch/x86/include/asm/perf_event.h +5 -0
index f61c62f..7d7e57f 100644
@@ -201,7 +201,18 @@ struct perf_guest_switch_msr {
 	u64 host, guest;
 };
 
+struct x86_pmu_capability {
+	int version;
+	int num_counters_gp;
+	int num_counters_fixed;
+	int bit_width_gp;

arch/x86/include/asm/perf_event.h struct perf_guest_switch_msr { · line 218

Quoted 10 lines
arch/x86/include/asm/perf_event.h +6 -0
+	int bit_width_fixed;
+	unsigned int events_mask;
+	int events_mask_len;
+};
+
 extern struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr);
+extern void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap);
 #else
 static inline perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
 {

What about version of perf_get_x86_pmu_capability for CONFIG_PERF_EVENTS not enabled in host kernel? Next patch for KVM assumes the function is defined.

David

Quoted 53 lines
arch/x86/kernel/cpu/perf_event.c +11 -0
index 6408910..94ac9ca 100644
@@ -1570,3 +1570,14 @@ unsigned long perf_misc_flags(struct pt_regs *regs)
 
 	return misc;
 }
+
+void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap)
+{
+	cap->version = x86_pmu.version;
+	cap->num_counters_gp = x86_pmu.num_counters;
+	cap->num_counters_fixed = x86_pmu.num_counters_fixed;
+	cap->bit_width_gp = cap->bit_width_fixed = x86_pmu.cntval_bits;
+	cap->events_mask = x86_pmu.events_mask;
+	cap->events_mask_len = x86_pmu.events_mask_len;
+}
+EXPORT_SYMBOL_GPL(perf_get_x86_pmu_capability);
arch/x86/kernel/cpu/perf_event.h +2 -0
index b9698d4..e9ed238 100644
@@ -259,6 +259,8 @@ struct x86_pmu {
 	int		num_counters_fixed;
 	int		cntval_bits;
 	u64		cntval_mask;
+	u32		events_mask;
+	int		events_mask_len;
 	int		apic;
 	u64		max_period;
 	struct event_constraint *
arch/x86/kernel/cpu/perf_event_intel.c +3 -0
index e09ca20..64e5f35 100644
@@ -1580,6 +1580,8 @@ __init int intel_pmu_init(void)
 	x86_pmu.num_counters		= eax.split.num_counters;
 	x86_pmu.cntval_bits		= eax.split.bit_width;
 	x86_pmu.cntval_mask		= (1ULL << eax.split.bit_width) - 1;
+	x86_pmu.events_mask		= ebx;
+	x86_pmu.events_mask_len		= eax.split.mask_length;
 
 	/*
 	 * Quirk: v2 perfmon does not report fixed-purpose events, so
@@ -1651,6 +1653,7 @@ __init int intel_pmu_init(void)
 			 * architectural event which is often completely bogus:
 			 */
 			intel_perfmon_event_map[PERF_COUNT_HW_BRANCH_MISSES] = 0x7f89;
+			x86_pmu.events_mask &= ~0x40;
 
 			pr_cont("erratum AAJ80 worked around, ");
 		}
#22 lore raw

Re: [PATCH 6/9] perf: expose perf capability to other modules.

Quoted 31 lines · David Ahern
Quoted 30 lines · Gleb Natapov

KVM needs to know perf capability to decide which PMU it can expose to a guest.

---
 arch/x86/include/asm/perf_event.h      |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.c       |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.h       |    2 ++
 arch/x86/kernel/cpu/perf_event_intel.c |    3 +++
 4 files changed, 27 insertions(+), 0 deletions(-)
arch/x86/include/asm/perf_event.h +10 -0
index f61c62f..7d7e57f 100644
@@ -201,7 +201,18 @@ struct perf_guest_switch_msr {
 	u64 host, guest;
 };
 
+struct x86_pmu_capability {
+	int version;
+	int num_counters_gp;
+	int num_counters_fixed;
+	int bit_width_gp;
+	int bit_width_fixed;
+	unsigned int events_mask;
+	int events_mask_len;
+};
+
 extern struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr);
Quoted 9 lines

arch/x86/include/asm/perf_event.h struct perf_guest_switch_msr { · line 218

Quoted 4 lines
arch/x86/include/asm/perf_event.h +1 -0
+extern void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap);
 #else
 static inline perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
 {

What about version of perf_get_x86_pmu_capability for CONFIG_PERF_EVENTS not enabled in host kernel? Next patch for KVM assumes the function is defined.

As far as I understand it is not possible to build x86 without CONFIG_PERF_EVENTS right now. Actually kvm pmu code depends on CONFIG_PERF_EVENTS been enabled. I can easily provide the stub if needed though.

Quoted 54 lines

David

Quoted 51 lines
arch/x86/kernel/cpu/perf_event.c +11 -0
index 6408910..94ac9ca 100644
@@ -1570,3 +1570,14 @@ unsigned long perf_misc_flags(struct pt_regs *regs)
 
 	return misc;
 }
+
+void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap)
+{
+	cap->version = x86_pmu.version;
+	cap->num_counters_gp = x86_pmu.num_counters;
+	cap->num_counters_fixed = x86_pmu.num_counters_fixed;
+	cap->bit_width_gp = cap->bit_width_fixed = x86_pmu.cntval_bits;
+	cap->events_mask = x86_pmu.events_mask;
+	cap->events_mask_len = x86_pmu.events_mask_len;
+}
+EXPORT_SYMBOL_GPL(perf_get_x86_pmu_capability);
arch/x86/kernel/cpu/perf_event.h +2 -0
index b9698d4..e9ed238 100644
@@ -259,6 +259,8 @@ struct x86_pmu {
 	int		num_counters_fixed;
 	int		cntval_bits;
 	u64		cntval_mask;
+	u32		events_mask;
+	int		events_mask_len;
 	int		apic;
 	u64		max_period;
 	struct event_constraint *
arch/x86/kernel/cpu/perf_event_intel.c +3 -0
index e09ca20..64e5f35 100644
@@ -1580,6 +1580,8 @@ __init int intel_pmu_init(void)
 	x86_pmu.num_counters		= eax.split.num_counters;
 	x86_pmu.cntval_bits		= eax.split.bit_width;
 	x86_pmu.cntval_mask		= (1ULL << eax.split.bit_width) - 1;
+	x86_pmu.events_mask		= ebx;
+	x86_pmu.events_mask_len		= eax.split.mask_length;
 
 	/*
 	 * Quirk: v2 perfmon does not report fixed-purpose events, so
@@ -1651,6 +1653,7 @@ __init int intel_pmu_init(void)
 			 * architectural event which is often completely bogus:
 			 */
 			intel_perfmon_event_map[PERF_COUNT_HW_BRANCH_MISSES] = 0x7f89;
+			x86_pmu.events_mask &= ~0x40;
 
Quoted 2 lines

arch/x86/kernel/cpu/perf_event_intel.c __init int intel_pmu_init(void) · line 1659

Quoted 2 lines
arch/x86/kernel/cpu/perf_event_intel.c +0 -0
 			pr_cont("erratum AAJ80 worked around, ");
 		}

--
Gleb.

#23 lore raw

Re: [PATCH 6/9] perf: expose perf capability to other modules.

Quoted 41 lines · Gleb Natapov
Quoted 31 lines · David Ahern
Quoted 30 lines · Gleb Natapov

KVM needs to know perf capability to decide which PMU it can expose to a guest.

---
 arch/x86/include/asm/perf_event.h      |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.c       |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.h       |    2 ++
 arch/x86/kernel/cpu/perf_event_intel.c |    3 +++
 4 files changed, 27 insertions(+), 0 deletions(-)
arch/x86/include/asm/perf_event.h +10 -0
index f61c62f..7d7e57f 100644
@@ -201,7 +201,18 @@ struct perf_guest_switch_msr {
 	u64 host, guest;
 };
 
+struct x86_pmu_capability {
+	int version;
+	int num_counters_gp;
+	int num_counters_fixed;
+	int bit_width_gp;
+	int bit_width_fixed;
+	unsigned int events_mask;
+	int events_mask_len;
+};
+
 extern struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr);
Quoted 9 lines

arch/x86/include/asm/perf_event.h struct perf_guest_switch_msr { · line 218

Quoted 4 lines
arch/x86/include/asm/perf_event.h +1 -0
+extern void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap);
 #else
 static inline perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
 {

What about version of perf_get_x86_pmu_capability for CONFIG_PERF_EVENTS not enabled in host kernel? Next patch for KVM assumes the function is defined.

Quoted 4 lines

As far as I understand it is not possible to build x86 without CONFIG_PERF_EVENTS right now. Actually kvm pmu code depends on CONFIG_PERF_EVENTS been enabled. I can easily provide the stub if needed though.

Right. Originally it could be enabled/disabled. Right now it cannot be, but I believe Frederic is working on making it configurable again.

David

#24 lore raw

Re: [PATCH 6/9] perf: expose perf capability to other modules.

Quoted 44 lines · David Ahern
Quoted 41 lines · Gleb Natapov
Quoted 40 lines · David Ahern
Quoted 24 lines · Gleb Natapov

KVM needs to know perf capability to decide which PMU it can expose to a guest.

---
 arch/x86/include/asm/perf_event.h      |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.c       |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.h       |    2 ++
 arch/x86/kernel/cpu/perf_event_intel.c |    3 +++
 4 files changed, 27 insertions(+), 0 deletions(-)
arch/x86/include/asm/perf_event.h +5 -0
index f61c62f..7d7e57f 100644
@@ -201,7 +201,18 @@ struct perf_guest_switch_msr {
 	u64 host, guest;
 };
 
+struct x86_pmu_capability {
+	int version;
+	int num_counters_gp;
+	int num_counters_fixed;
+	int bit_width_gp;

arch/x86/include/asm/perf_event.h struct perf_guest_switch_msr { · line 218

Quoted 10 lines
arch/x86/include/asm/perf_event.h +6 -0
+	int bit_width_fixed;
+	unsigned int events_mask;
+	int events_mask_len;
+};
+
 extern struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr);
+extern void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap);
 #else
 static inline perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
 {

What about version of perf_get_x86_pmu_capability for CONFIG_PERF_EVENTS not enabled in host kernel? Next patch for KVM assumes the function is defined.

Quoted 8 lines
Quoted 4 lines

As far as I understand it is not possible to build x86 without CONFIG_PERF_EVENTS right now. Actually kvm pmu code depends on CONFIG_PERF_EVENTS been enabled. I can easily provide the stub if needed though.

Right. Originally it could be enabled/disabled. Right now it cannot be, but I believe Frederic is working on making it configurable again.

OK, I'll provide stub function in the next version, but I will not be able to test it :)

--
Gleb.

#25 lore raw

Re: [PATCH 6/9] perf: expose perf capability to other modules.

In reply to David Ahern
Quoted 49 lines · David Ahern
Quoted 41 lines · Gleb Natapov
Quoted 31 lines · David Ahern
Quoted 30 lines · Gleb Natapov

KVM needs to know perf capability to decide which PMU it can expose to a guest.

---
 arch/x86/include/asm/perf_event.h      |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.c       |   11 +++++++++++
 arch/x86/kernel/cpu/perf_event.h       |    2 ++
 arch/x86/kernel/cpu/perf_event_intel.c |    3 +++
 4 files changed, 27 insertions(+), 0 deletions(-)
arch/x86/include/asm/perf_event.h +10 -0
index f61c62f..7d7e57f 100644
@@ -201,7 +201,18 @@ struct perf_guest_switch_msr {
 	u64 host, guest;
 };
 
+struct x86_pmu_capability {
+	int version;
+	int num_counters_gp;
+	int num_counters_fixed;
+	int bit_width_gp;
+	int bit_width_fixed;
+	unsigned int events_mask;
+	int events_mask_len;
+};
+
 extern struct perf_guest_switch_msr *perf_guest_get_msrs(int *nr);
Quoted 9 lines

arch/x86/include/asm/perf_event.h struct perf_guest_switch_msr { · line 218

Quoted 4 lines
arch/x86/include/asm/perf_event.h +1 -0
+extern void perf_get_x86_pmu_capability(struct x86_pmu_capability *cap);
 #else
 static inline perf_guest_switch_msr *perf_guest_get_msrs(int *nr)
 {

What about version of perf_get_x86_pmu_capability for CONFIG_PERF_EVENTS not enabled in host kernel? Next patch for KVM assumes the function is defined.

Quoted 4 lines

As far as I understand it is not possible to build x86 without CONFIG_PERF_EVENTS right now. Actually kvm pmu code depends on CONFIG_PERF_EVENTS been enabled. I can easily provide the stub if needed though.

Quoted 4 lines

Right. Originally it could be enabled/disabled. Right now it cannot be, but I believe Frederic is working on making it configurable again.

David

Yep. Will Deacon is working on making the breakpoints able to process pure arch informations (ie: without beeing forced to use the perf attr as a midlayer to define them).

Once we have that I can seperate the breakpoints implementation from perf and make it opt-able.

#26 lore raw

Re: [PATCH 6/9] perf: expose perf capability to other modules.

Hi Frederic,

Quoted 4 lines · Frederic Weisbecker
Quoted 3 lines · David Ahern

Right. Originally it could be enabled/disabled. Right now it cannot be, but I believe Frederic is working on making it configurable again.

Quoted 8 lines
Quoted 1 line

David

Yep. Will Deacon is working on making the breakpoints able to process pure arch informations (ie: without beeing forced to use the perf attr as a midlayer to define them).

Once we have that I can seperate the breakpoints implementation from perf and make it opt-able.

How do you foresee kdb fitting into this? I see that currently [on x86] we cook up perf_event structures with a specific overflow handler set. If we want to move this over to using a completely arch-defined structure, then we're going to end up with an overflow handler field in both perf_event *and* the arch-specific structure, which doesn't feel right to me.

Of course, if the goal is only to separate ptrace (i.e. user debugging) from the perf dependency then we don't need the overflow handler because we'll always just send SIGTRAP to the current task.

Any ideas?

Will

#27 lore raw

Re: [PATCH 6/9] perf: expose perf capability to other modules.

Quoted 22 lines · Will Deacon

Hi Frederic,

Quoted 4 lines · Frederic Weisbecker
Quoted 3 lines · David Ahern

Right. Originally it could be enabled/disabled. Right now it cannot be, but I believe Frederic is working on making it configurable again.

Quoted 8 lines
Quoted 1 line

David

Yep. Will Deacon is working on making the breakpoints able to process pure arch informations (ie: without beeing forced to use the perf attr as a midlayer to define them).

Once we have that I can seperate the breakpoints implementation from perf and make it opt-able.

How do you foresee kdb fitting into this? I see that currently [on x86] we cook up perf_event structures with a specific overflow handler set. If we want to move this over to using a completely arch-defined structure, then we're going to end up with an overflow handler field in both perf_event *and* the arch-specific structure, which doesn't feel right to me.

Quoted 5 lines

Of course, if the goal is only to separate ptrace (i.e. user debugging) from the perf dependency then we don't need the overflow handler because we'll always just send SIGTRAP to the current task.

Any ideas?

I don't know if we want to convert x86/kgdb to use pure arch breakpoints. If kgdb one day wants to extend this use to generic code, it may be a good idea to keep the things as is. I don't know, I'm adding Jason in Cc.

In any case I think we have a problem if we want to default to send a SIGTRAP. Look at this:

		bp = per_cpu(bp_per_reg[i], cpu);
		/*
		 * Reset the 'i'th TRAP bit in dr6 to denote completion of
		 * exception handling
		 */
		(*dr6_p) &= ~(DR_TRAP0 << i);
		/*
		 * bp can be NULL due to lazy debug register switching
		 * or due to concurrent perf counter removing.
		 */
		if (!bp) {
			rcu_read_unlock();
			break;
		}

		perf_bp_event(bp, args->regs);

I don't have the details about how lazy the debug register switching can be. And also we want to avoid a locking between the perf event scheduling (removing) and the breakpoint triggering path.

A solution is to look at the ptrace breakpoints in the thread struct and see if the one in the index is there. That can reside in its own callback or as a fallback in hw_breakpoint_handler(). I don't feel that strong with choosing either of those solutions.

#28 lore raw

Re: [PATCH 6/9] perf: expose perf capability to other modules.

Quoted 21 lines · Frederic Weisbecker
Quoted 20 lines · Will Deacon

Hi Frederic,

Quoted 11 lines · Frederic Weisbecker
Quoted 4 lines · David Ahern

Right. Originally it could be enabled/disabled. Right now it cannot be, but I believe Frederic is working on making it configurable again.

David

Yep. Will Deacon is working on making the breakpoints able to process pure arch informations (ie: without beeing forced to use the perf attr as a midlayer to define them).

Once we have that I can seperate the breakpoints implementation from perf and make it opt-able.

How do you foresee kdb fitting into this? I see that currently [on x86] we cook up perf_event structures with a specific overflow handler set. If we want to move this over to using a completely arch-defined structure, then we're going to end up with an overflow handler field in both perf_event *and* the arch-specific structure, which doesn't feel right to me.

Quoted 8 lines
Quoted 5 lines

Of course, if the goal is only to separate ptrace (i.e. user debugging) from the perf dependency then we don't need the overflow handler because we'll always just send SIGTRAP to the current task.

Any ideas?

I don't know if we want to convert x86/kgdb to use pure arch breakpoints. If kgdb one day wants to extend this use to generic code, it may be a good idea to keep the things as is. I don't know, I'm adding Jason in Cc.

I think the important part is to share the allocation code (meaning who owns which break point slots). This is why kgdb/kdb allocates the perf structures. The kgdb code will also directly write data to the slots once it has reserved them it would be good to share this code, but it was not shared because it was not usable early enough in the boot cycle on x86.

Certainly there are others who could consume the same infrastructure such as kprobes.

Jason.

#29 lore raw

Re: [PATCH 6/9] perf: expose perf capability to other modules.

Quoted 31 lines · Jason Wessel
Quoted 21 lines · Frederic Weisbecker
Quoted 20 lines · Will Deacon

Hi Frederic,

Quoted 11 lines · Frederic Weisbecker
Quoted 4 lines · David Ahern

Right. Originally it could be enabled/disabled. Right now it cannot be, but I believe Frederic is working on making it configurable again.

David

Yep. Will Deacon is working on making the breakpoints able to process pure arch informations (ie: without beeing forced to use the perf attr as a midlayer to define them).

Once we have that I can seperate the breakpoints implementation from perf and make it opt-able.

How do you foresee kdb fitting into this? I see that currently [on x86] we cook up perf_event structures with a specific overflow handler set. If we want to move this over to using a completely arch-defined structure, then we're going to end up with an overflow handler field in both perf_event *and* the arch-specific structure, which doesn't feel right to me.

Quoted 8 lines
Quoted 5 lines

Of course, if the goal is only to separate ptrace (i.e. user debugging) from the perf dependency then we don't need the overflow handler because we'll always just send SIGTRAP to the current task.

Any ideas?

I don't know if we want to convert x86/kgdb to use pure arch breakpoints. If kgdb one day wants to extend this use to generic code, it may be a good idea to keep the things as is. I don't know, I'm adding Jason in Cc.

Quoted 5 lines

I think the important part is to share the allocation code (meaning who owns which break point slots). This is why kgdb/kdb allocates the perf structures. The kgdb code will also directly write data to the slots once it has reserved them it would be good to share this code, but it was not shared because it was not usable early enough in the boot cycle on x86.

Certainly there are others who could consume the same infrastructure such as kprobes.

Jason.

Yeah sure, in any case we want to keep the slot allocation/reservation handled in kernel/event/hw_breakpoint.c

#30 lore raw

PATCH 7/9 KVM: Expose the architectural performance monitoring CPUID leaf

In reply to Gleb Natapov

From: Avi Kivity <[email protected]>

Provide a CPUID leaf that describes the emulated PMU.

---
 arch/x86/kvm/x86.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +22 -1
index 5ea4cb8..56153a9 100644
@@ -2543,6 +2543,28 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	}
 	case 9:
 		break;
+	case 0xa: { /* Architectural Performance Monitoring */
+		struct x86_pmu_capability cap;
+
+		perf_get_x86_pmu_capability(&cap);
+
+		/*
+		 * Only support guest architectural pmu on a host
+		 * with architectural pmu.
+		 */
+		if (!cap.version)
+			memset(&cap, 0, sizeof(cap));
+
+		entry->eax = min(cap.version, 2)
+			| (cap.num_counters_gp << 8)
+			| (cap.bit_width_gp << 16)
+			| (cap.events_mask_len << 24);
+		entry->ebx = cap.events_mask;
+		entry->ecx = 0;
+		entry->edx = cap.num_counters_fixed
+			| (cap.bit_width_fixed << 5);
+		break;
+	}
 	/* function 0xb has additional index. */
 	case 0xb: {
 		int i, level_type;
@@ -2637,7 +2659,6 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 3: /* Processor serial number */
 	case 5: /* MONITOR/MWAIT */
 	case 6: /* Thermal management */
-	case 0xA: /* Architectural Performance Monitoring */
 	case 0x80000007: /* Advanced power management */
 	case 0xC0000002:
 	case 0xC0000003:
Signature · 1 line
1.7.5.3
#31 lore raw

Re: [PATCH 7/9] KVM: Expose the architectural performance monitoring CPUID leaf

Quoted 24 lines · Gleb Natapov

From: Avi Kivity <[email protected]>

Provide a CPUID leaf that describes the emulated PMU.

---
 arch/x86/kvm/x86.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +6 -0
index 5ea4cb8..56153a9 100644
@@ -2543,6 +2543,28 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	}
 	case 9:
 		break;
+	case 0xa: { /* Architectural Performance Monitoring */
+		struct x86_pmu_capability cap;
+
+		perf_get_x86_pmu_capability(&cap);
+
+		/*

arch/x86/kvm/x86.c static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 2561

Quoted 10 lines
arch/x86/kvm/x86.c +10 -0
+		 * Only support guest architectural pmu on a host
+		 * with architectural pmu.
+		 */
+		if (!cap.version)
+			memset(&cap, 0, sizeof(cap));
+
+		entry->eax = min(cap.version, 2)
+			| (cap.num_counters_gp << 8)
+			| (cap.bit_width_gp << 16)
+			| (cap.events_mask_len << 24);

What if num_counters_gp is larger than our array?

arch/x86/kvm/x86.c static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 2565

Quoted 4 lines
arch/x86/kvm/x86.c +4 -0
+		entry->ebx = cap.events_mask;
+		entry->ecx = 0;
+		entry->edx = cap.num_counters_fixed
+			| (cap.bit_width_fixed << 5);

Ditto.

Signature · 2 lines
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.
#32 lore raw

Re: [PATCH 7/9] KVM: Expose the architectural performance monitoring CPUID leaf

Quoted 29 lines · Avi Kivity
Quoted 28 lines · Gleb Natapov

From: Avi Kivity <[email protected]>

Provide a CPUID leaf that describes the emulated PMU.

---
 arch/x86/kvm/x86.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +10 -0
index 5ea4cb8..56153a9 100644
@@ -2543,6 +2543,28 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	}
 	case 9:
 		break;
+	case 0xa: { /* Architectural Performance Monitoring */
+		struct x86_pmu_capability cap;
+
+		perf_get_x86_pmu_capability(&cap);
+
+		/*
+		 * Only support guest architectural pmu on a host
+		 * with architectural pmu.
+		 */
+		if (!cap.version)
Quoted 9 lines

arch/x86/kvm/x86.c static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 2561

Quoted 6 lines
arch/x86/kvm/x86.c +6 -0
+			memset(&cap, 0, sizeof(cap));
+
+		entry->eax = min(cap.version, 2)
+			| (cap.num_counters_gp << 8)
+			| (cap.bit_width_gp << 16)
+			| (cap.events_mask_len << 24);

What if num_counters_gp is larger than our array?

I am using perf defines for array sizes (X86_PMC_MAX_(GENERIC|FIXED)) and perf takes care of checking num_counters_(gp|fixed) boundaries for us.

Quoted 7 lines

arch/x86/kvm/x86.c static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 2565

Quoted 4 lines
arch/x86/kvm/x86.c +4 -0
+		entry->ebx = cap.events_mask;
+		entry->ecx = 0;
+		entry->edx = cap.num_counters_fixed
+			| (cap.bit_width_fixed << 5);

Ditto.

--
Gleb.

#33 lore raw

Re: [PATCH 7/9] KVM: Expose the architectural performance monitoring CPUID leaf

In reply to Gleb Natapov
Quoted 30 lines · Gleb Natapov

From: Avi Kivity <[email protected]>

Provide a CPUID leaf that describes the emulated PMU.

---
 arch/x86/kvm/x86.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +12 -0
index 5ea4cb8..56153a9 100644
@@ -2543,6 +2543,28 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	}
 	case 9:
 		break;
+	case 0xa: { /* Architectural Performance Monitoring */
+		struct x86_pmu_capability cap;
+
+		perf_get_x86_pmu_capability(&cap);
+
+		/*
+		 * Only support guest architectural pmu on a host
+		 * with architectural pmu.
+		 */
+		if (!cap.version)
+			memset(&cap, 0, sizeof(cap));
+

arch/x86/kvm/x86.c static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 2567

Quoted 10 lines
arch/x86/kvm/x86.c +10 -0
+		entry->eax = min(cap.version, 2)
+			| (cap.num_counters_gp << 8)
+			| (cap.bit_width_gp << 16)
+			| (cap.events_mask_len << 24);
+		entry->ebx = cap.events_mask;
+		entry->ecx = 0;
+		entry->edx = cap.num_counters_fixed
+			| (cap.bit_width_fixed << 5);
+		break;
+	}

If PERF_EVENTS is disabled in the host kernel will KVM return the right thing for the guest that might have PERF_EVENTS enabled?

David

Quoted 11 lines
arch/x86/kvm/x86.c +0 -1
 	/* function 0xb has additional index. */
 	case 0xb: {
 		int i, level_type;
@@ -2637,7 +2659,6 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 3: /* Processor serial number */
 	case 5: /* MONITOR/MWAIT */
 	case 6: /* Thermal management */
-	case 0xA: /* Architectural Performance Monitoring */
 	case 0x80000007: /* Advanced power management */
 	case 0xC0000002:
 	case 0xC0000003:
#34 lore raw

Re: [PATCH 7/9] KVM: Expose the architectural performance monitoring CPUID leaf

Quoted 36 lines · David Ahern
Quoted 35 lines · Gleb Natapov

From: Avi Kivity <[email protected]>

Provide a CPUID leaf that describes the emulated PMU.

---
 arch/x86/kvm/x86.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +17 -0
index 5ea4cb8..56153a9 100644
@@ -2543,6 +2543,28 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	}
 	case 9:
 		break;
+	case 0xa: { /* Architectural Performance Monitoring */
+		struct x86_pmu_capability cap;
+
+		perf_get_x86_pmu_capability(&cap);
+
+		/*
+		 * Only support guest architectural pmu on a host
+		 * with architectural pmu.
+		 */
+		if (!cap.version)
+			memset(&cap, 0, sizeof(cap));
+
+		entry->eax = min(cap.version, 2)
+			| (cap.num_counters_gp << 8)
+			| (cap.bit_width_gp << 16)
+			| (cap.events_mask_len << 24);
+		entry->ebx = cap.events_mask;
Quoted 9 lines

arch/x86/kvm/x86.c static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 2567

Quoted 5 lines
arch/x86/kvm/x86.c +5 -0
+		entry->ecx = 0;
+		entry->edx = cap.num_counters_fixed
+			| (cap.bit_width_fixed << 5);
+		break;
+	}

If PERF_EVENTS is disabled in the host kernel will KVM return the right thing for the guest that might have PERF_EVENTS enabled?

With correct implementation of perf_get_x86_pmu_capability() it should. PERF_EVENTS disable version should set cap.version to zero, so guest will find that vcpu does not provide architectural PMU.

Quoted 14 lines

David

arch/x86/kvm/x86.c static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 2664

Quoted 11 lines
arch/x86/kvm/x86.c +0 -1
 	/* function 0xb has additional index. */
 	case 0xb: {
 		int i, level_type;
@@ -2637,7 +2659,6 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	case 3: /* Processor serial number */
 	case 5: /* MONITOR/MWAIT */
 	case 6: /* Thermal management */
-	case 0xA: /* Architectural Performance Monitoring */
 	case 0x80000007: /* Advanced power management */
 	case 0xC0000002:
 	case 0xC0000003:

--
Gleb.

#35 lore raw

Re: [PATCH 7/9] KVM: Expose the architectural performance monitoring CPUID leaf

Quoted 46 lines · Gleb Natapov
Quoted 36 lines · David Ahern
Quoted 35 lines · Gleb Natapov

From: Avi Kivity <[email protected]>

Provide a CPUID leaf that describes the emulated PMU.

---
 arch/x86/kvm/x86.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +17 -0
index 5ea4cb8..56153a9 100644
@@ -2543,6 +2543,28 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	}
 	case 9:
 		break;
+	case 0xa: { /* Architectural Performance Monitoring */
+		struct x86_pmu_capability cap;
+
+		perf_get_x86_pmu_capability(&cap);
+
+		/*
+		 * Only support guest architectural pmu on a host
+		 * with architectural pmu.
+		 */
+		if (!cap.version)
+			memset(&cap, 0, sizeof(cap));
+
+		entry->eax = min(cap.version, 2)
+			| (cap.num_counters_gp << 8)
+			| (cap.bit_width_gp << 16)
+			| (cap.events_mask_len << 24);
+		entry->ebx = cap.events_mask;
Quoted 9 lines

arch/x86/kvm/x86.c static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 2567

Quoted 5 lines
arch/x86/kvm/x86.c +5 -0
+		entry->ecx = 0;
+		entry->edx = cap.num_counters_fixed
+			| (cap.bit_width_fixed << 5);
+		break;
+	}

If PERF_EVENTS is disabled in the host kernel will KVM return the right thing for the guest that might have PERF_EVENTS enabled?

Quoted 3 lines

With correct implementation of perf_get_x86_pmu_capability() it should. PERF_EVENTS disable version should set cap.version to zero, so guest will find that vcpu does not provide architectural PMU.

Ok.

I was also thinking about kvm-kmod where newer KVM source is compiled against older kernels - which at one point had PERF_EVENTS as a configurable parameter. In this case it is possible to have perf events disabled host side yet enabled guest side.

David

#36 lore raw

Re: [PATCH 7/9] KVM: Expose the architectural performance monitoring CPUID leaf

Quoted 53 lines · David Ahern
Quoted 46 lines · Gleb Natapov
Quoted 36 lines · David Ahern
Quoted 35 lines · Gleb Natapov

From: Avi Kivity <[email protected]>

Provide a CPUID leaf that describes the emulated PMU.

---
 arch/x86/kvm/x86.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)
arch/x86/kvm/x86.c +17 -0
index 5ea4cb8..56153a9 100644
@@ -2543,6 +2543,28 @@ static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	}
 	case 9:
 		break;
+	case 0xa: { /* Architectural Performance Monitoring */
+		struct x86_pmu_capability cap;
+
+		perf_get_x86_pmu_capability(&cap);
+
+		/*
+		 * Only support guest architectural pmu on a host
+		 * with architectural pmu.
+		 */
+		if (!cap.version)
+			memset(&cap, 0, sizeof(cap));
+
+		entry->eax = min(cap.version, 2)
+			| (cap.num_counters_gp << 8)
+			| (cap.bit_width_gp << 16)
+			| (cap.events_mask_len << 24);
+		entry->ebx = cap.events_mask;
Quoted 9 lines

arch/x86/kvm/x86.c static void do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, · line 2567

Quoted 5 lines
arch/x86/kvm/x86.c +5 -0
+		entry->ecx = 0;
+		entry->edx = cap.num_counters_fixed
+			| (cap.bit_width_fixed << 5);
+		break;
+	}

If PERF_EVENTS is disabled in the host kernel will KVM return the right thing for the guest that might have PERF_EVENTS enabled?

Quoted 3 lines

With correct implementation of perf_get_x86_pmu_capability() it should. PERF_EVENTS disable version should set cap.version to zero, so guest will find that vcpu does not provide architectural PMU.

Quoted 7 lines

Ok.

I was also thinking about kvm-kmod where newer KVM source is compiled against older kernels - which at one point had PERF_EVENTS as a configurable parameter. In this case it is possible to have perf events disabled host side yet enabled guest side.

KVM PMU implementation uses host perf events to talk to HW. Without host perf infrastructure KVM will not be able to provide PMU to a guest. Or are you saying that we should support compiling KVM with !PERF_EVENTS? I'd rather just make KVM select PERF_EVENTS.

--
Gleb.

#37 lore raw

Re: [PATCH 7/9] KVM: Expose the architectural performance monitoring CPUID leaf

Quoted 14 lines · Gleb Natapov
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

If PERF_EVENTS is disabled in the host kernel will KVM return the right thing for the guest that might have PERF_EVENTS enabled?

With correct implementation of perf_get_x86_pmu_capability() it should. PERF_EVENTS disable version should set cap.version to zero, so guest will find that vcpu does not provide architectural PMU.

Quoted 7 lines

Ok.

I was also thinking about kvm-kmod where newer KVM source is compiled against older kernels - which at one point had PERF_EVENTS as a configurable parameter. In this case it is possible to have perf events disabled host side yet enabled guest side.

Quoted 4 lines

KVM PMU implementation uses host perf events to talk to HW. Without host perf infrastructure KVM will not be able to provide PMU to a guest. Or are you saying that we should support compiling KVM with !PERF_EVENTS? I'd rather just make KVM select PERF_EVENTS.

Strictly speaking KVM does not require PERF_EVENTS; it only requires it if a PMU is to be offered to the guest -- which is a new KVM feature.

Not requiring PERF_EVENTS makes kvm-kmod a bit simpler, a use case where the kvm source deals with a pre-configured kernel.

David

#38 lore raw

Re: [PATCH 7/9] KVM: Expose the architectural performance monitoring CPUID leaf

Quoted 22 lines · David Ahern
Quoted 14 lines · Gleb Natapov
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines

If PERF_EVENTS is disabled in the host kernel will KVM return the right thing for the guest that might have PERF_EVENTS enabled?

With correct implementation of perf_get_x86_pmu_capability() it should. PERF_EVENTS disable version should set cap.version to zero, so guest will find that vcpu does not provide architectural PMU.

Quoted 7 lines

Ok.

I was also thinking about kvm-kmod where newer KVM source is compiled against older kernels - which at one point had PERF_EVENTS as a configurable parameter. In this case it is possible to have perf events disabled host side yet enabled guest side.

Quoted 4 lines

KVM PMU implementation uses host perf events to talk to HW. Without host perf infrastructure KVM will not be able to provide PMU to a guest. Or are you saying that we should support compiling KVM with !PERF_EVENTS? I'd rather just make KVM select PERF_EVENTS.

Quoted 6 lines

Strictly speaking KVM does not require PERF_EVENTS; it only requires it if a PMU is to be offered to the guest -- which is a new KVM feature.

Not requiring PERF_EVENTS makes kvm-kmod a bit simpler, a use case where the kvm source deals with a pre-configured kernel.

Making kvm-kmod life simpler is not consideration for new KVM features and in KVM we usually do note make optional features, but just select required subsystem. That said it shouldn't be hard to make KVM PMU be dependant on PERF_EVENTS, but since I can't disable PERF_EVENTS right now I can't test it either, so for now I'll just add select PERF_EVENTS to KVM.

--
Gleb.

#39 lore raw

PATCH 8/9 KVM: x86 emulator: fix RDPMC privilege check

In reply to Gleb Natapov

From: Avi Kivity <[email protected]>

RDPMC is only privileged if CR4.PCE=0. check_rdpmc() already implements this, so all we need to do is drop the Priv flag.

---
 arch/x86/kvm/emulate.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kvm/emulate.c +1 -1
index 8547958..c0ee85b 100644
@@ -3254,7 +3254,7 @@ static struct opcode twobyte_table[256] = {
 	DI(ImplicitOps | Priv, wrmsr),
 	IIP(ImplicitOps, em_rdtsc, rdtsc, check_rdtsc),
 	DI(ImplicitOps | Priv, rdmsr),
-	DIP(ImplicitOps | Priv, rdpmc, check_rdpmc),
+	DIP(ImplicitOps, rdpmc, check_rdpmc),
 	I(ImplicitOps | VendorSpecific, em_sysenter),
 	I(ImplicitOps | Priv | VendorSpecific, em_sysexit),
 	N, N,
Signature · 1 line
1.7.5.3
#40 lore raw

PATCH 9/9 KVM: x86 emulator: implement RDPMC (0F 33)

In reply to Gleb Natapov

From: Avi Kivity <[email protected]>

---
 arch/x86/include/asm/kvm_emulate.h |    1 +
 arch/x86/kvm/emulate.c             |   13 ++++++++++++-
 arch/x86/kvm/x86.c                 |    7 +++++++
 3 files changed, 20 insertions(+), 1 deletions(-)
arch/x86/include/asm/kvm_emulate.h +1 -0
index 9a4acf4..ab4092e 100644
@@ -181,6 +181,7 @@ struct x86_emulate_ops {
 	int (*set_dr)(struct x86_emulate_ctxt *ctxt, int dr, ulong value);
 	int (*set_msr)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 data);
 	int (*get_msr)(struct x86_emulate_ctxt *ctxt, u32 msr_index, u64 *pdata);
+	int (*read_pmc)(struct x86_emulate_ctxt *ctxt, u32 pmc, u64 *pdata);
 	void (*halt)(struct x86_emulate_ctxt *ctxt);
 	void (*wbinvd)(struct x86_emulate_ctxt *ctxt);
 	int (*fix_hypercall)(struct x86_emulate_ctxt *ctxt);
arch/x86/kvm/emulate.c +12 -1
index c0ee85b..d76a852 100644
@@ -2623,6 +2623,17 @@ static int em_rdtsc(struct x86_emulate_ctxt *ctxt)
 	return X86EMUL_CONTINUE;
 }
 
+static int em_rdpmc(struct x86_emulate_ctxt *ctxt)
+{
+	u64 pmc;
+
+	if (ctxt->ops->read_pmc(ctxt, ctxt->regs[VCPU_REGS_RCX], &pmc))
+		return emulate_gp(ctxt, 0);
+	ctxt->regs[VCPU_REGS_RAX] = (u32)pmc;
+	ctxt->regs[VCPU_REGS_RDX] = pmc >> 32;
+	return X86EMUL_CONTINUE;
+}
+
 static int em_mov(struct x86_emulate_ctxt *ctxt)
 {
 	ctxt->dst.val = ctxt->src.val;
@@ -3254,7 +3265,7 @@ static struct opcode twobyte_table[256] = {
 	DI(ImplicitOps | Priv, wrmsr),
 	IIP(ImplicitOps, em_rdtsc, rdtsc, check_rdtsc),
 	DI(ImplicitOps | Priv, rdmsr),
-	DIP(ImplicitOps, rdpmc, check_rdpmc),
+	IIP(ImplicitOps, em_rdpmc, rdpmc, check_rdpmc),
 	I(ImplicitOps | VendorSpecific, em_sysenter),
 	I(ImplicitOps | Priv | VendorSpecific, em_sysexit),
 	N, N,
arch/x86/kvm/x86.c +7 -0
index 56153a9..b1bd52f 100644
@@ -4654,6 +4654,12 @@ static int emulator_set_msr(struct x86_emulate_ctxt *ctxt,
 	return kvm_set_msr(emul_to_vcpu(ctxt), msr_index, data);
 }
 
+static int emulator_read_pmc(struct x86_emulate_ctxt *ctxt,
+			     u32 pmc, u64 *pdata)
+{
+	return kvm_pmu_read_pmc(emul_to_vcpu(ctxt), pmc, pdata);
+}
+
 static void emulator_halt(struct x86_emulate_ctxt *ctxt)
 {
 	emul_to_vcpu(ctxt)->arch.halt_request = 1;
@@ -4706,6 +4712,7 @@ static struct x86_emulate_ops emulate_ops = {
 	.set_dr              = emulator_set_dr,
 	.set_msr             = emulator_set_msr,
 	.get_msr             = emulator_get_msr,
+	.read_pmc            = emulator_read_pmc,
 	.halt                = emulator_halt,
 	.wbinvd              = emulator_wbinvd,
 	.fix_hypercall       = emulator_fix_hypercall,
Signature · 1 line
1.7.5.3
#41 lore raw
In reply to Gleb Natapov

Hmm, linux-kernel address is broken. Sorry about that. Just drop it when replying please. The patchset is mostly interesting to kvm developers anyway.

Quoted 85 lines · Gleb Natapov

This patchset exposes an emulated version 2 architectural performance monitoring unit to KVM guests. The PMU is emulated using perf_events, so the host kernel can multiplex host-wide, host-user, and the guest on available resources.

The patches should be applied on top of KVM patches from the patch series "[PATCH v2 0/9] perf support for x86 guest/host-only bits" [1]

If you want to try running perf in a guest you need to apply the patch below to qemu-kvm and use -cpu host on qemu command line. But DO NOT TRY those patches without applying [2][3] to the host kernel first. Don't tell me I didn't warn you!

[1] https://lkml.org/lkml/2011/10/5/153
[2] https://lkml.org/lkml/2011/10/18/390
[3] https://lkml.org/lkml/2011/10/23/163

Avi Kivity (8):
KVM: Expose kvm_lapic_local_deliver()
KVM: Expose a version 2 architectural PMU to a guests
KVM: Add generic RDPMC support
KVM: SVM: Intercept RDPMC
KVM: VMX: Intercept RDPMC
KVM: Expose the architectural performance monitoring CPUID leaf
KVM: x86 emulator: fix RDPMC privilege check
KVM: x86 emulator: implement RDPMC (0F 33)

Gleb Natapov (1):
perf: expose perf capability to other modules.

 arch/x86/include/asm/kvm_emulate.h     |    1 +
 arch/x86/include/asm/kvm_host.h        |   44 +++
 arch/x86/include/asm/perf_event.h      |   11 +
 arch/x86/kernel/cpu/perf_event.c       |   11 +
 arch/x86/kernel/cpu/perf_event.h       |    2 +
 arch/x86/kernel/cpu/perf_event_intel.c |    3 +
 arch/x86/kvm/Makefile                  |    2 +-
 arch/x86/kvm/emulate.c                 |   13 +-
 arch/x86/kvm/lapic.c                   |    2 +-
 arch/x86/kvm/lapic.h                   |    1 +
 arch/x86/kvm/pmu.c                     |  513 ++++++++++++++++++++++++++++++++
 arch/x86/kvm/svm.c                     |   15 +
 arch/x86/kvm/vmx.c                     |   15 +-
 arch/x86/kvm/x86.c                     |   65 ++++-
 include/linux/kvm_host.h               |    1 +
 15 files changed, 686 insertions(+), 13 deletions(-)
 create mode 100644 arch/x86/kvm/pmu.c
target-i386/cpuid.c +14 -5
index f179999..ff2a0ca 100644
@@ -1178,11 +1178,20 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         *edx = 0;
         break;
     case 0xA:
-        /* Architectural Performance Monitoring Leaf */
-        *eax = 0;
-        *ebx = 0;
-        *ecx = 0;
-        *edx = 0;
+	if (kvm_enabled()) {
+            KVMState *s = env->kvm_state;
+
+            *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX);
+            *ebx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EBX);
+            *ecx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_ECX);
+            *edx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EDX);
+	} else {
+		/* Architectural Performance Monitoring Leaf */
+		*eax = 0; //0x07280402;
+		*ebx = 0;
+		*ecx = 0;
+		*edx = 0; //0x00000503;
+	}
         break;
     case 0xD:
         /* Processor Extended State */
Signature · 6 lines
1.7.5.3

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Gleb.