* [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
@ 2013-06-04 16:02 Bandan Das
2013-06-04 16:02 ` [PATCH v2 1/2] kvm: make vendor_intel a generic function Bandan Das
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Bandan Das @ 2013-06-04 16:02 UTC (permalink / raw)
To: kvm; +Cc: Gleb Natapov, Paolo Bonzini, Marcelo Tosatti
These patches add an emulated MSR_PLATFORM_INFO that kvm guests
can read as described in section 14.3.2.4 of the Intel SDM.
The relevant changes and details are in [2/2]; [1/2] makes vendor_intel
generic. There are atleat two known applications that fail to run because
of this MSR missing - Sandra and vTune.
v2: Addressed suggested changes
Bandan Das (2):
kvm: make vendor_intel a generic function
kvm: x86: emulate MSR_PLATFORM_INFO
arch/x86/include/asm/kvm_emulate.h | 13 ----------
arch/x86/include/asm/kvm_host.h | 20 +++++++++++++++
arch/x86/include/uapi/asm/msr-index.h | 2 ++
arch/x86/kvm/cpuid.c | 19 ++++++++++++++
arch/x86/kvm/cpuid.h | 16 ++++++++++++
arch/x86/kvm/emulate.c | 16 +++---------
arch/x86/kvm/x86.c | 48 +++++++++++++++++++++++++++++++++++
7 files changed, 109 insertions(+), 25 deletions(-)
--
1.8.1.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/2] kvm: make vendor_intel a generic function
2013-06-04 16:02 [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO Bandan Das
@ 2013-06-04 16:02 ` Bandan Das
2013-06-04 23:33 ` Paolo Bonzini
2013-06-04 16:02 ` [PATCH v2 2/2] kvm: x86: emulate MSR_PLATFORM_INFO Bandan Das
2013-06-05 8:42 ` [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO Gleb Natapov
2 siblings, 1 reply; 17+ messages in thread
From: Bandan Das @ 2013-06-04 16:02 UTC (permalink / raw)
To: kvm; +Cc: Gleb Natapov, Paolo Bonzini, Marcelo Tosatti
Make vendor_intel generic so that functions in x86.c
can use it.
v2:
Change vendor_intel function signature because the emulator
shouldn't be dealing with struct vcpu
Signed-off-by: Bandan Das <bsd@redhat.com>
---
arch/x86/include/asm/kvm_emulate.h | 13 -------------
arch/x86/include/asm/kvm_host.h | 20 ++++++++++++++++++++
arch/x86/kvm/emulate.c | 16 ++++------------
3 files changed, 24 insertions(+), 25 deletions(-)
diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
index 15f960c..611a55f 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -319,19 +319,6 @@ struct x86_emulate_ctxt {
#define REPE_PREFIX 0xf3
#define REPNE_PREFIX 0xf2
-/* CPUID vendors */
-#define X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx 0x68747541
-#define X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx 0x444d4163
-#define X86EMUL_CPUID_VENDOR_AuthenticAMD_edx 0x69746e65
-
-#define X86EMUL_CPUID_VENDOR_AMDisbetterI_ebx 0x69444d41
-#define X86EMUL_CPUID_VENDOR_AMDisbetterI_ecx 0x21726574
-#define X86EMUL_CPUID_VENDOR_AMDisbetterI_edx 0x74656273
-
-#define X86EMUL_CPUID_VENDOR_GenuineIntel_ebx 0x756e6547
-#define X86EMUL_CPUID_VENDOR_GenuineIntel_ecx 0x6c65746e
-#define X86EMUL_CPUID_VENDOR_GenuineIntel_edx 0x49656e69
-
enum x86_intercept_stage {
X86_ICTP_NONE = 0, /* Allow zero-init to not match anything */
X86_ICPT_PRE_EXCEPT,
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 3741c65..ce9a44f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -144,6 +144,19 @@ enum {
#include <asm/kvm_emulate.h>
+/* CPUID vendors */
+#define X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx 0x68747541
+#define X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx 0x444d4163
+#define X86EMUL_CPUID_VENDOR_AuthenticAMD_edx 0x69746e65
+
+#define X86EMUL_CPUID_VENDOR_AMDisbetterI_ebx 0x69444d41
+#define X86EMUL_CPUID_VENDOR_AMDisbetterI_ecx 0x21726574
+#define X86EMUL_CPUID_VENDOR_AMDisbetterI_edx 0x74656273
+
+#define X86EMUL_CPUID_VENDOR_GenuineIntel_ebx 0x756e6547
+#define X86EMUL_CPUID_VENDOR_GenuineIntel_ecx 0x6c65746e
+#define X86EMUL_CPUID_VENDOR_GenuineIntel_edx 0x49656e69
+
#define KVM_NR_MEM_OBJS 40
#define KVM_NR_DB_REGS 4
@@ -942,6 +955,13 @@ static inline unsigned long read_msr(unsigned long msr)
}
#endif
+static inline bool vendor_intel(u32 ebx, u32 ecx, u32 edx)
+{
+ return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx
+ && ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx
+ && edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx;
+}
+
static inline u32 get_rdx_init_val(void)
{
return 0x600; /* P6 family */
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 8db0010..87f12fc 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -2280,17 +2280,6 @@ setup_syscalls_segments(struct x86_emulate_ctxt *ctxt,
ss->avl = 0;
}
-static bool vendor_intel(struct x86_emulate_ctxt *ctxt)
-{
- u32 eax, ebx, ecx, edx;
-
- eax = ecx = 0;
- ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
- return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx
- && ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx
- && edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx;
-}
-
static bool em_syscall_is_enabled(struct x86_emulate_ctxt *ctxt)
{
const struct x86_emulate_ops *ops = ctxt->ops;
@@ -2400,6 +2389,7 @@ static int em_sysenter(struct x86_emulate_ctxt *ctxt)
u64 msr_data;
u16 cs_sel, ss_sel;
u64 efer = 0;
+ u32 eax, ebx, ecx, edx;
ops->get_msr(ctxt, MSR_EFER, &efer);
/* inject #GP if in real mode */
@@ -2410,8 +2400,10 @@ static int em_sysenter(struct x86_emulate_ctxt *ctxt)
* Not recognized on AMD in compat mode (but is recognized in legacy
* mode).
*/
+ eax = ecx = 0;
+ ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
if ((ctxt->mode == X86EMUL_MODE_PROT32) && (efer & EFER_LMA)
- && !vendor_intel(ctxt))
+ && !vendor_intel(ebx, ecx, edx))
return emulate_ud(ctxt);
/* XXX sysenter/sysexit have not been tested in 64bit mode.
--
1.8.1.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] kvm: x86: emulate MSR_PLATFORM_INFO
2013-06-04 16:02 [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO Bandan Das
2013-06-04 16:02 ` [PATCH v2 1/2] kvm: make vendor_intel a generic function Bandan Das
@ 2013-06-04 16:02 ` Bandan Das
2013-06-18 14:07 ` Paolo Bonzini
2013-06-05 8:42 ` [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO Gleb Natapov
2 siblings, 1 reply; 17+ messages in thread
From: Bandan Das @ 2013-06-04 16:02 UTC (permalink / raw)
To: kvm; +Cc: Gleb Natapov, Paolo Bonzini, Marcelo Tosatti
To emulate MSR_PLATFORM_INFO, we divide guest virtual_tsc_khz by
the base clock - which is guest CPU model dependent.
The relevant bits in this emulated MSR are :
Max Non-Turbo Ratio (15:8) - virtual_tsc_khz/bclk
Max Effi Ratio (47:40) - virtual_tsc_khz/bclk
Prog Ratio Limit for Trubo (28) - 0
Prog TDC-TDP Limit for Turbo (29) - 0
v2:
Change function name to kvm_cpuid_get_intel_model and
call the new vendor_intel()
Signed-off-by: Bandan Das <bsd@redhat.com>
---
arch/x86/include/uapi/asm/msr-index.h | 2 ++
arch/x86/kvm/cpuid.c | 19 ++++++++++++++
arch/x86/kvm/cpuid.h | 16 ++++++++++++
arch/x86/kvm/x86.c | 48 +++++++++++++++++++++++++++++++++++
4 files changed, 85 insertions(+)
diff --git a/arch/x86/include/uapi/asm/msr-index.h b/arch/x86/include/uapi/asm/msr-index.h
index 2af848d..b0268d5 100644
--- a/arch/x86/include/uapi/asm/msr-index.h
+++ b/arch/x86/include/uapi/asm/msr-index.h
@@ -331,6 +331,8 @@
#define MSR_IA32_MISC_ENABLE 0x000001a0
+#define MSR_IA32_PLATFORM_INFO 0x000000ce
+
#define MSR_IA32_TEMPERATURE_TARGET 0x000001a2
#define MSR_IA32_ENERGY_PERF_BIAS 0x000001b0
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index a20ecb5..ae970d3 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -608,6 +608,25 @@ struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
}
EXPORT_SYMBOL_GPL(kvm_find_cpuid_entry);
+u8 kvm_cpuid_get_intel_model(struct kvm_vcpu *vcpu)
+{
+ struct kvm_cpuid_entry2 *best;
+ u8 cpuid_model, cpuid_extmodel;
+
+ best = kvm_find_cpuid_entry(vcpu, 0, 0);
+ if (!vendor_intel(best->ebx, best->ecx, best->edx))
+ return CPUID_MODEL_UNKNOWN;
+
+ best = kvm_find_cpuid_entry(vcpu, 1, 0);
+
+ cpuid_model = (best->eax >> 4) & 0xf;
+ cpuid_extmodel = (best->eax >> 16) & 0xf;
+
+ cpuid_model += (cpuid_extmodel << 4);
+
+ return cpuid_model;
+}
+
int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
{
struct kvm_cpuid_entry2 *best;
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index b7fd079..081461d 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -3,6 +3,21 @@
#include "x86.h"
+#define MODEL_NEHALEM_CLARKSFIELD 0x1e /* Core i7 and Ex BCLK: 133Mhz */
+#define MODEL_NEHALEM_BLOOMFIELD 0x1a /* Core i7 Xeon 3000 BCLK: 133 Mhz */
+#define MODEL_NEHALEM_EX 0x2e /* Core i7 and i5 Nehalem BCLK: 133 Mhz */
+#define MODEL_WESTMERE_ARRANDALE 0x25 /* Celeron/Pentium/Core i3/i5/i7 BCLK: 133 Mhz */
+#define MODEL_WESTMERE_EX 0x2f /* Xeon E7 BCLK: 133 Mhz */
+#define MODEL_WESTMERE_GULFTOWN 0x2c /* Core i7 Xeon 3000 BCLK: 133 Mhz */
+#define MODEL_SANDYBRIDGE_SANDY 0x2a /* Core/Celeron/Pentium/Xeon BCLK: 133 Mhz */
+#define MODEL_SANDYBRIDGE_E 0x2d /* Core i7 and Ex BCLK: 100 Mhz */
+#define MODEL_IVYBRIDGE_IVY 0x3a /* Core i3/i5/i7 (Ex) and Xeon E3 BCLK: 100 Mhz */
+#define MODEL_HASWELL_HASWELL 0x3c /* BCLK: 100 Mhz */
+#define CPUID_MODEL_UNKNOWN 0x0 /* Everything else */
+
+#define BCLK_133_DEFAULT (133 * 1000)
+#define BCLK_100_DEFAULT (100 * 1000)
+
void kvm_update_cpuid(struct kvm_vcpu *vcpu);
struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
u32 function, u32 index);
@@ -18,6 +33,7 @@ int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
struct kvm_cpuid2 *cpuid,
struct kvm_cpuid_entry2 __user *entries);
void kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx, u32 *ecx, u32 *edx);
+u8 kvm_cpuid_get_intel_model(struct kvm_vcpu *vcpu);
static inline bool guest_cpuid_has_xsave(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 094b5d9..f9b2830 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -239,6 +239,49 @@ void kvm_set_shared_msr(unsigned slot, u64 value, u64 mask)
}
EXPORT_SYMBOL_GPL(kvm_set_shared_msr);
+static u64 kvm_get_platform_info(struct kvm_vcpu *vcpu)
+{
+ u8 cpumodel;
+ u32 bclk;
+
+ /*
+ * Programmable Ratio Limit for Turbo Mode (bit 28): 0
+ * Programmable TDC-TDP Limit for Turbo Mode (bit 29): 0
+ */
+ u64 platform_info = 0, max_nonturbo_ratio = 0, max_effi_ratio = 0;
+
+ cpumodel = kvm_cpuid_get_intel_model(vcpu);
+
+ switch (cpumodel) {
+ case MODEL_NEHALEM_CLARKSFIELD:
+ case MODEL_NEHALEM_BLOOMFIELD:
+ case MODEL_NEHALEM_EX:
+ case MODEL_WESTMERE_ARRANDALE:
+ case MODEL_WESTMERE_GULFTOWN:
+ case MODEL_WESTMERE_EX:
+ bclk = BCLK_133_DEFAULT;
+ break;
+ case MODEL_SANDYBRIDGE_SANDY:
+ case MODEL_SANDYBRIDGE_E:
+ case MODEL_IVYBRIDGE_IVY:
+ case MODEL_HASWELL_HASWELL:
+ bclk = BCLK_100_DEFAULT;
+ break;
+ default:
+ bclk = 0;
+ break;
+ }
+
+ if (bclk) {
+ max_nonturbo_ratio = max_effi_ratio
+ = (u8)(vcpu->arch.virtual_tsc_khz / bclk);
+ platform_info = (max_effi_ratio << 40)
+ | (max_nonturbo_ratio << 8);
+ }
+
+ return platform_info;
+}
+
static void drop_user_return_notifiers(void *ignore)
{
unsigned int cpu = smp_processor_id();
@@ -1974,6 +2017,8 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
case MSR_IA32_MISC_ENABLE:
vcpu->arch.ia32_misc_enable_msr = data;
break;
+ case MSR_IA32_PLATFORM_INFO:
+ return 1;
case MSR_KVM_WALL_CLOCK_NEW:
case MSR_KVM_WALL_CLOCK:
vcpu->kvm->arch.wall_clock = data;
@@ -2339,6 +2384,9 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata)
case MSR_IA32_MISC_ENABLE:
data = vcpu->arch.ia32_misc_enable_msr;
break;
+ case MSR_IA32_PLATFORM_INFO:
+ data = kvm_get_platform_info(vcpu);
+ break;
case MSR_IA32_PERF_STATUS:
/* TSC increment by tick */
data = 1000ULL;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] kvm: make vendor_intel a generic function
2013-06-04 16:02 ` [PATCH v2 1/2] kvm: make vendor_intel a generic function Bandan Das
@ 2013-06-04 23:33 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2013-06-04 23:33 UTC (permalink / raw)
To: Bandan Das; +Cc: kvm, Gleb Natapov, Marcelo Tosatti
Il 04/06/2013 18:02, Bandan Das ha scritto:
> Make vendor_intel generic so that functions in x86.c
> can use it.
>
> v2:
> Change vendor_intel function signature because the emulator
> shouldn't be dealing with struct vcpu
>
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
> arch/x86/include/asm/kvm_emulate.h | 13 -------------
> arch/x86/include/asm/kvm_host.h | 20 ++++++++++++++++++++
> arch/x86/kvm/emulate.c | 16 ++++------------
> 3 files changed, 24 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> index 15f960c..611a55f 100644
> --- a/arch/x86/include/asm/kvm_emulate.h
> +++ b/arch/x86/include/asm/kvm_emulate.h
> @@ -319,19 +319,6 @@ struct x86_emulate_ctxt {
> #define REPE_PREFIX 0xf3
> #define REPNE_PREFIX 0xf2
>
> -/* CPUID vendors */
> -#define X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx 0x68747541
> -#define X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx 0x444d4163
> -#define X86EMUL_CPUID_VENDOR_AuthenticAMD_edx 0x69746e65
> -
> -#define X86EMUL_CPUID_VENDOR_AMDisbetterI_ebx 0x69444d41
> -#define X86EMUL_CPUID_VENDOR_AMDisbetterI_ecx 0x21726574
> -#define X86EMUL_CPUID_VENDOR_AMDisbetterI_edx 0x74656273
> -
> -#define X86EMUL_CPUID_VENDOR_GenuineIntel_ebx 0x756e6547
> -#define X86EMUL_CPUID_VENDOR_GenuineIntel_ecx 0x6c65746e
> -#define X86EMUL_CPUID_VENDOR_GenuineIntel_edx 0x49656e69
> -
> enum x86_intercept_stage {
> X86_ICTP_NONE = 0, /* Allow zero-init to not match anything */
> X86_ICPT_PRE_EXCEPT,
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 3741c65..ce9a44f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -144,6 +144,19 @@ enum {
>
> #include <asm/kvm_emulate.h>
>
> +/* CPUID vendors */
> +#define X86EMUL_CPUID_VENDOR_AuthenticAMD_ebx 0x68747541
> +#define X86EMUL_CPUID_VENDOR_AuthenticAMD_ecx 0x444d4163
> +#define X86EMUL_CPUID_VENDOR_AuthenticAMD_edx 0x69746e65
> +
> +#define X86EMUL_CPUID_VENDOR_AMDisbetterI_ebx 0x69444d41
> +#define X86EMUL_CPUID_VENDOR_AMDisbetterI_ecx 0x21726574
> +#define X86EMUL_CPUID_VENDOR_AMDisbetterI_edx 0x74656273
> +
> +#define X86EMUL_CPUID_VENDOR_GenuineIntel_ebx 0x756e6547
> +#define X86EMUL_CPUID_VENDOR_GenuineIntel_ecx 0x6c65746e
> +#define X86EMUL_CPUID_VENDOR_GenuineIntel_edx 0x49656e69
> +
> #define KVM_NR_MEM_OBJS 40
>
> #define KVM_NR_DB_REGS 4
> @@ -942,6 +955,13 @@ static inline unsigned long read_msr(unsigned long msr)
> }
> #endif
>
> +static inline bool vendor_intel(u32 ebx, u32 ecx, u32 edx)
> +{
> + return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx
> + && ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx
> + && edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx;
> +}
> +
> static inline u32 get_rdx_init_val(void)
> {
> return 0x600; /* P6 family */
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 8db0010..87f12fc 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2280,17 +2280,6 @@ setup_syscalls_segments(struct x86_emulate_ctxt *ctxt,
> ss->avl = 0;
> }
>
> -static bool vendor_intel(struct x86_emulate_ctxt *ctxt)
> -{
> - u32 eax, ebx, ecx, edx;
> -
> - eax = ecx = 0;
> - ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> - return ebx == X86EMUL_CPUID_VENDOR_GenuineIntel_ebx
> - && ecx == X86EMUL_CPUID_VENDOR_GenuineIntel_ecx
> - && edx == X86EMUL_CPUID_VENDOR_GenuineIntel_edx;
> -}
> -
> static bool em_syscall_is_enabled(struct x86_emulate_ctxt *ctxt)
> {
> const struct x86_emulate_ops *ops = ctxt->ops;
> @@ -2400,6 +2389,7 @@ static int em_sysenter(struct x86_emulate_ctxt *ctxt)
> u64 msr_data;
> u16 cs_sel, ss_sel;
> u64 efer = 0;
> + u32 eax, ebx, ecx, edx;
>
> ops->get_msr(ctxt, MSR_EFER, &efer);
> /* inject #GP if in real mode */
> @@ -2410,8 +2400,10 @@ static int em_sysenter(struct x86_emulate_ctxt *ctxt)
> * Not recognized on AMD in compat mode (but is recognized in legacy
> * mode).
> */
> + eax = ecx = 0;
> + ctxt->ops->get_cpuid(ctxt, &eax, &ebx, &ecx, &edx);
> if ((ctxt->mode == X86EMUL_MODE_PROT32) && (efer & EFER_LMA)
> - && !vendor_intel(ctxt))
> + && !vendor_intel(ebx, ecx, edx))
> return emulate_ud(ctxt);
>
> /* XXX sysenter/sysexit have not been tested in 64bit mode.
>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
2013-06-04 16:02 [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO Bandan Das
2013-06-04 16:02 ` [PATCH v2 1/2] kvm: make vendor_intel a generic function Bandan Das
2013-06-04 16:02 ` [PATCH v2 2/2] kvm: x86: emulate MSR_PLATFORM_INFO Bandan Das
@ 2013-06-05 8:42 ` Gleb Natapov
2013-06-18 14:05 ` Paolo Bonzini
2 siblings, 1 reply; 17+ messages in thread
From: Gleb Natapov @ 2013-06-05 8:42 UTC (permalink / raw)
To: Bandan Das; +Cc: kvm, Paolo Bonzini, Marcelo Tosatti, Nakajima, Jun
On Tue, Jun 04, 2013 at 12:02:16PM -0400, Bandan Das wrote:
> These patches add an emulated MSR_PLATFORM_INFO that kvm guests
> can read as described in section 14.3.2.4 of the Intel SDM.
> The relevant changes and details are in [2/2]; [1/2] makes vendor_intel
> generic. There are atleat two known applications that fail to run because
> of this MSR missing - Sandra and vTune.
So I really want Intel opinion on this. Right now it is impossible to
implement the MSR correctly in the face of migration (may be with tsc
scaling it will be possible) and while it is unimplemented if application
tries to use it it fails, but if we will implement it application will
just produce incorrect result without any means for user to detect it.
>
> v2: Addressed suggested changes
>
> Bandan Das (2):
> kvm: make vendor_intel a generic function
> kvm: x86: emulate MSR_PLATFORM_INFO
>
> arch/x86/include/asm/kvm_emulate.h | 13 ----------
> arch/x86/include/asm/kvm_host.h | 20 +++++++++++++++
> arch/x86/include/uapi/asm/msr-index.h | 2 ++
> arch/x86/kvm/cpuid.c | 19 ++++++++++++++
> arch/x86/kvm/cpuid.h | 16 ++++++++++++
> arch/x86/kvm/emulate.c | 16 +++---------
> arch/x86/kvm/x86.c | 48 +++++++++++++++++++++++++++++++++++
> 7 files changed, 109 insertions(+), 25 deletions(-)
>
> --
> 1.8.1.4
--
Gleb.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
2013-06-05 8:42 ` [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO Gleb Natapov
@ 2013-06-18 14:05 ` Paolo Bonzini
2013-06-18 15:16 ` Gleb Natapov
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2013-06-18 14:05 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Bandan Das, kvm, Marcelo Tosatti, Nakajima, Jun
Il 05/06/2013 10:42, Gleb Natapov ha scritto:
>> > These patches add an emulated MSR_PLATFORM_INFO that kvm guests
>> > can read as described in section 14.3.2.4 of the Intel SDM.
>> > The relevant changes and details are in [2/2]; [1/2] makes vendor_intel
>> > generic. There are atleat two known applications that fail to run because
>> > of this MSR missing - Sandra and vTune.
> So I really want Intel opinion on this. Right now it is impossible to
> implement the MSR correctly in the face of migration (may be with tsc
> scaling it will be possible) and while it is unimplemented if application
> tries to use it it fails, but if we will implement it application will
> just produce incorrect result without any means for user to detect it.
Jun, ping? (Perhaps Gleb you want to ask a more specific question though).
I don't think this is much different from any other RDTSC usage in
applications (they will typically do their calibration manually, and do
it just once). I'm applying it to queue.
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] kvm: x86: emulate MSR_PLATFORM_INFO
2013-06-04 16:02 ` [PATCH v2 2/2] kvm: x86: emulate MSR_PLATFORM_INFO Bandan Das
@ 2013-06-18 14:07 ` Paolo Bonzini
2013-06-18 15:11 ` Bandan Das
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2013-06-18 14:07 UTC (permalink / raw)
To: Bandan Das; +Cc: kvm, Gleb Natapov, Marcelo Tosatti
Il 04/06/2013 18:02, Bandan Das ha scritto:
> +static u64 kvm_get_platform_info(struct kvm_vcpu *vcpu)
> +{
> + u8 cpumodel;
> + u32 bclk;
> +
> + /*
> + * Programmable Ratio Limit for Turbo Mode (bit 28): 0
> + * Programmable TDC-TDP Limit for Turbo Mode (bit 29): 0
> + */
> + u64 platform_info = 0, max_nonturbo_ratio = 0, max_effi_ratio = 0;
> +
> + cpumodel = kvm_cpuid_get_intel_model(vcpu);
> +
> + switch (cpumodel) {
> + case MODEL_NEHALEM_CLARKSFIELD:
> + case MODEL_NEHALEM_BLOOMFIELD:
> + case MODEL_NEHALEM_EX:
> + case MODEL_WESTMERE_ARRANDALE:
> + case MODEL_WESTMERE_GULFTOWN:
> + case MODEL_WESTMERE_EX:
> + bclk = BCLK_133_DEFAULT;
Just one change: please rename this to base_clock_khz and remove the
BCLK_*_DEFAULT constants please.
Paolo
> + break;
> + case MODEL_SANDYBRIDGE_SANDY:
> + case MODEL_SANDYBRIDGE_E:
> + case MODEL_IVYBRIDGE_IVY:
> + case MODEL_HASWELL_HASWELL:
> + bclk = BCLK_100_DEFAULT;
> + break;
> + default:
> + bclk = 0;
> + break;
> + }
> +
> + if (bclk) {
> + max_nonturbo_ratio = max_effi_ratio
> + = (u8)(vcpu->arch.virtual_tsc_khz / bclk);
> + platform_info = (max_effi_ratio << 40)
> + | (max_nonturbo_ratio << 8);
> + }
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] kvm: x86: emulate MSR_PLATFORM_INFO
2013-06-18 14:07 ` Paolo Bonzini
@ 2013-06-18 15:11 ` Bandan Das
0 siblings, 0 replies; 17+ messages in thread
From: Bandan Das @ 2013-06-18 15:11 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kvm, Gleb Natapov, Marcelo Tosatti
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 04/06/2013 18:02, Bandan Das ha scritto:
>> +static u64 kvm_get_platform_info(struct kvm_vcpu *vcpu)
>> +{
>> + u8 cpumodel;
>> + u32 bclk;
>> +
>> + /*
>> + * Programmable Ratio Limit for Turbo Mode (bit 28): 0
>> + * Programmable TDC-TDP Limit for Turbo Mode (bit 29): 0
>> + */
>> + u64 platform_info = 0, max_nonturbo_ratio = 0, max_effi_ratio = 0;
>> +
>> + cpumodel = kvm_cpuid_get_intel_model(vcpu);
>> +
>> + switch (cpumodel) {
>> + case MODEL_NEHALEM_CLARKSFIELD:
>> + case MODEL_NEHALEM_BLOOMFIELD:
>> + case MODEL_NEHALEM_EX:
>> + case MODEL_WESTMERE_ARRANDALE:
>> + case MODEL_WESTMERE_GULFTOWN:
>> + case MODEL_WESTMERE_EX:
>> + bclk = BCLK_133_DEFAULT;
>
> Just one change: please rename this to base_clock_khz and remove the
> BCLK_*_DEFAULT constants please.
Thanks for the review! I thought the base clock is usually referred to
as bclk in Intel parlance but not sure :)
Anyway, I will send a new one.
> Paolo
>
>> + break;
>> + case MODEL_SANDYBRIDGE_SANDY:
>> + case MODEL_SANDYBRIDGE_E:
>> + case MODEL_IVYBRIDGE_IVY:
>> + case MODEL_HASWELL_HASWELL:
>> + bclk = BCLK_100_DEFAULT;
>> + break;
>> + default:
>> + bclk = 0;
>> + break;
>> + }
>> +
>> + if (bclk) {
>> + max_nonturbo_ratio = max_effi_ratio
>> + = (u8)(vcpu->arch.virtual_tsc_khz / bclk);
>> + platform_info = (max_effi_ratio << 40)
>> + | (max_nonturbo_ratio << 8);
>> + }
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
2013-06-18 14:05 ` Paolo Bonzini
@ 2013-06-18 15:16 ` Gleb Natapov
2013-06-18 15:29 ` Bandan Das
2013-06-18 17:59 ` Nakajima, Jun
0 siblings, 2 replies; 17+ messages in thread
From: Gleb Natapov @ 2013-06-18 15:16 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Bandan Das, kvm, Marcelo Tosatti, Nakajima, Jun
On Tue, Jun 18, 2013 at 04:05:08PM +0200, Paolo Bonzini wrote:
> Il 05/06/2013 10:42, Gleb Natapov ha scritto:
> >> > These patches add an emulated MSR_PLATFORM_INFO that kvm guests
> >> > can read as described in section 14.3.2.4 of the Intel SDM.
> >> > The relevant changes and details are in [2/2]; [1/2] makes vendor_intel
> >> > generic. There are atleat two known applications that fail to run because
> >> > of this MSR missing - Sandra and vTune.
> > So I really want Intel opinion on this. Right now it is impossible to
> > implement the MSR correctly in the face of migration (may be with tsc
> > scaling it will be possible) and while it is unimplemented if application
> > tries to use it it fails, but if we will implement it application will
> > just produce incorrect result without any means for user to detect it.
>
> Jun, ping? (Perhaps Gleb you want to ask a more specific question though).
>
> I don't think this is much different from any other RDTSC usage in
> applications (they will typically do their calibration manually, and do
> it just once). I'm applying it to queue.
>
And we do not support application that uses RDTSC directly! If we could
catch those it would be good from support point of view, so the way
MSR_PLATFORM_INFO behaves now it better then proposed alternative.
--
Gleb.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
2013-06-18 15:16 ` Gleb Natapov
@ 2013-06-18 15:29 ` Bandan Das
2013-06-18 15:40 ` Gleb Natapov
2013-06-18 17:59 ` Nakajima, Jun
1 sibling, 1 reply; 17+ messages in thread
From: Bandan Das @ 2013-06-18 15:29 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Paolo Bonzini, kvm, Marcelo Tosatti, Nakajima, Jun
Gleb Natapov <gleb@redhat.com> writes:
> On Tue, Jun 18, 2013 at 04:05:08PM +0200, Paolo Bonzini wrote:
>> Il 05/06/2013 10:42, Gleb Natapov ha scritto:
>> >> > These patches add an emulated MSR_PLATFORM_INFO that kvm guests
>> >> > can read as described in section 14.3.2.4 of the Intel SDM.
>> >> > The relevant changes and details are in [2/2]; [1/2] makes vendor_intel
>> >> > generic. There are atleat two known applications that fail to run because
>> >> > of this MSR missing - Sandra and vTune.
>> > So I really want Intel opinion on this. Right now it is impossible to
>> > implement the MSR correctly in the face of migration (may be with tsc
>> > scaling it will be possible) and while it is unimplemented if application
>> > tries to use it it fails, but if we will implement it application will
>> > just produce incorrect result without any means for user to detect it.
>>
>> Jun, ping? (Perhaps Gleb you want to ask a more specific question though).
>>
>> I don't think this is much different from any other RDTSC usage in
>> applications (they will typically do their calibration manually, and do
>> it just once). I'm applying it to queue.
>>
> And we do not support application that uses RDTSC directly! If we could
> catch those it would be good from support point of view, so the way
> MSR_PLATFORM_INFO behaves now it better then proposed alternative.
If support is the issue, can't we have a flag that disables this by
default and users who want to take the plunge (and be responsible
for the consequences) can enable it to read platform_info ?
> --
> Gleb.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
2013-06-18 15:29 ` Bandan Das
@ 2013-06-18 15:40 ` Gleb Natapov
2013-06-19 17:50 ` Bandan Das
0 siblings, 1 reply; 17+ messages in thread
From: Gleb Natapov @ 2013-06-18 15:40 UTC (permalink / raw)
To: Bandan Das; +Cc: Paolo Bonzini, kvm, Marcelo Tosatti, Nakajima, Jun
On Tue, Jun 18, 2013 at 11:29:27AM -0400, Bandan Das wrote:
> Gleb Natapov <gleb@redhat.com> writes:
>
> > On Tue, Jun 18, 2013 at 04:05:08PM +0200, Paolo Bonzini wrote:
> >> Il 05/06/2013 10:42, Gleb Natapov ha scritto:
> >> >> > These patches add an emulated MSR_PLATFORM_INFO that kvm guests
> >> >> > can read as described in section 14.3.2.4 of the Intel SDM.
> >> >> > The relevant changes and details are in [2/2]; [1/2] makes vendor_intel
> >> >> > generic. There are atleat two known applications that fail to run because
> >> >> > of this MSR missing - Sandra and vTune.
> >> > So I really want Intel opinion on this. Right now it is impossible to
> >> > implement the MSR correctly in the face of migration (may be with tsc
> >> > scaling it will be possible) and while it is unimplemented if application
> >> > tries to use it it fails, but if we will implement it application will
> >> > just produce incorrect result without any means for user to detect it.
> >>
> >> Jun, ping? (Perhaps Gleb you want to ask a more specific question though).
> >>
> >> I don't think this is much different from any other RDTSC usage in
> >> applications (they will typically do their calibration manually, and do
> >> it just once). I'm applying it to queue.
> >>
> > And we do not support application that uses RDTSC directly! If we could
> > catch those it would be good from support point of view, so the way
> > MSR_PLATFORM_INFO behaves now it better then proposed alternative.
>
> If support is the issue, can't we have a flag that disables this by
> default and users who want to take the plunge (and be responsible
> for the consequences) can enable it to read platform_info ?
>
We have it already :) ignore_msrs. If it is set unimplemented MSRs will
not inject #GP, but return zero value instead. Zero it as incorrect as
anything else in the case of migration.
--
Gleb.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
2013-06-18 15:16 ` Gleb Natapov
2013-06-18 15:29 ` Bandan Das
@ 2013-06-18 17:59 ` Nakajima, Jun
2013-06-19 10:57 ` Gleb Natapov
1 sibling, 1 reply; 17+ messages in thread
From: Nakajima, Jun @ 2013-06-18 17:59 UTC (permalink / raw)
To: Gleb Natapov
Cc: Paolo Bonzini, Bandan Das, kvm@vger.kernel.org, Marcelo Tosatti
On Tue, Jun 18, 2013 at 8:16 AM, Gleb Natapov <gleb@redhat.com> wrote:
> On Tue, Jun 18, 2013 at 04:05:08PM +0200, Paolo Bonzini wrote:
>> Il 05/06/2013 10:42, Gleb Natapov ha scritto:
>> >> > These patches add an emulated MSR_PLATFORM_INFO that kvm guests
>> >> > can read as described in section 14.3.2.4 of the Intel SDM.
>> >> > The relevant changes and details are in [2/2]; [1/2] makes vendor_intel
>> >> > generic. There are atleat two known applications that fail to run because
>> >> > of this MSR missing - Sandra and vTune.
>> > So I really want Intel opinion on this. Right now it is impossible to
>> > implement the MSR correctly in the face of migration (may be with tsc
>> > scaling it will be possible) and while it is unimplemented if application
>> > tries to use it it fails, but if we will implement it application will
>> > just produce incorrect result without any means for user to detect it.
>>
>> Jun, ping? (Perhaps Gleb you want to ask a more specific question though).
>>
>> I don't think this is much different from any other RDTSC usage in
>> applications (they will typically do their calibration manually, and do
>> it just once). I'm applying it to queue.
>>
> And we do not support application that uses RDTSC directly! If we could
> catch those it would be good from support point of view, so the way
> MSR_PLATFORM_INFO behaves now it better then proposed alternative.
Is it reasonable or possible to expose MSR_PLATFORM_INFO more and then
disable migration? Some use cases (like VTune) don't need live
migration.
--
Jun
Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
2013-06-18 17:59 ` Nakajima, Jun
@ 2013-06-19 10:57 ` Gleb Natapov
0 siblings, 0 replies; 17+ messages in thread
From: Gleb Natapov @ 2013-06-19 10:57 UTC (permalink / raw)
To: Nakajima, Jun
Cc: Paolo Bonzini, Bandan Das, kvm@vger.kernel.org, Marcelo Tosatti
On Tue, Jun 18, 2013 at 10:59:37AM -0700, Nakajima, Jun wrote:
> On Tue, Jun 18, 2013 at 8:16 AM, Gleb Natapov <gleb@redhat.com> wrote:
> > On Tue, Jun 18, 2013 at 04:05:08PM +0200, Paolo Bonzini wrote:
> >> Il 05/06/2013 10:42, Gleb Natapov ha scritto:
> >> >> > These patches add an emulated MSR_PLATFORM_INFO that kvm guests
> >> >> > can read as described in section 14.3.2.4 of the Intel SDM.
> >> >> > The relevant changes and details are in [2/2]; [1/2] makes vendor_intel
> >> >> > generic. There are atleat two known applications that fail to run because
> >> >> > of this MSR missing - Sandra and vTune.
> >> > So I really want Intel opinion on this. Right now it is impossible to
> >> > implement the MSR correctly in the face of migration (may be with tsc
> >> > scaling it will be possible) and while it is unimplemented if application
> >> > tries to use it it fails, but if we will implement it application will
> >> > just produce incorrect result without any means for user to detect it.
> >>
> >> Jun, ping? (Perhaps Gleb you want to ask a more specific question though).
> >>
> >> I don't think this is much different from any other RDTSC usage in
> >> applications (they will typically do their calibration manually, and do
> >> it just once). I'm applying it to queue.
> >>
> > And we do not support application that uses RDTSC directly! If we could
> > catch those it would be good from support point of view, so the way
> > MSR_PLATFORM_INFO behaves now it better then proposed alternative.
>
> Is it reasonable or possible to expose MSR_PLATFORM_INFO more and then
> disable migration? Some use cases (like VTune) don't need live
> migration.
>
VTune is just an application and IT wants to run developers machine on a
"cloud" and developers want to use the tool. Also, since MSR_PLATFORM_INFO
is non architectural MSR there is no way to know when it is exposed;
there is no cpuid bit that management sets to "expose" it to a guest. Of
course we can create some other mechanism for that, but it looks like
a lot of ad-hoc work for a little gain.
The simplest thing to do is to return zero for the MSR and do not
#GP. What VTune will do in this case? The value is obviously incorrect,
so my hope is that it will disable the feature that depends on it instead
of blindly use it and crash with divide by zero later on.
--
Gleb.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
2013-06-18 15:40 ` Gleb Natapov
@ 2013-06-19 17:50 ` Bandan Das
2013-06-20 7:31 ` Gleb Natapov
0 siblings, 1 reply; 17+ messages in thread
From: Bandan Das @ 2013-06-19 17:50 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Paolo Bonzini, kvm, Marcelo Tosatti, Nakajima, Jun
Gleb Natapov <gleb@redhat.com> writes:
> On Tue, Jun 18, 2013 at 11:29:27AM -0400, Bandan Das wrote:
>> Gleb Natapov <gleb@redhat.com> writes:
>>
>> > On Tue, Jun 18, 2013 at 04:05:08PM +0200, Paolo Bonzini wrote:
>> >> Il 05/06/2013 10:42, Gleb Natapov ha scritto:
>> >> >> > These patches add an emulated MSR_PLATFORM_INFO that kvm guests
>> >> >> > can read as described in section 14.3.2.4 of the Intel SDM.
>> >> >> > The relevant changes and details are in [2/2]; [1/2] makes vendor_intel
>> >> >> > generic. There are atleat two known applications that fail to run because
>> >> >> > of this MSR missing - Sandra and vTune.
>> >> > So I really want Intel opinion on this. Right now it is impossible to
>> >> > implement the MSR correctly in the face of migration (may be with tsc
>> >> > scaling it will be possible) and while it is unimplemented if application
>> >> > tries to use it it fails, but if we will implement it application will
>> >> > just produce incorrect result without any means for user to detect it.
>> >>
>> >> Jun, ping? (Perhaps Gleb you want to ask a more specific question though).
>> >>
>> >> I don't think this is much different from any other RDTSC usage in
>> >> applications (they will typically do their calibration manually, and do
>> >> it just once). I'm applying it to queue.
>> >>
>> > And we do not support application that uses RDTSC directly! If we could
>> > catch those it would be good from support point of view, so the way
>> > MSR_PLATFORM_INFO behaves now it better then proposed alternative.
>>
>> If support is the issue, can't we have a flag that disables this by
>> default and users who want to take the plunge (and be responsible
>> for the consequences) can enable it to read platform_info ?
>>
> We have it already :) ignore_msrs. If it is set unimplemented MSRs will
> not inject #GP, but return zero value instead. Zero it as incorrect as
> anything else in the case of migration.
But does the intended purpose of ignore_msrs fit here ? Even if we return
0 after a migration, there's no guarantee that the application will
give correct results based on the old value of the MSR it read.
I agree with you on the potential problems but I think we are completely
ignoring the "non-migration" use case. These users will probably benefit
from a correct value of (virtual) msr_platform_info. And it appears, the
easiest way to give both options to the user is using a new module_param,
say ignore_platform_info.
Scenarios :
1. ignore_platform_info = 0 (Default). Inject #GP if application tries to
read this MSR.
2. ignore_platform_info = 1. User wants to read the calculated value, her
environment doesn't require migration.
3. ignore_msrs = 1. If this is set, we always return 0 and application will
hopefully resort to a workaround.
Bandan
> --
> Gleb.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
2013-06-19 17:50 ` Bandan Das
@ 2013-06-20 7:31 ` Gleb Natapov
2013-06-20 8:34 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Gleb Natapov @ 2013-06-20 7:31 UTC (permalink / raw)
To: Bandan Das; +Cc: Paolo Bonzini, kvm, Marcelo Tosatti, Nakajima, Jun
On Wed, Jun 19, 2013 at 01:50:45PM -0400, Bandan Das wrote:
> Gleb Natapov <gleb@redhat.com> writes:
>
> > On Tue, Jun 18, 2013 at 11:29:27AM -0400, Bandan Das wrote:
> >> Gleb Natapov <gleb@redhat.com> writes:
> >>
> >> > On Tue, Jun 18, 2013 at 04:05:08PM +0200, Paolo Bonzini wrote:
> >> >> Il 05/06/2013 10:42, Gleb Natapov ha scritto:
> >> >> >> > These patches add an emulated MSR_PLATFORM_INFO that kvm guests
> >> >> >> > can read as described in section 14.3.2.4 of the Intel SDM.
> >> >> >> > The relevant changes and details are in [2/2]; [1/2] makes vendor_intel
> >> >> >> > generic. There are atleat two known applications that fail to run because
> >> >> >> > of this MSR missing - Sandra and vTune.
> >> >> > So I really want Intel opinion on this. Right now it is impossible to
> >> >> > implement the MSR correctly in the face of migration (may be with tsc
> >> >> > scaling it will be possible) and while it is unimplemented if application
> >> >> > tries to use it it fails, but if we will implement it application will
> >> >> > just produce incorrect result without any means for user to detect it.
> >> >>
> >> >> Jun, ping? (Perhaps Gleb you want to ask a more specific question though).
> >> >>
> >> >> I don't think this is much different from any other RDTSC usage in
> >> >> applications (they will typically do their calibration manually, and do
> >> >> it just once). I'm applying it to queue.
> >> >>
> >> > And we do not support application that uses RDTSC directly! If we could
> >> > catch those it would be good from support point of view, so the way
> >> > MSR_PLATFORM_INFO behaves now it better then proposed alternative.
> >>
> >> If support is the issue, can't we have a flag that disables this by
> >> default and users who want to take the plunge (and be responsible
> >> for the consequences) can enable it to read platform_info ?
> >>
> > We have it already :) ignore_msrs. If it is set unimplemented MSRs will
> > not inject #GP, but return zero value instead. Zero it as incorrect as
> > anything else in the case of migration.
>
> But does the intended purpose of ignore_msrs fit here ? Even if we return
> 0 after a migration, there's no guarantee that the application will
> give correct results based on the old value of the MSR it read.
>
I am not sure I understand. I am saying that since we cannot have
correct implementation of the MSR, any implementation that does not
crash a guest is as good as any other.
> I agree with you on the potential problems but I think we are completely
> ignoring the "non-migration" use case. These users will probably benefit
> from a correct value of (virtual) msr_platform_info. And it appears, the
> easiest way to give both options to the user is using a new module_param,
> say ignore_platform_info.
>
Migration is important part of virtualization. PMU emulation currently
is in demo status because migration is not implemented yet, but at least
it is possible to implement it.
> Scenarios :
> 1. ignore_platform_info = 0 (Default). Inject #GP if application tries to
> read this MSR.
> 2. ignore_platform_info = 1. User wants to read the calculated value, her
> environment doesn't require migration.
> 3. ignore_msrs = 1. If this is set, we always return 0 and application will
> hopefully resort to a workaround.
>
Module flag is global for all VMs on a host. Implementing it like this
will guaranty that the feature will not be used in production ever.
ignore_msrs exists only for developers to quickly check if a problem goes
away if some MSR does not #GP, never as a real solution.
To make it somewhat useful the flag should be per-vm and exposed to all
layers up to a management. Management is the one who enables it per VM
basis and guaranties that VM with the feature enabled is never live
migrated. Frankly IMO it will be another management knob that will never
be set.
--
Gleb.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
2013-06-20 7:31 ` Gleb Natapov
@ 2013-06-20 8:34 ` Paolo Bonzini
2013-06-20 8:57 ` Gleb Natapov
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2013-06-20 8:34 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Bandan Das, kvm, Marcelo Tosatti, Nakajima, Jun
Il 20/06/2013 09:31, Gleb Natapov ha scritto:
>> I agree with you on the potential problems but I think we are completely
>> ignoring the "non-migration" use case. These users will probably benefit
>> from a correct value of (virtual) msr_platform_info. And it appears, the
>> easiest way to give both options to the user is using a new module_param,
>> say ignore_platform_info.
>>
> Migration is important part of virtualization. PMU emulation currently
> is in demo status because migration is not implemented yet, but at least
> it is possible to implement it.
But there's a lot more to do for migration than just moving the MSR
contents from one machine to another. We need to support customizing
the values of CPUID leaf 0ah, so that PMU migration can work if you
specify "lowest common denominator" CPU models.
I haven't looked at it yet because QEMU support for this is not in sight.
>> Scenarios :
>> 1. ignore_platform_info = 0 (Default). Inject #GP if application tries to
>> read this MSR.
>> 2. ignore_platform_info = 1. User wants to read the calculated value, her
>> environment doesn't require migration.
>> 3. ignore_msrs = 1. If this is set, we always return 0 and application will
>> hopefully resort to a workaround.
>>
> Module flag is global for all VMs on a host. Implementing it like this
> will guaranty that the feature will not be used in production ever.
> ignore_msrs exists only for developers to quickly check if a problem goes
> away if some MSR does not #GP, never as a real solution.
>
> To make it somewhat useful the flag should be per-vm and exposed to all
> layers up to a management. Management is the one who enables it per VM
> basis and guaranties that VM with the feature enabled is never live
> migrated. Frankly IMO it will be another management knob that will never
> be set.
I agree.
I think it's fine to apply Bandan's patches. It's just one more thing
to care about when migrating machines that use the PMU. And I hope that
Intel will add TSC scaling sooner or later, which will fix the issue
automatically. Hear, Jun! :)
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO
2013-06-20 8:34 ` Paolo Bonzini
@ 2013-06-20 8:57 ` Gleb Natapov
0 siblings, 0 replies; 17+ messages in thread
From: Gleb Natapov @ 2013-06-20 8:57 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Bandan Das, kvm, Marcelo Tosatti, Nakajima, Jun
On Thu, Jun 20, 2013 at 10:34:39AM +0200, Paolo Bonzini wrote:
> Il 20/06/2013 09:31, Gleb Natapov ha scritto:
> >> I agree with you on the potential problems but I think we are completely
> >> ignoring the "non-migration" use case. These users will probably benefit
> >> from a correct value of (virtual) msr_platform_info. And it appears, the
> >> easiest way to give both options to the user is using a new module_param,
> >> say ignore_platform_info.
> >>
> > Migration is important part of virtualization. PMU emulation currently
> > is in demo status because migration is not implemented yet, but at least
> > it is possible to implement it.
>
> But there's a lot more to do for migration than just moving the MSR
> contents from one machine to another. We need to support customizing
> the values of CPUID leaf 0ah, so that PMU migration can work if you
> specify "lowest common denominator" CPU models.
>
> I haven't looked at it yet because QEMU support for this is not in sight.
>
Eduardo working on it.
> >> Scenarios :
> >> 1. ignore_platform_info = 0 (Default). Inject #GP if application tries to
> >> read this MSR.
> >> 2. ignore_platform_info = 1. User wants to read the calculated value, her
> >> environment doesn't require migration.
> >> 3. ignore_msrs = 1. If this is set, we always return 0 and application will
> >> hopefully resort to a workaround.
> >>
> > Module flag is global for all VMs on a host. Implementing it like this
> > will guaranty that the feature will not be used in production ever.
> > ignore_msrs exists only for developers to quickly check if a problem goes
> > away if some MSR does not #GP, never as a real solution.
> >
> > To make it somewhat useful the flag should be per-vm and exposed to all
> > layers up to a management. Management is the one who enables it per VM
> > basis and guaranties that VM with the feature enabled is never live
> > migrated. Frankly IMO it will be another management knob that will never
> > be set.
>
> I agree.
>
> I think it's fine to apply Bandan's patches. It's just one more thing
> to care about when migrating machines that use the PMU. And I hope that
> Intel will add TSC scaling sooner or later, which will fix the issue
> automatically. Hear, Jun! :)
>
I am find with adding things without migration support if we know how
migration can be fixed, in this case we are adding something that cannot
be fixed without HW support and, as such, should be enabled only on
hypothetical future HW that will support required feature.
So enabling it by default is wrong, and I listed pros and cons of global
ignore_platform_info option and per vm enable option above.
--
Gleb.
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-06-20 8:57 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-04 16:02 [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO Bandan Das
2013-06-04 16:02 ` [PATCH v2 1/2] kvm: make vendor_intel a generic function Bandan Das
2013-06-04 23:33 ` Paolo Bonzini
2013-06-04 16:02 ` [PATCH v2 2/2] kvm: x86: emulate MSR_PLATFORM_INFO Bandan Das
2013-06-18 14:07 ` Paolo Bonzini
2013-06-18 15:11 ` Bandan Das
2013-06-05 8:42 ` [PATCH v2 0/2] kvm: x86: Emulate MSR_PLATFORM_INFO Gleb Natapov
2013-06-18 14:05 ` Paolo Bonzini
2013-06-18 15:16 ` Gleb Natapov
2013-06-18 15:29 ` Bandan Das
2013-06-18 15:40 ` Gleb Natapov
2013-06-19 17:50 ` Bandan Das
2013-06-20 7:31 ` Gleb Natapov
2013-06-20 8:34 ` Paolo Bonzini
2013-06-20 8:57 ` Gleb Natapov
2013-06-18 17:59 ` Nakajima, Jun
2013-06-19 10:57 ` Gleb Natapov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).