From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57858) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zwx6N-0007fK-Mf for qemu-devel@nongnu.org; Thu, 12 Nov 2015 14:00:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zwx6I-0004SX-JT for qemu-devel@nongnu.org; Thu, 12 Nov 2015 14:00:07 -0500 Received: from e39.co.us.ibm.com ([32.97.110.160]:34732) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zwx6I-0004SN-Ca for qemu-devel@nongnu.org; Thu, 12 Nov 2015 14:00:02 -0500 Received: from localhost by e39.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 12 Nov 2015 12:00:00 -0700 Message-ID: <5644E1A6.90702@linux.vnet.ibm.com> Date: Fri, 13 Nov 2015 00:29:50 +0530 From: Aravinda Prasad MIME-Version: 1.0 References: <20151111171135.4328.41819.stgit@aravindap> <20151111171515.4328.22622.stgit@aravindap> <56444D32.5070105@redhat.com> In-Reply-To: <56444D32.5070105@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/4] spapr: Extend rtas-blob List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: benh@au1.ibm.com, aik@ozlabs.ru, agraf@suse.de, qemu-devel@nongnu.org, paulus@samba.org, qemu-ppc@nongnu.org, sam.bobroff@au1.ibm.com, david@gibson.dropbear.id.au On Thursday 12 November 2015 01:56 PM, Thomas Huth wrote: > 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. >> >> Signed-off-by: Aravinda Prasad >> --- >> hw/ppc/spapr.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> 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 = get_image_size(filename); >> + >> + /* Resize blob to accommodate error log. */ >> + spapr->rtas_size = TARGET_PAGE_ALIGN(spapr->rtas_size); >> + >> spapr->rtas_blob = 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! > ==> Please add an "assert(spapr->rtas_size < RTAS_ERRLOG_OFFSET)" at the > beginning of your patch. It is good to add an assert. Will include in next revision. > > 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) > ==> Please set the size to a proper value like > RTAS_ERRLOG_OFFSET + sizeof(struct rtas_mc_log) > instead! sure will add it. Regards, Aravinda > > Thomas > -- Regards, Aravinda