[Libva] [PATCH 1/2] Use the inline CPUID assembly to obtain CPUID string instead of /proc/cpuinfo

[Sean V Kelley]

Hi,

On Wed, Sep 24, 2014 at 2:10 AM, Gwenole Beauchesne <gb.devel at gmail.com> wrote:
> Hi,
>
> Great to see that finally fixed.
>
> 2014-09-24 9:13 GMT+02:00 Zhao Yakui <yakui.zhao at intel.com>:
>> On some systems there is no access to /proc/cpuinfo. So the inline assembly
>> is used directly to detect the CPUID string.
>>
>> Signed-off-by: Zhao Yakui <yakui.zhao at intel.com>
>> ---
>>  src/i965_device_info.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/src/i965_device_info.c b/src/i965_device_info.c
>> index 5ebea2a..4ffba61 100755
>> --- a/src/i965_device_info.c
>> +++ b/src/i965_device_info.c
>> @@ -378,6 +378,7 @@ i965_get_device_info(int devid)
>>      }
>>  }
>>
>> +#if 0
>>  static int intel_driver_detect_cpustring(char *model_id)
>>  {
>>      FILE *fp;
>> @@ -416,7 +417,52 @@ static int intel_driver_detect_cpustring(char *model_id)
>>      else
>>          return -EINVAL;
>>  }
>> +#else
>> +static inline void cpuid(unsigned int op,
>> +                         unsigned int *eax, unsigned int *ebx,
>> +                         unsigned int *ecx, unsigned int *edx)
>
> Use uint32_t so that to not make the code below confusing when
> different 32-bit/64-bit asm is involved and you are sure the operands
> are 32-bits, just say so.

Agree.

>
>> +{
>> +        *eax = op;
>> +        *ecx = 0;
>> +        /* ecx is often an input as well as an output. */
>> +        asm volatile("cpuid"
>> +            : "=a" (*eax),
>> +              "=b" (*ebx),
>> +              "=c" (*ecx),
>> +              "=d" (*edx)
>> +            : "0" (*eax), "2" (*ecx)
>> +               :"memory");
>> +}
>
> This will likely fail in 32-bit mode. %ebx is the PIC register there.
> e.g. use %edi as a temporary in this case. And split into two
> versions.
>
> Besides, choose between in/out operands (+) and keep *eax/*ecx
> initializations, or use separate outputs and inputs and initialize
> eax/ecx there, e.g. "a" (op), "c" (0) and drop the *eax/*ecx lines.
> Don't mix both.

Agree and it raises the point this should also be tested against 32bit mode.

Sean

>
>> +
>> +/*
>> + * This function doesn't check the length. And the caller should
>> + * assure that the length of input string should be greater than 48.
>> + */
>> +static int intel_driver_detect_cpustring(char *model_id)
>
> This is totally useless then, worded as is, and doesn't bring in more
> robustness. Use dynamic allocation, or model_id + length of model_id.
>
>> +{
>> +    unsigned int *rdata;
>>
>> +    if (model_id == NULL)
>> +        return -EINVAL;
>> +
>> +    rdata = (unsigned int *)model_id;
>> +
>> +    /* obtain the max supported extended CPUID info */
>> +    cpuid(0x80000000, &rdata[0], &rdata[1], &rdata[2], &rdata[3]);
>> +
>> +    /* If the max extended CPUID info is less than 0x80000004, fail */
>> +    if (rdata[0] < 0x80000004)
>> +       return -EINVAL;
>> +
>> +    /* obtain the CPUID string */
>> +    cpuid(0x80000002, &rdata[0], &rdata[1], &rdata[2], &rdata[3]);
>> +    cpuid(0x80000003, &rdata[4], &rdata[5], &rdata[6], &rdata[7]);
>> +    cpuid(0x80000004, &rdata[8], &rdata[9], &rdata[10], &rdata[11]);
>> +
>> +    *(model_id + 48) = '\0';
>
> That's model_id[48] = '\0'; much simpler to read.
>
>> +    return 0;
>> +}
>> +#endif
>>  /*
>>   * the hook_list for HSW.
>>   * It is captured by /proc/cpuinfo and the space character is stripped.
>> --
>> 1.7.10.1
>>
>> _______________________________________________
>> Libva mailing list
>> Libva at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/libva
>
> Regards,
> --
> Gwenole Beauchesne
> Intel Corporation SAS / 2 rue de Paris, 92196 Meudon Cedex, France
> Registration Number (RCS): Nanterre B 302 456 199
> _______________________________________________
> Libva mailing list
> Libva at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libva



-- 
Sean V. Kelley <sean.v.kelley at intel.com>
Open Source Technology Center / SSG
Intel Corp.