Message ID | 94338540-4bcc-7ad7-9de1-944c0dc96709@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | tools/xen-detect: avoid possible pitfall with cpuid() | expand |
On 03.12.2021 13:09, Jan Beulich wrote: > The 64-bit form forces %ecx to 0 while the 32-bit one so far didn't - it > only ended up that way when "pv_context" is zero. While presently no > leaf queried by callers has separate subleaves, let's avoid chancing it. > > While there > - replace references to operands by number, > - relax constraints where possible, > - limit PUSH/POP to just the registers not also used as input, > all where applicable also for the 64-bit variant. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Any chance of getting an ack or otherwise here? (Anthony, I realize you've not been on the Cc list originally, as the patch we sent before the maintainership adjustment.) Jan > --- > I'm pretty sure %edx also wouldn't need to be subject to PUSH/POP here, > but I didn't want to go more "against" the comment than obviously > justifiable by the input registers used. In fact I've observed gcc to > pick %edx for putting "pv_context" in. > > --- a/tools/misc/xen-detect.c > +++ b/tools/misc/xen-detect.c > @@ -52,17 +52,19 @@ static void cpuid(uint32_t idx, uint32_t > #ifdef __i386__ > /* Use the stack to avoid reg constraint failures with some gcc flags */ > asm volatile ( > - "push %%eax; push %%ebx; push %%ecx; push %%edx\n\t" > - "test %1,%1 ; jz 1f ; ud2a ; .ascii \"xen\" ; 1: cpuid\n\t" > - "mov %%eax,(%2); mov %%ebx,4(%2)\n\t" > - "mov %%ecx,8(%2); mov %%edx,12(%2)\n\t" > - "pop %%edx; pop %%ecx; pop %%ebx; pop %%eax\n\t" > - : : "a" (idx), "c" (pv_context), "S" (regs) : "memory" ); > + "push %%ebx; push %%edx\n\t" > + "test %[pv],%[pv] ; jz 1f ; ud2a ; .ascii \"xen\" ; 1: cpuid\n\t" > + "mov %%eax,(%[regs]); mov %%ebx,4(%[regs])\n\t" > + "mov %%ecx,8(%[regs]); mov %%edx,12(%[regs])\n\t" > + "pop %%edx; pop %%ebx\n\t" > + : "+a" (idx), "=c" (idx /* dummy */) > + : "c" (0), [pv] "r" (pv_context), [regs] "SD" (regs) > + : "memory" ); > #else > asm volatile ( > - "test %5,%5 ; jz 1f ; ud2a ; .ascii \"xen\" ; 1: cpuid\n\t" > + "test %[pv],%[pv] ; jz 1f ; ud2a ; .ascii \"xen\" ; 1: cpuid\n\t" > : "=a" (regs[0]), "=b" (regs[1]), "=c" (regs[2]), "=d" (regs[3]) > - : "0" (idx), "1" (pv_context), "2" (0) ); > + : "0" (idx), "2" (0), [pv] "r" (pv_context) ); > #endif > } > > >
On Fri, Dec 03, 2021 at 01:09:04PM +0100, Jan Beulich wrote: > The 64-bit form forces %ecx to 0 while the 32-bit one so far didn't - it > only ended up that way when "pv_context" is zero. While presently no > leaf queried by callers has separate subleaves, let's avoid chancing it. > > While there > - replace references to operands by number, > - relax constraints where possible, > - limit PUSH/POP to just the registers not also used as input, > all where applicable also for the 64-bit variant. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com> Thanks,
--- a/tools/misc/xen-detect.c +++ b/tools/misc/xen-detect.c @@ -52,17 +52,19 @@ static void cpuid(uint32_t idx, uint32_t #ifdef __i386__ /* Use the stack to avoid reg constraint failures with some gcc flags */ asm volatile ( - "push %%eax; push %%ebx; push %%ecx; push %%edx\n\t" - "test %1,%1 ; jz 1f ; ud2a ; .ascii \"xen\" ; 1: cpuid\n\t" - "mov %%eax,(%2); mov %%ebx,4(%2)\n\t" - "mov %%ecx,8(%2); mov %%edx,12(%2)\n\t" - "pop %%edx; pop %%ecx; pop %%ebx; pop %%eax\n\t" - : : "a" (idx), "c" (pv_context), "S" (regs) : "memory" ); + "push %%ebx; push %%edx\n\t" + "test %[pv],%[pv] ; jz 1f ; ud2a ; .ascii \"xen\" ; 1: cpuid\n\t" + "mov %%eax,(%[regs]); mov %%ebx,4(%[regs])\n\t" + "mov %%ecx,8(%[regs]); mov %%edx,12(%[regs])\n\t" + "pop %%edx; pop %%ebx\n\t" + : "+a" (idx), "=c" (idx /* dummy */) + : "c" (0), [pv] "r" (pv_context), [regs] "SD" (regs) + : "memory" ); #else asm volatile ( - "test %5,%5 ; jz 1f ; ud2a ; .ascii \"xen\" ; 1: cpuid\n\t" + "test %[pv],%[pv] ; jz 1f ; ud2a ; .ascii \"xen\" ; 1: cpuid\n\t" : "=a" (regs[0]), "=b" (regs[1]), "=c" (regs[2]), "=d" (regs[3]) - : "0" (idx), "1" (pv_context), "2" (0) ); + : "0" (idx), "2" (0), [pv] "r" (pv_context) ); #endif }
The 64-bit form forces %ecx to 0 while the 32-bit one so far didn't - it only ended up that way when "pv_context" is zero. While presently no leaf queried by callers has separate subleaves, let's avoid chancing it. While there - replace references to operands by number, - relax constraints where possible, - limit PUSH/POP to just the registers not also used as input, all where applicable also for the 64-bit variant. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I'm pretty sure %edx also wouldn't need to be subject to PUSH/POP here, but I didn't want to go more "against" the comment than obviously justifiable by the input registers used. In fact I've observed gcc to pick %edx for putting "pv_context" in.