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

[Gwenole Beauchesne]

Hi,

2014-09-25 3:00 GMT+02:00 Zhao, Yakui <yakui.zhao at intel.com>:
> On Wed, 2014-09-24 at 03:10 -0600, Gwenole Beauchesne 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.
>>
>
> OK. I can change it to uint32_t.
>
> (In fact the "unsigned int" is treated as 32 bit).

And? You can't claim in a previous thread to aim for better human
readability, and now claiming you don't care because of unsigned int
== uint32_t?

>> > +{
>> > +        *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.
>>
>
> I don't think that it is necessary to split it into two version. It can
> work on both 32/64-bit version.

I am 100% sure your code won't even compile in 32-bit mode with PIC
code, which is the default when compiling a DSO. Are you denying this?

Oh, BTW, forgot to mention: use __asm__ __volatile__. Though, I reckon
this would be too much of nitpicking, :)

> When the inline assembly is handled, the compiler will help to arrange
> the used register list and do some extra protections. In such case the
> programmer don't care the used register list.

Is your comment supposed to attest that you still think your code is
going to work in 32-bit mode and you don't want to change it?

> I suggest that you can take a look at the cpuid implementation in
> Linux_kernel.
> The following is another example of cpuid.
>   http://www.ibm.com/developerworks/library/l-ia/
>
> (In fact the above similar code can be found in Linux kernel. Be sure
> that it can work on 32-bit/64-bit version. And it has no problem about
> the used register list).

Are you saying that because your code is "similar" to Kernel code, so
you are sure it works for you here? Honestly?
Or, are you saying "similar" so that we don't think you copied GPLv2
code snippet as is to bring it into a MIT-like licensed project?

Though, cpuid should be trivial code, but here, it's additionally broken.

>> 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.
>>
>> > +
>> > +/*
>> > + * 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