On 1/29/21 12:41 PM, Greg Kurz wrote:
> On Fri, 29 Jan 2021 12:17:19 +0100
> Cédric Le Goater <c...@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 <c...@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 <gr...@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;
>
