On Wed, Jan 15, 2020 at 04:07:21PM +0100, Igor Mammedov wrote: > If user provided non-sense RAM size, board will complain and > continue running with max RAM size supported or sometimes > crash like this: > %QEMU -M bamboo -m 1 > exec.c:1926: find_ram_offset: Assertion `size != 0' failed. > Aborted (core dumped) > Also RAM is going to be allocated by generic code, so it won't be > possible for board to fix things up for user. > > Make it error message and exit to force user fix CLI, > instead of accepting non-sense CLI values. > That also fixes crash issue, since wrongly calculated size > isn't used to allocate RAM > > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
Acked-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> v2:
> * s/ppc4xx_sdram_adjust/ppc4xx_sdram_prep/
> (BALATON Zoltan <bala...@eik.bme.hu>)
> * print possible valid ram size id not all RAM was distributed
> * initialize ram_bases/ram_bases at the same time we are checking
> that user supplied RAM would fit available banks and drop nested
> loop that were duplicating the same calculations.
> * coincidentally fix crash when -m is less than minimal bank size
>
> CC: bala...@eik.bme.hu
> CC: da...@gibson.dropbear.id.au
> CC: qemu-...@nongnu.org
> ---
> include/hw/ppc/ppc4xx.h | 9 ++++----
> hw/ppc/ppc440_bamboo.c | 11 ++++------
> hw/ppc/ppc4xx_devs.c | 56
> +++++++++++++++++++++++++------------------------
> hw/ppc/sam460ex.c | 5 ++---
> 4 files changed, 39 insertions(+), 42 deletions(-)
>
> diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
> index 7d82259..103c875 100644
> --- a/include/hw/ppc/ppc4xx.h
> +++ b/include/hw/ppc/ppc4xx.h
> @@ -42,11 +42,10 @@ enum {
> qemu_irq *ppcuic_init (CPUPPCState *env, qemu_irq *irqs,
> uint32_t dcr_base, int has_ssr, int has_vr);
>
> -ram_addr_t ppc4xx_sdram_adjust(ram_addr_t ram_size, int nr_banks,
> - MemoryRegion ram_memories[],
> - hwaddr ram_bases[],
> - hwaddr ram_sizes[],
> - const ram_addr_t sdram_bank_sizes[]);
> +void ppc4xx_sdram_prep(ram_addr_t ram_size, int nr_banks,
> + MemoryRegion ram_memories[],
> + hwaddr ram_bases[], hwaddr ram_sizes[],
> + const ram_addr_t sdram_bank_sizes[]);
>
> void ppc4xx_sdram_init (CPUPPCState *env, qemu_irq irq, int nbanks,
> MemoryRegion ram_memories[],
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index b782641..c162598 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -158,7 +158,6 @@ static void main_cpu_reset(void *opaque)
>
> static void bamboo_init(MachineState *machine)
> {
> - ram_addr_t ram_size = machine->ram_size;
> const char *kernel_filename = machine->kernel_filename;
> const char *kernel_cmdline = machine->kernel_cmdline;
> const char *initrd_filename = machine->initrd_filename;
> @@ -203,10 +202,8 @@ static void bamboo_init(MachineState *machine)
> /* SDRAM controller */
> memset(ram_bases, 0, sizeof(ram_bases));
> memset(ram_sizes, 0, sizeof(ram_sizes));
> - ram_size = ppc4xx_sdram_adjust(ram_size, PPC440EP_SDRAM_NR_BANKS,
> - ram_memories,
> - ram_bases, ram_sizes,
> - ppc440ep_sdram_bank_sizes);
> + ppc4xx_sdram_prep(ram_size, PPC440EP_SDRAM_NR_BANKS, ram_memories,
> + ram_bases, ram_sizes, ppc440ep_sdram_bank_sizes);
> /* XXX 440EP's ECC interrupts are on UIC1, but we've only created UIC0.
> */
> ppc4xx_sdram_init(env, pic[14], PPC440EP_SDRAM_NR_BANKS, ram_memories,
> ram_bases, ram_sizes, 1);
> @@ -268,7 +265,7 @@ static void bamboo_init(MachineState *machine)
> /* Load initrd. */
> if (initrd_filename) {
> initrd_size = load_image_targphys(initrd_filename, RAMDISK_ADDR,
> - ram_size - RAMDISK_ADDR);
> + machine->ram_size - RAMDISK_ADDR);
>
> if (initrd_size < 0) {
> error_report("could not load ram disk '%s' at %x",
> @@ -279,7 +276,7 @@ static void bamboo_init(MachineState *machine)
>
> /* If we're loading a kernel directly, we must load the device tree too.
> */
> if (kernel_filename) {
> - if (bamboo_load_device_tree(FDT_ADDR, ram_size, RAMDISK_ADDR,
> + if (bamboo_load_device_tree(FDT_ADDR, machine->ram_size,
> RAMDISK_ADDR,
> initrd_size, kernel_cmdline) < 0) {
> error_report("couldn't load device tree");
> exit(1);
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index c2e5013..92d33a4 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -673,16 +673,16 @@ void ppc4xx_sdram_init (CPUPPCState *env, qemu_irq irq,
> int nbanks,
> * The 4xx SDRAM controller supports a small number of banks, and each bank
> * must be one of a small set of sizes. The number of banks and the supported
> * sizes varies by SoC. */
> -ram_addr_t ppc4xx_sdram_adjust(ram_addr_t ram_size, int nr_banks,
> - MemoryRegion ram_memories[],
> - hwaddr ram_bases[],
> - hwaddr ram_sizes[],
> - const ram_addr_t sdram_bank_sizes[])
> +void ppc4xx_sdram_prep(ram_addr_t ram_size, int nr_banks,
> + MemoryRegion ram_memories[],
> + hwaddr ram_bases[], hwaddr ram_sizes[],
> + const ram_addr_t sdram_bank_sizes[])
> {
> MemoryRegion *ram = g_malloc0(sizeof(*ram));
> ram_addr_t size_left = ram_size;
> ram_addr_t base = 0;
> ram_addr_t bank_size;
> + int last_bank = 0;
> int i;
> int j;
>
> @@ -690,7 +690,11 @@ ram_addr_t ppc4xx_sdram_adjust(ram_addr_t ram_size, int
> nr_banks,
> for (j = 0; sdram_bank_sizes[j] != 0; j++) {
> bank_size = sdram_bank_sizes[j];
> if (bank_size <= size_left) {
> + ram_bases[i] = base;
> + ram_sizes[i] = bank_size;
> + base += bank_size;
> size_left -= bank_size;
> + last_bank = i;
> }
> }
> if (!size_left) {
> @@ -699,34 +703,32 @@ ram_addr_t ppc4xx_sdram_adjust(ram_addr_t ram_size, int
> nr_banks,
> }
> }
>
> - ram_size -= size_left;
> if (size_left) {
> - error_report("Truncating memory to %" PRId64 " MiB to fit SDRAM"
> - " controller limits", ram_size / MiB);
> + ram_addr_t used_size = ram_size - size_left;
> + GString *s = g_string_new(NULL);
> +
> + for (i = 0; sdram_bank_sizes[i]; i++) {
> + g_string_append_printf(s, "%" PRIi64 "%s",
> + sdram_bank_sizes[i] / MiB,
> + sdram_bank_sizes[i + 1] ? " ," : "");
> + }
> + error_report("Max %d banks of %s MB DIMM/bank supported",
> + nr_banks, s->str);
> + error_report("Possible valid RAM size: %" PRIi64,
> + used_size ? used_size / MiB : sdram_bank_sizes[i - 1] / MiB);
> +
> + g_string_free(s, true);
> + exit(EXIT_FAILURE);
> }
>
> memory_region_allocate_system_memory(ram, NULL, "ppc4xx.sdram",
> ram_size);
>
> - size_left = ram_size;
> - for (i = 0; i < nr_banks && size_left; i++) {
> - for (j = 0; sdram_bank_sizes[j] != 0; j++) {
> - bank_size = sdram_bank_sizes[j];
> -
> - if (bank_size <= size_left) {
> - char name[32];
> - snprintf(name, sizeof(name), "ppc4xx.sdram%d", i);
> - memory_region_init_alias(&ram_memories[i], NULL, name, ram,
> - base, bank_size);
> - ram_bases[i] = base;
> - ram_sizes[i] = bank_size;
> - base += bank_size;
> - size_left -= bank_size;
> - break;
> - }
> - }
> + for (i = 0; i <= last_bank; i++) {
> + char name[32];
> + snprintf(name, sizeof(name), "ppc4xx.sdram%d", i);
> + memory_region_init_alias(&ram_memories[i], NULL, name, ram,
> + ram_bases[i], ram_sizes[i]);
> }
> -
> - return ram_size;
> }
>
>
> /*****************************************************************************/
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 437e214..ec7ac1f 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -324,9 +324,8 @@ static void sam460ex_init(MachineState *machine)
> /* SDRAM controller */
> /* put all RAM on first bank because board has one slot
> * and firmware only checks that */
> - machine->ram_size = ppc4xx_sdram_adjust(machine->ram_size, 1,
> - ram_memories, ram_bases, ram_sizes,
> - ppc460ex_sdram_bank_sizes);
> + ppc4xx_sdram_prep(machine->ram_size, 1, ram_memories, ram_bases,
> ram_sizes,
> + ppc460ex_sdram_bank_sizes);
>
> /* FIXME: does 460EX have ECC interrupts? */
> ppc440_sdram_init(env, SDRAM_NR_BANKS, ram_memories,
--
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
