[v2,09/15] target-i386: Add property getter for CPU model

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-i386/cpu.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

On Tue, Apr 24, 2012 at 11:33:35AM +0200, Andreas Färber wrote:
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  target-i386/cpu.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 9479717..643289f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -640,6 +640,18 @@ static void x86_cpuid_version_set_family(Object *obj, Visitor *v, void *opaque,
>      }
>  }
>  
> +static void x86_cpuid_version_get_model(Object *obj, Visitor *v, void *opaque,
> +                                        const char *name, Error **errp)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    CPUX86State *env = &cpu->env;
> +    int64_t value;
> +
> +    value = (env->cpuid_version >> 4) & 0xf;
> +    value |= ((env->cpuid_version >> 16) & 0xf) << 4;
> +    visit_type_int(v, &value, name, errp);
> +}
> +

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

Just a note though,

The setter code does:

    env->cpuid_version &= ~0xf00f0;
    env->cpuid_version |= ((model & 0xf) << 4) | ((model >> 4) << 16);

So as a result I think there's a potential for the getter to not report bits
that were incorrectly set and exposed to the guest, since we mask off
bits outside the valid range in your code. But that would be a bug in the
setter code/cpudef of course and could be addressed outside this series.

>  static void x86_cpuid_version_set_model(Object *obj, Visitor *v, void *opaque,
>                                          const char *name, Error **errp)
>  {
> @@ -1557,7 +1569,7 @@ static void x86_cpu_initfn(Object *obj)
>                          x86_cpuid_version_get_family,
>                          x86_cpuid_version_set_family, NULL, NULL, NULL);
>      object_property_add(obj, "model", "int",
> -                        NULL,
> +                        x86_cpuid_version_get_model,
>                          x86_cpuid_version_set_model, NULL, NULL, NULL);
>      object_property_add(obj, "stepping", "int",
>                          NULL,
> -- 
> 1.7.7
> 
>

Am 24.04.2012 18:36, schrieb Michael Roth:
> On Tue, Apr 24, 2012 at 11:33:35AM +0200, Andreas Färber wrote:
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  target-i386/cpu.c |   14 +++++++++++++-
>>  1 files changed, 13 insertions(+), 1 deletions(-)
>>
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 9479717..643289f 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -640,6 +640,18 @@ static void x86_cpuid_version_set_family(Object *obj, Visitor *v, void *opaque,
>>      }
>>  }
>>  
>> +static void x86_cpuid_version_get_model(Object *obj, Visitor *v, void *opaque,
>> +                                        const char *name, Error **errp)
>> +{
>> +    X86CPU *cpu = X86_CPU(obj);
>> +    CPUX86State *env = &cpu->env;
>> +    int64_t value;
>> +
>> +    value = (env->cpuid_version >> 4) & 0xf;
>> +    value |= ((env->cpuid_version >> 16) & 0xf) << 4;
>> +    visit_type_int(v, &value, name, errp);
>> +}
>> +
> 
> Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> 
> Just a note though,
> 
> The setter code does:
> 
>     env->cpuid_version &= ~0xf00f0;
>     env->cpuid_version |= ((model & 0xf) << 4) | ((model >> 4) << 16);
> 
> So as a result I think there's a potential for the getter to not report bits
> that were incorrectly set and exposed to the guest, since we mask off
> bits outside the valid range in your code. But that would be a bug in the
> setter code/cpudef of course and could be addressed outside this series.

Sorry, I don't follow... Are you missing the if (value > 0xff) return;
path in the setter (05/15)? Or do you have example numbers that break?

I did it in two lines due to the 80-char limit. And env->cpuid_version
contains more than just the model so we must mask in the getter.

Are you saying the 16-bit limit is wrong and there should be a third
nibble somewhere?

Andreas

On Tue, Apr 24, 2012 at 06:50:28PM +0200, Andreas Färber wrote:
> Am 24.04.2012 18:36, schrieb Michael Roth:
> > On Tue, Apr 24, 2012 at 11:33:35AM +0200, Andreas Färber wrote:
> >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> ---
> >>  target-i386/cpu.c |   14 +++++++++++++-
> >>  1 files changed, 13 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >> index 9479717..643289f 100644
> >> --- a/target-i386/cpu.c
> >> +++ b/target-i386/cpu.c
> >> @@ -640,6 +640,18 @@ static void x86_cpuid_version_set_family(Object *obj, Visitor *v, void *opaque,
> >>      }
> >>  }
> >>  
> >> +static void x86_cpuid_version_get_model(Object *obj, Visitor *v, void *opaque,
> >> +                                        const char *name, Error **errp)
> >> +{
> >> +    X86CPU *cpu = X86_CPU(obj);
> >> +    CPUX86State *env = &cpu->env;
> >> +    int64_t value;
> >> +
> >> +    value = (env->cpuid_version >> 4) & 0xf;
> >> +    value |= ((env->cpuid_version >> 16) & 0xf) << 4;
> >> +    visit_type_int(v, &value, name, errp);
> >> +}
> >> +
> > 
> > Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> > 
> > Just a note though,
> > 
> > The setter code does:
> > 
> >     env->cpuid_version &= ~0xf00f0;
> >     env->cpuid_version |= ((model & 0xf) << 4) | ((model >> 4) << 16);
> > 
> > So as a result I think there's a potential for the getter to not report bits
> > that were incorrectly set and exposed to the guest, since we mask off
> > bits outside the valid range in your code. But that would be a bug in the
> > setter code/cpudef of course and could be addressed outside this series.
> 
> Sorry, I don't follow... Are you missing the if (value > 0xff) return;
> path in the setter (05/15)? Or do you have example numbers that break?

Sorry, you're right, I missed the range check you added in 05/15. Looks
good.

> 
> I did it in two lines due to the 80-char limit. And env->cpuid_version
> contains more than just the model so we must mask in the getter.
> 
> Are you saying the 16-bit limit is wrong and there should be a third
> nibble somewhere?
> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>