| Message ID | 1355962591-6467-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <imammedo@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"). 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 >
On Thu, 20 Dec 2012 10:48:08 -0200 Eduardo Habkost <ehabkost@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 <imammedo@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 > > >
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; /* 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)); + 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); + } } 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;
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 <imammedo@redhat.com> --- target-i386/cpu.c | 20 +++++--------------- target-i386/cpu.h | 1 - 2 files changed, 5 insertions(+), 16 deletions(-)