On Thu, Nov 12, 2015 at 09:26:26AM +0100, 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 <aravi...@linux.vnet.ibm.com> > > --- > > 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.
The assert is a good idea, although I think the chances of the rtas ever growing beyond its current 20 bytes is just about nil. > 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! That's a good point. -- 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
signature.asc
Description: PGP signature