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

[Laszlo Ersek]

On 08/05/15 19:35, Peter Maydell wrote:
> On 5 August 2015 at 18:16, Laszlo Ersek <address@hidden> 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).

See also CARD-1779.

Also, guest userspace is not QEMU's problem :)

Thanks
Laszlo