From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37627) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bxOzZ-00020T-2k for qemu-devel@nongnu.org; Thu, 20 Oct 2016 21:51:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bxOzW-0001mC-0l for qemu-devel@nongnu.org; Thu, 20 Oct 2016 21:51:29 -0400 Date: Fri, 21 Oct 2016 12:24:20 +1100 From: David Gibson Message-ID: <20161021012420.GA3706@umbus.fritz.box> References: <1476940330-27705-1-git-send-email-david@gibson.dropbear.id.au> <1476940330-27705-3-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="huq684BweRXVnRxX" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 02/13] pseries: Split device tree construction from device tree load List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, groug@kaod.org, agraf@suse.de, lvivier@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org --huq684BweRXVnRxX Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Oct 20, 2016 at 09:03:04AM +0200, Thomas Huth wrote: > On 20.10.2016 07:11, David Gibson wrote: > > spapr_finalize_fdt() both finishes building the device tree for the gue= st > > and loads it into guest memory. For future cleanups, it's going to be > > more convenient to do these two things separately. The loading portion= is > > pretty trivial, so we move it inline into the caller, ppc_spapr_reset(). > >=20 > > We also rename spapr_finalize_fdt(), because the current name is going = to > > become inaccurate. > >=20 > > Signed-off-by: David Gibson > > --- > > hw/ppc/spapr.c | 41 ++++++++++++++++++++++------------------- > > 1 file changed, 22 insertions(+), 19 deletions(-) > >=20 > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index ddb7438..7ff3590 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -900,10 +900,9 @@ int spapr_h_cas_compose_response(sPAPRMachineState= *spapr, > > return 0; > > } > > =20 > > -static void spapr_finalize_fdt(sPAPRMachineState *spapr, > > - hwaddr fdt_addr, > > - hwaddr rtas_addr, > > - hwaddr rtas_size) > > +static void *spapr_build_fdt(sPAPRMachineState *spapr, > > + hwaddr rtas_addr, > > + hwaddr rtas_size) > > { > > MachineState *machine =3D MACHINE(qdev_get_machine()); > > MachineClass *mc =3D MACHINE_GET_CLASS(machine); > > @@ -999,19 +998,8 @@ static void spapr_finalize_fdt(sPAPRMachineState *= spapr, > > } > > } > > =20 > > - _FDT((fdt_pack(fdt))); > > - > > - if (fdt_totalsize(fdt) > FDT_MAX_SIZE) { > > - error_report("FDT too big ! 0x%x bytes (max is 0x%x)", > > - fdt_totalsize(fdt), FDT_MAX_SIZE); > > - exit(1); > > - } > > - > > - qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt)); > > - cpu_physical_memory_write(fdt_addr, fdt, fdt_totalsize(fdt)); > > - > > g_free(bootlist); > > - g_free(fdt); >=20 > Here you removed a g_free(fdt) ... >=20 > > + return fdt; > > } > > =20 > > static uint64_t translate_kernel_address(void *opaque, uint64_t addr) > > @@ -1147,6 +1135,8 @@ static void ppc_spapr_reset(void) > > sPAPRMachineState *spapr =3D SPAPR_MACHINE(machine); > > PowerPCCPU *first_ppc_cpu; > > uint32_t rtas_limit; > > + void *fdt; > > + int rc; > > =20 > > /* Check for unknown sysbus devices */ > > foreach_dynamic_sysbus_device(find_unknown_sysbus_device, NULL); > > @@ -1173,14 +1163,27 @@ static void ppc_spapr_reset(void) > > spapr->rtas_addr =3D rtas_limit - RTAS_MAX_SIZE; > > spapr->fdt_addr =3D spapr->rtas_addr - FDT_MAX_SIZE; > > =20 > > - /* Load the fdt */ > > - spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr, > > - spapr->rtas_size); > > + fdt =3D spapr_build_fdt(spapr, spapr->rtas_addr, spapr->rtas_size); > > =20 > > /* Copy RTAS over */ > > cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob, > > spapr->rtas_size); > > =20 > > + rc =3D fdt_pack(fdt); > > + > > + /* Should only fail if we've built a corrupted tree */ > > + assert(rc =3D=3D 0); > > + > > + if (fdt_totalsize(fdt) > FDT_MAX_SIZE) { > > + error_report("FDT too big ! 0x%x bytes (max is 0x%x)", > > + fdt_totalsize(fdt), FDT_MAX_SIZE); > > + exit(1); > > + } > > + > > + /* Load the fdt */ > > + qemu_fdt_dumpdtb(fdt, fdt_totalsize(fdt)); > > + cpu_physical_memory_write(spapr->fdt_addr, fdt, fdt_totalsize(fdt)= ); > > + >=20 > ... but you did not add a g_free(fdt) here. Is this a memory leak? Yes it is. Thanks for the catch. --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --huq684BweRXVnRxX Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYCW5DAAoJEGw4ysog2bOSaTQP/1PLIAiYv0b2yEhxGTMK7KbD WiiOvyB6ygtl3U8UAu5RO4mkShVc7KE7WPkR/kGjA+0G/hQf/RyOxQhMXTijY7ha dU2Kx3maJyoIAw9Y0k5iPGkrtt6dckII+i7S5V2mHqW4fx+2J+O2CiGL+DX/zrAp gTrXeAHTlTR2gKuGhoMELtl8G7CEjw3bMtnZ+ZrTVMkulwL9t7PwpKJcXeYBMM/O xEfhoqCbALDH/MVSMRGI+cAFGI7AbU4kg/wFltS0sfusboEvlzuMalh8jrKF17mu F0awf7UryRJMpHbY8/E3gKV/AlfPJpMRpWSY3gd64tverFigSXS+T78pGphVQEVg 6WqumRUyKodBcSbo2N5Wi8Orm6+tYdJonnTsjam8ctJYPtHERnowcf9rgK3emXAp u0Ejvdeid5P3zGHZsbonEuPMzDDKFjspAAlVzYpt1WjFRg4rPC3tYkvgKi/v8Bye tRthK52NXCuWCJN/wPLQ5l4MJiEf50v1HgnGlYgZBYKyKPf/FP/ujLb4RE4HdrEm XaMzeH6xXQQPCfhf6tkRZElk7gaush+Gm58lhaLTENuko1XFxmG1cDLSM9mne+Qf q4DsnDe1Mw4L84OZwgkZ+SNofoFWlYtlUXM3wW3EOfeXliVjmXivFEXtuE/80hZg wnj2CaVQTBnLgNNSZVSl =2Qm2 -----END PGP SIGNATURE----- --huq684BweRXVnRxX--