Re: [Qemu-devel] [PATCH 1/4] spapr: Extend rtas-blob

[Aravinda Prasad <aravinda@linux.vnet.ibm.com>]

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 <aravinda@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.

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