* [PATCH v4 1/2] i386: Add Intel Processor Trace feature support
@ 2018-03-04 16:48 ` Luwei Kang
0 siblings, 0 replies; 30+ messages in thread
From: Luwei Kang @ 2018-03-04 16:48 UTC (permalink / raw)
To: qemu-devel, kvm; +Cc: Luwei Kang, ehabkost, mtosatti, Chao Peng, pbonzini, rth
From: Chao Peng <chao.p.peng@linux.intel.com>
Expose Intel Processor Trace feature to guest.
To make Intel PT live migration safe and get same CPUID information
with same CPU model on diffrent host. CPUID[14] is constant in this
patch. Intel PT use EPT is first supported in IceLake, the CPUID[14]
get on this machine as default value. Intel PT would be disabled
if any machine don't support this minial feature list.
Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
>From V3:
- fix some typo;
- add some comments and safty check.
---
target/i386/cpu.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
target/i386/cpu.h | 1 +
target/i386/kvm.c | 23 ++++++++++++++++
3 files changed, 100 insertions(+), 2 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b5e431e..24e1693 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -173,7 +173,32 @@
#define L2_ITLB_4K_ASSOC 4
#define L2_ITLB_4K_ENTRIES 512
-
+/* CPUID Leaf 0x14 constants: */
+#define INTEL_PT_MAX_SUBLEAF 0x1
+/*
+ * bit[00]: IA32_RTIT_CTL.CR3 filter can be set to 1 and IA32_RTIT_CR3_MATCH
+ * MSR can be accessed;
+ * bit[01]: Support Configurable PSB and Cycle-Accurate Mode;
+ * bit[02]: Support IP Filtering, TraceStop filtering, and preservation
+ * of Intel PT MSRs across warm reset;
+ * bit[03]: Support MTC timing packet and suppression of COFI-based packets;
+ */
+#define INTEL_PT_MINIMAL_EBX 0xf
+/*
+ * bit[00]: Tracing can be enabled with IA32_RTIT_CTL.ToPA = 1 and
+ * IA32_RTIT_OUTPUT_BASE and IA32_RTIT_OUTPUT_MASK_PTRS MSRs can be
+ * accessed;
+ * bit[01]: ToPA tables can hold any number of output entries, up to the
+ * maximum allowed by the MaskOrTableOffset field of
+ * IA32_RTIT_OUTPUT_MASK_PTRS;
+ * bit[02]: Support Single-Range Output scheme;
+ */
+#define INTEL_PT_MINIMAL_ECX 0x7
+#define INTEL_PT_ADDR_RANGES_NUM 0x2 /* Number of configurable address ranges */
+#define INTEL_PT_ADDR_RANGES_NUM_MASK 0x3
+#define INTEL_PT_MTC_BITMAP (0x0249 << 16) /* Support ART(0,3,6,9) */
+#define INTEL_PT_CYCLE_BITMAP 0x1fff /* Support 0,2^(0~11) */
+#define INTEL_PT_PSB_BITMAP (0x003f << 16) /* Support 2K,4K,8K,16K,32K,64K */
static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
uint32_t vendor2, uint32_t vendor3)
@@ -428,7 +453,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
NULL, NULL, "mpx", NULL,
"avx512f", "avx512dq", "rdseed", "adx",
"smap", "avx512ifma", "pcommit", "clflushopt",
- "clwb", NULL, "avx512pf", "avx512er",
+ "clwb", "intel-pt", "avx512pf", "avx512er",
"avx512cd", "sha-ni", "avx512bw", "avx512vl",
},
.cpuid_eax = 7,
@@ -3453,6 +3478,27 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
}
break;
}
+ case 0x14: {
+ /* Intel Processor Trace Enumeration */
+ *eax = 0;
+ *ebx = 0;
+ *ecx = 0;
+ *edx = 0;
+ if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) ||
+ !kvm_enabled()) {
+ break;
+ }
+
+ if (count == 0) {
+ *eax = INTEL_PT_MAX_SUBLEAF;
+ *ebx = INTEL_PT_MINIMAL_EBX;
+ *ecx = INTEL_PT_MINIMAL_ECX;
+ } else if (count == 1) {
+ *eax = INTEL_PT_MTC_BITMAP | INTEL_PT_ADDR_RANGES_NUM;
+ *ebx = INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP;
+ }
+ break;
+ }
case 0x40000000:
/*
* CPUID code in kvm_arch_init_vcpu() ignores stuff
@@ -4083,6 +4129,34 @@ static int x86_cpu_filter_features(X86CPU *cpu)
}
}
+ if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
+ kvm_enabled()) {
+ KVMState *s = CPU(cpu)->kvm_state;
+ uint32_t eax_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EAX);
+ uint32_t ebx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EBX);
+ uint32_t ecx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_ECX);
+ uint32_t eax_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EAX);
+ uint32_t ebx_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EBX);
+
+ if (!eax_0 ||
+ ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
+ ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
+ ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
+ ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
+ INTEL_PT_ADDR_RANGES_NUM) ||
+ ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
+ (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) {
+ /*
+ * Processor Trace capabilities aren't configurable, so if the
+ * host can't emulate the capabilities we report on
+ * cpu_x86_cpuid(), intel-pt can't be enabled on the current host.
+ */
+ env->features[FEAT_7_0_EBX] &= ~CPUID_7_0_EBX_INTEL_PT;
+ cpu->filtered_features[FEAT_7_0_EBX] |= CPUID_7_0_EBX_INTEL_PT;
+ rv = 1;
+ }
+ }
+
return rv;
}
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index faf39ec..f5b9ecb 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -640,6 +640,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
#define CPUID_7_0_EBX_PCOMMIT (1U << 22) /* Persistent Commit */
#define CPUID_7_0_EBX_CLFLUSHOPT (1U << 23) /* Flush a Cache Line Optimized */
#define CPUID_7_0_EBX_CLWB (1U << 24) /* Cache Line Write Back */
+#define CPUID_7_0_EBX_INTEL_PT (1U << 25) /* Intel Processor Trace */
#define CPUID_7_0_EBX_AVX512PF (1U << 26) /* AVX-512 Prefetch */
#define CPUID_7_0_EBX_AVX512ER (1U << 27) /* AVX-512 Exponential and Reciprocal */
#define CPUID_7_0_EBX_AVX512CD (1U << 28) /* AVX-512 Conflict Detection */
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index ad4b159..f9f4cd1 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -865,6 +865,29 @@ int kvm_arch_init_vcpu(CPUState *cs)
c = &cpuid_data.entries[cpuid_i++];
}
break;
+ case 0x14: {
+ uint32_t times;
+
+ c->function = i;
+ c->index = 0;
+ c->flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+ cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
+ times = c->eax;
+
+ for (j = 1; j <= times; ++j) {
+ if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
+ fprintf(stderr, "cpuid_data is full, no space for "
+ "cpuid(eax:0x14,ecx:0x%x)\n", j);
+ abort();
+ }
+ c = &cpuid_data.entries[cpuid_i++];
+ c->function = i;
+ c->index = j;
+ c->flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+ cpu_x86_cpuid(env, i, j, &c->eax, &c->ebx, &c->ecx, &c->edx);
+ }
+ break;
+ }
default:
c->function = i;
c->flags = 0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v4 1/2] i386: Add Intel Processor Trace feature support
@ 2018-03-04 16:48 ` Luwei Kang
0 siblings, 0 replies; 30+ messages in thread
From: Luwei Kang @ 2018-03-04 16:48 UTC (permalink / raw)
To: qemu-devel, kvm; +Cc: pbonzini, rth, ehabkost, mtosatti, Chao Peng, Luwei Kang
From: Chao Peng <chao.p.peng@linux.intel.com>
Expose Intel Processor Trace feature to guest.
To make Intel PT live migration safe and get same CPUID information
with same CPU model on diffrent host. CPUID[14] is constant in this
patch. Intel PT use EPT is first supported in IceLake, the CPUID[14]
get on this machine as default value. Intel PT would be disabled
if any machine don't support this minial feature list.
Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
>From V3:
- fix some typo;
- add some comments and safty check.
---
target/i386/cpu.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
target/i386/cpu.h | 1 +
target/i386/kvm.c | 23 ++++++++++++++++
3 files changed, 100 insertions(+), 2 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b5e431e..24e1693 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -173,7 +173,32 @@
#define L2_ITLB_4K_ASSOC 4
#define L2_ITLB_4K_ENTRIES 512
-
+/* CPUID Leaf 0x14 constants: */
+#define INTEL_PT_MAX_SUBLEAF 0x1
+/*
+ * bit[00]: IA32_RTIT_CTL.CR3 filter can be set to 1 and IA32_RTIT_CR3_MATCH
+ * MSR can be accessed;
+ * bit[01]: Support Configurable PSB and Cycle-Accurate Mode;
+ * bit[02]: Support IP Filtering, TraceStop filtering, and preservation
+ * of Intel PT MSRs across warm reset;
+ * bit[03]: Support MTC timing packet and suppression of COFI-based packets;
+ */
+#define INTEL_PT_MINIMAL_EBX 0xf
+/*
+ * bit[00]: Tracing can be enabled with IA32_RTIT_CTL.ToPA = 1 and
+ * IA32_RTIT_OUTPUT_BASE and IA32_RTIT_OUTPUT_MASK_PTRS MSRs can be
+ * accessed;
+ * bit[01]: ToPA tables can hold any number of output entries, up to the
+ * maximum allowed by the MaskOrTableOffset field of
+ * IA32_RTIT_OUTPUT_MASK_PTRS;
+ * bit[02]: Support Single-Range Output scheme;
+ */
+#define INTEL_PT_MINIMAL_ECX 0x7
+#define INTEL_PT_ADDR_RANGES_NUM 0x2 /* Number of configurable address ranges */
+#define INTEL_PT_ADDR_RANGES_NUM_MASK 0x3
+#define INTEL_PT_MTC_BITMAP (0x0249 << 16) /* Support ART(0,3,6,9) */
+#define INTEL_PT_CYCLE_BITMAP 0x1fff /* Support 0,2^(0~11) */
+#define INTEL_PT_PSB_BITMAP (0x003f << 16) /* Support 2K,4K,8K,16K,32K,64K */
static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
uint32_t vendor2, uint32_t vendor3)
@@ -428,7 +453,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
NULL, NULL, "mpx", NULL,
"avx512f", "avx512dq", "rdseed", "adx",
"smap", "avx512ifma", "pcommit", "clflushopt",
- "clwb", NULL, "avx512pf", "avx512er",
+ "clwb", "intel-pt", "avx512pf", "avx512er",
"avx512cd", "sha-ni", "avx512bw", "avx512vl",
},
.cpuid_eax = 7,
@@ -3453,6 +3478,27 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
}
break;
}
+ case 0x14: {
+ /* Intel Processor Trace Enumeration */
+ *eax = 0;
+ *ebx = 0;
+ *ecx = 0;
+ *edx = 0;
+ if (!(env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) ||
+ !kvm_enabled()) {
+ break;
+ }
+
+ if (count == 0) {
+ *eax = INTEL_PT_MAX_SUBLEAF;
+ *ebx = INTEL_PT_MINIMAL_EBX;
+ *ecx = INTEL_PT_MINIMAL_ECX;
+ } else if (count == 1) {
+ *eax = INTEL_PT_MTC_BITMAP | INTEL_PT_ADDR_RANGES_NUM;
+ *ebx = INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP;
+ }
+ break;
+ }
case 0x40000000:
/*
* CPUID code in kvm_arch_init_vcpu() ignores stuff
@@ -4083,6 +4129,34 @@ static int x86_cpu_filter_features(X86CPU *cpu)
}
}
+ if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
+ kvm_enabled()) {
+ KVMState *s = CPU(cpu)->kvm_state;
+ uint32_t eax_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EAX);
+ uint32_t ebx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EBX);
+ uint32_t ecx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_ECX);
+ uint32_t eax_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EAX);
+ uint32_t ebx_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EBX);
+
+ if (!eax_0 ||
+ ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
+ ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
+ ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
+ ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
+ INTEL_PT_ADDR_RANGES_NUM) ||
+ ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
+ (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) {
+ /*
+ * Processor Trace capabilities aren't configurable, so if the
+ * host can't emulate the capabilities we report on
+ * cpu_x86_cpuid(), intel-pt can't be enabled on the current host.
+ */
+ env->features[FEAT_7_0_EBX] &= ~CPUID_7_0_EBX_INTEL_PT;
+ cpu->filtered_features[FEAT_7_0_EBX] |= CPUID_7_0_EBX_INTEL_PT;
+ rv = 1;
+ }
+ }
+
return rv;
}
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index faf39ec..f5b9ecb 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -640,6 +640,7 @@ typedef uint32_t FeatureWordArray[FEATURE_WORDS];
#define CPUID_7_0_EBX_PCOMMIT (1U << 22) /* Persistent Commit */
#define CPUID_7_0_EBX_CLFLUSHOPT (1U << 23) /* Flush a Cache Line Optimized */
#define CPUID_7_0_EBX_CLWB (1U << 24) /* Cache Line Write Back */
+#define CPUID_7_0_EBX_INTEL_PT (1U << 25) /* Intel Processor Trace */
#define CPUID_7_0_EBX_AVX512PF (1U << 26) /* AVX-512 Prefetch */
#define CPUID_7_0_EBX_AVX512ER (1U << 27) /* AVX-512 Exponential and Reciprocal */
#define CPUID_7_0_EBX_AVX512CD (1U << 28) /* AVX-512 Conflict Detection */
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index ad4b159..f9f4cd1 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -865,6 +865,29 @@ int kvm_arch_init_vcpu(CPUState *cs)
c = &cpuid_data.entries[cpuid_i++];
}
break;
+ case 0x14: {
+ uint32_t times;
+
+ c->function = i;
+ c->index = 0;
+ c->flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+ cpu_x86_cpuid(env, i, 0, &c->eax, &c->ebx, &c->ecx, &c->edx);
+ times = c->eax;
+
+ for (j = 1; j <= times; ++j) {
+ if (cpuid_i == KVM_MAX_CPUID_ENTRIES) {
+ fprintf(stderr, "cpuid_data is full, no space for "
+ "cpuid(eax:0x14,ecx:0x%x)\n", j);
+ abort();
+ }
+ c = &cpuid_data.entries[cpuid_i++];
+ c->function = i;
+ c->index = j;
+ c->flags = KVM_CPUID_FLAG_SIGNIFCANT_INDEX;
+ cpu_x86_cpuid(env, i, j, &c->eax, &c->ebx, &c->ecx, &c->edx);
+ }
+ break;
+ }
default:
c->function = i;
c->flags = 0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
2018-03-04 16:48 ` [Qemu-devel] " Luwei Kang
@ 2018-03-04 16:48 ` Luwei Kang
-1 siblings, 0 replies; 30+ messages in thread
From: Luwei Kang @ 2018-03-04 16:48 UTC (permalink / raw)
To: qemu-devel, kvm; +Cc: Luwei Kang, ehabkost, mtosatti, Chao Peng, pbonzini, rth
From: Chao Peng <chao.p.peng@linux.intel.com>
Add Intel Processor Trace related definition. It also add
corresponding part to kvm_get/set_msr and vmstate.
Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
target/i386/cpu.h | 22 ++++++++++++++++++++++
target/i386/kvm.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
target/i386/machine.c | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 111 insertions(+)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index f5b9ecb..5457d3b 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -415,6 +415,21 @@ typedef enum X86Seg {
#define MSR_MC0_ADDR 0x402
#define MSR_MC0_MISC 0x403
+#define MSR_IA32_RTIT_OUTPUT_BASE 0x560
+#define MSR_IA32_RTIT_OUTPUT_MASK 0x561
+#define MSR_IA32_RTIT_CTL 0x570
+#define MSR_IA32_RTIT_STATUS 0x571
+#define MSR_IA32_RTIT_CR3_MATCH 0x572
+#define MSR_IA32_RTIT_ADDR0_A 0x580
+#define MSR_IA32_RTIT_ADDR0_B 0x581
+#define MSR_IA32_RTIT_ADDR1_A 0x582
+#define MSR_IA32_RTIT_ADDR1_B 0x583
+#define MSR_IA32_RTIT_ADDR2_A 0x584
+#define MSR_IA32_RTIT_ADDR2_B 0x585
+#define MSR_IA32_RTIT_ADDR3_A 0x586
+#define MSR_IA32_RTIT_ADDR3_B 0x587
+#define MAX_RTIT_ADDRS 8
+
#define MSR_EFER 0xc0000080
#define MSR_EFER_SCE (1 << 0)
@@ -1154,6 +1169,13 @@ typedef struct CPUX86State {
uint64_t msr_hv_stimer_config[HV_STIMER_COUNT];
uint64_t msr_hv_stimer_count[HV_STIMER_COUNT];
+ uint64_t msr_rtit_ctrl;
+ uint64_t msr_rtit_status;
+ uint64_t msr_rtit_output_base;
+ uint64_t msr_rtit_output_mask;
+ uint64_t msr_rtit_cr3_match;
+ uint64_t msr_rtit_addrs[MAX_RTIT_ADDRS];
+
/* exception/interrupt handling */
int error_code;
int exception_is_int;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index f9f4cd1..097c953 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
}
}
+ if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
+ int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
+ 0x14, 1, R_EAX) & 0x7;
+
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
+ env->msr_rtit_ctrl);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
+ env->msr_rtit_status);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
+ env->msr_rtit_output_base);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_MASK,
+ env->msr_rtit_output_mask);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CR3_MATCH,
+ env->msr_rtit_cr3_match);
+ for (i = 0; i < addr_num; i++) {
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_ADDR0_A + i,
+ env->msr_rtit_addrs[i]);
+ }
+ }
/* Note: MSR_IA32_FEATURE_CONTROL is written separately, see
* kvm_put_msr_feature_control. */
@@ -2124,6 +2143,20 @@ static int kvm_get_msrs(X86CPU *cpu)
}
}
+ if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
+ int addr_num =
+ kvm_arch_get_supported_cpuid(kvm_state, 0x14, 1, R_EAX) & 0x7;
+
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL, 0);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS, 0);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE, 0);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_MASK, 0);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CR3_MATCH, 0);
+ for (i = 0; i < addr_num; i++) {
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_ADDR0_A + i, 0);
+ }
+ }
+
ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, cpu->kvm_msr_buf);
if (ret < 0) {
return ret;
@@ -2364,6 +2397,24 @@ static int kvm_get_msrs(X86CPU *cpu)
case MSR_IA32_SPEC_CTRL:
env->spec_ctrl = msrs[i].data;
break;
+ case MSR_IA32_RTIT_CTL:
+ env->msr_rtit_ctrl = msrs[i].data;
+ break;
+ case MSR_IA32_RTIT_STATUS:
+ env->msr_rtit_status = msrs[i].data;
+ break;
+ case MSR_IA32_RTIT_OUTPUT_BASE:
+ env->msr_rtit_output_base = msrs[i].data;
+ break;
+ case MSR_IA32_RTIT_OUTPUT_MASK:
+ env->msr_rtit_output_mask = msrs[i].data;
+ break;
+ case MSR_IA32_RTIT_CR3_MATCH:
+ env->msr_rtit_cr3_match = msrs[i].data;
+ break;
+ case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
+ env->msr_rtit_addrs[index - MSR_IA32_RTIT_ADDR0_A] = msrs[i].data;
+ break;
}
}
diff --git a/target/i386/machine.c b/target/i386/machine.c
index 361c05a..c05fe6f 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -837,6 +837,43 @@ static const VMStateDescription vmstate_spec_ctrl = {
}
};
+static bool intel_pt_enable_needed(void *opaque)
+{
+ X86CPU *cpu = opaque;
+ CPUX86State *env = &cpu->env;
+ int i;
+
+ if (env->msr_rtit_ctrl || env->msr_rtit_status ||
+ env->msr_rtit_output_base || env->msr_rtit_output_mask ||
+ env->msr_rtit_cr3_match) {
+ return true;
+ }
+
+ for (i = 0; i < MAX_RTIT_ADDRS; i++) {
+ if (env->msr_rtit_addrs[i]) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+static const VMStateDescription vmstate_msr_intel_pt = {
+ .name = "cpu/intel_pt",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = intel_pt_enable_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(env.msr_rtit_ctrl, X86CPU),
+ VMSTATE_UINT64(env.msr_rtit_status, X86CPU),
+ VMSTATE_UINT64(env.msr_rtit_output_base, X86CPU),
+ VMSTATE_UINT64(env.msr_rtit_output_mask, X86CPU),
+ VMSTATE_UINT64(env.msr_rtit_cr3_match, X86CPU),
+ VMSTATE_UINT64_ARRAY(env.msr_rtit_addrs, X86CPU, MAX_RTIT_ADDRS),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
VMStateDescription vmstate_x86_cpu = {
.name = "cpu",
.version_id = 12,
@@ -957,6 +994,7 @@ VMStateDescription vmstate_x86_cpu = {
#endif
&vmstate_spec_ctrl,
&vmstate_mcg_ext_ctl,
+ &vmstate_msr_intel_pt,
NULL
}
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [Qemu-devel] [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
@ 2018-03-04 16:48 ` Luwei Kang
0 siblings, 0 replies; 30+ messages in thread
From: Luwei Kang @ 2018-03-04 16:48 UTC (permalink / raw)
To: qemu-devel, kvm; +Cc: pbonzini, rth, ehabkost, mtosatti, Chao Peng, Luwei Kang
From: Chao Peng <chao.p.peng@linux.intel.com>
Add Intel Processor Trace related definition. It also add
corresponding part to kvm_get/set_msr and vmstate.
Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
target/i386/cpu.h | 22 ++++++++++++++++++++++
target/i386/kvm.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
target/i386/machine.c | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 111 insertions(+)
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index f5b9ecb..5457d3b 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -415,6 +415,21 @@ typedef enum X86Seg {
#define MSR_MC0_ADDR 0x402
#define MSR_MC0_MISC 0x403
+#define MSR_IA32_RTIT_OUTPUT_BASE 0x560
+#define MSR_IA32_RTIT_OUTPUT_MASK 0x561
+#define MSR_IA32_RTIT_CTL 0x570
+#define MSR_IA32_RTIT_STATUS 0x571
+#define MSR_IA32_RTIT_CR3_MATCH 0x572
+#define MSR_IA32_RTIT_ADDR0_A 0x580
+#define MSR_IA32_RTIT_ADDR0_B 0x581
+#define MSR_IA32_RTIT_ADDR1_A 0x582
+#define MSR_IA32_RTIT_ADDR1_B 0x583
+#define MSR_IA32_RTIT_ADDR2_A 0x584
+#define MSR_IA32_RTIT_ADDR2_B 0x585
+#define MSR_IA32_RTIT_ADDR3_A 0x586
+#define MSR_IA32_RTIT_ADDR3_B 0x587
+#define MAX_RTIT_ADDRS 8
+
#define MSR_EFER 0xc0000080
#define MSR_EFER_SCE (1 << 0)
@@ -1154,6 +1169,13 @@ typedef struct CPUX86State {
uint64_t msr_hv_stimer_config[HV_STIMER_COUNT];
uint64_t msr_hv_stimer_count[HV_STIMER_COUNT];
+ uint64_t msr_rtit_ctrl;
+ uint64_t msr_rtit_status;
+ uint64_t msr_rtit_output_base;
+ uint64_t msr_rtit_output_mask;
+ uint64_t msr_rtit_cr3_match;
+ uint64_t msr_rtit_addrs[MAX_RTIT_ADDRS];
+
/* exception/interrupt handling */
int error_code;
int exception_is_int;
diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index f9f4cd1..097c953 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
}
}
+ if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
+ int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
+ 0x14, 1, R_EAX) & 0x7;
+
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
+ env->msr_rtit_ctrl);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
+ env->msr_rtit_status);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
+ env->msr_rtit_output_base);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_MASK,
+ env->msr_rtit_output_mask);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CR3_MATCH,
+ env->msr_rtit_cr3_match);
+ for (i = 0; i < addr_num; i++) {
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_ADDR0_A + i,
+ env->msr_rtit_addrs[i]);
+ }
+ }
/* Note: MSR_IA32_FEATURE_CONTROL is written separately, see
* kvm_put_msr_feature_control. */
@@ -2124,6 +2143,20 @@ static int kvm_get_msrs(X86CPU *cpu)
}
}
+ if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
+ int addr_num =
+ kvm_arch_get_supported_cpuid(kvm_state, 0x14, 1, R_EAX) & 0x7;
+
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL, 0);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS, 0);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE, 0);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_MASK, 0);
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CR3_MATCH, 0);
+ for (i = 0; i < addr_num; i++) {
+ kvm_msr_entry_add(cpu, MSR_IA32_RTIT_ADDR0_A + i, 0);
+ }
+ }
+
ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, cpu->kvm_msr_buf);
if (ret < 0) {
return ret;
@@ -2364,6 +2397,24 @@ static int kvm_get_msrs(X86CPU *cpu)
case MSR_IA32_SPEC_CTRL:
env->spec_ctrl = msrs[i].data;
break;
+ case MSR_IA32_RTIT_CTL:
+ env->msr_rtit_ctrl = msrs[i].data;
+ break;
+ case MSR_IA32_RTIT_STATUS:
+ env->msr_rtit_status = msrs[i].data;
+ break;
+ case MSR_IA32_RTIT_OUTPUT_BASE:
+ env->msr_rtit_output_base = msrs[i].data;
+ break;
+ case MSR_IA32_RTIT_OUTPUT_MASK:
+ env->msr_rtit_output_mask = msrs[i].data;
+ break;
+ case MSR_IA32_RTIT_CR3_MATCH:
+ env->msr_rtit_cr3_match = msrs[i].data;
+ break;
+ case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
+ env->msr_rtit_addrs[index - MSR_IA32_RTIT_ADDR0_A] = msrs[i].data;
+ break;
}
}
diff --git a/target/i386/machine.c b/target/i386/machine.c
index 361c05a..c05fe6f 100644
--- a/target/i386/machine.c
+++ b/target/i386/machine.c
@@ -837,6 +837,43 @@ static const VMStateDescription vmstate_spec_ctrl = {
}
};
+static bool intel_pt_enable_needed(void *opaque)
+{
+ X86CPU *cpu = opaque;
+ CPUX86State *env = &cpu->env;
+ int i;
+
+ if (env->msr_rtit_ctrl || env->msr_rtit_status ||
+ env->msr_rtit_output_base || env->msr_rtit_output_mask ||
+ env->msr_rtit_cr3_match) {
+ return true;
+ }
+
+ for (i = 0; i < MAX_RTIT_ADDRS; i++) {
+ if (env->msr_rtit_addrs[i]) {
+ return true;
+ }
+ }
+
+ return false;
+}
+
+static const VMStateDescription vmstate_msr_intel_pt = {
+ .name = "cpu/intel_pt",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = intel_pt_enable_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(env.msr_rtit_ctrl, X86CPU),
+ VMSTATE_UINT64(env.msr_rtit_status, X86CPU),
+ VMSTATE_UINT64(env.msr_rtit_output_base, X86CPU),
+ VMSTATE_UINT64(env.msr_rtit_output_mask, X86CPU),
+ VMSTATE_UINT64(env.msr_rtit_cr3_match, X86CPU),
+ VMSTATE_UINT64_ARRAY(env.msr_rtit_addrs, X86CPU, MAX_RTIT_ADDRS),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
VMStateDescription vmstate_x86_cpu = {
.name = "cpu",
.version_id = 12,
@@ -957,6 +994,7 @@ VMStateDescription vmstate_x86_cpu = {
#endif
&vmstate_spec_ctrl,
&vmstate_mcg_ext_ctl,
+ &vmstate_msr_intel_pt,
NULL
}
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v4 1/2] i386: Add Intel Processor Trace feature support
2018-03-04 16:48 ` [Qemu-devel] " Luwei Kang
@ 2018-03-09 19:10 ` Eduardo Habkost
-1 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2018-03-09 19:10 UTC (permalink / raw)
To: Luwei Kang; +Cc: kvm, mtosatti, qemu-devel, pbonzini, Chao Peng, rth
On Mon, Mar 05, 2018 at 12:48:35AM +0800, Luwei Kang wrote:
> From: Chao Peng <chao.p.peng@linux.intel.com>
>
> Expose Intel Processor Trace feature to guest.
>
> To make Intel PT live migration safe and get same CPUID information
> with same CPU model on diffrent host. CPUID[14] is constant in this
> patch. Intel PT use EPT is first supported in IceLake, the CPUID[14]
> get on this machine as default value. Intel PT would be disabled
> if any machine don't support this minial feature list.
>
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> ---
> From V3:
> - fix some typo;
> - add some comments and safty check.
>
> ---
> target/i386/cpu.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> target/i386/cpu.h | 1 +
> target/i386/kvm.c | 23 ++++++++++++++++
> 3 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b5e431e..24e1693 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -173,7 +173,32 @@
> #define L2_ITLB_4K_ASSOC 4
> #define L2_ITLB_4K_ENTRIES 512
>
> -
> +/* CPUID Leaf 0x14 constants: */
> +#define INTEL_PT_MAX_SUBLEAF 0x1
> +/*
> + * bit[00]: IA32_RTIT_CTL.CR3 filter can be set to 1 and IA32_RTIT_CR3_MATCH
> + * MSR can be accessed;
> + * bit[01]: Support Configurable PSB and Cycle-Accurate Mode;
> + * bit[02]: Support IP Filtering, TraceStop filtering, and preservation
> + * of Intel PT MSRs across warm reset;
> + * bit[03]: Support MTC timing packet and suppression of COFI-based packets;
> + */
> +#define INTEL_PT_MINIMAL_EBX 0xf
Thanks! I didn't expect a detailed description of each bit. I
thought that just adding macros for each bit instead of
hardcoding 0xf would be enough.
But after reading the docs, I understand it could be difficult to
choose a macro name for something like "support of IP Filtering,
TraceStop filtering, and preservation of Intel PT MSRs across
warm reset", so this description looks like the best we can do.
:)
I only see a problem below:
> +/*
> + * bit[00]: Tracing can be enabled with IA32_RTIT_CTL.ToPA = 1 and
> + * IA32_RTIT_OUTPUT_BASE and IA32_RTIT_OUTPUT_MASK_PTRS MSRs can be
> + * accessed;
> + * bit[01]: ToPA tables can hold any number of output entries, up to the
> + * maximum allowed by the MaskOrTableOffset field of
> + * IA32_RTIT_OUTPUT_MASK_PTRS;
> + * bit[02]: Support Single-Range Output scheme;
> + */
> +#define INTEL_PT_MINIMAL_ECX 0x7
> +#define INTEL_PT_ADDR_RANGES_NUM 0x2 /* Number of configurable address ranges */
> +#define INTEL_PT_ADDR_RANGES_NUM_MASK 0x3
> +#define INTEL_PT_MTC_BITMAP (0x0249 << 16) /* Support ART(0,3,6,9) */
> +#define INTEL_PT_CYCLE_BITMAP 0x1fff /* Support 0,2^(0~11) */
> +#define INTEL_PT_PSB_BITMAP (0x003f << 16) /* Support 2K,4K,8K,16K,32K,64K */
>
> static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> uint32_t vendor2, uint32_t vendor3)
[...]
> @@ -4083,6 +4129,34 @@ static int x86_cpu_filter_features(X86CPU *cpu)
> }
> }
>
> + if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
> + kvm_enabled()) {
> + KVMState *s = CPU(cpu)->kvm_state;
> + uint32_t eax_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EAX);
> + uint32_t ebx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EBX);
> + uint32_t ecx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_ECX);
> + uint32_t eax_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EAX);
> + uint32_t ebx_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EBX);
> +
> + if (!eax_0 ||
> + ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
> + ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
> + ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
> + ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
> + INTEL_PT_ADDR_RANGES_NUM) ||
> + ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
> + (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) {
I still don't see a check to ensure the host has bit 31 on ecx_0
set to 0, as I mentioned when reviewing v3.
The rest of the patch looks good.
> + /*
> + * Processor Trace capabilities aren't configurable, so if the
> + * host can't emulate the capabilities we report on
> + * cpu_x86_cpuid(), intel-pt can't be enabled on the current host.
> + */
> + env->features[FEAT_7_0_EBX] &= ~CPUID_7_0_EBX_INTEL_PT;
> + cpu->filtered_features[FEAT_7_0_EBX] |= CPUID_7_0_EBX_INTEL_PT;
> + rv = 1;
> + }
> + }
> +
> return rv;
> }
>
[...]
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] i386: Add Intel Processor Trace feature support
@ 2018-03-09 19:10 ` Eduardo Habkost
0 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2018-03-09 19:10 UTC (permalink / raw)
To: Luwei Kang; +Cc: qemu-devel, kvm, mtosatti, Chao Peng, pbonzini, rth
On Mon, Mar 05, 2018 at 12:48:35AM +0800, Luwei Kang wrote:
> From: Chao Peng <chao.p.peng@linux.intel.com>
>
> Expose Intel Processor Trace feature to guest.
>
> To make Intel PT live migration safe and get same CPUID information
> with same CPU model on diffrent host. CPUID[14] is constant in this
> patch. Intel PT use EPT is first supported in IceLake, the CPUID[14]
> get on this machine as default value. Intel PT would be disabled
> if any machine don't support this minial feature list.
>
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
> ---
> From V3:
> - fix some typo;
> - add some comments and safty check.
>
> ---
> target/i386/cpu.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> target/i386/cpu.h | 1 +
> target/i386/kvm.c | 23 ++++++++++++++++
> 3 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index b5e431e..24e1693 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -173,7 +173,32 @@
> #define L2_ITLB_4K_ASSOC 4
> #define L2_ITLB_4K_ENTRIES 512
>
> -
> +/* CPUID Leaf 0x14 constants: */
> +#define INTEL_PT_MAX_SUBLEAF 0x1
> +/*
> + * bit[00]: IA32_RTIT_CTL.CR3 filter can be set to 1 and IA32_RTIT_CR3_MATCH
> + * MSR can be accessed;
> + * bit[01]: Support Configurable PSB and Cycle-Accurate Mode;
> + * bit[02]: Support IP Filtering, TraceStop filtering, and preservation
> + * of Intel PT MSRs across warm reset;
> + * bit[03]: Support MTC timing packet and suppression of COFI-based packets;
> + */
> +#define INTEL_PT_MINIMAL_EBX 0xf
Thanks! I didn't expect a detailed description of each bit. I
thought that just adding macros for each bit instead of
hardcoding 0xf would be enough.
But after reading the docs, I understand it could be difficult to
choose a macro name for something like "support of IP Filtering,
TraceStop filtering, and preservation of Intel PT MSRs across
warm reset", so this description looks like the best we can do.
:)
I only see a problem below:
> +/*
> + * bit[00]: Tracing can be enabled with IA32_RTIT_CTL.ToPA = 1 and
> + * IA32_RTIT_OUTPUT_BASE and IA32_RTIT_OUTPUT_MASK_PTRS MSRs can be
> + * accessed;
> + * bit[01]: ToPA tables can hold any number of output entries, up to the
> + * maximum allowed by the MaskOrTableOffset field of
> + * IA32_RTIT_OUTPUT_MASK_PTRS;
> + * bit[02]: Support Single-Range Output scheme;
> + */
> +#define INTEL_PT_MINIMAL_ECX 0x7
> +#define INTEL_PT_ADDR_RANGES_NUM 0x2 /* Number of configurable address ranges */
> +#define INTEL_PT_ADDR_RANGES_NUM_MASK 0x3
> +#define INTEL_PT_MTC_BITMAP (0x0249 << 16) /* Support ART(0,3,6,9) */
> +#define INTEL_PT_CYCLE_BITMAP 0x1fff /* Support 0,2^(0~11) */
> +#define INTEL_PT_PSB_BITMAP (0x003f << 16) /* Support 2K,4K,8K,16K,32K,64K */
>
> static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
> uint32_t vendor2, uint32_t vendor3)
[...]
> @@ -4083,6 +4129,34 @@ static int x86_cpu_filter_features(X86CPU *cpu)
> }
> }
>
> + if ((env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) &&
> + kvm_enabled()) {
> + KVMState *s = CPU(cpu)->kvm_state;
> + uint32_t eax_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EAX);
> + uint32_t ebx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_EBX);
> + uint32_t ecx_0 = kvm_arch_get_supported_cpuid(s, 0x14, 0, R_ECX);
> + uint32_t eax_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EAX);
> + uint32_t ebx_1 = kvm_arch_get_supported_cpuid(s, 0x14, 1, R_EBX);
> +
> + if (!eax_0 ||
> + ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
> + ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
> + ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
> + ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
> + INTEL_PT_ADDR_RANGES_NUM) ||
> + ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
> + (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) {
I still don't see a check to ensure the host has bit 31 on ecx_0
set to 0, as I mentioned when reviewing v3.
The rest of the patch looks good.
> + /*
> + * Processor Trace capabilities aren't configurable, so if the
> + * host can't emulate the capabilities we report on
> + * cpu_x86_cpuid(), intel-pt can't be enabled on the current host.
> + */
> + env->features[FEAT_7_0_EBX] &= ~CPUID_7_0_EBX_INTEL_PT;
> + cpu->filtered_features[FEAT_7_0_EBX] |= CPUID_7_0_EBX_INTEL_PT;
> + rv = 1;
> + }
> + }
> +
> return rv;
> }
>
[...]
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 1/2] i386: Add Intel Processor Trace feature support
2018-03-09 19:10 ` [Qemu-devel] " Eduardo Habkost
@ 2018-03-12 9:07 ` Kang, Luwei
-1 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2018-03-12 9:07 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: kvm, mtosatti, qemu-devel, pbonzini, Chao Peng, rth
> > +
> > + if (!eax_0 ||
> > + ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
> > + ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
> > + ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
> > + ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
> > + INTEL_PT_ADDR_RANGES_NUM) ||
> > + ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
> > + (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) {
>
> I still don't see a check to ensure the host has bit 31 on ecx_0 set to 0, as I mentioned when reviewing v3.
Hi Eduardo,
Thanks for the code review. I don't quite understand here why bit31 must same with host (meaning we must reject a host
where ecx_0 & (1 << 31) is set).
Do you mean PT must be disabled in guest when host bit31 is set?
Bit 31: If 1, generated packets which contain IP payloads have LIP values, which include the CS base component.
I can't find any special on this bit. Could you help clarify?
Thanks,
Luwei Kang
>
> The rest of the patch looks good.
>
> > + /*
> > + * Processor Trace capabilities aren't configurable, so if the
> > + * host can't emulate the capabilities we report on
> > + * cpu_x86_cpuid(), intel-pt can't be enabled on the current host.
> > + */
> > + env->features[FEAT_7_0_EBX] &= ~CPUID_7_0_EBX_INTEL_PT;
> > + cpu->filtered_features[FEAT_7_0_EBX] |= CPUID_7_0_EBX_INTEL_PT;
> > + rv = 1;
> > + }
> > + }
> > +
> > return rv;
> > }
> >
> [...]
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] i386: Add Intel Processor Trace feature support
@ 2018-03-12 9:07 ` Kang, Luwei
0 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2018-03-12 9:07 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, kvm, mtosatti, Chao Peng, pbonzini, rth
> > +
> > + if (!eax_0 ||
> > + ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
> > + ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
> > + ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
> > + ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
> > + INTEL_PT_ADDR_RANGES_NUM) ||
> > + ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
> > + (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) {
>
> I still don't see a check to ensure the host has bit 31 on ecx_0 set to 0, as I mentioned when reviewing v3.
Hi Eduardo,
Thanks for the code review. I don't quite understand here why bit31 must same with host (meaning we must reject a host
where ecx_0 & (1 << 31) is set).
Do you mean PT must be disabled in guest when host bit31 is set?
Bit 31: If 1, generated packets which contain IP payloads have LIP values, which include the CS base component.
I can't find any special on this bit. Could you help clarify?
Thanks,
Luwei Kang
>
> The rest of the patch looks good.
>
> > + /*
> > + * Processor Trace capabilities aren't configurable, so if the
> > + * host can't emulate the capabilities we report on
> > + * cpu_x86_cpuid(), intel-pt can't be enabled on the current host.
> > + */
> > + env->features[FEAT_7_0_EBX] &= ~CPUID_7_0_EBX_INTEL_PT;
> > + cpu->filtered_features[FEAT_7_0_EBX] |= CPUID_7_0_EBX_INTEL_PT;
> > + rv = 1;
> > + }
> > + }
> > +
> > return rv;
> > }
> >
> [...]
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 1/2] i386: Add Intel Processor Trace feature support
2018-03-12 9:07 ` [Qemu-devel] " Kang, Luwei
@ 2018-03-12 16:45 ` Eduardo Habkost
-1 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2018-03-12 16:45 UTC (permalink / raw)
To: Kang, Luwei; +Cc: kvm, mtosatti, qemu-devel, pbonzini, Chao Peng, rth
On Mon, Mar 12, 2018 at 09:07:41AM +0000, Kang, Luwei wrote:
> > > +
> > > + if (!eax_0 ||
> > > + ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
> > > + ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
> > > + ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
> > > + ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
> > > + INTEL_PT_ADDR_RANGES_NUM) ||
> > > + ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
> > > + (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) {
> >
> > I still don't see a check to ensure the host has bit 31 on ecx_0 set to 0, as I mentioned when reviewing v3.
>
> Hi Eduardo,
> Thanks for the code review. I don't quite understand here why bit31 must same with host (meaning we must reject a host
> where ecx_0 & (1 << 31) is set).
If the guest sees the bit set to 0, it will expect IP payloads
with RIP values, but the host CPU will generate IP payloads with
LIP values. I assume KVM won't do RIP<->LIP translation on the
packets generated by the host before the guest sees them, will
it?
> Do you mean PT must be disabled in guest when host bit31 is set?
> Bit 31: If 1, generated packets which contain IP payloads have LIP values, which include the CS base component.
> I can't find any special on this bit. Could you help clarify?
As far as I understand, this bit is special because KVM can't
emulate a value that's different from the host.
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] i386: Add Intel Processor Trace feature support
@ 2018-03-12 16:45 ` Eduardo Habkost
0 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2018-03-12 16:45 UTC (permalink / raw)
To: Kang, Luwei; +Cc: qemu-devel, kvm, mtosatti, Chao Peng, pbonzini, rth
On Mon, Mar 12, 2018 at 09:07:41AM +0000, Kang, Luwei wrote:
> > > +
> > > + if (!eax_0 ||
> > > + ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
> > > + ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
> > > + ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
> > > + ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
> > > + INTEL_PT_ADDR_RANGES_NUM) ||
> > > + ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
> > > + (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) {
> >
> > I still don't see a check to ensure the host has bit 31 on ecx_0 set to 0, as I mentioned when reviewing v3.
>
> Hi Eduardo,
> Thanks for the code review. I don't quite understand here why bit31 must same with host (meaning we must reject a host
> where ecx_0 & (1 << 31) is set).
If the guest sees the bit set to 0, it will expect IP payloads
with RIP values, but the host CPU will generate IP payloads with
LIP values. I assume KVM won't do RIP<->LIP translation on the
packets generated by the host before the guest sees them, will
it?
> Do you mean PT must be disabled in guest when host bit31 is set?
> Bit 31: If 1, generated packets which contain IP payloads have LIP values, which include the CS base component.
> I can't find any special on this bit. Could you help clarify?
As far as I understand, this bit is special because KVM can't
emulate a value that's different from the host.
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 1/2] i386: Add Intel Processor Trace feature support
2018-03-12 16:45 ` [Qemu-devel] " Eduardo Habkost
@ 2018-03-13 11:16 ` Kang, Luwei
-1 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2018-03-13 11:16 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: kvm, mtosatti, qemu-devel, pbonzini, Chao Peng, rth
> > > > + if (!eax_0 ||
> > > > + ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
> > > > + ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
> > > > + ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
> > > > + ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
> > > > + INTEL_PT_ADDR_RANGES_NUM) ||
> > > > + ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
> > > > + (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) {
> > >
> > > I still don't see a check to ensure the host has bit 31 on ecx_0 set to 0, as I mentioned when reviewing v3.
> >
> > Hi Eduardo,
> > Thanks for the code review. I don't quite understand here why
> > bit31 must same with host (meaning we must reject a host where ecx_0 & (1 << 31) is set).
>
> If the guest sees the bit set to 0, it will expect IP payloads with RIP values, but the host CPU will generate IP payloads with LIP values.
> I assume KVM won't do RIP<->LIP translation on the packets generated by the host before the guest sees them, will it?
Fully understand. Will make a separate patch on this.
Thanks,
Luwei Kang
>
>
> > Do you mean PT must be disabled in guest when host bit31 is set?
> > Bit 31: If 1, generated packets which contain IP payloads have LIP values, which include the CS base component.
> > I can't find any special on this bit. Could you help clarify?
>
> As far as I understand, this bit is special because KVM can't emulate a value that's different from the host.
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/2] i386: Add Intel Processor Trace feature support
@ 2018-03-13 11:16 ` Kang, Luwei
0 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2018-03-13 11:16 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, kvm, mtosatti, Chao Peng, pbonzini, rth
> > > > + if (!eax_0 ||
> > > > + ((ebx_0 & INTEL_PT_MINIMAL_EBX) != INTEL_PT_MINIMAL_EBX) ||
> > > > + ((ecx_0 & INTEL_PT_MINIMAL_ECX) != INTEL_PT_MINIMAL_ECX) ||
> > > > + ((eax_1 & INTEL_PT_MTC_BITMAP) != INTEL_PT_MTC_BITMAP) ||
> > > > + ((eax_1 & INTEL_PT_ADDR_RANGES_NUM_MASK) <
> > > > + INTEL_PT_ADDR_RANGES_NUM) ||
> > > > + ((ebx_1 & (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP)) !=
> > > > + (INTEL_PT_PSB_BITMAP | INTEL_PT_CYCLE_BITMAP))) {
> > >
> > > I still don't see a check to ensure the host has bit 31 on ecx_0 set to 0, as I mentioned when reviewing v3.
> >
> > Hi Eduardo,
> > Thanks for the code review. I don't quite understand here why
> > bit31 must same with host (meaning we must reject a host where ecx_0 & (1 << 31) is set).
>
> If the guest sees the bit set to 0, it will expect IP payloads with RIP values, but the host CPU will generate IP payloads with LIP values.
> I assume KVM won't do RIP<->LIP translation on the packets generated by the host before the guest sees them, will it?
Fully understand. Will make a separate patch on this.
Thanks,
Luwei Kang
>
>
> > Do you mean PT must be disabled in guest when host bit31 is set?
> > Bit 31: If 1, generated packets which contain IP payloads have LIP values, which include the CS base component.
> > I can't find any special on this bit. Could you help clarify?
>
> As far as I understand, this bit is special because KVM can't emulate a value that's different from the host.
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
2018-03-04 16:48 ` [Qemu-devel] " Luwei Kang
@ 2019-10-12 3:14 ` Eduardo Habkost
-1 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2019-10-12 3:14 UTC (permalink / raw)
To: Luwei Kang; +Cc: qemu-devel, kvm, pbonzini, rth, mtosatti, Chao Peng
On Mon, Mar 05, 2018 at 12:48:36AM +0800, Luwei Kang wrote:
> From: Chao Peng <chao.p.peng@linux.intel.com>
>
> Add Intel Processor Trace related definition. It also add
> corresponding part to kvm_get/set_msr and vmstate.
>
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
[...]
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index f9f4cd1..097c953 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> }
> }
> + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> + 0x14, 1, R_EAX) & 0x7;
> +
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> + env->msr_rtit_ctrl);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> + env->msr_rtit_status);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> + env->msr_rtit_output_base);
This causes the following crash on some hosts:
qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
additional conditions that might prevent writing to this MSR
(PT_CAP_topa_output && PT_CAP_single_range_output). This causes
QEMU to crash if some of the conditions aren't met.
Writing and reading this MSR (and the ones below) need to be
conditional on KVM_GET_MSR_INDEX_LIST.
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_MASK,
> + env->msr_rtit_output_mask);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CR3_MATCH,
> + env->msr_rtit_cr3_match);
> + for (i = 0; i < addr_num; i++) {
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_ADDR0_A + i,
> + env->msr_rtit_addrs[i]);
> + }
> + }
>
> /* Note: MSR_IA32_FEATURE_CONTROL is written separately, see
> * kvm_put_msr_feature_control. */
> @@ -2124,6 +2143,20 @@ static int kvm_get_msrs(X86CPU *cpu)
> }
> }
>
> + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> + int addr_num =
> + kvm_arch_get_supported_cpuid(kvm_state, 0x14, 1, R_EAX) & 0x7;
> +
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL, 0);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS, 0);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE, 0);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_MASK, 0);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CR3_MATCH, 0);
> + for (i = 0; i < addr_num; i++) {
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_ADDR0_A + i, 0);
> + }
> + }
> +
> ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, cpu->kvm_msr_buf);
> if (ret < 0) {
> return ret;
> @@ -2364,6 +2397,24 @@ static int kvm_get_msrs(X86CPU *cpu)
> case MSR_IA32_SPEC_CTRL:
> env->spec_ctrl = msrs[i].data;
> break;
> + case MSR_IA32_RTIT_CTL:
> + env->msr_rtit_ctrl = msrs[i].data;
> + break;
> + case MSR_IA32_RTIT_STATUS:
> + env->msr_rtit_status = msrs[i].data;
> + break;
> + case MSR_IA32_RTIT_OUTPUT_BASE:
> + env->msr_rtit_output_base = msrs[i].data;
> + break;
> + case MSR_IA32_RTIT_OUTPUT_MASK:
> + env->msr_rtit_output_mask = msrs[i].data;
> + break;
> + case MSR_IA32_RTIT_CR3_MATCH:
> + env->msr_rtit_cr3_match = msrs[i].data;
> + break;
> + case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
> + env->msr_rtit_addrs[index - MSR_IA32_RTIT_ADDR0_A] = msrs[i].data;
> + break;
> }
> }
>
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index 361c05a..c05fe6f 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -837,6 +837,43 @@ static const VMStateDescription vmstate_spec_ctrl = {
> }
> };
>
> +static bool intel_pt_enable_needed(void *opaque)
> +{
> + X86CPU *cpu = opaque;
> + CPUX86State *env = &cpu->env;
> + int i;
> +
> + if (env->msr_rtit_ctrl || env->msr_rtit_status ||
> + env->msr_rtit_output_base || env->msr_rtit_output_mask ||
> + env->msr_rtit_cr3_match) {
> + return true;
> + }
> +
> + for (i = 0; i < MAX_RTIT_ADDRS; i++) {
> + if (env->msr_rtit_addrs[i]) {
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> +static const VMStateDescription vmstate_msr_intel_pt = {
> + .name = "cpu/intel_pt",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = intel_pt_enable_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT64(env.msr_rtit_ctrl, X86CPU),
> + VMSTATE_UINT64(env.msr_rtit_status, X86CPU),
> + VMSTATE_UINT64(env.msr_rtit_output_base, X86CPU),
> + VMSTATE_UINT64(env.msr_rtit_output_mask, X86CPU),
> + VMSTATE_UINT64(env.msr_rtit_cr3_match, X86CPU),
> + VMSTATE_UINT64_ARRAY(env.msr_rtit_addrs, X86CPU, MAX_RTIT_ADDRS),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> VMStateDescription vmstate_x86_cpu = {
> .name = "cpu",
> .version_id = 12,
> @@ -957,6 +994,7 @@ VMStateDescription vmstate_x86_cpu = {
> #endif
> &vmstate_spec_ctrl,
> &vmstate_mcg_ext_ctl,
> + &vmstate_msr_intel_pt,
> NULL
> }
> };
> --
> 1.8.3.1
>
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
@ 2019-10-12 3:14 ` Eduardo Habkost
0 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2019-10-12 3:14 UTC (permalink / raw)
To: Luwei Kang; +Cc: kvm, mtosatti, qemu-devel, Chao Peng, pbonzini, rth
On Mon, Mar 05, 2018 at 12:48:36AM +0800, Luwei Kang wrote:
> From: Chao Peng <chao.p.peng@linux.intel.com>
>
> Add Intel Processor Trace related definition. It also add
> corresponding part to kvm_get/set_msr and vmstate.
>
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> Signed-off-by: Luwei Kang <luwei.kang@intel.com>
[...]
> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> index f9f4cd1..097c953 100644
> --- a/target/i386/kvm.c
> +++ b/target/i386/kvm.c
> @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> }
> }
> + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> + 0x14, 1, R_EAX) & 0x7;
> +
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> + env->msr_rtit_ctrl);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> + env->msr_rtit_status);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> + env->msr_rtit_output_base);
This causes the following crash on some hosts:
qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
additional conditions that might prevent writing to this MSR
(PT_CAP_topa_output && PT_CAP_single_range_output). This causes
QEMU to crash if some of the conditions aren't met.
Writing and reading this MSR (and the ones below) need to be
conditional on KVM_GET_MSR_INDEX_LIST.
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_MASK,
> + env->msr_rtit_output_mask);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CR3_MATCH,
> + env->msr_rtit_cr3_match);
> + for (i = 0; i < addr_num; i++) {
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_ADDR0_A + i,
> + env->msr_rtit_addrs[i]);
> + }
> + }
>
> /* Note: MSR_IA32_FEATURE_CONTROL is written separately, see
> * kvm_put_msr_feature_control. */
> @@ -2124,6 +2143,20 @@ static int kvm_get_msrs(X86CPU *cpu)
> }
> }
>
> + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> + int addr_num =
> + kvm_arch_get_supported_cpuid(kvm_state, 0x14, 1, R_EAX) & 0x7;
> +
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL, 0);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS, 0);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE, 0);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_MASK, 0);
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CR3_MATCH, 0);
> + for (i = 0; i < addr_num; i++) {
> + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_ADDR0_A + i, 0);
> + }
> + }
> +
> ret = kvm_vcpu_ioctl(CPU(cpu), KVM_GET_MSRS, cpu->kvm_msr_buf);
> if (ret < 0) {
> return ret;
> @@ -2364,6 +2397,24 @@ static int kvm_get_msrs(X86CPU *cpu)
> case MSR_IA32_SPEC_CTRL:
> env->spec_ctrl = msrs[i].data;
> break;
> + case MSR_IA32_RTIT_CTL:
> + env->msr_rtit_ctrl = msrs[i].data;
> + break;
> + case MSR_IA32_RTIT_STATUS:
> + env->msr_rtit_status = msrs[i].data;
> + break;
> + case MSR_IA32_RTIT_OUTPUT_BASE:
> + env->msr_rtit_output_base = msrs[i].data;
> + break;
> + case MSR_IA32_RTIT_OUTPUT_MASK:
> + env->msr_rtit_output_mask = msrs[i].data;
> + break;
> + case MSR_IA32_RTIT_CR3_MATCH:
> + env->msr_rtit_cr3_match = msrs[i].data;
> + break;
> + case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
> + env->msr_rtit_addrs[index - MSR_IA32_RTIT_ADDR0_A] = msrs[i].data;
> + break;
> }
> }
>
> diff --git a/target/i386/machine.c b/target/i386/machine.c
> index 361c05a..c05fe6f 100644
> --- a/target/i386/machine.c
> +++ b/target/i386/machine.c
> @@ -837,6 +837,43 @@ static const VMStateDescription vmstate_spec_ctrl = {
> }
> };
>
> +static bool intel_pt_enable_needed(void *opaque)
> +{
> + X86CPU *cpu = opaque;
> + CPUX86State *env = &cpu->env;
> + int i;
> +
> + if (env->msr_rtit_ctrl || env->msr_rtit_status ||
> + env->msr_rtit_output_base || env->msr_rtit_output_mask ||
> + env->msr_rtit_cr3_match) {
> + return true;
> + }
> +
> + for (i = 0; i < MAX_RTIT_ADDRS; i++) {
> + if (env->msr_rtit_addrs[i]) {
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
> +static const VMStateDescription vmstate_msr_intel_pt = {
> + .name = "cpu/intel_pt",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = intel_pt_enable_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT64(env.msr_rtit_ctrl, X86CPU),
> + VMSTATE_UINT64(env.msr_rtit_status, X86CPU),
> + VMSTATE_UINT64(env.msr_rtit_output_base, X86CPU),
> + VMSTATE_UINT64(env.msr_rtit_output_mask, X86CPU),
> + VMSTATE_UINT64(env.msr_rtit_cr3_match, X86CPU),
> + VMSTATE_UINT64_ARRAY(env.msr_rtit_addrs, X86CPU, MAX_RTIT_ADDRS),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> VMStateDescription vmstate_x86_cpu = {
> .name = "cpu",
> .version_id = 12,
> @@ -957,6 +994,7 @@ VMStateDescription vmstate_x86_cpu = {
> #endif
> &vmstate_spec_ctrl,
> &vmstate_mcg_ext_ctl,
> + &vmstate_msr_intel_pt,
> NULL
> }
> };
> --
> 1.8.3.1
>
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
2019-10-12 3:14 ` Eduardo Habkost
@ 2019-10-15 12:51 ` Kang, Luwei
-1 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2019-10-15 12:51 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, kvm, pbonzini, rth, mtosatti, Chao Peng
qemu> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c index
> > f9f4cd1..097c953 100644
> > --- a/target/i386/kvm.c
> > +++ b/target/i386/kvm.c
> > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > }
> > }
> > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > + 0x14, 1, R_EAX) &
> > + 0x7;
> > +
> > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > + env->msr_rtit_ctrl);
> > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > + env->msr_rtit_status);
> > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > + env->msr_rtit_output_base);
>
> This causes the following crash on some hosts:
>
> qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
>
> Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has additional conditions that might prevent writing to this MSR
> (PT_CAP_topa_output && PT_CAP_single_range_output). This causes QEMU to crash if some of the conditions aren't met.
>
> Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
>
Hi Eduardo,
I found this issue can't be reproduced in upstream source code but can be reproduced on RHEL8.1. I haven't got the qemu source code of RHEL8.1. But after adding some trace in KVM, I found the KVM has reported the complete Intel PT CPUID information to qemu but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID to KVM (cpuid level is 0xd). It looks like lost the below patch.
commit f24c3a79a415042f6dc195f029a2ba7247d14cac
Author: Luwei Kang <luwei.kang@intel.com>
Date: Tue Jan 29 18:52:59 2019 -0500
i386: extended the cpuid_level when Intel PT is enabled
Intel Processor Trace required CPUID[0x14] but the cpuid_level
have no change when create a kvm guest with
e.g. "-cpu qemu64,+intel-pt".
Thanks,
Luwei Kang
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
@ 2019-10-15 12:51 ` Kang, Luwei
0 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2019-10-15 12:51 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: kvm, mtosatti, qemu-devel, Chao Peng, pbonzini, rth
qemu> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c index
> > f9f4cd1..097c953 100644
> > --- a/target/i386/kvm.c
> > +++ b/target/i386/kvm.c
> > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > }
> > }
> > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > + 0x14, 1, R_EAX) &
> > + 0x7;
> > +
> > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > + env->msr_rtit_ctrl);
> > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > + env->msr_rtit_status);
> > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > + env->msr_rtit_output_base);
>
> This causes the following crash on some hosts:
>
> qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
>
> Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has additional conditions that might prevent writing to this MSR
> (PT_CAP_topa_output && PT_CAP_single_range_output). This causes QEMU to crash if some of the conditions aren't met.
>
> Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
>
Hi Eduardo,
I found this issue can't be reproduced in upstream source code but can be reproduced on RHEL8.1. I haven't got the qemu source code of RHEL8.1. But after adding some trace in KVM, I found the KVM has reported the complete Intel PT CPUID information to qemu but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID to KVM (cpuid level is 0xd). It looks like lost the below patch.
commit f24c3a79a415042f6dc195f029a2ba7247d14cac
Author: Luwei Kang <luwei.kang@intel.com>
Date: Tue Jan 29 18:52:59 2019 -0500
i386: extended the cpuid_level when Intel PT is enabled
Intel Processor Trace required CPUID[0x14] but the cpuid_level
have no change when create a kvm guest with
e.g. "-cpu qemu64,+intel-pt".
Thanks,
Luwei Kang
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
2019-10-15 12:51 ` Kang, Luwei
@ 2019-10-15 13:29 ` Eduardo Habkost
-1 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2019-10-15 13:29 UTC (permalink / raw)
To: Kang, Luwei; +Cc: qemu-devel, kvm, pbonzini, rth, mtosatti, Chao Peng
On Tue, Oct 15, 2019 at 12:51:48PM +0000, Kang, Luwei wrote:
> qemu> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c index
> > > f9f4cd1..097c953 100644
> > > --- a/target/i386/kvm.c
> > > +++ b/target/i386/kvm.c
> > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > }
> > > }
> > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > + 0x14, 1, R_EAX) &
> > > + 0x7;
> > > +
> > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > + env->msr_rtit_ctrl);
> > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > + env->msr_rtit_status);
> > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > + env->msr_rtit_output_base);
> >
> > This causes the following crash on some hosts:
> >
> > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> >
> > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has additional conditions that might prevent writing to this MSR
> > (PT_CAP_topa_output && PT_CAP_single_range_output). This causes QEMU to crash if some of the conditions aren't met.
> >
> > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> >
>
> Hi Eduardo,
> I found this issue can't be reproduced in upstream source code but can be reproduced on RHEL8.1. I haven't got the qemu source code of RHEL8.1. But after adding some trace in KVM, I found the KVM has reported the complete Intel PT CPUID information to qemu but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID to KVM (cpuid level is 0xd). It looks like lost the below patch.
>
> commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> Author: Luwei Kang <luwei.kang@intel.com>
> Date: Tue Jan 29 18:52:59 2019 -0500
> i386: extended the cpuid_level when Intel PT is enabled
>
> Intel Processor Trace required CPUID[0x14] but the cpuid_level
> have no change when create a kvm guest with
> e.g. "-cpu qemu64,+intel-pt".
Thanks for the pointer. This may avoid triggering the bug in the
default configuration, but we still need to make the MSR writing
conditional on KVM_GET_MSR_INDEX_LIST. Older machine-types have
x-intel-pt-auto-level=off, and the user may set `level` manually.
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
@ 2019-10-15 13:29 ` Eduardo Habkost
0 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2019-10-15 13:29 UTC (permalink / raw)
To: Kang, Luwei; +Cc: kvm, mtosatti, qemu-devel, Chao Peng, pbonzini, rth
On Tue, Oct 15, 2019 at 12:51:48PM +0000, Kang, Luwei wrote:
> qemu> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c index
> > > f9f4cd1..097c953 100644
> > > --- a/target/i386/kvm.c
> > > +++ b/target/i386/kvm.c
> > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > }
> > > }
> > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > + 0x14, 1, R_EAX) &
> > > + 0x7;
> > > +
> > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > + env->msr_rtit_ctrl);
> > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > + env->msr_rtit_status);
> > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > + env->msr_rtit_output_base);
> >
> > This causes the following crash on some hosts:
> >
> > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> >
> > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has additional conditions that might prevent writing to this MSR
> > (PT_CAP_topa_output && PT_CAP_single_range_output). This causes QEMU to crash if some of the conditions aren't met.
> >
> > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> >
>
> Hi Eduardo,
> I found this issue can't be reproduced in upstream source code but can be reproduced on RHEL8.1. I haven't got the qemu source code of RHEL8.1. But after adding some trace in KVM, I found the KVM has reported the complete Intel PT CPUID information to qemu but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID to KVM (cpuid level is 0xd). It looks like lost the below patch.
>
> commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> Author: Luwei Kang <luwei.kang@intel.com>
> Date: Tue Jan 29 18:52:59 2019 -0500
> i386: extended the cpuid_level when Intel PT is enabled
>
> Intel Processor Trace required CPUID[0x14] but the cpuid_level
> have no change when create a kvm guest with
> e.g. "-cpu qemu64,+intel-pt".
Thanks for the pointer. This may avoid triggering the bug in the
default configuration, but we still need to make the MSR writing
conditional on KVM_GET_MSR_INDEX_LIST. Older machine-types have
x-intel-pt-auto-level=off, and the user may set `level` manually.
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
2019-10-15 13:29 ` Eduardo Habkost
@ 2019-10-21 6:02 ` Kang, Luwei
-1 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2019-10-21 6:02 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, kvm, pbonzini, rth, mtosatti, Chao Peng
> > > > f9f4cd1..097c953 100644
> > > > --- a/target/i386/kvm.c
> > > > +++ b/target/i386/kvm.c
> > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > }
> > > > }
> > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > + 0x14, 1,
> > > > + R_EAX) & 0x7;
> > > > +
> > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > + env->msr_rtit_ctrl);
> > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > + env->msr_rtit_status);
> > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > + env->msr_rtit_output_base);
> > >
> > > This causes the following crash on some hosts:
> > >
> > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > >
> > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > additional conditions that might prevent writing to this MSR (PT_CAP_topa_output && PT_CAP_single_range_output). This
> causes QEMU to crash if some of the conditions aren't met.
> > >
> > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > >
> >
> > Hi Eduardo,
> > I found this issue can't be reproduced in upstream source code but can be reproduced on RHEL8.1. I haven't got the qemu source
> code of RHEL8.1. But after adding some trace in KVM, I found the KVM has reported the complete Intel PT CPUID information to qemu
> but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID to KVM (cpuid level is 0xd). It looks like lost the below patch.
> >
> > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > Author: Luwei Kang <luwei.kang@intel.com>
> > Date: Tue Jan 29 18:52:59 2019 -0500
> > i386: extended the cpuid_level when Intel PT is enabled
> >
> > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > have no change when create a kvm guest with
> > e.g. "-cpu qemu64,+intel-pt".
>
> Thanks for the pointer. This may avoid triggering the bug in the default configuration, but we still need to make the MSR writing
> conditional on KVM_GET_MSR_INDEX_LIST. Older machine-types have x-intel-pt-auto-level=off, and the user may set `level`
> manually.
Hi Eduardo,
Sorry for a delay reply because my mail filter. I tried with the Q35 machine type and default, all looks work well (With some old cpu type + "intel_pt" also work well). KVM will check the Intel PT work mode and HW to decide if Intel PT can be exposed to guest, only extended the CPUID level is useless. If the guest doesn't support Intel PT, any MSR read or write will cause #GP. Please remind me if I lost something.
Thanks,
Luwei Kang
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
@ 2019-10-21 6:02 ` Kang, Luwei
0 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2019-10-21 6:02 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: kvm, mtosatti, qemu-devel, Chao Peng, pbonzini, rth
> > > > f9f4cd1..097c953 100644
> > > > --- a/target/i386/kvm.c
> > > > +++ b/target/i386/kvm.c
> > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > }
> > > > }
> > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > + 0x14, 1,
> > > > + R_EAX) & 0x7;
> > > > +
> > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > + env->msr_rtit_ctrl);
> > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > + env->msr_rtit_status);
> > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > + env->msr_rtit_output_base);
> > >
> > > This causes the following crash on some hosts:
> > >
> > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > >
> > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > additional conditions that might prevent writing to this MSR (PT_CAP_topa_output && PT_CAP_single_range_output). This
> causes QEMU to crash if some of the conditions aren't met.
> > >
> > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > >
> >
> > Hi Eduardo,
> > I found this issue can't be reproduced in upstream source code but can be reproduced on RHEL8.1. I haven't got the qemu source
> code of RHEL8.1. But after adding some trace in KVM, I found the KVM has reported the complete Intel PT CPUID information to qemu
> but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID to KVM (cpuid level is 0xd). It looks like lost the below patch.
> >
> > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > Author: Luwei Kang <luwei.kang@intel.com>
> > Date: Tue Jan 29 18:52:59 2019 -0500
> > i386: extended the cpuid_level when Intel PT is enabled
> >
> > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > have no change when create a kvm guest with
> > e.g. "-cpu qemu64,+intel-pt".
>
> Thanks for the pointer. This may avoid triggering the bug in the default configuration, but we still need to make the MSR writing
> conditional on KVM_GET_MSR_INDEX_LIST. Older machine-types have x-intel-pt-auto-level=off, and the user may set `level`
> manually.
Hi Eduardo,
Sorry for a delay reply because my mail filter. I tried with the Q35 machine type and default, all looks work well (With some old cpu type + "intel_pt" also work well). KVM will check the Intel PT work mode and HW to decide if Intel PT can be exposed to guest, only extended the CPUID level is useless. If the guest doesn't support Intel PT, any MSR read or write will cause #GP. Please remind me if I lost something.
Thanks,
Luwei Kang
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
2019-10-21 6:02 ` Kang, Luwei
@ 2019-10-22 21:44 ` Eduardo Habkost
-1 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2019-10-22 21:44 UTC (permalink / raw)
To: Kang, Luwei; +Cc: qemu-devel, kvm, pbonzini, rth, mtosatti, Chao Peng
On Mon, Oct 21, 2019 at 06:02:28AM +0000, Kang, Luwei wrote:
> > > > > f9f4cd1..097c953 100644
> > > > > --- a/target/i386/kvm.c
> > > > > +++ b/target/i386/kvm.c
> > > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > > }
> > > > > }
> > > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > > + 0x14, 1,
> > > > > + R_EAX) & 0x7;
> > > > > +
> > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > > + env->msr_rtit_ctrl);
> > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > > + env->msr_rtit_status);
> > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > > + env->msr_rtit_output_base);
> > > >
> > > > This causes the following crash on some hosts:
> > > >
> > > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > > >
> > > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > > additional conditions that might prevent writing to this MSR (PT_CAP_topa_output && PT_CAP_single_range_output). This
> > causes QEMU to crash if some of the conditions aren't met.
> > > >
> > > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > > >
> > >
> > > Hi Eduardo,
> > > I found this issue can't be reproduced in upstream source code but can be reproduced on RHEL8.1. I haven't got the qemu source
> > code of RHEL8.1. But after adding some trace in KVM, I found the KVM has reported the complete Intel PT CPUID information to qemu
> > but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID to KVM (cpuid level is 0xd). It looks like lost the below patch.
> > >
> > > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > > Author: Luwei Kang <luwei.kang@intel.com>
> > > Date: Tue Jan 29 18:52:59 2019 -0500
> > > i386: extended the cpuid_level when Intel PT is enabled
> > >
> > > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > > have no change when create a kvm guest with
> > > e.g. "-cpu qemu64,+intel-pt".
> >
> > Thanks for the pointer. This may avoid triggering the bug in the default configuration, but we still need to make the MSR writing
> > conditional on KVM_GET_MSR_INDEX_LIST. Older machine-types have x-intel-pt-auto-level=off, and the user may set `level`
> > manually.
>
> Hi Eduardo,
> Sorry for a delay reply because my mail filter. I tried with
> the Q35 machine type and default, all looks work well (With
> some old cpu type + "intel_pt" also work well). KVM will check
> the Intel PT work mode and HW to decide if Intel PT can be
> exposed to guest, only extended the CPUID level is useless. If
> the guest doesn't support Intel PT, any MSR read or write will
> cause #GP. Please remind me if I lost something.
I understand you have tried q35 and pc, but have you tried with
older machine-type versions?
Commit f24c3a79a415 doesn't change behavior on pc-*-3.1 and
older, so it only avoids triggering the crash in the default
case. Doesn't QEMU crash if running:
"-cpu qemu64,+intel-pt -machine pc-i440fx-3.1"?
KVM rejecting MSR writes when something is missing is correct.
QEMU trying to write the MSR when something is missing (and
crashing because of that) is a bug.
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
@ 2019-10-22 21:44 ` Eduardo Habkost
0 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2019-10-22 21:44 UTC (permalink / raw)
To: Kang, Luwei; +Cc: kvm, mtosatti, qemu-devel, Chao Peng, pbonzini, rth
On Mon, Oct 21, 2019 at 06:02:28AM +0000, Kang, Luwei wrote:
> > > > > f9f4cd1..097c953 100644
> > > > > --- a/target/i386/kvm.c
> > > > > +++ b/target/i386/kvm.c
> > > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > > }
> > > > > }
> > > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > > + 0x14, 1,
> > > > > + R_EAX) & 0x7;
> > > > > +
> > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > > + env->msr_rtit_ctrl);
> > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > > + env->msr_rtit_status);
> > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > > + env->msr_rtit_output_base);
> > > >
> > > > This causes the following crash on some hosts:
> > > >
> > > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > > >
> > > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > > additional conditions that might prevent writing to this MSR (PT_CAP_topa_output && PT_CAP_single_range_output). This
> > causes QEMU to crash if some of the conditions aren't met.
> > > >
> > > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > > >
> > >
> > > Hi Eduardo,
> > > I found this issue can't be reproduced in upstream source code but can be reproduced on RHEL8.1. I haven't got the qemu source
> > code of RHEL8.1. But after adding some trace in KVM, I found the KVM has reported the complete Intel PT CPUID information to qemu
> > but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID to KVM (cpuid level is 0xd). It looks like lost the below patch.
> > >
> > > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > > Author: Luwei Kang <luwei.kang@intel.com>
> > > Date: Tue Jan 29 18:52:59 2019 -0500
> > > i386: extended the cpuid_level when Intel PT is enabled
> > >
> > > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > > have no change when create a kvm guest with
> > > e.g. "-cpu qemu64,+intel-pt".
> >
> > Thanks for the pointer. This may avoid triggering the bug in the default configuration, but we still need to make the MSR writing
> > conditional on KVM_GET_MSR_INDEX_LIST. Older machine-types have x-intel-pt-auto-level=off, and the user may set `level`
> > manually.
>
> Hi Eduardo,
> Sorry for a delay reply because my mail filter. I tried with
> the Q35 machine type and default, all looks work well (With
> some old cpu type + "intel_pt" also work well). KVM will check
> the Intel PT work mode and HW to decide if Intel PT can be
> exposed to guest, only extended the CPUID level is useless. If
> the guest doesn't support Intel PT, any MSR read or write will
> cause #GP. Please remind me if I lost something.
I understand you have tried q35 and pc, but have you tried with
older machine-type versions?
Commit f24c3a79a415 doesn't change behavior on pc-*-3.1 and
older, so it only avoids triggering the crash in the default
case. Doesn't QEMU crash if running:
"-cpu qemu64,+intel-pt -machine pc-i440fx-3.1"?
KVM rejecting MSR writes when something is missing is correct.
QEMU trying to write the MSR when something is missing (and
crashing because of that) is a bug.
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
2019-10-22 21:44 ` Eduardo Habkost
@ 2019-10-24 11:22 ` Kang, Luwei
-1 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2019-10-24 11:22 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, kvm, pbonzini, rth, mtosatti, Chao Peng
> > > > > > f9f4cd1..097c953 100644
> > > > > > --- a/target/i386/kvm.c
> > > > > > +++ b/target/i386/kvm.c
> > > > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > > > }
> > > > > > }
> > > > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > > > + 0x14, 1,
> > > > > > + R_EAX) & 0x7;
> > > > > > +
> > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > > > + env->msr_rtit_ctrl);
> > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > > > + env->msr_rtit_status);
> > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > > > + env->msr_rtit_output_base);
> > > > >
> > > > > This causes the following crash on some hosts:
> > > > >
> > > > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > > > >
> > > > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > > > additional conditions that might prevent writing to this MSR
> > > > > (PT_CAP_topa_output && PT_CAP_single_range_output). This
> > > causes QEMU to crash if some of the conditions aren't met.
> > > > >
> > > > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > > > >
> > > >
> > > > Hi Eduardo,
> > > > I found this issue can't be reproduced in upstream source code
> > > > but can be reproduced on RHEL8.1. I haven't got the qemu source
> > > code of RHEL8.1. But after adding some trace in KVM, I found the KVM
> > > has reported the complete Intel PT CPUID information to qemu but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID
> to KVM (cpuid level is 0xd). It looks like lost the below patch.
> > > >
> > > > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > > > Author: Luwei Kang <luwei.kang@intel.com>
> > > > Date: Tue Jan 29 18:52:59 2019 -0500
> > > > i386: extended the cpuid_level when Intel PT is enabled
> > > >
> > > > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > > > have no change when create a kvm guest with
> > > > e.g. "-cpu qemu64,+intel-pt".
> > >
> > > Thanks for the pointer. This may avoid triggering the bug in the
> > > default configuration, but we still need to make the MSR writing
> > > conditional on KVM_GET_MSR_INDEX_LIST. Older machine-types have x-intel-pt-auto-level=off, and the user may set `level`
> manually.
> >
> > Hi Eduardo,
> > Sorry for a delay reply because my mail filter. I tried with the Q35
> > machine type and default, all looks work well (With some old cpu type
> > + "intel_pt" also work well). KVM will check the Intel PT work mode
> > and HW to decide if Intel PT can be exposed to guest, only extended
> > the CPUID level is useless. If the guest doesn't support Intel PT, any
> > MSR read or write will cause #GP. Please remind me if I lost
> > something.
>
> I understand you have tried q35 and pc, but have you tried with older machine-type versions?
>
> Commit f24c3a79a415 doesn't change behavior on pc-*-3.1 and older, so it only avoids triggering the crash in the default case.
> Doesn't QEMU crash if running:
> "-cpu qemu64,+intel-pt -machine pc-i440fx-3.1"?
>
> KVM rejecting MSR writes when something is missing is correct.
> QEMU trying to write the MSR when something is missing (and crashing because of that) is a bug.
Hi Eduardo,
Yes, you are right. Intel PT is only set in leaf 0x7.ebx but leaf 0x14 is lost because of the leaf number still 0xd (should 0x14).
May I remove the "off" like this?
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -132,7 +132,6 @@ GlobalProperty pc_compat_3_1[] = {
{ "Icelake-Client" "-" TYPE_X86_CPU, "mpx", "on" },
{ "Icelake-Server" "-" TYPE_X86_CPU, "mpx", "on" },
{ "Cascadelake-Server" "-" TYPE_X86_CPU, "stepping", "5" },
- { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" },
};
const size_t pc_compat_3_1_len = G_N_ELEMENTS(pc_compat_3_1);
Thanks,
Luwei Kang
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
@ 2019-10-24 11:22 ` Kang, Luwei
0 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2019-10-24 11:22 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: kvm, mtosatti, qemu-devel, Chao Peng, pbonzini, rth
> > > > > > f9f4cd1..097c953 100644
> > > > > > --- a/target/i386/kvm.c
> > > > > > +++ b/target/i386/kvm.c
> > > > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > > > }
> > > > > > }
> > > > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > > > + 0x14, 1,
> > > > > > + R_EAX) & 0x7;
> > > > > > +
> > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > > > + env->msr_rtit_ctrl);
> > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > > > + env->msr_rtit_status);
> > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > > > + env->msr_rtit_output_base);
> > > > >
> > > > > This causes the following crash on some hosts:
> > > > >
> > > > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > > > >
> > > > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > > > additional conditions that might prevent writing to this MSR
> > > > > (PT_CAP_topa_output && PT_CAP_single_range_output). This
> > > causes QEMU to crash if some of the conditions aren't met.
> > > > >
> > > > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > > > >
> > > >
> > > > Hi Eduardo,
> > > > I found this issue can't be reproduced in upstream source code
> > > > but can be reproduced on RHEL8.1. I haven't got the qemu source
> > > code of RHEL8.1. But after adding some trace in KVM, I found the KVM
> > > has reported the complete Intel PT CPUID information to qemu but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID
> to KVM (cpuid level is 0xd). It looks like lost the below patch.
> > > >
> > > > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > > > Author: Luwei Kang <luwei.kang@intel.com>
> > > > Date: Tue Jan 29 18:52:59 2019 -0500
> > > > i386: extended the cpuid_level when Intel PT is enabled
> > > >
> > > > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > > > have no change when create a kvm guest with
> > > > e.g. "-cpu qemu64,+intel-pt".
> > >
> > > Thanks for the pointer. This may avoid triggering the bug in the
> > > default configuration, but we still need to make the MSR writing
> > > conditional on KVM_GET_MSR_INDEX_LIST. Older machine-types have x-intel-pt-auto-level=off, and the user may set `level`
> manually.
> >
> > Hi Eduardo,
> > Sorry for a delay reply because my mail filter. I tried with the Q35
> > machine type and default, all looks work well (With some old cpu type
> > + "intel_pt" also work well). KVM will check the Intel PT work mode
> > and HW to decide if Intel PT can be exposed to guest, only extended
> > the CPUID level is useless. If the guest doesn't support Intel PT, any
> > MSR read or write will cause #GP. Please remind me if I lost
> > something.
>
> I understand you have tried q35 and pc, but have you tried with older machine-type versions?
>
> Commit f24c3a79a415 doesn't change behavior on pc-*-3.1 and older, so it only avoids triggering the crash in the default case.
> Doesn't QEMU crash if running:
> "-cpu qemu64,+intel-pt -machine pc-i440fx-3.1"?
>
> KVM rejecting MSR writes when something is missing is correct.
> QEMU trying to write the MSR when something is missing (and crashing because of that) is a bug.
Hi Eduardo,
Yes, you are right. Intel PT is only set in leaf 0x7.ebx but leaf 0x14 is lost because of the leaf number still 0xd (should 0x14).
May I remove the "off" like this?
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -132,7 +132,6 @@ GlobalProperty pc_compat_3_1[] = {
{ "Icelake-Client" "-" TYPE_X86_CPU, "mpx", "on" },
{ "Icelake-Server" "-" TYPE_X86_CPU, "mpx", "on" },
{ "Cascadelake-Server" "-" TYPE_X86_CPU, "stepping", "5" },
- { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" },
};
const size_t pc_compat_3_1_len = G_N_ELEMENTS(pc_compat_3_1);
Thanks,
Luwei Kang
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
2019-10-24 11:22 ` Kang, Luwei
@ 2019-10-24 13:24 ` Eduardo Habkost
-1 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2019-10-24 13:24 UTC (permalink / raw)
To: Kang, Luwei; +Cc: qemu-devel, kvm, pbonzini, rth, mtosatti, Chao Peng
On Thu, Oct 24, 2019 at 11:22:18AM +0000, Kang, Luwei wrote:
> > > > > > > f9f4cd1..097c953 100644
> > > > > > > --- a/target/i386/kvm.c
> > > > > > > +++ b/target/i386/kvm.c
> > > > > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > > > > }
> > > > > > > }
> > > > > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > > > > + 0x14, 1,
> > > > > > > + R_EAX) & 0x7;
> > > > > > > +
> > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > > > > + env->msr_rtit_ctrl);
> > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > > > > + env->msr_rtit_status);
> > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > > > > + env->msr_rtit_output_base);
> > > > > >
> > > > > > This causes the following crash on some hosts:
> > > > > >
> > > > > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > > > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > > > > >
> > > > > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > > > > additional conditions that might prevent writing to this MSR
> > > > > > (PT_CAP_topa_output && PT_CAP_single_range_output). This
> > > > causes QEMU to crash if some of the conditions aren't met.
> > > > > >
> > > > > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > > > > >
> > > > >
> > > > > Hi Eduardo,
> > > > > I found this issue can't be reproduced in upstream source code
> > > > > but can be reproduced on RHEL8.1. I haven't got the qemu source
> > > > code of RHEL8.1. But after adding some trace in KVM, I found the KVM
> > > > has reported the complete Intel PT CPUID information to qemu but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID
> > to KVM (cpuid level is 0xd). It looks like lost the below patch.
> > > > >
> > > > > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > > > > Author: Luwei Kang <luwei.kang@intel.com>
> > > > > Date: Tue Jan 29 18:52:59 2019 -0500
> > > > > i386: extended the cpuid_level when Intel PT is enabled
> > > > >
> > > > > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > > > > have no change when create a kvm guest with
> > > > > e.g. "-cpu qemu64,+intel-pt".
> > > >
> > > > Thanks for the pointer. This may avoid triggering the bug in the
> > > > default configuration, but we still need to make the MSR writing
> > > > conditional on KVM_GET_MSR_INDEX_LIST. Older machine-types have x-intel-pt-auto-level=off, and the user may set `level`
> > manually.
> > >
> > > Hi Eduardo,
> > > Sorry for a delay reply because my mail filter. I tried with the Q35
> > > machine type and default, all looks work well (With some old cpu type
> > > + "intel_pt" also work well). KVM will check the Intel PT work mode
> > > and HW to decide if Intel PT can be exposed to guest, only extended
> > > the CPUID level is useless. If the guest doesn't support Intel PT, any
> > > MSR read or write will cause #GP. Please remind me if I lost
> > > something.
> >
> > I understand you have tried q35 and pc, but have you tried with older machine-type versions?
> >
> > Commit f24c3a79a415 doesn't change behavior on pc-*-3.1 and older, so it only avoids triggering the crash in the default case.
> > Doesn't QEMU crash if running:
> > "-cpu qemu64,+intel-pt -machine pc-i440fx-3.1"?
> >
> > KVM rejecting MSR writes when something is missing is correct.
> > QEMU trying to write the MSR when something is missing (and crashing because of that) is a bug.
>
> Hi Eduardo,
> Yes, you are right. Intel PT is only set in leaf 0x7.ebx but leaf 0x14 is lost because of the leaf number still 0xd (should 0x14).
> May I remove the "off" like this?
We can't. This is necessary to keep guest ABI compatibility.
Instead, we need to make QEMU not crash if xlevel is too low,
because xlevel can be configured by the user.
>
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -132,7 +132,6 @@ GlobalProperty pc_compat_3_1[] = {
> { "Icelake-Client" "-" TYPE_X86_CPU, "mpx", "on" },
> { "Icelake-Server" "-" TYPE_X86_CPU, "mpx", "on" },
> { "Cascadelake-Server" "-" TYPE_X86_CPU, "stepping", "5" },
> - { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" },
> };
> const size_t pc_compat_3_1_len = G_N_ELEMENTS(pc_compat_3_1);
>
> Thanks,
> Luwei Kang
>
> >
> > --
> > Eduardo
>
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
@ 2019-10-24 13:24 ` Eduardo Habkost
0 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2019-10-24 13:24 UTC (permalink / raw)
To: Kang, Luwei; +Cc: kvm, mtosatti, qemu-devel, Chao Peng, pbonzini, rth
On Thu, Oct 24, 2019 at 11:22:18AM +0000, Kang, Luwei wrote:
> > > > > > > f9f4cd1..097c953 100644
> > > > > > > --- a/target/i386/kvm.c
> > > > > > > +++ b/target/i386/kvm.c
> > > > > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > > > > }
> > > > > > > }
> > > > > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > > > > + 0x14, 1,
> > > > > > > + R_EAX) & 0x7;
> > > > > > > +
> > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > > > > + env->msr_rtit_ctrl);
> > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > > > > + env->msr_rtit_status);
> > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > > > > + env->msr_rtit_output_base);
> > > > > >
> > > > > > This causes the following crash on some hosts:
> > > > > >
> > > > > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > > > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > > > > >
> > > > > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > > > > additional conditions that might prevent writing to this MSR
> > > > > > (PT_CAP_topa_output && PT_CAP_single_range_output). This
> > > > causes QEMU to crash if some of the conditions aren't met.
> > > > > >
> > > > > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > > > > >
> > > > >
> > > > > Hi Eduardo,
> > > > > I found this issue can't be reproduced in upstream source code
> > > > > but can be reproduced on RHEL8.1. I haven't got the qemu source
> > > > code of RHEL8.1. But after adding some trace in KVM, I found the KVM
> > > > has reported the complete Intel PT CPUID information to qemu but the Intel PT CPUID (0x14) is lost when qemu setting the CPUID
> > to KVM (cpuid level is 0xd). It looks like lost the below patch.
> > > > >
> > > > > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > > > > Author: Luwei Kang <luwei.kang@intel.com>
> > > > > Date: Tue Jan 29 18:52:59 2019 -0500
> > > > > i386: extended the cpuid_level when Intel PT is enabled
> > > > >
> > > > > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > > > > have no change when create a kvm guest with
> > > > > e.g. "-cpu qemu64,+intel-pt".
> > > >
> > > > Thanks for the pointer. This may avoid triggering the bug in the
> > > > default configuration, but we still need to make the MSR writing
> > > > conditional on KVM_GET_MSR_INDEX_LIST. Older machine-types have x-intel-pt-auto-level=off, and the user may set `level`
> > manually.
> > >
> > > Hi Eduardo,
> > > Sorry for a delay reply because my mail filter. I tried with the Q35
> > > machine type and default, all looks work well (With some old cpu type
> > > + "intel_pt" also work well). KVM will check the Intel PT work mode
> > > and HW to decide if Intel PT can be exposed to guest, only extended
> > > the CPUID level is useless. If the guest doesn't support Intel PT, any
> > > MSR read or write will cause #GP. Please remind me if I lost
> > > something.
> >
> > I understand you have tried q35 and pc, but have you tried with older machine-type versions?
> >
> > Commit f24c3a79a415 doesn't change behavior on pc-*-3.1 and older, so it only avoids triggering the crash in the default case.
> > Doesn't QEMU crash if running:
> > "-cpu qemu64,+intel-pt -machine pc-i440fx-3.1"?
> >
> > KVM rejecting MSR writes when something is missing is correct.
> > QEMU trying to write the MSR when something is missing (and crashing because of that) is a bug.
>
> Hi Eduardo,
> Yes, you are right. Intel PT is only set in leaf 0x7.ebx but leaf 0x14 is lost because of the leaf number still 0xd (should 0x14).
> May I remove the "off" like this?
We can't. This is necessary to keep guest ABI compatibility.
Instead, we need to make QEMU not crash if xlevel is too low,
because xlevel can be configured by the user.
>
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -132,7 +132,6 @@ GlobalProperty pc_compat_3_1[] = {
> { "Icelake-Client" "-" TYPE_X86_CPU, "mpx", "on" },
> { "Icelake-Server" "-" TYPE_X86_CPU, "mpx", "on" },
> { "Cascadelake-Server" "-" TYPE_X86_CPU, "stepping", "5" },
> - { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" },
> };
> const size_t pc_compat_3_1_len = G_N_ELEMENTS(pc_compat_3_1);
>
> Thanks,
> Luwei Kang
>
> >
> > --
> > Eduardo
>
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
2019-10-24 13:24 ` Eduardo Habkost
@ 2019-10-24 13:36 ` Kang, Luwei
-1 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2019-10-24 13:36 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: qemu-devel, kvm, pbonzini, rth, mtosatti, Chao Peng
> > > > > > > > f9f4cd1..097c953 100644
> > > > > > > > --- a/target/i386/kvm.c
> > > > > > > > +++ b/target/i386/kvm.c
> > > > > > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > > > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > > > > > }
> > > > > > > > }
> > > > > > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > > > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > > > > > + 0x14,
> > > > > > > > + 1,
> > > > > > > > + R_EAX) & 0x7;
> > > > > > > > +
> > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > > > > > + env->msr_rtit_ctrl);
> > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > > > > > + env->msr_rtit_status);
> > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > > > > > + env->msr_rtit_output_base);
> > > > > > >
> > > > > > > This causes the following crash on some hosts:
> > > > > > >
> > > > > > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > > > > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > > > > > >
> > > > > > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > > > > > additional conditions that might prevent writing to this MSR
> > > > > > > (PT_CAP_topa_output && PT_CAP_single_range_output). This
> > > > > causes QEMU to crash if some of the conditions aren't met.
> > > > > > >
> > > > > > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > > > > > >
> > > > > >
> > > > > > Hi Eduardo,
> > > > > > I found this issue can't be reproduced in upstream source
> > > > > > code but can be reproduced on RHEL8.1. I haven't got the qemu
> > > > > > source
> > > > > code of RHEL8.1. But after adding some trace in KVM, I found the
> > > > > KVM has reported the complete Intel PT CPUID information to qemu
> > > > > but the Intel PT CPUID (0x14) is lost when qemu setting the
> > > > > CPUID
> > > to KVM (cpuid level is 0xd). It looks like lost the below patch.
> > > > > >
> > > > > > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > > > > > Author: Luwei Kang <luwei.kang@intel.com>
> > > > > > Date: Tue Jan 29 18:52:59 2019 -0500
> > > > > > i386: extended the cpuid_level when Intel PT is enabled
> > > > > >
> > > > > > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > > > > > have no change when create a kvm guest with
> > > > > > e.g. "-cpu qemu64,+intel-pt".
> > > > >
> > > > > Thanks for the pointer. This may avoid triggering the bug in
> > > > > the default configuration, but we still need to make the MSR
> > > > > writing conditional on KVM_GET_MSR_INDEX_LIST. Older
> > > > > machine-types have x-intel-pt-auto-level=off, and the user may
> > > > > set `level`
> > > manually.
> > > >
> > > > Hi Eduardo,
> > > > Sorry for a delay reply because my mail filter. I tried with the
> > > > Q35 machine type and default, all looks work well (With some old
> > > > cpu type
> > > > + "intel_pt" also work well). KVM will check the Intel PT work
> > > > + mode
> > > > and HW to decide if Intel PT can be exposed to guest, only
> > > > extended the CPUID level is useless. If the guest doesn't support
> > > > Intel PT, any MSR read or write will cause #GP. Please remind me
> > > > if I lost something.
> > >
> > > I understand you have tried q35 and pc, but have you tried with older machine-type versions?
> > >
> > > Commit f24c3a79a415 doesn't change behavior on pc-*-3.1 and older, so it only avoids triggering the crash in the default case.
> > > Doesn't QEMU crash if running:
> > > "-cpu qemu64,+intel-pt -machine pc-i440fx-3.1"?
> > >
> > > KVM rejecting MSR writes when something is missing is correct.
> > > QEMU trying to write the MSR when something is missing (and crashing because of that) is a bug.
> >
> > Hi Eduardo,
> > Yes, you are right. Intel PT is only set in leaf 0x7.ebx but leaf 0x14 is lost because of the leaf number still 0xd (should 0x14).
> > May I remove the "off" like this?
>
> We can't. This is necessary to keep guest ABI compatibility.
> Instead, we need to make QEMU not crash if xlevel is too low, because xlevel can be configured by the user.
Thanks Eduardo. But I think it is a little complex for user. User found crash but how does he know it need to configure the xlevel or others?
If we want to the old machine type support PT can we add some code to extend the level to 0x14? Or old machine type can't support PT, mask off this feature from leaf 0x07.ebx[25] directly and output some messages?
Luwei Kang
>
> >
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -132,7 +132,6 @@ GlobalProperty pc_compat_3_1[] = {
> > { "Icelake-Client" "-" TYPE_X86_CPU, "mpx", "on" },
> > { "Icelake-Server" "-" TYPE_X86_CPU, "mpx", "on" },
> > { "Cascadelake-Server" "-" TYPE_X86_CPU, "stepping", "5" },
> > - { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" },
> > };
> > const size_t pc_compat_3_1_len = G_N_ELEMENTS(pc_compat_3_1);
> >
> > Thanks,
> > Luwei Kang
> >
> > >
> > > --
> > > Eduardo
> >
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* RE: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
@ 2019-10-24 13:36 ` Kang, Luwei
0 siblings, 0 replies; 30+ messages in thread
From: Kang, Luwei @ 2019-10-24 13:36 UTC (permalink / raw)
To: Eduardo Habkost; +Cc: kvm, mtosatti, qemu-devel, Chao Peng, pbonzini, rth
> > > > > > > > f9f4cd1..097c953 100644
> > > > > > > > --- a/target/i386/kvm.c
> > > > > > > > +++ b/target/i386/kvm.c
> > > > > > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > > > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > > > > > }
> > > > > > > > }
> > > > > > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > > > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > > > > > + 0x14,
> > > > > > > > + 1,
> > > > > > > > + R_EAX) & 0x7;
> > > > > > > > +
> > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > > > > > + env->msr_rtit_ctrl);
> > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > > > > > + env->msr_rtit_status);
> > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > > > > > + env->msr_rtit_output_base);
> > > > > > >
> > > > > > > This causes the following crash on some hosts:
> > > > > > >
> > > > > > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > > > > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > > > > > >
> > > > > > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > > > > > additional conditions that might prevent writing to this MSR
> > > > > > > (PT_CAP_topa_output && PT_CAP_single_range_output). This
> > > > > causes QEMU to crash if some of the conditions aren't met.
> > > > > > >
> > > > > > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > > > > > >
> > > > > >
> > > > > > Hi Eduardo,
> > > > > > I found this issue can't be reproduced in upstream source
> > > > > > code but can be reproduced on RHEL8.1. I haven't got the qemu
> > > > > > source
> > > > > code of RHEL8.1. But after adding some trace in KVM, I found the
> > > > > KVM has reported the complete Intel PT CPUID information to qemu
> > > > > but the Intel PT CPUID (0x14) is lost when qemu setting the
> > > > > CPUID
> > > to KVM (cpuid level is 0xd). It looks like lost the below patch.
> > > > > >
> > > > > > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > > > > > Author: Luwei Kang <luwei.kang@intel.com>
> > > > > > Date: Tue Jan 29 18:52:59 2019 -0500
> > > > > > i386: extended the cpuid_level when Intel PT is enabled
> > > > > >
> > > > > > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > > > > > have no change when create a kvm guest with
> > > > > > e.g. "-cpu qemu64,+intel-pt".
> > > > >
> > > > > Thanks for the pointer. This may avoid triggering the bug in
> > > > > the default configuration, but we still need to make the MSR
> > > > > writing conditional on KVM_GET_MSR_INDEX_LIST. Older
> > > > > machine-types have x-intel-pt-auto-level=off, and the user may
> > > > > set `level`
> > > manually.
> > > >
> > > > Hi Eduardo,
> > > > Sorry for a delay reply because my mail filter. I tried with the
> > > > Q35 machine type and default, all looks work well (With some old
> > > > cpu type
> > > > + "intel_pt" also work well). KVM will check the Intel PT work
> > > > + mode
> > > > and HW to decide if Intel PT can be exposed to guest, only
> > > > extended the CPUID level is useless. If the guest doesn't support
> > > > Intel PT, any MSR read or write will cause #GP. Please remind me
> > > > if I lost something.
> > >
> > > I understand you have tried q35 and pc, but have you tried with older machine-type versions?
> > >
> > > Commit f24c3a79a415 doesn't change behavior on pc-*-3.1 and older, so it only avoids triggering the crash in the default case.
> > > Doesn't QEMU crash if running:
> > > "-cpu qemu64,+intel-pt -machine pc-i440fx-3.1"?
> > >
> > > KVM rejecting MSR writes when something is missing is correct.
> > > QEMU trying to write the MSR when something is missing (and crashing because of that) is a bug.
> >
> > Hi Eduardo,
> > Yes, you are right. Intel PT is only set in leaf 0x7.ebx but leaf 0x14 is lost because of the leaf number still 0xd (should 0x14).
> > May I remove the "off" like this?
>
> We can't. This is necessary to keep guest ABI compatibility.
> Instead, we need to make QEMU not crash if xlevel is too low, because xlevel can be configured by the user.
Thanks Eduardo. But I think it is a little complex for user. User found crash but how does he know it need to configure the xlevel or others?
If we want to the old machine type support PT can we add some code to extend the level to 0x14? Or old machine type can't support PT, mask off this feature from leaf 0x07.ebx[25] directly and output some messages?
Luwei Kang
>
> >
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -132,7 +132,6 @@ GlobalProperty pc_compat_3_1[] = {
> > { "Icelake-Client" "-" TYPE_X86_CPU, "mpx", "on" },
> > { "Icelake-Server" "-" TYPE_X86_CPU, "mpx", "on" },
> > { "Cascadelake-Server" "-" TYPE_X86_CPU, "stepping", "5" },
> > - { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" },
> > };
> > const size_t pc_compat_3_1_len = G_N_ELEMENTS(pc_compat_3_1);
> >
> > Thanks,
> > Luwei Kang
> >
> > >
> > > --
> > > Eduardo
> >
>
> --
> Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
2019-10-24 13:36 ` Kang, Luwei
@ 2019-10-24 14:15 ` Eduardo Habkost
-1 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2019-10-24 14:15 UTC (permalink / raw)
To: Kang, Luwei; +Cc: qemu-devel, kvm, pbonzini, rth, mtosatti, Chao Peng
On Thu, Oct 24, 2019 at 01:36:50PM +0000, Kang, Luwei wrote:
> > > > > > > > > f9f4cd1..097c953 100644
> > > > > > > > > --- a/target/i386/kvm.c
> > > > > > > > > +++ b/target/i386/kvm.c
> > > > > > > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > > > > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > > > > > > }
> > > > > > > > > }
> > > > > > > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > > > > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > > > > > > + 0x14,
> > > > > > > > > + 1,
> > > > > > > > > + R_EAX) & 0x7;
> > > > > > > > > +
> > > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > > > > > > + env->msr_rtit_ctrl);
> > > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > > > > > > + env->msr_rtit_status);
> > > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > > > > > > + env->msr_rtit_output_base);
> > > > > > > >
> > > > > > > > This causes the following crash on some hosts:
> > > > > > > >
> > > > > > > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > > > > > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > > > > > > >
> > > > > > > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > > > > > > additional conditions that might prevent writing to this MSR
> > > > > > > > (PT_CAP_topa_output && PT_CAP_single_range_output). This
> > > > > > causes QEMU to crash if some of the conditions aren't met.
> > > > > > > >
> > > > > > > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > > > > > > >
> > > > > > >
> > > > > > > Hi Eduardo,
> > > > > > > I found this issue can't be reproduced in upstream source
> > > > > > > code but can be reproduced on RHEL8.1. I haven't got the qemu
> > > > > > > source
> > > > > > code of RHEL8.1. But after adding some trace in KVM, I found the
> > > > > > KVM has reported the complete Intel PT CPUID information to qemu
> > > > > > but the Intel PT CPUID (0x14) is lost when qemu setting the
> > > > > > CPUID
> > > > to KVM (cpuid level is 0xd). It looks like lost the below patch.
> > > > > > >
> > > > > > > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > > > > > > Author: Luwei Kang <luwei.kang@intel.com>
> > > > > > > Date: Tue Jan 29 18:52:59 2019 -0500
> > > > > > > i386: extended the cpuid_level when Intel PT is enabled
> > > > > > >
> > > > > > > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > > > > > > have no change when create a kvm guest with
> > > > > > > e.g. "-cpu qemu64,+intel-pt".
> > > > > >
> > > > > > Thanks for the pointer. This may avoid triggering the bug in
> > > > > > the default configuration, but we still need to make the MSR
> > > > > > writing conditional on KVM_GET_MSR_INDEX_LIST. Older
> > > > > > machine-types have x-intel-pt-auto-level=off, and the user may
> > > > > > set `level`
> > > > manually.
> > > > >
> > > > > Hi Eduardo,
> > > > > Sorry for a delay reply because my mail filter. I tried with the
> > > > > Q35 machine type and default, all looks work well (With some old
> > > > > cpu type
> > > > > + "intel_pt" also work well). KVM will check the Intel PT work
> > > > > + mode
> > > > > and HW to decide if Intel PT can be exposed to guest, only
> > > > > extended the CPUID level is useless. If the guest doesn't support
> > > > > Intel PT, any MSR read or write will cause #GP. Please remind me
> > > > > if I lost something.
> > > >
> > > > I understand you have tried q35 and pc, but have you tried with older machine-type versions?
> > > >
> > > > Commit f24c3a79a415 doesn't change behavior on pc-*-3.1 and older, so it only avoids triggering the crash in the default case.
> > > > Doesn't QEMU crash if running:
> > > > "-cpu qemu64,+intel-pt -machine pc-i440fx-3.1"?
> > > >
> > > > KVM rejecting MSR writes when something is missing is correct.
> > > > QEMU trying to write the MSR when something is missing (and crashing because of that) is a bug.
> > >
> > > Hi Eduardo,
> > > Yes, you are right. Intel PT is only set in leaf 0x7.ebx but leaf 0x14 is lost because of the leaf number still 0xd (should 0x14).
> > > May I remove the "off" like this?
> >
> > We can't. This is necessary to keep guest ABI compatibility.
> > Instead, we need to make QEMU not crash if xlevel is too low, because xlevel can be configured by the user.
>
> Thanks Eduardo. But I think it is a little complex for user.
> User found crash but how does he know it need to configure the
> xlevel or others?
> If we want to the old machine type support PT can we add some
> code to extend the level to 0x14? Or old machine type can't
> support PT, mask off this feature from leaf 0x07.ebx[25]
> directly and output some messages?
I agree it's complex for the user, but let's address this
separately:
the first issue here is the crash: QEMU must not crash if using
(e.g.) "-cpu ...,+intel-pt,xlevel=0x13". This can't be solved by
making any machine-type changes.
The second issue is usability. This is hard to fix on old
machine-types because we must keep guest ABI compatibility.
In QEMU 3.1 the results of:
-machine pc-i440fx-3.1 -cpu qemu64,+intel-pt
was:
CPUID[0].EAX (level) = 7
CPUID[7].EBX[25] (intel-pt) = 1
and we can't change the behavior of pc-i440fx-3.1.
Your suggestion of printing a warning is good, though. We can do
that if intel-pt is enabled and level < 0x14.
>
> Luwei Kang
>
> >
> > >
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -132,7 +132,6 @@ GlobalProperty pc_compat_3_1[] = {
> > > { "Icelake-Client" "-" TYPE_X86_CPU, "mpx", "on" },
> > > { "Icelake-Server" "-" TYPE_X86_CPU, "mpx", "on" },
> > > { "Cascadelake-Server" "-" TYPE_X86_CPU, "stepping", "5" },
> > > - { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" },
> > > };
> > > const size_t pc_compat_3_1_len = G_N_ELEMENTS(pc_compat_3_1);
> > >
> > > Thanks,
> > > Luwei Kang
> > >
> > > >
> > > > --
> > > > Eduardo
> > >
> >
> > --
> > Eduardo
>
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature
@ 2019-10-24 14:15 ` Eduardo Habkost
0 siblings, 0 replies; 30+ messages in thread
From: Eduardo Habkost @ 2019-10-24 14:15 UTC (permalink / raw)
To: Kang, Luwei; +Cc: kvm, mtosatti, qemu-devel, Chao Peng, pbonzini, rth
On Thu, Oct 24, 2019 at 01:36:50PM +0000, Kang, Luwei wrote:
> > > > > > > > > f9f4cd1..097c953 100644
> > > > > > > > > --- a/target/i386/kvm.c
> > > > > > > > > +++ b/target/i386/kvm.c
> > > > > > > > > @@ -1811,6 +1811,25 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
> > > > > > > > > kvm_msr_entry_add(cpu, MSR_MTRRphysMask(i), mask);
> > > > > > > > > }
> > > > > > > > > }
> > > > > > > > > + if (env->features[FEAT_7_0_EBX] & CPUID_7_0_EBX_INTEL_PT) {
> > > > > > > > > + int addr_num = kvm_arch_get_supported_cpuid(kvm_state,
> > > > > > > > > + 0x14,
> > > > > > > > > + 1,
> > > > > > > > > + R_EAX) & 0x7;
> > > > > > > > > +
> > > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_CTL,
> > > > > > > > > + env->msr_rtit_ctrl);
> > > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_STATUS,
> > > > > > > > > + env->msr_rtit_status);
> > > > > > > > > + kvm_msr_entry_add(cpu, MSR_IA32_RTIT_OUTPUT_BASE,
> > > > > > > > > + env->msr_rtit_output_base);
> > > > > > > >
> > > > > > > > This causes the following crash on some hosts:
> > > > > > > >
> > > > > > > > qemu-system-x86_64: error: failed to set MSR 0x560 to 0x0
> > > > > > > > qemu-system-x86_64: target/i386/kvm.c:2673: kvm_put_msrs: Assertion `ret == cpu->kvm_msr_buf->nmsrs' failed.
> > > > > > > >
> > > > > > > > Checking for CPUID_7_0_EBX_INTEL_PT is not enough: KVM has
> > > > > > > > additional conditions that might prevent writing to this MSR
> > > > > > > > (PT_CAP_topa_output && PT_CAP_single_range_output). This
> > > > > > causes QEMU to crash if some of the conditions aren't met.
> > > > > > > >
> > > > > > > > Writing and reading this MSR (and the ones below) need to be conditional on KVM_GET_MSR_INDEX_LIST.
> > > > > > > >
> > > > > > >
> > > > > > > Hi Eduardo,
> > > > > > > I found this issue can't be reproduced in upstream source
> > > > > > > code but can be reproduced on RHEL8.1. I haven't got the qemu
> > > > > > > source
> > > > > > code of RHEL8.1. But after adding some trace in KVM, I found the
> > > > > > KVM has reported the complete Intel PT CPUID information to qemu
> > > > > > but the Intel PT CPUID (0x14) is lost when qemu setting the
> > > > > > CPUID
> > > > to KVM (cpuid level is 0xd). It looks like lost the below patch.
> > > > > > >
> > > > > > > commit f24c3a79a415042f6dc195f029a2ba7247d14cac
> > > > > > > Author: Luwei Kang <luwei.kang@intel.com>
> > > > > > > Date: Tue Jan 29 18:52:59 2019 -0500
> > > > > > > i386: extended the cpuid_level when Intel PT is enabled
> > > > > > >
> > > > > > > Intel Processor Trace required CPUID[0x14] but the cpuid_level
> > > > > > > have no change when create a kvm guest with
> > > > > > > e.g. "-cpu qemu64,+intel-pt".
> > > > > >
> > > > > > Thanks for the pointer. This may avoid triggering the bug in
> > > > > > the default configuration, but we still need to make the MSR
> > > > > > writing conditional on KVM_GET_MSR_INDEX_LIST. Older
> > > > > > machine-types have x-intel-pt-auto-level=off, and the user may
> > > > > > set `level`
> > > > manually.
> > > > >
> > > > > Hi Eduardo,
> > > > > Sorry for a delay reply because my mail filter. I tried with the
> > > > > Q35 machine type and default, all looks work well (With some old
> > > > > cpu type
> > > > > + "intel_pt" also work well). KVM will check the Intel PT work
> > > > > + mode
> > > > > and HW to decide if Intel PT can be exposed to guest, only
> > > > > extended the CPUID level is useless. If the guest doesn't support
> > > > > Intel PT, any MSR read or write will cause #GP. Please remind me
> > > > > if I lost something.
> > > >
> > > > I understand you have tried q35 and pc, but have you tried with older machine-type versions?
> > > >
> > > > Commit f24c3a79a415 doesn't change behavior on pc-*-3.1 and older, so it only avoids triggering the crash in the default case.
> > > > Doesn't QEMU crash if running:
> > > > "-cpu qemu64,+intel-pt -machine pc-i440fx-3.1"?
> > > >
> > > > KVM rejecting MSR writes when something is missing is correct.
> > > > QEMU trying to write the MSR when something is missing (and crashing because of that) is a bug.
> > >
> > > Hi Eduardo,
> > > Yes, you are right. Intel PT is only set in leaf 0x7.ebx but leaf 0x14 is lost because of the leaf number still 0xd (should 0x14).
> > > May I remove the "off" like this?
> >
> > We can't. This is necessary to keep guest ABI compatibility.
> > Instead, we need to make QEMU not crash if xlevel is too low, because xlevel can be configured by the user.
>
> Thanks Eduardo. But I think it is a little complex for user.
> User found crash but how does he know it need to configure the
> xlevel or others?
> If we want to the old machine type support PT can we add some
> code to extend the level to 0x14? Or old machine type can't
> support PT, mask off this feature from leaf 0x07.ebx[25]
> directly and output some messages?
I agree it's complex for the user, but let's address this
separately:
the first issue here is the crash: QEMU must not crash if using
(e.g.) "-cpu ...,+intel-pt,xlevel=0x13". This can't be solved by
making any machine-type changes.
The second issue is usability. This is hard to fix on old
machine-types because we must keep guest ABI compatibility.
In QEMU 3.1 the results of:
-machine pc-i440fx-3.1 -cpu qemu64,+intel-pt
was:
CPUID[0].EAX (level) = 7
CPUID[7].EBX[25] (intel-pt) = 1
and we can't change the behavior of pc-i440fx-3.1.
Your suggestion of printing a warning is good, though. We can do
that if intel-pt is enabled and level < 0x14.
>
> Luwei Kang
>
> >
> > >
> > > --- a/hw/i386/pc.c
> > > +++ b/hw/i386/pc.c
> > > @@ -132,7 +132,6 @@ GlobalProperty pc_compat_3_1[] = {
> > > { "Icelake-Client" "-" TYPE_X86_CPU, "mpx", "on" },
> > > { "Icelake-Server" "-" TYPE_X86_CPU, "mpx", "on" },
> > > { "Cascadelake-Server" "-" TYPE_X86_CPU, "stepping", "5" },
> > > - { TYPE_X86_CPU, "x-intel-pt-auto-level", "off" },
> > > };
> > > const size_t pc_compat_3_1_len = G_N_ELEMENTS(pc_compat_3_1);
> > >
> > > Thanks,
> > > Luwei Kang
> > >
> > > >
> > > > --
> > > > Eduardo
> > >
> >
> > --
> > Eduardo
>
--
Eduardo
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2019-10-24 15:22 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-04 16:48 [PATCH v4 1/2] i386: Add Intel Processor Trace feature support Luwei Kang
2018-03-04 16:48 ` [Qemu-devel] " Luwei Kang
2018-03-04 16:48 ` [PATCH v4 2/2] i386: Add support to get/set/migrate Intel Processor Trace feature Luwei Kang
2018-03-04 16:48 ` [Qemu-devel] " Luwei Kang
2019-10-12 3:14 ` Eduardo Habkost
2019-10-12 3:14 ` Eduardo Habkost
2019-10-15 12:51 ` Kang, Luwei
2019-10-15 12:51 ` Kang, Luwei
2019-10-15 13:29 ` Eduardo Habkost
2019-10-15 13:29 ` Eduardo Habkost
2019-10-21 6:02 ` Kang, Luwei
2019-10-21 6:02 ` Kang, Luwei
2019-10-22 21:44 ` Eduardo Habkost
2019-10-22 21:44 ` Eduardo Habkost
2019-10-24 11:22 ` Kang, Luwei
2019-10-24 11:22 ` Kang, Luwei
2019-10-24 13:24 ` Eduardo Habkost
2019-10-24 13:24 ` Eduardo Habkost
2019-10-24 13:36 ` Kang, Luwei
2019-10-24 13:36 ` Kang, Luwei
2019-10-24 14:15 ` Eduardo Habkost
2019-10-24 14:15 ` Eduardo Habkost
2018-03-09 19:10 ` [PATCH v4 1/2] i386: Add Intel Processor Trace feature support Eduardo Habkost
2018-03-09 19:10 ` [Qemu-devel] " Eduardo Habkost
2018-03-12 9:07 ` Kang, Luwei
2018-03-12 9:07 ` [Qemu-devel] " Kang, Luwei
2018-03-12 16:45 ` Eduardo Habkost
2018-03-12 16:45 ` [Qemu-devel] " Eduardo Habkost
2018-03-13 11:16 ` Kang, Luwei
2018-03-13 11:16 ` [Qemu-devel] " Kang, Luwei
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.