* [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor
@ 2016-09-23 19:45 Eduardo Habkost
2016-09-23 19:45 ` [Qemu-devel] [PATCH 1/7] target-i386: Move feature name arrays inside FeatureWordInfo Eduardo Habkost
` (7 more replies)
0 siblings, 8 replies; 17+ messages in thread
From: Eduardo Habkost @ 2016-09-23 19:45 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, Igor Mammedov
This series refactor the xsave CPUID handling so it won't
silently disable any XSAVE components on CPUID[0xD] in case the
host doesn't support it. It will instead use the exisitng
check/enforce logic for filtering the CPUID bits and checking for
host-side support.
This series is available on git at:
https://github.com/ehabkost/qemu-hacks.git work/xsave-cpuid-cleanup
The series is based on my x86-next branch, that contains other
CPUID-related changes:
https://github.com/ehabkost/qemu.git x8-next
Eduardo Habkost (7):
target-i386: Move feature name arrays inside FeatureWordInfo
target-i386: Don't try to enable PT State xsave component
target-i386: xsave: Calculate enabled components only once
target-i386: xsave: Simplify CPUID[0xD,0].{EAX,EDX} calculation
target-i386: xsave: Helper function to calculate xsave area size
target-i386: xsave: Calculate set of xsave components on realize
target-i386: Move xsave component mask to features array
target-i386/cpu.c | 457 +++++++++++++++++++++++++++---------------------------
target-i386/cpu.h | 2 +
2 files changed, 230 insertions(+), 229 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH 1/7] target-i386: Move feature name arrays inside FeatureWordInfo 2016-09-23 19:45 [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost @ 2016-09-23 19:45 ` Eduardo Habkost 2016-09-23 20:01 ` Richard Henderson 2016-09-23 19:45 ` [Qemu-devel] [PATCH 2/7] target-i386: Don't try to enable PT State xsave component Eduardo Habkost ` (6 subsequent siblings) 7 siblings, 1 reply; 17+ messages in thread From: Eduardo Habkost @ 2016-09-23 19:45 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, Igor Mammedov It makes it easier to guarantee the arrays are the right size, and to find information when looking at the code. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 370 +++++++++++++++++++++++++----------------------------- 1 file changed, 170 insertions(+), 200 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index a5d3b1a..cc07fdb 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -181,185 +181,6 @@ static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1, dst[CPUID_VENDOR_SZ] = '\0'; } -/* feature flags taken from "Intel Processor Identification and the CPUID - * Instruction" and AMD's "CPUID Specification". In cases of disagreement - * between feature naming conventions, aliases may be added. - */ -static const char *feature_name[] = { - "fpu", "vme", "de", "pse", - "tsc", "msr", "pae", "mce", - "cx8", "apic", NULL, "sep", - "mtrr", "pge", "mca", "cmov", - "pat", "pse36", "pn" /* Intel psn */, "clflush" /* Intel clfsh */, - NULL, "ds" /* Intel dts */, "acpi", "mmx", - "fxsr", "sse", "sse2", "ss", - "ht" /* Intel htt */, "tm", "ia64", "pbe", -}; -static const char *ext_feature_name[] = { - "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", "monitor", - "ds_cpl", "vmx", "smx", "est", - "tm2", "ssse3", "cid", NULL, - "fma", "cx16", "xtpr", "pdcm", - NULL, "pcid", "dca", "sse4.1|sse4_1", - "sse4.2|sse4_2", "x2apic", "movbe", "popcnt", - "tsc-deadline", "aes", "xsave", "osxsave", - "avx", "f16c", "rdrand", "hypervisor", -}; -/* Feature names that are already defined on feature_name[] but are set on - * CPUID[8000_0001].EDX on AMD CPUs don't have their names on - * ext2_feature_name[]. They are copied automatically to cpuid_ext2_features - * if and only if CPU vendor is AMD. - */ -static const char *ext2_feature_name[] = { - NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */, - NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */, - NULL /* cx8 */ /* AMD CMPXCHG8B */, NULL /* apic */, NULL, "syscall", - NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */, - NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */, - "nx|xd", NULL, "mmxext", NULL /* mmx */, - NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp", - NULL, "lm|i64", "3dnowext", "3dnow", -}; -static const char *ext3_feature_name[] = { - "lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD ExtApicSpace */, - "cr8legacy" /* AMD AltMovCr8 */, "abm", "sse4a", "misalignsse", - "3dnowprefetch", "osvw", "ibs", "xop", - "skinit", "wdt", NULL, "lwp", - "fma4", "tce", NULL, "nodeid_msr", - NULL, "tbm", "topoext", "perfctr_core", - "perfctr_nb", NULL, NULL, NULL, - NULL, NULL, NULL, NULL, -}; - -static const char *ext4_feature_name[] = { - NULL, NULL, "xstore", "xstore-en", - NULL, NULL, "xcrypt", "xcrypt-en", - "ace2", "ace2-en", "phe", "phe-en", - "pmm", "pmm-en", NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, -}; - -static const char *kvm_feature_name[] = { - "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", - "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt", - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - "kvmclock-stable-bit", NULL, NULL, NULL, - NULL, NULL, NULL, NULL, -}; - -static const char *hyperv_priv_feature_name[] = { - NULL /* hv_msr_vp_runtime_access */, NULL /* hv_msr_time_refcount_access */, - NULL /* hv_msr_synic_access */, NULL /* hv_msr_stimer_access */, - NULL /* hv_msr_apic_access */, NULL /* hv_msr_hypercall_access */, - NULL /* hv_vpindex_access */, NULL /* hv_msr_reset_access */, - NULL /* hv_msr_stats_access */, NULL /* hv_reftsc_access */, - NULL /* hv_msr_idle_access */, NULL /* hv_msr_frequency_access */, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, -}; - -static const char *hyperv_ident_feature_name[] = { - NULL /* hv_create_partitions */, NULL /* hv_access_partition_id */, - NULL /* hv_access_memory_pool */, NULL /* hv_adjust_message_buffers */, - NULL /* hv_post_messages */, NULL /* hv_signal_events */, - NULL /* hv_create_port */, NULL /* hv_connect_port */, - NULL /* hv_access_stats */, NULL, NULL, NULL /* hv_debugging */, - NULL /* hv_cpu_power_management */, NULL /* hv_configure_profiler */, - NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, -}; - -static const char *hyperv_misc_feature_name[] = { - NULL /* hv_mwait */, NULL /* hv_guest_debugging */, - NULL /* hv_perf_monitor */, NULL /* hv_cpu_dynamic_part */, - NULL /* hv_hypercall_params_xmm */, NULL /* hv_guest_idle_state */, - NULL, NULL, - NULL, NULL, NULL /* hv_guest_crash_msr */, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, -}; - -static const char *svm_feature_name[] = { - "npt", "lbrv", "svm_lock", "nrip_save", - "tsc_scale", "vmcb_clean", "flushbyasid", "decodeassists", - NULL, NULL, "pause_filter", NULL, - "pfthreshold", NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, -}; - -static const char *cpuid_7_0_ebx_feature_name[] = { - "fsgsbase", "tsc_adjust", NULL, "bmi1", - "hle", "avx2", NULL, "smep", - "bmi2", "erms", "invpcid", "rtm", - NULL, NULL, "mpx", NULL, - "avx512f", "avx512dq", "rdseed", "adx", - "smap", "avx512ifma", "pcommit", "clflushopt", - "clwb", NULL, "avx512pf", "avx512er", - "avx512cd", NULL, "avx512bw", "avx512vl", -}; - -static const char *cpuid_7_0_ecx_feature_name[] = { - NULL, "avx512vbmi", "umip", "pku", - "ospke", NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, "rdpid", NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, -}; - -static const char *cpuid_apm_edx_feature_name[] = { - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - "invtsc", NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, -}; - -static const char *cpuid_xsave_feature_name[] = { - "xsaveopt", "xsavec", "xgetbv1", "xsaves", - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, -}; - -static const char *cpuid_6_feature_name[] = { - NULL, NULL, "arat", NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, - NULL, NULL, NULL, NULL, -}; - #define I486_FEATURES (CPUID_FP87 | CPUID_VME | CPUID_PSE) #define PENTIUM_FEATURES (I486_FEATURES | CPUID_DE | CPUID_TSC | \ CPUID_MSR | CPUID_MCE | CPUID_CX8 | CPUID_MMX | CPUID_APIC) @@ -425,7 +246,12 @@ static const char *cpuid_6_feature_name[] = { CPUID_XSAVE_XSAVEC, CPUID_XSAVE_XSAVES */ typedef struct FeatureWordInfo { - const char **feat_names; + /* feature flags names are taken from "Intel Processor Identification and + * the CPUID Instruction" and AMD's "CPUID Specification". + * In cases of disagreement between feature naming conventions, + * aliases may be added. + */ + const char *feat_names[32]; uint32_t cpuid_eax; /* Input EAX for CPUID */ bool cpuid_needs_ecx; /* CPUID instruction uses ECX as input */ uint32_t cpuid_ecx; /* Input ECX value for CPUID */ @@ -436,82 +262,230 @@ typedef struct FeatureWordInfo { static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { [FEAT_1_EDX] = { - .feat_names = feature_name, + .feat_names = { + "fpu", "vme", "de", "pse", + "tsc", "msr", "pae", "mce", + "cx8", "apic", NULL, "sep", + "mtrr", "pge", "mca", "cmov", + "pat", "pse36", "pn" /* Intel psn */, "clflush" /* Intel clfsh */, + NULL, "ds" /* Intel dts */, "acpi", "mmx", + "fxsr", "sse", "sse2", "ss", + "ht" /* Intel htt */, "tm", "ia64", "pbe", + }, .cpuid_eax = 1, .cpuid_reg = R_EDX, .tcg_features = TCG_FEATURES, }, [FEAT_1_ECX] = { - .feat_names = ext_feature_name, + .feat_names = { + "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", "monitor", + "ds_cpl", "vmx", "smx", "est", + "tm2", "ssse3", "cid", NULL, + "fma", "cx16", "xtpr", "pdcm", + NULL, "pcid", "dca", "sse4.1|sse4_1", + "sse4.2|sse4_2", "x2apic", "movbe", "popcnt", + "tsc-deadline", "aes", "xsave", "osxsave", + "avx", "f16c", "rdrand", "hypervisor", + }, .cpuid_eax = 1, .cpuid_reg = R_ECX, .tcg_features = TCG_EXT_FEATURES, }, + /* Feature names that are already defined on feature_name[] but + * are set on CPUID[8000_0001].EDX on AMD CPUs don't have their + * names on feat_names below. They are copied automatically + * to features[FEAT_8000_0001_EDX] if and only if CPU vendor is AMD. + */ [FEAT_8000_0001_EDX] = { - .feat_names = ext2_feature_name, + .feat_names = { + NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */, + NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */, + NULL /* cx8 */, NULL /* apic */, NULL, "syscall", + NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */, + NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */, + "nx|xd", NULL, "mmxext", NULL /* mmx */, + NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb", "rdtscp", + NULL, "lm|i64", "3dnowext", "3dnow", + }, .cpuid_eax = 0x80000001, .cpuid_reg = R_EDX, .tcg_features = TCG_EXT2_FEATURES, }, [FEAT_8000_0001_ECX] = { - .feat_names = ext3_feature_name, + .feat_names = { + "lahf_lm", "cmp_legacy", "svm", "extapic", + "cr8legacy", "abm", "sse4a", "misalignsse", + "3dnowprefetch", "osvw", "ibs", "xop", + "skinit", "wdt", NULL, "lwp", + "fma4", "tce", NULL, "nodeid_msr", + NULL, "tbm", "topoext", "perfctr_core", + "perfctr_nb", NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, .cpuid_eax = 0x80000001, .cpuid_reg = R_ECX, .tcg_features = TCG_EXT3_FEATURES, }, [FEAT_C000_0001_EDX] = { - .feat_names = ext4_feature_name, + .feat_names = { + NULL, NULL, "xstore", "xstore-en", + NULL, NULL, "xcrypt", "xcrypt-en", + "ace2", "ace2-en", "phe", "phe-en", + "pmm", "pmm-en", NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, .cpuid_eax = 0xC0000001, .cpuid_reg = R_EDX, .tcg_features = TCG_EXT4_FEATURES, }, [FEAT_KVM] = { - .feat_names = kvm_feature_name, + .feat_names = { + "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", + "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt", + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + "kvmclock-stable-bit", NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EAX, .tcg_features = TCG_KVM_FEATURES, }, [FEAT_HYPERV_EAX] = { - .feat_names = hyperv_priv_feature_name, + .feat_names = { + NULL /* hv_msr_vp_runtime_access */, NULL /* hv_msr_time_refcount_access */, + NULL /* hv_msr_synic_access */, NULL /* hv_msr_stimer_access */, + NULL /* hv_msr_apic_access */, NULL /* hv_msr_hypercall_access */, + NULL /* hv_vpindex_access */, NULL /* hv_msr_reset_access */, + NULL /* hv_msr_stats_access */, NULL /* hv_reftsc_access */, + NULL /* hv_msr_idle_access */, NULL /* hv_msr_frequency_access */, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, .cpuid_eax = 0x40000003, .cpuid_reg = R_EAX, }, [FEAT_HYPERV_EBX] = { - .feat_names = hyperv_ident_feature_name, + .feat_names = { + NULL /* hv_create_partitions */, NULL /* hv_access_partition_id */, + NULL /* hv_access_memory_pool */, NULL /* hv_adjust_message_buffers */, + NULL /* hv_post_messages */, NULL /* hv_signal_events */, + NULL /* hv_create_port */, NULL /* hv_connect_port */, + NULL /* hv_access_stats */, NULL, NULL, NULL /* hv_debugging */, + NULL /* hv_cpu_power_management */, NULL /* hv_configure_profiler */, + NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, .cpuid_eax = 0x40000003, .cpuid_reg = R_EBX, }, [FEAT_HYPERV_EDX] = { - .feat_names = hyperv_misc_feature_name, + .feat_names = { + NULL /* hv_mwait */, NULL /* hv_guest_debugging */, + NULL /* hv_perf_monitor */, NULL /* hv_cpu_dynamic_part */, + NULL /* hv_hypercall_params_xmm */, NULL /* hv_guest_idle_state */, + NULL, NULL, + NULL, NULL, NULL /* hv_guest_crash_msr */, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, .cpuid_eax = 0x40000003, .cpuid_reg = R_EDX, }, [FEAT_SVM] = { - .feat_names = svm_feature_name, + .feat_names = { + "npt", "lbrv", "svm_lock", "nrip_save", + "tsc_scale", "vmcb_clean", "flushbyasid", "decodeassists", + NULL, NULL, "pause_filter", NULL, + "pfthreshold", NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, .cpuid_eax = 0x8000000A, .cpuid_reg = R_EDX, .tcg_features = TCG_SVM_FEATURES, }, [FEAT_7_0_EBX] = { - .feat_names = cpuid_7_0_ebx_feature_name, + .feat_names = { + "fsgsbase", "tsc_adjust", NULL, "bmi1", + "hle", "avx2", NULL, "smep", + "bmi2", "erms", "invpcid", "rtm", + NULL, NULL, "mpx", NULL, + "avx512f", "avx512dq", "rdseed", "adx", + "smap", "avx512ifma", "pcommit", "clflushopt", + "clwb", NULL, "avx512pf", "avx512er", + "avx512cd", NULL, "avx512bw", "avx512vl", + }, .cpuid_eax = 7, .cpuid_needs_ecx = true, .cpuid_ecx = 0, .cpuid_reg = R_EBX, .tcg_features = TCG_7_0_EBX_FEATURES, }, [FEAT_7_0_ECX] = { - .feat_names = cpuid_7_0_ecx_feature_name, + .feat_names = { + NULL, "avx512vbmi", "umip", "pku", + "ospke", NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, "rdpid", NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, .cpuid_eax = 7, .cpuid_needs_ecx = true, .cpuid_ecx = 0, .cpuid_reg = R_ECX, .tcg_features = TCG_7_0_ECX_FEATURES, }, [FEAT_8000_0007_EDX] = { - .feat_names = cpuid_apm_edx_feature_name, + .feat_names = { + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + "invtsc", NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, .cpuid_eax = 0x80000007, .cpuid_reg = R_EDX, .tcg_features = TCG_APM_FEATURES, .unmigratable_flags = CPUID_APM_INVTSC, }, [FEAT_XSAVE] = { - .feat_names = cpuid_xsave_feature_name, + .feat_names = { + "xsaveopt", "xsavec", "xgetbv1", "xsaves", + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, .cpuid_eax = 0xd, .cpuid_needs_ecx = true, .cpuid_ecx = 1, .cpuid_reg = R_EAX, .tcg_features = TCG_XSAVE_FEATURES, }, [FEAT_6_EAX] = { - .feat_names = cpuid_6_feature_name, + .feat_names = { + NULL, NULL, "arat", NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + NULL, NULL, NULL, NULL, + }, .cpuid_eax = 6, .cpuid_reg = R_EAX, .tcg_features = TCG_6_EAX_FEATURES, }, @@ -711,8 +685,7 @@ static void add_flagname_to_bitmaps(const char *flagname, FeatureWord w; for (w = 0; w < FEATURE_WORDS; w++) { FeatureWordInfo *wi = &feature_word_info[w]; - if (wi->feat_names && - lookup_feature(&words[w], flagname, NULL, wi->feat_names)) { + if (lookup_feature(&words[w], flagname, NULL, wi->feat_names)) { break; } } @@ -3324,9 +3297,6 @@ static void x86_cpu_register_feature_bit_props(X86CPU *cpu, char **names; FeatureWordInfo *fi = &feature_word_info[w]; - if (!fi->feat_names) { - return; - } if (!fi->feat_names[bitnr]) { return; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/7] target-i386: Move feature name arrays inside FeatureWordInfo 2016-09-23 19:45 ` [Qemu-devel] [PATCH 1/7] target-i386: Move feature name arrays inside FeatureWordInfo Eduardo Habkost @ 2016-09-23 20:01 ` Richard Henderson 0 siblings, 0 replies; 17+ messages in thread From: Richard Henderson @ 2016-09-23 20:01 UTC (permalink / raw) To: Eduardo Habkost, qemu-devel; +Cc: Paolo Bonzini, Igor Mammedov On 09/23/2016 12:45 PM, Eduardo Habkost wrote: > It makes it easier to guarantee the arrays are the right size, > and to find information when looking at the code. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 370 +++++++++++++++++++++++++----------------------------- > 1 file changed, 170 insertions(+), 200 deletions(-) Reviewed-by: Richard Henderson <rth@twiddle.net> > static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { > [FEAT_1_EDX] = { > - .feat_names = feature_name, > + .feat_names = { > + "fpu", "vme", "de", "pse", > + "tsc", "msr", "pae", "mce", > + "cx8", "apic", NULL, "sep", > + "mtrr", "pge", "mca", "cmov", > + "pat", "pse36", "pn" /* Intel psn */, "clflush" /* Intel clfsh */, > + NULL, "ds" /* Intel dts */, "acpi", "mmx", > + "fxsr", "sse", "sse2", "ss", > + "ht" /* Intel htt */, "tm", "ia64", "pbe", > + }, Unrelated, but can we make this feature_word_info structure const? It may require the addition of const to other function parameters, in which case the change should be a separate patch. r~ ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/7] target-i386: Don't try to enable PT State xsave component 2016-09-23 19:45 [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost 2016-09-23 19:45 ` [Qemu-devel] [PATCH 1/7] target-i386: Move feature name arrays inside FeatureWordInfo Eduardo Habkost @ 2016-09-23 19:45 ` Eduardo Habkost 2016-09-23 20:04 ` Richard Henderson 2016-09-23 19:45 ` [Qemu-devel] [PATCH 3/7] target-i386: xsave: Calculate enabled components only once Eduardo Habkost ` (5 subsequent siblings) 7 siblings, 1 reply; 17+ messages in thread From: Eduardo Habkost @ 2016-09-23 19:45 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, Igor Mammedov The code that calculates the set of supported XSAVE components on CPUID looks at ext_save_areas to find out which components should be enabled. However, if there are zeroed entries in the ext_save_areas array, the ((env->features[esa->feature] & esa->bits) == esa->bits) check will always succeed and QEMU will unconditionally try to enable the component. Luckily this never caused any problems because the only missing entry in ext_save_areas is the PT State component (bit 8), and KVM currently doesn't support it (so it was cleared on ena_mask). But the code was still incorrect and would break if KVM starts returning CPUID[EAX=0xD,ECX=0].EAX[bit 8] as supported on GET_SUPPORTED_CPUID. Fix the problem by changing the code to not enable a XSAVE component if ExtSaveArea::bits is zero. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index cc07fdb..25ab4f8 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2514,7 +2514,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, *ecx = 0x240; for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { const ExtSaveArea *esa = &x86_ext_save_areas[i]; - if ((env->features[esa->feature] & esa->bits) == esa->bits + if ((env->features[esa->feature] & esa->bits) && ((ena_mask >> i) & 1) != 0) { if (i < 32) { *eax |= 1u << i; @@ -2530,7 +2530,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, *eax = env->features[FEAT_XSAVE]; } else if (count < ARRAY_SIZE(x86_ext_save_areas)) { const ExtSaveArea *esa = &x86_ext_save_areas[count]; - if ((env->features[esa->feature] & esa->bits) == esa->bits + if ((env->features[esa->feature] & esa->bits) && ((ena_mask >> count) & 1) != 0) { *eax = esa->size; *ebx = esa->offset; @@ -2766,7 +2766,7 @@ static void x86_cpu_reset(CPUState *s) } for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { const ExtSaveArea *esa = &x86_ext_save_areas[i]; - if ((env->features[esa->feature] & esa->bits) == esa->bits) { + if (env->features[esa->feature] & esa->bits) { xcr0 |= 1ull << i; } } -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/7] target-i386: Don't try to enable PT State xsave component 2016-09-23 19:45 ` [Qemu-devel] [PATCH 2/7] target-i386: Don't try to enable PT State xsave component Eduardo Habkost @ 2016-09-23 20:04 ` Richard Henderson 0 siblings, 0 replies; 17+ messages in thread From: Richard Henderson @ 2016-09-23 20:04 UTC (permalink / raw) To: Eduardo Habkost, qemu-devel; +Cc: Paolo Bonzini, Igor Mammedov On 09/23/2016 12:45 PM, Eduardo Habkost wrote: > The code that calculates the set of supported XSAVE components on > CPUID looks at ext_save_areas to find out which components should > be enabled. However, if there are zeroed entries in the > ext_save_areas array, the > ((env->features[esa->feature] & esa->bits) == esa->bits) > check will always succeed and QEMU will unconditionally try to > enable the component. > > Luckily this never caused any problems because the only missing > entry in ext_save_areas is the PT State component (bit 8), and > KVM currently doesn't support it (so it was cleared on ena_mask). > But the code was still incorrect and would break if KVM starts > returning CPUID[EAX=0xD,ECX=0].EAX[bit 8] as supported on > GET_SUPPORTED_CPUID. > > Fix the problem by changing the code to not enable a XSAVE > component if ExtSaveArea::bits is zero. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Richard Henderson <rth@twiddle.net> r~ ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 3/7] target-i386: xsave: Calculate enabled components only once 2016-09-23 19:45 [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost 2016-09-23 19:45 ` [Qemu-devel] [PATCH 1/7] target-i386: Move feature name arrays inside FeatureWordInfo Eduardo Habkost 2016-09-23 19:45 ` [Qemu-devel] [PATCH 2/7] target-i386: Don't try to enable PT State xsave component Eduardo Habkost @ 2016-09-23 19:45 ` Eduardo Habkost 2016-09-23 20:05 ` Richard Henderson 2016-09-23 19:45 ` [Qemu-devel] [PATCH 4/7] target-i386: xsave: Simplify CPUID[0xD, 0].{EAX, EDX} calculation Eduardo Habkost ` (4 subsequent siblings) 7 siblings, 1 reply; 17+ messages in thread From: Eduardo Habkost @ 2016-09-23 19:45 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, Igor Mammedov Instead of checking both env->features and ena_mask at two different places in the CPUID code, initialize ena_mask based on the features that are enabled for the CPU, and then clear unsupported bits based on kvm_arch_get_supported_cpuid(). The results should be exactly the same, but it will make it easier to move the mask calculation elsewhare, and reuse x86_cpu_filter_features() for the kvm_arch_get_supported_cpuid() check. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 25ab4f8..9968581 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2490,7 +2490,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, *ebx &= 0xffff; /* The count doesn't need to be reliable. */ break; case 0xD: { - KVMState *s = cs->kvm_state; uint64_t ena_mask; int i; @@ -2502,20 +2501,28 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) { break; } + + ena_mask = (XSTATE_FP_MASK | XSTATE_SSE_MASK); + for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { + const ExtSaveArea *esa = &x86_ext_save_areas[i]; + if (env->features[esa->feature] & esa->bits) { + ena_mask |= (1ULL << i); + } + } + if (kvm_enabled()) { - ena_mask = kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX); - ena_mask <<= 32; - ena_mask |= kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX); - } else { - ena_mask = -1; + KVMState *s = cs->kvm_state; + uint64_t kvm_mask = kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX); + kvm_mask <<= 32; + kvm_mask |= kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX); + ena_mask &= kvm_mask; } if (count == 0) { *ecx = 0x240; for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { const ExtSaveArea *esa = &x86_ext_save_areas[i]; - if ((env->features[esa->feature] & esa->bits) - && ((ena_mask >> i) & 1) != 0) { + if ((ena_mask >> i) & 1) { if (i < 32) { *eax |= 1u << i; } else { @@ -2530,8 +2537,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, *eax = env->features[FEAT_XSAVE]; } else if (count < ARRAY_SIZE(x86_ext_save_areas)) { const ExtSaveArea *esa = &x86_ext_save_areas[count]; - if ((env->features[esa->feature] & esa->bits) - && ((ena_mask >> count) & 1) != 0) { + if ((ena_mask >> count) & 1) { *eax = esa->size; *ebx = esa->offset; } -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 3/7] target-i386: xsave: Calculate enabled components only once 2016-09-23 19:45 ` [Qemu-devel] [PATCH 3/7] target-i386: xsave: Calculate enabled components only once Eduardo Habkost @ 2016-09-23 20:05 ` Richard Henderson 0 siblings, 0 replies; 17+ messages in thread From: Richard Henderson @ 2016-09-23 20:05 UTC (permalink / raw) To: Eduardo Habkost, qemu-devel; +Cc: Paolo Bonzini, Igor Mammedov On 09/23/2016 12:45 PM, Eduardo Habkost wrote: > Instead of checking both env->features and ena_mask at two > different places in the CPUID code, initialize ena_mask based on > the features that are enabled for the CPU, and then clear > unsupported bits based on kvm_arch_get_supported_cpuid(). > > The results should be exactly the same, but it will make it > easier to move the mask calculation elsewhare, and reuse > x86_cpu_filter_features() for the kvm_arch_get_supported_cpuid() > check. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 26 ++++++++++++++++---------- > 1 file changed, 16 insertions(+), 10 deletions(-) Reviewed-by: Richard Henderson <rth@twiddle.net> r~ ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 4/7] target-i386: xsave: Simplify CPUID[0xD, 0].{EAX, EDX} calculation 2016-09-23 19:45 [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost ` (2 preceding siblings ...) 2016-09-23 19:45 ` [Qemu-devel] [PATCH 3/7] target-i386: xsave: Calculate enabled components only once Eduardo Habkost @ 2016-09-23 19:45 ` Eduardo Habkost 2016-09-23 20:06 ` Richard Henderson 2016-09-23 19:45 ` [Qemu-devel] [PATCH 5/7] target-i386: xsave: Helper function to calculate xsave area size Eduardo Habkost ` (3 subsequent siblings) 7 siblings, 1 reply; 17+ messages in thread From: Eduardo Habkost @ 2016-09-23 19:45 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, Igor Mammedov Instead of assigning individual bits in a loop, just copy the values from ena_mask. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 9968581..7e66003 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2523,15 +2523,11 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { const ExtSaveArea *esa = &x86_ext_save_areas[i]; if ((ena_mask >> i) & 1) { - if (i < 32) { - *eax |= 1u << i; - } else { - *edx |= 1u << (i - 32); - } *ecx = MAX(*ecx, esa->offset + esa->size); } } - *eax |= ena_mask & (XSTATE_FP_MASK | XSTATE_SSE_MASK); + *eax = ena_mask; + *edx = ena_mask >> 32; *ebx = *ecx; } else if (count == 1) { *eax = env->features[FEAT_XSAVE]; -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 4/7] target-i386: xsave: Simplify CPUID[0xD, 0].{EAX, EDX} calculation 2016-09-23 19:45 ` [Qemu-devel] [PATCH 4/7] target-i386: xsave: Simplify CPUID[0xD, 0].{EAX, EDX} calculation Eduardo Habkost @ 2016-09-23 20:06 ` Richard Henderson 0 siblings, 0 replies; 17+ messages in thread From: Richard Henderson @ 2016-09-23 20:06 UTC (permalink / raw) To: Eduardo Habkost, qemu-devel; +Cc: Paolo Bonzini, Igor Mammedov On 09/23/2016 12:45 PM, Eduardo Habkost wrote: > Instead of assigning individual bits in a loop, just copy the > values from ena_mask. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) Reviewed-by: Richard Henderson <rth@twiddle.net> r~ ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 5/7] target-i386: xsave: Helper function to calculate xsave area size 2016-09-23 19:45 [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost ` (3 preceding siblings ...) 2016-09-23 19:45 ` [Qemu-devel] [PATCH 4/7] target-i386: xsave: Simplify CPUID[0xD, 0].{EAX, EDX} calculation Eduardo Habkost @ 2016-09-23 19:45 ` Eduardo Habkost 2016-09-23 20:07 ` Richard Henderson 2016-09-23 19:45 ` [Qemu-devel] [PATCH 6/7] target-i386: xsave: Calculate set of xsave components on realize Eduardo Habkost ` (2 subsequent siblings) 7 siblings, 1 reply; 17+ messages in thread From: Eduardo Habkost @ 2016-09-23 19:45 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, Igor Mammedov Move the xsave area size calculation from cpu_x86_cpuid() inside its own function. While doing it, change it to use the XSAVE area struct sizes for the initial size, instead of the magic 0x240 number. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 7e66003..9034d8e 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -548,6 +548,20 @@ static const ExtSaveArea x86_ext_save_areas[] = { .size = sizeof(XSavePKRU) }, }; +static uint32_t xsave_area_size(uint64_t mask) +{ + int i; + uint64_t ret = sizeof(X86LegacyXSaveArea) + sizeof(X86XSaveHeader); + + for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { + const ExtSaveArea *esa = &x86_ext_save_areas[i]; + if ((mask >> i) & 1) { + ret = MAX(ret, esa->offset + esa->size); + } + } + return ret; +} + const char *get_register_name_32(unsigned int reg) { if (reg >= CPU_NB_REGS32) { @@ -2519,13 +2533,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, } if (count == 0) { - *ecx = 0x240; - for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { - const ExtSaveArea *esa = &x86_ext_save_areas[i]; - if ((ena_mask >> i) & 1) { - *ecx = MAX(*ecx, esa->offset + esa->size); - } - } + *ecx = xsave_area_size(ena_mask);; *eax = ena_mask; *edx = ena_mask >> 32; *ebx = *ecx; -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 5/7] target-i386: xsave: Helper function to calculate xsave area size 2016-09-23 19:45 ` [Qemu-devel] [PATCH 5/7] target-i386: xsave: Helper function to calculate xsave area size Eduardo Habkost @ 2016-09-23 20:07 ` Richard Henderson 0 siblings, 0 replies; 17+ messages in thread From: Richard Henderson @ 2016-09-23 20:07 UTC (permalink / raw) To: Eduardo Habkost, qemu-devel; +Cc: Paolo Bonzini, Igor Mammedov On 09/23/2016 12:45 PM, Eduardo Habkost wrote: > Move the xsave area size calculation from cpu_x86_cpuid() inside > its own function. While doing it, change it to use the XSAVE area > struct sizes for the initial size, instead of the magic 0x240 > number. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) Reviewed-by: Richard Henderson <rth@twiddle.net> r~ ^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 6/7] target-i386: xsave: Calculate set of xsave components on realize 2016-09-23 19:45 [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost ` (4 preceding siblings ...) 2016-09-23 19:45 ` [Qemu-devel] [PATCH 5/7] target-i386: xsave: Helper function to calculate xsave area size Eduardo Habkost @ 2016-09-23 19:45 ` Eduardo Habkost 2016-09-23 20:09 ` Richard Henderson 2016-09-27 20:06 ` Eduardo Habkost 2016-09-23 19:45 ` [Qemu-devel] [PATCH 7/7] target-i386: Move xsave component mask to features array Eduardo Habkost 2016-09-27 12:45 ` [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost 7 siblings, 2 replies; 17+ messages in thread From: Eduardo Habkost @ 2016-09-23 19:45 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, Igor Mammedov Instead of doing complex calculations and calling kvm_arch_get_supported_cpuid() inside cpu_x86_cpuid(), calculate the set of required XSAVE components earlier, at realize time. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 51 ++++++++++++++++++++++++++++----------------------- target-i386/cpu.h | 1 + 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 9034d8e..e6525e7 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2504,9 +2504,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, *ebx &= 0xffff; /* The count doesn't need to be reliable. */ break; case 0xD: { - uint64_t ena_mask; - int i; - /* Processor Extended State */ *eax = 0; *ebx = 0; @@ -2516,32 +2513,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, break; } - ena_mask = (XSTATE_FP_MASK | XSTATE_SSE_MASK); - for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { - const ExtSaveArea *esa = &x86_ext_save_areas[i]; - if (env->features[esa->feature] & esa->bits) { - ena_mask |= (1ULL << i); - } - } - - if (kvm_enabled()) { - KVMState *s = cs->kvm_state; - uint64_t kvm_mask = kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX); - kvm_mask <<= 32; - kvm_mask |= kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX); - ena_mask &= kvm_mask; - } - if (count == 0) { - *ecx = xsave_area_size(ena_mask);; - *eax = ena_mask; - *edx = ena_mask >> 32; + *ecx = xsave_area_size(env->xsave_components); + *eax = env->xsave_components; + *edx = env->xsave_components >> 32; *ebx = *ecx; } else if (count == 1) { *eax = env->features[FEAT_XSAVE]; } else if (count < ARRAY_SIZE(x86_ext_save_areas)) { const ExtSaveArea *esa = &x86_ext_save_areas[count]; - if ((ena_mask >> count) & 1) { + if ((env->xsave_components >> count) & 1) { *eax = esa->size; *ebx = esa->offset; } @@ -2971,6 +2952,29 @@ static void x86_cpu_adjust_feat_level(X86CPU *cpu, FeatureWord w) } } +/* Calculate XSAVE components based on the configured CPU feature flags */ +static void x86_cpu_enable_xsave_components(X86CPU *cpu) +{ + CPUX86State *env = &cpu->env; + int i; + + env->xsave_components = (XSTATE_FP_MASK | XSTATE_SSE_MASK); + for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { + const ExtSaveArea *esa = &x86_ext_save_areas[i]; + if (env->features[esa->feature] & esa->bits) { + env->xsave_components |= (1ULL << i); + } + } + + if (kvm_enabled()) { + KVMState *s = kvm_state; + uint64_t kvm_mask = kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX); + kvm_mask <<= 32; + kvm_mask |= kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX); + env->xsave_components &= kvm_mask; + } +} + #define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \ (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \ (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3) @@ -3016,6 +3020,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) cpu->env.features[w] &= ~minus_features[w]; } + x86_cpu_enable_xsave_components(cpu); /* CPUID[EAX=7,ECX=0].EBX always increased level automatically: */ x86_cpu_adjust_feat_level(cpu, FEAT_7_0_EBX); diff --git a/target-i386/cpu.h b/target-i386/cpu.h index aaa45f0..6c457ed 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -1122,6 +1122,7 @@ typedef struct CPUX86State { uint32_t cpuid_vendor3; uint32_t cpuid_version; FeatureWordArray features; + uint64_t xsave_components; uint32_t cpuid_model[12]; /* MTRRs */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] target-i386: xsave: Calculate set of xsave components on realize 2016-09-23 19:45 ` [Qemu-devel] [PATCH 6/7] target-i386: xsave: Calculate set of xsave components on realize Eduardo Habkost @ 2016-09-23 20:09 ` Richard Henderson 2016-09-27 20:06 ` Eduardo Habkost 1 sibling, 0 replies; 17+ messages in thread From: Richard Henderson @ 2016-09-23 20:09 UTC (permalink / raw) To: Eduardo Habkost, qemu-devel; +Cc: Paolo Bonzini, Igor Mammedov On 09/23/2016 12:45 PM, Eduardo Habkost wrote: > Instead of doing complex calculations and calling > kvm_arch_get_supported_cpuid() inside cpu_x86_cpuid(), calculate > the set of required XSAVE components earlier, at realize time. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 51 ++++++++++++++++++++++++++++----------------------- > target-i386/cpu.h | 1 + > 2 files changed, 29 insertions(+), 23 deletions(-) Reviewed-by: Richard Henderson <rth@twiddle.net> > @@ -2504,9 +2504,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > *ebx &= 0xffff; /* The count doesn't need to be reliable. */ > break; > case 0xD: { > - uint64_t ena_mask; > - int i; > - > /* Processor Extended State */ > *eax = 0; > *ebx = 0; We should be able to drop the braces around this case as well, please. r~ ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 6/7] target-i386: xsave: Calculate set of xsave components on realize 2016-09-23 19:45 ` [Qemu-devel] [PATCH 6/7] target-i386: xsave: Calculate set of xsave components on realize Eduardo Habkost 2016-09-23 20:09 ` Richard Henderson @ 2016-09-27 20:06 ` Eduardo Habkost 1 sibling, 0 replies; 17+ messages in thread From: Eduardo Habkost @ 2016-09-27 20:06 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Igor Mammedov, Richard Henderson On Fri, Sep 23, 2016 at 04:45:35PM -0300, Eduardo Habkost wrote: [...] > @@ -2971,6 +2952,29 @@ static void x86_cpu_adjust_feat_level(X86CPU *cpu, FeatureWord w) > } > } > > +/* Calculate XSAVE components based on the configured CPU feature flags */ > +static void x86_cpu_enable_xsave_components(X86CPU *cpu) > +{ > + CPUX86State *env = &cpu->env; > + int i; > + > + env->xsave_components = (XSTATE_FP_MASK | XSTATE_SSE_MASK); We shouldn't set xsave_components if XSAVE is disabled. The following fix was squashed while applying: diff --git a/target-i386/cpu.c b/target-i386/cpu.c index e6525e7..8bef3cf 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -2958,6 +2958,10 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu) CPUX86State *env = &cpu->env; int i; + if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) { + return; + } + env->xsave_components = (XSTATE_FP_MASK | XSTATE_SSE_MASK); for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { const ExtSaveArea *esa = &x86_ext_save_areas[i]; > + for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { > + const ExtSaveArea *esa = &x86_ext_save_areas[i]; > + if (env->features[esa->feature] & esa->bits) { > + env->xsave_components |= (1ULL << i); > + } > + } > + > + if (kvm_enabled()) { > + KVMState *s = kvm_state; > + uint64_t kvm_mask = kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX); > + kvm_mask <<= 32; > + kvm_mask |= kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX); > + env->xsave_components &= kvm_mask; > + } > +} > + > #define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \ > (env)->cpuid_vendor2 == CPUID_VENDOR_INTEL_2 && \ > (env)->cpuid_vendor3 == CPUID_VENDOR_INTEL_3) > @@ -3016,6 +3020,7 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > cpu->env.features[w] &= ~minus_features[w]; > } > > + x86_cpu_enable_xsave_components(cpu); > > /* CPUID[EAX=7,ECX=0].EBX always increased level automatically: */ > x86_cpu_adjust_feat_level(cpu, FEAT_7_0_EBX); > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index aaa45f0..6c457ed 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -1122,6 +1122,7 @@ typedef struct CPUX86State { > uint32_t cpuid_vendor3; > uint32_t cpuid_version; > FeatureWordArray features; > + uint64_t xsave_components; > uint32_t cpuid_model[12]; > > /* MTRRs */ > -- > 2.7.4 > > -- Eduardo ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 7/7] target-i386: Move xsave component mask to features array 2016-09-23 19:45 [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost ` (5 preceding siblings ...) 2016-09-23 19:45 ` [Qemu-devel] [PATCH 6/7] target-i386: xsave: Calculate set of xsave components on realize Eduardo Habkost @ 2016-09-23 19:45 ` Eduardo Habkost 2016-09-23 20:20 ` Richard Henderson 2016-09-27 12:45 ` [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost 7 siblings, 1 reply; 17+ messages in thread From: Eduardo Habkost @ 2016-09-23 19:45 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Richard Henderson, Igor Mammedov This will reuse the existing check/enforce logic in x86_cpu_filter_features() to check the xsave component bits against GET_SUPPORTED_CPUID. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 42 ++++++++++++++++++++++++++++-------------- target-i386/cpu.h | 3 ++- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/target-i386/cpu.c b/target-i386/cpu.c index e6525e7..b2c3e17 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -489,6 +489,18 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] = { .cpuid_eax = 6, .cpuid_reg = R_EAX, .tcg_features = TCG_6_EAX_FEATURES, }, + [FEAT_XSAVE_COMP_LO] = { + .cpuid_eax = 0xD, + .cpuid_needs_ecx = true, .cpuid_ecx = 0, + .cpuid_reg = R_EAX, + .tcg_features = ~0U, + }, + [FEAT_XSAVE_COMP_HI] = { + .cpuid_eax = 0xD, + .cpuid_needs_ecx = true, .cpuid_ecx = 0, + .cpuid_reg = R_EDX, + .tcg_features = ~0U, + }, }; typedef struct X86RegisterInfo32 { @@ -562,6 +574,12 @@ static uint32_t xsave_area_size(uint64_t mask) return ret; } +static inline uint64_t x86_cpu_xsave_components(X86CPU *cpu) +{ + return ((uint64_t)cpu->env.features[FEAT_XSAVE_COMP_HI]) << 32 | + cpu->env.features[FEAT_XSAVE_COMP_LO]; +} + const char *get_register_name_32(unsigned int reg) { if (reg >= CPU_NB_REGS32) { @@ -2514,15 +2532,15 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, } if (count == 0) { - *ecx = xsave_area_size(env->xsave_components); - *eax = env->xsave_components; - *edx = env->xsave_components >> 32; + *ecx = xsave_area_size(x86_cpu_xsave_components(cpu)); + *eax = env->features[FEAT_XSAVE_COMP_LO]; + *edx = env->features[FEAT_XSAVE_COMP_HI]; *ebx = *ecx; } else if (count == 1) { *eax = env->features[FEAT_XSAVE]; } else if (count < ARRAY_SIZE(x86_ext_save_areas)) { - const ExtSaveArea *esa = &x86_ext_save_areas[count]; - if ((env->xsave_components >> count) & 1) { + if ((x86_cpu_xsave_components(cpu) >> count) & 1) { + const ExtSaveArea *esa = &x86_ext_save_areas[count]; *eax = esa->size; *ebx = esa->offset; } @@ -2957,22 +2975,18 @@ static void x86_cpu_enable_xsave_components(X86CPU *cpu) { CPUX86State *env = &cpu->env; int i; + uint64_t mask; - env->xsave_components = (XSTATE_FP_MASK | XSTATE_SSE_MASK); + mask = (XSTATE_FP_MASK | XSTATE_SSE_MASK); for (i = 2; i < ARRAY_SIZE(x86_ext_save_areas); i++) { const ExtSaveArea *esa = &x86_ext_save_areas[i]; if (env->features[esa->feature] & esa->bits) { - env->xsave_components |= (1ULL << i); + mask |= (1ULL << i); } } - if (kvm_enabled()) { - KVMState *s = kvm_state; - uint64_t kvm_mask = kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EDX); - kvm_mask <<= 32; - kvm_mask |= kvm_arch_get_supported_cpuid(s, 0xd, 0, R_EAX); - env->xsave_components &= kvm_mask; - } + env->features[FEAT_XSAVE_COMP_LO] = mask; + env->features[FEAT_XSAVE_COMP_HI] = mask >> 32; } #define IS_INTEL_CPU(env) ((env)->cpuid_vendor1 == CPUID_VENDOR_INTEL_1 && \ diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 6c457ed..1cb32ae 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -453,6 +453,8 @@ typedef enum FeatureWord { FEAT_SVM, /* CPUID[8000_000A].EDX */ FEAT_XSAVE, /* CPUID[EAX=0xd,ECX=1].EAX */ FEAT_6_EAX, /* CPUID[6].EAX */ + FEAT_XSAVE_COMP_LO, /* CPUID[EAX=0xd,ECX=0].EAX */ + FEAT_XSAVE_COMP_HI, /* CPUID[EAX=0xd,ECX=0].EDX */ FEATURE_WORDS, } FeatureWord; @@ -1122,7 +1124,6 @@ typedef struct CPUX86State { uint32_t cpuid_vendor3; uint32_t cpuid_version; FeatureWordArray features; - uint64_t xsave_components; uint32_t cpuid_model[12]; /* MTRRs */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 7/7] target-i386: Move xsave component mask to features array 2016-09-23 19:45 ` [Qemu-devel] [PATCH 7/7] target-i386: Move xsave component mask to features array Eduardo Habkost @ 2016-09-23 20:20 ` Richard Henderson 0 siblings, 0 replies; 17+ messages in thread From: Richard Henderson @ 2016-09-23 20:20 UTC (permalink / raw) To: Eduardo Habkost, qemu-devel; +Cc: Paolo Bonzini, Igor Mammedov On 09/23/2016 12:45 PM, Eduardo Habkost wrote: > This will reuse the existing check/enforce logic in > x86_cpu_filter_features() to check the xsave component bits > against GET_SUPPORTED_CPUID. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 42 ++++++++++++++++++++++++++++-------------- > target-i386/cpu.h | 3 ++- > 2 files changed, 30 insertions(+), 15 deletions(-) Reviewed-by: Richard Henderson <rth@twiddle.net> r~ ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor 2016-09-23 19:45 [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost ` (6 preceding siblings ...) 2016-09-23 19:45 ` [Qemu-devel] [PATCH 7/7] target-i386: Move xsave component mask to features array Eduardo Habkost @ 2016-09-27 12:45 ` Eduardo Habkost 7 siblings, 0 replies; 17+ messages in thread From: Eduardo Habkost @ 2016-09-27 12:45 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Igor Mammedov, Richard Henderson On Fri, Sep 23, 2016 at 04:45:29PM -0300, Eduardo Habkost wrote: > This series refactor the xsave CPUID handling so it won't > silently disable any XSAVE components on CPUID[0xD] in case the > host doesn't support it. It will instead use the exisitng > check/enforce logic for filtering the CPUID bits and checking for > host-side support. > > This series is available on git at: > https://github.com/ehabkost/qemu-hacks.git work/xsave-cpuid-cleanup > > The series is based on my x86-next branch, that contains other > CPUID-related changes: > https://github.com/ehabkost/qemu.git x8-next > > Eduardo Habkost (7): > target-i386: Move feature name arrays inside FeatureWordInfo > target-i386: Don't try to enable PT State xsave component > target-i386: xsave: Calculate enabled components only once > target-i386: xsave: Simplify CPUID[0xD,0].{EAX,EDX} calculation > target-i386: xsave: Helper function to calculate xsave area size > target-i386: xsave: Calculate set of xsave components on realize > target-i386: Move xsave component mask to features array Queued on x86-next. -- Eduardo ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-09-27 20:06 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-23 19:45 [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost
2016-09-23 19:45 ` [Qemu-devel] [PATCH 1/7] target-i386: Move feature name arrays inside FeatureWordInfo Eduardo Habkost
2016-09-23 20:01 ` Richard Henderson
2016-09-23 19:45 ` [Qemu-devel] [PATCH 2/7] target-i386: Don't try to enable PT State xsave component Eduardo Habkost
2016-09-23 20:04 ` Richard Henderson
2016-09-23 19:45 ` [Qemu-devel] [PATCH 3/7] target-i386: xsave: Calculate enabled components only once Eduardo Habkost
2016-09-23 20:05 ` Richard Henderson
2016-09-23 19:45 ` [Qemu-devel] [PATCH 4/7] target-i386: xsave: Simplify CPUID[0xD, 0].{EAX, EDX} calculation Eduardo Habkost
2016-09-23 20:06 ` Richard Henderson
2016-09-23 19:45 ` [Qemu-devel] [PATCH 5/7] target-i386: xsave: Helper function to calculate xsave area size Eduardo Habkost
2016-09-23 20:07 ` Richard Henderson
2016-09-23 19:45 ` [Qemu-devel] [PATCH 6/7] target-i386: xsave: Calculate set of xsave components on realize Eduardo Habkost
2016-09-23 20:09 ` Richard Henderson
2016-09-27 20:06 ` Eduardo Habkost
2016-09-23 19:45 ` [Qemu-devel] [PATCH 7/7] target-i386: Move xsave component mask to features array Eduardo Habkost
2016-09-23 20:20 ` Richard Henderson
2016-09-27 12:45 ` [Qemu-devel] [PATCH 0/7] target-i386: xsave CPUID handling refactor Eduardo Habkost
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).