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

[Thomas Huth]

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 
> ---
>  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

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

[David Gibson]

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 
> > ---
> >  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

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

[Aravinda Prasad]

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 
>> ---
>>  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

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

[Aravinda Prasad]

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 
---
 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);

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

[David Gibson]

On Wed, Nov 11, 2015 at 10:45:15PM +0530, 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 
Reviewed-by: David Gibson 

> ---
>  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);
> 

-- 
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