Re: [dmidecode] [PATCH] dmidecode: Support new format of Processor ID from SMBIOS 3.4.0

[Jean Delvare]

Hi Nhi,

On Tue, 17 Aug 2021 20:04:40 +0700, Nhi Pham via dmidecode-devel wrote:
> For ARM64-class CPUs, the format of the Processor ID field depends on
> the processor's support of the SMCCC_ARCH_SOC_ID architectural call.
> This supports decoding the Processor ID correctly in case the SoC ID is
> supported. This patch should work for ARM32-class CPUs also.
> 
> Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
> ---
>  dmidecode.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/dmidecode.c b/dmidecode.c
> index 69ea0e8..a44f969 100644
> --- a/dmidecode.c
> +++ b/dmidecode.c
> @@ -1126,18 +1126,35 @@ static void dmi_processor_id(const struct dmi_header 
> *h)
>       else if ((type >= 0x100 && type <= 0x101) /* ARM */
>             || (type >= 0x118 && type <= 0x119)) /* ARM */
>       {
> -             u32 midr = DWORD(p);
> +             u64 id = QWORD(p);

While the specification first describes this field as a QWORD, this is
essentially a placeholder. How the bytes within are being grouped
depends on the condition tested below. But in both ARM cases, the
specification clearly states that the field is split in two DWORD
values. So I see no point in going with one QWORD variable, which makes
the code harder to read. Also note that you don't use the value of this
field in your test, so it isn't mandatory to read it early.

Plus going with QWORD here could get wrong later if the code is reused
for an architecture with a different endianness.

>               /*
> -              * The format of this field was not defined for ARM processors
> -              * before version 3.1.0 of the SMBIOS specification, so we
> -              * silently skip it if it reads all zeroes.
> -              */
> -             if (midr == 0)
> -                     return;
> -             pr_attr("Signature",
> -                     "Implementor 0x%02x, Variant 0x%x, Architecture %u, 
> Part 0x%03x, Revision %u",
> -                     midr >> 24, (midr >> 20) & 0xF,
> -                     (midr >> 16) & 0xF, (midr >> 4) & 0xFFF, midr & 0xF);
> +             * The field's format depends on the processor's support of
> +             * the SMCCC_ARCH_SOC_ID architectural call. Software can 
> determine
> +             * the support for SoC ID by examining the Processor 
> Characteristicsfield

Missing space before "field".

> +             * for "Arm64 SoC ID" bit.
> +             */
> +             if (((WORD(data + 0x26) & 0x200) >> 9) != 0)

Shifting isn't needed, masking is enough. Best is to use the bit number
to generate the mask, so there is no room for mistake:

                if (WORD(data + 0x26) & (1 << 9))

Also this needs a length test. You have no guarantee at this point that
the DMI record is long enough to include the "Processor
Characteristics" field. So you could be reading something completely
different or even reading beyond the DMI table. If the record is not
long enough to include this field then you should assume that the
"Arm64 SoC ID" bit isn't set.

> +             {
> +                     /*
> +                     * If Soc ID is supported, the first DWORD is the 
> JEP-106 code;
> +                     * the second DWORD is the SoC revision value.
> +                     */

Hmm, this is once again calling for a shared document listing the
JEP-106 vendor names (like we have /usr/share/pci.ids for PCI vendor
names). Out of scope for this patch, obviously, I'm just thinking out
loud.

Is there any public documentation for the values returned by the
SMCCC_ARCH_SOC_ID call? Your code below is dealing with more than just
a "JEP-106 code", but it's hard to review when I don't know the exact
format. All I have for now is some source code in the Linux kernel
(specifically drivers/firmware/smccc/soc_id.c).

> +                     pr_attr("Signature",
> +                             "JEP-106 Code 0x%02X%02X, SoC ID 0x%02X, SoC 
> Revision 0x%X",
> +                             (id.h >> 24) & 0x7F, (id.h >> 16) & 0xFF, id.h 
> & 0xF, id.l);

The way you display the JEP-106 code is confusing. Bits 30..24 contain
the "bank" in which the manufacturer is located, and then bits 23..16 contain
the manufacturer's ID (or offset) within that bank. However the MSB
of that field is a parity bit, so it's not really part of the code.
This bit should be discarded. In theory it could be checked first,
however to be honest I see no reason for doing that, as this field
isn't any more likely to contain an error than any other field in the
DMI table. It's a typical case of over-engineering I'm afraid.

While it would be possible to assemble both 7-bit values (bank and
manufacturer's code) into a single 14-bit number, I've not seen this
done before, and the JEP-106 document doesn't mention anything like
that, so I would avoid it. As the presence or absence of parity bit
creates some confusion, I believe it is preferable to explicitly print
the bank and the manufacturer's ID code separately.

IF you disagree and still want to print it all as a single value, then
I recommend using the same format as used in the Linux kernel for
consistency, which is: "%02x%02x", bank, offset-without-parity.

https://elixir.bootlin.com/linux/v5.13.12/source/drivers/firmware/smccc/soc_id.c#L88

Is there any reason to restrict the SoC ID to a 4-bit value? The code
in the Linux kernel suggests it's a 16-bit value. At any rate, your
code is inconsistent (4-bit mask but 8-bit format).

> +             } else {
> +                     /*
> +                     * The format of this field was not defined for ARM 
> processors
> +                     * before version 3.1.0 of the SMBIOS specification, so 
> we
> +                     * silently skip it if it reads all zeroes.
> +                     */
> +                     if (id.l == 0)
> +                             return;
> +                     pr_attr("Signature",
> +                             "Implementor 0x%02x, Variant 0x%x, Architecture 
> %u, Part 0x%03x, Revision %u",
> +                             id.l >> 24, (id.l >> 20) & 0xF,
> +                             (id.l >> 16) & 0xF, (id.l >> 4) & 0xFFF, id.l & 
> 0xF);
> +             }
>               return;
>       }
>       else if ((type >= 0x0B && type <= 0x15) /* Intel, Cyrix */

Thanks,
-- 
Jean Delvare
SUSE L3 Support