From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35113) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNkwq-0000kw-0s for qemu-devel@nongnu.org; Fri, 07 Aug 2015 12:56:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZNkwo-0002mS-LX for qemu-devel@nongnu.org; Fri, 07 Aug 2015 12:56:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60138) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZNkwo-0002mL-EM for qemu-devel@nongnu.org; Fri, 07 Aug 2015 12:56:46 -0400 Message-ID: <55C4E34A.1020500@redhat.com> Date: Fri, 07 Aug 2015 11:56:42 -0500 From: Wei Huang MIME-Version: 1.0 References: <1438881300-21738-1-git-send-email-wei@redhat.com> <1438881300-21738-4-git-send-email-wei@redhat.com> <55C4DFB0.4050302@redhat.com> In-Reply-To: <55C4DFB0.4050302@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: Laszlo Ersek , 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/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 >> --- >> 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 > > to the next version at once. > > Thanks > Laszlo >