Re: [Qemu-devel] [ARM SMBIOS V1 PATCH 0/6] SMBIOS Support for ARM

[Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>]

Hi Laszlo

On 05.08.15 21:35, Laszlo Ersek wrote:
> On 08/05/15 19:35, Peter Maydell wrote:
>> On 5 August 2015 at 18:16, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 07/28/15 08:00, Wei Huang wrote:
>>>> SMBIOS tables present userful system hardware info to management
>>>> applications, such as DMI tools. Even though SMBIOS was originally
>>>> developed for Intel x86, it has been extended to both Itanium and
>>>> ARM (32bit & 64bit). More and more ARM server releases, such as
>>>> RHEL Server for ARM, start to integrate support for SMBIOS.
>>>>
>>>> This patchset is intendted to provid SMBIOS tables for ARM mach-virt
>>>> machine. The SMBIOS tables are created and stored in fw_cfg, relying on
>>>> OVMF (AAVMF) to parse/present SMBIOS entry.
>>>>
>>>> RFC version have been tested by Laszlo using his customized version of
>>>> AAVMF. We were able to detect SMBIOS 2.8 tables using dmidecode inside
>>>> an AArch64 guest VM. Moving forward, it is better to support SMBIOS 3.0
>>>> for ARM guest VM. This new version (V1) integrates SMBIOS 3.0 support
>>>> for ARM mach-virt. I have tested this version by forcing SMBIOS 2.1
>>>> format (i.e. passing SMBIOS_21_ENTRY_POINT to smbios_set_defaults()).
>>>> SMBIOS 3.0 hasn't been tested yet as it requires AAVMF to install 3.0 entry.
>>>>
>>>> RFC->V1:
>>>>   * Add SMBIOS 3.0 support for buidling SMBIOS
>>>>   * Switch from SMBIOS 2.1 to 3.0 for ARM mach-virt
>>>>   * RFC version Tested-by Laszlo Ersek and Acked-by Gabriel Somlo
>>>>
>>>> Thanks,
>>>> -Wei
>>>>
>>>> Wei Huang (6):
>>>>    smbios: extract x86 smbios building code into a function
>>>>    smbios: remove dependency on x86 e820 tables
>>>>    smbios: pass ram size as a parameter to build smbios tables
>>>>    smbios: move smbios code into a common folder
>>>>    smbios: add smbios 3.0 support
>>>>    smbios: implement smbios support for mach-virt
>>>>
>>>>   arch_init.c                          |  2 +-
>>>>   default-configs/arm-softmmu.mak      |  1 +
>>>>   default-configs/i386-softmmu.mak     |  1 +
>>>>   default-configs/x86_64-softmmu.mak   |  1 +
>>>>   hw/Makefile.objs                     |  1 +
>>>>   hw/arm/virt.c                        | 24 +++++++++
>>>>   hw/i386/Makefile.objs                |  2 +-
>>>>   hw/i386/pc.c                         | 56 ++++++++++++++-------
>>>>   hw/i386/pc_piix.c                    |  5 +-
>>>>   hw/i386/pc_q35.c                     |  5 +-
>>>>   hw/smbios/Makefile.objs              |  1 +
>>>>   hw/{i386 => smbios}/smbios.c         | 96 +++++++++++++++++++++++-------------
>>>>   include/hw/arm/virt-acpi-build.h     |  1 +
>>>>   include/hw/{i386 => smbios}/smbios.h | 42 ++++++++++++++--
>>>>   tests/bios-tables-test.c             |  2 +-
>>>>   vl.c                                 |  2 +-
>>>>   16 files changed, 179 insertions(+), 63 deletions(-)
>>>>   create mode 100644 hw/smbios/Makefile.objs
>>>>   rename hw/{i386 => smbios}/smbios.c (93%)
>>>>   rename include/hw/{i386 => smbios}/smbios.h (84%)
>>>>
>>>
>>> I was hoping there would be a focused review from the subsystem
>>> maintainers / feature owners for this patchset. Thus far only Shannon
>>> commented on the series, plus I tested it and reported a small bug (with
>>> a fix).
>>>
>>> Peter: if I review this series (and version 2 that Wei is already
>>> planning to post, in order to address the notes above, plus anything
>>> that further review might turn up), will my review suffice for you to
>>> apply this series (after 2.4 is out)?
>>
>> Maybe. I haven't looked at the series at all, because it fell
>> under "not for 2.4 and not something I know enough about to
>> easily and quickly review" (and besides 5 out of 6 patches are
>> not ARM-related but just about refactoring the x86 code).
>>
>> What is SMBIOS supposed to provide for ARM virt anyway?
>
> No clue. It is dictated by the most recent SMBIOS (3.0) spec. The actual
> contents is dictated by asset management needs ("run dmidecode, look at
> this and that in the output"). So this series should not be reviewed
> primarily for SMBIOS payload, but for correctness of refactoring, and
> well-formedness of the new tables. At least the latter point seems to be
> confirmed by testing already.
>
>> I would have expected all the information a guest needs
>> to be in the dtb or ACPI tables...
>
> SMBIOS is usually not needed by the guest OS or the firmware. The spec says,
>
>    [...] the System Management BIOS Reference Specification addresses
>    how motherboard and system vendors present management information
>    about their products in a standard format by extending the BIOS
>    interface on Intel architecture systems. The information is intended
>    to allow generic instrumentation to deliver this data to management
>    applications that use CIM (the WBEM data model) or direct access and
>    eliminates the need for error prone operations such as probing system
>    hardware for presence detection. [...]
>
> (The Intel-specificity in the Introduction that I quoted is certainly a
> bug in the spec, because under 1 Scope | 1.1 Supported processor
> architectures, Aarch32 and Aarch64 too are listed.)
>
> As far as I know (although I have no hard evidence), SMBIOS 3.0 was
> brought about by ARM platform needs. Aarch64 hardware that I have access
> to seems to expose SMBIOS 3.0 indeed, therefore we should do the same in
> qemu.
>
>> Is support for this all in the mainline kernel yet?
>
> Yes, it is. See (minimally)
>
> $ git log --reverse fc43026278^.. -- drivers/firmware/dmi*
>
> There are patches for arch/arm64/ and drivers/firmware/efi/ too (Ard
> will know better, he wrote at least a few of them).
>
> The userspace tools are in a more messy state AFAICT. For example I'm
> unable to locate a *git* repository for upstream dmidecode. But, Ivan
> and Roy should know better (Cc'd).

There is no git repo for dmidecode.
Only CVS: http://savannah.nongnu.org/cvs/?group=dmidecode
AFAIK, It supports SMBIOSv3 already.

+ Jean Delvare, who is creator of dmidecode, as I know.

>
> See also CARD-1779.
>
> Also, guest userspace is not QEMU's problem :)
>
> Thanks
> Laszlo
>

-- 
Regards,
Ivan Khoronzhuk