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 <w...@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 <ler...@redhat.com>
>
> to the next version at once.
>
> Thanks
> Laszlo
>
