All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] Add a module for retrieving SMBIOS information
@ 2015-02-08 18:54 David Michael
  2015-02-08 20:13 ` Prarit Bhargava
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: David Michael @ 2015-02-08 18:54 UTC (permalink / raw)
  To: grub-devel
  Cc: Prarit Bhargava, Raghuraman Thirumalairajan, Andrei Borzenkov,
	Rajat Jain, Leif Lindholm, Sanjay Jain, Stu Grossman

The following are two use cases from Rajat Jain <rajatjain@juniper.net>:

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.

* grub-core/commands/smbios.c: New file.
* grub-core/Makefile.core.def (smbios): New module.
* docs/grub.texi (smbios): New node.
(Command-line and menu entry commands): Add a menu entry for smbios.
---

Changes since v1:

* Removed formfeeds and quoted use cases in the commit messages as
  suggested by Prarit Bhargava.

* Enabled building the module on all EFI platforms as suggested by Leif
  Lindholm.

 docs/grub.texi              |  67 ++++++++
 grub-core/Makefile.core.def |   7 +
 grub-core/commands/smbios.c | 361 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 435 insertions(+)
 create mode 100644 grub-core/commands/smbios.c

diff --git a/docs/grub.texi b/docs/grub.texi
index 46b9e7f..f4e187f 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,72 @@ if timeout was interrupted by @key{ESC}.
 @end deffn
 
 
+@node smbios
+@subsection smbios
+
+@deffn Command smbios @
+ [@option{-t} @var{type}] @
+ [@option{-h} @var{handle}] @
+ [@option{-m} @var{match}] @
+ [ (@option{-b} | @option{-w} | @option{-d} | @option{-q} | @option{-s}) @
+   @var{offset} [@option{-v} @var{variable}] ]
+Retrieve SMBIOS information.  This command is only available on x86 and EFI
+systems.
+
+When run with no options, @command{smbios} will print the SMBIOS table entries
+to the console as the header values, a hex dump, and a string list.  The
+following options can filter which table entries are printed.
+
+@itemize @bullet
+@item
+Specifying @option{-t} will only print entries with a matching @var{type}.  The
+type can be any value from 0 to 255.  Specify a value outside this range to
+match entries with any type.  The default is -1.
+@item
+Specifying @option{-h} will only print entries with a matching @var{handle}.
+The handle can be any value from 0 to 65535.  Specify a value outside this
+range to match entries with any handle.  The default is -1.
+@item
+Specifying @option{-m} will only print number @var{match} in the list of all
+filtered entries; e.g. @code{smbios -m 3} will print the third entry.  The list
+is always ordered the same as the hardware's SMBIOS table.  The match number
+can be any positive value.  Specify 0 to match all entries.  The default is 0.
+@end itemize
+
+The remaining options print the value of a field in the matched SMBIOS table
+entry.  Only one of these options may be specified.  When they are used, the
+option @code{-m 0} is synonymous with @code{-m 1} to match the first entry.
+
+@itemize @bullet
+@item
+When given @option{-b}, print the value of the byte
+at @var{offset} bytes into the matched SMBIOS table entry.
+@item
+When given @option{-w}, print the value of the word (two bytes)
+at @var{offset} bytes into the matched SMBIOS table entry.
+@item
+When given @option{-d}, print the value of the dword (four bytes)
+at @var{offset} bytes into the matched SMBIOS table entry.
+@item
+When given @option{-q}, print the value of the qword (eight bytes)
+at @var{offset} bytes into the matched SMBIOS table entry.
+@item
+When given @option{-s}, print the string table entry with its index found
+at @var{offset} bytes into the matched SMBIOS table entry.
+@end itemize
+
+If any of the options in the above list were given, a variable name can be
+specified with @option{-v} to store the value instead of printing it.
+
+For example, this will store and display the system manufacturer string.
+
+@example
+smbios -t 1 -s 4 -v system_manufacturer
+echo $system_manufacturer
+@end example
+@end deffn
+
+
 @node source
 @subsection source
 
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 42443bc..f87de70 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1023,6 +1023,13 @@ module = {
 };
 
 module = {
+  name = smbios;
+  common = commands/smbios.c;
+  enable = efi;
+  enable = x86;
+};
+
+module = {
   name = suspend;
   ieee1275 = commands/ieee1275/suspend.c;
   enable = i386_ieee1275;
diff --git a/grub-core/commands/smbios.c b/grub-core/commands/smbios.c
new file mode 100644
index 0000000..d8edeea
--- /dev/null
+++ b/grub-core/commands/smbios.c
@@ -0,0 +1,361 @@
+/* Expose SMBIOS data to the console and configuration files */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2013,2014,2015  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/dl.h>
+#include <grub/extcmd.h>
+#include <grub/env.h>
+#include <grub/i18n.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+
+#ifdef GRUB_MACHINE_EFI
+#include <grub/efi/efi.h>
+#else
+#include <grub/acpi.h>
+#endif
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+/* Reference: DMTF Standard DSP0134 2.7.1 Table 1 */
+
+struct __attribute__ ((packed)) grub_smbios_ieps
+  {
+    grub_uint8_t  anchor[5]; /* "_DMI_" */
+    grub_uint8_t  checksum;
+    grub_uint16_t table_length;
+    grub_uint32_t table_address;
+    grub_uint16_t structures;
+    grub_uint8_t  revision;
+  };
+
+struct __attribute__ ((packed)) grub_smbios_eps
+  {
+    grub_uint8_t  anchor[4]; /* "_SM_" */
+    grub_uint8_t  checksum;
+    grub_uint8_t  length;
+    grub_uint8_t  version_major;
+    grub_uint8_t  version_minor;
+    grub_uint16_t maximum_structure_size;
+    grub_uint8_t  revision;
+    grub_uint8_t  formatted[5];
+    struct grub_smbios_ieps intermediate;
+  };
+
+static const struct grub_smbios_eps *eps = NULL;
+
+/* Reference: DMTF Standard DSP0134 2.7.1 Section 5.2.1 */
+
+/*
+ * 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
+
+  grub_dprintf ("smbios", "Failed to locate entry point structure\n");
+  return NULL;
+}
+
+/* Reference: DMTF Standard DSP0134 2.7.1 Sections 6.1.2-6.1.3 */
+
+/*
+ * Given a pointer to the first bytes of an entry, print its contents in a
+ * semi-human-readable format.  Return the size of the entry printed.
+ */
+static grub_uint16_t
+grub_smbios_dump_entry (const grub_uint8_t *entry)
+{
+  const grub_uint8_t *ptr = entry;
+  grub_uint8_t newstr = 1;
+  grub_uint8_t length;
+
+  /* Write the entry's mandatory four header bytes. */
+  length = ptr[1];
+  grub_printf ("Entry: Type=0x%02x Length=0x%02x Handle=0x%04x\n",
+               ptr[0], length, *(1 + (grub_uint16_t *)ptr));
+
+  /* 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 list. */
+  while (ptr[0] != 0 || ptr[1] != 0)
+    {
+      if (newstr)
+        grub_printf (" String: %s\n", ptr);
+      newstr = *ptr++ == 0;
+    }
+  ptr += 2;
+
+  /* Return the total number of bytes covered. */
+  return ptr - entry;
+}
+
+/* Reference: DMTF Standard DSP0134 2.7.1 Sections 6.1.2-6.1.3 */
+
+/*
+ * Return or print a matched table entry.  Multiple entries can be matched if
+ * they are being printed.
+ *
+ * This method can handle up to three criteria for selecting an entry:
+ *   - The entry's "type" field             (use outside [0,0xFF] to ignore)
+ *   - The entry's "handle" field           (use outside [0,0xFFFF] to ignore)
+ *   - The entry 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_entry (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)
+{
+  grub_addr_t table_address = (grub_addr_t)ep->intermediate.table_address;
+  grub_uint16_t table_length = ep->intermediate.table_length;
+  grub_uint16_t structures = ep->intermediate.structures;
+  grub_uint16_t structure = 0;
+  grub_uint16_t matches = 0;
+  const grub_uint8_t *table = (const grub_uint8_t *)table_address;
+  const grub_uint8_t *ptr = table;
+
+  while (ptr - table < table_length && structure++ < structures)
+
+    /* Test if the current entry matches the given parameters. */
+    if ((handle < 0 || 65535 < handle || handle == *(1 + (grub_uint16_t *)ptr))
+        && (type < 0 || 255 < type || type == ptr[0])
+        && (match == 0 || match == ++matches))
+      {
+        if (print)
+          {
+            ptr += grub_smbios_dump_entry (ptr);
+            if (match > 0)
+              break;
+          }
+        else
+          return ptr;
+      }
+
+    /* If the entry didn't match, skip it (formatted area and string list). */
+    else
+      {
+        ptr += ptr[1];
+        while (*ptr++ != 0 || *ptr++ != 0);
+      }
+
+  return NULL;
+}
+
+/* Reference: DMTF Standard DSP0134 2.7.1 Section 6.1.3 */
+
+/*
+ * Given a pointer to a string list and a number, iterate over each of the
+ * strings until the desired item is reached.  (The value of 1 indicates the
+ * first string, etc.)
+ */
+static const char *
+grub_smbios_get_string (const grub_uint8_t *strings, const grub_uint8_t number)
+{
+  const char *ptr = (const char *)strings;
+  grub_uint8_t newstr = 1;
+  grub_uint8_t index = 1;
+
+  /* A string referenced with zero is interpreted as unset. */
+  if (number == 0)
+    return NULL;
+
+  /* Search the string table, incrementing the counter as each null passes. */
+  while (ptr[0] != 0 || ptr[1] != 0)
+    {
+      if (newstr && number == index++)
+        return ptr;
+      newstr = *ptr++ == 0;
+    }
+
+  /* The requested string index is greater than the number of table entries. */
+  return NULL;
+}
+
+static grub_err_t
+grub_cmd_smbios (grub_extcmd_context_t ctxt,
+                 int argc __attribute__ ((unused)),
+                 char **argv __attribute__ ((unused)))
+{
+  struct grub_arg_list *state = ctxt->state;
+
+  grub_int16_t type = -1;
+  grub_int32_t handle = -1;
+  grub_uint16_t match = 0;
+  grub_uint8_t offset = 0;
+
+  const grub_uint8_t *entry;
+  grub_uint8_t accessors;
+  grub_uint8_t i;
+  char buffer[24]; /* 64-bit number -> maximum 20 decimal digits */
+  const char *value = buffer;
+
+  if (eps == NULL)
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("missing entry point"));
+
+  /* Only one value can be returned at a time; reject multiple selections. */
+  accessors = !!state[3].set + !!state[4].set + !!state[5].set +
+              !!state[6].set + !!state[7].set;
+  if (accessors > 1)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("too many -b|-w|-d|-q|-s"));
+
+  /* Reject the environment variable if no value was selected. */
+  if (accessors == 0 && state[8].set)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("-v without -b|-w|-d|-q|-s"));
+
+  /* Read the given matching parameters. */
+  if (state[0].set)
+    type = grub_strtol (state[0].arg, NULL, 0);
+  if (state[1].set)
+    handle = grub_strtol (state[1].arg, NULL, 0);
+  if (state[2].set)
+    match = grub_strtoul (state[2].arg, NULL, 0);
+
+  /* When not selecting a value, print all matching entries and quit. */
+  if (accessors == 0)
+    {
+      grub_smbios_match_entry (eps, type, handle, match, 1);
+      return GRUB_ERR_NONE;
+    }
+
+  /* Select a single entry from the matching parameters. */
+  entry = grub_smbios_match_entry (eps, type, handle, match, 0);
+  if (entry == NULL)
+    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("no such entry"));
+
+  /* Ensure the specified offset+length is within the matched entry. */
+  for (i = 3; i <= 7; i++)
+    if (state[i].set)
+      {
+        offset = grub_strtoul (state[i].arg, NULL, 0);
+        if (offset + (1 << (i == 7 ? 0 : i - 3)) > entry[1])
+          return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("value outside entry"));
+        break;
+      }
+
+  /* If a string was requested, try to find its pointer. */
+  if (state[7].set)
+    {
+      value = grub_smbios_get_string (entry + entry[1], entry[offset]);
+      if (value == NULL)
+        return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("string not defined"));
+    }
+
+  /* Create a string from a numeric value suitable for printing. */
+  else if (state[3].set)
+    grub_snprintf (buffer, sizeof (buffer), "%u", entry[offset]);
+  else if (state[4].set)
+    grub_snprintf (buffer, sizeof (buffer), "%u",
+                   *(grub_uint16_t *)(entry + offset));
+  else if (state[5].set)
+    grub_snprintf (buffer, sizeof (buffer), "%" PRIuGRUB_UINT32_T,
+                   *(grub_uint32_t *)(entry + offset));
+  else if (state[6].set)
+    grub_snprintf (buffer, sizeof (buffer), "%" PRIuGRUB_UINT64_T,
+                   *(grub_uint64_t *)(entry + offset));
+
+  /* Store or print the requested value. */
+  if (state[8].set)
+    {
+      grub_env_set (state[8].arg, value);
+      grub_env_export (state[8].arg);
+    }
+  else
+    grub_printf ("%s\n", value);
+
+  return GRUB_ERR_NONE;
+}
+
+static grub_extcmd_t cmd;
+
+static const struct grub_arg_option options[] =
+  {
+    {"type",       't', 0, N_("Match entries with the given type."),
+                           N_("byte"), ARG_TYPE_INT},
+    {"handle",     'h', 0, N_("Match entries with the given handle."),
+                           N_("word"), ARG_TYPE_INT},
+    {"match",      'm', 0, N_("Select the entry to use when several match."),
+                           N_("number"), ARG_TYPE_INT},
+    {"get-byte",   'b', 0, N_("Get the byte's value at the given offset."),
+                           N_("offset"), ARG_TYPE_INT},
+    {"get-word",   'w', 0, N_("Get two bytes' value at the given offset."),
+                           N_("offset"), ARG_TYPE_INT},
+    {"get-dword",  'd', 0, N_("Get four bytes' value at the given offset."),
+                           N_("offset"), ARG_TYPE_INT},
+    {"get-qword",  'q', 0, N_("Get eight bytes' value at the given offset."),
+                           N_("offset"), ARG_TYPE_INT},
+    {"get-string", 's', 0, N_("Get the string specified at the given offset."),
+                           N_("offset"), ARG_TYPE_INT},
+    {"variable",   'v', 0, N_("Store the value in the given variable name."),
+                           N_("name"), ARG_TYPE_STRING},
+    {0, 0, 0, 0, 0, 0}
+  };
+
+GRUB_MOD_INIT(smbios)
+{
+  /* SMBIOS data is supposed to be static, so find it only once during init. */
+  eps = grub_smbios_locate_eps ();
+
+  cmd = grub_register_extcmd ("smbios", grub_cmd_smbios, 0,
+                              N_("[-t byte] [-h word] [-m number] "
+                                 "[(-b|-w|-d|-q|-s) offset [-v name]]"),
+                              N_("Retrieve SMBIOS information."), options);
+}
+
+GRUB_MOD_FINI(smbios)
+{
+  grub_unregister_extcmd (cmd);
+}
-- 
2.1.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] Add a module for retrieving SMBIOS information
  2015-02-08 18:54 [PATCH v2] Add a module for retrieving SMBIOS information David Michael
@ 2015-02-08 20:13 ` Prarit Bhargava
  2015-02-09 12:43 ` Prarit Bhargava
  2015-02-11 11:34 ` Andrei Borzenkov
  2 siblings, 0 replies; 11+ messages in thread
From: Prarit Bhargava @ 2015-02-08 20:13 UTC (permalink / raw)
  To: grub-devel



On 02/08/2015 01:54 PM, David Michael wrote:
> The following are two use cases from Rajat Jain <rajatjain@juniper.net>:
> 
> 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.
> 

why isn't 1) already in the kernel itself?

P.

> 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.
> 
> * grub-core/commands/smbios.c: New file.
> * grub-core/Makefile.core.def (smbios): New module.
> * docs/grub.texi (smbios): New node.
> (Command-line and menu entry commands): Add a menu entry for smbios.
> ---
> 
> Changes since v1:
> 
> * Removed formfeeds and quoted use cases in the commit messages as
>   suggested by Prarit Bhargava.
> 
> * Enabled building the module on all EFI platforms as suggested by Leif
>   Lindholm.
> 
>  docs/grub.texi              |  67 ++++++++
>  grub-core/Makefile.core.def |   7 +
>  grub-core/commands/smbios.c | 361 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 435 insertions(+)
>  create mode 100644 grub-core/commands/smbios.c
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 46b9e7f..f4e187f 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,72 @@ if timeout was interrupted by @key{ESC}.
>  @end deffn
>  
>  
> +@node smbios
> +@subsection smbios
> +
> +@deffn Command smbios @
> + [@option{-t} @var{type}] @
> + [@option{-h} @var{handle}] @
> + [@option{-m} @var{match}] @
> + [ (@option{-b} | @option{-w} | @option{-d} | @option{-q} | @option{-s}) @
> +   @var{offset} [@option{-v} @var{variable}] ]
> +Retrieve SMBIOS information.  This command is only available on x86 and EFI
> +systems.
> +
> +When run with no options, @command{smbios} will print the SMBIOS table entries
> +to the console as the header values, a hex dump, and a string list.  The
> +following options can filter which table entries are printed.
> +
> +@itemize @bullet
> +@item
> +Specifying @option{-t} will only print entries with a matching @var{type}.  The
> +type can be any value from 0 to 255.  Specify a value outside this range to
> +match entries with any type.  The default is -1.
> +@item
> +Specifying @option{-h} will only print entries with a matching @var{handle}.
> +The handle can be any value from 0 to 65535.  Specify a value outside this
> +range to match entries with any handle.  The default is -1.
> +@item
> +Specifying @option{-m} will only print number @var{match} in the list of all
> +filtered entries; e.g. @code{smbios -m 3} will print the third entry.  The list
> +is always ordered the same as the hardware's SMBIOS table.  The match number
> +can be any positive value.  Specify 0 to match all entries.  The default is 0.
> +@end itemize
> +
> +The remaining options print the value of a field in the matched SMBIOS table
> +entry.  Only one of these options may be specified.  When they are used, the
> +option @code{-m 0} is synonymous with @code{-m 1} to match the first entry.
> +
> +@itemize @bullet
> +@item
> +When given @option{-b}, print the value of the byte
> +at @var{offset} bytes into the matched SMBIOS table entry.
> +@item
> +When given @option{-w}, print the value of the word (two bytes)
> +at @var{offset} bytes into the matched SMBIOS table entry.
> +@item
> +When given @option{-d}, print the value of the dword (four bytes)
> +at @var{offset} bytes into the matched SMBIOS table entry.
> +@item
> +When given @option{-q}, print the value of the qword (eight bytes)
> +at @var{offset} bytes into the matched SMBIOS table entry.
> +@item
> +When given @option{-s}, print the string table entry with its index found
> +at @var{offset} bytes into the matched SMBIOS table entry.
> +@end itemize
> +
> +If any of the options in the above list were given, a variable name can be
> +specified with @option{-v} to store the value instead of printing it.
> +
> +For example, this will store and display the system manufacturer string.
> +
> +@example
> +smbios -t 1 -s 4 -v system_manufacturer
> +echo $system_manufacturer
> +@end example
> +@end deffn
> +
> +
>  @node source
>  @subsection source
>  
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 42443bc..f87de70 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1023,6 +1023,13 @@ module = {
>  };
>  
>  module = {
> +  name = smbios;
> +  common = commands/smbios.c;
> +  enable = efi;
> +  enable = x86;
> +};
> +
> +module = {
>    name = suspend;
>    ieee1275 = commands/ieee1275/suspend.c;
>    enable = i386_ieee1275;
> diff --git a/grub-core/commands/smbios.c b/grub-core/commands/smbios.c
> new file mode 100644
> index 0000000..d8edeea
> --- /dev/null
> +++ b/grub-core/commands/smbios.c
> @@ -0,0 +1,361 @@
> +/* Expose SMBIOS data to the console and configuration files */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2013,2014,2015  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/dl.h>
> +#include <grub/extcmd.h>
> +#include <grub/env.h>
> +#include <grub/i18n.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +
> +#ifdef GRUB_MACHINE_EFI
> +#include <grub/efi/efi.h>
> +#else
> +#include <grub/acpi.h>
> +#endif
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +/* Reference: DMTF Standard DSP0134 2.7.1 Table 1 */
> +
> +struct __attribute__ ((packed)) grub_smbios_ieps
> +  {
> +    grub_uint8_t  anchor[5]; /* "_DMI_" */
> +    grub_uint8_t  checksum;
> +    grub_uint16_t table_length;
> +    grub_uint32_t table_address;
> +    grub_uint16_t structures;
> +    grub_uint8_t  revision;
> +  };
> +
> +struct __attribute__ ((packed)) grub_smbios_eps
> +  {
> +    grub_uint8_t  anchor[4]; /* "_SM_" */
> +    grub_uint8_t  checksum;
> +    grub_uint8_t  length;
> +    grub_uint8_t  version_major;
> +    grub_uint8_t  version_minor;
> +    grub_uint16_t maximum_structure_size;
> +    grub_uint8_t  revision;
> +    grub_uint8_t  formatted[5];
> +    struct grub_smbios_ieps intermediate;
> +  };
> +
> +static const struct grub_smbios_eps *eps = NULL;
> +
> +/* Reference: DMTF Standard DSP0134 2.7.1 Section 5.2.1 */
> +
> +/*
> + * 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
> +
> +  grub_dprintf ("smbios", "Failed to locate entry point structure\n");
> +  return NULL;
> +}
> +
> +/* Reference: DMTF Standard DSP0134 2.7.1 Sections 6.1.2-6.1.3 */
> +
> +/*
> + * Given a pointer to the first bytes of an entry, print its contents in a
> + * semi-human-readable format.  Return the size of the entry printed.
> + */
> +static grub_uint16_t
> +grub_smbios_dump_entry (const grub_uint8_t *entry)
> +{
> +  const grub_uint8_t *ptr = entry;
> +  grub_uint8_t newstr = 1;
> +  grub_uint8_t length;
> +
> +  /* Write the entry's mandatory four header bytes. */
> +  length = ptr[1];
> +  grub_printf ("Entry: Type=0x%02x Length=0x%02x Handle=0x%04x\n",
> +               ptr[0], length, *(1 + (grub_uint16_t *)ptr));
> +
> +  /* 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 list. */
> +  while (ptr[0] != 0 || ptr[1] != 0)
> +    {
> +      if (newstr)
> +        grub_printf (" String: %s\n", ptr);
> +      newstr = *ptr++ == 0;
> +    }
> +  ptr += 2;
> +
> +  /* Return the total number of bytes covered. */
> +  return ptr - entry;
> +}
> +
> +/* Reference: DMTF Standard DSP0134 2.7.1 Sections 6.1.2-6.1.3 */
> +
> +/*
> + * Return or print a matched table entry.  Multiple entries can be matched if
> + * they are being printed.
> + *
> + * This method can handle up to three criteria for selecting an entry:
> + *   - The entry's "type" field             (use outside [0,0xFF] to ignore)
> + *   - The entry's "handle" field           (use outside [0,0xFFFF] to ignore)
> + *   - The entry 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_entry (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)
> +{
> +  grub_addr_t table_address = (grub_addr_t)ep->intermediate.table_address;
> +  grub_uint16_t table_length = ep->intermediate.table_length;
> +  grub_uint16_t structures = ep->intermediate.structures;
> +  grub_uint16_t structure = 0;
> +  grub_uint16_t matches = 0;
> +  const grub_uint8_t *table = (const grub_uint8_t *)table_address;
> +  const grub_uint8_t *ptr = table;
> +
> +  while (ptr - table < table_length && structure++ < structures)
> +
> +    /* Test if the current entry matches the given parameters. */
> +    if ((handle < 0 || 65535 < handle || handle == *(1 + (grub_uint16_t *)ptr))
> +        && (type < 0 || 255 < type || type == ptr[0])
> +        && (match == 0 || match == ++matches))
> +      {
> +        if (print)
> +          {
> +            ptr += grub_smbios_dump_entry (ptr);
> +            if (match > 0)
> +              break;
> +          }
> +        else
> +          return ptr;
> +      }
> +
> +    /* If the entry didn't match, skip it (formatted area and string list). */
> +    else
> +      {
> +        ptr += ptr[1];
> +        while (*ptr++ != 0 || *ptr++ != 0);
> +      }
> +
> +  return NULL;
> +}
> +
> +/* Reference: DMTF Standard DSP0134 2.7.1 Section 6.1.3 */
> +
> +/*
> + * Given a pointer to a string list and a number, iterate over each of the
> + * strings until the desired item is reached.  (The value of 1 indicates the
> + * first string, etc.)
> + */
> +static const char *
> +grub_smbios_get_string (const grub_uint8_t *strings, const grub_uint8_t number)
> +{
> +  const char *ptr = (const char *)strings;
> +  grub_uint8_t newstr = 1;
> +  grub_uint8_t index = 1;
> +
> +  /* A string referenced with zero is interpreted as unset. */
> +  if (number == 0)
> +    return NULL;
> +
> +  /* Search the string table, incrementing the counter as each null passes. */
> +  while (ptr[0] != 0 || ptr[1] != 0)
> +    {
> +      if (newstr && number == index++)
> +        return ptr;
> +      newstr = *ptr++ == 0;
> +    }
> +
> +  /* The requested string index is greater than the number of table entries. */
> +  return NULL;
> +}
> +
> +static grub_err_t
> +grub_cmd_smbios (grub_extcmd_context_t ctxt,
> +                 int argc __attribute__ ((unused)),
> +                 char **argv __attribute__ ((unused)))
> +{
> +  struct grub_arg_list *state = ctxt->state;
> +
> +  grub_int16_t type = -1;
> +  grub_int32_t handle = -1;
> +  grub_uint16_t match = 0;
> +  grub_uint8_t offset = 0;
> +
> +  const grub_uint8_t *entry;
> +  grub_uint8_t accessors;
> +  grub_uint8_t i;
> +  char buffer[24]; /* 64-bit number -> maximum 20 decimal digits */
> +  const char *value = buffer;
> +
> +  if (eps == NULL)
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("missing entry point"));
> +
> +  /* Only one value can be returned at a time; reject multiple selections. */
> +  accessors = !!state[3].set + !!state[4].set + !!state[5].set +
> +              !!state[6].set + !!state[7].set;
> +  if (accessors > 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("too many -b|-w|-d|-q|-s"));
> +
> +  /* Reject the environment variable if no value was selected. */
> +  if (accessors == 0 && state[8].set)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("-v without -b|-w|-d|-q|-s"));
> +
> +  /* Read the given matching parameters. */
> +  if (state[0].set)
> +    type = grub_strtol (state[0].arg, NULL, 0);
> +  if (state[1].set)
> +    handle = grub_strtol (state[1].arg, NULL, 0);
> +  if (state[2].set)
> +    match = grub_strtoul (state[2].arg, NULL, 0);
> +
> +  /* When not selecting a value, print all matching entries and quit. */
> +  if (accessors == 0)
> +    {
> +      grub_smbios_match_entry (eps, type, handle, match, 1);
> +      return GRUB_ERR_NONE;
> +    }
> +
> +  /* Select a single entry from the matching parameters. */
> +  entry = grub_smbios_match_entry (eps, type, handle, match, 0);
> +  if (entry == NULL)
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("no such entry"));
> +
> +  /* Ensure the specified offset+length is within the matched entry. */
> +  for (i = 3; i <= 7; i++)
> +    if (state[i].set)
> +      {
> +        offset = grub_strtoul (state[i].arg, NULL, 0);
> +        if (offset + (1 << (i == 7 ? 0 : i - 3)) > entry[1])
> +          return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("value outside entry"));
> +        break;
> +      }
> +
> +  /* If a string was requested, try to find its pointer. */
> +  if (state[7].set)
> +    {
> +      value = grub_smbios_get_string (entry + entry[1], entry[offset]);
> +      if (value == NULL)
> +        return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("string not defined"));
> +    }
> +
> +  /* Create a string from a numeric value suitable for printing. */
> +  else if (state[3].set)
> +    grub_snprintf (buffer, sizeof (buffer), "%u", entry[offset]);
> +  else if (state[4].set)
> +    grub_snprintf (buffer, sizeof (buffer), "%u",
> +                   *(grub_uint16_t *)(entry + offset));
> +  else if (state[5].set)
> +    grub_snprintf (buffer, sizeof (buffer), "%" PRIuGRUB_UINT32_T,
> +                   *(grub_uint32_t *)(entry + offset));
> +  else if (state[6].set)
> +    grub_snprintf (buffer, sizeof (buffer), "%" PRIuGRUB_UINT64_T,
> +                   *(grub_uint64_t *)(entry + offset));
> +
> +  /* Store or print the requested value. */
> +  if (state[8].set)
> +    {
> +      grub_env_set (state[8].arg, value);
> +      grub_env_export (state[8].arg);
> +    }
> +  else
> +    grub_printf ("%s\n", value);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_extcmd_t cmd;
> +
> +static const struct grub_arg_option options[] =
> +  {
> +    {"type",       't', 0, N_("Match entries with the given type."),
> +                           N_("byte"), ARG_TYPE_INT},
> +    {"handle",     'h', 0, N_("Match entries with the given handle."),
> +                           N_("word"), ARG_TYPE_INT},
> +    {"match",      'm', 0, N_("Select the entry to use when several match."),
> +                           N_("number"), ARG_TYPE_INT},
> +    {"get-byte",   'b', 0, N_("Get the byte's value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-word",   'w', 0, N_("Get two bytes' value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-dword",  'd', 0, N_("Get four bytes' value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-qword",  'q', 0, N_("Get eight bytes' value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-string", 's', 0, N_("Get the string specified at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"variable",   'v', 0, N_("Store the value in the given variable name."),
> +                           N_("name"), ARG_TYPE_STRING},
> +    {0, 0, 0, 0, 0, 0}
> +  };
> +
> +GRUB_MOD_INIT(smbios)
> +{
> +  /* SMBIOS data is supposed to be static, so find it only once during init. */
> +  eps = grub_smbios_locate_eps ();
> +
> +  cmd = grub_register_extcmd ("smbios", grub_cmd_smbios, 0,
> +                              N_("[-t byte] [-h word] [-m number] "
> +                                 "[(-b|-w|-d|-q|-s) offset [-v name]]"),
> +                              N_("Retrieve SMBIOS information."), options);
> +}
> +
> +GRUB_MOD_FINI(smbios)
> +{
> +  grub_unregister_extcmd (cmd);
> +}
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] Add a module for retrieving SMBIOS information
  2015-02-08 18:54 [PATCH v2] Add a module for retrieving SMBIOS information David Michael
  2015-02-08 20:13 ` Prarit Bhargava
@ 2015-02-09 12:43 ` Prarit Bhargava
  2015-02-09 15:27   ` Rajat Jain
  2015-02-11 11:34 ` Andrei Borzenkov
  2 siblings, 1 reply; 11+ messages in thread
From: Prarit Bhargava @ 2015-02-09 12:43 UTC (permalink / raw)
  To: David Michael
  Cc: grub-devel, Raghuraman Thirumalairajan, Andrei Borzenkov,
	Rajat Jain, Leif Lindholm, Sanjay Jain, Stu Grossman

On 02/08/2015 01:54 PM, David Michael wrote:
> The following are two use cases from Rajat Jain <rajatjain@juniper.net>:
> 
> 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.
> 

Whups ... somehow stripped all the cc's on my original reply.

why isn't 1) already in the kernel itself?

P.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2] Add a module for retrieving SMBIOS information
  2015-02-09 12:43 ` Prarit Bhargava
@ 2015-02-09 15:27   ` Rajat Jain
  2015-02-09 16:28     ` Prarit Bhargava
  0 siblings, 1 reply; 11+ messages in thread
From: Rajat Jain @ 2015-02-09 15:27 UTC (permalink / raw)
  To: Prarit Bhargava, David Michael
  Cc: grub-devel, Raghuraman Thirumalairajan, Andrei Borzenkov,
	Leif Lindholm, Sanjay Jain, Stu Grossman

Hello,

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

Actually, I had simplified the problem statement, to avoid complicating the discussion. Basically, we have enhanced the grub with a "devicetree" command that supplies a flattened device tree to the kernel (just like u-boot or others do in embedded world).

http://www.denx.de/wiki/view/DULG/LinuxFDTBlob

However, we need to pass different flattened device tree to the kernel based on different CHASSIS_TYPE. (because different chassis have different hardware components, slots etc that need to be handled differently).

> >
> 
> Whups ... somehow stripped all the cc's on my original reply.
> 
> why isn't 1) already in the kernel itself?

I hope my problem statement clarification above makes it clearer. However, I think the problem is more generic - essentially depending on the current machine type, we may have to do different things on different platforms, such as:
* Loading different kernels / initrd / device tree etc.
* Passing different kernel parameters for:
	- different root fs mount points (root=/dev/sda[a/b/c/d]......)
	- enabling disabling different kernel features / drivers / subsystem (pci=[nocrs/use_crs]...)
	- etc.

Thanks,

Rajat 



> -----Original Message-----
> From: Prarit Bhargava [mailto:prarit@redhat.com]
> Sent: Monday, February 09, 2015 4:44 AM
> To: David Michael
> Cc: grub-devel@gnu.org; Leif Lindholm; Andrei Borzenkov; Rajat Jain; Sanjay
> Jain; Raghuraman Thirumalairajan; Stu Grossman
> Subject: Re: [PATCH v2] Add a module for retrieving SMBIOS information
> 
> On 02/08/2015 01:54 PM, David Michael wrote:
> > The following are two use cases from Rajat Jain <rajatjain@juniper.net>:
> >
> > 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.
> >
> 
> Whups ... somehow stripped all the cc's on my original reply.
> 
> why isn't 1) already in the kernel itself?
> 
> P.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] Add a module for retrieving SMBIOS information
  2015-02-09 15:27   ` Rajat Jain
@ 2015-02-09 16:28     ` Prarit Bhargava
  2015-02-09 17:38       ` Rajat Jain
  0 siblings, 1 reply; 11+ messages in thread
From: Prarit Bhargava @ 2015-02-09 16:28 UTC (permalink / raw)
  To: Rajat Jain
  Cc: grub-devel, David Michael, Raghuraman Thirumalairajan,
	Andrei Borzenkov, Leif Lindholm, Sanjay Jain, Stu Grossman



On 02/09/2015 10:27 AM, Rajat Jain wrote:
> Hello,
> 
>>> 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.
> 
> Actually, I had simplified the problem statement, to avoid complicating the discussion. Basically, we have enhanced the grub with a "devicetree" command that supplies a flattened device tree to the kernel (just like u-boot or others do in embedded world).
> 
> http://www.denx.de/wiki/view/DULG/LinuxFDTBlob
> 
> However, we need to pass different flattened device tree to the kernel based on different CHASSIS_TYPE. (because different chassis have different hardware components, slots etc that need to be handled differently).
> 
>>>
>>
>> Whups ... somehow stripped all the cc's on my original reply.
>>
>> why isn't 1) already in the kernel itself?
> 
> I hope my problem statement clarification above makes it clearer. However, I think the problem is more generic - essentially depending on the current machine type, we may have to do different things on different platforms, such as:
> * Loading different kernels / initrd / device tree etc.
> * Passing different kernel parameters for:
> 	- different root fs mount points (root=/dev/sda[a/b/c/d]......)
> 	- enabling disabling different kernel features / drivers / subsystem (pci=[nocrs/use_crs]...)
> 	- etc.
> 

Well I understand the request for different kernels, but 1) implies that you're
adding additional kernel parameters based on the CHASSIS type?  So I'm wondering
why you're not making some boot time decision based on the CHASSIS type instead
of a bootloader time decision.

/me is not against the patchset.  I think the idea of booting different kernels
based on the HW sounds reasonable from a administration perspective.

P.
> Thanks,
> 
> Rajat 
> 
> 
> 
>> -----Original Message-----
>> From: Prarit Bhargava [mailto:prarit@redhat.com]
>> Sent: Monday, February 09, 2015 4:44 AM
>> To: David Michael
>> Cc: grub-devel@gnu.org; Leif Lindholm; Andrei Borzenkov; Rajat Jain; Sanjay
>> Jain; Raghuraman Thirumalairajan; Stu Grossman
>> Subject: Re: [PATCH v2] Add a module for retrieving SMBIOS information
>>
>> On 02/08/2015 01:54 PM, David Michael wrote:
>>> The following are two use cases from Rajat Jain <rajatjain@juniper.net>:
>>>
>>> 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.
>>>
>>
>> Whups ... somehow stripped all the cc's on my original reply.
>>
>> why isn't 1) already in the kernel itself?
>>
>> P.
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2] Add a module for retrieving SMBIOS information
  2015-02-09 16:28     ` Prarit Bhargava
@ 2015-02-09 17:38       ` Rajat Jain
  2015-02-09 17:43         ` Prarit Bhargava
  0 siblings, 1 reply; 11+ messages in thread
From: Rajat Jain @ 2015-02-09 17:38 UTC (permalink / raw)
  To: Prarit Bhargava
  Cc: grub-devel, David Michael, Raghuraman Thirumalairajan,
	Andrei Borzenkov, Leif Lindholm, Sanjay Jain, Stu Grossman

Hello Prarit,

> -----Original Message-----
> From: Prarit Bhargava [mailto:prarit@redhat.com]
> Sent: Monday, February 09, 2015 8:29 AM
> To: Rajat Jain
> Cc: David Michael; grub-devel@gnu.org; Leif Lindholm; Andrei Borzenkov;
> Sanjay Jain; Raghuraman Thirumalairajan; Stu Grossman
> Subject: Re: [PATCH v2] Add a module for retrieving SMBIOS information
> 
> 
> 
> On 02/09/2015 10:27 AM, Rajat Jain wrote:
> > Hello,
> >
> >>> 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.
> >
> > Actually, I had simplified the problem statement, to avoid complicating the
> discussion. Basically, we have enhanced the grub with a "devicetree"
> command that supplies a flattened device tree to the kernel (just like u-boot
> or others do in embedded world).
> >
> > http://www.denx.de/wiki/view/DULG/LinuxFDTBlob
> >
> > However, we need to pass different flattened device tree to the kernel
> based on different CHASSIS_TYPE. (because different chassis have different
> hardware components, slots etc that need to be handled differently).
> >
> >>>
> >>
> >> Whups ... somehow stripped all the cc's on my original reply.
> >>
> >> why isn't 1) already in the kernel itself?
> >
> > I hope my problem statement clarification above makes it clearer.
> However, I think the problem is more generic - essentially depending on the
> current machine type, we may have to do different things on different
> platforms, such as:
> > * Loading different kernels / initrd / device tree etc.
> > * Passing different kernel parameters for:
> > 	- different root fs mount points (root=/dev/sda[a/b/c/d]......)
> > 	- enabling disabling different kernel features / drivers / subsystem
> (pci=[nocrs/use_crs]...)
> > 	- etc.
> >
> 
> Well I understand the request for different kernels, but 1) implies that you're
> adding additional kernel parameters based on the CHASSIS type?  So I'm
> wondering why you're not making some boot time decision based on the
> CHASSIS type instead of a bootloader time decision.

Oh OK. I guess you are saying why not read the CHASSIS_TYPE in the kernel and take different paths depending on the value? The reason is because we may want to pass different kernel options that *exist today*. (E.g. different root file systems, or hw tuning parametes etc). Ideally once deployed, we neither want to change the bootloader, nor the kernel since that would require a recompilation which is best avoided. If something changes, it is much easier to change the grub config file which can be changed anytime, and thus we are requesting the *capability* to do it in a grub script or config file.

> 
> /me is not against the patchset.  I think the idea of booting different kernels
> based on the HW sounds reasonable from a administration perspective.

Yup, understood. 

Also from admin perspective, I'm suggesting that we may need tune the kernel differently depending on the HW.

Thanks,

Rajat

> 
> P.
> > Thanks,
> >
> > Rajat
> >
> >
> >
> >> -----Original Message-----
> >> From: Prarit Bhargava [mailto:prarit@redhat.com]
> >> Sent: Monday, February 09, 2015 4:44 AM
> >> To: David Michael
> >> Cc: grub-devel@gnu.org; Leif Lindholm; Andrei Borzenkov; Rajat Jain;
> >> Sanjay Jain; Raghuraman Thirumalairajan; Stu Grossman
> >> Subject: Re: [PATCH v2] Add a module for retrieving SMBIOS
> >> information
> >>
> >> On 02/08/2015 01:54 PM, David Michael wrote:
> >>> The following are two use cases from Rajat Jain <rajatjain@juniper.net>:
> >>>
> >>> 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.
> >>>
> >>
> >> Whups ... somehow stripped all the cc's on my original reply.
> >>
> >> why isn't 1) already in the kernel itself?
> >>
> >> P.
> >


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] Add a module for retrieving SMBIOS information
  2015-02-09 17:38       ` Rajat Jain
@ 2015-02-09 17:43         ` Prarit Bhargava
  0 siblings, 0 replies; 11+ messages in thread
From: Prarit Bhargava @ 2015-02-09 17:43 UTC (permalink / raw)
  To: The development of GNU GRUB
  Cc: Raghuraman Thirumalairajan, David Michael, Andrei Borzenkov,
	Rajat Jain, Leif Lindholm, Sanjay Jain, Stu Grossman



On 02/09/2015 12:38 PM, Rajat Jain wrote:
> Hello Prarit,
> 
>> -----Original Message-----
>> From: Prarit Bhargava [mailto:prarit@redhat.com]
>> Sent: Monday, February 09, 2015 8:29 AM
>> To: Rajat Jain
>> Cc: David Michael; grub-devel@gnu.org; Leif Lindholm; Andrei Borzenkov;
>> Sanjay Jain; Raghuraman Thirumalairajan; Stu Grossman
>> Subject: Re: [PATCH v2] Add a module for retrieving SMBIOS information
>>
>>
>>
>> On 02/09/2015 10:27 AM, Rajat Jain wrote:
>>> Hello,
>>>
>>>>> 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.
>>>
>>> Actually, I had simplified the problem statement, to avoid complicating the
>> discussion. Basically, we have enhanced the grub with a "devicetree"
>> command that supplies a flattened device tree to the kernel (just like u-boot
>> or others do in embedded world).
>>>
>>> http://www.denx.de/wiki/view/DULG/LinuxFDTBlob
>>>
>>> However, we need to pass different flattened device tree to the kernel
>> based on different CHASSIS_TYPE. (because different chassis have different
>> hardware components, slots etc that need to be handled differently).
>>>
>>>>>
>>>>
>>>> Whups ... somehow stripped all the cc's on my original reply.
>>>>
>>>> why isn't 1) already in the kernel itself?
>>>
>>> I hope my problem statement clarification above makes it clearer.
>> However, I think the problem is more generic - essentially depending on the
>> current machine type, we may have to do different things on different
>> platforms, such as:
>>> * Loading different kernels / initrd / device tree etc.
>>> * Passing different kernel parameters for:
>>> 	- different root fs mount points (root=/dev/sda[a/b/c/d]......)
>>> 	- enabling disabling different kernel features / drivers / subsystem
>> (pci=[nocrs/use_crs]...)
>>> 	- etc.
>>>
>>
>> Well I understand the request for different kernels, but 1) implies that you're
>> adding additional kernel parameters based on the CHASSIS type?  So I'm
>> wondering why you're not making some boot time decision based on the
>> CHASSIS type instead of a bootloader time decision.
> 
> Oh OK. I guess you are saying why not read the CHASSIS_TYPE in the kernel and take different paths depending on the value? The reason is because we may want to pass different kernel options that *exist today*. (E.g. different root file systems, or hw tuning parametes etc). Ideally once deployed, we neither want to change the bootloader, nor the kernel since that would require a recompilation which is best avoided. If something changes, it is much easier to change the grub config file which can be changed anytime, and thus we are requesting the *capability* to do it in a grub script or config file.
> 

Ah, I get it.  That certainly makes sense.  I just thought there was some
specific HW command that you always wanted executed on these boards.

>>
>> /me is not against the patchset.  I think the idea of booting different kernels
>> based on the HW sounds reasonable from a administration perspective.
> 
> Yup, understood. 
> 
> Also from admin perspective, I'm suggesting that we may need tune the kernel differently depending on the HW.

Thanks for the info :)

P.

> 
> Thanks,
> 
> Rajat
> 
>>
>> P.
>>> Thanks,
>>>
>>> Rajat
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Prarit Bhargava [mailto:prarit@redhat.com]
>>>> Sent: Monday, February 09, 2015 4:44 AM
>>>> To: David Michael
>>>> Cc: grub-devel@gnu.org; Leif Lindholm; Andrei Borzenkov; Rajat Jain;
>>>> Sanjay Jain; Raghuraman Thirumalairajan; Stu Grossman
>>>> Subject: Re: [PATCH v2] Add a module for retrieving SMBIOS
>>>> information
>>>>
>>>> On 02/08/2015 01:54 PM, David Michael wrote:
>>>>> The following are two use cases from Rajat Jain <rajatjain@juniper.net>:
>>>>>
>>>>> 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.
>>>>>
>>>>
>>>> Whups ... somehow stripped all the cc's on my original reply.
>>>>
>>>> why isn't 1) already in the kernel itself?
>>>>
>>>> P.
>>>
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] Add a module for retrieving SMBIOS information
  2015-02-08 18:54 [PATCH v2] Add a module for retrieving SMBIOS information David Michael
  2015-02-08 20:13 ` Prarit Bhargava
  2015-02-09 12:43 ` Prarit Bhargava
@ 2015-02-11 11:34 ` Andrei Borzenkov
  2015-02-16  3:10   ` David Michael
  2 siblings, 1 reply; 11+ messages in thread
From: Andrei Borzenkov @ 2015-02-11 11:34 UTC (permalink / raw)
  To: David Michael
  Cc: Prarit Bhargava, grub-devel, Raghuraman Thirumalairajan,
	Rajat Jain, Leif Lindholm, Sanjay Jain, Stu Grossman

В Sun, 08 Feb 2015 13:54:46 -0500
David Michael <fedora.dm0@gmail.com> пишет:

> The following are two use cases from Rajat Jain <rajatjain@juniper.net>:
> 
> 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.
> 
> * grub-core/commands/smbios.c: New file.
> * grub-core/Makefile.core.def (smbios): New module.
> * docs/grub.texi (smbios): New node.
> (Command-line and menu entry commands): Add a menu entry for smbios.

That's not needed. ChangeLog is now generated from GIT commits.

> ---
> 
> Changes since v1:
> 
> * Removed formfeeds and quoted use cases in the commit messages as
>   suggested by Prarit Bhargava.
> 
> * Enabled building the module on all EFI platforms as suggested by Leif
>   Lindholm.
> 
>  docs/grub.texi              |  67 ++++++++
>  grub-core/Makefile.core.def |   7 +
>  grub-core/commands/smbios.c | 361 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 435 insertions(+)
>  create mode 100644 grub-core/commands/smbios.c
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 46b9e7f..f4e187f 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,72 @@ if timeout was interrupted by @key{ESC}.
>  @end deffn
>  
>  
> +@node smbios
> +@subsection smbios
> +
> +@deffn Command smbios @
> + [@option{-t} @var{type}] @
> + [@option{-h} @var{handle}] @
> + [@option{-m} @var{match}] @
> + [ (@option{-b} | @option{-w} | @option{-d} | @option{-q} | @option{-s}) @
> +   @var{offset} [@option{-v} @var{variable}] ]

I think that in general we should endorse and advertise long options -
they make scripts much more readable - and mention single letter
options in description (if at all) or help output. Look e.g. at search
command description.

In other places we use "--set var" to set variable. Let's remain
consistent with this usage. If you want to spare some typing, make it
optional, like

smbios offset [[--set] var]

> +Retrieve SMBIOS information.  This command is only available on x86 and EFI
> +systems.
> +
> +When run with no options, @command{smbios} will print the SMBIOS table entries
> +to the console as the header values, a hex dump, and a string list.  The
> +following options can filter which table entries are printed.
> +
> +@itemize @bullet
> +@item
> +Specifying @option{-t} will only print entries with a matching @var{type}.  The
> +type can be any value from 0 to 255.  Specify a value outside this range to
> +match entries with any type.  The default is -1.

I think out of range value should result in error. 

> +@item
> +Specifying @option{-h} will only print entries with a matching @var{handle}.
> +The handle can be any value from 0 to 65535.  Specify a value outside this
> +range to match entries with any handle.  The default is -1.

ditto.

> +@item
> +Specifying @option{-m} will only print number @var{match} in the list of all
> +filtered entries; e.g. @code{smbios -m 3} will print the third entry.  The list
> +is always ordered the same as the hardware's SMBIOS table.  The match number
> +can be any positive value.  Specify 0 to match all entries.  The default is 0.
> +@end itemize
> +
> +The remaining options print the value of a field in the matched SMBIOS table
> +entry.  Only one of these options may be specified.  When they are used, the
> +option @code{-m 0} is synonymous with @code{-m 1} to match the first entry.

If user provides conflicting options this should really be an error.

> +
> +@itemize @bullet
> +@item
> +When given @option{-b}, print the value of the byte
> +at @var{offset} bytes into the matched SMBIOS table entry.
> +@item
> +When given @option{-w}, print the value of the word (two bytes)
> +at @var{offset} bytes into the matched SMBIOS table entry.
> +@item
> +When given @option{-d}, print the value of the dword (four bytes)
> +at @var{offset} bytes into the matched SMBIOS table entry.
> +@item
> +When given @option{-q}, print the value of the qword (eight bytes)
> +at @var{offset} bytes into the matched SMBIOS table entry.
> +@item
> +When given @option{-s}, print the string table entry with its index found
> +at @var{offset} bytes into the matched SMBIOS table entry.
> +@end itemize
> +
> +If any of the options in the above list were given, a variable name can be
> +specified with @option{-v} to store the value instead of printing it.
> +
> +For example, this will store and display the system manufacturer string.
> +
> +@example
> +smbios -t 1 -s 4 -v system_manufacturer
> +echo $system_manufacturer
> +@end example
> +@end deffn
> +
> +
>  @node source
>  @subsection source
>  
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 42443bc..f87de70 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1023,6 +1023,13 @@ module = {
>  };
>  
>  module = {
> +  name = smbios;
> +  common = commands/smbios.c;
> +  enable = efi;
> +  enable = x86;
> +};
> +
> +module = {
>    name = suspend;
>    ieee1275 = commands/ieee1275/suspend.c;
>    enable = i386_ieee1275;
> diff --git a/grub-core/commands/smbios.c b/grub-core/commands/smbios.c
> new file mode 100644
> index 0000000..d8edeea
> --- /dev/null
> +++ b/grub-core/commands/smbios.c
> @@ -0,0 +1,361 @@
> +/* Expose SMBIOS data to the console and configuration files */
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2013,2014,2015  Free Software Foundation, Inc.
> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/dl.h>
> +#include <grub/extcmd.h>
> +#include <grub/env.h>
> +#include <grub/i18n.h>
> +#include <grub/misc.h>
> +#include <grub/mm.h>
> +
> +#ifdef GRUB_MACHINE_EFI
> +#include <grub/efi/efi.h>
> +#else
> +#include <grub/acpi.h>
> +#endif
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +/* Reference: DMTF Standard DSP0134 2.7.1 Table 1 */
> +
> +struct __attribute__ ((packed)) grub_smbios_ieps
> +  {
> +    grub_uint8_t  anchor[5]; /* "_DMI_" */
> +    grub_uint8_t  checksum;
> +    grub_uint16_t table_length;
> +    grub_uint32_t table_address;
> +    grub_uint16_t structures;
> +    grub_uint8_t  revision;
> +  };
> +
> +struct __attribute__ ((packed)) grub_smbios_eps
> +  {
> +    grub_uint8_t  anchor[4]; /* "_SM_" */
> +    grub_uint8_t  checksum;
> +    grub_uint8_t  length;
> +    grub_uint8_t  version_major;
> +    grub_uint8_t  version_minor;
> +    grub_uint16_t maximum_structure_size;
> +    grub_uint8_t  revision;
> +    grub_uint8_t  formatted[5];
> +    struct grub_smbios_ieps intermediate;
> +  };
> +
> +static const struct grub_smbios_eps *eps = NULL;
> +
> +/* Reference: DMTF Standard DSP0134 2.7.1 Section 5.2.1 */
> +
> +/*
> + * 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
> +
> +  grub_dprintf ("smbios", "Failed to locate entry point structure\n");
> +  return NULL;
> +}
> +
> +/* Reference: DMTF Standard DSP0134 2.7.1 Sections 6.1.2-6.1.3 */
> +
> +/*
> + * Given a pointer to the first bytes of an entry, print its contents in a
> + * semi-human-readable format.  Return the size of the entry printed.
> + */
> +static grub_uint16_t
> +grub_smbios_dump_entry (const grub_uint8_t *entry)
> +{
> +  const grub_uint8_t *ptr = entry;
> +  grub_uint8_t newstr = 1;
> +  grub_uint8_t length;
> +
> +  /* Write the entry's mandatory four header bytes. */
> +  length = ptr[1];
> +  grub_printf ("Entry: Type=0x%02x Length=0x%02x Handle=0x%04x\n",
> +               ptr[0], length, *(1 + (grub_uint16_t *)ptr));

Could you reverse the order in 1 + (grub_uint16_t *)ptr and in other
places too?

> +
> +  /* 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 list. */
> +  while (ptr[0] != 0 || ptr[1] != 0)

I wonder do we have any ad hoc way to limit it? It looks like Linux
kernel does more sanity checks and does not blindly trust data.

> +    {
> +      if (newstr)
> +        grub_printf (" String: %s\n", ptr);
> +      newstr = *ptr++ == 0;
> +    }
> +  ptr += 2;
> +
> +  /* Return the total number of bytes covered. */
> +  return ptr - entry;
> +}
> +
> +/* Reference: DMTF Standard DSP0134 2.7.1 Sections 6.1.2-6.1.3 */
> +
> +/*
> + * Return or print a matched table entry.  Multiple entries can be matched if
> + * they are being printed.
> + *
> + * This method can handle up to three criteria for selecting an entry:
> + *   - The entry's "type" field             (use outside [0,0xFF] to ignore)
> + *   - The entry's "handle" field           (use outside [0,0xFFFF] to ignore)
> + *   - The entry 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_entry (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)
> +{
> +  grub_addr_t table_address = (grub_addr_t)ep->intermediate.table_address;
> +  grub_uint16_t table_length = ep->intermediate.table_length;
> +  grub_uint16_t structures = ep->intermediate.structures;
> +  grub_uint16_t structure = 0;
> +  grub_uint16_t matches = 0;
> +  const grub_uint8_t *table = (const grub_uint8_t *)table_address;
> +  const grub_uint8_t *ptr = table;
> +
> +  while (ptr - table < table_length && structure++ < structures)
> +
> +    /* Test if the current entry matches the given parameters. */
> +    if ((handle < 0 || 65535 < handle || handle == *(1 + (grub_uint16_t *)ptr))
> +        && (type < 0 || 255 < type || type == ptr[0])
> +        && (match == 0 || match == ++matches))

As said it is better to validate on input, so handle > 65535 etc can be
omitted.

> +      {
> +        if (print)
> +          {
> +            ptr += grub_smbios_dump_entry (ptr);
> +            if (match > 0)
> +              break;
> +          }
> +        else
> +          return ptr;
> +      }
> +
> +    /* If the entry didn't match, skip it (formatted area and string list). */
> +    else
> +      {
> +        ptr += ptr[1];
> +        while (*ptr++ != 0 || *ptr++ != 0);

Same here about more checks for table end.

> +      }
> +
> +  return NULL;
> +}
> +
> +/* Reference: DMTF Standard DSP0134 2.7.1 Section 6.1.3 */
> +
> +/*
> + * Given a pointer to a string list and a number, iterate over each of the
> + * strings until the desired item is reached.  (The value of 1 indicates the
> + * first string, etc.)
> + */
> +static const char *
> +grub_smbios_get_string (const grub_uint8_t *strings, const grub_uint8_t number)
> +{
> +  const char *ptr = (const char *)strings;
> +  grub_uint8_t newstr = 1;
> +  grub_uint8_t index = 1;
> +
> +  /* A string referenced with zero is interpreted as unset. */
> +  if (number == 0)
> +    return NULL;
> +
> +  /* Search the string table, incrementing the counter as each null passes. */
> +  while (ptr[0] != 0 || ptr[1] != 0)

And here.

> +    {
> +      if (newstr && number == index++)
> +        return ptr;
> +      newstr = *ptr++ == 0;
> +    }
> +
> +  /* The requested string index is greater than the number of table entries. */
> +  return NULL;
> +}
> +
> +static grub_err_t
> +grub_cmd_smbios (grub_extcmd_context_t ctxt,
> +                 int argc __attribute__ ((unused)),
> +                 char **argv __attribute__ ((unused)))
> +{
> +  struct grub_arg_list *state = ctxt->state;
> +
> +  grub_int16_t type = -1;
> +  grub_int32_t handle = -1;
> +  grub_uint16_t match = 0;
> +  grub_uint8_t offset = 0;
> +
> +  const grub_uint8_t *entry;
> +  grub_uint8_t accessors;
> +  grub_uint8_t i;
> +  char buffer[24]; /* 64-bit number -> maximum 20 decimal digits */
> +  const char *value = buffer;
> +
> +  if (eps == NULL)
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("missing entry point"));

"SMBIOS table not found" or something like this, so it is more
understandable.

> +
> +  /* Only one value can be returned at a time; reject multiple selections. */
> +  accessors = !!state[3].set + !!state[4].set + !!state[5].set +
> +              !!state[6].set + !!state[7].set;
> +  if (accessors > 1)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("too many -b|-w|-d|-q|-s"));

Only one of ... can be specified

> +
> +  /* Reject the environment variable if no value was selected. */
> +  if (accessors == 0 && state[8].set)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("-v without -b|-w|-d|-q|-s"));

--set requires explicit value type.

> +
> +  /* Read the given matching parameters. */
> +  if (state[0].set)
> +    type = grub_strtol (state[0].arg, NULL, 0);
> +  if (state[1].set)
> +    handle = grub_strtol (state[1].arg, NULL, 0);
> +  if (state[2].set)
> +    match = grub_strtoul (state[2].arg, NULL, 0);
> +
> +  /* When not selecting a value, print all matching entries and quit. */
> +  if (accessors == 0)
> +    {
> +      grub_smbios_match_entry (eps, type, handle, match, 1);
> +      return GRUB_ERR_NONE;
> +    }
> +
> +  /* Select a single entry from the matching parameters. */
> +  entry = grub_smbios_match_entry (eps, type, handle, match, 0);
> +  if (entry == NULL)
> +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, N_("no such entry"));
> +
> +  /* Ensure the specified offset+length is within the matched entry. */
> +  for (i = 3; i <= 7; i++)
> +    if (state[i].set)
> +      {
> +        offset = grub_strtoul (state[i].arg, NULL, 0);
> +        if (offset + (1 << (i == 7 ? 0 : i - 3)) > entry[1])

Such optimizations will really strike back in future, as soon as options
are changed and make code confusing. Just explicitly set entry size when
parsing arguments.

> +          return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("value outside entry"));
> +        break;
> +      }
> +
> +  /* If a string was requested, try to find its pointer. */
> +  if (state[7].set)
> +    {
> +      value = grub_smbios_get_string (entry + entry[1], entry[offset]);
> +      if (value == NULL)
> +        return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("string not defined"));
> +    }
> +
> +  /* Create a string from a numeric value suitable for printing. */
> +  else if (state[3].set)
> +    grub_snprintf (buffer, sizeof (buffer), "%u", entry[offset]);
> +  else if (state[4].set)
> +    grub_snprintf (buffer, sizeof (buffer), "%u",
> +                   *(grub_uint16_t *)(entry + offset));

Will it also work on other platforms (alignment)?

> +  else if (state[5].set)
> +    grub_snprintf (buffer, sizeof (buffer), "%" PRIuGRUB_UINT32_T,
> +                   *(grub_uint32_t *)(entry + offset));

Ditto.

> +  else if (state[6].set)
> +    grub_snprintf (buffer, sizeof (buffer), "%" PRIuGRUB_UINT64_T,
> +                   *(grub_uint64_t *)(entry + offset));
> +

Ditto.

> +  /* Store or print the requested value. */
> +  if (state[8].set)
> +    {
> +      grub_env_set (state[8].arg, value);
> +      grub_env_export (state[8].arg);
> +    }
> +  else
> +    grub_printf ("%s\n", value);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +static grub_extcmd_t cmd;
> +
> +static const struct grub_arg_option options[] =
> +  {
> +    {"type",       't', 0, N_("Match entries with the given type."),
> +                           N_("byte"), ARG_TYPE_INT},
> +    {"handle",     'h', 0, N_("Match entries with the given handle."),
> +                           N_("word"), ARG_TYPE_INT},
> +    {"match",      'm', 0, N_("Select the entry to use when several match."),
> +                           N_("number"), ARG_TYPE_INT},
> +    {"get-byte",   'b', 0, N_("Get the byte's value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-word",   'w', 0, N_("Get two bytes' value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-dword",  'd', 0, N_("Get four bytes' value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-qword",  'q', 0, N_("Get eight bytes' value at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"get-string", 's', 0, N_("Get the string specified at the given offset."),
> +                           N_("offset"), ARG_TYPE_INT},
> +    {"variable",   'v', 0, N_("Store the value in the given variable name."),
> +                           N_("name"), ARG_TYPE_STRING},
> +    {0, 0, 0, 0, 0, 0}
> +  };
> +
> +GRUB_MOD_INIT(smbios)
> +{
> +  /* SMBIOS data is supposed to be static, so find it only once during init. */
> +  eps = grub_smbios_locate_eps ();
> +
> +  cmd = grub_register_extcmd ("smbios", grub_cmd_smbios, 0,
> +                              N_("[-t byte] [-h word] [-m number] "
> +                                 "[(-b|-w|-d|-q|-s) offset [-v name]]"),
> +                              N_("Retrieve SMBIOS information."), options);
> +}
> +
> +GRUB_MOD_FINI(smbios)
> +{
> +  grub_unregister_extcmd (cmd);
> +}



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] Add a module for retrieving SMBIOS information
  2015-02-11 11:34 ` Andrei Borzenkov
@ 2015-02-16  3:10   ` David Michael
  2015-03-16 18:26     ` Rajat Jain
  0 siblings, 1 reply; 11+ messages in thread
From: David Michael @ 2015-02-16  3:10 UTC (permalink / raw)
  To: Andrei Borzenkov
  Cc: Prarit Bhargava, The development of GNU GRUB,
	Raghuraman Thirumalairajan, Rajat Jain, Leif Lindholm,
	Sanjay Jain, Stu Grossman

On Wed, Feb 11, 2015 at 6:34 AM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> В Sun, 08 Feb 2015 13:54:46 -0500
> David Michael <fedora.dm0@gmail.com> пишет:
>> +@item
>> +Specifying @option{-t} will only print entries with a matching @var{type}.  The
>> +type can be any value from 0 to 255.  Specify a value outside this range to
>> +match entries with any type.  The default is -1.
>
> I think out of range value should result in error.

Okay, but I think the user should still be able to specify a value
that can remove a previously specified type.  Maybe a separate long
option "--no-type" or a magic value like "-t any"?

>> +  /* Write the entry's mandatory four header bytes. */
>> +  length = ptr[1];
>> +  grub_printf ("Entry: Type=0x%02x Length=0x%02x Handle=0x%04x\n",
>> +               ptr[0], length, *(1 + (grub_uint16_t *)ptr));
>
> Could you reverse the order in 1 + (grub_uint16_t *)ptr and in other
> places too?

I'll replace all these pointer casts+dereferences with functions that
avoid alignment issues.

>> +  /* 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 list. */
>> +  while (ptr[0] != 0 || ptr[1] != 0)
>
> I wonder do we have any ad hoc way to limit it? It looks like Linux
> kernel does more sanity checks and does not blindly trust data.

The entry point structure has a two-byte field for total SMBIOS table
length, so that can put an upper limit on the string sets.

>

Thanks for the detailed review.  I hope to be able to be able to send
a new revision later in the week.

Side note: A new major version of the SMBIOS specification was posted
last week which includes 64-bit addresses and such.  I'm not planning
to try to add this at the moment, unless anyone has a need for it.

Thanks.

David


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2] Add a module for retrieving SMBIOS information
  2015-02-16  3:10   ` David Michael
@ 2015-03-16 18:26     ` Rajat Jain
  2015-03-18  3:04       ` David Michael
  0 siblings, 1 reply; 11+ messages in thread
From: Rajat Jain @ 2015-03-16 18:26 UTC (permalink / raw)
  To: David Michael, Andrei Borzenkov
  Cc: Prarit Bhargava, The development of GNU GRUB,
	Raghuraman Thirumalairajan, Leif Lindholm, Sanjay Jain,
	Stu Grossman

Hello,

Just wanted to check on the status of this patch. Has it been accepted / merged?

Thanks,

Rajat


> -----Original Message-----
> From: David Michael [mailto:fedora.dm0@gmail.com]
> Sent: Sunday, February 15, 2015 7:11 PM
> To: Andrei Borzenkov
> Cc: The development of GNU GRUB; Prarit Bhargava; Leif Lindholm; Rajat
> Jain; Sanjay Jain; Raghuraman Thirumalairajan; Stu Grossman
> Subject: Re: [PATCH v2] Add a module for retrieving SMBIOS information
> 
> On Wed, Feb 11, 2015 at 6:34 AM, Andrei Borzenkov <arvidjaar@gmail.com>
> wrote:
> > В Sun, 08 Feb 2015 13:54:46 -0500
> > David Michael <fedora.dm0@gmail.com> пишет:
> >> +@item
> >> +Specifying @option{-t} will only print entries with a matching
> >> +@var{type}.  The type can be any value from 0 to 255.  Specify a
> >> +value outside this range to match entries with any type.  The default is -
> 1.
> >
> > I think out of range value should result in error.
> 
> Okay, but I think the user should still be able to specify a value that can
> remove a previously specified type.  Maybe a separate long option "--no-
> type" or a magic value like "-t any"?
> 
> >> +  /* Write the entry's mandatory four header bytes. */  length =
> >> + ptr[1];  grub_printf ("Entry: Type=0x%02x Length=0x%02x
> >> + Handle=0x%04x\n",
> >> +               ptr[0], length, *(1 + (grub_uint16_t *)ptr));
> >
> > Could you reverse the order in 1 + (grub_uint16_t *)ptr and in other
> > places too?
> 
> I'll replace all these pointer casts+dereferences with functions that avoid
> alignment issues.
> 
> >> +  /* 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 list. */  while
> >> + (ptr[0] != 0 || ptr[1] != 0)
> >
> > I wonder do we have any ad hoc way to limit it? It looks like Linux
> > kernel does more sanity checks and does not blindly trust data.
> 
> The entry point structure has a two-byte field for total SMBIOS table length,
> so that can put an upper limit on the string sets.
> 
> >
> 
> Thanks for the detailed review.  I hope to be able to be able to send a new
> revision later in the week.
> 
> Side note: A new major version of the SMBIOS specification was posted last
> week which includes 64-bit addresses and such.  I'm not planning to try to
> add this at the moment, unless anyone has a need for it.
> 
> Thanks.
> 
> David

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] Add a module for retrieving SMBIOS information
  2015-03-16 18:26     ` Rajat Jain
@ 2015-03-18  3:04       ` David Michael
  0 siblings, 0 replies; 11+ messages in thread
From: David Michael @ 2015-03-18  3:04 UTC (permalink / raw)
  To: Rajat Jain
  Cc: Prarit Bhargava, The development of GNU GRUB,
	Raghuraman Thirumalairajan, Andrei Borzenkov, Leif Lindholm,
	Sanjay Jain, Stu Grossman

On Mon, Mar 16, 2015 at 2:26 PM, Rajat Jain <rajatjain@juniper.net> wrote:
> Hello,
>
> Just wanted to check on the status of this patch. Has it been accepted / merged?

No, sorry, I lost track of it.  There are still a few points I need to
change from Andrei's review before sending a new revision.

David


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-03-18  3:04 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-08 18:54 [PATCH v2] Add a module for retrieving SMBIOS information David Michael
2015-02-08 20:13 ` Prarit Bhargava
2015-02-09 12:43 ` Prarit Bhargava
2015-02-09 15:27   ` Rajat Jain
2015-02-09 16:28     ` Prarit Bhargava
2015-02-09 17:38       ` Rajat Jain
2015-02-09 17:43         ` Prarit Bhargava
2015-02-11 11:34 ` Andrei Borzenkov
2015-02-16  3:10   ` David Michael
2015-03-16 18:26     ` Rajat Jain
2015-03-18  3:04       ` David Michael

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.