| Message ID | 1355220666-31722-3-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Dec 11, 2012 at 11:11:02AM +0100, Igor Mammedov wrote: > when CPU properties are implemented, ext2_features may change > between object_new(CPU) and cpu_realize_fn(). Sanitizing > ext2_features for AMD based CPU at realize() time will keep > current behavior after CPU features are converted to properties. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v2: > - style fix, make line shorter than 80 characters > > amd san Incomplete sentence? > --- > target-i386/cpu.c | 21 +++++++++++---------- > 1 files changed, 11 insertions(+), 10 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 63aae86..64b7637 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000, > "tsc-frequency", &error); > > - /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on > - * CPUID[1].EDX. > - */ > - if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && > - env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && > - env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) { > - env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES; > - env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES); > - } > - > object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error); > if (error) { > fprintf(stderr, "%s\n", error_get_pretty(error)); > @@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp) > env->cpuid_level = 7; > } > > + /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on > + * CPUID[1].EDX. > + */ > + if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && > + env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && > + env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) { I would add extra indentation space here, to make it not align with the statements below, making the condition visually distinct from the body, like in the original code you are moving. > + env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES; > + env->cpuid_ext2_features |= (env->cpuid_features > + & CPUID_EXT2_AMD_ALIASES); Weird spacing around the "&" above (3 spaces indent, 2 spaces after the "&"). I would align this as: env->cpuid_ext2_features |= (env->cpuid_features & CPUID_EXT2_AMD_ALIASES); As the above issues are only cosmetic: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > + } > + > if (!kvm_enabled()) { > env->cpuid_features &= TCG_FEATURES; > env->cpuid_ext_features &= TCG_EXT_FEATURES; > -- > 1.7.1 >
On Tue, 11 Dec 2012 11:31:45 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Dec 11, 2012 at 11:11:02AM +0100, Igor Mammedov wrote: > > when CPU properties are implemented, ext2_features may change > > between object_new(CPU) and cpu_realize_fn(). Sanitizing > > ext2_features for AMD based CPU at realize() time will keep > > current behavior after CPU features are converted to properties. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > v2: > > - style fix, make line shorter than 80 characters > > > > amd san > > Incomplete sentence? I forgot to remove comment from squashed in commit with alignment changes, which I got wrong anyway. I'll fix it and resubmit this patch + update git tree on github. Thanks! > > > --- > > target-i386/cpu.c | 21 +++++++++++---------- > > 1 files changed, 11 insertions(+), 10 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 63aae86..64b7637 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char > > *cpu_model) object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * > > 1000, "tsc-frequency", &error); > > > > - /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on > > - * CPUID[1].EDX. > > - */ > > - if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && > > - env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && > > - env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) { > > - env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES; > > - env->cpuid_ext2_features |= (def->features & > > CPUID_EXT2_AMD_ALIASES); > > - } > > - > > object_property_set_str(OBJECT(cpu), def->model_id, "model-id", > > &error); if (error) { > > fprintf(stderr, "%s\n", error_get_pretty(error)); > > @@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp) > > env->cpuid_level = 7; > > } > > > > + /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on > > + * CPUID[1].EDX. > > + */ > > + if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && > > + env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && > > + env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) { > > I would add extra indentation space here, to make it not align with the > statements below, making the condition visually distinct from the body, > like in the original code you are moving. > > > + env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES; > > + env->cpuid_ext2_features |= (env->cpuid_features > > + & CPUID_EXT2_AMD_ALIASES); > > Weird spacing around the "&" above (3 spaces indent, 2 spaces after the > "&"). > > I would align this as: > > env->cpuid_ext2_features |= (env->cpuid_features & > CPUID_EXT2_AMD_ALIASES); > > > As the above issues are only cosmetic: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > + } > > + > > if (!kvm_enabled()) { > > env->cpuid_features &= TCG_FEATURES; > > env->cpuid_ext_features &= TCG_EXT_FEATURES; > > -- > > 1.7.1 > > >
On Tue, 11 Dec 2012 11:31:45 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: [...] > > --- > > target-i386/cpu.c | 21 +++++++++++---------- > > 1 files changed, 11 insertions(+), 10 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 63aae86..64b7637 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char > > *cpu_model) object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * > > 1000, "tsc-frequency", &error); > > > > - /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on > > - * CPUID[1].EDX. > > - */ > > - if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && > > - env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && > > - env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) { > > - env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES; > > - env->cpuid_ext2_features |= (def->features & > > CPUID_EXT2_AMD_ALIASES); > > - } > > - > > object_property_set_str(OBJECT(cpu), def->model_id, "model-id", > > &error); if (error) { > > fprintf(stderr, "%s\n", error_get_pretty(error)); > > @@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp) > > env->cpuid_level = 7; > > } > > > > + /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on > > + * CPUID[1].EDX. > > + */ > > + if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && > > + env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && > > + env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) { > > I would add extra indentation space here, to make it not align with the > statements below, making the condition visually distinct from the body, > like in the original code you are moving. I've thought people would object to ident here, that's why I've changed original indentation to a more consistent with rules. BTW: git grep -A 3 "if (.*[^{)]$" shows that many places use this style including fairly recent ones: aio-posix.c first hit and we have in target-i386/cpu.c if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 & ... with the same alignment style and at least one more similar. Lets leave it this way to be consistent with the rest of the code. > > > + env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES; > > + env->cpuid_ext2_features |= (env->cpuid_features > > + & CPUID_EXT2_AMD_ALIASES); > > Weird spacing around the "&" above (3 spaces indent, 2 spaces after the > "&"). > > I would align this as: > > env->cpuid_ext2_features |= (env->cpuid_features & > CPUID_EXT2_AMD_ALIASES); Thanks, fixed. > > > As the above issues are only cosmetic: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > > + } > > + > > if (!kvm_enabled()) { > > env->cpuid_features &= TCG_FEATURES; > > env->cpuid_ext_features &= TCG_EXT_FEATURES; > > -- > > 1.7.1 > > >
On Tue, Dec 11, 2012 at 03:08:56PM +0100, Igor Mammedov wrote: [...] > > > + /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on > > > + * CPUID[1].EDX. > > > + */ > > > + if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && > > > + env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && > > > + env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) { > > > > I would add extra indentation space here, to make it not align with the > > statements below, making the condition visually distinct from the body, > > like in the original code you are moving. > I've thought people would object to ident here, that's why I've changed > original indentation to a more consistent with rules. > > BTW: git grep -A 3 "if (.*[^{)]$" > shows that many places use this style including fairly recent ones: > aio-posix.c first hit > > and we have in target-i386/cpu.c > if (x86_cpu_def->vendor1 == CPUID_VENDOR_VIA_1 & > ... > with the same alignment style and at least one more similar. > Lets leave it this way to be consistent with the rest of the code. No problem. I was expressing personal preference, not knowing what was the preferred/usual style in QEMU. My only data point was the original code you moved. :-)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 63aae86..64b7637 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1539,16 +1539,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) object_property_set_int(OBJECT(cpu), (int64_t)def->tsc_khz * 1000, "tsc-frequency", &error); - /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on - * CPUID[1].EDX. - */ - if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && - env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && - env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) { - env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES; - env->cpuid_ext2_features |= (def->features & CPUID_EXT2_AMD_ALIASES); - } - object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error); if (error) { fprintf(stderr, "%s\n", error_get_pretty(error)); @@ -2062,6 +2052,17 @@ void x86_cpu_realize(Object *obj, Error **errp) env->cpuid_level = 7; } + /* On AMD CPUs, some CPUID[8000_0001].EDX bits must match the bits on + * CPUID[1].EDX. + */ + if (env->cpuid_vendor1 == CPUID_VENDOR_AMD_1 && + env->cpuid_vendor2 == CPUID_VENDOR_AMD_2 && + env->cpuid_vendor3 == CPUID_VENDOR_AMD_3) { + env->cpuid_ext2_features &= ~CPUID_EXT2_AMD_ALIASES; + env->cpuid_ext2_features |= (env->cpuid_features + & CPUID_EXT2_AMD_ALIASES); + } + if (!kvm_enabled()) { env->cpuid_features &= TCG_FEATURES; env->cpuid_ext_features &= TCG_EXT_FEATURES;
when CPU properties are implemented, ext2_features may change between object_new(CPU) and cpu_realize_fn(). Sanitizing ext2_features for AMD based CPU at realize() time will keep current behavior after CPU features are converted to properties. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v2: - style fix, make line shorter than 80 characters amd san --- target-i386/cpu.c | 21 +++++++++++---------- 1 files changed, 11 insertions(+), 10 deletions(-)