From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58765) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNkiB-0007wC-Mu for qemu-devel@nongnu.org; Fri, 07 Aug 2015 12:41:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZNki2-00039Q-Pm for qemu-devel@nongnu.org; Fri, 07 Aug 2015 12:41:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48978) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNki2-00039J-Ik for qemu-devel@nongnu.org; Fri, 07 Aug 2015 12:41:30 -0400 References: <1438881300-21738-1-git-send-email-wei@redhat.com> <1438881300-21738-4-git-send-email-wei@redhat.com> From: Laszlo Ersek Message-ID: <55C4DFB0.4050302@redhat.com> Date: Fri, 7 Aug 2015 18:41:20 +0200 MIME-Version: 1.0 In-Reply-To: <1438881300-21738-4-git-send-email-wei@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [ARM SMBIOS V2 PATCH 3/6] smbios: pass ram size as a parameter to build smbios tables List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wei Huang , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, drjones@redhat.com, ard.biesheuvel@linaro.org, ehabkost@redhat.com, ivan.khoronzhuk@linaro.org, mst@redhat.com, somlo@cmu.edu, zhaoshenglong@huawei.com, roy.franz@linaro.org, pbonzini@redhat.com, imammedo@redhat.com, jdelvare@suse.de, rth@twiddle.net 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 > --- > 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 to the next version at once. Thanks Laszlo