Re: [PATCH v3] Add a module for retrieving SMBIOS information

[Vladimir 'φ-coder/phcoder' Serbinenko]

On 23.03.2015 03:01, David Michael wrote:
> The following are two use cases from Rajat Jain<address@hidden>:
>
> 1) We have a board that boots Linux and this board itself can be plugged into one of 
> different chassis types. We need to pass different parameters to the kernel based on the 
> "CHASSIS_TYPE" information that is passed by the bios in the DMI / SMBIOS 
> tables.
>
> 2) We may have a USB stick that can go into multiple boards, and the exact 
> kernel to be loaded depends on the machine information (PRODUCT_NAME etc) 
> passed via the DMI.
> ---
>
> Changes since v2:
>
> * Switched to language like "string set" and "SMBIOS structure" to use
>    terminology consistent with the specification.
>
> To address points suggested by Andrei Borzenkov:
>
> * Dropped ChangeLog text from the commit message.
>
> * Changed to long options in the documentation.
>
> * Renamed --variable to --set.
>
> * Exit with an error when given out-of-range option values instead of
>    resetting the option.
>
> * Functions were added to retrieve data types that should avoid
>    alignment and endianness issues.
>
> * Force string set searches to terminate at end of table.
>
> * Each data type now has a separate size/bounds test.
>
> * Error messages have better explanations.
>
>   docs/grub.texi              |  65 +++++++
>   grub-core/Makefile.core.def |   7 +
>   grub-core/commands/smbios.c | 445 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 517 insertions(+)
>   create mode 100644 grub-core/commands/smbios.c
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 46b9e7f..73f0909 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3830,6 +3830,7 @@ you forget a command, you can run the command 
> @command{help}
>   * sha256sum::                   Compute or check SHA256 hash
>   * sha512sum::                   Compute or check SHA512 hash
>   * sleep::                       Wait for a specified number of seconds
> +* smbios::                      Retrieve SMBIOS information
>   * source::                      Read a configuration file in same context
>   * test::                        Check file types and compare values
>   * true::                        Do nothing, successfully
> @@ -4944,6 +4945,70 @@ if timeout was interrupted by @key{ESC}.
>   @end deffn
>
>
> address@hidden smbios
> address@hidden smbios
> +
> address@hidden Command smbios @
> + address@hidden @var{type}] @
> + address@hidden @var{handle}] @
> + address@hidden @var{match}] @
> + [(@option{--get-byte} | @option{--get-word} | @option{--get-dword} | @
> +   @option{--get-qword} | @option{--get-string}) @
> +   @var{offset} address@hidden @var{variable}]]
> +Retrieve SMBIOS information.  This command is only available on x86 and EFI
> +systems.
> +
Could we avoid exposing such details as offset in structures? It's way 
too technical perhaps something like
smbios [--handle=HANDLE|--instance=N] [--set VAR] [TABLE.VARNAME]
where table and varname will be string identifiers and would be 
translated using dictionaries. TABLE.VARNAME as whole string can be the 
key, that format is just to avoid conflicts when similar fields are 
exported in different tables. You can just put the vars you care about 
in the dictionary.
> +/*
> + * In order for any of this module to function, it needs to find an entry point
> + * structure.  This returns a pointer to a struct that identifies the fields of
> + * the EPS, or NULL if it cannot be located.
> + */
> +static const struct grub_smbios_eps *
> +grub_smbios_locate_eps (void)
> +{
> +#ifdef GRUB_MACHINE_EFI
> +  static const grub_efi_guid_t smbios_guid = GRUB_EFI_SMBIOS_TABLE_GUID;
> +  const grub_efi_system_table_t *st = grub_efi_system_table;
> +  const grub_efi_configuration_table_t *t = st->configuration_table;
> +  unsigned int i;
> +
> +  for (i = 0; i < st->num_table_entries; i++)
> +    if (grub_memcmp (&smbios_guid, &t->vendor_guid, sizeof (smbios_guid)) == 0)
> +      {
> +        grub_dprintf ("smbios", "Found entry point structure at %p\n",
> +                      t->vendor_table);
> +        return (const struct grub_smbios_eps *)t->vendor_table;
> +      }
> +    else
> +      t++;
> +#else
> +  grub_uint8_t *ptr;
> +
> +  for (ptr = (grub_uint8_t *)0x000F0000;
> +       ptr < (grub_uint8_t *)0x00100000;
> +       ptr += 16)
> +    if (grub_memcmp (ptr, "_SM_", 4) == 0
> +        && grub_byte_checksum (ptr, ptr[5]) == 0)
> +      {
> +        grub_dprintf ("smbios", "Found entry point structure at %p\n", ptr);
> +        return (const struct grub_smbios_eps *)ptr;
> +      }
> +#endif
> +
This is already present in grub-core/efiemu/*/cfgtables.c. It should 
rather be moved to separate file than duplicated.
> +/*
> + * Given a pointer to an SMBIOS structure, return the unsigned little-endian
> + * value of the requested number of bytes.  These functions avoid alignment and
> + * endianness issues.
> + */
> +static inline grub_uint8_t
> +grub_smbios_get_byte (const grub_uint8_t *structure, grub_uint8_t offset)
> +{
> +  return structure[offset];
> +}
> +
> +static inline grub_uint16_t
> +grub_smbios_get_word (const grub_uint8_t *structure, grub_uint8_t offset)
> +{
> +  return (grub_uint16_t)(grub_smbios_get_byte (structure, offset) |
> +         ((grub_uint16_t)grub_smbios_get_byte (structure, offset + 1) << 8));
> +}
> +
> +static inline grub_uint32_t
> +grub_smbios_get_dword (const grub_uint8_t *structure, grub_uint8_t offset)
> +{
> +  return (grub_uint32_t)(grub_smbios_get_word (structure, offset) |
> +         ((grub_uint32_t)grub_smbios_get_word (structure, offset + 2) << 16));
> +}
> +
> +static inline grub_uint64_t
> +grub_smbios_get_qword (const grub_uint8_t *structure, grub_uint8_t offset)
> +{
> +  return (grub_uint64_t)(grub_smbios_get_dword (structure, offset) |
> +         ((grub_uint64_t)grub_smbios_get_dword (structure, offset + 4) << 32));
> +}
> +
Why is endianness an issue? AFAIK SMBIOS is LE-only and so are all 
platforms that use it. GRUB has get_unaligned functions.
> +static grub_uint16_t
> +grub_smbios_dump_structure (const struct grub_smbios_eps *ep,
> +                            const grub_uint8_t *structure)
> +{
> +  const grub_uint8_t *ptr = structure;
> +  const grub_uint8_t *table_end = (const grub_uint8_t *)eps_table_end (ep);
> +  grub_uint8_t length = ptr[1];
> +
> +  /* Write the SMBIOS structure's mandatory four header bytes. */
> +  grub_printf ("Structure: Type=%u Length=%u Handle=%u\n",
> +               ptr[0], length, grub_smbios_get_word (ptr, 2));
> +
> +  /* Dump of the formatted area (including the header) in hex. */
> +  grub_printf (" Hex Dump: ");
> +  while (length-- > 0)
> +    grub_printf ("%02x", *ptr++);
> +  grub_printf ("\n");
> +
> +  /* Print each string found in the appended string set. */
> +  if (*ptr == 0)
> +    ptr++;
> +  while (*ptr != 0 && ptr < table_end)
> +    ptr += grub_printf (" String: %s\n", ptr) - sizeof (" String: ") + 1;
> +  ptr++;
> +
> +  /* Return the total number of bytes covered. */
> +  return ptr - structure;
> +}
> +
It's better to just dump the values GRUB knows about. Hex dump is 
probably to be avoided.
> +/* Reference: DMTF Standard DSP0134 2.7.1 Sections 6.1.2-6.1.3 */
> +
> +/*
> + * Return or print a matched SMBIOS structure.  Multiple entries can be matched
> + * if they are being printed.
> + *
> + * This method can use up to three criteria for selecting a structure:
> + *   - The "type" field                  (use -1 to ignore)
> + *   - The "handle" field                (use -1 to ignore)
> + *   - Which to return if several match  (use 0 to print all matches)
> + *
> + * The parameter "print" was added for verbose debugging.  When non-zero, the
> + * matched entries are printed to the console instead of returned.
> + */
> +static const grub_uint8_t *
> +grub_smbios_match_structure (const struct grub_smbios_eps *ep,
> +                             const grub_int16_t type,
> +                             const grub_int32_t handle,
> +                             const grub_uint16_t match,
> +                             const grub_uint8_t print)
> +{
> +  const grub_uint8_t *ptr = (const grub_uint8_t *)eps_table_begin (ep);
> +  const grub_uint8_t *table_end = (const grub_uint8_t *)eps_table_end (ep);
> +  grub_uint16_t structures = ep->intermediate.structures;
> +  grub_uint16_t structure = 0;
> +  grub_uint16_t matches = 0;
> +
> +  while (structure++ < structures && ptr < table_end)
> +    {
> +      grub_uint16_t structure_handle = grub_smbios_get_word (ptr, 2);
> +      grub_uint8_t structure_type = ptr[0];
> +
This needs to check that ptr[1] is not zero to avoid infinite loop.
> +  /* Select a single structure from the given filtering options. */
> +  structure = grub_smbios_match_structure (eps, type, handle, match, 0);
> +  if (structure == NULL)
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND,
> +                       N_("no SMBIOS structure matched the given options"));
> +
It's not a file. Generic GRUB_ERR_IO is a better match.