Patchwork [2/6] target-i386: sanitize AMD's ext2_features at realize time

login
register
mail settings
Submitter Igor Mammedov
Date Dec. 11, 2012, 10:11 a.m.
Message ID <1355220666-31722-3-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/205152/
State New
Headers show

Comments

Igor Mammedov - Dec. 11, 2012, 10:11 a.m.
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(-)
Eduardo Habkost - Dec. 11, 2012, 1:31 p.m.
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
>
Igor Mammedov - Dec. 11, 2012, 1:41 p.m.
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
> > 
>
Igor Mammedov - Dec. 11, 2012, 2:08 p.m.
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
> > 
>
Eduardo Habkost - Dec. 11, 2012, 2:25 p.m.
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.  :-)

Patch

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;