From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60152) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VAdzW-00088W-Go for qemu-devel@nongnu.org; Sat, 17 Aug 2013 06:44:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VAdzR-0007Iw-K4 for qemu-devel@nongnu.org; Sat, 17 Aug 2013 06:44:18 -0400 Message-ID: <520F548C.2090903@redhat.com> Date: Sat, 17 Aug 2013 12:46:36 +0200 From: Laszlo Ersek MIME-Version: 1.0 References: <1376651630-9151-1-git-send-email-armbru@redhat.com> <1376651630-9151-6-git-send-email-armbru@redhat.com> In-Reply-To: <1376651630-9151-6-git-send-email-armbru@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 5/6] ppc: Don't duplicate QEMUMachineInitArgs in PPCE500Params List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: armbru@redhat.com Cc: aliguori@us.ibm.com, mjt@tls.msk.ru, qemu-devel@nongnu.org, agraf@suse.de, blauwirbel@gmail.com, qemu-ppc@nongnu.org, aviksil@linux.vnet.ibm.com comments below On 08/16/13 13:13, armbru@redhat.com wrote: > From: Markus Armbruster > > Pass on the generic arguments unadulterated, and the machine-specific > ones as separate argument. > > Signed-off-by: Markus Armbruster > Acked-by: Alexander Graf > --- > hw/ppc/e500.c | 35 ++++++++++++++++++----------------- > hw/ppc/e500.h | 13 +++---------- > hw/ppc/e500plat.c | 8 +------- > hw/ppc/mpc8544ds.c | 8 +------- > 4 files changed, 23 insertions(+), 41 deletions(-) Please always use -O/path/to/order_file when invoking git-format-patch. The contents of "order_file" should be minimally configure Makefile* *.json *.h *.c It's much easier to review a patch when "declarative changes" are shown first (ie. in approximate logical dependency order). Then, > -void ppce500_init(PPCE500Params *params) > +void ppce500_init(QEMUMachineInitArgs *args, PPCE500Params *params) > { > MemoryRegion *address_space_mem = get_system_memory(); > MemoryRegion *ram = g_new(MemoryRegion, 1); > @@ -584,8 +585,8 @@ void ppce500_init(PPCE500Params *params) > PPCE500CCSRState *ccsr; > > /* Setup CPUs */ > - if (params->cpu_model == NULL) { > - params->cpu_model = "e500v2_v30"; > + if (args->cpu_model == NULL) { > + args->cpu_model = "e500v2_v30"; > } As discussed before, this change will modify the "args.cpu_model" member in main(), but that's OK. > @@ -634,7 +635,7 @@ void ppce500_init(PPCE500Params *params) > > /* Fixup Memory size on a alignment boundary */ > ram_size &= ~(RAM_SIZES_ALIGN - 1); > - params->ram_size = ram_size; > + args->ram_size = ram_size; This hackery (commendably left intact by the patch) is convincing me that QEMUMachineInitArgs should not have a "ram_size" member at all. If "ram_size" is a well-founded global, then let's treat it as such. Whatever. Reviewed-by: Laszlo Ersek