Skip to contentSkip to conversation

vPMU support for AMD system

RFC In review Wei Huang 3 participants 5 messages Oct 1, 2014 – Oct 6, 2014

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Wei Huang

author
#0 lore raw

RFC 1/1 vPMU support for AMD system

Hi Paolo and Gleb,

The attached file is a preliminary version of AMD vPMU support for KVM. Currently I am working on a formal patch set; but realized that there are some design choice to make (see below). I thought it is better to send it out now, asking for your comments before sending out patchset v1.

If you are OK with current approach, please let me know. Otherwise please share your suggestions/comments on the design choice (see suggestions below). I will send out split patch set soon.

Thanks,
-Wei

================
1. OBJECTIVE
To enable PMU for guest VMs running on AMD host. Currently only software events are supported for AMD guest VMs. This is useless to profile guest VM on AMD systems.

2. CURRENT DESIGN
The design is similar to existing vPMU support. Many data structures such as "struct kvm_pmc" and "struct kvm_pmu" are re-used for this design.

Original pmu.c code was designed with Intel PMU in mind. To avoid the confusion, the current design converts existing pmu.c into intel-pmu.c and create a similar code, named amd-pmu.c. PMU function calls in x86.c are then dispatched via function pointers (i.e. struct kvm_pmu_ops). The initial setup is done in kvm_pmu_arch_init().

3. STATUS
I have briefly tested using AMD machine and the profiling data seems to be fairly accurate. With that said, more tests are required especially on PMI and extensive HW perf events.

4. DESIGN QUESTIONS
The current design is described in (2). However it is not perfect, especially due to the code similarity between intel-pmu.c and amd-pmu.c.

I have other ideas listed below. Please review and give some suggestion:

(a) Merge intel-pmu.c and amd-pmu.c into pmu.c
* PMU will probe the architecture first before first usage;
* It will still use function pointers to dispatch global functions (e.g. kvm_pmu_reset);
* For static functions that are the same (e.g. pmc_is_gp), we will use the same copy;
* For static functions that are slightly different (e.g. global_idx_to_pmc), we will use a shared version with if-else statement, depending on Intel or AMD.

I think this approach is quite acceptable, except that there will be many if-else in the code. Not clean enough.

(b) Convert intel-pmu.c => vmx.c and convert amd-pmu.c => svm.c
* PMU function pointers will be created in kvm_x86_ops;
* The entry functions will be created inside vmx.c and svm.c respectively;
* There might be common functions defined in pmu.c.

This design is viable too. But to be honest, it is a bit messy compared with (a).

==========================

arch/x86/include/asm/kvm_host.h +30 -17
index 9f83c17..6fc856f 100644
@@ -305,6 +305,12 @@ enum pmc_type {
 	KVM_PMC_FIXED,
 };
 
+struct msr_data {
+	bool host_initiated;
+	u32 index;
+	u64 data;
+};
+
 struct kvm_pmc {
 	enum pmc_type type;
 	u8 idx;

@@ -314,6 +320,27 @@ struct kvm_pmc {
 	struct kvm_vcpu *vcpu;
 };
 
+/* mapping from event selection to PERF_TYPE_HAARDWARE events */
+struct kvm_event_hw_type_mapping {
+	u8 eventsel;
+	u8 unit_mask;
+	unsigned event_type;
+	bool inexact;
+};
+
+struct kvm_pmu_ops {
+	void (*pmu_cpuid_update)(struct kvm_vcpu *vcpu);
+	int (*pmu_read_pmc)(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
+	int (*pmu_set_msr)(struct kvm_vcpu *vcpu, struct msr_data *msr_info);
+	int (*pmu_get_msr)(struct kvm_vcpu *vcpu, u32 index, u64 *data);
+	bool (*is_pmu_msr)(struct kvm_vcpu *vcpu, u32 msr);
+	void (*pmu_deliver_pmi)(struct kvm_vcpu *vcpu);
+	void (*pmu_handle_event)(struct kvm_vcpu *vcpu);
+	void (*pmu_reset)(struct kvm_vcpu *vcpu);
+	void (*pmu_init)(struct kvm_vcpu *vcpu);
+	void (*pmu_destroy)(struct kvm_vcpu *vcpu);
+};
+
 struct kvm_pmu {
 	unsigned nr_arch_gp_counters;
 	unsigned nr_arch_fixed_counters;

@@ -654,12 +681,6 @@ struct kvm_vcpu_stat {
 
 struct x86_instruction_info;
 
-struct msr_data {
-	bool host_initiated;
-	u32 index;
-	u64 data;
-};
-
 struct kvm_x86_ops {
 	int (*cpu_has_kvm_support)(void);          /* __init */
 	int (*disabled_by_bios)(void);             /* __init */

@@ -780,6 +801,7 @@ struct kvm_arch_async_pf {
 };
 
 extern struct kvm_x86_ops *kvm_x86_ops;
+extern struct kvm_pmu_ops *kvm_pmu_ops;
 
 static inline void adjust_tsc_offset_guest(struct kvm_vcpu *vcpu,
 					   s64 adjustment)

@@ -1063,15 +1085,6 @@ void kvm_complete_insn_gp(struct kvm_vcpu *vcpu, int err);
 
 int kvm_is_in_guest(void);
 
-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, struct msr_data *msr_info);
-int kvm_pmu_read_pmc(struct kvm_vcpu *vcpu, unsigned pmc, u64 *data);
-void kvm_handle_pmu_event(struct kvm_vcpu *vcpu);
-void kvm_deliver_pmi(struct kvm_vcpu *vcpu);
-
+extern struct kvm_pmu_ops intel_pmu_ops;
+extern struct kvm_pmu_ops amd_pmu_ops;
 #endif /* _ASM_X86_KVM_HOST_H */
arch/x86/kvm/Makefile +1 -1
index 25d22b2..187ad0a 100644
@@ -14,7 +14,7 @@ kvm-$(CONFIG_KVM_DEVICE_ASSIGNMENT)	+= $(KVM)/assigned-dev.o $(KVM)/iommu.o
 kvm-$(CONFIG_KVM_ASYNC_PF)	+= $(KVM)/async_pf.o
 
 kvm-y			+= x86.o mmu.o emulate.o i8259.o irq.o lapic.o \
-			   i8254.o cpuid.o pmu.o
+			   i8254.o cpuid.o intel-pmu.o amd-pmu.o
 kvm-intel-y		+= vmx.o
 kvm-amd-y		+= svm.o
arch/x86/kvm/amd-pmu.c new file +381 -0

381 lines not on this page · raw message

arch/x86/kvm/cpuid.c +1 -1
index 38a0afe..de5ee45 100644
@@ -88,7 +88,7 @@ void kvm_update_cpuid(struct kvm_vcpu *vcpu)
 			xstate_required_size(vcpu->arch.xcr0);
 	}
 
-	kvm_pmu_cpuid_update(vcpu);
+	kvm_pmu_ops->pmu_cpuid_update(vcpu);
 }
 
 static int is_efer_nx(void)
arch/x86/kvm/intel-pmu.c new file +572 -0

572 lines not on this page · raw message

arch/x86/kvm/pmu.c deleted +0 -563

563 lines not on this page · raw message

arch/x86/kvm/x86.c +48 -29

177 lines not on this page · raw message

#1 lore raw

2014-09-30 21:07-0500, Wei Huang:

Quoted 13 lines

Hi Paolo and Gleb,
2. CURRENT DESIGN
The design is similar to existing vPMU support. Many data structures such as "struct kvm_pmc" and "struct kvm_pmu" are re-used for this design.

Original pmu.c code was designed with Intel PMU in mind. To avoid the confusion, the current design converts existing pmu.c into intel-pmu.c and create a similar code, named amd-pmu.c. PMU function calls in x86.c are then dispatched via function pointers (i.e. struct kvm_pmu_ops). The initial setup is done in kvm_pmu_arch_init().

I have other ideas listed below. Please review and give some suggestion:

Quoted 9 lines

(a) Merge intel-pmu.c and amd-pmu.c into pmu.c
* PMU will probe the architecture first before first usage;
* It will still use function pointers to dispatch global functions (e.g. kvm_pmu_reset);
* For static functions that are the same (e.g. pmc_is_gp), we will use the same copy;
* For static functions that are slightly different (e.g. global_idx_to_pmc), we will use a shared version with if-else statement, depending on Intel or AMD.

(if-else forest is daunting.)

Quoted 6 lines

I think this approach is quite acceptable, except that there will be many if-else in the code. Not clean enough.

(b) Convert intel-pmu.c => vmx.c and convert amd-pmu.c => svm.c
* PMU function pointers will be created in kvm_x86_ops;
* The entry functions will be created inside vmx.c and svm.c respectively;

I would be nicer to keep them in separate files and link to vmx/svm.

Quoted 4 lines

* There might be common functions defined in pmu.c.

This design is viable too. But to be honest, it is a bit messy compared with (a).

This makes sense as we will use only vmx+intel_pmu and svm+amd_pmu, so we'd have less code loaded in both cases.

I consider design (c) strictly better than the current one (2):
(c) keep {intel,amd}-pmu.c and introduce pmu.[hc] that joins duplicate functions and wraps kvm_pmu_ops

And if we decide to move VMX/SVM related code into their respective modules, we won't have to change callers.

author
#2 lore raw
Quoted 17 lines

(if-else forest is daunting.)

Quoted 6 lines

I think this approach is quite acceptable, except that there will be many if-else in the code. Not clean enough.

(b) Convert intel-pmu.c => vmx.c and convert amd-pmu.c => svm.c
* PMU function pointers will be created in kvm_x86_ops;
* The entry functions will be created inside vmx.c and svm.c respectively;

I would be nicer to keep them in separate files and link to vmx/svm.

Quoted 4 lines

* There might be common functions defined in pmu.c.

This design is viable too. But to be honest, it is a bit messy compared with (a).

Quoted 6 lines

This makes sense as we will use only vmx+intel_pmu and svm+amd_pmu, so we'd have less code loaded in both cases.

I consider design (c) strictly better than the current one (2):
(c) keep {intel,amd}-pmu.c and introduce pmu.[hc] that joins duplicate functions and wraps kvm_pmu_ops

Thanks for the suggestion. I will take this into consideration. Your suggestion is similar to existing svm.c/vmx.c/x86.c, which split distinct functions in vmx.c/svm.c; but in the meanwhile merging them in x86.c for common code.

Quoted 4 lines

And if we decide to move VMX/SVM related code into their respective modules, we won't have to change callers.

Yes.

#3 lore raw
In reply to Wei Huang

Hi,

Quoted 11 lines · Wei Huang

Hi Paolo and Gleb,

The attached file is a preliminary version of AMD vPMU support for KVM. Currently I am working on a formal patch set; but realized that there are some design choice to make (see below). I thought it is better to send it out now, asking for your comments before sending out patchset v1.

If you are OK with current approach, please let me know. Otherwise please share your suggestions/comments on the design choice (see suggestions below). I will send out split patch set soon.

I am not very familiar with AMD PMU, but IIRC is lacks architectural PMU, so the first question that comes to my mind is:

arch/x86/kvm/amd-pmu.c line 29

Quoted 9 lines
arch/x86/kvm/amd-pmu.c new file +9 -0
+static struct kvm_event_hw_type_mapping amd_event_mapping[] = {
+	[0] = { 0x76, 0x00, PERF_COUNT_HW_CPU_CYCLES },
+	[1] = { 0xc0, 0x00, PERF_COUNT_HW_INSTRUCTIONS },
+	[3] = { 0x80, 0x00, PERF_COUNT_HW_CACHE_REFERENCES },
+	[4] = { 0x81, 0x00, PERF_COUNT_HW_CACHE_MISSES },
+	[5] = { 0xc4, 0x00, PERF_COUNT_HW_BRANCH_INSTRUCTIONS },
+	[6] = { 0xc5, 0x00, PERF_COUNT_HW_BRANCH_MISSES },
+};
+

Are those evens count exactly same things on all AMD cpus?

--
Gleb.

author
#4 lore raw

This

Quoted 17 lines · Gleb Natapov

Hi,

Quoted 11 lines · Wei Huang

Hi Paolo and Gleb,

The attached file is a preliminary version of AMD vPMU support for KVM. Currently I am working on a formal patch set; but realized that there are some design choice to make (see below). I thought it is better to send it out now, asking for your comments before sending out patchset v1.

If you are OK with current approach, please let me know. Otherwise please share your suggestions/comments on the design choice (see suggestions below). I will send out split patch set soon.

I am not very familiar with AMD PMU, but IIRC is lacks architectural PMU, so the first question that comes to my mind is:

Quoted 10 lines

arch/x86/kvm/amd-pmu.c line 29

Quoted 9 lines
arch/x86/kvm/amd-pmu.c new file +9 -0
+static struct kvm_event_hw_type_mapping amd_event_mapping[] = {
+	[0] = { 0x76, 0x00, PERF_COUNT_HW_CPU_CYCLES },
+	[1] = { 0xc0, 0x00, PERF_COUNT_HW_INSTRUCTIONS },
+	[3] = { 0x80, 0x00, PERF_COUNT_HW_CACHE_REFERENCES },
+	[4] = { 0x81, 0x00, PERF_COUNT_HW_CACHE_MISSES },
+	[5] = { 0xc4, 0x00, PERF_COUNT_HW_BRANCH_INSTRUCTIONS },
+	[6] = { 0xc5, 0x00, PERF_COUNT_HW_BRANCH_MISSES },
+};
+

Are those evens count exactly same things on all AMD cpus?

These mappings were pulled off from Linux perf code. From the code and several AMD BKDGs I read, they were consistent across CPU families.

If there is no guaranteed consistency, I think we can fall back to raw perf counters. So this amd_event_mapping table can be removed.

-Wei

Quoted 5 lines

--
Gleb.