| Message ID | 53750D57.7010403@suse.de |
|---|---|
| State | New |
| Headers | show |
On Thu, May 15, 2014 at 08:54:15PM +0200, Andreas Färber wrote: > Am 30.04.2014 18:48, schrieb Eduardo Habkost: > > If enforce/check is specified in TCG mode, QEMU will ensure all CPU > > features are supported by TCG, so no CPU feature is silently disabled. > > > > Reviewed-by: Richard Henderson <rth@twiddle.net> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > Changes v1 -> v2: > > * Trivial rebase to latest qom-cpu (commit 90c5d39c) > > (Reviewed-by line kept) > > Changes v2 -> v3: > > * Trivial rebase after QEMU 2.0 (onto commit 2d03b49) > > (Reviewed-by line kept) > > --- > > target-i386/cpu.c | 34 ++++++++++++++++------------------ > > 1 file changed, 16 insertions(+), 18 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index b2e30ca..53b5038 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1265,8 +1265,9 @@ static int report_unavailable_features(FeatureWord w, uint32_t mask) > > if (1 << i & mask) { > > const char *reg = get_register_name_32(f->cpuid_reg); > > assert(reg); > > - fprintf(stderr, "warning: host doesn't support requested feature: " > > + fprintf(stderr, "warning: %s doesn't support requested feature: " > > "CPUID.%02XH:%s%s%s [bit %d]\n", > > + kvm_enabled() ? "host" : "TCG", > > f->cpuid_eax, reg, > > f->feat_names[i] ? "." : "", > > f->feat_names[i] ? f->feat_names[i] : "", i); > > @@ -1826,17 +1827,18 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp) > > static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w) > > { > > FeatureWordInfo *wi = &feature_word_info[w]; > > - assert(kvm_enabled()); > > - return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax, > > - wi->cpuid_ecx, > > - wi->cpuid_reg); > > + if (kvm_enabled()) { > > + return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax, > > + wi->cpuid_ecx, > > + wi->cpuid_reg); > > + } else { > > + return wi->tcg_features; > > + } > > } > > This function is called unconditionally now, so apply the following? > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 48ba1d8..112b437 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1839,8 +1839,10 @@ static uint32_t > x86_cpu_get_supported_feature_word(FeatureWord w) > return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax, > wi->cpuid_ecx, > wi->cpuid_reg); > - } else { > + } else if (tcg_enabled()) { > return wi->tcg_features; > + } else { > + return UINT32_MAX; Agreed, but I would prefer writing it as ~0 instead of UINT32_MAX. > } > } > > > Not sure what to do about the warning message. It wouldn't occur though > due to the suggested mask, so we could just ignore it for now. One day we may be able to simply ask the machine object for the current accelerator name. In the meantime, we could use: "warning: host (%s) doesn't support requested feature [...]", kvm_enabled() ? "KVM" : (tcg_enabled() ? "TCG" : "QEMU") (But I won't object if you prefer to keep the warning message I originally sent.)
Am 15.05.2014 21:12, schrieb Eduardo Habkost: > On Thu, May 15, 2014 at 08:54:15PM +0200, Andreas Färber wrote: >> Am 30.04.2014 18:48, schrieb Eduardo Habkost: >>> If enforce/check is specified in TCG mode, QEMU will ensure all CPU >>> features are supported by TCG, so no CPU feature is silently disabled. >>> >>> Reviewed-by: Richard Henderson <rth@twiddle.net> >>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> >>> --- >>> Changes v1 -> v2: >>> * Trivial rebase to latest qom-cpu (commit 90c5d39c) >>> (Reviewed-by line kept) >>> Changes v2 -> v3: >>> * Trivial rebase after QEMU 2.0 (onto commit 2d03b49) >>> (Reviewed-by line kept) >>> --- >>> target-i386/cpu.c | 34 ++++++++++++++++------------------ >>> 1 file changed, 16 insertions(+), 18 deletions(-) >>> >>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >>> index b2e30ca..53b5038 100644 >>> --- a/target-i386/cpu.c >>> +++ b/target-i386/cpu.c >>> @@ -1265,8 +1265,9 @@ static int report_unavailable_features(FeatureWord w, uint32_t mask) >>> if (1 << i & mask) { >>> const char *reg = get_register_name_32(f->cpuid_reg); >>> assert(reg); >>> - fprintf(stderr, "warning: host doesn't support requested feature: " >>> + fprintf(stderr, "warning: %s doesn't support requested feature: " >>> "CPUID.%02XH:%s%s%s [bit %d]\n", >>> + kvm_enabled() ? "host" : "TCG", >>> f->cpuid_eax, reg, >>> f->feat_names[i] ? "." : "", >>> f->feat_names[i] ? f->feat_names[i] : "", i); >>> @@ -1826,17 +1827,18 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp) >>> static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w) >>> { >>> FeatureWordInfo *wi = &feature_word_info[w]; >>> - assert(kvm_enabled()); >>> - return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax, >>> - wi->cpuid_ecx, >>> - wi->cpuid_reg); >>> + if (kvm_enabled()) { >>> + return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax, >>> + wi->cpuid_ecx, >>> + wi->cpuid_reg); >>> + } else { >>> + return wi->tcg_features; >>> + } >>> } >> >> This function is called unconditionally now, so apply the following? >> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >> index 48ba1d8..112b437 100644 >> --- a/target-i386/cpu.c >> +++ b/target-i386/cpu.c >> @@ -1839,8 +1839,10 @@ static uint32_t >> x86_cpu_get_supported_feature_word(FeatureWord w) >> return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax, >> wi->cpuid_ecx, >> wi->cpuid_reg); >> - } else { >> + } else if (tcg_enabled()) { >> return wi->tcg_features; >> + } else { >> + return UINT32_MAX; > > Agreed, but I would prefer writing it as ~0 instead of UINT32_MAX. FTR done as ~0u to avoid any signedness issues. > >> } >> } >> >> >> Not sure what to do about the warning message. It wouldn't occur though >> due to the suggested mask, so we could just ignore it for now. > > One day we may be able to simply ask the machine object for the current > accelerator name. In the meantime, we could use: > > "warning: host (%s) doesn't support requested feature [...]", > kvm_enabled() ? "KVM" : (tcg_enabled() ? "TCG" : "QEMU") > > (But I won't object if you prefer to keep the warning message I > originally sent.) I think I did the latter, yes... Andreas
Il 18/06/2014 17:50, Andreas Färber ha scritto: >>> >> + } else if (tcg_enabled()) { >>> >> return wi->tcg_features; >>> >> + } else { >>> >> + return UINT32_MAX; >> > >> > Agreed, but I would prefer writing it as ~0 instead of UINT32_MAX. > FTR done as ~0u to avoid any signedness issues. > FWIW, I find ~0u worse than ~0, because the former expands to 0xffffffff if x86_cpu_get_supported_feature_word is ever changed to return uint64_t. The latter instead returns all-ones as desired. Paolo
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 48ba1d8..112b437 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1839,8 +1839,10 @@ static uint32_t x86_cpu_get_supported_feature_word(FeatureWord w) return kvm_arch_get_supported_cpuid(kvm_state, wi->cpuid_eax, wi->cpuid_ecx, wi->cpuid_reg); - } else { + } else if (tcg_enabled()) { return wi->tcg_features; + } else { + return UINT32_MAX; } }