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

[Nhi Pham]

Hi Jean,

On 25/08/2021 22:54, Jean Delvare wrote:
> 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.
Thanks, will read them in each DWORD.
> 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".
Will fix.
> > +               * 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))
Thanks, will fix.
> 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.
I got it. Thanks, I will add the length test.
> > +               {
> > +                       /*
> > +                       * 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).
The SMC Calling Convention spec 
(https://developer.arm.com/documentation/den0028/d/) describes the 
values returned by SMCCC_ARCH_SOC_ID call.
> > +                       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,

Thanks for catching that. I will correct the mask.

According to SMCC spec, the format of JEP-106 code is as follows:

Bit[31] must be zero

Bit[30:24] JEP-106 bank index for SiP

Bit[23:16] JEP-106 identification code with parity bit for the SiP

> 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.
Totally agree. Printing separately also looks apparent and makes them 
easier to check.
> 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).

My bad, the mask of SoC ID must be 16-bit value as described in the SMCC 
spec.

Thank you so much for all your valuable comments! I will address all 
comments and send v2 of this patch shortly.

Nhi

> > +               } 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,