Re: [PATCH] lscpu: get the processor information by DMI

[Jeremy Linton <jeremy.linton@arm.com>]

On 6/14/21 5:41 AM, Karel Zak wrote:
> 
> CC: Masayoshi and Jeffrey,
> 
> On Mon, Jun 14, 2021 at 09:48:45AM +0000, Huang Shijie wrote:
>> The patch :367c85c47286 ("lscpu: use SMBIOS tables on ARM for lscpu")
>> relies on the existence of "/sys/firmware/dmi/entries/4-0/raw",
>> which may not exist in standard linux kernel.
>>
>> But "/sys/firmware/dmi/tables/DMI" should exist and can provide the required
>> processor information.
> 
> Good idea to add a fallback solution.
> 
>> This patch uses "/sys/firmware/dmi/tables/DMI"
>> to get the processor information:
>>    1.) Use the DMI to provide more accurate "Model name" information.
> 
> We've had a long discussion about data from DMI  and we had a few
> attempts to implement it ;-) The conclusion is to differentiate
> between information decoded from IDs and information from BIOS, so now
> we have two fields ct->bios_modelname and ct->modelname (and
> ct->bios_vendor).
> 
> The reason is that in some cases the strings from DMI do not provide
> well-known CPU names and info by user.
> 
>          Vendor ID:           ARM
>          BIOS Vendor ID:      https://www.mellanox.com
>          Model:               0
>          Model name:          Cortex-A72
>          BIOS Model name:     Mellanox BlueField-2 [A0] A72(D08) r1p0
> 
> "Cortex-A72" is pretty well-known, Mellanox BlueField is some
> marketing name, another example:
> 
>          Vendor ID:           Cavium
>          BIOS Vendor ID:      CN8890-2000BG2601-AAP-PR-Y-G
>          Model:               0
>          Model name:          ThunderX 88XX
>          BIOS Model name:     2.0


Yes, I was one of the people who asked that the DMI and the lookup table 
be displayed as different fields and so far it looks like its working 
well. Thanks!

> 
>> After this patch, we can get the lscpu output
>> in Ampere Altra platform:
>>     ---------------------------------------------
>> 	Architecture:                    aarch64
>> 	CPU op-mode(s):                  32-bit, 64-bit
>> 	Byte Order:                      Little Endian
>> 	CPU(s):                          160
>> 	On-line CPU(s) list:             0-159
>> 	Vendor ID:                       ARM
>> 	Model name:                      Ampere(R) Altra(R) Processor Q00-00 CPU @ 3.0GHz

> 
> Should be
> 
>      Model name:          Neoverse-N1
>      BIOS Model name:     Ampere(R) Altra(R) Processor Q00-00 CPU @ 3.0GHz

Right, another example :


Vendor ID:              ARM
   BIOS Vendor ID:       Broadcom
   Model name:           Cortex-A72
     BIOS Model name:    BCM2711 (ARM Cortex-A72)


Which is helpful when comparing with various other utilities, like gcc, 
which take cortex-a72 as -mtune parameters.

> 
>>   static void arm_decode(struct lscpu_cxt *cxt, struct lscpu_cputype *ct)
>>   {
>> +	/* dmi_decode_cputype may get more accurate information later */
>> +	arm_ids_decode(ct);
>> +
>>   	/* use SMBIOS Type 4 data if available */
>>   	if (!cxt->noalive && access(_PATH_SYS_DMI_TYPE4, R_OK) == 0)
>>   		arm_smbios_decode(ct);
>> +	else if (!cxt->noalive && access(_PATH_SYS_DMI, R_OK) == 0)
>> +		dmi_decode_cputype(ct);
>>   
>> -	arm_ids_decode(ct);
> 
> Please, do not move arm_ids_decode().
> 
>> +int dmi_decode_cputype(struct lscpu_cputype *ct)
>> +{
>> +	static char const sys_fw_dmi_tables[] = _PATH_SYS_DMI;
>> +	struct dmi_info di = { };
>> +	struct stat st;
>> +	uint8_t *data;
>> +	int rc = 0;
>> +	char buf[100] = { };
>> +
>> +	if (stat(sys_fw_dmi_tables, &st))
>> +		return rc;
>> +
>> +	data = get_mem_chunk(0, st.st_size, sys_fw_dmi_tables);
>> +	if (!data)
>> +		return rc;
>> +
>> +	rc = parse_dmi_table(st.st_size, st.st_size/4, data, &di);
>> +	if (rc < 0) {
>> +		free(data);
>> +		return rc;
>> +	}
>> +
>> +	/* Get module name */
>> +	sprintf(buf, "%s %s CPU @ %d.%dGHz", di.processor_version, di.part_num,
>> +			di.current_speed/1000, (di.current_speed % 1000) / 100);
> 
> So, it's not string from DMI, but it's composed from more information
> and it seems compatible to "model name:" from (x86) /proc/cpuinfo. I'm
> fine with it.
> 
>> +	free(ct->modelname);
>> +	ct->modelname = xstrdup(buf);
> 
> Please:
> 
>    ct->bios_modelname = xstrdup(buf);
> 
> 
>> +	/* Get CPU family */
>> +	memset(buf, 0, sizeof(buf));
>> +	sprintf(buf, "%d", di.processor_family);
>> +	free(ct->family);
>> +	ct->family = xstrdup(buf);
> 
> is there any difference between "cpu family" from /proc/cpuinfo and
> this DMI field? Do we need a new field ct->bios_family or overwrite
> the ct->family good enough? I don't know ;-)
> 
>    Karel
>