[Libva] [PATCH 1/2] Use the inline CPUID assembly to obtain CPUID string instead of /proc/cpuinfo
Zhao, Yakui
yakui.zhao at intel.com
Wed Sep 24 23:27:47 PDT 2014
On Wed, 2014-09-24 at 23:35 -0600, Gwenole Beauchesne wrote:
> 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?
Generally speaking, the unsigned int will always be equal to
"uint32_t".
I don't know what you want to say.
> >> > +{
> >> > + *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?
In my test I don't compile it with PIC flag.
But as I mentioned that the above code is similar to the implementation
in Linux kernel, I am sure that it is safe on 32/64.
>
> Oh, BTW, forgot to mention: use __asm__ __volatile__. Though, I reckon
> this would be too much of nitpicking, :)
I am OK to use __asm__ __volatile__. It will be updated in next
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.
>
> 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?
Yes.
> 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?
After reading the spec of GCC inline assembly and CPUID instruction, we
can also write the similar style. But it is accidently the same.
The following is one example about cpuid inline instruction, which is
described in http://www.ibm.com/developerworks/library/l-ia
> asm ("cpuid"
: "=a" (_eax),
"=b" (_ebx),
"=c" (_ecx),
"=d" (_edx)
: "a" (op));
After the above exmaple is reworked, it can be similar to the
implementation in kernel.
OK. I will use what Matt Turner suggested to obtain the CPUID string.
Thanks.
Yakui
>
> 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,
> >
> >
>
>
>
More information about the Libva
mailing list