[Libva] [PATCH 1/2] Use the inline CPUID assembly to obtain CPUID string instead of /proc/cpuinfo
Gwenole Beauchesne
gb.devel at gmail.com
Wed Sep 24 23:57:27 PDT 2014
2014-09-25 8:27 GMT+02:00 Zhao, Yakui <yakui.zhao at intel.com>:
> 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.
Try to build libva with your patch applied in 32-bit. It won't
compile. I would be very surprised if it does.
I mentioned to you that the code was not going to work in 32-bit mode
with PIC (to build a DSO). Again, are you denying this?
>> 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.
If you really followed the sample code your provided, you wouldn't
have that *eax = op; *ecx = 0; code style. You would have been using
clear outputs/inputs, i.e. "=a" (*eax) ... "0" (op), not the code you
provided in patch.
Sorry, but I don't buy your argument, you make impossible statements.
> 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,
>> >
>> >
>>
>>
>>
>
>
--
Gwenole Beauchesne
Intel Corporation SAS / 2 rue de Paris, 92196 Meudon Cedex, France
Registration Number (RCS): Nanterre B 302 456 199
More information about the Libva
mailing list