[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/4] spapr: Extend rtas-blob
From: |
David Gibson |
Subject: |
Re: [Qemu-devel] [PATCH 1/4] spapr: Extend rtas-blob |
Date: |
Thu, 12 Nov 2015 22:53:16 +1100 |
User-agent: |
Mutt/1.5.23 (2015-06-09) |
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 <address@hidden>
> > ---
> > 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