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

[Thomas Huth <thuth@redhat.com>]

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.

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!

 Thomas