| Message ID | 1533909989-56115-4-git-send-email-robert.hu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86: QEMU side support on MSR based features | expand |
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; > + }
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; > > + } >
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 > >
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
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 > > > > >
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
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.
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
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 > >
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.
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.
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.
> -----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
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.
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. >
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; }
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(-)