Re: [PATCH 1/3] dmidecode: Add support for type 44 entries

[Jean Delvare]

Hi Armin,

On Tue, 19 Dec 2023 01:43:24 +0100, Armin Wolf via dmidecode-devel wrote:
> Type 44 is called "Processor Additional Information" and provides
> additional information about the installed processor.

Do you have access to a system which implements this type? If you do, I
would appreciate a copy of the DMI table for my collection. You can
generate it with "dmidecode --dump-bin" and send it to me privately.

> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
>  dmidecode.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/dmidecode.c b/dmidecode.c
> index 77cb2fc..d1b179b 100644
> --- a/dmidecode.c
> +++ b/dmidecode.c
> @@ -4331,6 +4331,51 @@ static void dmi_tpm_characteristics(u64 code)
>                       pr_list_item("%s", characteristics[i - 2]);
>  }
> 
> +/*
> + * 7.45 Processor Additional Information (Type 44)
> + */
> +
> +static void dmi_processor_specific_block(const u8 *data, u8 length)
> +{
> +     static const char *architectures[] = {
> +             "Reserved",
> +             "x86",
> +             "x86-64",
> +             "Intel Itanium",
> +             "Aarch32",
> +             "Aarch64",
> +             "RV32",
> +             "RV64",
> +             "RV128",
> +             "LoongArch32",
> +             "LoongArch64" /* 0x0A */
> +     };
> +     u8 block_length, processor_type;
> +     int i;
> +
> +     if (length < 0x02)
> +             return;

This check would rather be on the caller side.

> +
> +     block_length = data[0x00];
> +     if (block_length + 0x02 > length)
> +             block_length = length;

If data is inconsistent then you just can't trust it, I'd rather report
that as an error rather than silently fixing it up and hoping for the best.

> +
> +     processor_type = data[0x01];
> +     if (processor_type > 0x0A)
> +             pr_attr("Processor Type", out_of_spec);
> +     else
> +             pr_attr("Processor Type", "%s", architectures[processor_type]);
> +
> +     if (block_length == 0)
> +             return;
> +
> +     pr_list_start("Processor-Specific Data", NULL);
> +     for (i = 0; i < block_length; i++)
> +             pr_list_item("%02X", data[i + 0x02]);

Do we have any idea what the data means?

If we are going to print raw data then it should be formatted better,
one number per line is taking way too much space. But see below.

> +
> +     pr_list_end();
> +}
> +
>  /*
>   * 7.46 Firmware Inventory Information (Type 45)
>   */
> @@ -5435,6 +5480,15 @@ static void dmi_decode(const struct dmi_header *h, u16 
> ver)
>                               DWORD(data + 0x1B));
>                       break;
> 
> +             case 44: /* 7.45 Processor Additional Information */
> +                     pr_handle_name("Processor Additional Information");
> +                     if (h->length < 0x06)
> +                             break;

Please stick to the coding style used for other record types. I know
it's unconventional, but let's be consistent.

> +
> +                     pr_attr("Referenced Handle", "0x%04X", WORD(data + 
> 0x04));
> +                     dmi_processor_specific_block(data + 0x06, h->length - 
> 0x06);
> +                     break;

When type 44 was added, I looked at it and decided not to add support
for it because I didn't see any value in it. And this patch comforts me
in my opinion. Your code is going to print 2 things:
* The processor architecture, which is already available in the
  associated main CPU record (type 4).
* Binary data which we aren't able to decode.

I just can't see how adding this to the output of dmidecode is helpful.
As long as we don't know the meaning of the data block, I'd rather not
decode type 44 at all.

-- 
Jean Delvare
SUSE L3 Support