* [Qemu-devel] [PATCH v3 0/3] x86: QEMU side support on MSR based features @ 2018-08-10 14:06 Robert Hoo 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support " Robert Hoo ` (2 more replies) 0 siblings, 3 replies; 40+ messages in thread From: Robert Hoo @ 2018-08-10 14:06 UTC (permalink / raw) To: pbonzini, rth, ehabkost, thomas.lendacky Cc: qemu-devel, robert.hu, jingqi.liu, Robert Hoo KVM side has added the framework (kvm.git:d1d93fa90) to support MSR based features. Here is the QEMU part, including data structure changes/expanding, referring functions changes, and the implementations on KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl. Changelog: v3: patch 2&3 in v2 are corrupted. Re-format patches. v2: coding style changes to pass ./scripts/checkpatch.pl. ---------------- Robert Hoo (3): x86: Data structure changes to support MSR based features kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl Change other funcitons referring to feature_word_info[] include/sysemu/kvm.h | 2 + target/i386/cpu.c | 210 +++++++++++++++++++++++++++++++++++++-------------- target/i386/cpu.h | 5 ++ target/i386/kvm.c | 79 +++++++++++++++++++ 4 files changed, 238 insertions(+), 58 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features 2018-08-10 14:06 [Qemu-devel] [PATCH v3 0/3] x86: QEMU side support on MSR based features Robert Hoo @ 2018-08-10 14:06 ` Robert Hoo 2018-08-17 3:10 ` Eduardo Habkost 2018-08-17 15:50 ` Paolo Bonzini 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl Robert Hoo 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] Robert Hoo 2 siblings, 2 replies; 40+ messages in thread From: Robert Hoo @ 2018-08-10 14:06 UTC (permalink / raw) To: pbonzini, rth, ehabkost, thomas.lendacky Cc: qemu-devel, robert.hu, jingqi.liu, Robert Hoo Define FeatureWordType. Expand FeatureWordInfo to support both CPUID type feature word as well as MSR type's. Change feature_word_info[] accordingly. Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> --- target/i386/cpu.c | 133 ++++++++++++++++++++++++++++++++++++++---------------- target/i386/cpu.h | 5 ++ 2 files changed, 99 insertions(+), 39 deletions(-) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index ba7abe5..f7c70d9 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 * 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,10 +870,11 @@ 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, }, [FEAT_C000_0001_EDX] = { + .type = CPUID_FEATURE_WORD, .feat_names = { NULL, NULL, "xstore", "xstore-en", NULL, NULL, "xcrypt", "xcrypt-en", @@ -861,10 +885,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", @@ -875,10 +900,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, @@ -889,7 +915,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 @@ -898,6 +924,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 */, @@ -912,9 +939,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 */, @@ -928,9 +956,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 */, @@ -943,9 +972,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", @@ -956,10 +986,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", @@ -970,12 +1001,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, }, .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, @@ -986,12 +1018,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, @@ -1002,13 +1035,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, @@ -1019,12 +1053,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, @@ -1035,12 +1069,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, @@ -1051,12 +1085,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, @@ -1067,13 +1102,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 | @@ -1081,11 +1117,30 @@ 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, }, + /*Below are MSR exposed features*/ + [FEATURE_WORDS_ARCH_CAPABILITIES] = { + .type = MSR_FEATURE_WORD, + .feat_names = { + "rdctl-no", "ibrs-all", "rsba", NULL, + "ssb-no", 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, + }, + .msr = { .index = MSR_IA32_ARCH_CAPABILITIES, + .cpuid_dep = { FEAT_7_0_EDX, + CPUID_7_0_EDX_ARCH_CAPABILITIES } + }, + }, }; typedef struct X86RegisterInfo32 { diff --git a/target/i386/cpu.h b/target/i386/cpu.h index cddf9d9..9e8879e 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -502,9 +502,14 @@ typedef enum FeatureWord { 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_NUM_CPUID, + FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID, + FEATURE_WORDS_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR, FEATURE_WORDS, } FeatureWord; +#define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - FEATURE_WORDS_FIRST_MSR) + typedef uint32_t FeatureWordArray[FEATURE_WORDS]; /* cpuid_features bits */ -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support " Robert Hoo @ 2018-08-17 3:10 ` Eduardo Habkost 2018-08-18 3:10 ` Robert Hoo 2018-08-17 15:50 ` Paolo Bonzini 1 sibling, 1 reply; 40+ messages in thread From: Eduardo Habkost @ 2018-08-17 3:10 UTC (permalink / raw) To: Robert Hoo Cc: pbonzini, rth, thomas.lendacky, robert.hu, qemu-devel, jingqi.liu Hi, Thanks for the patch and sorry for taking so long to review. Comments below: On Fri, Aug 10, 2018 at 10:06:27PM +0800, Robert Hoo wrote: > Define FeatureWordType. > Expand FeatureWordInfo to support both CPUID type feature word as well as > MSR type's. > Change feature_word_info[] accordingly. > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > --- > target/i386/cpu.c | 133 ++++++++++++++++++++++++++++++++++++++---------------- > target/i386/cpu.h | 5 ++ > 2 files changed, 99 insertions(+), 39 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index ba7abe5..f7c70d9 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 > * 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 */ The data structure change looks good, but you are breaking the build if you touch them without updating the existing code. If you break the build in your series, you break git-bisect. [...] > + /*Below are MSR exposed features*/ > + [FEATURE_WORDS_ARCH_CAPABILITIES] = { > + .type = MSR_FEATURE_WORD, > + .feat_names = { > + "rdctl-no", "ibrs-all", "rsba", NULL, > + "ssb-no", 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, > + }, > + .msr = { .index = MSR_IA32_ARCH_CAPABILITIES, > + .cpuid_dep = { FEAT_7_0_EDX, > + CPUID_7_0_EDX_ARCH_CAPABILITIES } > + }, > + }, I suggest moving this hunk to a separate patch: first we refactor the existing code without changing behavior or adding new features, then we add the new features. > }; > > typedef struct X86RegisterInfo32 { > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index cddf9d9..9e8879e 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -502,9 +502,14 @@ typedef enum FeatureWord { > 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_NUM_CPUID, > + FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID, > + FEATURE_WORDS_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR, > FEATURE_WORDS, > } FeatureWord; > > +#define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - FEATURE_WORDS_FIRST_MSR) > + > typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > > /* cpuid_features bits */ > -- > 1.8.3.1 > > -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features 2018-08-17 3:10 ` Eduardo Habkost @ 2018-08-18 3:10 ` Robert Hoo 2018-08-18 5:48 ` Robert Hoo 0 siblings, 1 reply; 40+ messages in thread From: Robert Hoo @ 2018-08-18 3:10 UTC (permalink / raw) To: Eduardo Habkost Cc: robert.hu, robert.hu, pbonzini, rth, thomas.lendacky, qemu-devel, jingqi.liu On Fri, 2018-08-17 at 00:10 -0300, Eduardo Habkost wrote: [trim...] > > + > > 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 > > * 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 */ > > The data structure change looks good, but you are breaking the > build if you touch them without updating the existing code. If > you break the build in your series, you break git-bisect. > I had tested in my environment before send out, build has no even a warning. Is it because this patch is based on master + previous icelake cpu model set? I see you are pulling in previous icelake cpu model patch set. I will rebase this patch to then master. Will previous icelake cpu model patch set appear in master soon? > > [...] > > + /*Below are MSR exposed features*/ > > + [FEATURE_WORDS_ARCH_CAPABILITIES] = { > > + .type = MSR_FEATURE_WORD, > > + .feat_names = { > > + "rdctl-no", "ibrs-all", "rsba", NULL, > > + "ssb-no", 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, > > + }, > > + .msr = { .index = MSR_IA32_ARCH_CAPABILITIES, > > + .cpuid_dep = { FEAT_7_0_EDX, > > + CPUID_7_0_EDX_ARCH_CAPABILITIES } > > + }, > > + }, > > I suggest moving this hunk to a separate patch: first we refactor > the existing code without changing behavior or adding new > features, then we add the new features. > Sure. > > }; > > > > typedef struct X86RegisterInfo32 { > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > index cddf9d9..9e8879e 100644 > > --- a/target/i386/cpu.h > > +++ b/target/i386/cpu.h > > @@ -502,9 +502,14 @@ typedef enum FeatureWord { > > 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_NUM_CPUID, > > + FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID, > > + FEATURE_WORDS_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR, > > FEATURE_WORDS, > > } FeatureWord; > > > > +#define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - FEATURE_WORDS_FIRST_MSR) > > + > > typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > > > > /* cpuid_features bits */ > > -- > > 1.8.3.1 > > > > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features 2018-08-18 3:10 ` Robert Hoo @ 2018-08-18 5:48 ` Robert Hoo 2018-08-18 7:50 ` Paolo Bonzini 0 siblings, 1 reply; 40+ messages in thread From: Robert Hoo @ 2018-08-18 5:48 UTC (permalink / raw) To: Eduardo Habkost Cc: robert.hu, robert.hu, pbonzini, rth, thomas.lendacky, qemu-devel, jingqi.liu On Sat, 2018-08-18 at 11:10 +0800, Robert Hoo wrote: > On Fri, 2018-08-17 at 00:10 -0300, Eduardo Habkost wrote: > [trim...] > > > + > > > 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 > > > * 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 */ > > > > The data structure change looks good, but you are breaking the > > build if you touch them without updating the existing code. If > > you break the build in your series, you break git-bisect. > > > I had tested in my environment before send out, build has no even a > warning. Is it because this patch is based on master + previous icelake > cpu model set? I see you are pulling in previous icelake cpu model patch > set. I will rebase this patch to then master. Will previous icelake cpu > model patch set appear in master soon? or you mean each single patch in a series should be individually built pass? then it will a huge one here: data structure changes + reference functions. > > > > [...] > > > + /*Below are MSR exposed features*/ > > > + [FEATURE_WORDS_ARCH_CAPABILITIES] = { > > > + .type = MSR_FEATURE_WORD, > > > + .feat_names = { > > > + "rdctl-no", "ibrs-all", "rsba", NULL, > > > + "ssb-no", 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, > > > + }, > > > + .msr = { .index = MSR_IA32_ARCH_CAPABILITIES, > > > + .cpuid_dep = { FEAT_7_0_EDX, > > > + CPUID_7_0_EDX_ARCH_CAPABILITIES } > > > + }, > > > + }, > > > > I suggest moving this hunk to a separate patch: first we refactor > > the existing code without changing behavior or adding new > > features, then we add the new features. > > > Sure. > > > > }; > > > > > > typedef struct X86RegisterInfo32 { > > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > > > index cddf9d9..9e8879e 100644 > > > --- a/target/i386/cpu.h > > > +++ b/target/i386/cpu.h > > > @@ -502,9 +502,14 @@ typedef enum FeatureWord { > > > 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_NUM_CPUID, > > > + FEATURE_WORDS_FIRST_MSR = FEATURE_WORDS_NUM_CPUID, > > > + FEATURE_WORDS_ARCH_CAPABILITIES = FEATURE_WORDS_FIRST_MSR, > > > FEATURE_WORDS, > > > } FeatureWord; > > > > > > +#define FEATURE_WORDS_NUM_MSRS (FEATURE_WORDS - FEATURE_WORDS_FIRST_MSR) > > > + > > > typedef uint32_t FeatureWordArray[FEATURE_WORDS]; > > > > > > /* cpuid_features bits */ > > > -- > > > 1.8.3.1 > > > > > > > > > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features 2018-08-18 5:48 ` Robert Hoo @ 2018-08-18 7:50 ` Paolo Bonzini 0 siblings, 0 replies; 40+ messages in thread From: Paolo Bonzini @ 2018-08-18 7:50 UTC (permalink / raw) To: Robert Hoo Cc: Eduardo Habkost, robert hu, rth, thomas lendacky, qemu-devel, jingqi liu ----- Original Message ----- > From: "Robert Hoo" <robert.hu@linux.intel.com> > To: "Eduardo Habkost" <ehabkost@redhat.com> > Cc: "robert hu" <robert.hu@intel.com>, "robert hu" <robert.hu@linux.intel.com>, pbonzini@redhat.com, rth@twiddle.net, > "thomas lendacky" <thomas.lendacky@amd.com>, qemu-devel@nongnu.org, "jingqi liu" <jingqi.liu@intel.com> > Sent: Saturday, August 18, 2018 7:48:43 AM > Subject: Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features > > On Sat, 2018-08-18 at 11:10 +0800, Robert Hoo wrote: > > On Fri, 2018-08-17 at 00:10 -0300, Eduardo Habkost wrote: > > [trim...] > > > > + > > > > 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 > > > > * 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 > > > > */ > > > > > > The data structure change looks good, but you are breaking the > > > build if you touch them without updating the existing code. If > > > you break the build in your series, you break git-bisect. > > > > > I had tested in my environment before send out, build has no even a > > warning. Is it because this patch is based on master + previous icelake > > cpu model set? I see you are pulling in previous icelake cpu model patch > > set. I will rebase this patch to then master. Will previous icelake cpu > > model patch set appear in master soon? > > or you mean each single patch in a series should be individually built > pass? then it will a huge one here: data structure changes + reference > functions. Yes, each patch should individually build and pass. That's why you should first introduce the changes to CPUID feature words (data structure and functions) and then add support for MSR features. Paolo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support " Robert Hoo 2018-08-17 3:10 ` Eduardo Habkost @ 2018-08-17 15:50 ` Paolo Bonzini 2018-08-17 15:55 ` Eduardo Habkost 1 sibling, 1 reply; 40+ messages in thread From: Paolo Bonzini @ 2018-08-17 15:50 UTC (permalink / raw) To: Robert Hoo, rth, ehabkost, thomas.lendacky Cc: qemu-devel, robert.hu, jingqi.liu On 10/08/2018 16:06, Robert Hoo wrote: > +typedef enum FeatureWordType { > + CPUID_FEATURE_WORD, > + MSR_FEATURE_WORD, > +} FeatureWordType; This enum probably should be defined with QAPI, so that it can be reused in the feature-words property: # @X86CPUFeatureWordType: # # Kinds of X86 CPU feature words # # @cpuid: A CPUID leaf # # @msr: An MSR ## { 'enum': 'X86CPUFeatureWordType', 'data': 'cpuid', 'msr' } The names will be X86_CPU_FEATURE_WORD_TYPE_CPUID and X86_CPU_FEATURE_WORD_TYPE_MSR. Thanks, Paolo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features 2018-08-17 15:50 ` Paolo Bonzini @ 2018-08-17 15:55 ` Eduardo Habkost 2018-08-17 17:34 ` Paolo Bonzini 0 siblings, 1 reply; 40+ messages in thread From: Eduardo Habkost @ 2018-08-17 15:55 UTC (permalink / raw) To: Paolo Bonzini Cc: Robert Hoo, rth, thomas.lendacky, qemu-devel, robert.hu, jingqi.liu On Fri, Aug 17, 2018 at 05:50:15PM +0200, Paolo Bonzini wrote: > On 10/08/2018 16:06, Robert Hoo wrote: > > +typedef enum FeatureWordType { > > + CPUID_FEATURE_WORD, > > + MSR_FEATURE_WORD, > > +} FeatureWordType; > > This enum probably should be defined with QAPI, so that it can be reused > in the feature-words property: > > # @X86CPUFeatureWordType: > # > # Kinds of X86 CPU feature words > # > # @cpuid: A CPUID leaf > # > # @msr: An MSR > ## > { 'enum': 'X86CPUFeatureWordType', > 'data': 'cpuid', 'msr' } > > The names will be X86_CPU_FEATURE_WORD_TYPE_CPUID and > X86_CPU_FEATURE_WORD_TYPE_MSR. I wouldn't like to make this an external API unless really necessary. I would rather deprecate the "feature-words" property because nobody ended up using it. -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features 2018-08-17 15:55 ` Eduardo Habkost @ 2018-08-17 17:34 ` Paolo Bonzini 2018-08-17 17:48 ` [Qemu-devel] X86CPU "feature-words" property on QEMU (was Re: [PATCH v3 1/3] x86: Data structure changes to support MSR based) features Eduardo Habkost 0 siblings, 1 reply; 40+ messages in thread From: Paolo Bonzini @ 2018-08-17 17:34 UTC (permalink / raw) To: Eduardo Habkost Cc: Robert Hoo, rth, thomas.lendacky, qemu-devel, robert.hu, jingqi.liu On 17/08/2018 17:55, Eduardo Habkost wrote: >> The names will be X86_CPU_FEATURE_WORD_TYPE_CPUID and >> X86_CPU_FEATURE_WORD_TYPE_MSR. > I wouldn't like to make this an external API unless really > necessary. I would rather deprecate the "feature-words" property > because nobody ended up using it. I think we should either remove it directly, or extend it to support MSR features. Deprecating and only supporting CPUID features is the worst of both worlds... Paolo ^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] X86CPU "feature-words" property on QEMU (was Re: [PATCH v3 1/3] x86: Data structure changes to support MSR based) features 2018-08-17 17:34 ` Paolo Bonzini @ 2018-08-17 17:48 ` Eduardo Habkost 2018-08-17 17:59 ` Paolo Bonzini 0 siblings, 1 reply; 40+ messages in thread From: Eduardo Habkost @ 2018-08-17 17:48 UTC (permalink / raw) To: Paolo Bonzini Cc: Robert Hoo, rth, thomas.lendacky, qemu-devel, robert.hu, jingqi.liu, Jiri Denemark, libvir-list On Fri, Aug 17, 2018 at 07:34:13PM +0200, Paolo Bonzini wrote: > On 17/08/2018 17:55, Eduardo Habkost wrote: > >> The names will be X86_CPU_FEATURE_WORD_TYPE_CPUID and > >> X86_CPU_FEATURE_WORD_TYPE_MSR. > > I wouldn't like to make this an external API unless really > > necessary. I would rather deprecate the "feature-words" property > > because nobody ended up using it. > > I think we should either remove it directly, or extend it to support MSR > features. Deprecating and only supporting CPUID features is the worst > of both worlds... So let's do what's necessary to remove it. But I don't think the removal of "feature-words" should block the inclusion of this series. Now, should QOM properties follow our feature deprecation policy, or they were never a supported external API and we can remove it immediately? CCing Jiri and libvir-list, because I just found that there's code on libvirt that uses it, but I don't know exactly it does with that info. -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] X86CPU "feature-words" property on QEMU (was Re: [PATCH v3 1/3] x86: Data structure changes to support MSR based) features 2018-08-17 17:48 ` [Qemu-devel] X86CPU "feature-words" property on QEMU (was Re: [PATCH v3 1/3] x86: Data structure changes to support MSR based) features Eduardo Habkost @ 2018-08-17 17:59 ` Paolo Bonzini 2018-08-17 18:10 ` Eduardo Habkost 0 siblings, 1 reply; 40+ messages in thread From: Paolo Bonzini @ 2018-08-17 17:59 UTC (permalink / raw) To: Eduardo Habkost Cc: Robert Hoo, rth, thomas.lendacky, qemu-devel, robert.hu, jingqi.liu, Jiri Denemark, libvir-list On 17/08/2018 19:48, Eduardo Habkost wrote: > So let's do what's necessary to remove it. But I don't think the > removal of "feature-words" should block the inclusion of this > series. > > Now, should QOM properties follow our feature deprecation policy, > or they were never a supported external API and we can remove it > immediately? > > CCing Jiri and libvir-list, because I just found that there's > code on libvirt that uses it, but I don't know exactly it does > with that info. It is used to check whether the host supports invtsc and pv-unhalt and avoid changing the guest ABI when migrating: see https://marc.info/?l=libvir-list&m=152445761414746&w=2 for a patch that introduces one user. I think we should extend it to MSRs, not remove it. Paolo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] X86CPU "feature-words" property on QEMU (was Re: [PATCH v3 1/3] x86: Data structure changes to support MSR based) features 2018-08-17 17:59 ` Paolo Bonzini @ 2018-08-17 18:10 ` Eduardo Habkost 0 siblings, 0 replies; 40+ messages in thread From: Eduardo Habkost @ 2018-08-17 18:10 UTC (permalink / raw) To: Paolo Bonzini Cc: Robert Hoo, rth, thomas.lendacky, qemu-devel, robert.hu, jingqi.liu, Jiri Denemark, libvir-list On Fri, Aug 17, 2018 at 07:59:40PM +0200, Paolo Bonzini wrote: > On 17/08/2018 19:48, Eduardo Habkost wrote: > > So let's do what's necessary to remove it. But I don't think the > > removal of "feature-words" should block the inclusion of this > > series. > > > > Now, should QOM properties follow our feature deprecation policy, > > or they were never a supported external API and we can remove it > > immediately? > > > > CCing Jiri and libvir-list, because I just found that there's > > code on libvirt that uses it, but I don't know exactly it does > > with that info. > > It is used to check whether the host supports invtsc and pv-unhalt and > avoid changing the guest ABI when migrating: see > https://marc.info/?l=libvir-list&m=152445761414746&w=2 for a patch that > introduces one user. > > I think we should extend it to MSRs, not remove it. Well, I would prefer if libvirt simply did (e.g.) "qom-get property=invtsc" instead of fetching raw feature-words data. But if we do have an existing user, I now agree with you: let's keep it working and extend it to include MSR info too. BTW, libvirt must stop using this hardcoded QOM path: #define QOM_CPU_PATH "/machine/unattached/device[0]" It should use the "qom_path" field of "query-cpus" instead. -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-10 14:06 [Qemu-devel] [PATCH v3 0/3] x86: QEMU side support on MSR based features Robert Hoo 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support " Robert Hoo @ 2018-08-10 14:06 ` Robert Hoo 2018-08-17 13:18 ` Eduardo Habkost 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] Robert Hoo 2 siblings, 1 reply; 40+ messages in thread From: Robert Hoo @ 2018-08-10 14:06 UTC (permalink / raw) To: pbonzini, rth, ehabkost, thomas.lendacky Cc: qemu-devel, robert.hu, jingqi.liu, Robert Hoo Add kvm_get_supported_feature_msrs() to get supported MSR feature index list. Add kvm_arch_get_supported_msr_feature() to get each MSR features value. kvm_get_supported_feature_msrs() is called in kvm_arch_init(). kvm_arch_get_supported_msr_feature() is called by x86_cpu_get_supported_feature_word(). Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> --- include/sysemu/kvm.h | 2 ++ target/i386/kvm.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h index 0b64b8e..97d8d9d 100644 --- a/include/sysemu/kvm.h +++ b/include/sysemu/kvm.h @@ -463,6 +463,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension); uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function, uint32_t index, int reg); +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index); + void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len); diff --git a/target/i386/kvm.c b/target/i386/kvm.c index 9313602..70d8606 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -107,6 +107,7 @@ static int has_pit_state2; static bool has_msr_mcg_ext_ctl; static struct kvm_cpuid2 *cpuid_cache; +static struct kvm_msr_list *kvm_feature_msrs; int kvm_has_pit_state2(void) { @@ -420,6 +421,41 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, return ret; } +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index) +{ + struct { + struct kvm_msrs info; + struct kvm_msr_entry entries[1]; + } msr_data; + uint32_t ret; + + /*Check if the requested feature MSR is supported*/ + int i; + for (i = 0; i < kvm_feature_msrs->nmsrs; i++) { + if (index == kvm_feature_msrs->indices[i]) { + break; + } + } + if (i >= kvm_feature_msrs->nmsrs) { + fprintf(stderr, "Requested MSR (index= %d) is not supported.\n", index); + return 0; + } + + msr_data.info.nmsrs = 1; + msr_data.entries[0].index = index; + + ret = kvm_ioctl(s, KVM_GET_MSRS, &msr_data); + + if (ret != 1) { + fprintf(stderr, "KVM get MSR (index=0x%x) feature failed, %s\n", + index, strerror(-ret)); + exit(1); + } + + return msr_data.entries[0].data; +} + + typedef struct HWPoisonPage { ram_addr_t ram_addr; QLIST_ENTRY(HWPoisonPage) list; @@ -1238,7 +1274,45 @@ void kvm_arch_do_init_vcpu(X86CPU *cpu) env->mp_state = KVM_MP_STATE_INIT_RECEIVED; } } +static int kvm_get_supported_feature_msrs(KVMState *s) +{ + static int kvm_supported_feature_msrs; + int ret = 0; + + if (kvm_supported_feature_msrs == 0) { + struct kvm_msr_list msr_list; + + kvm_supported_feature_msrs++; + + msr_list.nmsrs = 0; + ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, &msr_list); + if (ret < 0 && ret != -E2BIG) { + return ret; + } + + assert(msr_list.nmsrs > 0); + kvm_feature_msrs = (struct kvm_msr_list *) \ + g_malloc0(sizeof(msr_list) + + msr_list.nmsrs * sizeof(msr_list.indices[0])); + if (kvm_feature_msrs == NULL) { + fprintf(stderr, "Failed to allocate space for KVM Feature MSRs" + "which has %d MSRs\n", msr_list.nmsrs); + return -1; + } + + kvm_feature_msrs->nmsrs = msr_list.nmsrs; + ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, kvm_feature_msrs); + if (ret < 0) { /*ioctl failure*/ + fprintf(stderr, "Fetch KVM feature MSRs failed: %s\n", + strerror(-ret)); + g_free(kvm_feature_msrs); + return ret; + } + } + + return 0; +} static int kvm_get_supported_msrs(KVMState *s) { static int kvm_supported_msrs; @@ -1400,6 +1474,11 @@ int kvm_arch_init(MachineState *ms, KVMState *s) return ret; } + ret = kvm_get_supported_feature_msrs(s); + if (ret < 0) { + return ret; + } + uname(&utsname); lm_capable_kernel = strcmp(utsname.machine, "x86_64") == 0; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl Robert Hoo @ 2018-08-17 13:18 ` Eduardo Habkost 2018-08-18 7:27 ` Robert Hoo 0 siblings, 1 reply; 40+ messages in thread From: Eduardo Habkost @ 2018-08-17 13:18 UTC (permalink / raw) To: Robert Hoo Cc: pbonzini, rth, thomas.lendacky, robert.hu, qemu-devel, jingqi.liu Thanks for the patch, comments below: On Fri, Aug 10, 2018 at 10:06:28PM +0800, Robert Hoo wrote: > Add kvm_get_supported_feature_msrs() to get supported MSR feature index list. > Add kvm_arch_get_supported_msr_feature() to get each MSR features value. > > kvm_get_supported_feature_msrs() is called in kvm_arch_init(). > kvm_arch_get_supported_msr_feature() is called by > x86_cpu_get_supported_feature_word(). > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > --- > include/sysemu/kvm.h | 2 ++ > target/i386/kvm.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 81 insertions(+) > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > index 0b64b8e..97d8d9d 100644 > --- a/include/sysemu/kvm.h > +++ b/include/sysemu/kvm.h > @@ -463,6 +463,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension); > > uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function, > uint32_t index, int reg); > +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index); > + > > void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len); > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index 9313602..70d8606 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -107,6 +107,7 @@ static int has_pit_state2; > static bool has_msr_mcg_ext_ctl; > > static struct kvm_cpuid2 *cpuid_cache; > +static struct kvm_msr_list *kvm_feature_msrs; I was going to suggest moving this to KVMState, but KVMState is a arch-independent struct. I guess we'll have to live with this by now. > > int kvm_has_pit_state2(void) > { > @@ -420,6 +421,41 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, > return ret; > } > > +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index) > +{ > + struct { > + struct kvm_msrs info; > + struct kvm_msr_entry entries[1]; > + } msr_data; > + uint32_t ret; > + > + /*Check if the requested feature MSR is supported*/ > + int i; > + for (i = 0; i < kvm_feature_msrs->nmsrs; i++) { > + if (index == kvm_feature_msrs->indices[i]) { > + break; > + } > + } We are duplicating the logic that's already inside KVM (checking the list of supported MSRs and returning an error). Can't we make this simpler and just remove this check? If the MSR is not supported, KVM_GET_MSRS would just return 0. > + if (i >= kvm_feature_msrs->nmsrs) { > + fprintf(stderr, "Requested MSR (index= %d) is not supported.\n", index); This error message is meaningless for QEMU users. Do we really need to print it? > + return 0; Returning 0 for MSRs that are not supported is probably OK, but we need to see this function being used, to be sure (I didn't look at all the patches in this series yet). > + } > + > + msr_data.info.nmsrs = 1; > + msr_data.entries[0].index = index; > + > + ret = kvm_ioctl(s, KVM_GET_MSRS, &msr_data); > + > + if (ret != 1) { > + fprintf(stderr, "KVM get MSR (index=0x%x) feature failed, %s\n", > + index, strerror(-ret)); > + exit(1); I'm not a fan of APIs that write to stdout/stderr or exit(), unless they are clearly just initialization functions that should never fail in normal circumstances. But if you call KVM_GET_MSRS for all MSRs at kvm_get_supported_feature_msrs() below, this function would never need to report an error. We are already going through the trouble of allocating kvm_feature_msrs in kvm_get_supported_feature_msrs() anyway. > + } > + > + return msr_data.entries[0].data; > +} > + > + > typedef struct HWPoisonPage { > ram_addr_t ram_addr; > QLIST_ENTRY(HWPoisonPage) list; > @@ -1238,7 +1274,45 @@ void kvm_arch_do_init_vcpu(X86CPU *cpu) > env->mp_state = KVM_MP_STATE_INIT_RECEIVED; > } > } Nit: please add an extra newline between functions. > +static int kvm_get_supported_feature_msrs(KVMState *s) > +{ > + static int kvm_supported_feature_msrs; > + int ret = 0; > + > + if (kvm_supported_feature_msrs == 0) { Do you really need the kvm_supported_feature_msrs variable? You could just check if kvm_feature_msrs is NULL. I also suggest doing this: if (already initialized) { return 0; } /* regular initialization code here */ to reduce one indentation level. > + struct kvm_msr_list msr_list; > + > + kvm_supported_feature_msrs++; > + > + msr_list.nmsrs = 0; > + ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, &msr_list); > + if (ret < 0 && ret != -E2BIG) { > + return ret; What if the host KVM version doesn't support KVM_GET_MSR_FEATURE_INDEX_LIST? > + } > + > + assert(msr_list.nmsrs > 0); > + kvm_feature_msrs = (struct kvm_msr_list *) \ > + g_malloc0(sizeof(msr_list) + > + msr_list.nmsrs * sizeof(msr_list.indices[0])); > + if (kvm_feature_msrs == NULL) { g_malloc0() never returns NULL on failure. See https://developer.gnome.org/glib/2.56/glib-Memory-Allocation.html#glib-Memory-Allocation.description > + fprintf(stderr, "Failed to allocate space for KVM Feature MSRs" > + "which has %d MSRs\n", msr_list.nmsrs); > + return -1; > + } > + > + kvm_feature_msrs->nmsrs = msr_list.nmsrs; > + ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, kvm_feature_msrs); > > + if (ret < 0) { /*ioctl failure*/ Small nit: the "ioctl failure" comment seems unnecessary, I would just remove it. > + fprintf(stderr, "Fetch KVM feature MSRs failed: %s\n", > + strerror(-ret)); > + g_free(kvm_feature_msrs); > + return ret; > + } > + } > + > + return 0; > +} Same as above: please add an extra newline between functions. > static int kvm_get_supported_msrs(KVMState *s) > { > static int kvm_supported_msrs; > @@ -1400,6 +1474,11 @@ int kvm_arch_init(MachineState *ms, KVMState *s) > return ret; > } > > + ret = kvm_get_supported_feature_msrs(s); > + if (ret < 0) { > + return ret; > + } > + > uname(&utsname); > lm_capable_kernel = strcmp(utsname.machine, "x86_64") == 0; > > -- > 1.8.3.1 > > -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-17 13:18 ` Eduardo Habkost @ 2018-08-18 7:27 ` Robert Hoo 2018-08-18 15:05 ` Eduardo Habkost 0 siblings, 1 reply; 40+ messages in thread From: Robert Hoo @ 2018-08-18 7:27 UTC (permalink / raw) To: Eduardo Habkost Cc: robert.hu, robert.hu, pbonzini, rth, thomas.lendacky, qemu-devel, jingqi.liu On Fri, 2018-08-17 at 10:18 -0300, Eduardo Habkost wrote: > Thanks for the patch, comments below: > > On Fri, Aug 10, 2018 at 10:06:28PM +0800, Robert Hoo wrote: > > Add kvm_get_supported_feature_msrs() to get supported MSR feature index list. > > Add kvm_arch_get_supported_msr_feature() to get each MSR features value. > > > > kvm_get_supported_feature_msrs() is called in kvm_arch_init(). > > kvm_arch_get_supported_msr_feature() is called by > > x86_cpu_get_supported_feature_word(). > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > --- > > include/sysemu/kvm.h | 2 ++ > > target/i386/kvm.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 81 insertions(+) > > > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > > index 0b64b8e..97d8d9d 100644 > > --- a/include/sysemu/kvm.h > > +++ b/include/sysemu/kvm.h > > @@ -463,6 +463,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension); > > > > uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function, > > uint32_t index, int reg); > > +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index); > > + > > > > void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len); > > > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > > index 9313602..70d8606 100644 > > --- a/target/i386/kvm.c > > +++ b/target/i386/kvm.c > > @@ -107,6 +107,7 @@ static int has_pit_state2; > > static bool has_msr_mcg_ext_ctl; > > > > static struct kvm_cpuid2 *cpuid_cache; > > +static struct kvm_msr_list *kvm_feature_msrs; > > I was going to suggest moving this to KVMState, but KVMState is a > arch-independent struct. I guess we'll have to live with this by > now. > > > > > int kvm_has_pit_state2(void) > > { > > @@ -420,6 +421,41 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, > > return ret; > > } > > > > +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index) > > +{ > > + struct { > > + struct kvm_msrs info; > > + struct kvm_msr_entry entries[1]; > > + } msr_data; > > + uint32_t ret; > > + > > + /*Check if the requested feature MSR is supported*/ > > + int i; > > + for (i = 0; i < kvm_feature_msrs->nmsrs; i++) { > > + if (index == kvm_feature_msrs->indices[i]) { > > + break; > > + } > > + } > > We are duplicating the logic that's already inside KVM (checking > the list of supported MSRs and returning an error). Can't we > make this simpler and just remove this check? If the MSR is not > supported, KVM_GET_MSRS would just return 0. Yes, seems dup. but if we remove this hunk, the kvm_get_supported_feature_msrs() is unnecessary at all. > > > + if (i >= kvm_feature_msrs->nmsrs) { > > + fprintf(stderr, "Requested MSR (index= %d) is not supported.\n", index); > > This error message is meaningless for QEMU users. Do we really > need to print it? > > > + return 0; > > Returning 0 for MSRs that are not supported is probably OK, but > we need to see this function being used, to be sure (I didn't > look at all the patches in this series yet). > > > + } > > + > > + msr_data.info.nmsrs = 1; > > + msr_data.entries[0].index = index; > > + > > + ret = kvm_ioctl(s, KVM_GET_MSRS, &msr_data); > > + > > + if (ret != 1) { > > + fprintf(stderr, "KVM get MSR (index=0x%x) feature failed, %s\n", > > + index, strerror(-ret)); > > + exit(1); > > I'm not a fan of APIs that write to stdout/stderr or exit(), > unless they are clearly just initialization functions that should > never fail in normal circumstances. > > But if you call KVM_GET_MSRS for all MSRs at > kvm_get_supported_feature_msrs() below, this function would never > need to report an error. > > We are already going through the trouble of allocating > kvm_feature_msrs in kvm_get_supported_feature_msrs() anyway. I had looked into KVM KVM_GET_MSRS handling, though less likely, still have changes copy_from/to_user() fail. Therefore I added the check and warning here, in that seldom case happens. exit() or not, I'm also not sure. Seems not necessary, while my usual programming philosophy is never let wrong goes further. For in the case of ioctl(KVM_GET_MSRS) returns failure, something goes wrong underlying, I would prefer to let user know this, rather than let it pass and goes further. > > > + } > > + > > + return msr_data.entries[0].data; > > +} > > + > > + > > typedef struct HWPoisonPage { > > ram_addr_t ram_addr; > > QLIST_ENTRY(HWPoisonPage) list; > > @@ -1238,7 +1274,45 @@ void kvm_arch_do_init_vcpu(X86CPU *cpu) > > env->mp_state = KVM_MP_STATE_INIT_RECEIVED; > > } > > } > > Nit: please add an extra newline between functions. > > > +static int kvm_get_supported_feature_msrs(KVMState *s) > > +{ > > + static int kvm_supported_feature_msrs; > > + int ret = 0; > > + > > + if (kvm_supported_feature_msrs == 0) { > > Do you really need the kvm_supported_feature_msrs variable? You > could just check if kvm_feature_msrs is NULL. > > I also suggest doing this: > > if (already initialized) { > return 0; > } > > /* regular initialization code here */ > > to reduce one indentation level. > Right. > > > + struct kvm_msr_list msr_list; > > + > > + kvm_supported_feature_msrs++; > > + > > + msr_list.nmsrs = 0; > > + ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, &msr_list); > > + if (ret < 0 && ret != -E2BIG) { > > + return ret; > > What if the host KVM version doesn't support > KVM_GET_MSR_FEATURE_INDEX_LIST? Going to add kvm_check_extension(s, KVM_CAP_GET_MSR_FEATURES) before this ioctl. if not support, return -1. (Then the kvm_init will fail and exit.) > > > + } > > + > > + assert(msr_list.nmsrs > 0); > > + kvm_feature_msrs = (struct kvm_msr_list *) \ > > + g_malloc0(sizeof(msr_list) + > > + msr_list.nmsrs * sizeof(msr_list.indices[0])); > > + if (kvm_feature_msrs == NULL) { > > g_malloc0() never returns NULL on failure. See > https://developer.gnome.org/glib/2.56/glib-Memory-Allocation.html#glib-Memory-Allocation.description > Thanks! > > + fprintf(stderr, "Failed to allocate space for KVM Feature MSRs" > > + "which has %d MSRs\n", msr_list.nmsrs); > > + return -1; > > + } > > + > > + kvm_feature_msrs->nmsrs = msr_list.nmsrs; > > + ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, kvm_feature_msrs); > > > > + if (ret < 0) { /*ioctl failure*/ > > Small nit: the "ioctl failure" comment seems unnecessary, I would > just remove it. > > > + fprintf(stderr, "Fetch KVM feature MSRs failed: %s\n", > > + strerror(-ret)); > > + g_free(kvm_feature_msrs); > > + return ret; > > + } > > + } > > + > > + return 0; > > +} > > Same as above: please add an extra newline between functions. > > > static int kvm_get_supported_msrs(KVMState *s) > > { > > static int kvm_supported_msrs; > > @@ -1400,6 +1474,11 @@ int kvm_arch_init(MachineState *ms, KVMState *s) > > return ret; > > } > > > > + ret = kvm_get_supported_feature_msrs(s); > > + if (ret < 0) { > > + return ret; > > + } > > + > > uname(&utsname); > > lm_capable_kernel = strcmp(utsname.machine, "x86_64") == 0; > > > > -- > > 1.8.3.1 > > > > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-18 7:27 ` Robert Hoo @ 2018-08-18 15:05 ` Eduardo Habkost 2018-08-23 6:28 ` Robert Hoo 0 siblings, 1 reply; 40+ messages in thread From: Eduardo Habkost @ 2018-08-18 15:05 UTC (permalink / raw) To: Robert Hoo Cc: robert.hu, pbonzini, rth, thomas.lendacky, qemu-devel, jingqi.liu On Sat, Aug 18, 2018 at 03:27:01PM +0800, Robert Hoo wrote: > On Fri, 2018-08-17 at 10:18 -0300, Eduardo Habkost wrote: > > Thanks for the patch, comments below: > > > > On Fri, Aug 10, 2018 at 10:06:28PM +0800, Robert Hoo wrote: > > > Add kvm_get_supported_feature_msrs() to get supported MSR feature index list. > > > Add kvm_arch_get_supported_msr_feature() to get each MSR features value. > > > > > > kvm_get_supported_feature_msrs() is called in kvm_arch_init(). > > > kvm_arch_get_supported_msr_feature() is called by > > > x86_cpu_get_supported_feature_word(). > > > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > > --- > > > include/sysemu/kvm.h | 2 ++ > > > target/i386/kvm.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 81 insertions(+) > > > > > > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h > > > index 0b64b8e..97d8d9d 100644 > > > --- a/include/sysemu/kvm.h > > > +++ b/include/sysemu/kvm.h > > > @@ -463,6 +463,8 @@ int kvm_vm_check_extension(KVMState *s, unsigned int extension); > > > > > > uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function, > > > uint32_t index, int reg); > > > +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index); > > > + > > > > > > void kvm_set_sigmask_len(KVMState *s, unsigned int sigmask_len); > > > > > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > > > index 9313602..70d8606 100644 > > > --- a/target/i386/kvm.c > > > +++ b/target/i386/kvm.c > > > @@ -107,6 +107,7 @@ static int has_pit_state2; > > > static bool has_msr_mcg_ext_ctl; > > > > > > static struct kvm_cpuid2 *cpuid_cache; > > > +static struct kvm_msr_list *kvm_feature_msrs; > > > > I was going to suggest moving this to KVMState, but KVMState is a > > arch-independent struct. I guess we'll have to live with this by > > now. > > > > > > > > int kvm_has_pit_state2(void) > > > { > > > @@ -420,6 +421,41 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, > > > return ret; > > > } > > > > > > +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index) > > > +{ > > > + struct { > > > + struct kvm_msrs info; > > > + struct kvm_msr_entry entries[1]; > > > + } msr_data; > > > + uint32_t ret; > > > + > > > + /*Check if the requested feature MSR is supported*/ > > > + int i; > > > + for (i = 0; i < kvm_feature_msrs->nmsrs; i++) { > > > + if (index == kvm_feature_msrs->indices[i]) { > > > + break; > > > + } > > > + } > > > > We are duplicating the logic that's already inside KVM (checking > > the list of supported MSRs and returning an error). Can't we > > make this simpler and just remove this check? If the MSR is not > > supported, KVM_GET_MSRS would just return 0. > > Yes, seems dup. but if we remove this hunk, the > kvm_get_supported_feature_msrs() is unnecessary at all. So maybe kvm_get_supported_feature_msrs() really is unnecessary? We seem to have two alternatives: * calling KVM_GET_MSRS for all MSRs only once, at kvm_get_supported_feature_msrs() (as I had suggested previously); * calling KVM_GET_MSRS for one MSR unconditionally here, but _not_ treating 0 as a fatal error. I prefer the former, but I would be OK with both alternatives. Note that we need to check for KVM capabilities in both cases. > > > > > + if (i >= kvm_feature_msrs->nmsrs) { > > > + fprintf(stderr, "Requested MSR (index= %d) is not supported.\n", index); > > > > This error message is meaningless for QEMU users. Do we really > > need to print it? > > > > > + return 0; > > > > Returning 0 for MSRs that are not supported is probably OK, but > > we need to see this function being used, to be sure (I didn't > > look at all the patches in this series yet). > > > > > + } > > > + > > > + msr_data.info.nmsrs = 1; > > > + msr_data.entries[0].index = index; > > > + > > > + ret = kvm_ioctl(s, KVM_GET_MSRS, &msr_data); > > > + > > > + if (ret != 1) { > > > + fprintf(stderr, "KVM get MSR (index=0x%x) feature failed, %s\n", > > > + index, strerror(-ret)); > > > + exit(1); > > > > I'm not a fan of APIs that write to stdout/stderr or exit(), > > unless they are clearly just initialization functions that should > > never fail in normal circumstances. > > > > But if you call KVM_GET_MSRS for all MSRs at > > kvm_get_supported_feature_msrs() below, this function would never > > need to report an error. > > > > We are already going through the trouble of allocating > > kvm_feature_msrs in kvm_get_supported_feature_msrs() anyway. > > I had looked into KVM KVM_GET_MSRS handling, though less likely, still > have changes copy_from/to_user() fail. Therefore I added the check and > warning here, in that seldom case happens. > > exit() or not, I'm also not sure. Seems not necessary, while my usual > programming philosophy is never let wrong goes further. For in the case > of ioctl(KVM_GET_MSRS) returns failure, something goes wrong underlying, > I would prefer to let user know this, rather than let it pass and goes > further. We probably have no option other than exiting if KVM_GET_MSRS returns an unexpected error. It's just that I find the code harder to review, because we need to be sure this won't terminate QEMU under normal circumstances. But if you demonstrate that all errors here are truly fatal and unexpected, calling exit() may be OK. (See the KVM_CAP_GET_MSR_FEATURES comment below for one example where exiting is not going to be OK.) Proper error reporting using Error** would be even better than exiting, but considering that none of the functions at i386/cpu.c return errors using Error**, I won't force you to do that. > > [...] > > > + struct kvm_msr_list msr_list; > > > + > > > + kvm_supported_feature_msrs++; > > > + > > > + msr_list.nmsrs = 0; > > > + ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, &msr_list); > > > + if (ret < 0 && ret != -E2BIG) { > > > + return ret; > > > > What if the host KVM version doesn't support > > KVM_GET_MSR_FEATURE_INDEX_LIST? > > Going to add kvm_check_extension(s, KVM_CAP_GET_MSR_FEATURES) before > this ioctl. if not support, return -1. (Then the kvm_init will fail and > exit.) We don't want QEMU to refuse to run if the kernel doesn't have KVM_CAP_GET_MSR_FEATURES. We can treat missing capability as equivalent to returning an empty list of MSRs. -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-18 15:05 ` Eduardo Habkost @ 2018-08-23 6:28 ` Robert Hoo 2018-08-23 17:11 ` Eduardo Habkost 0 siblings, 1 reply; 40+ messages in thread From: Robert Hoo @ 2018-08-23 6:28 UTC (permalink / raw) To: Eduardo Habkost Cc: robert.hu, robert.hu, pbonzini, rth, thomas.lendacky, qemu-devel, jingqi.liu On Sat, 2018-08-18 at 12:05 -0300, Eduardo Habkost wrote: > > > > > > > > int kvm_has_pit_state2(void) > > > > { > > > > @@ -420,6 +421,41 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function, > > > > return ret; > > > > } > > > > > > > > +uint32_t kvm_arch_get_supported_msr_feature(KVMState *s, uint32_t index) > > > > +{ > > > > + struct { > > > > + struct kvm_msrs info; > > > > + struct kvm_msr_entry entries[1]; > > > > + } msr_data; > > > > + uint32_t ret; > > > > + > > > > + /*Check if the requested feature MSR is supported*/ > > > > + int i; > > > > + for (i = 0; i < kvm_feature_msrs->nmsrs; i++) { > > > > + if (index == kvm_feature_msrs->indices[i]) { > > > > + break; > > > > + } > > > > + } > > > > > > We are duplicating the logic that's already inside KVM (checking > > > the list of supported MSRs and returning an error). Can't we > > > make this simpler and just remove this check? If the MSR is not > > > supported, KVM_GET_MSRS would just return 0. > > > > Yes, seems dup. but if we remove this hunk, the > > kvm_get_supported_feature_msrs() is unnecessary at all. > > So maybe kvm_get_supported_feature_msrs() really is unnecessary? I'll keep it, for 1) check KVM_CAP_GET_MSR_FEATURES capabilities; 2) get kvm_feature_msrs, so later kvm_arch_get_supported_msr_feature() use it to check if MSR features are supported. > > We seem to have two alternatives: > * calling KVM_GET_MSRS for all MSRs only once, at > kvm_get_supported_feature_msrs() (as I had suggested > previously); > * calling KVM_GET_MSRS for one MSR unconditionally here, but > _not_ treating 0 as a fatal error. > > I prefer the former, but I would be OK with both alternatives. > Note that we need to check for KVM capabilities in both cases. > > I'll choose the latter :). > > > > > > > + if (i >= kvm_feature_msrs->nmsrs) { > > > > + fprintf(stderr, "Requested MSR (index= %d) is not supported.\n", index); > > > > > > This error message is meaningless for QEMU users. Do we really > > > need to print it? > > > > > > > + return 0; > > > > > > Returning 0 for MSRs that are not supported is probably OK, but > > > we need to see this function being used, to be sure (I didn't > > > look at all the patches in this series yet). > > > > > > > + } > > > > + > > > > + msr_data.info.nmsrs = 1; > > > > + msr_data.entries[0].index = index; > > > > + > > > > + ret = kvm_ioctl(s, KVM_GET_MSRS, &msr_data); > > > > + > > > > + if (ret != 1) { > > > > + fprintf(stderr, "KVM get MSR (index=0x%x) feature failed, %s\n", > > > > + index, strerror(-ret)); > > > > + exit(1); > > > > > > I'm not a fan of APIs that write to stdout/stderr or exit(), > > > unless they are clearly just initialization functions that should > > > never fail in normal circumstances. > > > > > > But if you call KVM_GET_MSRS for all MSRs at > > > kvm_get_supported_feature_msrs() below, this function would never > > > need to report an error. > > > > > > We are already going through the trouble of allocating > > > kvm_feature_msrs in kvm_get_supported_feature_msrs() anyway. > > > > I had looked into KVM KVM_GET_MSRS handling, though less likely, still > > have changes copy_from/to_user() fail. Therefore I added the check and > > warning here, in that seldom case happens. > > > > exit() or not, I'm also not sure. Seems not necessary, while my usual > > programming philosophy is never let wrong goes further. For in the case > > of ioctl(KVM_GET_MSRS) returns failure, something goes wrong underlying, > > I would prefer to let user know this, rather than let it pass and goes > > further. > > We probably have no option other than exiting if KVM_GET_MSRS > returns an unexpected error. It's just that I find the code > harder to review, because we need to be sure this won't terminate > QEMU under normal circumstances. > > But if you demonstrate that all errors here are truly fatal and > unexpected, calling exit() may be OK. (See the > KVM_CAP_GET_MSR_FEATURES comment below for one example where > exiting is not going to be OK.) > > Proper error reporting using Error** would be even better than > exiting, but considering that none of the functions at i386/cpu.c > return errors using Error**, I won't force you to do that. > Thanks. > > > > > [...] > > > > + struct kvm_msr_list msr_list; > > > > + > > > > + kvm_supported_feature_msrs++; > > > > + > > > > + msr_list.nmsrs = 0; > > > > + ret = kvm_ioctl(s, KVM_GET_MSR_FEATURE_INDEX_LIST, &msr_list); > > > > + if (ret < 0 && ret != -E2BIG) { > > > > + return ret; > > > > > > What if the host KVM version doesn't support > > > KVM_GET_MSR_FEATURE_INDEX_LIST? > > > > Going to add kvm_check_extension(s, KVM_CAP_GET_MSR_FEATURES) before > > this ioctl. if not support, return -1. (Then the kvm_init will fail and > > exit.) > > We don't want QEMU to refuse to run if the kernel doesn't have > KVM_CAP_GET_MSR_FEATURES. We can treat missing capability as > equivalent to returning an empty list of MSRs. Yes. I'll let caller (kvm_arch_init) ignore the return value but a simple warning. > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-23 6:28 ` Robert Hoo @ 2018-08-23 17:11 ` Eduardo Habkost 2018-08-23 17:28 ` Paolo Bonzini 2018-08-30 4:22 ` Robert Hoo 0 siblings, 2 replies; 40+ messages in thread From: Eduardo Habkost @ 2018-08-23 17:11 UTC (permalink / raw) To: Robert Hoo Cc: robert.hu, pbonzini, rth, thomas.lendacky, qemu-devel, jingqi.liu On Thu, Aug 23, 2018 at 02:28:28PM +0800, Robert Hoo wrote: > On Sat, 2018-08-18 at 12:05 -0300, Eduardo Habkost wrote: [...] > > We don't want QEMU to refuse to run if the kernel doesn't have > > KVM_CAP_GET_MSR_FEATURES. We can treat missing capability as > > equivalent to returning an empty list of MSRs. > Yes. I'll let caller (kvm_arch_init) ignore the return value but a > simple warning. Warnings tend to be ignored and are generally a sign that QEMU isn't doing the right thing. Sometimes we have no choice, but I don't think that's the case here. As far as I can see, we have only two possibilities here: 1) The host can run a VM that behaves exactly as requested on the command-line (no warning required). 2) The host can't run the requested configuration (fatal error, not a warning). -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-23 17:11 ` Eduardo Habkost @ 2018-08-23 17:28 ` Paolo Bonzini 2018-08-23 17:36 ` Eduardo Habkost 2018-08-30 4:22 ` Robert Hoo 1 sibling, 1 reply; 40+ messages in thread From: Paolo Bonzini @ 2018-08-23 17:28 UTC (permalink / raw) To: Eduardo Habkost, Robert Hoo Cc: robert.hu, rth, thomas.lendacky, qemu-devel, jingqi.liu On 23/08/2018 19:11, Eduardo Habkost wrote: >>> We don't want QEMU to refuse to run if the kernel doesn't have >>> KVM_CAP_GET_MSR_FEATURES. We can treat missing capability as >>> equivalent to returning an empty list of MSRs. >> Yes. I'll let caller (kvm_arch_init) ignore the return value but a >> simple warning. > Warnings tend to be ignored and are generally a sign that QEMU > isn't doing the right thing. Sometimes we have no choice, but I > don't think that's the case here. > > As far as I can see, we have only two possibilities here: > 1) The host can run a VM that behaves exactly as requested on the > command-line (no warning required). > 2) The host can't run the requested configuration (fatal error, > not a warning). Right, but if KVM_CAP_GET_MSR_FEATURES is not available I guess you can assume the MSRs to be zero for everything except "-cpu host". Paolo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-23 17:28 ` Paolo Bonzini @ 2018-08-23 17:36 ` Eduardo Habkost 2018-08-23 20:23 ` Paolo Bonzini 0 siblings, 1 reply; 40+ messages in thread From: Eduardo Habkost @ 2018-08-23 17:36 UTC (permalink / raw) To: Paolo Bonzini Cc: Robert Hoo, robert.hu, rth, thomas.lendacky, qemu-devel, jingqi.liu On Thu, Aug 23, 2018 at 07:28:25PM +0200, Paolo Bonzini wrote: > On 23/08/2018 19:11, Eduardo Habkost wrote: > >>> We don't want QEMU to refuse to run if the kernel doesn't have > >>> KVM_CAP_GET_MSR_FEATURES. We can treat missing capability as > >>> equivalent to returning an empty list of MSRs. > >> Yes. I'll let caller (kvm_arch_init) ignore the return value but a > >> simple warning. > > Warnings tend to be ignored and are generally a sign that QEMU > > isn't doing the right thing. Sometimes we have no choice, but I > > don't think that's the case here. > > > > As far as I can see, we have only two possibilities here: > > 1) The host can run a VM that behaves exactly as requested on the > > command-line (no warning required). > > 2) The host can't run the requested configuration (fatal error, > > not a warning). > > Right, but if KVM_CAP_GET_MSR_FEATURES is not available I guess you can > assume the MSRs to be zero for everything except "-cpu host". Yes, that would be simplest way to implement the above. Then QEMU would refuse to run if the feature was part of the requested configuration (2), or not care at all because the feature was not set in the configuration (1). But I'm not sure I follow the suggestion to not consider the MSR to be zero on "-cpu host". If we don't need and KVM-side code to support a given MSR feature, we can enable it on all models. IF we need KVM-side code, we can't enable it even on "-cpu host". This is not different from CPUID-based features. -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-23 17:36 ` Eduardo Habkost @ 2018-08-23 20:23 ` Paolo Bonzini 2018-08-25 17:27 ` Eduardo Habkost 0 siblings, 1 reply; 40+ messages in thread From: Paolo Bonzini @ 2018-08-23 20:23 UTC (permalink / raw) To: Eduardo Habkost Cc: Robert Hoo, robert.hu, rth, thomas.lendacky, qemu-devel, jingqi.liu On 23/08/2018 19:36, Eduardo Habkost wrote: >> Right, but if KVM_CAP_GET_MSR_FEATURES is not available I guess you can >> assume the MSRs to be zero for everything except "-cpu host". > Yes, that would be simplest way to implement the above. Then > QEMU would refuse to run if the feature was part of the requested > configuration (2), or not care at all because the feature was not > set in the configuration (1). > > But I'm not sure I follow the suggestion to not consider the MSR > to be zero on "-cpu host". If we don't need and KVM-side code to > support a given MSR feature, we can enable it on all models. The case I was thinking about, is where there is no KVM-side code to support the MSR feature, but you still need the host CPU to have it. Without KVM's feature MSR support, you cannot read the host value of the MSR. My idea was that feature MSRs will default to the host value returned by KVM_GET_MSR on the /dev/kvm file descriptor, so "-cpu host" could just ignore KVM_CAP_GET_MSR_FEATURES and use KVM's default value of the MSRs. However, I guess we can just use the default value for _all_ CPU models, because in practice the only effect would be on nested VMX MSRs, and nested VMX can only be migrated on kernels that have KVM_CAP_GET_MSR_FEATURES. So in practice people using nested VMX are using "-cpu host" anyway, not "-cpu SandyBridge,+vmx". My proposalis: basically, if KVM_CAP_GET_MSR_FEATURES is unavailable: - MSR-based features would cause an error if specified on the command line; - kvm_arch_init_vcpu would skip the KVM_SET_MSR for feature MSRs completely. What do you think? Paolo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-23 20:23 ` Paolo Bonzini @ 2018-08-25 17:27 ` Eduardo Habkost 0 siblings, 0 replies; 40+ messages in thread From: Eduardo Habkost @ 2018-08-25 17:27 UTC (permalink / raw) To: Paolo Bonzini Cc: Robert Hoo, robert.hu, rth, thomas.lendacky, qemu-devel, jingqi.liu On Thu, Aug 23, 2018 at 10:23:53PM +0200, Paolo Bonzini wrote: > On 23/08/2018 19:36, Eduardo Habkost wrote: > >> Right, but if KVM_CAP_GET_MSR_FEATURES is not available I guess you can > >> assume the MSRs to be zero for everything except "-cpu host". > > Yes, that would be simplest way to implement the above. Then > > QEMU would refuse to run if the feature was part of the requested > > configuration (2), or not care at all because the feature was not > > set in the configuration (1). > > > > But I'm not sure I follow the suggestion to not consider the MSR > > to be zero on "-cpu host". If we don't need and KVM-side code to > > support a given MSR feature, we can enable it on all models. > > The case I was thinking about, is where there is no KVM-side code to > support the MSR feature, but you still need the host CPU to have it. > Without KVM's feature MSR support, you cannot read the host value of the > MSR. Don't we always need some KVM code to support the feature, even if it's just to make KVM enable the feature by default? > > My idea was that feature MSRs will default to the host value returned by > KVM_GET_MSR on the /dev/kvm file descriptor, so "-cpu host" could just > ignore KVM_CAP_GET_MSR_FEATURES and use KVM's default value of the MSRs. > > However, I guess we can just use the default value for _all_ CPU models, > because in practice the only effect would be on nested VMX MSRs, and > nested VMX can only be migrated on kernels that have > KVM_CAP_GET_MSR_FEATURES. So in practice people using nested VMX are > using "-cpu host" anyway, not "-cpu SandyBridge,+vmx". If the default is to use the host value, I see two problems: 1) we can't use the default on any migration-safe CPU model (i.e. all except "host"); 2) it will be unsafe for older QEMU versions (that don't know yet how to call KVM_SET_MSR for that MSR). If this is just about VMX caps, this shouldn't be a problem because we already treat VMX as not migration-safe. We will need to keep that in mind if we want to make VMX migration-safe in the future, though. > > My proposalis: basically, if KVM_CAP_GET_MSR_FEATURES is unavailable: > > - MSR-based features would cause an error if specified on the command line; > > - kvm_arch_init_vcpu would skip the KVM_SET_MSR for feature MSRs completely. If we already had some MSR features being enabled by default before KVM_CAP_GET_MSR_FEATURES was introduced (I didn't know we had such cases), this sense. But I don't think we should do it for new MSRs. -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-23 17:11 ` Eduardo Habkost 2018-08-23 17:28 ` Paolo Bonzini @ 2018-08-30 4:22 ` Robert Hoo 2018-08-30 18:28 ` Eduardo Habkost 1 sibling, 1 reply; 40+ messages in thread From: Robert Hoo @ 2018-08-30 4:22 UTC (permalink / raw) To: Eduardo Habkost Cc: robert.hu, robert.hu, pbonzini, rth, thomas.lendacky, qemu-devel, jingqi.liu On Thu, 2018-08-23 at 14:11 -0300, Eduardo Habkost wrote: > On Thu, Aug 23, 2018 at 02:28:28PM +0800, Robert Hoo wrote: > > On Sat, 2018-08-18 at 12:05 -0300, Eduardo Habkost wrote: > [...] > > > We don't want QEMU to refuse to run if the kernel doesn't have > > > KVM_CAP_GET_MSR_FEATURES. We can treat missing capability as > > > equivalent to returning an empty list of MSRs. > > Yes. I'll let caller (kvm_arch_init) ignore the return value but a > > simple warning. > > Warnings tend to be ignored and are generally a sign that QEMU > isn't doing the right thing. Sometimes we have no choice, but I > don't think that's the case here. > > As far as I can see, we have only two possibilities here: > 1) The host can run a VM that behaves exactly as requested on the > command-line (no warning required). > 2) The host can't run the requested configuration (fatal error, > not a warning). > I mean the kvm_arch_init() --> kvm_get_supported_feature_msrs(), when kvm_get_supported_feature_msrs() returns error, can we ignore it? The cases of kvm_get_supported_feature_msrs() returning error: 1) underlying KVM doesn't support GET_MSR_FEATURES capability. 2) error in KVM_GET_MSR_FEATURE_INDEX_LIST. given the principle you guided before, I think we can ignore above error cases, and later kvm_arch_get_supported_msr_feature() would always return 0 (of course, except those special cases like IA32_ARCH_CAPABILITIES.RSBA) ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl 2018-08-30 4:22 ` Robert Hoo @ 2018-08-30 18:28 ` Eduardo Habkost 0 siblings, 0 replies; 40+ messages in thread From: Eduardo Habkost @ 2018-08-30 18:28 UTC (permalink / raw) To: Robert Hoo Cc: robert.hu, pbonzini, rth, thomas.lendacky, qemu-devel, jingqi.liu On Thu, Aug 30, 2018 at 12:22:10PM +0800, Robert Hoo wrote: > On Thu, 2018-08-23 at 14:11 -0300, Eduardo Habkost wrote: > > On Thu, Aug 23, 2018 at 02:28:28PM +0800, Robert Hoo wrote: > > > On Sat, 2018-08-18 at 12:05 -0300, Eduardo Habkost wrote: > > [...] > > > > We don't want QEMU to refuse to run if the kernel doesn't have > > > > KVM_CAP_GET_MSR_FEATURES. We can treat missing capability as > > > > equivalent to returning an empty list of MSRs. > > > Yes. I'll let caller (kvm_arch_init) ignore the return value but a > > > simple warning. > > > > Warnings tend to be ignored and are generally a sign that QEMU > > isn't doing the right thing. Sometimes we have no choice, but I > > don't think that's the case here. > > > > As far as I can see, we have only two possibilities here: > > 1) The host can run a VM that behaves exactly as requested on the > > command-line (no warning required). > > 2) The host can't run the requested configuration (fatal error, > > not a warning). > > > I mean the kvm_arch_init() --> kvm_get_supported_feature_msrs(), when > kvm_get_supported_feature_msrs() returns error, can we ignore it? > The cases of kvm_get_supported_feature_msrs() returning error: > 1) underlying KVM doesn't support GET_MSR_FEATURES capability. > 2) error in KVM_GET_MSR_FEATURE_INDEX_LIST. > > given the principle you guided before, I think we can ignore above error > cases, and later kvm_arch_get_supported_msr_feature() would always > return 0 (of course, except those special cases like > IA32_ARCH_CAPABILITIES.RSBA) Making kvm_arch_get_supported_msr_feature() return 0 if the capability isn't available (except for RSBA) seems to be enough to ensure we do the right thing on both cases (1 and 2 above). Now, if KVM_GET_MSR_FEATURE_INDEX_LIST returns an unexpected error, it sounds like an exceptional situation that can justify a fatal error (or at least a warning). -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-08-10 14:06 [Qemu-devel] [PATCH v3 0/3] x86: QEMU side support on MSR based features Robert Hoo 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support " Robert Hoo 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl Robert Hoo @ 2018-08-10 14:06 ` Robert Hoo 2018-08-10 15:17 ` Eric Blake ` (2 more replies) 2 siblings, 3 replies; 40+ messages in thread From: Robert Hoo @ 2018-08-10 14:06 UTC (permalink / raw) To: pbonzini, rth, ehabkost, thomas.lendacky Cc: qemu-devel, robert.hu, jingqi.liu, Robert Hoo Add an util function feature_word_description(), which help construct the string describing the feature word (both CPUID and MSR types). report_unavailable_features(): add MSR_FEATURE_WORD type support. x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only. x86_cpu_get_supported_feature_word(): add MSR_FEATURE_WORD type support. x86_cpu_adjust_feat_level(): assert the requested feature must be CPUID_FEATURE_WORD type. Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> --- target/i386/cpu.c | 77 +++++++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 19 deletions(-) diff --git a/target/i386/cpu.c b/target/i386/cpu.c index f7c70d9..21ed599 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -3024,21 +3024,51 @@ static const TypeInfo host_x86_cpu_type_info = { #endif +/* +*caller should have input str no less than 64 byte length. +*/ +#define FEATURE_WORD_DESCPTION_LEN 64 +static int feature_word_description(char str[], FeatureWordInfo *f, + uint32_t bit) +{ + int ret; + + assert(f->type == CPUID_FEATURE_WORD || + f->type == MSR_FEATURE_WORD); + switch (f->type) { + case CPUID_FEATURE_WORD: + { + const char *reg = get_register_name_32(f->cpuid.reg); + assert(reg); + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, + "CPUID.%02XH:%s%s%s [bit %d]", + f->cpuid.eax, reg, + f->feat_names[bit] ? "." : "", + f->feat_names[bit] ? f->feat_names[bit] : "", bit); + break; + } + case MSR_FEATURE_WORD: + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, + "MSR(%xH).%s [bit %d]", + f->msr.index, + f->feat_names[bit] ? f->feat_names[bit] : "", bit); + break; + } + return ret > 0; +} + static void report_unavailable_features(FeatureWord w, uint32_t mask) { FeatureWordInfo *f = &feature_word_info[w]; int i; + char feat_word_dscrp_str[FEATURE_WORD_DESCPTION_LEN]; 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]", + feature_word_description(feat_word_dscrp_str, f, i); + warn_report("%s doesn't support requested feature: %s", accel_uses_host_cpuid() ? "host" : "TCG", - f->cpuid_eax, reg, - f->feat_names[i] ? "." : "", - f->feat_names[i] ? f->feat_names[i] : "", i); + feat_word_dscrp_str); } } } @@ -3276,17 +3306,17 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v, { uint32_t *array = (uint32_t *)opaque; FeatureWord w; - X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { }; - X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { }; + X86CPUFeatureWordInfo word_infos[FEATURE_WORDS_NUM_CPUID] = { }; + X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS_NUM_CPUID] = { }; X86CPUFeatureWordInfoList *list = NULL; - for (w = 0; w < FEATURE_WORDS; w++) { + for (w = 0; w < FEATURE_WORDS_NUM_CPUID; w++) { FeatureWordInfo *wi = &feature_word_info[w]; 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 */ @@ -3659,12 +3689,20 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, bool migratable_only) { FeatureWordInfo *wi = &feature_word_info[w]; - uint32_t r; + uint32_t r = 0; 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); + break; + case MSR_FEATURE_WORD: + r = kvm_arch_get_supported_msr_feature(kvm_state, + wi->msr.index); + break; + } } else if (hvf_enabled()) { r = hvf_get_supported_cpuid(wi->cpuid_eax, wi->cpuid_ecx, @@ -4732,9 +4770,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; } -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] Robert Hoo @ 2018-08-10 15:17 ` Eric Blake 2018-08-14 10:06 ` Robert Hoo 2018-08-17 13:28 ` Eduardo Habkost 2018-08-17 15:52 ` Paolo Bonzini 2 siblings, 1 reply; 40+ messages in thread From: Eric Blake @ 2018-08-10 15:17 UTC (permalink / raw) To: Robert Hoo, pbonzini, rth, ehabkost, thomas.lendacky Cc: robert.hu, qemu-devel, jingqi.liu On 08/10/2018 09:06 AM, Robert Hoo wrote: In the subject: s/funcitons/functions/ Also, it may be worth using a topic prefix (most of our commit messages resemble: topic: Description of patch to make it easier to spot patches by topic). > Add an util function feature_word_description(), which help construct the string s/an util/a util/ s/help/helps/ > describing the feature word (both CPUID and MSR types). > > report_unavailable_features(): add MSR_FEATURE_WORD type support. > x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only. > x86_cpu_get_supported_feature_word(): add MSR_FEATURE_WORD type support. > x86_cpu_adjust_feat_level(): assert the requested feature must be > CPUID_FEATURE_WORD type. > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > --- > target/i386/cpu.c | 77 +++++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 58 insertions(+), 19 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index f7c70d9..21ed599 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -3024,21 +3024,51 @@ static const TypeInfo host_x86_cpu_type_info = { > > #endif > > +/* > +*caller should have input str no less than 64 byte length. > +*/ > +#define FEATURE_WORD_DESCPTION_LEN 64 s/DESCPTION/DESCRIPTION/ > +static int feature_word_description(char str[], FeatureWordInfo *f, > + uint32_t bit) > +{ Prone to buffer overflow if the caller doesn't pass in the length. Would it be better to just g_strdup_printf() into a malloc'd result instead of trying to snprintf()'ing into a buffer that you hope the caller sized large enough? > + int ret; > + > + assert(f->type == CPUID_FEATURE_WORD || > + f->type == MSR_FEATURE_WORD); > + switch (f->type) { > + case CPUID_FEATURE_WORD: > + { > + const char *reg = get_register_name_32(f->cpuid.reg); > + assert(reg); > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > + "CPUID.%02XH:%s%s%s [bit %d]", > + f->cpuid.eax, reg, > + f->feat_names[bit] ? "." : "", > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); > + break; > + } > + case MSR_FEATURE_WORD: > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > + "MSR(%xH).%s [bit %d]", > + f->msr.index, > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); > + break; > + } -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-08-10 15:17 ` Eric Blake @ 2018-08-14 10:06 ` Robert Hoo 0 siblings, 0 replies; 40+ messages in thread From: Robert Hoo @ 2018-08-14 10:06 UTC (permalink / raw) To: Eric Blake, pbonzini, rth, ehabkost Cc: robert.hu, robert.hu, thomas.lendacky, qemu-devel, jingqi.liu On Fri, 2018-08-10 at 10:17 -0500, Eric Blake wrote: > On 08/10/2018 09:06 AM, Robert Hoo wrote: > > In the subject: s/funcitons/functions/ > > Also, it may be worth using a topic prefix (most of our commit messages > resemble: > > topic: Description of patch > > to make it easier to spot patches by topic). > > > Add an util function feature_word_description(), which help construct the string > > s/an util/a util/ > s/help/helps/ > Thanks Eric, will fix these typos in v2. > > describing the feature word (both CPUID and MSR types). > > > > report_unavailable_features(): add MSR_FEATURE_WORD type support. > > x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only. > > x86_cpu_get_supported_feature_word(): add MSR_FEATURE_WORD type support. > > x86_cpu_adjust_feat_level(): assert the requested feature must be > > CPUID_FEATURE_WORD type. > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > --- > > target/i386/cpu.c | 77 +++++++++++++++++++++++++++++++++++++++++-------------- > > 1 file changed, 58 insertions(+), 19 deletions(-) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index f7c70d9..21ed599 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -3024,21 +3024,51 @@ static const TypeInfo host_x86_cpu_type_info = { > > > > #endif > > > > +/* > > +*caller should have input str no less than 64 byte length. > > +*/ > > +#define FEATURE_WORD_DESCPTION_LEN 64 > > s/DESCPTION/DESCRIPTION/ > > > +static int feature_word_description(char str[], FeatureWordInfo *f, > > + uint32_t bit) > > +{ > > Prone to buffer overflow if the caller doesn't pass in the length. > Would it be better to just g_strdup_printf() into a malloc'd result > instead of trying to snprintf()'ing into a buffer that you hope the > caller sized large enough? > Oh, wasn't aware of such good util functions. Sure I'd like to use them. Is there some web page introducing them? Eduardo/Paolo, do you have more comments? > > + int ret; > > + > > + assert(f->type == CPUID_FEATURE_WORD || > > + f->type == MSR_FEATURE_WORD); > > + switch (f->type) { > > + case CPUID_FEATURE_WORD: > > + { > > + const char *reg = get_register_name_32(f->cpuid.reg); > > + assert(reg); > > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > > + "CPUID.%02XH:%s%s%s [bit %d]", > > + f->cpuid.eax, reg, > > + f->feat_names[bit] ? "." : "", > > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); > > + break; > > + } > > + case MSR_FEATURE_WORD: > > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > > + "MSR(%xH).%s [bit %d]", > > + f->msr.index, > > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); > > + break; > > + } > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] Robert Hoo 2018-08-10 15:17 ` Eric Blake @ 2018-08-17 13:28 ` Eduardo Habkost 2018-08-18 9:01 ` Robert Hoo 2018-08-17 15:52 ` Paolo Bonzini 2 siblings, 1 reply; 40+ messages in thread From: Eduardo Habkost @ 2018-08-17 13:28 UTC (permalink / raw) To: Robert Hoo Cc: pbonzini, rth, thomas.lendacky, robert.hu, qemu-devel, jingqi.liu On Fri, Aug 10, 2018 at 10:06:29PM +0800, Robert Hoo wrote: > Add an util function feature_word_description(), which help construct the string > describing the feature word (both CPUID and MSR types). > > report_unavailable_features(): add MSR_FEATURE_WORD type support. > x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only. > x86_cpu_get_supported_feature_word(): add MSR_FEATURE_WORD type support. > x86_cpu_adjust_feat_level(): assert the requested feature must be > CPUID_FEATURE_WORD type. > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > --- > target/i386/cpu.c | 77 +++++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 58 insertions(+), 19 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index f7c70d9..21ed599 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -3024,21 +3024,51 @@ static const TypeInfo host_x86_cpu_type_info = { > > #endif > > +/* > +*caller should have input str no less than 64 byte length. > +*/ > +#define FEATURE_WORD_DESCPTION_LEN 64 > +static int feature_word_description(char str[], FeatureWordInfo *f, > + uint32_t bit) I agree with Eric: g_strup_printf() would be much simpler here. > +{ > + int ret; > + > + assert(f->type == CPUID_FEATURE_WORD || > + f->type == MSR_FEATURE_WORD); > + switch (f->type) { > + case CPUID_FEATURE_WORD: > + { > + const char *reg = get_register_name_32(f->cpuid.reg); > + assert(reg); > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > + "CPUID.%02XH:%s%s%s [bit %d]", > + f->cpuid.eax, reg, > + f->feat_names[bit] ? "." : "", > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); > + break; > + } > + case MSR_FEATURE_WORD: > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > + "MSR(%xH).%s [bit %d]", > + f->msr.index, > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); What about leaving the (f->feat_names[bit] ? ... : ...) part in report_unavailable_features() and just make this function return "CPUID[...]" or "MSR[...]"? > + break; > + } > + return ret > 0; > +} > + > static void report_unavailable_features(FeatureWord w, uint32_t mask) > { > FeatureWordInfo *f = &feature_word_info[w]; > int i; > + char feat_word_dscrp_str[FEATURE_WORD_DESCPTION_LEN]; > > 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]", > + feature_word_description(feat_word_dscrp_str, f, i); > + warn_report("%s doesn't support requested feature: %s", > accel_uses_host_cpuid() ? "host" : "TCG", > - f->cpuid_eax, reg, > - f->feat_names[i] ? "." : "", > - f->feat_names[i] ? f->feat_names[i] : "", i); > + feat_word_dscrp_str); > } > } > } > @@ -3276,17 +3306,17 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v, > { > uint32_t *array = (uint32_t *)opaque; > FeatureWord w; > - X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { }; > - X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { }; > + X86CPUFeatureWordInfo word_infos[FEATURE_WORDS_NUM_CPUID] = { }; > + X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS_NUM_CPUID] = { }; > X86CPUFeatureWordInfoList *list = NULL; > > - for (w = 0; w < FEATURE_WORDS; w++) { > + for (w = 0; w < FEATURE_WORDS_NUM_CPUID; w++) { > FeatureWordInfo *wi = &feature_word_info[w]; > 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; Looks OK, but I would add an assert(wi->type == CPUID_FEATURE_WORD) on every block of code that uses the wi->cpuid field. I would also get rid of FEATURE_WORDS_NUM_CPUID and FEATURE_WORDS_FIRST_MSR to reduce the chance of future mistakes. We can use FEATURE_WORDS in this loop and just check wi->type on each iteration. > qwi->features = array[w]; > > /* List will be in reverse order, but order shouldn't matter */ > @@ -3659,12 +3689,20 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, > bool migratable_only) > { > FeatureWordInfo *wi = &feature_word_info[w]; > - uint32_t r; > + uint32_t r = 0; > > 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); > + break; > + case MSR_FEATURE_WORD: > + r = kvm_arch_get_supported_msr_feature(kvm_state, > + wi->msr.index); > + break; I'm not sure this is correct in the case of IA32_ARCH_CAPABILITIES.RSBA: we do want to be able to set RSBA even if the host doesn't have it set. Probably you need to handle IA32_ARCH_CAPABILITIES.RSBA as a special case inside kvm_arch_get_supported_msr_feature(). (And we do want to warn the user in some way if RSBA is set in the host but not in the guest. But that's a separate problem.) > + } > } else if (hvf_enabled()) { > r = hvf_get_supported_cpuid(wi->cpuid_eax, > wi->cpuid_ecx, > @@ -4732,9 +4770,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; > } > -- > 1.8.3.1 > > -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-08-17 13:28 ` Eduardo Habkost @ 2018-08-18 9:01 ` Robert Hoo 2018-08-18 15:10 ` Eduardo Habkost 0 siblings, 1 reply; 40+ messages in thread From: Robert Hoo @ 2018-08-18 9:01 UTC (permalink / raw) To: Eduardo Habkost Cc: robert.hu, robert.hu, pbonzini, rth, thomas.lendacky, qemu-devel, jingqi.liu On Fri, 2018-08-17 at 10:28 -0300, Eduardo Habkost wrote: > On Fri, Aug 10, 2018 at 10:06:29PM +0800, Robert Hoo wrote: > > Add an util function feature_word_description(), which help construct the string > > describing the feature word (both CPUID and MSR types). > > > > report_unavailable_features(): add MSR_FEATURE_WORD type support. > > x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only. > > x86_cpu_get_supported_feature_word(): add MSR_FEATURE_WORD type support. > > x86_cpu_adjust_feat_level(): assert the requested feature must be > > CPUID_FEATURE_WORD type. > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > --- > > target/i386/cpu.c | 77 +++++++++++++++++++++++++++++++++++++++++-------------- > > 1 file changed, 58 insertions(+), 19 deletions(-) > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > index f7c70d9..21ed599 100644 > > --- a/target/i386/cpu.c > > +++ b/target/i386/cpu.c > > @@ -3024,21 +3024,51 @@ static const TypeInfo host_x86_cpu_type_info = { > > > > #endif > > > > +/* > > +*caller should have input str no less than 64 byte length. > > +*/ > > +#define FEATURE_WORD_DESCPTION_LEN 64 > > +static int feature_word_description(char str[], FeatureWordInfo *f, > > + uint32_t bit) > > I agree with Eric: g_strup_printf() would be much simpler here. > 1 question: I think caller should g_free() the returned str after it warn_report(), right? > > +{ > > + int ret; > > + > > + assert(f->type == CPUID_FEATURE_WORD || > > + f->type == MSR_FEATURE_WORD); > > + switch (f->type) { > > + case CPUID_FEATURE_WORD: > > + { > > + const char *reg = get_register_name_32(f->cpuid.reg); > > + assert(reg); > > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > > + "CPUID.%02XH:%s%s%s [bit %d]", > > + f->cpuid.eax, reg, > > + f->feat_names[bit] ? "." : "", > > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); > > + break; > > + } > > + case MSR_FEATURE_WORD: > > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > > + "MSR(%xH).%s [bit %d]", > > + f->msr.index, > > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); > > What about leaving the (f->feat_names[bit] ? ... : ...) part > in report_unavailable_features() and just make this function > return "CPUID[...]" or "MSR[...]"? > > > > + break; > > + } > > + return ret > 0; > > +} > > + > > static void report_unavailable_features(FeatureWord w, uint32_t mask) > > { > > FeatureWordInfo *f = &feature_word_info[w]; > > int i; > > + char feat_word_dscrp_str[FEATURE_WORD_DESCPTION_LEN]; > > > > 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]", > > + feature_word_description(feat_word_dscrp_str, f, i); > > + warn_report("%s doesn't support requested feature: %s", > > accel_uses_host_cpuid() ? "host" : "TCG", > > - f->cpuid_eax, reg, > > - f->feat_names[i] ? "." : "", > > - f->feat_names[i] ? f->feat_names[i] : "", i); > > + feat_word_dscrp_str); > > } > > } > > } > > @@ -3276,17 +3306,17 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v, > > { > > uint32_t *array = (uint32_t *)opaque; > > FeatureWord w; > > - X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { }; > > - X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { }; > > + X86CPUFeatureWordInfo word_infos[FEATURE_WORDS_NUM_CPUID] = { }; > > + X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS_NUM_CPUID] = { }; > > X86CPUFeatureWordInfoList *list = NULL; > > > > - for (w = 0; w < FEATURE_WORDS; w++) { > > + for (w = 0; w < FEATURE_WORDS_NUM_CPUID; w++) { > > FeatureWordInfo *wi = &feature_word_info[w]; > > 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; > > Looks OK, but I would add an > assert(wi->type == CPUID_FEATURE_WORD) on every block of code > that uses the wi->cpuid field. > > I would also get rid of FEATURE_WORDS_NUM_CPUID and > FEATURE_WORDS_FIRST_MSR to reduce the chance of future mistakes. > We can use FEATURE_WORDS in this loop and just check wi->type on > each iteration. > > > qwi->features = array[w]; > > > > /* List will be in reverse order, but order shouldn't matter */ > > @@ -3659,12 +3689,20 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, > > bool migratable_only) > > { > > FeatureWordInfo *wi = &feature_word_info[w]; > > - uint32_t r; > > + uint32_t r = 0; > > > > 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); > > + break; > > + case MSR_FEATURE_WORD: > > + r = kvm_arch_get_supported_msr_feature(kvm_state, > > + wi->msr.index); > > + break; > > I'm not sure this is correct in the case of > IA32_ARCH_CAPABILITIES.RSBA: we do want to be able to set RSBA > even if the host doesn't have it set. > > Probably you need to handle IA32_ARCH_CAPABILITIES.RSBA as a > special case inside kvm_arch_get_supported_msr_feature(). > > (And we do want to warn the user in some way if RSBA is set in > the host but not in the guest. But that's a separate problem.) The first part is easy to do. the latter, "to warn the user in some way if RSBA is set in the host but not in the guest", in kvm_arch_get_supported_msr_feature(), how can I know if guest has enabled IA32_ARCH_CAPABILITIES.RSBA or not? or you mean check in some other place, where is it? > > > + } > > } else if (hvf_enabled()) { > > r = hvf_get_supported_cpuid(wi->cpuid_eax, > > wi->cpuid_ecx, > > @@ -4732,9 +4770,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; > > } > > -- > > 1.8.3.1 > > > > > ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-08-18 9:01 ` Robert Hoo @ 2018-08-18 15:10 ` Eduardo Habkost 0 siblings, 0 replies; 40+ messages in thread From: Eduardo Habkost @ 2018-08-18 15:10 UTC (permalink / raw) To: Robert Hoo Cc: robert.hu, pbonzini, rth, thomas.lendacky, qemu-devel, jingqi.liu On Sat, Aug 18, 2018 at 05:01:45PM +0800, Robert Hoo wrote: > On Fri, 2018-08-17 at 10:28 -0300, Eduardo Habkost wrote: > > On Fri, Aug 10, 2018 at 10:06:29PM +0800, Robert Hoo wrote: > > > Add an util function feature_word_description(), which help construct the string > > > describing the feature word (both CPUID and MSR types). > > > > > > report_unavailable_features(): add MSR_FEATURE_WORD type support. > > > x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only. > > > x86_cpu_get_supported_feature_word(): add MSR_FEATURE_WORD type support. > > > x86_cpu_adjust_feat_level(): assert the requested feature must be > > > CPUID_FEATURE_WORD type. > > > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > > --- > > > target/i386/cpu.c | 77 +++++++++++++++++++++++++++++++++++++++++-------------- > > > 1 file changed, 58 insertions(+), 19 deletions(-) > > > > > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > index f7c70d9..21ed599 100644 > > > --- a/target/i386/cpu.c > > > +++ b/target/i386/cpu.c > > > @@ -3024,21 +3024,51 @@ static const TypeInfo host_x86_cpu_type_info = { > > > > > > #endif > > > > > > +/* > > > +*caller should have input str no less than 64 byte length. > > > +*/ > > > +#define FEATURE_WORD_DESCPTION_LEN 64 > > > +static int feature_word_description(char str[], FeatureWordInfo *f, > > > + uint32_t bit) > > > > I agree with Eric: g_strup_printf() would be much simpler here. > > > 1 question: I think caller should g_free() the returned str after it > warn_report(), right? > > > +{ > > > + int ret; > > > + > > > + assert(f->type == CPUID_FEATURE_WORD || > > > + f->type == MSR_FEATURE_WORD); > > > + switch (f->type) { > > > + case CPUID_FEATURE_WORD: > > > + { > > > + const char *reg = get_register_name_32(f->cpuid.reg); > > > + assert(reg); > > > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > > > + "CPUID.%02XH:%s%s%s [bit %d]", > > > + f->cpuid.eax, reg, > > > + f->feat_names[bit] ? "." : "", > > > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); > > > + break; > > > + } > > > + case MSR_FEATURE_WORD: > > > + ret = snprintf(str, FEATURE_WORD_DESCPTION_LEN, > > > + "MSR(%xH).%s [bit %d]", > > > + f->msr.index, > > > + f->feat_names[bit] ? f->feat_names[bit] : "", bit); > > > > What about leaving the (f->feat_names[bit] ? ... : ...) part > > in report_unavailable_features() and just make this function > > return "CPUID[...]" or "MSR[...]"? > > > > > > > + break; > > > + } > > > + return ret > 0; > > > +} > > > + > > > static void report_unavailable_features(FeatureWord w, uint32_t mask) > > > { > > > FeatureWordInfo *f = &feature_word_info[w]; > > > int i; > > > + char feat_word_dscrp_str[FEATURE_WORD_DESCPTION_LEN]; > > > > > > 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]", > > > + feature_word_description(feat_word_dscrp_str, f, i); > > > + warn_report("%s doesn't support requested feature: %s", > > > accel_uses_host_cpuid() ? "host" : "TCG", > > > - f->cpuid_eax, reg, > > > - f->feat_names[i] ? "." : "", > > > - f->feat_names[i] ? f->feat_names[i] : "", i); > > > + feat_word_dscrp_str); > > > } > > > } > > > } > > > @@ -3276,17 +3306,17 @@ static void x86_cpu_get_feature_words(Object *obj, Visitor *v, > > > { > > > uint32_t *array = (uint32_t *)opaque; > > > FeatureWord w; > > > - X86CPUFeatureWordInfo word_infos[FEATURE_WORDS] = { }; > > > - X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS] = { }; > > > + X86CPUFeatureWordInfo word_infos[FEATURE_WORDS_NUM_CPUID] = { }; > > > + X86CPUFeatureWordInfoList list_entries[FEATURE_WORDS_NUM_CPUID] = { }; > > > X86CPUFeatureWordInfoList *list = NULL; > > > > > > - for (w = 0; w < FEATURE_WORDS; w++) { > > > + for (w = 0; w < FEATURE_WORDS_NUM_CPUID; w++) { > > > FeatureWordInfo *wi = &feature_word_info[w]; > > > 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; > > > > Looks OK, but I would add an > > assert(wi->type == CPUID_FEATURE_WORD) on every block of code > > that uses the wi->cpuid field. > > > > I would also get rid of FEATURE_WORDS_NUM_CPUID and > > FEATURE_WORDS_FIRST_MSR to reduce the chance of future mistakes. > > We can use FEATURE_WORDS in this loop and just check wi->type on > > each iteration. > > > > > qwi->features = array[w]; > > > > > > /* List will be in reverse order, but order shouldn't matter */ > > > @@ -3659,12 +3689,20 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w, > > > bool migratable_only) > > > { > > > FeatureWordInfo *wi = &feature_word_info[w]; > > > - uint32_t r; > > > + uint32_t r = 0; > > > > > > 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); > > > + break; > > > + case MSR_FEATURE_WORD: > > > + r = kvm_arch_get_supported_msr_feature(kvm_state, > > > + wi->msr.index); > > > + break; > > > > I'm not sure this is correct in the case of > > IA32_ARCH_CAPABILITIES.RSBA: we do want to be able to set RSBA > > even if the host doesn't have it set. > > > > Probably you need to handle IA32_ARCH_CAPABILITIES.RSBA as a > > special case inside kvm_arch_get_supported_msr_feature(). > > > > (And we do want to warn the user in some way if RSBA is set in > > the host but not in the guest. But that's a separate problem.) > > The first part is easy to do. the latter, "to warn the user in some way > if RSBA is set in the host but not in the guest", in > kvm_arch_get_supported_msr_feature(), how can I know if guest has > enabled IA32_ARCH_CAPABILITIES.RSBA or not? or you mean check in some > other place, where is it? Probably X86CPU realize function is an obvious place, but the main problem is that we don't have an appropriate channel to report that warning except stderr (making the warning useless for users that are not running QEMU directly). We also need to ensure QEMU is doing its job before it complains to the user: before printing a warning about unsafe configuration, we should try to make QEMU enable safe configuration by default. -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] Robert Hoo 2018-08-10 15:17 ` Eric Blake 2018-08-17 13:28 ` Eduardo Habkost @ 2018-08-17 15:52 ` Paolo Bonzini 2018-08-18 12:53 ` Robert Hoo 2018-09-05 5:47 ` Robert Hoo 2 siblings, 2 replies; 40+ messages in thread From: Paolo Bonzini @ 2018-08-17 15:52 UTC (permalink / raw) To: Robert Hoo, rth, ehabkost, thomas.lendacky Cc: qemu-devel, robert.hu, jingqi.liu On 10/08/2018 16:06, Robert Hoo wrote: > x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only. This should also grow support for MSR feature words. My suggestion is that you add another patch after patch 1 that expands the definition of X86CPUFeatureWordInfo like this, and adjusts x86_cpu_get_feature_words() to match the new C structs. Then this patch can add MSR feature support somewhat easily. The QAPI definitions would then look like this: diff --git a/qapi/misc.json b/qapi/misc.json index d450cfef21..eae9243976 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -2663,9 +2663,9 @@ 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } ## -# @X86CPUFeatureWordInfo: +# @X86CPUIDFeatureWordInfo: # -# Information about a X86 CPU feature word +# Information about an X86 CPUID feature word # # @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word # @@ -2678,12 +2690,45 @@ # # Since: 1.5 ## -{ 'struct': 'X86CPUFeatureWordInfo', +{ 'struct': 'X86CPUIDFeatureWordInfo', 'data': { 'cpuid-input-eax': 'int', '*cpuid-input-ecx': 'int', 'cpuid-register': 'X86CPURegister32', 'features': 'int' } } +## +# @X86MSRFeatureWordInfo: +# +# Information about an X86 MSR feature word +# +# @index: Index of the model specific register +# +# @cpuid-feature: CPUID feature name that communicates the existance of the MSR +# +# @features: value of output register, containing the feature bits +# +# Since: 3.1 +## +{ 'struct': 'X86MSRFeatureWordInfo', + 'data': { 'index': 'int', + 'cpuid-feature': 'str', + 'features': 'int' } } + +## +# @X86CPUFeatureWordInfo: +# +# A discriminated record of X86 CPU feature words +# +# Since: 3.1 +## + +{ 'union': 'X86CPUFeatureWordInfo', + 'base': { 'type': 'X86CPUFeatureWordType' }, + 'discriminator': 'type', + 'data': { + 'cpuid': 'X86CPUIDFeatureWordInfo', + 'msr': 'X86MSRFeatureWordInfo' }} + ## # @DummyForceArrays: # I'm not sure if the cpuid-feature field is useful for libvirt and other management applications. Eduardo, what do you think? Thanks, Paolo ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-08-17 15:52 ` Paolo Bonzini @ 2018-08-18 12:53 ` Robert Hoo 2018-09-05 5:47 ` Robert Hoo 1 sibling, 0 replies; 40+ messages in thread From: Robert Hoo @ 2018-08-18 12:53 UTC (permalink / raw) To: Paolo Bonzini Cc: robert.hu, robert.hu, rth, ehabkost, thomas.lendacky, qemu-devel, jingqi.liu On Fri, 2018-08-17 at 17:52 +0200, Paolo Bonzini wrote: > On 10/08/2018 16:06, Robert Hoo wrote: > > x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only. > > This should also grow support for MSR feature words. > > My suggestion is that you add another patch after patch 1 that expands > the definition of X86CPUFeatureWordInfo like this, and adjusts > x86_cpu_get_feature_words() to match the new C structs. Then this > patch can add MSR feature support somewhat easily. > > The QAPI definitions would then look like this: I'm not familiar with json language. Can someone else compose this part? > > diff --git a/qapi/misc.json b/qapi/misc.json > index d450cfef21..eae9243976 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -2663,9 +2663,9 @@ > 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } > > ## > -# @X86CPUFeatureWordInfo: > +# @X86CPUIDFeatureWordInfo: > # > -# Information about a X86 CPU feature word > +# Information about an X86 CPUID feature word > # > # @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word > # > @@ -2678,12 +2690,45 @@ > # > # Since: 1.5 > ## > -{ 'struct': 'X86CPUFeatureWordInfo', > +{ 'struct': 'X86CPUIDFeatureWordInfo', > 'data': { 'cpuid-input-eax': 'int', > '*cpuid-input-ecx': 'int', > 'cpuid-register': 'X86CPURegister32', > 'features': 'int' } } > > +## > +# @X86MSRFeatureWordInfo: > +# > +# Information about an X86 MSR feature word > +# > +# @index: Index of the model specific register > +# > +# @cpuid-feature: CPUID feature name that communicates the existance of the MSR > +# > +# @features: value of output register, containing the feature bits > +# > +# Since: 3.1 > +## > +{ 'struct': 'X86MSRFeatureWordInfo', > + 'data': { 'index': 'int', > + 'cpuid-feature': 'str', > + 'features': 'int' } } > + > +## > +# @X86CPUFeatureWordInfo: > +# > +# A discriminated record of X86 CPU feature words > +# > +# Since: 3.1 > +## > + > +{ 'union': 'X86CPUFeatureWordInfo', > + 'base': { 'type': 'X86CPUFeatureWordType' }, > + 'discriminator': 'type', > + 'data': { > + 'cpuid': 'X86CPUIDFeatureWordInfo', > + 'msr': 'X86MSRFeatureWordInfo' }} > + > ## > # @DummyForceArrays: > # > > I'm not sure if the cpuid-feature field is useful for libvirt and > other management applications. Eduardo, what do you think? > > Thanks, > > Paolo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-08-17 15:52 ` Paolo Bonzini 2018-08-18 12:53 ` Robert Hoo @ 2018-09-05 5:47 ` Robert Hoo 2018-09-05 14:10 ` Eduardo Habkost 1 sibling, 1 reply; 40+ messages in thread From: Robert Hoo @ 2018-09-05 5:47 UTC (permalink / raw) To: Paolo Bonzini Cc: robert.hu, robert.hu, rth, ehabkost, thomas.lendacky, qemu-devel, jingqi.liu On Fri, 2018-08-17 at 17:52 +0200, Paolo Bonzini wrote: > On 10/08/2018 16:06, Robert Hoo wrote: > > x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only. > > This should also grow support for MSR feature words. > > My suggestion is that you add another patch after patch 1 that expands > the definition of X86CPUFeatureWordInfo like this, and adjusts > x86_cpu_get_feature_words() to match the new C structs. Then this > patch can add MSR feature support somewhat easily. > > The QAPI definitions would then look like this: > > diff --git a/qapi/misc.json b/qapi/misc.json > index d450cfef21..eae9243976 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -2663,9 +2663,9 @@ > 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } > > ## > -# @X86CPUFeatureWordInfo: > +# @X86CPUIDFeatureWordInfo: > # > -# Information about a X86 CPU feature word > +# Information about an X86 CPUID feature word > # > # @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word > # > @@ -2678,12 +2690,45 @@ > # > # Since: 1.5 > ## > -{ 'struct': 'X86CPUFeatureWordInfo', > +{ 'struct': 'X86CPUIDFeatureWordInfo', > 'data': { 'cpuid-input-eax': 'int', > '*cpuid-input-ecx': 'int', > 'cpuid-register': 'X86CPURegister32', > 'features': 'int' } } > > +## > +# @X86MSRFeatureWordInfo: > +# > +# Information about an X86 MSR feature word > +# > +# @index: Index of the model specific register > +# > +# @cpuid-feature: CPUID feature name that communicates the existance of the MSR > +# > +# @features: value of output register, containing the feature bits > +# > +# Since: 3.1 > +## > +{ 'struct': 'X86MSRFeatureWordInfo', > + 'data': { 'index': 'int', > + 'cpuid-feature': 'str', > + 'features': 'int' } } > + > +## > +# @X86CPUFeatureWordInfo: > +# > +# A discriminated record of X86 CPU feature words > +# > +# Since: 3.1 > +## > + > +{ 'union': 'X86CPUFeatureWordInfo', > + 'base': { 'type': 'X86CPUFeatureWordType' }, > + 'discriminator': 'type', > + 'data': { > + 'cpuid': 'X86CPUIDFeatureWordInfo', > + 'msr': 'X86MSRFeatureWordInfo' }} > + > ## > # @DummyForceArrays: > # [root@robert-ivt qemu]# git diff qapi/misc.json diff --git a/qapi/misc.json b/qapi/misc.json index d450cfe..7351dc2 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -2663,25 +2663,25 @@ 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } ## -# @X86CPUFeatureWordInfo: +# @X86CPUIDFeatureWordInfo: # -# Information about a X86 CPU feature word +# Information about a X86 CPUID feature word # -# @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word +# @input-eax: Input EAX value for CPUID instruction for that feature word # -# @cpuid-input-ecx: Input ECX value for CPUID instruction for that +# @input-ecx: Input ECX value for CPUID instruction for that # feature word # -# @cpuid-register: Output register containing the feature bits +# @register: Output register containing the feature bits # # @features: value of output register, containing the feature bits # # Since: 1.5 ## -{ 'struct': 'X86CPUFeatureWordInfo', - 'data': { 'cpuid-input-eax': 'int', - '*cpuid-input-ecx': 'int', - 'cpuid-register': 'X86CPURegister32', +{ 'struct': 'X86CPUIDFeatureWordInfo', + 'data': { 'input-eax': 'int', + '*input-ecx': 'int', + 'register': 'X86CPURegister32', 'features': 'int' } } ## @@ -2694,6 +2694,50 @@ { 'struct': 'DummyForceArrays', 'data': { 'unused': ['X86CPUFeatureWordInfo'] } } +## +# @X86MSRFeatureWordInfo: +# +# Information about an X86 MSR feature word +# +# @index: Index of the model specific register +# +# @cpuid-feature: CPUID feature name that communicates the existance of the MSR +# +# @features: value of output register, containing the feature bits +# +# Since: 3.1 +## +{ 'struct': 'X86MSRFeatureWordInfo', + 'data': { 'index': 'int', + 'cpuid-feature': 'str', + 'features': 'int' } } + +## +# @X86CPUFeatureWordType: +# +# Kinds of X86 CPU feature words +# +# @cpuid: A CPUID leaf +# +# @msr: An MSR +## +{ 'enum': 'X86CPUFeatureWordType', + 'data': [ 'cpuid', 'msr' ] } + +## +# @X86CPUFeatureWordInfo: +# +# A discriminated record of X86 CPU feature words +# +# Since: 3.1 +## +{ 'union': 'X86CPUFeatureWordInfo', + 'base': { 'type': 'X86CPUFeatureWordType' }, + 'discriminator': 'type', + 'data': { + 'cpuid': 'X86CPUIDFeatureWordInfo', + 'msr': 'X86MSRFeatureWordInfo' }} + ## # @NumaOptionsType: Hi Paolo, above is my draft change on the qapi json file. 1 question: struct X86MSRFeatureWordInfo { int64_t index; char *cpuid_feature; int64_t features; }; how to have a "const" prefix the "char *"? > > I'm not sure if the cpuid-feature field is useful for libvirt and > other management applications. Eduardo, what do you think? > > Thanks, > > Paolo ^ permalink raw reply related [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-09-05 5:47 ` Robert Hoo @ 2018-09-05 14:10 ` Eduardo Habkost 2018-09-05 15:32 ` Eric Blake 0 siblings, 1 reply; 40+ messages in thread From: Eduardo Habkost @ 2018-09-05 14:10 UTC (permalink / raw) To: Robert Hoo Cc: Paolo Bonzini, robert.hu, rth, thomas.lendacky, qemu-devel, jingqi.liu, Markus Armbruster, Eric Blake, Jiri Denemark Question to the QAPI schema maintainers below: On Wed, Sep 05, 2018 at 01:47:55PM +0800, Robert Hoo wrote: > On Fri, 2018-08-17 at 17:52 +0200, Paolo Bonzini wrote: > > On 10/08/2018 16:06, Robert Hoo wrote: > > > x86_cpu_get_feature_words(): limit to CPUID_FEATURE_WORD only. > > > > This should also grow support for MSR feature words. > > > > My suggestion is that you add another patch after patch 1 that expands > > the definition of X86CPUFeatureWordInfo like this, and adjusts > > x86_cpu_get_feature_words() to match the new C structs. Then this > > patch can add MSR feature support somewhat easily. > > > > The QAPI definitions would then look like this: > > > > diff --git a/qapi/misc.json b/qapi/misc.json > > index d450cfef21..eae9243976 100644 > > --- a/qapi/misc.json > > +++ b/qapi/misc.json > > @@ -2663,9 +2663,9 @@ > > 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } > > > > ## > > -# @X86CPUFeatureWordInfo: > > +# @X86CPUIDFeatureWordInfo: > > # > > -# Information about a X86 CPU feature word > > +# Information about an X86 CPUID feature word > > # > > # @cpuid-input-eax: Input EAX value for CPUID instruction for that feature word > > # > > @@ -2678,12 +2690,45 @@ > > # > > # Since: 1.5 > > ## > > -{ 'struct': 'X86CPUFeatureWordInfo', > > +{ 'struct': 'X86CPUIDFeatureWordInfo', > > 'data': { 'cpuid-input-eax': 'int', > > '*cpuid-input-ecx': 'int', > > 'cpuid-register': 'X86CPURegister32', > > 'features': 'int' } } > > > > +## > > +# @X86MSRFeatureWordInfo: > > +# > > +# Information about an X86 MSR feature word > > +# > > +# @index: Index of the model specific register > > +# > > +# @cpuid-feature: CPUID feature name that communicates the existance of the MSR > > +# > > +# @features: value of output register, containing the feature bits > > +# > > +# Since: 3.1 > > +## > > +{ 'struct': 'X86MSRFeatureWordInfo', > > + 'data': { 'index': 'int', > > + 'cpuid-feature': 'str', > > + 'features': 'int' } } > > + > > +## > > +# @X86CPUFeatureWordInfo: > > +# > > +# A discriminated record of X86 CPU feature words > > +# > > +# Since: 3.1 > > +## > > + > > +{ 'union': 'X86CPUFeatureWordInfo', > > + 'base': { 'type': 'X86CPUFeatureWordType' }, > > + 'discriminator': 'type', > > + 'data': { > > + 'cpuid': 'X86CPUIDFeatureWordInfo', > > + 'msr': 'X86MSRFeatureWordInfo' }} > > + > > ## > > # @DummyForceArrays: > > # > > [root@robert-ivt qemu]# git diff qapi/misc.json > diff --git a/qapi/misc.json b/qapi/misc.json > index d450cfe..7351dc2 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -2663,25 +2663,25 @@ > 'data': [ 'EAX', 'EBX', 'ECX', 'EDX', 'ESP', 'EBP', 'ESI', 'EDI' ] } > > ## > -# @X86CPUFeatureWordInfo: > +# @X86CPUIDFeatureWordInfo: > # > -# Information about a X86 CPU feature word > +# Information about a X86 CPUID feature word > # > -# @cpuid-input-eax: Input EAX value for CPUID instruction for that > feature word > +# @input-eax: Input EAX value for CPUID instruction for that feature > word > # > -# @cpuid-input-ecx: Input ECX value for CPUID instruction for that > +# @input-ecx: Input ECX value for CPUID instruction for that > # feature word > # > -# @cpuid-register: Output register containing the feature bits > +# @register: Output register containing the feature bits > # > # @features: value of output register, containing the feature bits > # > # Since: 1.5 > ## > -{ 'struct': 'X86CPUFeatureWordInfo', > - 'data': { 'cpuid-input-eax': 'int', > - '*cpuid-input-ecx': 'int', > - 'cpuid-register': 'X86CPURegister32', > +{ 'struct': 'X86CPUIDFeatureWordInfo', > + 'data': { 'input-eax': 'int', > + '*input-ecx': 'int', > + 'register': 'X86CPURegister32', > 'features': 'int' } } > > ## > @@ -2694,6 +2694,50 @@ > { 'struct': 'DummyForceArrays', > 'data': { 'unused': ['X86CPUFeatureWordInfo'] } } > > +## > +# @X86MSRFeatureWordInfo: > +# > +# Information about an X86 MSR feature word > +# > +# @index: Index of the model specific register > +# > +# @cpuid-feature: CPUID feature name that communicates the existance of > the MSR > +# > +# @features: value of output register, containing the feature bits > +# > +# Since: 3.1 > +## > +{ 'struct': 'X86MSRFeatureWordInfo', > + 'data': { 'index': 'int', > + 'cpuid-feature': 'str', > + 'features': 'int' } } > + > +## > +# @X86CPUFeatureWordType: > +# > +# Kinds of X86 CPU feature words > +# > +# @cpuid: A CPUID leaf > +# > +# @msr: An MSR > +## > +{ 'enum': 'X86CPUFeatureWordType', > + 'data': [ 'cpuid', 'msr' ] } > + > +## > +# @X86CPUFeatureWordInfo: > +# > +# A discriminated record of X86 CPU feature words > +# > +# Since: 3.1 > +## > +{ 'union': 'X86CPUFeatureWordInfo', > + 'base': { 'type': 'X86CPUFeatureWordType' }, > + 'discriminator': 'type', > + 'data': { > + 'cpuid': 'X86CPUIDFeatureWordInfo', > + 'msr': 'X86MSRFeatureWordInfo' }} Question to the QAPI maintainers: is this really allowed? With this change, data that conforms to the new schema may not conform to the old schema, because now the "input-eax" field will become optional. AFAICS, this will break the existing libvirt code: it will make qemuMonitorJSONParseCPUx86Features() error out because it won't find the "input-eax" field on all X86CPUFeatureWordInfo elements. > + > > ## > # @NumaOptionsType: > > Hi Paolo, > > above is my draft change on the qapi json file. 1 question: > struct X86MSRFeatureWordInfo { > int64_t index; > char *cpuid_feature; > int64_t features; > }; > how to have a "const" prefix the "char *"? QAPI will make qapi_free_X86MSRFeatureWordInfo(i) call g_free(i->cpuid_feature), which means you aren't supposed to use a const char pointer here. You can use g_strdup() if necessary. > > > > I'm not sure if the cpuid-feature field is useful for libvirt and > > other management applications. Eduardo, what do you think? It looks like a very limited way to represent dependencies between CPU features because it applies only to MSRs. I would prefer to represent feature dependencies in a more generic way later, and only if really necessary. > > > > Thanks, > > > > Paolo > > -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-09-05 14:10 ` Eduardo Habkost @ 2018-09-05 15:32 ` Eric Blake 2018-09-05 16:44 ` Eduardo Habkost 0 siblings, 1 reply; 40+ messages in thread From: Eric Blake @ 2018-09-05 15:32 UTC (permalink / raw) To: Eduardo Habkost, Robert Hoo Cc: Paolo Bonzini, robert.hu, rth, thomas.lendacky, qemu-devel, jingqi.liu, Markus Armbruster, Jiri Denemark On 09/05/2018 09:10 AM, Eduardo Habkost wrote: > Question to the QAPI schema maintainers below: > >> ## >> -{ 'struct': 'X86CPUFeatureWordInfo', >> - 'data': { 'cpuid-input-eax': 'int', >> - '*cpuid-input-ecx': 'int', >> - 'cpuid-register': 'X86CPURegister32', >> +{ 'struct': 'X86CPUIDFeatureWordInfo', >> + 'data': { 'input-eax': 'int', >> + '*input-ecx': 'int', >> + 'register': 'X86CPURegister32', >> 'features': 'int' } } You are renaming the struct (which is not visible over the wire), as well as renaming members within the struct (which is visible, if this type appears on the wire). However, 'grep FeatureWordInfo qapi/qapi-introspect.c' has no hits either before or after this patch (well, first you have to build with my currently-pending patch as part of Markus' pull request for this trick to work, https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg05968.html). Or, even without my patch, grepping for 'input-eax' has no hits as a member name in any struct. Which means that there are no QMP commands currently exposing this struct over the wire. > > > Question to the QAPI maintainers: is this really allowed? With > this change, data that conforms to the new schema may not conform > to the old schema, because now the "input-eax" field will become > optional. Yes, you can do whatever you want to QAPI structs that are not part of the QMP API, since they exist merely to make your C code easier. You don't have to worry about backwards compatibility, because the only clients of such structs are being compiled at the same time as you make modifications to that struct. Only once the struct appears in QMP introspection do you start having to worry about back-compat. There, the rules are the struct names don't matter, but member names do; if the struct is used on input, then removing members is bad, adding mandatory members is bad, but adding optional members is okay (older clients don't know to pass in the new members, and may continue to pass in the member name you no longer want to use); if the struct is used on output, then removing non-optional members is bad, removing optional members is okay, and adding members is okay (clients are supposed to ignore unknown members, but may break if a previously mandatory member disappears). > > AFAICS, this will break the existing libvirt code: it will make > qemuMonitorJSONParseCPUx86Features() error out because it won't > find the "input-eax" field on all X86CPUFeatureWordInfo elements. No, this change can't break libvirt, since I just proved there is no QMP command that libvirt could be using that either supplies an input member or expects an output member named "input-eax" in the first place. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-09-05 15:32 ` Eric Blake @ 2018-09-05 16:44 ` Eduardo Habkost 2018-09-05 17:41 ` Eric Blake 0 siblings, 1 reply; 40+ messages in thread From: Eduardo Habkost @ 2018-09-05 16:44 UTC (permalink / raw) To: Eric Blake Cc: Robert Hoo, Paolo Bonzini, robert.hu, rth, thomas.lendacky, qemu-devel, jingqi.liu, Markus Armbruster, Jiri Denemark On Wed, Sep 05, 2018 at 10:32:33AM -0500, Eric Blake wrote: > On 09/05/2018 09:10 AM, Eduardo Habkost wrote: > > Question to the QAPI schema maintainers below: > > > > > > ## > > > -{ 'struct': 'X86CPUFeatureWordInfo', > > > - 'data': { 'cpuid-input-eax': 'int', > > > - '*cpuid-input-ecx': 'int', > > > - 'cpuid-register': 'X86CPURegister32', > > > +{ 'struct': 'X86CPUIDFeatureWordInfo', > > > + 'data': { 'input-eax': 'int', > > > + '*input-ecx': 'int', > > > + 'register': 'X86CPURegister32', > > > 'features': 'int' } } > > You are renaming the struct (which is not visible over the wire), as well as > renaming members within the struct (which is visible, if this type appears > on the wire). > > However, 'grep FeatureWordInfo qapi/qapi-introspect.c' has no hits either > before or after this patch (well, first you have to build with my > currently-pending patch as part of Markus' pull request for this trick to > work, https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg05968.html). > Or, even without my patch, grepping for 'input-eax' has no hits as a member > name in any struct. Which means that there are no QMP commands currently > exposing this struct over the wire. There is one: qom-get. > > > > > > > Question to the QAPI maintainers: is this really allowed? With > > this change, data that conforms to the new schema may not conform > > to the old schema, because now the "input-eax" field will become > > optional. > > Yes, you can do whatever you want to QAPI structs that are not part of the > QMP API, since they exist merely to make your C code easier. You don't have > to worry about backwards compatibility, because the only clients of such > structs are being compiled at the same time as you make modifications to > that struct. Only once the struct appears in QMP introspection do you start > having to worry about back-compat. There, the rules are the struct names > don't matter, but member names do; if the struct is used on input, then > removing members is bad, adding mandatory members is bad, but adding > optional members is okay (older clients don't know to pass in the new > members, and may continue to pass in the member name you no longer want to > use); if the struct is used on output, then removing non-optional members is > bad, removing optional members is okay, and adding members is okay (clients > are supposed to ignore unknown members, but may break if a previously > mandatory member disappears). > > > > > AFAICS, this will break the existing libvirt code: it will make > > qemuMonitorJSONParseCPUx86Features() error out because it won't > > find the "input-eax" field on all X86CPUFeatureWordInfo elements. > > No, this change can't break libvirt, since I just proved there is no QMP > command that libvirt could be using that either supplies an input member or > expects an output member named "input-eax" in the first place. I'm sure it will break libvirt. libvirt uses "qom-get path=<cpu-path> property=feature-words" to get X86FeatureWordInfo data, and it expects the returned data to have a "input-eax" field. -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-09-05 16:44 ` Eduardo Habkost @ 2018-09-05 17:41 ` Eric Blake 2018-09-06 6:00 ` Hu, Robert 0 siblings, 1 reply; 40+ messages in thread From: Eric Blake @ 2018-09-05 17:41 UTC (permalink / raw) To: Eduardo Habkost Cc: Robert Hoo, Paolo Bonzini, robert.hu, rth, thomas.lendacky, qemu-devel, jingqi.liu, Markus Armbruster, Jiri Denemark On 09/05/2018 11:44 AM, Eduardo Habkost wrote: > On Wed, Sep 05, 2018 at 10:32:33AM -0500, Eric Blake wrote: >> On 09/05/2018 09:10 AM, Eduardo Habkost wrote: >>> Question to the QAPI schema maintainers below: >>> >> >>>> ## >>>> -{ 'struct': 'X86CPUFeatureWordInfo', >>>> - 'data': { 'cpuid-input-eax': 'int', >>>> - '*cpuid-input-ecx': 'int', >>>> - 'cpuid-register': 'X86CPURegister32', >>>> +{ 'struct': 'X86CPUIDFeatureWordInfo', >>>> + 'data': { 'input-eax': 'int', >>>> + '*input-ecx': 'int', >>>> + 'register': 'X86CPURegister32', >>>> 'features': 'int' } } >> >> You are renaming the struct (which is not visible over the wire), as well as >> renaming members within the struct (which is visible, if this type appears >> on the wire). >> >> However, 'grep FeatureWordInfo qapi/qapi-introspect.c' has no hits either >> before or after this patch (well, first you have to build with my >> currently-pending patch as part of Markus' pull request for this trick to >> work, https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg05968.html). >> Or, even without my patch, grepping for 'input-eax' has no hits as a member >> name in any struct. Which means that there are no QMP commands currently >> exposing this struct over the wire. > > There is one: qom-get. Oh, right, the nasty exception that is not visible to introspection :( >>> AFAICS, this will break the existing libvirt code: it will make >>> qemuMonitorJSONParseCPUx86Features() error out because it won't >>> find the "input-eax" field on all X86CPUFeatureWordInfo elements. >> >> No, this change can't break libvirt, since I just proved there is no QMP >> command that libvirt could be using that either supplies an input member or >> expects an output member named "input-eax" in the first place. > > I'm sure it will break libvirt. libvirt uses > "qom-get path=<cpu-path> property=feature-words" to get > X86FeatureWordInfo data, and it expects the returned data to > have a "input-eax" field. Yeah, since this type is used in the qom-get backdoor that evades introspection, it's even more important that you follow the backwards-compatibility rules and not rename or delete any previously-mandatory members, since libvirt CAN'T introspect if such changes have happened. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-09-05 17:41 ` Eric Blake @ 2018-09-06 6:00 ` Hu, Robert 2018-09-10 17:38 ` Eduardo Habkost 0 siblings, 1 reply; 40+ messages in thread From: Hu, Robert @ 2018-09-06 6:00 UTC (permalink / raw) To: Eric Blake, Eduardo Habkost Cc: Robert Hoo, Paolo Bonzini, rth, thomas.lendacky, qemu-devel, Liu, Jingqi, Markus Armbruster, Jiri Denemark > -----Original Message----- > From: Eric Blake [mailto:eblake@redhat.com] > Sent: Thursday, September 6, 2018 1:41 > To: Eduardo Habkost <ehabkost@redhat.com> > Cc: Robert Hoo <robert.hu@linux.intel.com>; Paolo Bonzini > <pbonzini@redhat.com>; Hu, Robert <robert.hu@intel.com>; rth@twiddle.net; > thomas.lendacky@amd.com; qemu-devel@nongnu.org; Liu, Jingqi > <jingqi.liu@intel.com>; Markus Armbruster <armbru@redhat.com>; Jiri > Denemark <jdenemar@redhat.com> > Subject: Re: [PATCH v3 3/3] Change other funcitons referring to > feature_word_info[] > > On 09/05/2018 11:44 AM, Eduardo Habkost wrote: > > On Wed, Sep 05, 2018 at 10:32:33AM -0500, Eric Blake wrote: > >> On 09/05/2018 09:10 AM, Eduardo Habkost wrote: > >>> Question to the QAPI schema maintainers below: > >>> > >> > >>>> ## > >>>> -{ 'struct': 'X86CPUFeatureWordInfo', > >>>> - 'data': { 'cpuid-input-eax': 'int', > >>>> - '*cpuid-input-ecx': 'int', > >>>> - 'cpuid-register': 'X86CPURegister32', > >>>> +{ 'struct': 'X86CPUIDFeatureWordInfo', > >>>> + 'data': { 'input-eax': 'int', > >>>> + '*input-ecx': 'int', > >>>> + 'register': 'X86CPURegister32', > >>>> 'features': 'int' } } > >> > >> You are renaming the struct (which is not visible over the wire), as > >> well as renaming members within the struct (which is visible, if this > >> type appears on the wire). > >> > >> However, 'grep FeatureWordInfo qapi/qapi-introspect.c' has no hits > >> either before or after this patch (well, first you have to build with > >> my currently-pending patch as part of Markus' pull request for this > >> trick to work, https://lists.gnu.org/archive/html/qemu-devel/2018- > 08/msg05968.html). > >> Or, even without my patch, grepping for 'input-eax' has no hits as a > >> member name in any struct. Which means that there are no QMP > >> commands currently exposing this struct over the wire. > > > > There is one: qom-get. > > Oh, right, the nasty exception that is not visible to introspection :( > > >>> AFAICS, this will break the existing libvirt code: it will make > >>> qemuMonitorJSONParseCPUx86Features() error out because it won't find > >>> the "input-eax" field on all X86CPUFeatureWordInfo elements. > >> > >> No, this change can't break libvirt, since I just proved there is no > >> QMP command that libvirt could be using that either supplies an input > >> member or expects an output member named "input-eax" in the first place. > > > > I'm sure it will break libvirt. libvirt uses "qom-get path=<cpu-path> > > property=feature-words" to get X86FeatureWordInfo data, and it expects > > the returned data to have a "input-eax" field. > > Yeah, since this type is used in the qom-get backdoor that evades introspection, > it's even more important that you follow the backwards-compatibility rules and > not rename or delete any previously-mandatory members, since libvirt CAN'T > introspect if such changes have happened. > [Robert Hoo] Oh, this is more complicated than my thought. So, Eduardo, how about leave the QAPI expansion for MSR based features to other professional people? > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-09-06 6:00 ` Hu, Robert @ 2018-09-10 17:38 ` Eduardo Habkost 2018-09-11 1:44 ` Robert Hoo 0 siblings, 1 reply; 40+ messages in thread From: Eduardo Habkost @ 2018-09-10 17:38 UTC (permalink / raw) To: Hu, Robert Cc: Eric Blake, Robert Hoo, Paolo Bonzini, rth, thomas.lendacky, qemu-devel, Liu, Jingqi, Markus Armbruster, Jiri Denemark On Thu, Sep 06, 2018 at 06:00:29AM +0000, Hu, Robert wrote: > > > -----Original Message----- > > From: Eric Blake [mailto:eblake@redhat.com] > > Sent: Thursday, September 6, 2018 1:41 > > To: Eduardo Habkost <ehabkost@redhat.com> > > Cc: Robert Hoo <robert.hu@linux.intel.com>; Paolo Bonzini > > <pbonzini@redhat.com>; Hu, Robert <robert.hu@intel.com>; rth@twiddle.net; > > thomas.lendacky@amd.com; qemu-devel@nongnu.org; Liu, Jingqi > > <jingqi.liu@intel.com>; Markus Armbruster <armbru@redhat.com>; Jiri > > Denemark <jdenemar@redhat.com> > > Subject: Re: [PATCH v3 3/3] Change other funcitons referring to > > feature_word_info[] > > > > On 09/05/2018 11:44 AM, Eduardo Habkost wrote: > > > On Wed, Sep 05, 2018 at 10:32:33AM -0500, Eric Blake wrote: > > >> On 09/05/2018 09:10 AM, Eduardo Habkost wrote: > > >>> Question to the QAPI schema maintainers below: > > >>> > > >> > > >>>> ## > > >>>> -{ 'struct': 'X86CPUFeatureWordInfo', > > >>>> - 'data': { 'cpuid-input-eax': 'int', > > >>>> - '*cpuid-input-ecx': 'int', > > >>>> - 'cpuid-register': 'X86CPURegister32', > > >>>> +{ 'struct': 'X86CPUIDFeatureWordInfo', > > >>>> + 'data': { 'input-eax': 'int', > > >>>> + '*input-ecx': 'int', > > >>>> + 'register': 'X86CPURegister32', > > >>>> 'features': 'int' } } > > >> > > >> You are renaming the struct (which is not visible over the wire), as > > >> well as renaming members within the struct (which is visible, if this > > >> type appears on the wire). > > >> > > >> However, 'grep FeatureWordInfo qapi/qapi-introspect.c' has no hits > > >> either before or after this patch (well, first you have to build with > > >> my currently-pending patch as part of Markus' pull request for this > > >> trick to work, https://lists.gnu.org/archive/html/qemu-devel/2018- > > 08/msg05968.html). > > >> Or, even without my patch, grepping for 'input-eax' has no hits as a > > >> member name in any struct. Which means that there are no QMP > > >> commands currently exposing this struct over the wire. > > > > > > There is one: qom-get. > > > > Oh, right, the nasty exception that is not visible to introspection :( > > > > >>> AFAICS, this will break the existing libvirt code: it will make > > >>> qemuMonitorJSONParseCPUx86Features() error out because it won't find > > >>> the "input-eax" field on all X86CPUFeatureWordInfo elements. > > >> > > >> No, this change can't break libvirt, since I just proved there is no > > >> QMP command that libvirt could be using that either supplies an input > > >> member or expects an output member named "input-eax" in the first place. > > > > > > I'm sure it will break libvirt. libvirt uses "qom-get path=<cpu-path> > > > property=feature-words" to get X86FeatureWordInfo data, and it expects > > > the returned data to have a "input-eax" field. > > > > Yeah, since this type is used in the qom-get backdoor that evades introspection, > > it's even more important that you follow the backwards-compatibility rules and > > not rename or delete any previously-mandatory members, since libvirt CAN'T > > introspect if such changes have happened. > > > [Robert Hoo] > Oh, this is more complicated than my thought. > So, Eduardo, how about leave the QAPI expansion for MSR based features to other > professional people? I think it's now clear that we can't add MSR features to "feature-words" in a compatible way, so any mechanism to introspect MSR features will need a separate property or QMP command. I also think "feature-words" needs to be replaced with something better. Probably libvirt should be able to grab all information it needs by looking at the boolean QOM properties instead of the low-level info at "feature-words". This means I agree to leave this functionality out of the MSR feature patch series for now. -- Eduardo ^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] 2018-09-10 17:38 ` Eduardo Habkost @ 2018-09-11 1:44 ` Robert Hoo 0 siblings, 0 replies; 40+ messages in thread From: Robert Hoo @ 2018-09-11 1:44 UTC (permalink / raw) To: Eduardo Habkost Cc: robert.hu, robert.hu, Eric Blake, Paolo Bonzini, rth, thomas.lendacky, qemu-devel, Liu, Jingqi, Markus Armbruster, Jiri Denemark On Mon, 2018-09-10 at 14:38 -0300, Eduardo Habkost wrote: > On Thu, Sep 06, 2018 at 06:00:29AM +0000, Hu, Robert wrote: > > > > > > > > Yeah, since this type is used in the qom-get backdoor that evades > > > introspection, > > > it's even more important that you follow the backwards- > > > compatibility rules and > > > not rename or delete any previously-mandatory members, since > > > libvirt CAN'T > > > introspect if such changes have happened. > > > > > > > [Robert Hoo] > > Oh, this is more complicated than my thought. > > So, Eduardo, how about leave the QAPI expansion for MSR based > > features to other > > professional people? > > I think it's now clear that we can't add MSR features to > "feature-words" in a compatible way, so any mechanism to > introspect MSR features will need a separate property or QMP > command. > > I also think "feature-words" needs to be replaced with something > better. Probably libvirt should be able to grab all information > it needs by looking at the boolean QOM properties instead of the > low-level info at "feature-words". > > This means I agree to leave this functionality out of the MSR > feature patch series for now. Thanks Eduardo. Clear to me now. > ^ permalink raw reply [flat|nested] 40+ messages in thread
end of thread, other threads:[~2018-09-11 1:44 UTC | newest] Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-10 14:06 [Qemu-devel] [PATCH v3 0/3] x86: QEMU side support on MSR based features Robert Hoo 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support " Robert Hoo 2018-08-17 3:10 ` Eduardo Habkost 2018-08-18 3:10 ` Robert Hoo 2018-08-18 5:48 ` Robert Hoo 2018-08-18 7:50 ` Paolo Bonzini 2018-08-17 15:50 ` Paolo Bonzini 2018-08-17 15:55 ` Eduardo Habkost 2018-08-17 17:34 ` Paolo Bonzini 2018-08-17 17:48 ` [Qemu-devel] X86CPU "feature-words" property on QEMU (was Re: [PATCH v3 1/3] x86: Data structure changes to support MSR based) features Eduardo Habkost 2018-08-17 17:59 ` Paolo Bonzini 2018-08-17 18:10 ` Eduardo Habkost 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 2/3] kvm: Add support to KVM_GET_MSR_FEATURE_INDEX_LIST and KVM_GET_MSRS system ioctl Robert Hoo 2018-08-17 13:18 ` Eduardo Habkost 2018-08-18 7:27 ` Robert Hoo 2018-08-18 15:05 ` Eduardo Habkost 2018-08-23 6:28 ` Robert Hoo 2018-08-23 17:11 ` Eduardo Habkost 2018-08-23 17:28 ` Paolo Bonzini 2018-08-23 17:36 ` Eduardo Habkost 2018-08-23 20:23 ` Paolo Bonzini 2018-08-25 17:27 ` Eduardo Habkost 2018-08-30 4:22 ` Robert Hoo 2018-08-30 18:28 ` Eduardo Habkost 2018-08-10 14:06 ` [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] Robert Hoo 2018-08-10 15:17 ` Eric Blake 2018-08-14 10:06 ` Robert Hoo 2018-08-17 13:28 ` Eduardo Habkost 2018-08-18 9:01 ` Robert Hoo 2018-08-18 15:10 ` Eduardo Habkost 2018-08-17 15:52 ` Paolo Bonzini 2018-08-18 12:53 ` Robert Hoo 2018-09-05 5:47 ` Robert Hoo 2018-09-05 14:10 ` Eduardo Habkost 2018-09-05 15:32 ` Eric Blake 2018-09-05 16:44 ` Eduardo Habkost 2018-09-05 17:41 ` Eric Blake 2018-09-06 6:00 ` Hu, Robert 2018-09-10 17:38 ` Eduardo Habkost 2018-09-11 1:44 ` Robert Hoo
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.