Re: [dmidecode] [PATCH] dmioem: Decode HPE OEM Record 224

[Jean Delvare]

Hi Jerry,

On Tue, 21 Jun 2022 15:59:07 -0600, Jerry Hoemann wrote:
> Decode HPE OEM Record 224: Trusted Module (TPM or TCM) Status (Type 224)
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  dmioem.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
> 
> diff --git a/dmioem.c b/dmioem.c
> index c26fff9..982e94e 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -319,6 +319,95 @@ static void dmi_hp_238_loc(const char *fname, unsigned 
> int code)
>       pr_attr(fname, "%s", str);
>  }
>  
> +static int dmi_hp_224_status(u32 code)

I'd prefer to have these dmi_hp_224_* helper functions after the
dmi_hp_238_* helper function rather than in the middle of them.

> +{
> +     static const char * const present[] = {
> +             "Not Present", /* 0x00 */
> +             "Present/Enabled",
> +             "Present/Disabled",
> +             "Reserved" /* 0x03 */
> +     };
> +
> +     pr_attr("Status", "%s", present[code & 0x03]);
> +     if ((code & 0x03) == 0x00)
> +             return 0;
> +     pr_attr("Option ROM Measuring", "%s", (code & (1 << 2)) ? "Yes" : "No");

For my own enlightenment, what does "Option ROM Measuring" mean?

> +     pr_attr("Visible/Hidden", "%s", (code & (1 << 3)) ? "Hidden" : 
> "Visible");

For consistency I'd go with "Hidden", "Yes", "No".

> +     return 1;
> +}
> +
> +static void dmi_hp_224_ex_status(u32 status, u32 code)
> +{
> +     const char *str = "Reserved";
> +     static const char * const disable_reason[] = {
> +             "Not Specified", /* 0x00 */
> +             "User Disabled",
> +             "Error Condition",
> +             "Reserved"      /* 0x03 */
> +     };
> +     static const char * const error_condition[] = {
> +             "Not Specified", /* 0x00 */
> +             "Self-Test",     /* 0x01 */
> +     };
> +     if ((status & 0x03) == (1 << 1))

status & 0x03 is not a bit field, it's a 2-bit value, so comparing it
with 1 << 1 doesn't really make sense. You should check for 0x02
instead.

> +             pr_attr("Disable Reason", "%s", disable_reason[code & 0x03]);
> +     if ((code & 0x03) == (1 << 1)) {

Same here.

> +             if (((code >> 2) & 0x0f) < ARRAY_SIZE(error_condition))
> +                     str = error_condition[(code >> 2) & 0x0f];

Storing (code >> 2) & 0x0f in a local variable would make the code
clearer IMHO.

> +             pr_attr("Error Condition", "%s", str);
> +     }
> +}
> +
> +static void dmi_hp_224_module_type(u32 code)

The size of the "code" parameter is not consistent with the value
passed to the function.

> +{
> +     const char *str = "Reserved";
> +     static const char * const type[] = {
> +             "Not Specified", /* 0x00 */
> +             "TPM 1.2",
> +             "TPM 2.0",
> +             "Intel PTT fTPM" /* 0x03 */
> +     };
> +     if ((code & 0x0f) < ARRAY_SIZE(type))
> +             str = type[code & 0x0f];
> +     pr_attr("Type", "%s", str);
> +     pr_attr("Standard Algorithm Supported", "%s", (((code >> 4) & 0x0f) & 
> (1 << 0)) ? "Yes" : "No");
> +     pr_attr("Chinese Algorithm Supported", "%s",  (((code >> 4) & 0x0f) & 
> (1 << 1)) ? "Yes" : "No");

Duplicate space after comma (no, we don't do opportunistic code
alignment in dmidecode).

There's some repeated arithmetic here that could be avoided. Either
access the bits directly:

        pr_attr("Standard Algorithm Supported", "%s", code & (1 << 4) ? "Yes" : 
"No");

Or ues a local variable to store the extracted field:

        u8 flags = (code >> 4) & 0x0f;

        (...)
        pr_attr("Standard Algorithm Supported", "%s", flags & (1 << 0) ? "Yes" 
: "No");

Both make the code more readable IMHO. I'd go for the first option for
simplicity but I'm fine either way. If you go for the second option,
note that masking is technically not needed if code is a u8 as I think
it should.

> +}
> +
> +static void dmi_hp_224_module_attr(u32 code)

Parameter size mismatch.

> +{
> +     static const char * const phys_attr[] = {
> +             "Not Specified", /* 0x00 */
> +             "Pluggable and Optional",
> +             "Plubaggable but Stanard",

I don't know these words :-D

I also don't really understand how "Standard" opposes itself to
"Pluggable" thus can't really make sense of the "but" between them.

> +             "Soldered Down on System Board"  /* 0x03 */
> +     };
> +     static const char * const fips_attr[] = {
> +             "Not Specified", /* 0x00 */
> +             "Not FIPS Certified",
> +             "FIPS Certified",
> +             "Reserved"  /* 0x03 */
> +     };
> +     pr_attr("Attributes", "%s", phys_attr[code & 0x3]);

"Attributes" seems a little vague as a description of this field. Can
we use something more descriptive, like "Physical Format" or "Form
Factor" maybe?

> +     pr_attr("FIPS Certification", "%s", fips_attr[((code >> 2) & 0x03)]);
> +}
> +
> +static void dmi_hp_224_chipid(u32 code)

Parameter size mismatch again.

> +{
> +     const char *str = "Reserved";
> +     static const char * const chipid[] = {
> +             "None", /* 0x00 */
> +             "STMicroGen10 TPM",
> +             "Intel firmware TPM (PTT)",
> +             "Nationz TPM",
> +             "STMicroGen10 Plus TPM",
> +             "STMicroGen11 TPM", /* 0x05 */
> +     };
> +     if ((code & 0xff) < ARRAY_SIZE(chipid))
> +             str = chipid[code & 0xff];
> +     pr_attr("Chip Identifier", "%s", str);
> +}
> +
>  static void dmi_hp_238_flags(const char *fname, unsigned int code)
>  {
>       const char *str = "Reserved";
> @@ -599,6 +688,36 @@ static int dmi_decode_hp(const struct dmi_header *h)
>                       pr_subattr("UEFI", "%s", feat & 0x1400 ? "Yes" : "No");
>                       break;
>  
> +             case 224:
> +                     /*
> +                      * Vendor Specific: Trusted Module (TPM or TCM) Status
> +                      *
> +                      * Offset |  Name  | Width | Description
> +                      * -------------------------------------
> +                      *  0x00  |  Type  | BYTE  | 0xE0, Trusted Module (TPM 
> or TCM) Status

"Type" should be left-aligned for consistency (although I see type 233
has is wrong too).

> +                      *  0x01  | Length | BYTE  | Length of structure
> +                      *  0x02  | Handle | WORD  | Unique handle
> +                      *  0x04  | Status | BYTE  | Status Flag Byte
> +                      *  0x05  | Ex Stat| BYTE  | TPM Extended Status
> +                      *  0x06  | Type   | BYTE  | Trusted Module Type
> +                      *  0x07  | Attrib | BYTE  | Trusted Module Attributes
> +                      *  0x08  | Handle | WORD  | Handle to map to Type 216
> +                      *  0x0A  | Chip ID| WORD  | Chip Identifier Values
> +                      */
> +                     pr_handle_name("%s Trusted Module (TPM or TCM) Status", 
> company);
> +                     if (h->length < 0x05) break;
> +                     if (!dmi_hp_224_status(data[0x04]))
> +                             break;
> +                     if (h->length < 0x0a) break;
> +                     dmi_hp_224_ex_status(data[0x04], data[0x05]);
> +                     dmi_hp_224_module_type(data[0x06]);
> +                     dmi_hp_224_module_attr(data[0x07]);
> +                     if (!(opt.flags & FLAG_QUIET))
> +                             pr_attr("Associated Handle", "0x%04X", 
> WORD(data + 0x8));
> +                     if (h->length < 0x0c) break;
> +                     dmi_hp_224_chipid(WORD(data + 0x0a));
> +                     break;
> +
>               case 233:
>                       /*
>                        * Vendor Specific: HPE ProLiant NIC MAC Information

Thanks,
-- 
Jean Delvare
SUSE L3 Support