On Thu, 20 Dec 2012 10:48:08 -0200 Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Thu, Dec 20, 2012 at 01:16:31AM +0100, Igor Mammedov wrote: > > commit 8935499831312 makes cpuid return to guest host's vendor value > > instead of built-in one by default if kvm_enabled() == true and allows > > to override this behavior if 'vendor' is specified on -cpu command line. > > > > But every time guest calls cpuid to get 'vendor' value, host's value is > > read again and again in default case. > > > > It complicates semantic of vendor property and makes it harder to use, > > due to split brain syndrome, lets simplify it. > > > > Instead of reading 'vendor' value from host every time cpuid[vendor] is > > called, override 'vendor' value only once in cpu_x86_find_by_name(), when > > built-in CPU model is found and if(kvm_enabled() == true). > > > > It provides the same default semantic > > if (kvm_enabled() == true) vendor = host's vendor > > else vendor = built-in vendor > > > > and then later: > > if (custom vendor) vendor = custom vendor > > > > 'vendor' value is overridden when user provides it on -cpu command line, > > and there isn't need in vendor_override field anymore, remove it. > > > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > > --- > > target-i386/cpu.c | 20 +++++--------------- > > target-i386/cpu.h | 1 - > > 2 files changed, 5 insertions(+), 16 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index a96aa33..a12d938 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -284,7 +284,6 @@ typedef struct x86_def_t { > > uint32_t kvm_features, svm_features; > > uint32_t xlevel; > > char model_id[48]; > > - int vendor_override; > > /* Store the results of Centaur's CPUID instructions */ > > uint32_t ext4_features; > > uint32_t xlevel2; > > > > @@ -865,7 +864,6 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def) > > kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX); > > > > cpu_x86_fill_model_id(x86_cpu_def->model_id); > > - x86_cpu_def->vendor_override = 0; > > > > It's funny how x86_def_t _never_ has vendor_override set to true. The > field was completely pointless. > > > > /* Call Centaur's CPUID instruction. */ > > if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) { > > @@ -1117,7 +1115,6 @@ static void x86_cpuid_set_vendor(Object *obj, const > > char *value, env->cpuid_vendor2 |= ((uint8_t)value[i + 4]) << (8 * i); > > env->cpuid_vendor3 |= ((uint8_t)value[i + 8]) << (8 * i); > > } > > - env->cpuid_vendor_override = 1; > > } > > > > static char *x86_cpuid_get_model_id(Object *obj, Error **errp) > > @@ -1194,7 +1191,6 @@ static void cpudef_2_x86_cpu(X86CPU *cpu, x86_def_t > > *def, Error **errp) > > assert(def->vendor[0]); > > object_property_set_str(OBJECT(cpu), def->vendor, "vendor", errp); > > - env->cpuid_vendor_override = def->vendor_override; > > object_property_set_int(OBJECT(cpu), def->level, "level", errp); > > object_property_set_int(OBJECT(cpu), def->family, "family", errp); > > object_property_set_int(OBJECT(cpu), def->model, "model", errp); > > @@ -1231,6 +1227,11 @@ static int cpu_x86_find_by_name(x86_def_t > > *x86_cpu_def, const char *name) return -1; > > } else { > > memcpy(x86_cpu_def, def, sizeof(*def)); > > Could you keep the old comment explaining the reason for the KVM > behavior, here? > > /* sysenter isn't supported on compatibility mode on AMD, syscall > * isn't supported in compatibility mode on Intel. > * Normally we advertise the actual cpu vendor, but you can override > * this if you want to use KVM's sysenter/syscall emulation > * in compatibility mode and when doing cross vendor migration > */ > > (I suggest replacing "you can override this" with "you can override this > using the 'vendor' property"). sure, I'll do it on series respin. > > The rest of the patch looks good to me. > > > > + if (kvm_enabled()) { > > + uint32_t ebx = 0, ecx = 0, edx = 0; > > + host_cpuid(0, 0, NULL, &ebx, &ecx, &edx); > > + x86cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, ecx); > > + } > > I like the fact that you are doing this as early as possible (even > before cpudef_2_x86_cpu() is called), making it easy to move the logic > to class_init. > > > > } > > > > return 0; > > @@ -1331,7 +1332,6 @@ static int cpu_x86_parse_featurestr(x86_def_t > > *x86_cpu_def, char *features, x86_cpu_def->xlevel = numvalue; > > } else if (!strcmp(featurestr, "vendor")) { > > pstrcpy(x86_cpu_def->vendor, > > sizeof(x86_cpu_def->vendor), val); > > - x86_cpu_def->vendor_override = 1; > > } else if (!strcmp(featurestr, "model_id")) { > > pstrcpy(x86_cpu_def->model_id, > > sizeof(x86_cpu_def->model_id), val); > > @@ -1582,16 +1582,6 @@ static void get_cpuid_vendor(CPUX86State *env, > > uint32_t *ebx, *ebx = env->cpuid_vendor1; > > *edx = env->cpuid_vendor2; > > *ecx = env->cpuid_vendor3; > > - > > - /* sysenter isn't supported on compatibility mode on AMD, syscall > > - * isn't supported in compatibility mode on Intel. > > - * Normally we advertise the actual cpu vendor, but you can override > > - * this if you want to use KVM's sysenter/syscall emulation > > - * in compatibility mode and when doing cross vendor migration > > - */ > > - if (kvm_enabled() && ! env->cpuid_vendor_override) { > > - host_cpuid(0, 0, NULL, ebx, ecx, edx); > > - } > > } > > > > void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count, > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > index fbbe730..a15a09e 100644 > > --- a/target-i386/cpu.h > > +++ b/target-i386/cpu.h > > @@ -812,7 +812,6 @@ typedef struct CPUX86State { > > uint32_t cpuid_ext2_features; > > uint32_t cpuid_ext3_features; > > uint32_t cpuid_apic_id; > > - int cpuid_vendor_override; > > /* Store the results of Centaur's CPUID instructions */ > > uint32_t cpuid_xlevel2; > > uint32_t cpuid_ext4_features; > > -- > > 1.7.11.7 > > >