Re: [Qemu-devel] [PATCH 2/6] target-i386: sanitize AMD's ext2_features at realize time

[Igor Mammedov <imammedo@redhat.com>]

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
> > 
>