Re: [Qemu-devel] [ARM SMBIOS V2 PATCH 6/6] smbios: implement smbios support for mach-virt

[Laszlo Ersek <lersek@redhat.com>]

comments below

On 08/06/15 19:15, Wei Huang wrote:
> This patch generates smbios tables for ARM mach-virt. Also add
> CONFIG_SMBIOS=y for ARM default config.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  default-configs/arm-softmmu.mak  |  1 +
>  hw/arm/virt.c                    | 27 +++++++++++++++++++++++++++
>  include/hw/arm/virt-acpi-build.h |  1 +
>  3 files changed, 29 insertions(+)
> 
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 74f1db3..99b41e9 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -102,3 +102,4 @@ CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
>  CONFIG_ACPI=y
> +CONFIG_SMBIOS=y
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 4846892..d46e815 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -48,6 +48,7 @@
>  #include "hw/arm/sysbus-fdt.h"
>  #include "hw/platform-bus.h"
>  #include "hw/arm/fdt.h"
> +#include "hw/smbios/smbios.h"
>  
>  /* Number of external interrupt lines to configure the GIC with */
>  #define NUM_IRQS 256
> @@ -780,12 +781,37 @@ static void *machvirt_dtb(const struct arm_boot_info *binfo, int *fdt_size)
>      return board->fdt;
>  }
>  
> +static void virt_build_smbios(VirtGuestInfo *guest_info)
> +{
> +    FWCfgState *fw_cfg = guest_info->fw_cfg;
> +    uint8_t *smbios_tables, *smbios_anchor;
> +    size_t smbios_tables_len, smbios_anchor_len;
> +
> +    if (!fw_cfg)
> +        return;
> +
> +    smbios_set_defaults("QEMU", "QEMU Virtual Machine",
> +                        "1.0", false, false, SMBIOS_ENTRY_POINT_30);

I think the fifth parameter ("uuid_encoded") is incorrect. Please see
commit caad057b, and "7.2.1 System — UUID" in the SMBIOS 3.0 spec.

I didn't specifically verify this earlier, but I have now. It's not
right. According to the spec,

  The UUID {00112233-4455-6677-8899-AABBCCDDEEFF} would thus be
  represented as:

  33 22 11 00 55 44 77 66 88 99 AA BB CC DD EE FF.

However, the SMBIOSVIEW command in the UEFI shell returns the following
raw hexdump, when

  -uuid 00112233-4455-6677-8899-AABBCCDDEEFF

is passed on the command line:

Dump Uuid
size=16:
00000000: 00 11 22 33 44 55 66 77-88 99 AA BB CC DD EE FF
*.."3DUfw........*

Otherwise the patch looks fine.

Thanks
Laszlo

> +
> +    smbios_get_tables(NULL, 0, guest_info->ram_size,
> +                      &smbios_tables, &smbios_tables_len,
> +                      &smbios_anchor, &smbios_anchor_len);
> +
> +    if (smbios_anchor) {
> +        fw_cfg_add_file(fw_cfg, "etc/smbios/smbios-tables",
> +                        smbios_tables, smbios_tables_len);
> +        fw_cfg_add_file(fw_cfg, "etc/smbios/smbios-anchor",
> +                        smbios_anchor, smbios_anchor_len);
> +    }
> +}
> +
>  static
>  void virt_guest_info_machine_done(Notifier *notifier, void *data)
>  {
>      VirtGuestInfoState *guest_info_state = container_of(notifier,
>                                                VirtGuestInfoState, machine_done);
>      virt_acpi_setup(&guest_info_state->info);
> +    virt_build_smbios(&guest_info_state->info);
>  }
>  
>  static void machvirt_init(MachineState *machine)
> @@ -894,6 +920,7 @@ static void machvirt_init(MachineState *machine)
>  
>      guest_info->smp_cpus = smp_cpus;
>      guest_info->fw_cfg = fw_cfg_find();
> +    guest_info->ram_size = machine->ram_size;
>      guest_info->memmap = vbi->memmap;
>      guest_info->irqmap = vbi->irqmap;
>      guest_info_state->machine_done.notify = virt_guest_info_machine_done;
> diff --git a/include/hw/arm/virt-acpi-build.h b/include/hw/arm/virt-acpi-build.h
> index 04f174d..ef274c6 100644
> --- a/include/hw/arm/virt-acpi-build.h
> +++ b/include/hw/arm/virt-acpi-build.h
> @@ -29,6 +29,7 @@
>  typedef struct VirtGuestInfo {
>      int smp_cpus;
>      FWCfgState *fw_cfg;
> +    ram_addr_t ram_size;
>      const MemMapEntry *memmap;
>      const int *irqmap;
>  } VirtGuestInfo;
>