[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feat
From: |
Eduardo Habkost |
Subject: |
Re: [Qemu-devel] [PATCH v3 3/3] Change other funcitons referring to feature_word_info[] |
Date: |
Fri, 17 Aug 2018 10:28:20 -0300 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
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 <address@hidden>
> ---
> 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
Re: [Qemu-devel] [PATCH v3 1/3] x86: Data structure changes to support MSR based features, Paolo Bonzini, 2018/08/17