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

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

[Cédric Le Goater]

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

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 | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 50810df83815..77af846cdfea 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -21,6 +21,7 @@
 #include "qemu-common.h"
 #include "qemu/datadir.h"
 #include "qemu/units.h"
+#include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "sysemu/qtest.h"
 #include "sysemu/sysemu.h"
@@ -725,8 +726,11 @@ static void pnv_init(MachineState *machine)
     DeviceState *dev;
 
     /* allocate RAM */
-    if (machine->ram_size < (1 * GiB)) {
-        warn_report("skiboot may not work with < 1GB of RAM");
+    if (machine->ram_size < mc->default_ram_size) {
+        char *sz = size_to_str(mc->default_ram_size);
+        error_report("Invalid RAM size, should be bigger than %s", sz);
+        g_free(sz);
+        exit(EXIT_FAILURE);
     }
     memory_region_add_subregion(get_system_memory(), 0, machine->ram);
 
@@ -1994,7 +1998,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 v2] ppc/pnv: Set default RAM size to 1 GB

[Greg Kurz]

On Fri, 29 Jan 2021 12:17:19 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> 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
>   #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.
> 
> 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 | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 50810df83815..77af846cdfea 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -21,6 +21,7 @@
>  #include "qemu-common.h"
>  #include "qemu/datadir.h"
>  #include "qemu/units.h"
> +#include "qemu/cutils.h"
>  #include "qapi/error.h"
>  #include "sysemu/qtest.h"
>  #include "sysemu/sysemu.h"
> @@ -725,8 +726,11 @@ static void pnv_init(MachineState *machine)
>      DeviceState *dev;
>  
>      /* allocate RAM */
> -    if (machine->ram_size < (1 * GiB)) {
> -        warn_report("skiboot may not work with < 1GB of RAM");
> +    if (machine->ram_size < mc->default_ram_size) {
> +        char *sz = size_to_str(mc->default_ram_size);
> +        error_report("Invalid RAM size, should be bigger than %s", sz);
> +        g_free(sz);

FWIW you could even not call g_free() at all since this is an
exit path and this code is unlikely to be reused differently,
or stay clean and tag 'sz' with g_autofree as recommended in
CODING_STYLE.

Anyway, this is definitely an improvement so:

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

> +        exit(EXIT_FAILURE);
>      }
>      memory_region_add_subregion(get_system_memory(), 0, machine->ram);
>  
> @@ -1994,7 +1998,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;

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

[Cédric Le Goater]

On 1/29/21 12:41 PM, Greg Kurz wrote:
> On Fri, 29 Jan 2021 12:17:19 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> 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
>>   #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.
>>
>> 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 | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 50810df83815..77af846cdfea 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -21,6 +21,7 @@
>>  #include "qemu-common.h"
>>  #include "qemu/datadir.h"
>>  #include "qemu/units.h"
>> +#include "qemu/cutils.h"
>>  #include "qapi/error.h"
>>  #include "sysemu/qtest.h"
>>  #include "sysemu/sysemu.h"
>> @@ -725,8 +726,11 @@ static void pnv_init(MachineState *machine)
>>      DeviceState *dev;
>>  
>>      /* allocate RAM */
>> -    if (machine->ram_size < (1 * GiB)) {
>> -        warn_report("skiboot may not work with < 1GB of RAM");
>> +    if (machine->ram_size < mc->default_ram_size) {
>> +        char *sz = size_to_str(mc->default_ram_size);
>> +        error_report("Invalid RAM size, should be bigger than %s", sz);
>> +        g_free(sz);
> 
> FWIW you could even not call g_free() at all since this is an
> exit path and this code is unlikely to be reused differently,
> or stay clean and tag 'sz' with g_autofree as recommended in
> CODING_STYLE.

Indeed. I copied that part from another machine and it should
disappear if we introduce a 'min_ram_size' MachineClass attribute.

> Anyway, this is definitely an improvement so:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

Thanks,

C.

 
>> +        exit(EXIT_FAILURE);
>>      }
>>      memory_region_add_subregion(get_system_memory(), 0, machine->ram);
>>  
>> @@ -1994,7 +1998,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;
> 

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

[Philippe Mathieu-Daudé]

On 1/29/21 12:41 PM, Greg Kurz wrote:
> On Fri, 29 Jan 2021 12:17:19 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> 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
>>   #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.
>>
>> 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 | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
...
>>      /* allocate RAM */
>> -    if (machine->ram_size < (1 * GiB)) {
>> -        warn_report("skiboot may not work with < 1GB of RAM");
>> +    if (machine->ram_size < mc->default_ram_size) {
>> +        char *sz = size_to_str(mc->default_ram_size);
>> +        error_report("Invalid RAM size, should be bigger than %s", sz);
>> +        g_free(sz);
> 
> FWIW you could even not call g_free() at all since this is an
> exit path and this code is unlikely to be reused differently,

Well, some tests check for invalid configs, and when run with
--enable-sanitizer they'd complain for memory leak, so better
free IMHO.

> or stay clean and tag 'sz' with g_autofree as recommended in
> CODING_STYLE.

Preferably using g_autofree:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Anyway, this is definitely an improvement so:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3228 bytes --]

On Fri, Jan 29, 2021 at 12:17:19PM +0100, Cédric Le Goater wrote:
65;6201;1c> 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
>   #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.
> 
> 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>

Applied to ppc-for-6.0, thanks.

> ---
>  hw/ppc/pnv.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 50810df83815..77af846cdfea 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -21,6 +21,7 @@
>  #include "qemu-common.h"
>  #include "qemu/datadir.h"
>  #include "qemu/units.h"
> +#include "qemu/cutils.h"
>  #include "qapi/error.h"
>  #include "sysemu/qtest.h"
>  #include "sysemu/sysemu.h"
> @@ -725,8 +726,11 @@ static void pnv_init(MachineState *machine)
>      DeviceState *dev;
>  
>      /* allocate RAM */
> -    if (machine->ram_size < (1 * GiB)) {
> -        warn_report("skiboot may not work with < 1GB of RAM");
> +    if (machine->ram_size < mc->default_ram_size) {
> +        char *sz = size_to_str(mc->default_ram_size);
> +        error_report("Invalid RAM size, should be bigger than %s", sz);
> +        g_free(sz);
> +        exit(EXIT_FAILURE);
>      }
>      memory_region_add_subregion(get_system_memory(), 0, machine->ram);
>  
> @@ -1994,7 +1998,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;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]