* [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 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.