Message-ID: <1424940642.25006.60.camel@chaos.site>
Date:	Thu, 26 Feb 2015 09:50:42 +0100
From:	Jean Delvare <jdelvare@...e.de>
To:	dmidecode-devel@...gnu.org,
	Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>
Cc:	Mark Salter <msalter@...hat.com>, matt.fleming@...el.com,
	ard.biesheuvel@...aro.org, linux-kernel@...r.kernel.org,
	leif.lindholm@...aro.org, grant.likely@...aro.org
Subject: Re: [dmidecode] [Patch v3] firmware: dmi-sysfs: add SMBIOS entry
 point area raw attribute

Hi Ivan, Mark and all,

Le Tuesday 03 February 2015 à 17:48 +0200, Ivan Khoronzhuk a écrit :
> On 02/03/2015 04:58 PM, Mark Salter wrote:
> > On Wed, 2015-01-28 at 14:39 +0200, Ivan Khoronzhuk wrote:
> >> Some utils, like dmidecode and smbios, needs to access SMBIOS entry
> >> table area in order to get information like SMBIOS version, size, etc.
> >> Currently it's done via /dev/mem. But for situation when /dev/mem
> >> usage is disabled, the utils have to use dmi sysfs instead, which
> >> doesn't represent SMBIOS entry. So this patch adds SMBIOS area to
> >> dmi-sysfs in order to allow utils in question to work correctly with
> >> dmi sysfs interface.
> >>
> >> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@...aro.org>
> >> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@...aro.org>
> > Sorry for coming in late, here. Why expose the raw header
> > instead of exposing the pieces as individual files like
> > the kernel does for the other dmi info? That way the kernel
> > decodes the header and presents it in an easy to read
> > format for dmidecode or even a shell script.

Sorry for coming in even later.

This is a common misconception that dmidecode would be happier with
pre-processed data. In fact it's exactly the opposite, dmidecode will be
much happier with raw data because it already has all the code to decode
the raw data, and that code will have to stay in place anyway as not all
systems have sysfs with the proper information exposed to avoid reading
the raw data from /dev/mem directly.

In a particular it should be noted that the
current /sys/firmware/dmi/entries interface is completely unsuited for
consumption by dmidecode. It would require a significant amount of extra
code in dmidecode to walk and parse the hundreds of files in this
directory. And there would be additional problems, such as slower
execution (500 file open/close cycles, thank you very much) and entries
not being displayed in the same order as when dmidecode reads the table
directly.

So not only Ivan is right with his idea of exposing the raw DMI/SMBIOS
entry point in sysfs, but I think we need to go even further and also
expose the raw DMI data table itself through sysfs. It should go
under /sys/firmware/dmi/tables/, much like ACPI tables live
under /sys/firmware/acpi/tables/.

I would also suggest that both the raw entry point and the raw table
data should be presented regardless of CONFIG_DMI_SYSFS. The code should
be small enough that it should be OK to include it unconditionally. Most
systems don't need the dmi-sysfs code, the use cases seem rather limited
to me, and on distributions it's generally built as a module and not
loaded by default.

> The SMBIOS entry point can contain specific fields depending on it's
> version. In the specification I didn't find any rules concerning this.
> 
> Only field that probably will be available is version number, but
> the version number is not only var that can be required by utils.
> For example, dmidecode needs also print some additional info like
> phys address where dmitable is placed.
> 
> I don't sure how exactly next SMBIOS version will be changed.
> It can happen that some new data is available...and some old is removed.
> It's better to export it as raw data like it was done for dmi entries
> via raw attribute and It's better to pass the whole entry table
> instead of each time modify the dmi sysfs interface when new SMBIOS
> version is issued.
> 
> 
> >
> >> ---
> >>
> >> v1: https://lkml.org/lkml/2015/1/23/643
> >> v2: https://lkml.org/lkml/2015/1/26/345
> >>
> >> v3..v2:

This notation is confusing, being opposite to the git syntax. Please
just write "Changes since v2".

> >>    firmware: dmi_scan: add symbol to get SMBIOS entry area
> >>    firmware: dmi-sysfs: add SMBIOS entry point area attribute
> >> 	combined in one patch
> >> 	added SMBIOS information to ABI sysfs-dmi documentaton
> >>
> >> v2..v1:
> >>    firmware: dmi_scan: add symbol to get SMBIOS entry area
> >> 	- used additional static var to hold SMBIOS raw table size
> >> 	- changed format of get_smbios_entry_area symbol
> >> 	  returned pointer on const smbios table
> >>
> >>    firmware: dmi-sysfs: add SMBIOS entry point area attribute
> >> 	- adopted to updated get_smbios_entry_area symbol
> >>    	- removed redundant array to save smbios table
> >>
> >>
> >>   Documentation/ABI/testing/sysfs-firmware-dmi | 10 +++++++
> >>   drivers/firmware/dmi-sysfs.c                 | 42 ++++++++++++++++++++++++++++
> >>   drivers/firmware/dmi_scan.c                  | 26 +++++++++++++++++
> >>   include/linux/dmi.h                          |  3 ++
> >>   4 files changed, 81 insertions(+)
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-firmware-dmi b/Documentation/ABI/testing/sysfs-firmware-dmi
> >> index c78f9ab..3a9ffe8 100644
> >> --- a/Documentation/ABI/testing/sysfs-firmware-dmi
> >> +++ b/Documentation/ABI/testing/sysfs-firmware-dmi
> >> @@ -12,6 +12,16 @@ Description:
> >>   		cannot ensure that the data as exported to userland is
> >>   		without error either.
> >>   
> >> +		The firmware provides DMI structures as a packed list of
> >> +		data referenced by a SMBIOS table entry point. The SMBIOS
> >> +		entry point contains general information, like SMBIOS
> >> +		version, DMI table size, etc. The structure, content and
> >> +		size of SMBIOS entry point is dependent on SMBIOS version.
> >> +		That's why SMBIOS entry point is represented in dmi sysfs
> >> +		like a raw attribute and is accessible via
> >> +		/sys/firmware/dmi/smbios_raw_header. The format of SMBIOS
> >> +		entry point header can be read in SMBIOS specification.
> >> +
> >>   		DMI is structured as a large table of entries, where
> >>   		each entry has a common header indicating the type and
> >>   		length of the entry, as well as a firmware-provided
> >> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c
> >> index e0f1cb3..61b6a38 100644
> >> --- a/drivers/firmware/dmi-sysfs.c
> >> +++ b/drivers/firmware/dmi-sysfs.c
> >> @@ -29,6 +29,8 @@
> >>   #define MAX_ENTRY_TYPE 255 /* Most of these aren't used, but we consider
> >>   			      the top entry type is only 8 bits */
> >>   
> >> +static const u8 *smbios_raw_header;

It's called an entry point in the specification and every document out
there (including your own text above), why do you want to suddenly call
it a "header"? The term "header" is used to designate something
completely different in the context of DMI/SMBIOS data so I find it
quite confusing.

Please also note that the recently released version 3.0.0 of the SMBIOS
specification introduces a new entry point format, and the firmware is
allowed to implement both the old and the new format. It may be
desirable to expose both to user-space under different names.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists