[Qemu-devel] [ARM SMBIOS V2 PATCH 0/6] SMBIOS Support for ARM

[Qemu-devel] [ARM SMBIOS V2 PATCH 0/6] SMBIOS Support for ARM

[Wei Huang]

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 (V2) integrates SMBIOS 3.0 support
for ARM mach-virt. I have tested this version using a customized AAVMF
created by Laszlo, who has submitted his patches to OVMF mailing list.

V1->V2:
 * Add NULL checking for fw_cfg (Shannon Zhao)
 * Init 3.0 entry point table max size to smbios_tables_len (Laszlo)
 * Minor re-arrangement of smbios.h layout with function headers to the bottom
 * Validated SMBIOS 3.0 tables with a customized AAVMF created by Laszlo

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                        | 27 ++++++++++
 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 | 56 ++++++++++++++++-----
 tests/bios-tables-test.c             |  2 +-
 vl.c                                 |  2 +-
 16 files changed, 188 insertions(+), 71 deletions(-)
 create mode 100644 hw/smbios/Makefile.objs
 rename hw/{i386 => smbios}/smbios.c (93%)
 rename include/hw/{i386 => smbios}/smbios.h (83%)

-- 
1.8.3.1

[Qemu-devel] [ARM SMBIOS V2 PATCH 1/6] smbios: extract x86 smbios building code into a function

[Wei Huang]

This patch extracts out the procedure of buidling x86 SMBIOS tables
into a dedicated function.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 hw/i386/pc.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 7661ea9..00e45f3 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -718,11 +718,30 @@ static unsigned int pc_apic_id_limit(unsigned int max_cpus)
     return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
 }
 
-static FWCfgState *bochs_bios_init(void)
+static void pc_build_smbios(FWCfgState *fw_cfg)
 {
-    FWCfgState *fw_cfg;
     uint8_t *smbios_tables, *smbios_anchor;
     size_t smbios_tables_len, smbios_anchor_len;
+
+    smbios_tables = smbios_get_table_legacy(&smbios_tables_len);
+    if (smbios_tables) {
+        fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
+                         smbios_tables, smbios_tables_len);
+    }
+
+    smbios_get_tables(&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 FWCfgState *bochs_bios_init(void)
+{
+    FWCfgState *fw_cfg;
     uint64_t *numa_fw_cfg;
     int i, j;
     unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
@@ -748,20 +767,7 @@ static FWCfgState *bochs_bios_init(void)
                      acpi_tables, acpi_tables_len);
     fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
 
-    smbios_tables = smbios_get_table_legacy(&smbios_tables_len);
-    if (smbios_tables) {
-        fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
-                         smbios_tables, smbios_tables_len);
-    }
-
-    smbios_get_tables(&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);
-    }
+    pc_build_smbios(fw_cfg);
 
     fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
                      &e820_reserve, sizeof(e820_reserve));
-- 
1.8.3.1

Re: [Qemu-devel] [ARM SMBIOS V2 PATCH 1/6] smbios: extract x86 smbios building code into a function

[Laszlo Ersek]

On 08/06/15 19:14, Wei Huang wrote:
> This patch extracts out the procedure of buidling x86 SMBIOS tables
> into a dedicated function.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/i386/pc.c | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 7661ea9..00e45f3 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -718,11 +718,30 @@ static unsigned int pc_apic_id_limit(unsigned int max_cpus)
>      return x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
>  }
>  
> -static FWCfgState *bochs_bios_init(void)
> +static void pc_build_smbios(FWCfgState *fw_cfg)
>  {
> -    FWCfgState *fw_cfg;
>      uint8_t *smbios_tables, *smbios_anchor;
>      size_t smbios_tables_len, smbios_anchor_len;
> +
> +    smbios_tables = smbios_get_table_legacy(&smbios_tables_len);
> +    if (smbios_tables) {
> +        fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
> +                         smbios_tables, smbios_tables_len);
> +    }
> +
> +    smbios_get_tables(&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 FWCfgState *bochs_bios_init(void)
> +{
> +    FWCfgState *fw_cfg;
>      uint64_t *numa_fw_cfg;
>      int i, j;
>      unsigned int apic_id_limit = pc_apic_id_limit(max_cpus);
> @@ -748,20 +767,7 @@ static FWCfgState *bochs_bios_init(void)
>                       acpi_tables, acpi_tables_len);
>      fw_cfg_add_i32(fw_cfg, FW_CFG_IRQ0_OVERRIDE, kvm_allows_irq0_override());
>  
> -    smbios_tables = smbios_get_table_legacy(&smbios_tables_len);
> -    if (smbios_tables) {
> -        fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES,
> -                         smbios_tables, smbios_tables_len);
> -    }
> -
> -    smbios_get_tables(&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);
> -    }
> +    pc_build_smbios(fw_cfg);
>  
>      fw_cfg_add_bytes(fw_cfg, FW_CFG_E820_TABLE,
>                       &e820_reserve, sizeof(e820_reserve));
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [ARM SMBIOS V2 PATCH 2/6] smbios: remove dependency on x86 e820 tables

[Wei Huang]

Current smbios builds type 19 table from e820, which is x86 specific.
This patch removes smbios' dependency on e820 by passing an array
of memory area to smbios_get_tables().

Signed-off-by: Wei Huang <wei@redhat.com>
---
 hw/i386/pc.c             | 18 +++++++++++++++++-
 hw/i386/smbios.c         | 14 +++++++-------
 include/hw/i386/smbios.h | 10 +++++++++-
 3 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 00e45f3..34e9133 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -722,6 +722,8 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
 {
     uint8_t *smbios_tables, *smbios_anchor;
     size_t smbios_tables_len, smbios_anchor_len;
+    struct smbios_phys_mem_area *mem_array;
+    unsigned i, array_count;
 
     smbios_tables = smbios_get_table_legacy(&smbios_tables_len);
     if (smbios_tables) {
@@ -729,8 +731,22 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
                          smbios_tables, smbios_tables_len);
     }
 
-    smbios_get_tables(&smbios_tables, &smbios_tables_len,
+    /* build the array of physical mem area from e820 table */
+    mem_array = g_malloc0(sizeof(*mem_array) * e820_get_num_entries());
+    for (i = 0, array_count = 0; i < e820_get_num_entries(); i++) {
+        uint64_t addr, len;
+
+        if (e820_get_entry(i, E820_RAM, &addr, &len)) {
+            mem_array[array_count].address = addr;
+            mem_array[array_count].length = len;
+            array_count++;
+        }
+    }
+    smbios_get_tables(mem_array, array_count,
+                      &smbios_tables, &smbios_tables_len,
                       &smbios_anchor, &smbios_anchor_len);
+    g_free(mem_array);
+
     if (smbios_anchor) {
         fw_cfg_add_file(fw_cfg, "etc/smbios/smbios-tables",
                         smbios_tables, smbios_tables_len);
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 1341e02..6f715c6 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -831,10 +831,12 @@ static void smbios_entry_point_setup(void)
     ep.structure_table_address = cpu_to_le32(0);
 }
 
-void smbios_get_tables(uint8_t **tables, size_t *tables_len,
+void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
+                       const unsigned int mem_array_size,
+                       uint8_t **tables, size_t *tables_len,
                        uint8_t **anchor, size_t *anchor_len)
 {
-    unsigned i, dimm_cnt, instance;
+    unsigned i, dimm_cnt;
 
     if (smbios_legacy) {
         *tables = *anchor = NULL;
@@ -867,11 +869,9 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len,
             smbios_build_type_17_table(i, GET_DIMM_SZ);
         }
 
-        for (i = 0, instance = 0; i < e820_get_num_entries(); i++) {
-            uint64_t address, length;
-            if (e820_get_entry(i, E820_RAM, &address, &length)) {
-                smbios_build_type_19_table(instance++, address, length);
-            }
+        for (i = 0; i < mem_array_size; i++) {
+            smbios_build_type_19_table(i, mem_array[i].address,
+                                       mem_array[i].length);
         }
 
         smbios_build_type_32_table();
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index d2850be..4269aab 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -17,13 +17,21 @@
 
 #define SMBIOS_MAX_TYPE 127
 
+/* memory area description, used by type 19 table */
+struct smbios_phys_mem_area {
+    uint64_t address;
+    uint64_t length;
+};
+
 void smbios_entry_add(QemuOpts *opts);
 void smbios_set_cpuid(uint32_t version, uint32_t features);
 void smbios_set_defaults(const char *manufacturer, const char *product,
                          const char *version, bool legacy_mode,
                          bool uuid_encoded);
 uint8_t *smbios_get_table_legacy(size_t *length);
-void smbios_get_tables(uint8_t **tables, size_t *tables_len,
+void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
+                       const unsigned int mem_array_size,
+                       uint8_t **tables, size_t *tables_len,
                        uint8_t **anchor, size_t *anchor_len);
 
 /*
-- 
1.8.3.1

Re: [Qemu-devel] [ARM SMBIOS V2 PATCH 2/6] smbios: remove dependency on x86 e820 tables

[Laszlo Ersek]

On 08/06/15 19:14, Wei Huang wrote:
> Current smbios builds type 19 table from e820, which is x86 specific.
> This patch removes smbios' dependency on e820 by passing an array
> of memory area to smbios_get_tables().
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/i386/pc.c             | 18 +++++++++++++++++-
>  hw/i386/smbios.c         | 14 +++++++-------
>  include/hw/i386/smbios.h | 10 +++++++++-
>  3 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 00e45f3..34e9133 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -722,6 +722,8 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
>  {
>      uint8_t *smbios_tables, *smbios_anchor;
>      size_t smbios_tables_len, smbios_anchor_len;
> +    struct smbios_phys_mem_area *mem_array;
> +    unsigned i, array_count;
>  
>      smbios_tables = smbios_get_table_legacy(&smbios_tables_len);
>      if (smbios_tables) {
> @@ -729,8 +731,22 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
>                           smbios_tables, smbios_tables_len);
>      }
>  
> -    smbios_get_tables(&smbios_tables, &smbios_tables_len,
> +    /* build the array of physical mem area from e820 table */
> +    mem_array = g_malloc0(sizeof(*mem_array) * e820_get_num_entries());
> +    for (i = 0, array_count = 0; i < e820_get_num_entries(); i++) {
> +        uint64_t addr, len;
> +
> +        if (e820_get_entry(i, E820_RAM, &addr, &len)) {
> +            mem_array[array_count].address = addr;
> +            mem_array[array_count].length = len;
> +            array_count++;
> +        }
> +    }
> +    smbios_get_tables(mem_array, array_count,
> +                      &smbios_tables, &smbios_tables_len,
>                        &smbios_anchor, &smbios_anchor_len);
> +    g_free(mem_array);
> +
>      if (smbios_anchor) {
>          fw_cfg_add_file(fw_cfg, "etc/smbios/smbios-tables",
>                          smbios_tables, smbios_tables_len);
> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
> index 1341e02..6f715c6 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/i386/smbios.c
> @@ -831,10 +831,12 @@ static void smbios_entry_point_setup(void)
>      ep.structure_table_address = cpu_to_le32(0);
>  }
>  
> -void smbios_get_tables(uint8_t **tables, size_t *tables_len,
> +void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> +                       const unsigned int mem_array_size,
> +                       uint8_t **tables, size_t *tables_len,
>                         uint8_t **anchor, size_t *anchor_len)
>  {
> -    unsigned i, dimm_cnt, instance;
> +    unsigned i, dimm_cnt;
>  
>      if (smbios_legacy) {
>          *tables = *anchor = NULL;
> @@ -867,11 +869,9 @@ void smbios_get_tables(uint8_t **tables, size_t *tables_len,
>              smbios_build_type_17_table(i, GET_DIMM_SZ);
>          }
>  
> -        for (i = 0, instance = 0; i < e820_get_num_entries(); i++) {
> -            uint64_t address, length;
> -            if (e820_get_entry(i, E820_RAM, &address, &length)) {
> -                smbios_build_type_19_table(instance++, address, length);
> -            }
> +        for (i = 0; i < mem_array_size; i++) {
> +            smbios_build_type_19_table(i, mem_array[i].address,
> +                                       mem_array[i].length);
>          }
>  
>          smbios_build_type_32_table();
> diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
> index d2850be..4269aab 100644
> --- a/include/hw/i386/smbios.h
> +++ b/include/hw/i386/smbios.h
> @@ -17,13 +17,21 @@
>  
>  #define SMBIOS_MAX_TYPE 127
>  
> +/* memory area description, used by type 19 table */
> +struct smbios_phys_mem_area {
> +    uint64_t address;
> +    uint64_t length;
> +};

In general we'd also introduce a CamelCase typedefs here, but
"include/hw/i386/smbios.h" does not follow that style at all. So this
structure type definition is consistent with the existent style in that
file.

> +
>  void smbios_entry_add(QemuOpts *opts);
>  void smbios_set_cpuid(uint32_t version, uint32_t features);
>  void smbios_set_defaults(const char *manufacturer, const char *product,
>                           const char *version, bool legacy_mode,
>                           bool uuid_encoded);
>  uint8_t *smbios_get_table_legacy(size_t *length);
> -void smbios_get_tables(uint8_t **tables, size_t *tables_len,
> +void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> +                       const unsigned int mem_array_size,
> +                       uint8_t **tables, size_t *tables_len,
>                         uint8_t **anchor, size_t *anchor_len);
>  
>  /*
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [ARM SMBIOS V2 PATCH 3/6] smbios: pass ram size as a parameter to build smbios tables

[Wei Huang]

This patch adds a new parameter, mem_size, to smbios_get_tables()
function. This step is required to make smbios code architect-independent.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 hw/i386/pc.c             | 2 +-
 hw/i386/smbios.c         | 8 ++++----
 include/hw/i386/smbios.h | 2 ++
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 34e9133..944d5b1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -742,7 +742,7 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
             array_count++;
         }
     }
-    smbios_get_tables(mem_array, array_count,
+    smbios_get_tables(mem_array, array_count, ram_size,
                       &smbios_tables, &smbios_tables_len,
                       &smbios_anchor, &smbios_anchor_len);
     g_free(mem_array);
diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
index 6f715c6..12aee90 100644
--- a/hw/i386/smbios.c
+++ b/hw/i386/smbios.c
@@ -19,10 +19,9 @@
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/cpus.h"
-#include "hw/i386/pc.h"
 #include "hw/i386/smbios.h"
 #include "hw/loader.h"
-
+#include "exec/cpu-common.h"
 
 /* legacy structures and constants for <= 2.0 machines */
 struct smbios_header {
@@ -649,7 +648,7 @@ static void smbios_build_type_4_table(unsigned instance)
 
 #define MAX_T16_STD_SZ 0x80000000 /* 2T in Kilobytes */
 
-static void smbios_build_type_16_table(unsigned dimm_cnt)
+static void smbios_build_type_16_table(unsigned dimm_cnt, ram_addr_t ram_size)
 {
     uint64_t size_kb;
 
@@ -833,6 +832,7 @@ static void smbios_entry_point_setup(void)
 
 void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
                        const unsigned int mem_array_size,
+                       const ram_addr_t ram_size,
                        uint8_t **tables, size_t *tables_len,
                        uint8_t **anchor, size_t *anchor_len)
 {
@@ -863,7 +863,7 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
 
         dimm_cnt = QEMU_ALIGN_UP(ram_size, MAX_DIMM_SZ) / MAX_DIMM_SZ;
 
-        smbios_build_type_16_table(dimm_cnt);
+        smbios_build_type_16_table(dimm_cnt, ram_size);
 
         for (i = 0; i < dimm_cnt; i++) {
             smbios_build_type_17_table(i, GET_DIMM_SZ);
diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
index 4269aab..e727233 100644
--- a/include/hw/i386/smbios.h
+++ b/include/hw/i386/smbios.h
@@ -14,6 +14,7 @@
  */
 
 #include "qemu/option.h"
+#include "exec/cpu-common.h"
 
 #define SMBIOS_MAX_TYPE 127
 
@@ -31,6 +32,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
 uint8_t *smbios_get_table_legacy(size_t *length);
 void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
                        const unsigned int mem_array_size,
+                       const ram_addr_t ram_size,
                        uint8_t **tables, size_t *tables_len,
                        uint8_t **anchor, size_t *anchor_len);
 
-- 
1.8.3.1

Re: [Qemu-devel] [ARM SMBIOS V2 PATCH 3/6] smbios: pass ram size as a parameter to build smbios tables

[Laszlo Ersek]

On 08/06/15 19:14, Wei Huang wrote:
> This patch adds a new parameter, mem_size, to smbios_get_tables()
> function. This step is required to make smbios code architect-independent.

(1) "architecture"-independent

> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/i386/pc.c             | 2 +-
>  hw/i386/smbios.c         | 8 ++++----
>  include/hw/i386/smbios.h | 2 ++
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 34e9133..944d5b1 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -742,7 +742,7 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
>              array_count++;
>          }
>      }
> -    smbios_get_tables(mem_array, array_count,
> +    smbios_get_tables(mem_array, array_count, ram_size,
>                        &smbios_tables, &smbios_tables_len,
>                        &smbios_anchor, &smbios_anchor_len);
>      g_free(mem_array);
> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
> index 6f715c6..12aee90 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/i386/smbios.c
> @@ -19,10 +19,9 @@
>  #include "qemu/error-report.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpus.h"
> -#include "hw/i386/pc.h"
>  #include "hw/i386/smbios.h"
>  #include "hw/loader.h"
> -
> +#include "exec/cpu-common.h"
>  
>  /* legacy structures and constants for <= 2.0 machines */
>  struct smbios_header {
> @@ -649,7 +648,7 @@ static void smbios_build_type_4_table(unsigned instance)
>  
>  #define MAX_T16_STD_SZ 0x80000000 /* 2T in Kilobytes */
>  
> -static void smbios_build_type_16_table(unsigned dimm_cnt)
> +static void smbios_build_type_16_table(unsigned dimm_cnt, ram_addr_t ram_size)
>  {
>      uint64_t size_kb;
>  
> @@ -833,6 +832,7 @@ static void smbios_entry_point_setup(void)
>  
>  void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>                         const unsigned int mem_array_size,
> +                       const ram_addr_t ram_size,
>                         uint8_t **tables, size_t *tables_len,
>                         uint8_t **anchor, size_t *anchor_len)
>  {
> @@ -863,7 +863,7 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>  
>          dimm_cnt = QEMU_ALIGN_UP(ram_size, MAX_DIMM_SZ) / MAX_DIMM_SZ;
>  
> -        smbios_build_type_16_table(dimm_cnt);
> +        smbios_build_type_16_table(dimm_cnt, ram_size);
>  
>          for (i = 0; i < dimm_cnt; i++) {
>              smbios_build_type_17_table(i, GET_DIMM_SZ);
> diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
> index 4269aab..e727233 100644
> --- a/include/hw/i386/smbios.h
> +++ b/include/hw/i386/smbios.h
> @@ -14,6 +14,7 @@
>   */
>  
>  #include "qemu/option.h"
> +#include "exec/cpu-common.h"
>  
>  #define SMBIOS_MAX_TYPE 127
>  
> @@ -31,6 +32,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
>  uint8_t *smbios_get_table_legacy(size_t *length);
>  void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>                         const unsigned int mem_array_size,
> +                       const ram_addr_t ram_size,
>                         uint8_t **tables, size_t *tables_len,
>                         uint8_t **anchor, size_t *anchor_len);
>  
> 

(2) I think I understand how this patch works, but I find it confusing.
I'd recommend to introduce the "ram_size" function parameters with a
different name, so that they don't needlessly shadow the "ram_size"
global variable.

The most confusing part is that the patch *relies* on this shadowing, in
the smbios_build_type_16_table() and smbios_get_tables() functions.

Once the parameters are renamed, accesses to them will have to be
patched as well, in these two functions. That will make for a larger,
but more understandable patch.

These changes are trivial enough that, if you address (1) and (2), you
can add

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

to the next version at once.

Thanks
Laszlo

Re: [Qemu-devel] [ARM SMBIOS V2 PATCH 3/6] smbios: pass ram size as a parameter to build smbios tables

[Wei Huang]

On 08/07/2015 11:41 AM, Laszlo Ersek wrote:
> On 08/06/15 19:14, Wei Huang wrote:
>> This patch adds a new parameter, mem_size, to smbios_get_tables()
>> function. This step is required to make smbios code architect-independent.
> 
> (1) "architecture"-independent
It was pointed out before; but slip into crack. I will address it in
next version.

> 
>>
>> Signed-off-by: Wei Huang <wei@redhat.com>
>> ---
>>  hw/i386/pc.c             | 2 +-
>>  hw/i386/smbios.c         | 8 ++++----
>>  include/hw/i386/smbios.h | 2 ++
>>  3 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index 34e9133..944d5b1 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -742,7 +742,7 @@ static void pc_build_smbios(FWCfgState *fw_cfg)
>>              array_count++;
>>          }
>>      }
>> -    smbios_get_tables(mem_array, array_count,
>> +    smbios_get_tables(mem_array, array_count, ram_size,
>>                        &smbios_tables, &smbios_tables_len,
>>                        &smbios_anchor, &smbios_anchor_len);
>>      g_free(mem_array);
>> diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c
>> index 6f715c6..12aee90 100644
>> --- a/hw/i386/smbios.c
>> +++ b/hw/i386/smbios.c
>> @@ -19,10 +19,9 @@
>>  #include "qemu/error-report.h"
>>  #include "sysemu/sysemu.h"
>>  #include "sysemu/cpus.h"
>> -#include "hw/i386/pc.h"
>>  #include "hw/i386/smbios.h"
>>  #include "hw/loader.h"
>> -
>> +#include "exec/cpu-common.h"
>>  
>>  /* legacy structures and constants for <= 2.0 machines */
>>  struct smbios_header {
>> @@ -649,7 +648,7 @@ static void smbios_build_type_4_table(unsigned instance)
>>  
>>  #define MAX_T16_STD_SZ 0x80000000 /* 2T in Kilobytes */
>>  
>> -static void smbios_build_type_16_table(unsigned dimm_cnt)
>> +static void smbios_build_type_16_table(unsigned dimm_cnt, ram_addr_t ram_size)
>>  {
>>      uint64_t size_kb;
>>  
>> @@ -833,6 +832,7 @@ static void smbios_entry_point_setup(void)
>>  
>>  void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>>                         const unsigned int mem_array_size,
>> +                       const ram_addr_t ram_size,
>>                         uint8_t **tables, size_t *tables_len,
>>                         uint8_t **anchor, size_t *anchor_len)
>>  {
>> @@ -863,7 +863,7 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>>  
>>          dimm_cnt = QEMU_ALIGN_UP(ram_size, MAX_DIMM_SZ) / MAX_DIMM_SZ;
>>  
>> -        smbios_build_type_16_table(dimm_cnt);
>> +        smbios_build_type_16_table(dimm_cnt, ram_size);
>>  
>>          for (i = 0; i < dimm_cnt; i++) {
>>              smbios_build_type_17_table(i, GET_DIMM_SZ);
>> diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h
>> index 4269aab..e727233 100644
>> --- a/include/hw/i386/smbios.h
>> +++ b/include/hw/i386/smbios.h
>> @@ -14,6 +14,7 @@
>>   */
>>  
>>  #include "qemu/option.h"
>> +#include "exec/cpu-common.h"
>>  
>>  #define SMBIOS_MAX_TYPE 127
>>  
>> @@ -31,6 +32,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
>>  uint8_t *smbios_get_table_legacy(size_t *length);
>>  void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>>                         const unsigned int mem_array_size,
>> +                       const ram_addr_t ram_size,
>>                         uint8_t **tables, size_t *tables_len,
>>                         uint8_t **anchor, size_t *anchor_len);
>>  
>>
> 
> (2) I think I understand how this patch works, but I find it confusing.
> I'd recommend to introduce the "ram_size" function parameters with a
> different name, so that they don't needlessly shadow the "ram_size"
> global variable.
> 
> The most confusing part is that the patch *relies* on this shadowing, in
> the smbios_build_type_16_table() and smbios_get_tables() functions.
> 
> Once the parameters are renamed, accesses to them will have to be
> patched as well, in these two functions. That will make for a larger,
> but more understandable patch.
> 
> These changes are trivial enough that, if you address (1) and (2), you
> can add
OK. Will do.

> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> to the next version at once.
> 
> Thanks
> Laszlo
> 

[Qemu-devel] [ARM SMBIOS V2 PATCH 4/6] smbios: move smbios code into a common folder

[Wei Huang]

To share smbios among different architectures, this patch moves SMBIOS
code (smbios.c and smbios.h) from x86 specific folders into new
hw/smbios directories. As a result, CONFIG_SMBIOS=y is defined in
x86 default config files.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 arch_init.c                          | 2 +-
 default-configs/i386-softmmu.mak     | 1 +
 default-configs/x86_64-softmmu.mak   | 1 +
 hw/Makefile.objs                     | 1 +
 hw/i386/Makefile.objs                | 2 +-
 hw/i386/pc.c                         | 2 +-
 hw/i386/pc_piix.c                    | 2 +-
 hw/i386/pc_q35.c                     | 2 +-
 hw/smbios/Makefile.objs              | 1 +
 hw/{i386 => smbios}/smbios.c         | 2 +-
 include/hw/{i386 => smbios}/smbios.h | 0
 tests/bios-tables-test.c             | 2 +-
 vl.c                                 | 2 +-
 13 files changed, 12 insertions(+), 8 deletions(-)
 create mode 100644 hw/smbios/Makefile.objs
 rename hw/{i386 => smbios}/smbios.c (99%)
 rename include/hw/{i386 => smbios}/smbios.h (100%)

diff --git a/arch_init.c b/arch_init.c
index 725c638..38f5fb9 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -26,7 +26,7 @@
 #include "sysemu/arch_init.h"
 #include "hw/pci/pci.h"
 #include "hw/audio/audio.h"
-#include "hw/i386/smbios.h"
+#include "hw/smbios/smbios.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "qmp-commands.h"
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 48b5762..5eaafa1 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -49,3 +49,4 @@ CONFIG_MEM_HOTPLUG=y
 CONFIG_XIO3130=y
 CONFIG_IOH3420=y
 CONFIG_I82801B11=y
+CONFIG_SMBIOS=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 4962ed7..28e2099 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -50,3 +50,4 @@ CONFIG_MEM_HOTPLUG=y
 CONFIG_XIO3130=y
 CONFIG_IOH3420=y
 CONFIG_I82801B11=y
+CONFIG_SMBIOS=y
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 73afa41..7e7c241 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -31,6 +31,7 @@ devices-dirs-$(CONFIG_VIRTIO) += virtio/
 devices-dirs-$(CONFIG_SOFTMMU) += watchdog/
 devices-dirs-$(CONFIG_SOFTMMU) += xen/
 devices-dirs-$(CONFIG_MEM_HOTPLUG) += mem/
+devices-dirs-$(CONFIG_SMBIOS) += smbios/
 devices-dirs-y += core/
 common-obj-y += $(devices-dirs-y)
 obj-y += $(devices-dirs-y)
diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
index bd4f147..723a4d8 100644
--- a/hw/i386/Makefile.objs
+++ b/hw/i386/Makefile.objs
@@ -1,5 +1,5 @@
 obj-$(CONFIG_KVM) += kvm/
-obj-y += multiboot.o smbios.o
+obj-y += multiboot.o
 obj-y += pc.o pc_piix.o pc_q35.o
 obj-y += pc_sysfw.o
 obj-y += intel_iommu.o
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 944d5b1..80291fe 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -33,7 +33,7 @@
 #include "hw/pci/pci_bus.h"
 #include "hw/nvram/fw_cfg.h"
 #include "hw/timer/hpet.h"
-#include "hw/i386/smbios.h"
+#include "hw/smbios/smbios.h"
 #include "hw/loader.h"
 #include "elf.h"
 #include "multiboot.h"
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index a896624..653c710 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -28,7 +28,7 @@
 #include "hw/loader.h"
 #include "hw/i386/pc.h"
 #include "hw/i386/apic.h"
-#include "hw/i386/smbios.h"
+#include "hw/smbios/smbios.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_ids.h"
 #include "hw/usb.h"
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 974aead..d83df14 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -39,7 +39,7 @@
 #include "hw/pci-host/q35.h"
 #include "exec/address-spaces.h"
 #include "hw/i386/ich9.h"
-#include "hw/i386/smbios.h"
+#include "hw/smbios/smbios.h"
 #include "hw/ide/pci.h"
 #include "hw/ide/ahci.h"
 #include "hw/usb.h"
diff --git a/hw/smbios/Makefile.objs b/hw/smbios/Makefile.objs
new file mode 100644
index 0000000..f69a92f
--- /dev/null
+++ b/hw/smbios/Makefile.objs
@@ -0,0 +1 @@
+common-obj-$(CONFIG_SMBIOS) += smbios.o
diff --git a/hw/i386/smbios.c b/hw/smbios/smbios.c
similarity index 99%
rename from hw/i386/smbios.c
rename to hw/smbios/smbios.c
index 12aee90..08ba62a 100644
--- a/hw/i386/smbios.c
+++ b/hw/smbios/smbios.c
@@ -19,7 +19,7 @@
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/cpus.h"
-#include "hw/i386/smbios.h"
+#include "hw/smbios/smbios.h"
 #include "hw/loader.h"
 #include "exec/cpu-common.h"
 
diff --git a/include/hw/i386/smbios.h b/include/hw/smbios/smbios.h
similarity index 100%
rename from include/hw/i386/smbios.h
rename to include/hw/smbios/smbios.h
diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
index 0de1742..613867a 100644
--- a/tests/bios-tables-test.c
+++ b/tests/bios-tables-test.c
@@ -18,7 +18,7 @@
 #include "libqtest.h"
 #include "qemu/compiler.h"
 #include "hw/acpi/acpi-defs.h"
-#include "hw/i386/smbios.h"
+#include "hw/smbios/smbios.h"
 #include "qemu/bitmap.h"
 
 #define MACHINE_PC "pc"
diff --git a/vl.c b/vl.c
index 0adbbd6..584ca88 100644
--- a/vl.c
+++ b/vl.c
@@ -68,7 +68,7 @@ int main(int argc, char **argv)
 #include "hw/isa/isa.h"
 #include "hw/bt.h"
 #include "sysemu/watchdog.h"
-#include "hw/i386/smbios.h"
+#include "hw/smbios/smbios.h"
 #include "hw/xen/xen.h"
 #include "hw/qdev.h"
 #include "hw/loader.h"
-- 
1.8.3.1

Re: [Qemu-devel] [ARM SMBIOS V2 PATCH 4/6] smbios: move smbios code into a common folder

[Laszlo Ersek]

On 08/06/15 19:14, Wei Huang wrote:
> To share smbios among different architectures, this patch moves SMBIOS
> code (smbios.c and smbios.h) from x86 specific folders into new
> hw/smbios directories.

Makes sense to me; "acpi" is also under "hw".

> As a result, CONFIG_SMBIOS=y is defined in
> x86 default config files.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  arch_init.c                          | 2 +-
>  default-configs/i386-softmmu.mak     | 1 +
>  default-configs/x86_64-softmmu.mak   | 1 +
>  hw/Makefile.objs                     | 1 +
>  hw/i386/Makefile.objs                | 2 +-
>  hw/i386/pc.c                         | 2 +-
>  hw/i386/pc_piix.c                    | 2 +-
>  hw/i386/pc_q35.c                     | 2 +-
>  hw/smbios/Makefile.objs              | 1 +
>  hw/{i386 => smbios}/smbios.c         | 2 +-
>  include/hw/{i386 => smbios}/smbios.h | 0
>  tests/bios-tables-test.c             | 2 +-
>  vl.c                                 | 2 +-
>  13 files changed, 12 insertions(+), 8 deletions(-)
>  create mode 100644 hw/smbios/Makefile.objs
>  rename hw/{i386 => smbios}/smbios.c (99%)
>  rename include/hw/{i386 => smbios}/smbios.h (100%)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 725c638..38f5fb9 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -26,7 +26,7 @@
>  #include "sysemu/arch_init.h"
>  #include "hw/pci/pci.h"
>  #include "hw/audio/audio.h"
> -#include "hw/i386/smbios.h"
> +#include "hw/smbios/smbios.h"
>  #include "qemu/config-file.h"
>  #include "qemu/error-report.h"
>  #include "qmp-commands.h"
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index 48b5762..5eaafa1 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -49,3 +49,4 @@ CONFIG_MEM_HOTPLUG=y
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> +CONFIG_SMBIOS=y
> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> index 4962ed7..28e2099 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -50,3 +50,4 @@ CONFIG_MEM_HOTPLUG=y
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> +CONFIG_SMBIOS=y
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 73afa41..7e7c241 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -31,6 +31,7 @@ devices-dirs-$(CONFIG_VIRTIO) += virtio/
>  devices-dirs-$(CONFIG_SOFTMMU) += watchdog/
>  devices-dirs-$(CONFIG_SOFTMMU) += xen/
>  devices-dirs-$(CONFIG_MEM_HOTPLUG) += mem/
> +devices-dirs-$(CONFIG_SMBIOS) += smbios/
>  devices-dirs-y += core/
>  common-obj-y += $(devices-dirs-y)
>  obj-y += $(devices-dirs-y)
> diff --git a/hw/i386/Makefile.objs b/hw/i386/Makefile.objs
> index bd4f147..723a4d8 100644
> --- a/hw/i386/Makefile.objs
> +++ b/hw/i386/Makefile.objs
> @@ -1,5 +1,5 @@
>  obj-$(CONFIG_KVM) += kvm/
> -obj-y += multiboot.o smbios.o
> +obj-y += multiboot.o
>  obj-y += pc.o pc_piix.o pc_q35.o
>  obj-y += pc_sysfw.o
>  obj-y += intel_iommu.o
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 944d5b1..80291fe 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -33,7 +33,7 @@
>  #include "hw/pci/pci_bus.h"
>  #include "hw/nvram/fw_cfg.h"
>  #include "hw/timer/hpet.h"
> -#include "hw/i386/smbios.h"
> +#include "hw/smbios/smbios.h"
>  #include "hw/loader.h"
>  #include "elf.h"
>  #include "multiboot.h"
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index a896624..653c710 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -28,7 +28,7 @@
>  #include "hw/loader.h"
>  #include "hw/i386/pc.h"
>  #include "hw/i386/apic.h"
> -#include "hw/i386/smbios.h"
> +#include "hw/smbios/smbios.h"
>  #include "hw/pci/pci.h"
>  #include "hw/pci/pci_ids.h"
>  #include "hw/usb.h"
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 974aead..d83df14 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -39,7 +39,7 @@
>  #include "hw/pci-host/q35.h"
>  #include "exec/address-spaces.h"
>  #include "hw/i386/ich9.h"
> -#include "hw/i386/smbios.h"
> +#include "hw/smbios/smbios.h"
>  #include "hw/ide/pci.h"
>  #include "hw/ide/ahci.h"
>  #include "hw/usb.h"
> diff --git a/hw/smbios/Makefile.objs b/hw/smbios/Makefile.objs
> new file mode 100644
> index 0000000..f69a92f
> --- /dev/null
> +++ b/hw/smbios/Makefile.objs
> @@ -0,0 +1 @@
> +common-obj-$(CONFIG_SMBIOS) += smbios.o
> diff --git a/hw/i386/smbios.c b/hw/smbios/smbios.c
> similarity index 99%
> rename from hw/i386/smbios.c
> rename to hw/smbios/smbios.c
> index 12aee90..08ba62a 100644
> --- a/hw/i386/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -19,7 +19,7 @@
>  #include "qemu/error-report.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/cpus.h"
> -#include "hw/i386/smbios.h"
> +#include "hw/smbios/smbios.h"
>  #include "hw/loader.h"
>  #include "exec/cpu-common.h"
>  
> diff --git a/include/hw/i386/smbios.h b/include/hw/smbios/smbios.h
> similarity index 100%
> rename from include/hw/i386/smbios.h
> rename to include/hw/smbios/smbios.h
> diff --git a/tests/bios-tables-test.c b/tests/bios-tables-test.c
> index 0de1742..613867a 100644
> --- a/tests/bios-tables-test.c
> +++ b/tests/bios-tables-test.c
> @@ -18,7 +18,7 @@
>  #include "libqtest.h"
>  #include "qemu/compiler.h"
>  #include "hw/acpi/acpi-defs.h"
> -#include "hw/i386/smbios.h"
> +#include "hw/smbios/smbios.h"
>  #include "qemu/bitmap.h"
>  
>  #define MACHINE_PC "pc"
> diff --git a/vl.c b/vl.c
> index 0adbbd6..584ca88 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -68,7 +68,7 @@ int main(int argc, char **argv)
>  #include "hw/isa/isa.h"
>  #include "hw/bt.h"
>  #include "sysemu/watchdog.h"
> -#include "hw/i386/smbios.h"
> +#include "hw/smbios/smbios.h"
>  #include "hw/xen/xen.h"
>  #include "hw/qdev.h"
>  #include "hw/loader.h"
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

[Qemu-devel] [ARM SMBIOS V2 PATCH 5/6] smbios: add smbios 3.0 support

[Wei Huang]

This patch adds support for SMBIOS 3.0 entry point. When caller invokes
smbios_set_defaults(), it can specify entry point as 2.1 or 3.0. Then
smbios_get_tables() will return the entry point table in right format.

Signed-off-by: Wei Huang <wei@redhat.com>
---
 hw/i386/pc_piix.c          |  3 +-
 hw/i386/pc_q35.c           |  3 +-
 hw/smbios/smbios.c         | 72 +++++++++++++++++++++++++++++++---------------
 include/hw/smbios/smbios.h | 54 ++++++++++++++++++++++++----------
 4 files changed, 92 insertions(+), 40 deletions(-)

diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 653c710..04636b1 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -173,7 +173,8 @@ static void pc_init1(MachineState *machine)
         MachineClass *mc = MACHINE_GET_CLASS(machine);
         /* These values are guest ABI, do not change */
         smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
-                            mc->name, smbios_legacy_mode, smbios_uuid_encoded);
+                            mc->name, smbios_legacy_mode, smbios_uuid_encoded,
+                            SMBIOS_ENTRY_POINT_21);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d83df14..061507d 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -165,7 +165,8 @@ static void pc_q35_init(MachineState *machine)
     if (smbios_defaults) {
         /* These values are guest ABI, do not change */
         smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
-                            mc->name, smbios_legacy_mode, smbios_uuid_encoded);
+                            mc->name, smbios_legacy_mode, smbios_uuid_encoded,
+                            SMBIOS_ENTRY_POINT_21);
     }
 
     /* allocate ram and load rom/bios */
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index 08ba62a..ea3a32d 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -48,6 +48,7 @@ static uint8_t *smbios_entries;
 static size_t smbios_entries_len;
 static bool smbios_legacy = true;
 static bool smbios_uuid_encoded = true;
+static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_21;
 /* end: legacy structures & constants for <= 2.0 machines */
 
 
@@ -55,7 +56,8 @@ static uint8_t *smbios_tables;
 static size_t smbios_tables_len;
 static unsigned smbios_table_max;
 static unsigned smbios_table_cnt;
-static struct smbios_entry_point ep;
+static SmbiosEntryPoint ep;
+static size_t ep_length;
 
 static int smbios_type4_count = 0;
 static bool smbios_immutable;
@@ -771,11 +773,12 @@ void smbios_set_cpuid(uint32_t version, uint32_t features)
 
 void smbios_set_defaults(const char *manufacturer, const char *product,
                          const char *version, bool legacy_mode,
-                         bool uuid_encoded)
+                         bool uuid_encoded, SmbiosEntryPointType ep_type)
 {
     smbios_have_defaults = true;
     smbios_legacy = legacy_mode;
     smbios_uuid_encoded = uuid_encoded;
+    smbios_ep_type = ep_type;
 
     /* drop unwanted version of command-line file blob(s) */
     if (smbios_legacy) {
@@ -808,26 +811,49 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
 
 static void smbios_entry_point_setup(void)
 {
-    memcpy(ep.anchor_string, "_SM_", 4);
-    memcpy(ep.intermediate_anchor_string, "_DMI_", 5);
-    ep.length = sizeof(struct smbios_entry_point);
-    ep.entry_point_revision = 0; /* formatted_area reserved, per spec v2.1+ */
-    memset(ep.formatted_area, 0, 5);
-
-    /* compliant with smbios spec v2.8 */
-    ep.smbios_major_version = 2;
-    ep.smbios_minor_version = 8;
-    ep.smbios_bcd_revision = 0x28;
-
-    /* set during table construction, but BIOS may override: */
-    ep.structure_table_length = cpu_to_le16(smbios_tables_len);
-    ep.max_structure_size = cpu_to_le16(smbios_table_max);
-    ep.number_of_structures = cpu_to_le16(smbios_table_cnt);
-
-    /* BIOS must recalculate: */
-    ep.checksum = 0;
-    ep.intermediate_checksum = 0;
-    ep.structure_table_address = cpu_to_le32(0);
+    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21) {
+        memcpy(ep.ep21.anchor_string, "_SM_", 4);
+        memcpy(ep.ep21.intermediate_anchor_string, "_DMI_", 5);
+        ep.ep21.length = sizeof(struct smbios_21_entry_point);
+        ep.ep21.entry_point_revision = 0; /* formatted_area reserved */
+        memset(ep.ep21.formatted_area, 0, 5);
+
+        /* compliant with smbios spec v2.8 */
+        ep.ep21.smbios_major_version = 2;
+        ep.ep21.smbios_minor_version = 8;
+        ep.ep21.smbios_bcd_revision = 0x28;
+
+        /* set during table construction, but BIOS may override: */
+        ep.ep21.structure_table_length = cpu_to_le16(smbios_tables_len);
+        ep.ep21.max_structure_size = cpu_to_le16(smbios_table_max);
+        ep.ep21.number_of_structures = cpu_to_le16(smbios_table_cnt);
+
+        /* BIOS must recalculate: */
+        ep.ep21.checksum = 0;
+        ep.ep21.intermediate_checksum = 0;
+        ep.ep21.structure_table_address = cpu_to_le32(0);
+
+        /* setup the anchor point length */
+        ep_length = sizeof (struct smbios_21_entry_point);
+    } else if (smbios_ep_type == SMBIOS_ENTRY_POINT_30) {
+        memcpy(ep.ep30.anchor_string, "_SM3_", 5);
+        ep.ep30.length = sizeof(struct smbios_30_entry_point);
+        ep.ep30.entry_point_revision = 1;
+
+        /* compliant with smbios spec 3.0 */
+        ep.ep30.smbios_major_version = 3;
+        ep.ep30.smbios_minor_version = 0;
+        ep.ep30.smbios_doc_rev = 0;
+
+        /* set during table construct, but BIOS might override */
+        ep.ep30.structure_table_max_size = cpu_to_le32(smbios_tables_len);
+
+        /* BIOS must recalculate */
+        ep.ep30.checksum = 0;
+        ep.ep30.structure_table_address = cpu_to_le64(0);
+
+        ep_length = sizeof (struct smbios_30_entry_point);
+    }
 }
 
 void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
@@ -886,7 +912,7 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
     *tables = smbios_tables;
     *tables_len = smbios_tables_len;
     *anchor = (uint8_t *)&ep;
-    *anchor_len = sizeof(struct smbios_entry_point);
+    *anchor_len = ep_length;
 }
 
 static void save_opt(const char **dest, QemuOpts *opts, const char *name)
diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h
index e727233..ba0e68f 100644
--- a/include/hw/smbios/smbios.h
+++ b/include/hw/smbios/smbios.h
@@ -24,26 +24,20 @@ struct smbios_phys_mem_area {
     uint64_t length;
 };
 
-void smbios_entry_add(QemuOpts *opts);
-void smbios_set_cpuid(uint32_t version, uint32_t features);
-void smbios_set_defaults(const char *manufacturer, const char *product,
-                         const char *version, bool legacy_mode,
-                         bool uuid_encoded);
-uint8_t *smbios_get_table_legacy(size_t *length);
-void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
-                       const unsigned int mem_array_size,
-                       const ram_addr_t ram_size,
-                       uint8_t **tables, size_t *tables_len,
-                       uint8_t **anchor, size_t *anchor_len);
-
 /*
  * SMBIOS spec defined tables
  */
+typedef enum SmbiosEntryPointType {
+    SMBIOS_ENTRY_POINT_21,
+    SMBIOS_ENTRY_POINT_30,
+} SmbiosEntryPointType;
 
-/* SMBIOS entry point (anchor).
- * BIOS must place this at a 16-bit-aligned address between 0xf0000 and 0xfffff.
+/* SMBIOS entry point
+ * Note: BIOS must place this at a 16-bit-aligned address between 0xf0000
+ * and 0xfffff.
  */
-struct smbios_entry_point {
+/* SMBIOS 2.1 entry point */
+struct smbios_21_entry_point {
     uint8_t anchor_string[4];
     uint8_t checksum;
     uint8_t length;
@@ -60,6 +54,25 @@ struct smbios_entry_point {
     uint8_t smbios_bcd_revision;
 } QEMU_PACKED;
 
+/* SMBIOS 3.0 entry point */
+struct smbios_30_entry_point {
+    uint8_t anchor_string[5];
+    uint8_t checksum;
+    uint8_t length;
+    uint8_t smbios_major_version;
+    uint8_t smbios_minor_version;
+    uint8_t smbios_doc_rev;
+    uint8_t entry_point_revision;
+    uint8_t _reserved;
+    uint32_t structure_table_max_size;
+    uint64_t structure_table_address;
+} QEMU_PACKED;
+
+typedef union QEMU_PACKED {
+    struct smbios_21_entry_point ep21;
+    struct smbios_30_entry_point ep30;
+} SmbiosEntryPoint;
+
 /* This goes at the beginning of every SMBIOS structure. */
 struct smbios_structure_header {
     uint8_t type;
@@ -234,4 +247,15 @@ struct smbios_type_127 {
     struct smbios_structure_header header;
 } QEMU_PACKED;
 
+void smbios_entry_add(QemuOpts *opts);
+void smbios_set_cpuid(uint32_t version, uint32_t features);
+void smbios_set_defaults(const char *manufacturer, const char *product,
+                         const char *version, bool legacy_mode,
+                         bool uuid_encoded, SmbiosEntryPointType ep_type);
+uint8_t *smbios_get_table_legacy(size_t *length);
+void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
+                       const unsigned int mem_array_size,
+                       const ram_addr_t ram_size,
+                       uint8_t **tables, size_t *tables_len,
+                       uint8_t **anchor, size_t *anchor_len);
 #endif /*QEMU_SMBIOS_H */
-- 
1.8.3.1

Re: [Qemu-devel] [ARM SMBIOS V2 PATCH 5/6] smbios: add smbios 3.0 support

[Laszlo Ersek]

comments below

On 08/06/15 19:14, Wei Huang wrote:
> This patch adds support for SMBIOS 3.0 entry point. When caller invokes
> smbios_set_defaults(), it can specify entry point as 2.1 or 3.0. Then
> smbios_get_tables() will return the entry point table in right format.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/i386/pc_piix.c          |  3 +-
>  hw/i386/pc_q35.c           |  3 +-
>  hw/smbios/smbios.c         | 72 +++++++++++++++++++++++++++++++---------------
>  include/hw/smbios/smbios.h | 54 ++++++++++++++++++++++++----------
>  4 files changed, 92 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 653c710..04636b1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -173,7 +173,8 @@ static void pc_init1(MachineState *machine)
>          MachineClass *mc = MACHINE_GET_CLASS(machine);
>          /* These values are guest ABI, do not change */
>          smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
> -                            mc->name, smbios_legacy_mode, smbios_uuid_encoded);
> +                            mc->name, smbios_legacy_mode, smbios_uuid_encoded,
> +                            SMBIOS_ENTRY_POINT_21);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d83df14..061507d 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -165,7 +165,8 @@ static void pc_q35_init(MachineState *machine)
>      if (smbios_defaults) {
>          /* These values are guest ABI, do not change */
>          smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
> -                            mc->name, smbios_legacy_mode, smbios_uuid_encoded);
> +                            mc->name, smbios_legacy_mode, smbios_uuid_encoded,
> +                            SMBIOS_ENTRY_POINT_21);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 08ba62a..ea3a32d 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -48,6 +48,7 @@ static uint8_t *smbios_entries;
>  static size_t smbios_entries_len;
>  static bool smbios_legacy = true;
>  static bool smbios_uuid_encoded = true;
> +static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_21;

Looks good. This initialization makes sure that smbios_ep_type is set
compatibly even if smbios_set_defaults() is never called. (And allows me
to forego verifying whether smbios_set_defaults() is "always called when
it is needed".) So, good.

>  /* end: legacy structures & constants for <= 2.0 machines */
>  
>  
> @@ -55,7 +56,8 @@ static uint8_t *smbios_tables;
>  static size_t smbios_tables_len;
>  static unsigned smbios_table_max;
>  static unsigned smbios_table_cnt;
> -static struct smbios_entry_point ep;
> +static SmbiosEntryPoint ep;
> +static size_t ep_length;
>  
>  static int smbios_type4_count = 0;
>  static bool smbios_immutable;
> @@ -771,11 +773,12 @@ void smbios_set_cpuid(uint32_t version, uint32_t features)
>  
>  void smbios_set_defaults(const char *manufacturer, const char *product,
>                           const char *version, bool legacy_mode,
> -                         bool uuid_encoded)
> +                         bool uuid_encoded, SmbiosEntryPointType ep_type)
>  {
>      smbios_have_defaults = true;
>      smbios_legacy = legacy_mode;
>      smbios_uuid_encoded = uuid_encoded;
> +    smbios_ep_type = ep_type;
>  
>      /* drop unwanted version of command-line file blob(s) */
>      if (smbios_legacy) {
> @@ -808,26 +811,49 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
>  
>  static void smbios_entry_point_setup(void)
>  {
> -    memcpy(ep.anchor_string, "_SM_", 4);
> -    memcpy(ep.intermediate_anchor_string, "_DMI_", 5);
> -    ep.length = sizeof(struct smbios_entry_point);
> -    ep.entry_point_revision = 0; /* formatted_area reserved, per spec v2.1+ */
> -    memset(ep.formatted_area, 0, 5);
> -
> -    /* compliant with smbios spec v2.8 */
> -    ep.smbios_major_version = 2;
> -    ep.smbios_minor_version = 8;
> -    ep.smbios_bcd_revision = 0x28;
> -
> -    /* set during table construction, but BIOS may override: */
> -    ep.structure_table_length = cpu_to_le16(smbios_tables_len);
> -    ep.max_structure_size = cpu_to_le16(smbios_table_max);
> -    ep.number_of_structures = cpu_to_le16(smbios_table_cnt);
> -
> -    /* BIOS must recalculate: */
> -    ep.checksum = 0;
> -    ep.intermediate_checksum = 0;
> -    ep.structure_table_address = cpu_to_le32(0);
> +    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21) {
> +        memcpy(ep.ep21.anchor_string, "_SM_", 4);
> +        memcpy(ep.ep21.intermediate_anchor_string, "_DMI_", 5);
> +        ep.ep21.length = sizeof(struct smbios_21_entry_point);
> +        ep.ep21.entry_point_revision = 0; /* formatted_area reserved */
> +        memset(ep.ep21.formatted_area, 0, 5);
> +
> +        /* compliant with smbios spec v2.8 */
> +        ep.ep21.smbios_major_version = 2;
> +        ep.ep21.smbios_minor_version = 8;
> +        ep.ep21.smbios_bcd_revision = 0x28;
> +
> +        /* set during table construction, but BIOS may override: */
> +        ep.ep21.structure_table_length = cpu_to_le16(smbios_tables_len);
> +        ep.ep21.max_structure_size = cpu_to_le16(smbios_table_max);
> +        ep.ep21.number_of_structures = cpu_to_le16(smbios_table_cnt);
> +
> +        /* BIOS must recalculate: */
> +        ep.ep21.checksum = 0;
> +        ep.ep21.intermediate_checksum = 0;
> +        ep.ep21.structure_table_address = cpu_to_le32(0);
> +
> +        /* setup the anchor point length */
> +        ep_length = sizeof (struct smbios_21_entry_point);
> +    } else if (smbios_ep_type == SMBIOS_ENTRY_POINT_30) {
> +        memcpy(ep.ep30.anchor_string, "_SM3_", 5);
> +        ep.ep30.length = sizeof(struct smbios_30_entry_point);
> +        ep.ep30.entry_point_revision = 1;
> +
> +        /* compliant with smbios spec 3.0 */
> +        ep.ep30.smbios_major_version = 3;
> +        ep.ep30.smbios_minor_version = 0;
> +        ep.ep30.smbios_doc_rev = 0;
> +
> +        /* set during table construct, but BIOS might override */
> +        ep.ep30.structure_table_max_size = cpu_to_le32(smbios_tables_len);
> +
> +        /* BIOS must recalculate */
> +        ep.ep30.checksum = 0;
> +        ep.ep30.structure_table_address = cpu_to_le64(0);
> +

The _reserved field is not zeroed.

It's not a problem in practice (because "ep" has static storage
duration, and is therefore zero-initialized), but I think zeroing it
explicitly it would be clearer.

> +        ep_length = sizeof (struct smbios_30_entry_point);
> +    }

I think you should assert here that smbios_ep_type has no other value.
(Alternatively, consider turning this into a switch statement, and
assert(false) under the "default" label.)

>  }
>  
>  void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> @@ -886,7 +912,7 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>      *tables = smbios_tables;
>      *tables_len = smbios_tables_len;
>      *anchor = (uint8_t *)&ep;
> -    *anchor_len = sizeof(struct smbios_entry_point);
> +    *anchor_len = ep_length;
>  }
>  
>  static void save_opt(const char **dest, QemuOpts *opts, const char *name)
> diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h
> index e727233..ba0e68f 100644
> --- a/include/hw/smbios/smbios.h
> +++ b/include/hw/smbios/smbios.h
> @@ -24,26 +24,20 @@ struct smbios_phys_mem_area {
>      uint64_t length;
>  };
>  
> -void smbios_entry_add(QemuOpts *opts);
> -void smbios_set_cpuid(uint32_t version, uint32_t features);
> -void smbios_set_defaults(const char *manufacturer, const char *product,
> -                         const char *version, bool legacy_mode,
> -                         bool uuid_encoded);
> -uint8_t *smbios_get_table_legacy(size_t *length);
> -void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> -                       const unsigned int mem_array_size,
> -                       const ram_addr_t ram_size,
> -                       uint8_t **tables, size_t *tables_len,
> -                       uint8_t **anchor, size_t *anchor_len);
> -
>  /*
>   * SMBIOS spec defined tables
>   */
> +typedef enum SmbiosEntryPointType {
> +    SMBIOS_ENTRY_POINT_21,
> +    SMBIOS_ENTRY_POINT_30,
> +} SmbiosEntryPointType;
>  
> -/* SMBIOS entry point (anchor).
> - * BIOS must place this at a 16-bit-aligned address between 0xf0000 and 0xfffff.
> +/* SMBIOS entry point
> + * Note: BIOS must place this at a 16-bit-aligned address between 0xf0000
> + * and 0xfffff.
>   */
> -struct smbios_entry_point {
> +/* SMBIOS 2.1 entry point */
> +struct smbios_21_entry_point {
>      uint8_t anchor_string[4];
>      uint8_t checksum;
>      uint8_t length;
> @@ -60,6 +54,25 @@ struct smbios_entry_point {
>      uint8_t smbios_bcd_revision;
>  } QEMU_PACKED;
>  
> +/* SMBIOS 3.0 entry point */
> +struct smbios_30_entry_point {
> +    uint8_t anchor_string[5];
> +    uint8_t checksum;
> +    uint8_t length;
> +    uint8_t smbios_major_version;
> +    uint8_t smbios_minor_version;
> +    uint8_t smbios_doc_rev;
> +    uint8_t entry_point_revision;
> +    uint8_t _reserved;

Please call this field "reserved", not "_reserved".

I'm asking for this *despite* the fact that "_reserved" is valid as a
field name.

>From the standard: "All identifiers that begin with an underscore are
always reserved for use as identifiers with file scope in both the
ordinary and tag name spaces." However, we're not in the ordinary
identifier nor the tag name space here -- "each structure or union has a
separate name space for its members".

But, I still think _reserved is not really idiomatic.

> +    uint32_t structure_table_max_size;
> +    uint64_t structure_table_address;
> +} QEMU_PACKED;
> +
> +typedef union QEMU_PACKED {
> +    struct smbios_21_entry_point ep21;
> +    struct smbios_30_entry_point ep30;
> +} SmbiosEntryPoint;
> +

QEMU_PACKED should come between "}" and "SmbiosEntryPoint", for
consistency with the rest of the code.

>  /* This goes at the beginning of every SMBIOS structure. */
>  struct smbios_structure_header {
>      uint8_t type;
> @@ -234,4 +247,15 @@ struct smbios_type_127 {
>      struct smbios_structure_header header;
>  } QEMU_PACKED;
>  
> +void smbios_entry_add(QemuOpts *opts);
> +void smbios_set_cpuid(uint32_t version, uint32_t features);
> +void smbios_set_defaults(const char *manufacturer, const char *product,
> +                         const char *version, bool legacy_mode,
> +                         bool uuid_encoded, SmbiosEntryPointType ep_type);
> +uint8_t *smbios_get_table_legacy(size_t *length);
> +void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> +                       const unsigned int mem_array_size,
> +                       const ram_addr_t ram_size,
> +                       uint8_t **tables, size_t *tables_len,
> +                       uint8_t **anchor, size_t *anchor_len);
>  #endif /*QEMU_SMBIOS_H */
> 

Thanks!
Laszlo

Re: [Qemu-devel] [ARM SMBIOS V2 PATCH 5/6] smbios: add smbios 3.0 support

[Laszlo Ersek]

sorry, another comment:

On 08/06/15 19:14, Wei Huang wrote:
> This patch adds support for SMBIOS 3.0 entry point. When caller invokes
> smbios_set_defaults(), it can specify entry point as 2.1 or 3.0. Then
> smbios_get_tables() will return the entry point table in right format.
> 
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/i386/pc_piix.c          |  3 +-
>  hw/i386/pc_q35.c           |  3 +-
>  hw/smbios/smbios.c         | 72 +++++++++++++++++++++++++++++++---------------
>  include/hw/smbios/smbios.h | 54 ++++++++++++++++++++++++----------
>  4 files changed, 92 insertions(+), 40 deletions(-)
> 
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 653c710..04636b1 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -173,7 +173,8 @@ static void pc_init1(MachineState *machine)
>          MachineClass *mc = MACHINE_GET_CLASS(machine);
>          /* These values are guest ABI, do not change */
>          smbios_set_defaults("QEMU", "Standard PC (i440FX + PIIX, 1996)",
> -                            mc->name, smbios_legacy_mode, smbios_uuid_encoded);
> +                            mc->name, smbios_legacy_mode, smbios_uuid_encoded,
> +                            SMBIOS_ENTRY_POINT_21);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d83df14..061507d 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -165,7 +165,8 @@ static void pc_q35_init(MachineState *machine)
>      if (smbios_defaults) {
>          /* These values are guest ABI, do not change */
>          smbios_set_defaults("QEMU", "Standard PC (Q35 + ICH9, 2009)",
> -                            mc->name, smbios_legacy_mode, smbios_uuid_encoded);
> +                            mc->name, smbios_legacy_mode, smbios_uuid_encoded,
> +                            SMBIOS_ENTRY_POINT_21);
>      }
>  
>      /* allocate ram and load rom/bios */
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index 08ba62a..ea3a32d 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -48,6 +48,7 @@ static uint8_t *smbios_entries;
>  static size_t smbios_entries_len;
>  static bool smbios_legacy = true;
>  static bool smbios_uuid_encoded = true;
> +static SmbiosEntryPointType smbios_ep_type = SMBIOS_ENTRY_POINT_21;
>  /* end: legacy structures & constants for <= 2.0 machines */

smbios_ep_type should not be defined in this "section". smbios_ep_type
is not used with the legacy SMBIOS interface (available on <= 2.0
machine types). That interface doesn't export an entry point at all.

Instead, smbios_ep_type belongs to the same spot where "ep" and
"ep_length" are defined:

>  
>  
> @@ -55,7 +56,8 @@ static uint8_t *smbios_tables;
>  static size_t smbios_tables_len;
>  static unsigned smbios_table_max;
>  static unsigned smbios_table_cnt;
> -static struct smbios_entry_point ep;
> +static SmbiosEntryPoint ep;
> +static size_t ep_length;

here.

Thanks
Laszlo

>  
>  static int smbios_type4_count = 0;
>  static bool smbios_immutable;
> @@ -771,11 +773,12 @@ void smbios_set_cpuid(uint32_t version, uint32_t features)
>  
>  void smbios_set_defaults(const char *manufacturer, const char *product,
>                           const char *version, bool legacy_mode,
> -                         bool uuid_encoded)
> +                         bool uuid_encoded, SmbiosEntryPointType ep_type)
>  {
>      smbios_have_defaults = true;
>      smbios_legacy = legacy_mode;
>      smbios_uuid_encoded = uuid_encoded;
> +    smbios_ep_type = ep_type;
>  
>      /* drop unwanted version of command-line file blob(s) */
>      if (smbios_legacy) {
> @@ -808,26 +811,49 @@ void smbios_set_defaults(const char *manufacturer, const char *product,
>  
>  static void smbios_entry_point_setup(void)
>  {
> -    memcpy(ep.anchor_string, "_SM_", 4);
> -    memcpy(ep.intermediate_anchor_string, "_DMI_", 5);
> -    ep.length = sizeof(struct smbios_entry_point);
> -    ep.entry_point_revision = 0; /* formatted_area reserved, per spec v2.1+ */
> -    memset(ep.formatted_area, 0, 5);
> -
> -    /* compliant with smbios spec v2.8 */
> -    ep.smbios_major_version = 2;
> -    ep.smbios_minor_version = 8;
> -    ep.smbios_bcd_revision = 0x28;
> -
> -    /* set during table construction, but BIOS may override: */
> -    ep.structure_table_length = cpu_to_le16(smbios_tables_len);
> -    ep.max_structure_size = cpu_to_le16(smbios_table_max);
> -    ep.number_of_structures = cpu_to_le16(smbios_table_cnt);
> -
> -    /* BIOS must recalculate: */
> -    ep.checksum = 0;
> -    ep.intermediate_checksum = 0;
> -    ep.structure_table_address = cpu_to_le32(0);
> +    if (smbios_ep_type == SMBIOS_ENTRY_POINT_21) {
> +        memcpy(ep.ep21.anchor_string, "_SM_", 4);
> +        memcpy(ep.ep21.intermediate_anchor_string, "_DMI_", 5);
> +        ep.ep21.length = sizeof(struct smbios_21_entry_point);
> +        ep.ep21.entry_point_revision = 0; /* formatted_area reserved */
> +        memset(ep.ep21.formatted_area, 0, 5);
> +
> +        /* compliant with smbios spec v2.8 */
> +        ep.ep21.smbios_major_version = 2;
> +        ep.ep21.smbios_minor_version = 8;
> +        ep.ep21.smbios_bcd_revision = 0x28;
> +
> +        /* set during table construction, but BIOS may override: */
> +        ep.ep21.structure_table_length = cpu_to_le16(smbios_tables_len);
> +        ep.ep21.max_structure_size = cpu_to_le16(smbios_table_max);
> +        ep.ep21.number_of_structures = cpu_to_le16(smbios_table_cnt);
> +
> +        /* BIOS must recalculate: */
> +        ep.ep21.checksum = 0;
> +        ep.ep21.intermediate_checksum = 0;
> +        ep.ep21.structure_table_address = cpu_to_le32(0);
> +
> +        /* setup the anchor point length */
> +        ep_length = sizeof (struct smbios_21_entry_point);
> +    } else if (smbios_ep_type == SMBIOS_ENTRY_POINT_30) {
> +        memcpy(ep.ep30.anchor_string, "_SM3_", 5);
> +        ep.ep30.length = sizeof(struct smbios_30_entry_point);
> +        ep.ep30.entry_point_revision = 1;
> +
> +        /* compliant with smbios spec 3.0 */
> +        ep.ep30.smbios_major_version = 3;
> +        ep.ep30.smbios_minor_version = 0;
> +        ep.ep30.smbios_doc_rev = 0;
> +
> +        /* set during table construct, but BIOS might override */
> +        ep.ep30.structure_table_max_size = cpu_to_le32(smbios_tables_len);
> +
> +        /* BIOS must recalculate */
> +        ep.ep30.checksum = 0;
> +        ep.ep30.structure_table_address = cpu_to_le64(0);
> +
> +        ep_length = sizeof (struct smbios_30_entry_point);
> +    }
>  }
>  
>  void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> @@ -886,7 +912,7 @@ void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
>      *tables = smbios_tables;
>      *tables_len = smbios_tables_len;
>      *anchor = (uint8_t *)&ep;
> -    *anchor_len = sizeof(struct smbios_entry_point);
> +    *anchor_len = ep_length;
>  }
>  
>  static void save_opt(const char **dest, QemuOpts *opts, const char *name)
> diff --git a/include/hw/smbios/smbios.h b/include/hw/smbios/smbios.h
> index e727233..ba0e68f 100644
> --- a/include/hw/smbios/smbios.h
> +++ b/include/hw/smbios/smbios.h
> @@ -24,26 +24,20 @@ struct smbios_phys_mem_area {
>      uint64_t length;
>  };
>  
> -void smbios_entry_add(QemuOpts *opts);
> -void smbios_set_cpuid(uint32_t version, uint32_t features);
> -void smbios_set_defaults(const char *manufacturer, const char *product,
> -                         const char *version, bool legacy_mode,
> -                         bool uuid_encoded);
> -uint8_t *smbios_get_table_legacy(size_t *length);
> -void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> -                       const unsigned int mem_array_size,
> -                       const ram_addr_t ram_size,
> -                       uint8_t **tables, size_t *tables_len,
> -                       uint8_t **anchor, size_t *anchor_len);
> -
>  /*
>   * SMBIOS spec defined tables
>   */
> +typedef enum SmbiosEntryPointType {
> +    SMBIOS_ENTRY_POINT_21,
> +    SMBIOS_ENTRY_POINT_30,
> +} SmbiosEntryPointType;
>  
> -/* SMBIOS entry point (anchor).
> - * BIOS must place this at a 16-bit-aligned address between 0xf0000 and 0xfffff.
> +/* SMBIOS entry point
> + * Note: BIOS must place this at a 16-bit-aligned address between 0xf0000
> + * and 0xfffff.
>   */
> -struct smbios_entry_point {
> +/* SMBIOS 2.1 entry point */
> +struct smbios_21_entry_point {
>      uint8_t anchor_string[4];
>      uint8_t checksum;
>      uint8_t length;
> @@ -60,6 +54,25 @@ struct smbios_entry_point {
>      uint8_t smbios_bcd_revision;
>  } QEMU_PACKED;
>  
> +/* SMBIOS 3.0 entry point */
> +struct smbios_30_entry_point {
> +    uint8_t anchor_string[5];
> +    uint8_t checksum;
> +    uint8_t length;
> +    uint8_t smbios_major_version;
> +    uint8_t smbios_minor_version;
> +    uint8_t smbios_doc_rev;
> +    uint8_t entry_point_revision;
> +    uint8_t _reserved;
> +    uint32_t structure_table_max_size;
> +    uint64_t structure_table_address;
> +} QEMU_PACKED;
> +
> +typedef union QEMU_PACKED {
> +    struct smbios_21_entry_point ep21;
> +    struct smbios_30_entry_point ep30;
> +} SmbiosEntryPoint;
> +
>  /* This goes at the beginning of every SMBIOS structure. */
>  struct smbios_structure_header {
>      uint8_t type;
> @@ -234,4 +247,15 @@ struct smbios_type_127 {
>      struct smbios_structure_header header;
>  } QEMU_PACKED;
>  
> +void smbios_entry_add(QemuOpts *opts);
> +void smbios_set_cpuid(uint32_t version, uint32_t features);
> +void smbios_set_defaults(const char *manufacturer, const char *product,
> +                         const char *version, bool legacy_mode,
> +                         bool uuid_encoded, SmbiosEntryPointType ep_type);
> +uint8_t *smbios_get_table_legacy(size_t *length);
> +void smbios_get_tables(const struct smbios_phys_mem_area *mem_array,
> +                       const unsigned int mem_array_size,
> +                       const ram_addr_t ram_size,
> +                       uint8_t **tables, size_t *tables_len,
> +                       uint8_t **anchor, size_t *anchor_len);
>  #endif /*QEMU_SMBIOS_H */
> 

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

[Wei Huang]

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);
+
+    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;
-- 
1.8.3.1

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

[Shannon Zhao]

On 2015/8/7 1: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>

Reviewed-by: Shannon Zhao <shannon.zhao@linaro.org>

> ---
>  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);
> +
> +    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;
> 

-- 
Shannon

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

[Laszlo Ersek]

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;
> 

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

[Gabriel L. Somlo]

On Thu, Aug 06, 2015 at 01:14:54PM -0400, 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 (V2) integrates SMBIOS 3.0 support
> for ARM mach-virt. I have tested this version using a customized AAVMF
> created by Laszlo, who has submitted his patches to OVMF mailing list.
> 
> V1->V2:
>  * Add NULL checking for fw_cfg (Shannon Zhao)
>  * Init 3.0 entry point table max size to smbios_tables_len (Laszlo)
>  * Minor re-arrangement of smbios.h layout with function headers to the bottom
>  * Validated SMBIOS 3.0 tables with a customized AAVMF created by Laszlo
> 
> 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

whole series:

Acked-by: Gabriel Somlo <somlo@cmu.edu>
Regression-tested-by: Gabriel Somlo <somlo@cmu.edu>

Thanks,
--Gabriel

> 
> 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                        | 27 ++++++++++
>  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 | 56 ++++++++++++++++-----
>  tests/bios-tables-test.c             |  2 +-
>  vl.c                                 |  2 +-
>  16 files changed, 188 insertions(+), 71 deletions(-)
>  create mode 100644 hw/smbios/Makefile.objs
>  rename hw/{i386 => smbios}/smbios.c (93%)
>  rename include/hw/{i386 => smbios}/smbios.h (83%)
> 
> -- 
> 1.8.3.1
> 

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

[Leif Lindholm]

Hi Wei,

So, I can verify that this works all the way into Linux userland
dmidecode on arm64.

One question:
This set still leaves the -smbios set of options x86 only.

Would

--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1412,7 +1412,7 @@ DEF("smbios", HAS_ARG, QEMU_OPTION_smbios,
     "-smbios
     type=17[,loc_pfx=str][,bank=str][,manufacturer=str][,serial=str]\n"
     "               [,asset=str][,part=str][,speed=%d]\n"
     "                specify SMBIOS type 17 fields\n",
-    QEMU_ARCH_I386)
+    QEMU_ARCH_I386 | QEMU_ARCH_ARM)
 STEXI
 @item -smbios file=@var{binary}
 @findex -smbios

be relevant to add here (6/6), and would that then be expected to work
for filling in values via command line (because that does not
currently appear to be the case).

/
    Leif 

On Thu, Aug 06, 2015 at 01:14:54PM -0400, 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 (V2) integrates SMBIOS 3.0 support
> for ARM mach-virt. I have tested this version using a customized AAVMF
> created by Laszlo, who has submitted his patches to OVMF mailing list.
> 
> V1->V2:
>  * Add NULL checking for fw_cfg (Shannon Zhao)
>  * Init 3.0 entry point table max size to smbios_tables_len (Laszlo)
>  * Minor re-arrangement of smbios.h layout with function headers to the bottom
>  * Validated SMBIOS 3.0 tables with a customized AAVMF created by Laszlo
> 
> 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                        | 27 ++++++++++
>  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 | 56 ++++++++++++++++-----
>  tests/bios-tables-test.c             |  2 +-
>  vl.c                                 |  2 +-
>  16 files changed, 188 insertions(+), 71 deletions(-)
>  create mode 100644 hw/smbios/Makefile.objs
>  rename hw/{i386 => smbios}/smbios.c (93%)
>  rename include/hw/{i386 => smbios}/smbios.h (83%)
> 
> -- 
> 1.8.3.1
> 
>