Re: CPUID Patch for IDT Winchip
On Tue, May 21, 2019 at 11:20 AM Uros Bizjak wrote:
>
> 2019-05-21 Uroš Bizjak
>
> * config/i386/cpuid.h (__cpuid): For 32bit targets, zero
> %ebx and %ecx bafore calling cpuid with leaf 1 or
> non-constant leaf argument.
>
> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>
> Committed to mainline SVN, will be backported to all active branches.
Has this been backported to active branches? I could only find it in
the 9.2.0 release using the git repositories.
- Matthew
CPUID Patch for IDT Winchip
I have confirmed that the IDT Winchip 2 does not expressly set %ecx
after a call to cpuid() with %eax = 1, and this causes incorrect
reporting of cpu capabilities. The %ecx register should return 0x0
(and likely %ebx should too) on this hardware. This patch proposes a
fix.
The symptoms of this arose on a Winchip 2 system when systemd called
cpuid() with %eax = 1 and it incorrectly reported rdrand() support
(bit 30 set in %ecx). A call to the supposedly supported rdrand()
function triggered an illegal instruction exception, causing the
system to panic early in booting.
The IDT Winchip documentation is silent on what happens to %ecx and
%ebx after such a call (see below). It appears to leave whatever is in
%ecx untouched instead of zeroing it. This patch defensively zeroes
out %ebx:%ecx:%edx before such a call in cpuid.h.
I should be able to test the earlier Winchip C6 model in the future. I
wlll also check older AMD, Cyrix, and VIA processors.
Thanks to Mike Gilbert for suggesting the solution. Thank you also to
Segher Boessenkool for helping get the inline assembly syntax correct.
Here is the assembly code fragment the patch generates with comments:
0xb7e21498 <+88>:xor%edi,%edi <--- zeroes %edi for use below
0xb7e2149a <+90>:mov%edi,%eax
0xb7e2149c <+92>:cpuid
0xb7e2149e <+94>:test %eax,%eax
0xb7e214a0 <+96>:je 0xb7e214c0
0xb7e214a2 <+98>:mov%edi,%ebx <--- zeroes %ebx
0xb7e214a4 <+100>: mov%edi,%ecx <--- zeroes %ecx
0xb7e214a6 <+102>: mov%edi,%edx <--- zeroes %edx
0xb7e214a8 <+104>: mov$0x1,%eax <--- call cpuid with %eax = 1
0xb7e214ad <+109>: cpuid
0xb7e214af <+111>: shr$0x1e
References:
http://datasheets.chipdb.org/IDT/x86/WinChip2/wc2ds.pdf
http://datasheets.chipdb.org/IDT/x86/C6/c6_data_sheet.pdf
- Matthew Whitehead
diff --git a/gcc/config/i386/cpuid.h b/gcc/config/i386/cpuid.h
index b3b0f91..7f6d726 100644
--- a/gcc/config/i386/cpuid.h
+++ b/gcc/config/i386/cpuid.h
@@ -251,6 +251,15 @@ __get_cpuid (unsigned int __leaf,
if (__maxlevel == 0 || __maxlevel < __leaf)
return 0;
+ /* At least one confirmed cpu (Winchip 2) does not set %ecx correctly for
+cpuid() %eax = 1, and leaves garbage in it (usually containing the results
+of a previous call to cpuid() %eax = 0, which puts 'auls' in %ecx from
+'CentaurHauls' in %ebx:%edx:%ecx, the vendor identification string).
+Forcibly zero the three registers before calling cpuid() as a
precaution. */
+
+ *__ebx = *__ecx = *__edx = 0;
+ asm volatile("" : "+b" (*__ebx), "+c" (*__ecx), "+d" (*__edx));
+
__cpuid (__leaf, *__eax, *__ebx, *__ecx, *__edx);
return 1;
}
Re: CPUID Patch for IDT Winchip
Uros, On Mon, May 20, 2019 at 3:29 AM Uros Bizjak wrote: > The CPUID documentation from Intel Architectures Developer's manual is > crystal clear on the implementation: > > --quote-- > CPUID—CPU Identification > > Returns processor identification and feature > information to the EAX, EBX, ECX, and EDX > registers, as determined by input entered in > EAX (in some cases, ECX as well). > --quote-- That is for the current versions of the documentation. Earlier versions are different (see below). > So, I dont see why we should implement the workaround for an > implementation bug in a widely used header for a relatively obscure > target. Please also note that there are other places in the compiler > that assume that the instruction returns values as specified above, > not to mention other applications that assumes the same. > > I think proposed target specific bugfix should be implemented > elsewhere, not in the cpuid.h header that assumes the specified > functionality. I did some research and found the Intel CPUID document that existed when the WinChip was manufactured (dated December 1996): http://bitsavers.trailing-edge.com/components/intel/appNotes/AP-485_Intel_Processor_Identification_and_the_CPUID_Instruction_Dec96.pdf It is ALSO silent about the contents of %ebx and %ecx after a cpuid() with %eax = 1. The Winchip met the Intel _published_ standard that existed at the time. Intel changed the standard a long time afterwards. This means that all the following processor manufacturers (and example cpus) of that era might demonstrate this behavior: AMD (Am486 DX2/DX4, Am5x86, K5) Centaur (WinChip C6, Winchip 2) NexGen (Nx586) Cyrix (Cx486DX2/DX4, 5x86, 6x86, 6x86MX) That is a lot of processors. Admittedly they are old, but they are still officially supported by GNU/Linux. Since the patch only introduces four (4) more opcodes, it is not computationally expensive. Here is the diff: test $0x20,%eax je 0xb7e214c0 - xor%eax,%eax + xor%edi,%edi + mov%edi,%eax cpuid test %eax,%eax je 0xb7e214c0 + mov%edi,%ebx + mov%edi,%ecx + mov%edi,%edx mov$0x1,%eax cpuid Lastly, this is just an example of "initialized your variables before you use them". - Matthew
Re: CPUID Patch for IDT Winchip
On Mon, May 20, 2019 at 11:52 AM Uros Bizjak wrote: > How about the attached patch that enables zeroing only for 32bit > processors, and only for cpuid leaf 1? Usually, __get_cpuid is used > with a constant leaf argument, so we benefit from constant propagation > into inline function. Also, according to the confusing documentation, > it looks to me that zeroing %ecx and %edx regs should be enough. Uros, you said "zeroing %ecx and %edx regs should be enough". That confuses me. The older documentation says that for %eax = 1 it is guaranteed to return a value in %eax and %edx, with %ebx and %ecx being 'Intel reserved. Do not use'. Did you instead mean "zeroing %EBX and %ECX regs should be enough"? Otherwise I like your proposal, I just have to fully understand the changed inline assembly and test it on actual hardware. - Matthew
Re: CPUID Patch for IDT Winchip
On Mon, May 20, 2019 at 2:57 PM Uros Bizjak wrote: > > On Mon, May 20, 2019 at 6:12 PM tedheadster wrote: > > Did you instead mean "zeroing %EBX and %ECX regs should be enough"? > > Ah, yes. This is what I meant to say. The patch clears %ebx and %ecx. > Uros, your patch worked on real 32-bit hardware. The assembly output is nearly identical to mine, with merely a re-ordering when setting the %eax, %ebx, and %ecx registers. But of course your code does not compile into x86_64 systems (nice touch). test $0x20,%eax je 0xb7e214c0 xor%edi,%edi <--- zero for use below mov%edi,%eax cpuid test %eax,%eax je 0xb7e214c0 mov$0x1,%eax mov%edi,%ebx <--- zero register mov%edi,%ecx <--- zero register cpuid If this is accepted upstream, can we please get it backported to some of the major releases: 8.2.0, 7.3.0, 6.4.0 and 5.4.0? - Matthew
