Re: [dmidecode] [Patch v3] firmware: dmi-sysfs: add SMBIOS entry point area raw attribute

["Ivan.khoronzhuk" <ivan.khoronzhuk@globallogic.com>]

Hi Jean,
See reply at v4.

On 02/26/2015 10:50 AM, Jean Delvare wrote:
> 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@linaro.org>
>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.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,

-- 
Regards,
Ivan Khoronzhuk