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

[Laszlo Ersek <lersek@redhat.com>]

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