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

[Armin Wolf]

Am 19.12.23 um 12:01 schrieb 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.

Sure, its an Asus Prime B650-Plus motherboard.

> > 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.
I understand, is there a standard function for dumping binary data?

> > +
> > +       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.

The format of this data block is documented for RISC-V and LoongArch,
and for the last we can access such SMBIOS data:

https://linux-hardware.org/?probe=97348ef480

I could implement decoding of the data block for this architecture,
but not for RISC-V (i have not found any RISC-V SMBIOS dumps online).
Would this be a good reason to add support for type 44 entries?

Armin Wolf