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 <[email protected]> >> --- >> 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
