On 4/9/15 14:43, Radim Krčmář wrote:
2015-04-08 12:18-0400, Wei Huang:
This patch converts existing Intel specific vPMU code into a common vPMU
interface with the following steps:
- A large portion of Intel vPMU code is now moved to pmu.c file. The rest
is re-arranged and hooked up with the newly-defined intel_pmu_ops.
- Create a corresponding pmu_amd.c file with empty functions for AMD SVM
- The PMU function pointer, kvm_pmu_ops, is initialized by calling
kvm_x86_ops->get_pmu_ops().
- To reduce the code size, Intel and AMD modules are now generated
from their corrponding arch and PMU files; In the meanwhile, due
to this arrangement several functions are exposed as public ones
to allow calling from PMU code.
Signed-off-by: Wei Huang <wei@xxxxxxxxxx>
---
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
| [...]
+static void pmc_reprogram_counter(struct kvm_pmc *pmc, u32 type,
+ unsigned config, bool exclude_user,
+ bool exclude_kernel, bool intr,
This patch introduces a lot of trailing whitespaces, please remove them.
(`git am` says 15.)
Some of them were carried from original pmu.c file. I will purge them in V3.
| [...]
+EXPORT_SYMBOL_GPL(reprogram_counter);
+
(double line)
OK
+
+void kvm_pmu_handle_event(struct kvm_vcpu *vcpu)
| [...]
+/* check if msr_idx is a valid index to access PMU */
+inline int kvm_pmu_check_msr_idx(struct kvm_vcpu *vcpu, unsigned msr_idx)
If we really want it inline, it's better done in header.
(I think GCC would inline this in-module anyway, but other modules still
have to call it.)
OK
+{
+ return kvm_pmu_ops->check_msr_idx(vcpu, msr_idx);
+}
+
| [...]
+bool kvm_pmu_is_msr(struct kvm_vcpu *vcpu, u32 msr)
(Might make sense to inline these trivial wrappers.)
OK, I will move them to pmu.h header as inline.
+{
+ return kvm_pmu_ops->is_pmu_msr(vcpu, msr);
+}
+
+int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, u32 msr, u64 *data)
+{
+ return kvm_pmu_ops->get_msr(vcpu, msr, data);
+}
+
+int kvm_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
+{
+ return kvm_pmu_ops->set_msr(vcpu, msr_info);
+
(Technically also a trailing newline.)
OK
+}
| [...]
+/* Called before using any PMU functions above */
+int kvm_pmu_arch_init(struct kvm_x86_ops *x86_ops)
+{
+ kvm_pmu_ops = x86_ops->get_pmu_ops();
I guess you forgot this line ^^.
Ahh, will fix it.
+
+ if (x86_ops && (kvm_pmu_ops = x86_ops->get_pmu_ops()) != NULL)
+ return 0;
+ else
+ return -EINVAL;
+}
diff --git a/arch/x86/kvm/pmu.h b/arch/x86/kvm/pmu.h
@@ -0,0 +1,98 @@
+#ifndef __KVM_X86_PMU_H
+#define __KVM_X86_PMU_H
+
+#define VCPU_TO_PMU(vcpu) (&(vcpu)->arch.pmu)
+#define PMU_TO_VCPU(pmu) (container_of((pmu), struct kvm_vcpu, arch.pmu))
+#define PMC_TO_PMU(pmc) (&(pmc)->vcpu->arch.pmu)
(We are replacing inline functions with these macros and they wouldn't
have been in caps, so I would use lower case; looks better too, IMO.)
OK, will make it to lower case.
+/* retrieve the control fields of IA32_FIXED_CTR_CTRL */
+#define fixed_ctr_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*4)) & 0xf)
| [...]
diff --git a/arch/x86/kvm/pmu_intel.c b/arch/x86/kvm/pmu_intel.c
@@ -328,20 +156,21 @@ bool kvm_pmu_msr(struct kvm_vcpu *vcpu, u32 msr)
ret = pmu->version > 1;
break;
default:
- ret = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0)
- || get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0)
- || get_fixed_pmc(pmu, msr);
+ ret = get_gp_pmc(pmu, msr, MSR_IA32_PERFCTR0) ||
+ get_gp_pmc(pmu, msr, MSR_P6_EVNTSEL0) ||
+ get_fixed_pmc(pmu, msr);
break;
}
+
return ret;
}
(This hunk and the following hunks where you mostly change index -> msr
could have been in a separate cleanup patch.)
OK, agreed that such changes can be separated. I will extract them to
make your life easier.
-int kvm_pmu_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data)
+static int intel_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)
+static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
| [...]
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 32bf19e..4d9e7de 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -28,6 +28,7 @@
#include "x86.h"
#include "cpuid.h"
#include "assigned-dev.h"
+#include "pmu.h"
#include <linux/clocksource.h>
#include <linux/interrupt.h>
@@ -899,7 +900,7 @@ bool kvm_rdpmc(struct kvm_vcpu *vcpu)
u64 data;
int err;
- err = kvm_pmu_read_pmc(vcpu, ecx, &data);
+ err = kvm_pmu_rdpmc(vcpu, ecx, &data);
(What was the reason behind the new name?)
Original kvm_pmu_read_pmc() was only used for RDPMC instruction. Such
change make it easier to be correlated with RDPMC directly; plus pmc is
a confusing term in vPMU code (see "Design Note" in pmu.c file), which
makes me to think kvm_pmu_read_pmc() should read from "struct kvm_pmc"
directly.
if (err)
return err;
kvm_register_write(vcpu, VCPU_REGS_RAX, (u32)data);
@@ -2279,7 +2280,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
pr = true;
case MSR_P6_EVNTSEL0:
case MSR_P6_EVNTSEL1:
- if (kvm_pmu_msr(vcpu, msr))
+ if (kvm_pmu_is_msr(vcpu, msr))
(I liked kvm_pmu_msr better. The 'is' is in a wrong spot ... we know it
is "a MSR", we want to know if it is "PMU MSR".)
OK
@@ -4918,13 +4919,13 @@ static int emulator_set_msr(struct x86_emulate_ctxt *ctxt,
static int emulator_check_pmc(struct x86_emulate_ctxt *ctxt,
u32 pmc)
{
- return kvm_pmu_check_pmc(emul_to_vcpu(ctxt), pmc);
+ return kvm_pmu_check_msr_idx(emul_to_vcpu(ctxt), pmc);
(Why not pmc?)
See "Design Note" in pmu.c for a better explanation. I tried to use msr
as real x86 MSR; and msr_idx refers to MSR offset.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
