[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 1/3] x86: Data structure changes to support M
|
From: |
Eduardo Habkost |
|
Subject: |
Re: [Qemu-devel] [PATCH v4 1/3] x86: Data structure changes to support MSR based features |
|
Date: |
Wed, 19 Sep 2018 23:51:05 -0300 |
|
User-agent: |
Mutt/1.9.2 (2017-12-15) |
Hi,
Thanks for the patch, and sorry for taking so long to review it.
On Sun, Sep 02, 2018 at 07:46:05PM +0800, Robert Hoo wrote:
> Add FeatureWordType indicator in struct FeatureWordInfo.
> Change feature_word_info[] accordingly.
> Change existing functions that refer to feature_word_info[] accordingly.
>
> Signed-off-by: Robert Hoo <address@hidden>
> ---
> target/i386/cpu.c | 172
> +++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 120 insertions(+), 52 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index f24295e..a252c26 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -770,17 +770,36 @@ static void x86_cpu_vendor_words2str(char *dst,
> uint32_t vendor1,
> /* missing:
> CPUID_XSAVE_XSAVEC, CPUID_XSAVE_XSAVES */
>
> +typedef enum FeatureWordType {
> + CPUID_FEATURE_WORD,
> + MSR_FEATURE_WORD,
> +} FeatureWordType;
> +
> typedef struct FeatureWordInfo {
> - /* feature flags names are taken from "Intel Processor Identification and
> + FeatureWordType type;
> + /* feature flags names are taken from "Intel Processor Identification and
Unintentional whitespace change, I assume.
> * 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 */
> - int cpuid_reg; /* output register (R_* constant) */
> + union {
> + /* If type==CPUID_FEATURE_WORD */
> + struct {
> + uint32_t eax; /* Input EAX for CPUID */
> + bool needs_ecx; /* CPUID instruction uses ECX as input */
> + uint32_t ecx; /* Input ECX value for CPUID */
> + int reg; /* output register (R_* constant) */
> + } cpuid;
> + /* If type==MSR_FEATURE_WORD */
> + struct {
> + uint32_t index;
> + struct { /*CPUID that enumerate this MSR*/
> + FeatureWord cpuid_class;
> + uint32_t cpuid_flag;
> + } cpuid_dep;
> + } msr;
> + };
> uint32_t tcg_features; /* Feature flags supported by TCG */
> uint32_t unmigratable_flags; /* Feature flags known to be unmigratable */
> uint32_t migratable_flags; /* Feature flags known to be migratable */
> @@ -790,6 +809,7 @@ typedef struct FeatureWordInfo {
>
> static FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> [FEAT_1_EDX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> "fpu", "vme", "de", "pse",
> "tsc", "msr", "pae", "mce",
> @@ -800,10 +820,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS]
> = {
> "fxsr", "sse", "sse2", "ss",
> "ht" /* Intel htt */, "tm", "ia64", "pbe",
> },
> - .cpuid_eax = 1, .cpuid_reg = R_EDX,
> + .cpuid = {.eax = 1, .reg = R_EDX, },
> .tcg_features = TCG_FEATURES,
> },
> [FEAT_1_ECX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> "pni" /* Intel,AMD sse3 */, "pclmulqdq", "dtes64", "monitor",
> "ds-cpl", "vmx", "smx", "est",
> @@ -814,7 +835,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] =
> {
> "tsc-deadline", "aes", "xsave", NULL /* osxsave */,
> "avx", "f16c", "rdrand", "hypervisor",
> },
> - .cpuid_eax = 1, .cpuid_reg = R_ECX,
> + .cpuid = { .eax = 1, .reg = R_ECX, },
> .tcg_features = TCG_EXT_FEATURES,
> },
> /* Feature names that are already defined on feature_name[] but
> @@ -823,6 +844,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] =
> {
> * to features[FEAT_8000_0001_EDX] if and only if CPU vendor is AMD.
> */
> [FEAT_8000_0001_EDX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */,
> NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */,
> @@ -833,10 +855,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS]
> = {
> NULL /* fxsr */, "fxsr-opt", "pdpe1gb", "rdtscp",
> NULL, "lm", "3dnowext", "3dnow",
> },
> - .cpuid_eax = 0x80000001, .cpuid_reg = R_EDX,
> + .cpuid = { .eax = 0x80000001, .reg = R_EDX, },
> .tcg_features = TCG_EXT2_FEATURES,
> },
> [FEAT_8000_0001_ECX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> "lahf-lm", "cmp-legacy", "svm", "extapic",
> "cr8legacy", "abm", "sse4a", "misalignsse",
> @@ -847,7 +870,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] =
> {
> "perfctr-nb", NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = 0x80000001, .cpuid_reg = R_ECX,
> + .cpuid = { .eax = 0x80000001, .reg = R_ECX, },
> .tcg_features = TCG_EXT3_FEATURES,
> /*
> * TOPOEXT is always allowed but can't be enabled blindly by
> @@ -857,6 +880,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] =
> {
> .no_autoenable_flags = CPUID_EXT3_TOPOEXT,
> },
> [FEAT_C000_0001_EDX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> NULL, NULL, "xstore", "xstore-en",
> NULL, NULL, "xcrypt", "xcrypt-en",
> @@ -867,10 +891,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS]
> = {
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = 0xC0000001, .cpuid_reg = R_EDX,
> + .cpuid = { .eax = 0xC0000001, .reg = R_EDX, },
> .tcg_features = TCG_EXT4_FEATURES,
> },
> [FEAT_KVM] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> "kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
> "kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
> @@ -881,10 +906,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS]
> = {
> "kvmclock-stable-bit", NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EAX,
> + .cpuid = { .eax = KVM_CPUID_FEATURES, .reg = R_EAX, },
> .tcg_features = TCG_KVM_FEATURES,
> },
> [FEAT_KVM_HINTS] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> "kvm-hint-dedicated", NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> @@ -895,7 +921,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] =
> {
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = KVM_CPUID_FEATURES, .cpuid_reg = R_EDX,
> + .cpuid = { .eax = KVM_CPUID_FEATURES, .reg = R_EDX, },
> .tcg_features = TCG_KVM_FEATURES,
> /*
> * KVM hints aren't auto-enabled by -cpu host, they need to be
> @@ -904,6 +930,7 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS] =
> {
> .no_autoenable_flags = ~0U,
> },
> [FEAT_HYPERV_EAX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> NULL /* hv_msr_vp_runtime_access */, NULL /*
> hv_msr_time_refcount_access */,
> NULL /* hv_msr_synic_access */, NULL /* hv_msr_stimer_access */,
> @@ -918,9 +945,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS]
> = {
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = 0x40000003, .cpuid_reg = R_EAX,
> + .cpuid = { .eax = 0x40000003, .reg = R_EAX, },
> },
> [FEAT_HYPERV_EBX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> NULL /* hv_create_partitions */, NULL /* hv_access_partition_id
> */,
> NULL /* hv_access_memory_pool */, NULL /*
> hv_adjust_message_buffers */,
> @@ -934,9 +962,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS]
> = {
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = 0x40000003, .cpuid_reg = R_EBX,
> + .cpuid = { .eax = 0x40000003, .reg = R_EBX, },
> },
> [FEAT_HYPERV_EDX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> NULL /* hv_mwait */, NULL /* hv_guest_debugging */,
> NULL /* hv_perf_monitor */, NULL /* hv_cpu_dynamic_part */,
> @@ -949,9 +978,10 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS]
> = {
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = 0x40000003, .cpuid_reg = R_EDX,
> + .cpuid = { .eax = 0x40000003, .reg = R_EDX, },
> },
> [FEAT_SVM] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> "npt", "lbrv", "svm-lock", "nrip-save",
> "tsc-scale", "vmcb-clean", "flushbyasid", "decodeassists",
> @@ -962,10 +992,11 @@ static FeatureWordInfo feature_word_info[FEATURE_WORDS]
> = {
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = 0x8000000A, .cpuid_reg = R_EDX,
> + .cpuid = { .eax = 0x8000000A, .reg = R_EDX, },
> .tcg_features = TCG_SVM_FEATURES,
> },
> [FEAT_7_0_EBX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> "fsgsbase", "tsc-adjust", NULL, "bmi1",
> "hle", "avx2", NULL, "smep",
> @@ -976,12 +1007,13 @@ static FeatureWordInfo
> feature_word_info[FEATURE_WORDS] = {
> "clwb", "intel-pt", "avx512pf", "avx512er",
> "avx512cd", "sha-ni", "avx512bw", "avx512vl",
> },
> - .cpuid_eax = 7,
> - .cpuid_needs_ecx = true, .cpuid_ecx = 0,
> - .cpuid_reg = R_EBX,
> + .cpuid = { .eax = 7,
> + .needs_ecx = true, .ecx = 0,
> + .reg = R_EBX, },
Nit: I would follow the same style of the other multiline struct
literals:
.cpuid = {
.eax = 7,
.needs_ecx = true, .ecx = 0,
.reg = R_EBX,
},
(Same on the other entries below)
> .tcg_features = TCG_7_0_EBX_FEATURES,
> },
> [FEAT_7_0_ECX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> NULL, "avx512vbmi", "umip", "pku",
> NULL /* ospke */, NULL, "avx512vbmi2", NULL,
> @@ -992,12 +1024,13 @@ static FeatureWordInfo
> feature_word_info[FEATURE_WORDS] = {
> NULL, "cldemote", NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = 7,
> - .cpuid_needs_ecx = true, .cpuid_ecx = 0,
> - .cpuid_reg = R_ECX,
> + .cpuid = { .eax = 7,
> + .needs_ecx = true, .ecx = 0,
> + .reg = R_ECX, },
> .tcg_features = TCG_7_0_ECX_FEATURES,
> },
> [FEAT_7_0_EDX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> NULL, NULL, "avx512-4vnniw", "avx512-4fmaps",
> NULL, NULL, NULL, NULL,
> @@ -1008,13 +1041,14 @@ static FeatureWordInfo
> feature_word_info[FEATURE_WORDS] = {
> NULL, NULL, "spec-ctrl", NULL,
> NULL, "arch-capabilities", NULL, "ssbd",
> },
> - .cpuid_eax = 7,
> - .cpuid_needs_ecx = true, .cpuid_ecx = 0,
> - .cpuid_reg = R_EDX,
> + .cpuid = { .eax = 7,
> + .needs_ecx = true, .ecx = 0,
> + .reg = R_EDX, },
> .tcg_features = TCG_7_0_EDX_FEATURES,
> .unmigratable_flags = CPUID_7_0_EDX_ARCH_CAPABILITIES,
> },
> [FEAT_8000_0007_EDX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> @@ -1025,12 +1059,12 @@ static FeatureWordInfo
> feature_word_info[FEATURE_WORDS] = {
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = 0x80000007,
> - .cpuid_reg = R_EDX,
> + .cpuid = { .eax = 0x80000007, .reg = R_EDX, },
> .tcg_features = TCG_APM_FEATURES,
> .unmigratable_flags = CPUID_APM_INVTSC,
> },
> [FEAT_8000_0008_EBX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> @@ -1041,12 +1075,12 @@ static FeatureWordInfo
> feature_word_info[FEATURE_WORDS] = {
> "amd-ssbd", "virt-ssbd", "amd-no-ssb", NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = 0x80000008,
> - .cpuid_reg = R_EBX,
> + .cpuid = { .eax = 0x80000008, .reg = R_EBX, },
> .tcg_features = 0,
> .unmigratable_flags = 0,
> },
> [FEAT_XSAVE] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> "xsaveopt", "xsavec", "xgetbv1", "xsaves",
> NULL, NULL, NULL, NULL,
> @@ -1057,12 +1091,13 @@ static FeatureWordInfo
> feature_word_info[FEATURE_WORDS] = {
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = 0xd,
> - .cpuid_needs_ecx = true, .cpuid_ecx = 1,
> - .cpuid_reg = R_EAX,
> + .cpuid = { .eax = 0xd,
> + .needs_ecx = true, .ecx = 1,
> + .reg = R_EAX, },
> .tcg_features = TCG_XSAVE_FEATURES,
> },
> [FEAT_6_EAX] = {
> + .type = CPUID_FEATURE_WORD,
> .feat_names = {
> NULL, NULL, "arat", NULL,
> NULL, NULL, NULL, NULL,
> @@ -1073,13 +1108,14 @@ static FeatureWordInfo
> feature_word_info[FEATURE_WORDS] = {
> NULL, NULL, NULL, NULL,
> NULL, NULL, NULL, NULL,
> },
> - .cpuid_eax = 6, .cpuid_reg = R_EAX,
> + .cpuid = { .eax = 6, .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,
> + .type = CPUID_FEATURE_WORD,
> + .cpuid = { .eax = 0xD,
> + .needs_ecx = true, .ecx = 0,
> + .reg = R_EAX, },
> .tcg_features = ~0U,
> .migratable_flags = XSTATE_FP_MASK | XSTATE_SSE_MASK |
> XSTATE_YMM_MASK | XSTATE_BNDREGS_MASK | XSTATE_BNDCSR_MASK |
> @@ -1087,9 +1123,10 @@ static FeatureWordInfo
> feature_word_info[FEATURE_WORDS] = {
> XSTATE_PKRU_MASK,
> },
> [FEAT_XSAVE_COMP_HI] = {
> - .cpuid_eax = 0xD,
> - .cpuid_needs_ecx = true, .cpuid_ecx = 0,
> - .cpuid_reg = R_EDX,
> + .type = CPUID_FEATURE_WORD,
> + .cpuid = { .eax = 0xD,
> + .needs_ecx = true, .ecx = 0,
> + .reg = R_EDX, },
> .tcg_features = ~0U,
> },
> };
> @@ -2975,21 +3012,41 @@ static const TypeInfo host_x86_cpu_type_info = {
>
> #endif
>
> +static char *feature_word_description(FeatureWordInfo *f, uint32_t bit)
> +{
> + assert(f->type == CPUID_FEATURE_WORD ||
> + f->type == MSR_FEATURE_WORD);
Nit: you don't need to break this line.
Nit: I suggest one blank line here for readability.
> + switch (f->type) {
> + case CPUID_FEATURE_WORD:
> + {
> + const char *reg = get_register_name_32(f->cpuid.reg);
> + assert(reg);
> + return g_strdup_printf("CPUID.%02XH:%s",
> + f->cpuid.eax, reg);
Nit: please align the 'f' here under the '"' on the line above
(just after the parenthesis).
> + }
> + case MSR_FEATURE_WORD:
> + return g_strdup_printf("MSR(%02XH)",
> + f->msr.index);
Same here.
> + }
> +
> + return NULL;
> +}
> +
> static void report_unavailable_features(FeatureWord w, uint32_t mask)
> {
> FeatureWordInfo *f = &feature_word_info[w];
> int i;
> + char *feat_word_str;
>
> for (i = 0; i < 32; ++i) {
> if ((1UL << i) & mask) {
> - const char *reg = get_register_name_32(f->cpuid_reg);
> - assert(reg);
> - warn_report("%s doesn't support requested feature: "
> - "CPUID.%02XH:%s%s%s [bit %d]",
> + feat_word_str = feature_word_description(f, i);
> + warn_report("%s doesn't support requested feature: %s%s%s [bit
> %d]",
> accel_uses_host_cpuid() ? "host" : "TCG",
> - f->cpuid_eax, reg,
> + feat_word_str,
> f->feat_names[i] ? "." : "",
> f->feat_names[i] ? f->feat_names[i] : "", i);
> + g_free(feat_word_str);
> }
> }
> }
> @@ -3233,11 +3290,14 @@ static void x86_cpu_get_feature_words(Object *obj,
> Visitor *v,
>
> for (w = 0; w < FEATURE_WORDS; w++) {
> FeatureWordInfo *wi = &feature_word_info[w];
> + if (wi->type != CPUID_FEATURE_WORD) {
> + continue;
> + }
I would add a comment explaining that we didn't have MSR features
when "feature-words" was introduced, and that's why other entires
are skipped.
> X86CPUFeatureWordInfo *qwi = &word_infos[w];
> - qwi->cpuid_input_eax = wi->cpuid_eax;
> - qwi->has_cpuid_input_ecx = wi->cpuid_needs_ecx;
> - qwi->cpuid_input_ecx = wi->cpuid_ecx;
> - qwi->cpuid_register = x86_reg_info_32[wi->cpuid_reg].qapi_enum;
> + qwi->cpuid_input_eax = wi->cpuid.eax;
> + qwi->has_cpuid_input_ecx = wi->cpuid.needs_ecx;
> + qwi->cpuid_input_ecx = wi->cpuid.ecx;
> + qwi->cpuid_register = x86_reg_info_32[wi->cpuid.reg].qapi_enum;
> qwi->features = array[w];
>
> /* List will be in reverse order, but order shouldn't matter */
> @@ -3613,9 +3673,16 @@ static uint32_t
> x86_cpu_get_supported_feature_word(FeatureWord w,
> uint32_t r;
>
> if (kvm_enabled()) {
> - r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax,
> - wi->cpuid_ecx,
> - wi->cpuid_reg);
> + switch (wi->type) {
> + case CPUID_FEATURE_WORD:
> + r = kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid.eax,
> + wi->cpuid.ecx,
> + wi->cpuid.reg);
Nit: please keep the alignment from the previous code.
> + break;
> + default:
> + r = 0;
> + break;
I would move patch 2/3 before this one, so we could make this code
return kvm_arch_get_supported_msr_feature() since the beginning.
> + }
> } else if (hvf_enabled()) {
> r = hvf_get_supported_cpuid(wi->cpuid_eax,
> wi->cpuid_ecx,
> @@ -4680,9 +4747,10 @@ static void x86_cpu_adjust_feat_level(X86CPU *cpu,
> FeatureWord w)
> {
> CPUX86State *env = &cpu->env;
> FeatureWordInfo *fi = &feature_word_info[w];
> - uint32_t eax = fi->cpuid_eax;
> + uint32_t eax = fi->cpuid.eax;
> uint32_t region = eax & 0xF0000000;
>
> + assert(feature_word_info[w].type == CPUID_FEATURE_WORD);
> if (!env->features[w]) {
> return;
> }
The rest of the patch looks OK to me. Thanks!
> --
> 1.8.3.1
>
>
--
Eduardo
- Re: [Qemu-devel] [PATCH v4 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl, (continued)
[Qemu-devel] [PATCH v4 3/3] x86: define a new MSR based feature word -- FEATURE_WORDS_ARCH_CAPABILITIES, Robert Hoo, 2018/09/02
[Qemu-devel] [PATCH v4 1/3] x86: Data structure changes to support MSR based features, Robert Hoo, 2018/09/02
- Re: [Qemu-devel] [PATCH v4 1/3] x86: Data structure changes to support MSR based features,
Eduardo Habkost <=
Re: [Qemu-devel] [PATCH v4 0/3] x86: QEMU side support on MSR based features, Robert Hoo, 2018/09/11