Re: [PATCH] fhandler_proc.cc(format_proc_cpuinfo): fix issues, add fields, flags

[Brian Inglis <Brian dot Inglis at SystematicSw dot ab dot ca>]

On 2019-10-05 15:06, Ken Brown wrote:
> On 10/4/2019 6:44 AM, Brian Inglis wrote:
>> fix cache size return code handling and make AMD/Intel code common;
>> fix cpuid level count as number of non-zero leafs excluding sub-leafs;
>> fix AMD physical cores count to be documented nc + 1;
>> round cpu MHz to correct Windows and match Linux cpuinfo;
>> add microcode from Windows registry Update Revision REG_BINARY;
>> add bogomips which has been cpu MHz*2 since Pentium MMX;
>> handle as common former Intel only feature flags also supported on AMD;
>> add 88 feature flags inc. AVX512 extensions, AES, SHA with 20 cpuid calls;
>> commented out flags are mostly used but not currently reported in cpuinfo
>> but some may not currently be used by Linux
> 
> Thanks!  This must have been a lot of work.

Already had the info in some of my own code, that pointed out the discrepancies
between Cygwin and Linux, and prompted the desired to level up.

> It would be easier to review if you would split it up into smaller patches, each 
> doing one thing, to the extent that this makes sense.  For example, the 
> simplification achieved by using the ftcprint macro could be done in a single 
> patch that's separate from the substantive changes.

Unfortunately, that was added later to make the got it/add it/skip it flag cross
checks in Linux order more certain vs my own sequential tabular source.

> A few nits:
> 
>> -      DWORD cpu_mhz = 0;
>> -      RTL_QUERY_REGISTRY_TABLE tab[2] = {
>> +      DWORD cpu_mhz;
>> +      DWORD bogomips;
>> +      long long microcode = 0xffffffff;	/* at least 8 bytes for AMD */
>> +      union {
>> +	  LONG len;
>> +	  char uc_microcode[16];
>> +      } uc;
>> +
>> +      cpu_mhz = 0;
>> +      bogomips = 0;
>> +      microcode = 0;
> 
> Why change the existing intialization style?  How about
> 
>        DWORD cpu_mhz = 0;
>        DWORD bogomips = 0;
>        long long microcode = 0;	/* at least 8 bytes for AMD */

Need to ensure they are initialized each time thru the CPU loop, not just once
on entry, mainly because of what I found out about what I needed to do to get
the variable length REG_BINARY key.

>> +      memset(&uc, 0, sizeof(uc.uc_microcode));
>                ^              ^
> (Space before parenthesis, here and in several other places.)
> 
>> +      cpu_mhz = ((cpu_mhz - 1)/10 + 1)*10;	/* round up a digit */
> 
> Please surround '/' and '*' by spaces (and similarly in what follows).  Also, 
> the comment is confusing.  Maybe "round up to a multiple of 10"?
> 
>> +      for (uint32_t l = maxf; 1 < l; --l) {
>                                              ^
> (Brace on next line, and also in one other place.)

Sorry missed the expected style in those places.
Will tweak and submit v2.

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.