The issue has been solved by my reviewer, so thank you all because as
usual I've learned interesting things :)
Comments inlined.
On Thu, Sep 12, 2013 at 2:53 PM, Florian Weimer <fweimer@xxxxxxxxxx> wrote:
> On 09/12/2013 02:11 PM, Dridi Boukelmoune wrote:
>
>>> This version should work in 32 bit mode, and only in 32 bit mode:
>>>
>>> void
>>> cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
>>> unsigned int *edx)
>>> {
>>> __asm volatile
>>> ("push %%ebx\n\t"
>>> "cpuid\n\t"
>>> "mov %%ebx, (%1)\n\t"
>>> "pop %%ebx"
>>> : "=a" (*eax),
>>> "=S" (ebx),
>>> "=c" (*ecx),
>>> "=d" (*edx)
>>> : "0" (*eax));
>>> }
>>
>>
>> I "kind of" understand what you're doing here, but it's not all clear.
>>
>> I get the push/pop instructions save and restore the reserved ebx
>> register, which is needed because apparently the cpuid instruction
>> would otherwise overwrite it.
>>
>> I don't understand the mov instruction, but I suppose you're storing
>> ebx's value from cpuid "somewhere else" before restoring it with the
>> pop instruction.
>
>
> Correct, the intent is to write the %ebx register value to the address in
> the %esi register. "(%1)" is a pointer dereference, as oppose to plain %1.
>
>
>> I don't understand the last 5 lines of __asm in both functions, I've
>> never seen this syntax before.
>
>
> These are register constraints. "a", "c", "d", "S" refer to the %eax, %ecx,
> %edx, %esi registers, respectively. "=" marks output constraints. The
> constraints before the final ":" are output registers, and after colon,
> there are the input registers. There's just one, and "0" means to reuse the
> first output register.
>
> Okay, silly me, I should have listed "S" among the output registers, instead
> the inputs:
Actually, this morning (in the train) I've tested your code with my
tmp variable and it works if I remove the deref!
mov %%ebx, %1
(btw, what do %1 and %4 mean ?)
I could painlessly add a println in my patch since the file already
includes stdio.h. Normal and hardened build provide the same cpuid
values.
> void
> cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
> unsigned int *edx)
> {
> __asm volatile
> ("push %%ebx\n\t"
> "cpuid\n\t"
> "mov %%ebx, (%4)\n\t"
> "pop %%ebx"
> : "=a" (*eax),
> "=c" (*ecx),
> "=d" (*edx)
> : "0" (*eax),
> "S" (ebx));
> }
>
> I also forget that for full correctness, there should now be a "memory"
> clobber as well (in the clobber section after yet another colon):
What would it do ? A compiler memory barrier ?
> void
> cpuid(unsigned int *eax, unsigned int *ebx, unsigned int *ecx,
> unsigned int *edx)
> {
> __asm volatile
> ("push %%ebx\n\t"
> "cpuid\n\t"
> "mov %%ebx, (%4)\n\t"
> "pop %%ebx"
> : "=a" (*eax),
> "=c" (*ecx),
> "=d" (*edx)
> : "0" (*eax),
> "S" (ebx)
> : "memory");
> }
It's a good thing my reviewer submitted a patch, because I wouldn't
feel that confident with mine :)
> By the way, we could generate much better code if the registers were passed
> as an array or struct, so that they are in consecutive memory:
>
> struct regs {
> unsigned eax, ebx, ecx, edx;
> };
>
> void
> cpuid(struct regs *r)
>
> {
> __asm volatile
> ("push %%ebx\n\t"
> "cpuid\n\t"
> "mov %%eax, (%0)\n\t"
> "mov %%ebx, 4(%0)\n\t"
> "mov %%ecx, 8(%0)\n\t"
> "mov %%edx, 12(%0)\n\t"
> "pop %%ebx"
> :
> : "S" (r)
> : "eax", "ecx", "edx", "memory");
> }
>
> Obviously, this needs adjustments to the callers.
Yup, but for the sake of simplicity, I wouldn't do that.
> --
> Florian Weimer / Red Hat Product Security Team
Dridi
--
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
