Robert Lunnon bobl@optushome.com.au writes:
Improves CPUID instruction support in cpu.c by reorganising the existing FREEBSD code. Added more robust cpuid detection with built in 386 detection. Code should work for all i386 platforms and has been tested under solaris. Created subroutines for detecting number of CPUs and OS SSE support as this can't be done in a uniform way across different OSes. Extracted relevant code from each section to perform these OS specific operations (Mostly untested).
It's hard to tell exactly what has changed, since you basically rewrote the whole thing, for no good reason IMO. Could you please try to merge your changes with the existing code instead?
On Thursday 15 January 2004 10:45, Alexandre Julliard wrote:
Robert Lunnon bobl@optushome.com.au writes:
Improves CPUID instruction support in cpu.c by reorganising the existing FREEBSD code. Added more robust cpuid detection with built in 386 detection. Code should work for all i386 platforms and has been tested under solaris. Created subroutines for detecting number of CPUs and OS SSE support as this can't be done in a uniform way across different OSes. Extracted relevant code from each section to perform these OS specific operations (Mostly untested).
It's hard to tell exactly what has changed, since you basically rewrote the whole thing, for no good reason IMO. Could you please try to merge your changes with the existing code instead?
I see no reason to, the code functions perfectly well as it is, I restructured it for a couple of reasons.
1. Firstly I had the code available tested and working. Increasing the distribution of the code through cpu.c risked breaking it.
2. The code as it stood was hard to understand particularly in seeing what was being detected from the cpuid instruction due to hardcoded shifts and masks. I think this code is much easier to read ( Especially without the CPUID instruction documentation in front of you )
3. Merging was difficult due to the specific calls to OS functions for number of cpus and the hardcoded numbers.
4. The code as it stands is more modular and more extensible/maintainable. For example Cyrix or Winchip support should be trivial to add as would support for other cpuid extensions (IA64 perhaps) Only CPUID_GetId should need to be changed to support future cpuid extensions assuming the chip manufactures follow the same calling conventions.
To assist I will explain the scope of the changes
1. The original cpuid and i386 detect are merged into a single assembly call for convenience.
2. The data is then interpreted into a structure for use. Understanding CPUID means knowing the cpuid_t structure and CPUID_IsFeatureAvailable call. All the specifics are isolated into GetId for maintainablity.
3. The OS Specifics for testing OS support of multiple CPUs and SSE were moved into their own subroutines. This abstracts these functions to permit the code in the FREEBSD section to be OS independent. Note that I copied/added code to each of these subroutines to support all the platforms that were already supported for that feature. Much of this code is currently redundant unless someone wants to change the existing linux or NETBSD sections (Which currently use other methods). The redundant code was added to allow this to happen.
3. The information collection and feature tests in the FREEBSD code were changed to use cpuid_t and IsFeatureAvailable in a way exactly equivalent to the original adding support for detection of PAE and SSE in the process (Though OS_SSESupported will need a test for SSE for FREEBSD to work) There is no test for Solaris SSE test since SSE isn't supported at all there (Yet). So there isn't a documented way to query it.
5. Finally #elif defined (__FREEBSD__) was changed to #elif defined (__i386__) since the code within now works for all i386 platforms. (Ultimately this was the objective of the patch)
The #ifdef sections for linux and NETBSD remain unchanged for the moment in order to minimise the possibility of me breaking anything.
Bob
Robert Lunnon bobl@optushome.com.au writes:
- The original cpuid and i386 detect are merged into a single assembly call
for convenience.
Much less readable IMO.
- The data is then interpreted into a structure for use. Understanding CPUID
means knowing the cpuid_t structure and CPUID_IsFeatureAvailable call. All the specifics are isolated into GetId for maintainablity.
There's no reason to introduce that structure, we can set the Windows fields directly. We don't need the specifics of CPUID anywhere else than for setting the corresponding bits which already have well defined names.
- The OS Specifics for testing OS support of multiple CPUs and SSE were moved
into their own subroutines. This abstracts these functions to permit the code in the FREEBSD section to be OS independent. Note that I copied/added code to each of these subroutines to support all the platforms that were already supported for that feature. Much of this code is currently redundant unless someone wants to change the existing linux or NETBSD sections (Which currently use other methods). The redundant code was added to allow this to happen.
On linux your new code will cause multiple opening and parsing of /proc/cpuinfo (and leak file descriptors...)
- The information collection and feature tests in the FREEBSD code were
changed to use cpuid_t and IsFeatureAvailable in a way exactly equivalent to the original adding support for detection of PAE and SSE in the process (Though OS_SSESupported will need a test for SSE for FREEBSD to work) There is no test for Solaris SSE test since SSE isn't supported at all there (Yet).
The new IsFeatureAvailable thing results in calling cpuid and parsing the results again and again at least a dozen times for no good reason.
Please consider working with the existing code instead of throwing it all away and replacing it by your own favorite routines which simply don't fit in with the rest of the code.
On Thursday 15 January 2004 15:53, Alexandre Julliard wrote:
Robert Lunnon bobl@optushome.com.au writes:
- The original cpuid and i386 detect are merged into a single assembly
call for convenience.
Much less readable IMO.
A little less readable in my opinion, this assembly code is documented in the intel/AMD handbooks as implemented. It performs a single logical function by determining CPUID information from 386 through Athlon.
- The data is then interpreted into a structure for use. Understanding
CPUID means knowing the cpuid_t structure and CPUID_IsFeatureAvailable call. All the specifics are isolated into GetId for maintainablity.
There's no reason to introduce that structure, we can set the Windows fields directly. We don't need the specifics of CPUID anywhere else than for setting the corresponding bits which already have well defined names.
We disagree, I think this is much easier to maintain, the names I use relating to registers are reflected in the cpuid documentation from intel and AMD so the actuall cpuid details are clearer. Because the feature names are used in adding the equivalent features to the Windows Structures it is easy to see where this information comes from.
- The OS Specifics for testing OS support of multiple CPUs and SSE were
moved into their own subroutines. This abstracts these functions to permit the code in the FREEBSD section to be OS independent. Note that I copied/added code to each of these subroutines to support all the platforms that were already supported for that feature. Much of this code is currently redundant unless someone wants to change the existing linux or NETBSD sections (Which currently use other methods). The redundant code was added to allow this to happen.
On linux your new code will cause multiple opening and parsing of /proc/cpuinfo (and leak file descriptors...)
OOPs, yes a bug, I will fix that if you like (or you can add the close call). And yes this re-reading is a side effect of making the cpuid code OS independent (hopefully the linux buffer-cache is good) ... There may be a better way to implement this under linux and avoid the re-reading but without knowing linux I wanted to avoid adding too many bugs that I can't necessarily debug (You already found one !)
- The information collection and feature tests in the FREEBSD code were
changed to use cpuid_t and IsFeatureAvailable in a way exactly equivalent to the original adding support for detection of PAE and SSE in the process (Though OS_SSESupported will need a test for SSE for FREEBSD to work) There is no test for Solaris SSE test since SSE isn't supported at all there (Yet).
The new IsFeatureAvailable thing results in calling cpuid and parsing the results again and again at least a dozen times for no good reason.
This is true, but it is not very time consuming. I had considered caching the cpuid results which would eliminate the multiple handling but didn't feel it was worth the effort. This function does not tend to get called often in windows programs (Usually once) but If you like I'll add the caching.
Please consider working with the existing code instead of throwing it all away and replacing it by your own favorite routines which simply don't fit in with the rest of the code.
I threw very little away, I just abstracted the interface to the cpuid instruction and OS specifics. The change in the assembly code is incidental.
Bob
Robert Lunnon bobl@optushome.com.au writes:
This is true, but it is not very time consuming. I had considered caching the cpuid results which would eliminate the multiple handling but didn't feel it was worth the effort. This function does not tend to get called often in windows programs (Usually once) but If you like I'll add the caching.
All that extra work is simply because you are adding abstractions where none are necessary. It looks like a bad case of Not Invented Here syndrome: you have already submitted that code a number of times, and I have already explained that it's way too complex for what we need, so I didn't put it in. Now someone else has done a simple implementation that works fine, and you are trying to replace it all once again with your original code that I already rejected. If you want to improve the cpuid support, please improve the existing code; as long as you keep pushing your all singing all dancing new implementation you won't get anywhere.
On Friday 16 January 2004 04:16, Alexandre Julliard wrote:
Robert Lunnon bobl@optushome.com.au writes:
This is true, but it is not very time consuming. I had considered caching the cpuid results which would eliminate the multiple handling but didn't feel it was worth the effort. This function does not tend to get called often in windows programs (Usually once) but If you like I'll add the caching.
All that extra work is simply because you are adding abstractions where none are necessary. It looks like a bad case of Not Invented Here syndrome: you have already submitted that code a number of times, and I have already explained that it's way too complex for what we need, so I didn't put it in. Now someone else has done a simple implementation that works fine, and you are trying to replace it all once again with your original code that I already rejected. If you want to improve the cpuid support, please improve the existing code; as long as you keep pushing your all singing all dancing new implementation you won't get anywhere.
This is an incorrect charaterisation, but since you hold the keys to the cvs do as you wish.
BTW: As a side note to the maintainers of rewind, the rewind project is welcome to use all my patches also.
Bob
-
Alexandre Julliard -
Robert Lunnon