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

[Eduardo Habkost]

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 <address@hidden>
> ---
> 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 <address@hidden>


> +    }
> +
>      if (!kvm_enabled()) {
>          env->cpuid_features &= TCG_FEATURES;
>          env->cpuid_ext_features &= TCG_EXT_FEATURES;
> -- 
> 1.7.1
> 

-- 
Eduardo