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

[Wei Huang <wei@redhat.com>]

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
>