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>