From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35366) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwnDK-00034J-Bd for qemu-devel@nongnu.org; Thu, 12 Nov 2015 03:26:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwnDF-0000T6-C2 for qemu-devel@nongnu.org; Thu, 12 Nov 2015 03:26:38 -0500 References: <20151111171135.4328.41819.stgit@aravindap> <20151111171515.4328.22622.stgit@aravindap> From: Thomas Huth Message-ID: <56444D32.5070105@redhat.com> Date: Thu, 12 Nov 2015 09:26:26 +0100 MIME-Version: 1.0 In-Reply-To: <20151111171515.4328.22622.stgit@aravindap> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/4] spapr: Extend rtas-blob List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aravinda Prasad , qemu-ppc@nongnu.org, agraf@suse.de, qemu-devel@nongnu.org Cc: aik@ozlabs.ru, benh@au1.ibm.com, paulus@samba.org, sam.bobroff@au1.ibm.com, david@gibson.dropbear.id.au On 11/11/15 18:15, Aravinda Prasad wrote: > Extend rtas-blob to accommodate error log. Error log > structure is saved in rtas space upon a machine check > exception. >=20 > Signed-off-by: Aravinda Prasad > --- > hw/ppc/spapr.c | 4 ++++ > 1 file changed, 4 insertions(+) >=20 > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 05926a3..b7b9e09 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1556,6 +1556,10 @@ static void ppc_spapr_init(MachineState *machine= ) > exit(1); > } > spapr->rtas_size =3D get_image_size(filename); > + > + /* Resize blob to accommodate error log. */ > + spapr->rtas_size =3D TARGET_PAGE_ALIGN(spapr->rtas_size); > + > spapr->rtas_blob =3D g_malloc(spapr->rtas_size); > if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) = < 0) { > error_report("Could not load LPAR rtas '%s'", filename); Sorry to say that, but this patch is horrible! 1) If the rtas blob ever gets bigger than 512 bytes, we will get "random" corruption of the RTAS code later when an NMI occurs since the mc log is blindly copied into the RTAS area later! =3D=3D> Please add an "assert(spapr->rtas_size < RTAS_ERRLOG_OFFSET)" at = the beginning of your patch. 2) Why resizing with TARGET_PAGE_ALIGN() ? In the very worst case, this would not change the size at all (if the rtas_size is already a multiple of PAGE_SIZE) =3D=3D> Please set the size to a proper value like RTAS_ERRLOG_OFFSET + sizeof(struct rtas_mc_log) instead! Thomas