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

[Zhao, Yakui]

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

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

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.

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

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