From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759895AbbCDMfw (ORCPT ); Wed, 4 Mar 2015 07:35:52 -0500 Received: from exprod5og125.obsmtp.com ([64.18.0.245]:49817 "EHLO exprod5og125.obsmtp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753926AbbCDMfs (ORCPT ); Wed, 4 Mar 2015 07:35:48 -0500 X-Greylist: delayed 459 seconds by postgrey-1.27 at vger.kernel.org; Wed, 04 Mar 2015 07:35:47 EST Message-ID: <54F6FA52.3010507@globallogic.com> Date: Wed, 04 Mar 2015 14:28:02 +0200 From: "Ivan.khoronzhuk" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Jean Delvare , dmidecode-devel@nongnu.org, Ivan Khoronzhuk CC: Mark Salter , matt.fleming@intel.com, ard.biesheuvel@linaro.org, linux-kernel@vger.kernel.org, leif.lindholm@linaro.org, grant.likely@linaro.org Subject: Re: [dmidecode] [Patch v3] firmware: dmi-sysfs: add SMBIOS entry point area raw attribute References: <1422448763-17583-1-git-send-email-ivan.khoronzhuk@linaro.org> <1422975508.18187.77.camel@deneb.redhat.com> <54D0EDE8.40207@linaro.org> <1424940642.25006.60.camel@chaos.site> In-Reply-To: <1424940642.25006.60.camel@chaos.site> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>>> Signed-off-by: Ivan Khoronzhuk >>> 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