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 <address@hidden>
> ---
>  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 <address@hidden>