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

[Roy Franz]

On Thu, Apr 16, 2015 at 5:02 AM, Jean Delvare <address@hidden> wrote:
> 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.)

I missed noticing this.  I will add a parameter to dmi_table()
>
> 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 have added prints identifying sysfs as the source, as well as differentiating
getting the entry point from EFI vs scanning memory for it.

>
> 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.
I've included a patch for this, as it will at least be convenient for now.
It might be more useful long term combined with an option to prefer the
32 bit entry point.  These two options (along with /dev/mem) access
would provide good debugging info if a platform is released with 2 entry
points, and 2 different tables.  The two options together should allow them
to dump both sets of entry points/tables.

>
>> 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.
Yeah, I think it was more of just noticing the information is
destroyed in the dumping
process.  I agree there is no way to fix it without breaking dumps being read on
older releases, so there is no point.  If the information was important, I would
think this would have come up before.

>
>> * 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.
I think that will be nice for the next person reading, but I'll hold
off doing name-changes
until functional changes are done.

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

I have added a read_file() function to handle this.
>
>> +     {
>> +             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.

Yup, taken care of in new series.
>
>> +             goto done;
>> +     }
>> +
>> +     /* next try EFI (ia64, Intel-based Mac) */
>
> Leading capital please.

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

I will shortly be posting a new series based on this feedback.  I have
not added handling of the SMBIOS v3 entry point,
as it sounded on IRC like you had that at least underway (and I ran
out of time today).  I wanted to get an updated patchset
out tonight so you would have it in the morning.  I should be more
available on IRC tomorrow than I was today.

Thanks,
Roy