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