Re: [dmidecode] Changes to support /sys/firmware/dmi/tables/

[Jean Delvare]

Hi Roy,

Note to all: I'll be on channel #dmidecode on FreeNode today and
tomorrow if anyone wants to discuss all these changes.

On Thu, 9 Apr 2015 17:43:16 -0700, Roy Franz wrote:
> I've taken a quick look adding support for the new SMBIOS tables in sysfs 
> that Ivan
> has posted Linux patches for 
> (http://marc.info/?l=linux-kernel&m=142797946923818&w=2),
> and it looks like support for these is quite simple since it if very similar 
> to how dump files
> are handled.
> The quick patch below works for me with Ivan's patches on x86_64.  A few 
> issues
> like how sysfs will deal with both v2 and v3 SMBIOS entry points remain to be
> dealt with.

Test results first: this works fine on my machine, except for:

Table at 0x00000000.

The actual table address should be displayed as it was before. Until
now it was always equal to the offset in the (/dev/mem) file from which
the table was read, but this is no longer the case, so some more work
will be needed (passing an additional parameter to dmi_table, or moving
some code around.)

Also I think we want to inform the user that we are reading SMBIOS/DMI
data from sysfs. This will be handy if we get bug reports for the next
version of dmidecode. We might as well add a similar message when
reading from /dev/mem too, as this will no longer be the default, and
this would be the only case where we don't quote our source.

I am also pondering the possibility of adding a command line parameter
--no-sysfs or similar to inhibit the new code. This would be useful
during development time and for debugging, but it might become useless
after that, so I'm not completely sure if it's worth it.

> A couple of other things that came to mind while working on this:
> * with smbios_decode() now taking an offset, the dumps could retain
>   the address of the SMBIOS tables, and the caller could provide
>   the appropriate fixed offset.  The address is probably not that
>   interesting, but it is nice to preserve the table intact.

I don't really understand what you mean here. I see that the code is
able to dissociate the offset and the address now (modulo my comment
above) but in the binary dump format as defined today, the address
field is overridden with the offset. While the offset is always 0x20,
the code reads that value from the binary dump file in the current and
previous versions of dmidecode, it is not hard-coded in the decoding
code. So if you suddenly decide to leave the physical address there,
and hard-code the offset in the code which reads the binary dump, then
dumps generated with dmidecode >= 2.13 can no longer be read with
dmidecode <= 2.12.

You can argue that it was a poor decision from me back then and I
wouldn't necessarily disagree, but things are the way they are now and
I don't want to break backward compatibility just to display the
original address when reading from a dump file.

> * being focused on looking at /dev/mem issues, I was constantly checking
>   to see what the *devmem arg was.  In all the places I looked, it was
>   really just the filename being used.  Would a patch that just changes
>   the *devmem arguments to *filename be acceptable?  I think this would
>   lead to more readable code.

The argument is named *devmem for historical reasons. I have no
objection to renaming it to something more generic now.

> commit 4f5be1941ef3973bdb5dd6d8af37313674cc6ad3
> Author: Roy Franz <address@hidden>
> Date:   Thu Apr 9 14:16:15 2015 -0700
> 
>     Add support for reading entry point and SMBIOS tables from sysfs
>     
>     ** Patch for discussion only, kernel sysfs interface not upstream yet **
>     
>     This patch adds support for using the SMBIOS entry point and tables from
>     /sys/firmware/dmi/tables/ instead of directly accessing /dev/mem.  The
>     sysfs files are tried first, with the existing methods used when sysfs
>     is not available.  This makes the sysfs files the preferred method.
>     An offset parameter is added to smbios_decode() to allow it to process
>     an unmodified entry point structure.
> 
> diff --git a/dmidecode.c b/dmidecode.c
> index a5304a7..0d84f13 100644
> --- a/dmidecode.c
> +++ b/dmidecode.c
> @@ -4468,7 +4468,7 @@ static void overwrite_dmi_address(u8 *buf)
>       buf[0x0B] = 0;
>  }
>  
> -static int smbios_decode(u8 *buf, const char *devmem)
> +static int smbios_decode(u8 *buf, const char *devmem, u32 offset)
>  {
>       u16 ver;
>  
> @@ -4499,7 +4499,7 @@ static int smbios_decode(u8 *buf, const char *devmem)
>               printf("SMBIOS %u.%u present.\n",
>                       ver >> 8, ver & 0xFF);
>  
> -     dmi_table(DWORD(buf + 0x18), WORD(buf + 0x16), WORD(buf + 0x1C),
> +     dmi_table(offset, WORD(buf + 0x16), WORD(buf + 0x1C),
>               ver, devmem);
>  
>       if (opt.flags & FLAG_DUMP_BIN)
> @@ -4646,7 +4646,7 @@ int main(int argc, char * const argv[])
>  
>               if (memcmp(buf, "_SM_", 4) == 0)
>               {
> -                     if (smbios_decode(buf, opt.dumpfile))
> +                     if (smbios_decode(buf, opt.dumpfile, DWORD(buf + 0x18)))
>                               found++;
>               }
>               else if (memcmp(buf, "_DMI_", 5) == 0)
> @@ -4657,7 +4657,20 @@ int main(int argc, char * const argv[])
>               goto done;
>       }
>  
> -     /* First try EFI (ia64, Intel-based Mac) */
> +     /* First try sysfs tables */
> +     if ((buf = mem_chunk(0, 0x1f, 
> "/sys/firmware/dmi/tables/smbios_entry_point")) != NULL)

mem_chunk() doesn't seem appropriate, as you don't know the size in
advance. Legacy, SMBIOS 2 and SMBIOS 3 entry points all have different
lengths. You need to read the whole file and then determine the entry
point type before you can call the right decoding function. I think
this calls for a new utility function in util.c, which should be a lot
simpler than mem_chunk() as reading from a sysfs attribute shouldn't
require any mmap trick.

> +     {
> +             if (memcmp(buf, "_SM_", 4) == 0)
> +             {
> +                     if (smbios_decode(buf, "/sys/firmware/dmi/tables/DMI", 
> 0))
> +                             found++;
> +             }
> +             // TODO - does legacy (ie no EFI) need to be supported with 
> this method?
> +             // Not sure what kernel will do with sysfs in those cases

I don't quite follow you, the sysfs interface will be available for
both EFI and non-EFI systems. What's missing here however is that the
entry point in /sys/firmware/dmi/tables/smbios_entry_point may be a
legacy one (starting with _DMI_, not _SM_), so you need to try both
smbios_decode and legacy_decode (and soon smbios3_decode as well.) So
function legacy_decode will need the offset parameter too.

> +             goto done;
> +     }
> +
> +     /* next try EFI (ia64, Intel-based Mac) */

Leading capital please.

>       efi = address_from_efi(&fp);
>       switch (efi)
>       {
> @@ -4674,7 +4687,7 @@ int main(int argc, char * const argv[])
>               goto exit_free;
>       }
>  
> -     if (smbios_decode(buf, opt.devmem))
> +     if (smbios_decode(buf, opt.devmem, DWORD(buf + 0x18)))
>               found++;
>       goto done;
>  
> @@ -4690,7 +4703,7 @@ memory_scan:
>       {
>               if (memcmp(buf + fp, "_SM_", 4) == 0 && fp <= 0xFFE0)
>               {
> -                     if (smbios_decode(buf+fp, opt.devmem))
> +                     if (smbios_decode(buf+fp, opt.devmem, DWORD(buf + fp + 
> 0x18)))
>                       {
>                               found++;
>                               fp += 16;


-- 
Jean Delvare
SUSE L3 Support