1. Patchew
  2. QEMU
  3. View series

[PATCH] pnv/ppc: Set default RAM size to 1 GB

Cédric Le Goater posted 1 patch 3 years, 3 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20210129092936.769412-1-clg@kaod.org
Maintainers: David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>, "Cédric Le Goater" <clg@kaod.org>
hw/ppc/pnv.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] pnv/ppc: Set default RAM size to 1 GB
Posted by Cédric Le Goater 3 years, 3 months ago 
Any value below will result in a skiboot crash :

    [    0.034949905,3] MEM: Partial overlap detected between regions:
    [    0.034959039,3] MEM: ibm,firmware-stacks [0x31c10000-0x3a450000] (new)
    [    0.034968576,3] MEM: ibm,firmware-allocs-memory@0 [0x31c10000-0x38400000]
    [    0.034980367,3] Out of memory adding skiboot reserved areas
    [    0.035074945,3] ***********************************************
    [    0.035093627,3] < assert failed at core/mem_region.c:1129 >
    [    0.035104247,3]     .
    [    0.035108025,3]      .
    [    0.035111651,3]       .
    [    0.035115231,3]         OO__)
    [    0.035119198,3]        <"__/
    [    0.035122980,3]         ^ ^

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 50810df83815..70ce12f6dc73 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1994,7 +1994,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
      * RAM defaults to less than 2048 for 32-bit hosts, and large
      * enough to fit the maximum initrd size at it's load address
      */
-    mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
+    mc->default_ram_size = 1 * GiB;
     mc->default_ram_id = "pnv.ram";
     ispc->print_info = pnv_pic_print_info;
     nc->nmi_monitor_handler = pnv_nmi;
-- 
2.26.2
Re: [PATCH] pnv/ppc: Set default RAM size to 1 GB
Posted by Philippe Mathieu-Daudé 3 years, 3 months ago 
On 1/29/21 10:29 AM, Cédric Le Goater wrote:
> Any value below will result in a skiboot crash :
> 
>     [    0.034949905,3] MEM: Partial overlap detected between regions:
>     [    0.034959039,3] MEM: ibm,firmware-stacks [0x31c10000-0x3a450000] (new)
>     [    0.034968576,3] MEM: ibm,firmware-allocs-memory@0 [0x31c10000-0x38400000]
>     [    0.034980367,3] Out of memory adding skiboot reserved areas
>     [    0.035074945,3] ***********************************************
>     [    0.035093627,3] < assert failed at core/mem_region.c:1129 >
>     [    0.035104247,3]     .
>     [    0.035108025,3]      .
>     [    0.035111651,3]       .
>     [    0.035115231,3]         OO__)
>     [    0.035119198,3]        <"__/
>     [    0.035122980,3]         ^ ^
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  hw/ppc/pnv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 50810df83815..70ce12f6dc73 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1994,7 +1994,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
>       * RAM defaults to less than 2048 for 32-bit hosts, and large
>       * enough to fit the maximum initrd size at it's load address
>       */
> -    mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
> +    mc->default_ram_size = 1 * GiB;

Maybe also just in case add before the definitions:

  QEMU_BUILD_BUG_ON(INITRD_LOAD_ADDR + INITRD_MAX_SIZE > 1 * GiB);

?

>      mc->default_ram_id = "pnv.ram";
>      ispc->print_info = pnv_pic_print_info;
>      nc->nmi_monitor_handler = pnv_nmi;
>
Re: [PATCH] pnv/ppc: Set default RAM size to 1 GB
Posted by Greg Kurz 3 years, 3 months ago 
On Fri, 29 Jan 2021 10:39:12 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/29/21 10:29 AM, Cédric Le Goater wrote:
> > Any value below will result in a skiboot crash :
> > 
> >     [    0.034949905,3] MEM: Partial overlap detected between regions:
> >     [    0.034959039,3] MEM: ibm,firmware-stacks [0x31c10000-0x3a450000] (new)
> >     [    0.034968576,3] MEM: ibm,firmware-allocs-memory@0 [0x31c10000-0x38400000]
> >     [    0.034980367,3] Out of memory adding skiboot reserved areas
> >     [    0.035074945,3] ***********************************************
> >     [    0.035093627,3] < assert failed at core/mem_region.c:1129 >
> >     [    0.035104247,3]     .
> >     [    0.035108025,3]      .
> >     [    0.035111651,3]       .
> >     [    0.035115231,3]         OO__)
> >     [    0.035119198,3]        <"__/
> >     [    0.035122980,3]         ^ ^
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > ---
> >  hw/ppc/pnv.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 50810df83815..70ce12f6dc73 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -1994,7 +1994,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
> >       * RAM defaults to less than 2048 for 32-bit hosts, and large
> >       * enough to fit the maximum initrd size at it's load address
> >       */
> > -    mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
> > +    mc->default_ram_size = 1 * GiB;
> 
> Maybe also just in case add before the definitions:
> 
>   QEMU_BUILD_BUG_ON(INITRD_LOAD_ADDR + INITRD_MAX_SIZE > 1 * GiB);
> 

or

    mc->default_ram_size = MAX(1 * GiB, INITRD_LOAD_ADDR + INITRD_MAX_SIZE)

> ?
> 
> >      mc->default_ram_id = "pnv.ram";
> >      ispc->print_info = pnv_pic_print_info;
> >      nc->nmi_monitor_handler = pnv_nmi;
> > 
>
Re: [PATCH] pnv/ppc: Set default RAM size to 1 GB
Posted by Cédric Le Goater 3 years, 3 months ago 
On 1/29/21 11:11 AM, Greg Kurz wrote:
> On Fri, 29 Jan 2021 10:39:12 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
>> On 1/29/21 10:29 AM, Cédric Le Goater wrote:
>>> Any value below will result in a skiboot crash :
>>>
>>>     [    0.034949905,3] MEM: Partial overlap detected between regions:
>>>     [    0.034959039,3] MEM: ibm,firmware-stacks [0x31c10000-0x3a450000] (new)
>>>     [    0.034968576,3] MEM: ibm,firmware-allocs-memory@0 [0x31c10000-0x38400000]
>>>     [    0.034980367,3] Out of memory adding skiboot reserved areas
>>>     [    0.035074945,3] ***********************************************
>>>     [    0.035093627,3] < assert failed at core/mem_region.c:1129 >
>>>     [    0.035104247,3]     .
>>>     [    0.035108025,3]      .
>>>     [    0.035111651,3]       .
>>>     [    0.035115231,3]         OO__)
>>>     [    0.035119198,3]        <"__/
>>>     [    0.035122980,3]         ^ ^
>>>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>  hw/ppc/pnv.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>> index 50810df83815..70ce12f6dc73 100644
>>> --- a/hw/ppc/pnv.c
>>> +++ b/hw/ppc/pnv.c
>>> @@ -1994,7 +1994,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
>>>       * RAM defaults to less than 2048 for 32-bit hosts, and large
>>>       * enough to fit the maximum initrd size at it's load address
>>>       */
>>> -    mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
>>> +    mc->default_ram_size = 1 * GiB;
>>
>> Maybe also just in case add before the definitions:
>>
>>   QEMU_BUILD_BUG_ON(INITRD_LOAD_ADDR + INITRD_MAX_SIZE > 1 * GiB);
>>
> 
> or
> 
>     mc->default_ram_size = MAX(1 * GiB, INITRD_LOAD_ADDR + INITRD_MAX_SIZE)


The memory layout of the PowerNV machine is defined as :

  #define KERNEL_LOAD_BASE	((void *)0x20000000)
  #define KERNEL_LOAD_SIZE	0x08000000

  #define INITRAMFS_LOAD_BASE	KERNEL_LOAD_BASE + KERNEL_LOAD_SIZE
  #define INITRAMFS_LOAD_SIZE	0x08000000

  #define SKIBOOT_BASE		0x30000000

  /* bunch of areas: head, console buffers, TCEs, etc  */
  #define SKIBOOT_SIZE		0x01c10000
  #define CPU_STACKS_BASE	(SKIBOOT_BASE + SKIBOOT_SIZE)

  #define STACK_SHIFT		15
  #define STACK_SIZE		(1 << STACK_SHIFT)

The overall size of the CPU stacks is (max PIR + 1) * 32K and the 
machine easily reaches 800MB of minimum required RAM.

I don't remember where INITRD_LOAD_ADDR + INITRD_MAX_SIZE came from
but it is a bit bogus.

For now, I will send a v2 with a stronger check similar to what other 
machines do. May be we should consider adding a 'min_ram_size' in
MachineClass. 

Thanks,

C.
Re: [PATCH] pnv/ppc: Set default RAM size to 1 GB
Posted by David Gibson 3 years, 3 months ago 
On Fri, Jan 29, 2021 at 11:11:15AM +0100, Greg Kurz wrote:
> On Fri, 29 Jan 2021 10:39:12 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
> > On 1/29/21 10:29 AM, Cédric Le Goater wrote:
> > > Any value below will result in a skiboot crash :
> > > 
> > >     [    0.034949905,3] MEM: Partial overlap detected between regions:
> > >     [    0.034959039,3] MEM: ibm,firmware-stacks [0x31c10000-0x3a450000] (new)
> > >     [    0.034968576,3] MEM: ibm,firmware-allocs-memory@0 [0x31c10000-0x38400000]
> > >     [    0.034980367,3] Out of memory adding skiboot reserved areas
> > >     [    0.035074945,3] ***********************************************
> > >     [    0.035093627,3] < assert failed at core/mem_region.c:1129 >
> > >     [    0.035104247,3]     .
> > >     [    0.035108025,3]      .
> > >     [    0.035111651,3]       .
> > >     [    0.035115231,3]         OO__)
> > >     [    0.035119198,3]        <"__/
> > >     [    0.035122980,3]         ^ ^
> > > 
> > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > ---
> > >  hw/ppc/pnv.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > > index 50810df83815..70ce12f6dc73 100644
> > > --- a/hw/ppc/pnv.c
> > > +++ b/hw/ppc/pnv.c
> > > @@ -1994,7 +1994,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
> > >       * RAM defaults to less than 2048 for 32-bit hosts, and large
> > >       * enough to fit the maximum initrd size at it's load address
> > >       */
> > > -    mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
> > > +    mc->default_ram_size = 1 * GiB;
> > 
> > Maybe also just in case add before the definitions:
> > 
> >   QEMU_BUILD_BUG_ON(INITRD_LOAD_ADDR + INITRD_MAX_SIZE > 1 * GiB);
> > 
> 
> or
> 
>     mc->default_ram_size = MAX(1 * GiB, INITRD_LOAD_ADDR +
>     INITRD_MAX_SIZE)

I prefer the BUILD_BUG_ON approach, less mysterious effects of one
setting on another.

> 
> > ?
> > 
> > >      mc->default_ram_id = "pnv.ram";
> > >      ispc->print_info = pnv_pic_print_info;
> > >      nc->nmi_monitor_handler = pnv_nmi;
> > > 
> > 
> 

-- 
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
Re: [PATCH] pnv/ppc: Set default RAM size to 1 GB
Posted by Greg Kurz 3 years, 3 months ago 
On Fri, 29 Jan 2021 10:29:36 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> Any value below will result in a skiboot crash :
> 
>     [    0.034949905,3] MEM: Partial overlap detected between regions:
>     [    0.034959039,3] MEM: ibm,firmware-stacks [0x31c10000-0x3a450000] (new)
>     [    0.034968576,3] MEM: ibm,firmware-allocs-memory@0 [0x31c10000-0x38400000]
>     [    0.034980367,3] Out of memory adding skiboot reserved areas
>     [    0.035074945,3] ***********************************************
>     [    0.035093627,3] < assert failed at core/mem_region.c:1129 >
>     [    0.035104247,3]     .
>     [    0.035108025,3]      .
>     [    0.035111651,3]       .
>     [    0.035115231,3]         OO__)
>     [    0.035119198,3]        <"__/
>     [    0.035122980,3]         ^ ^
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/pnv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 50810df83815..70ce12f6dc73 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1994,7 +1994,7 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
>       * RAM defaults to less than 2048 for 32-bit hosts, and large
>       * enough to fit the maximum initrd size at it's load address
>       */
> -    mc->default_ram_size = INITRD_LOAD_ADDR + INITRD_MAX_SIZE;
> +    mc->default_ram_size = 1 * GiB;
>      mc->default_ram_id = "pnv.ram";
>      ispc->print_info = pnv_pic_print_info;
>      nc->nmi_monitor_handler = pnv_nmi;

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.