[v2,66/86] ppc/{ppc440_bamboo,sam460x}: drop RAM size fixup
diff mbox series

Message ID 1579100861-73692-67-git-send-email-imammedo@redhat.com
State New
Headers show
Series
  • refactor main RAM allocation to use hostmem backend
Related show

Commit Message

Igor Mammedov Jan. 15, 2020, 3:07 p.m. UTC
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 <imammedo@redhat.com>
---
v2:
  * s/ppc4xx_sdram_adjust/ppc4xx_sdram_prep/
     (BALATON Zoltan <balaton@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: balaton@eik.bme.hu
CC: david@gibson.dropbear.id.au
CC: qemu-ppc@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(-)

Comments

BALATON Zoltan Jan. 15, 2020, 9:33 p.m. UTC | #1
Subject line is still wrong, it's sam460ex not sam460x. Also you change 
ppc:whatever to ppc/whatever here but left : as path separator in all 
other patches. This is not consistent with other commits where the tag in 
the title is usually a path of the changed part or in this case could be 
ppc/ arm/ etc. Colon as path separator looks weird so I think this change 
should be done for all patches to match other commits not just this one,

On Wed, 15 Jan 2020, 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 <imammedo@redhat.com>
> ---
> v2:
>  * s/ppc4xx_sdram_adjust/ppc4xx_sdram_prep/
>     (BALATON Zoltan <balaton@eik.bme.hu>)

Thanks but prep is not a good name in PPC context as one of the arch 
standards is also called that (PowerPC Reference Platform or PReP) so 
using that here is ambiguous. Better spell it out or call it something 
else (such as ppc4xx_sdram_banks).

>  * 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: balaton@eik.bme.hu
> CC: david@gibson.dropbear.id.au
> CC: qemu-ppc@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);

This is certainly better than to just bail out and let the user wonder 
what would work but I've not yet given up to keep current functionality 
(also for other boards where this series would drop it). If you don't like 
a generic callback then how about letting the board optionally specify in 
the machine class next to default_ram_size some constraints such as 
min_ram_size, max_ram_size and an array of valid_ram_sizes (like 
ppc460ex_sdram_bank_sizes[]) that the generic code doing the allocation 
could check and clamp sizes to the nearest valid one emitting a warning 
and only bail out if constraints are not given. This would cover most 
cases expect the e500 where it has an alignment without max so 
representing it as a list of valid values is probably not practical (could 
also have ram_alignment but then a generic callback may be simpler). Would 
that way work for you?

Regards,
BALATON Zoltan

>     }
>
>     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 Jan. 16, 2020, 4:24 a.m. UTC | #2
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 <imammedo@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
> v2:
>   * s/ppc4xx_sdram_adjust/ppc4xx_sdram_prep/
>      (BALATON Zoltan <balaton@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: balaton@eik.bme.hu
> CC: david@gibson.dropbear.id.au
> CC: qemu-ppc@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,
Igor Mammedov Jan. 17, 2020, 10:46 a.m. UTC | #3
On Wed, 15 Jan 2020 22:33:46 +0100 (CET)
BALATON Zoltan <balaton@eik.bme.hu> wrote:

> Subject line is still wrong, it's sam460ex not sam460x. Also you change 
> ppc:whatever to ppc/whatever here but left : as path separator in all 
> other patches. This is not consistent with other commits where the tag in 
> the title is usually a path of the changed part or in this case could be 
> ppc/ arm/ etc. Colon as path separator looks weird so I think this change 
> should be done for all patches to match other commits not just this one,
> 
> On Wed, 15 Jan 2020, 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 <imammedo@redhat.com>
> > ---
> > v2:
> >  * s/ppc4xx_sdram_adjust/ppc4xx_sdram_prep/
> >     (BALATON Zoltan <balaton@eik.bme.hu>)  
> 
> Thanks but prep is not a good name in PPC context as one of the arch 
> standards is also called that (PowerPC Reference Platform or PReP) so 
> using that here is ambiguous. Better spell it out or call it something 
> else (such as ppc4xx_sdram_banks).
Thanks, I'll change it as suggested

> 
> >  * 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: balaton@eik.bme.hu
> > CC: david@gibson.dropbear.id.au
> > CC: qemu-ppc@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);  
> 
> This is certainly better than to just bail out and let the user wonder 
> what would work

v2 basically works the same as original code, except of that 
instead of fixing up ram_size, it ask user to fix CLI to
the same value.

> but I've not yet given up to keep current functionality 
> (also for other boards where this series would drop it). If you don't like 
> a generic callback then how about letting the board optionally specify in 
> the machine class next to default_ram_size some constraints such as 
> min_ram_size, max_ram_size and an array of valid_ram_sizes (like 
> ppc460ex_sdram_bank_sizes[]) that the generic code doing the allocation 
> could check and clamp sizes to the nearest valid one emitting a warning 
> and only bail out if constraints are not given. This would cover most 
> cases expect the e500 where it has an alignment without max so 
> representing it as a list of valid values is probably not practical (could 
> also have ram_alignment but then a generic callback may be simpler). Would 
> that way work for you?

I agree that we need to consolidate ram_size checks,
that would allow to clean up boards that practically do
the mostly the same. In the end it would allow to introspect
supported ram_sizes which would be good for upper layers.

I even started something in that direction but dropped
all of that patches as not directly related to this
series purpose.
Phillipe is also trying to something like this for raspi
board.

As for clamping, I still think that it's not good idea
to accept invalid value and fix it up.
Users should get what they asked for or get a clear error
(it's double true if user is management software as it
won't read any sort of warnings, well not all human users
would head warnings either as far as QEMU continues to run)

Anyways, most maintainers agreed to dropping fixups
and generalizing checks or generalizing fix up for convenience
of users frameworks are out of scope of this series.

Someone can work on these refactorings later on
(assuming someone is enough interested in it)

PS:
If you insist on fixing ram_size up, it would still be possible
to do so locally within board. One can take all RAM that user
provided and then create clamped memory region alias.
It's rather ugly but will do the job without affecting generic
code. (though I wouldn't volunteer to write up such a hack)


> Regards,
> BALATON Zoltan
> 
> >     }
> >
> >     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,
> >  
>
Philippe Mathieu-Daudé Jan. 17, 2020, 3:38 p.m. UTC | #4
On 1/17/20 11:46 AM, Igor Mammedov wrote:
> On Wed, 15 Jan 2020 22:33:46 +0100 (CET)
> BALATON Zoltan <balaton@eik.bme.hu> wrote:
> 
[...]
> v2 basically works the same as original code, except of that
> instead of fixing up ram_size, it ask user to fix CLI to
> the same value.
> 
>> but I've not yet given up to keep current functionality
>> (also for other boards where this series would drop it). If you don't like
>> a generic callback then how about letting the board optionally specify in
>> the machine class next to default_ram_size some constraints such as
>> min_ram_size, max_ram_size and an array of valid_ram_sizes (like
>> ppc460ex_sdram_bank_sizes[]) that the generic code doing the allocation
>> could check and clamp sizes to the nearest valid one emitting a warning
>> and only bail out if constraints are not given. This would cover most
>> cases expect the e500 where it has an alignment without max so
>> representing it as a list of valid values is probably not practical (could
>> also have ram_alignment but then a generic callback may be simpler). Would
>> that way work for you?
> 
> I agree that we need to consolidate ram_size checks,
> that would allow to clean up boards that practically do
> the mostly the same. In the end it would allow to introspect
> supported ram_sizes which would be good for upper layers.
> 
> I even started something in that direction but dropped
> all of that patches as not directly related to this
> series purpose.
> Phillipe is also trying to something like this for raspi
> board.
> 
> As for clamping, I still think that it's not good idea
> to accept invalid value and fix it up.
> Users should get what they asked for or get a clear error
> (it's double true if user is management software as it
> won't read any sort of warnings, well not all human users
> would head warnings either as far as QEMU continues to run)
> 
> Anyways, most maintainers agreed to dropping fixups
> and generalizing checks or generalizing fix up for convenience
> of users frameworks are out of scope of this series.
> 
> Someone can work on these refactorings later on
> (assuming someone is enough interested in it)
> 
> PS:
> If you insist on fixing ram_size up, it would still be possible
> to do so locally within board. One can take all RAM that user
> provided and then create clamped memory region alias.
> It's rather ugly but will do the job without affecting generic
> code. (though I wouldn't volunteer to write up such a hack)

I think from the virtualization point of view, you don't want to 
allocate unused resources. Is that why you say it is "ugly"?

 From the emulation point of view, it is easier to stay as close as 
possible to the hardware, where sometimes resources are wasted to 
simplify the design (or reduce production delays).
If it matches hardware, clamping doesn't look "ugly" to me.
Igor Mammedov Jan. 17, 2020, 4:39 p.m. UTC | #5
On Fri, 17 Jan 2020 16:38:02 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/17/20 11:46 AM, Igor Mammedov wrote:
> > On Wed, 15 Jan 2020 22:33:46 +0100 (CET)
> > BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >   
> [...]
> > v2 basically works the same as original code, except of that
> > instead of fixing up ram_size, it ask user to fix CLI to
> > the same value.
> >   
> >> but I've not yet given up to keep current functionality
> >> (also for other boards where this series would drop it). If you don't like
> >> a generic callback then how about letting the board optionally specify in
> >> the machine class next to default_ram_size some constraints such as
> >> min_ram_size, max_ram_size and an array of valid_ram_sizes (like
> >> ppc460ex_sdram_bank_sizes[]) that the generic code doing the allocation
> >> could check and clamp sizes to the nearest valid one emitting a warning
> >> and only bail out if constraints are not given. This would cover most
> >> cases expect the e500 where it has an alignment without max so
> >> representing it as a list of valid values is probably not practical (could
> >> also have ram_alignment but then a generic callback may be simpler). Would
> >> that way work for you?  
> > 
> > I agree that we need to consolidate ram_size checks,
> > that would allow to clean up boards that practically do
> > the mostly the same. In the end it would allow to introspect
> > supported ram_sizes which would be good for upper layers.
> > 
> > I even started something in that direction but dropped
> > all of that patches as not directly related to this
> > series purpose.
> > Phillipe is also trying to something like this for raspi
> > board.
> > 
> > As for clamping, I still think that it's not good idea
> > to accept invalid value and fix it up.
> > Users should get what they asked for or get a clear error
> > (it's double true if user is management software as it
> > won't read any sort of warnings, well not all human users
> > would head warnings either as far as QEMU continues to run)
> > 
> > Anyways, most maintainers agreed to dropping fixups
> > and generalizing checks or generalizing fix up for convenience
> > of users frameworks are out of scope of this series.
> > 
> > Someone can work on these refactorings later on
> > (assuming someone is enough interested in it)
> > 
> > PS:
> > If you insist on fixing ram_size up, it would still be possible
> > to do so locally within board. One can take all RAM that user
> > provided and then create clamped memory region alias.
> > It's rather ugly but will do the job without affecting generic
> > code. (though I wouldn't volunteer to write up such a hack)  
> 
> I think from the virtualization point of view, you don't want to 
> allocate unused resources. Is that why you say it is "ugly"?
> 
>  From the emulation point of view, it is easier to stay as close as 
> possible to the hardware, where sometimes resources are wasted to 
> simplify the design (or reduce production delays).
> If it matches hardware, clamping doesn't look "ugly" to me.
> 

gist of discussion in this case is that:

bamboo HW supports 4 banks of RAM that could be populated by
256/128/64/32/16/8Mb DIMMs

so -m XXX must be multiple of that, otherwise we are not
able distribute all -m specified size between supported banks.

Current patch then would error out with
  "Max 4 banks of 256, 128, 64, 32, 16, 8 MB DIMM/bank supported"
  "Possible valid RAM size: XXX"
where XXX is clamped value original code uses to after fixup

Argument was that it's not convenient for users do what they
would do on real hw (i.e. calculate desired size manually
based on available banks/DIMMs).

I gave an attempt to dumping all possible sizes and it results
in not practically useful unique ~80 entries list, so that idea
also goes down the drain.

I agree that it's not convenient, but I think that human user
can easily adapt to error message and either fix CLI to use
suggested value (i.e. what they are getting now) or pick desired
DIMMs number/size and do the math.
BALATON Zoltan Jan. 17, 2020, 5:32 p.m. UTC | #6
On Fri, 17 Jan 2020, Igor Mammedov wrote:
> On Fri, 17 Jan 2020 16:38:02 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
>> On 1/17/20 11:46 AM, Igor Mammedov wrote:
>>> On Wed, 15 Jan 2020 22:33:46 +0100 (CET)
>>> BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>
>> [...]
>>> v2 basically works the same as original code, except of that
>>> instead of fixing up ram_size, it ask user to fix CLI to
>>> the same value.
>>>
>>>> but I've not yet given up to keep current functionality
>>>> (also for other boards where this series would drop it). If you don't like
>>>> a generic callback then how about letting the board optionally specify in
>>>> the machine class next to default_ram_size some constraints such as
>>>> min_ram_size, max_ram_size and an array of valid_ram_sizes (like
>>>> ppc460ex_sdram_bank_sizes[]) that the generic code doing the allocation
>>>> could check and clamp sizes to the nearest valid one emitting a warning
>>>> and only bail out if constraints are not given. This would cover most
>>>> cases expect the e500 where it has an alignment without max so
>>>> representing it as a list of valid values is probably not practical (could
>>>> also have ram_alignment but then a generic callback may be simpler). Would
>>>> that way work for you?
>>>
>>> I agree that we need to consolidate ram_size checks,
>>> that would allow to clean up boards that practically do
>>> the mostly the same. In the end it would allow to introspect
>>> supported ram_sizes which would be good for upper layers.
>>>
>>> I even started something in that direction but dropped
>>> all of that patches as not directly related to this
>>> series purpose.
>>> Phillipe is also trying to something like this for raspi
>>> board.
>>>
>>> As for clamping, I still think that it's not good idea
>>> to accept invalid value and fix it up.
>>> Users should get what they asked for or get a clear error
>>> (it's double true if user is management software as it
>>> won't read any sort of warnings, well not all human users
>>> would head warnings either as far as QEMU continues to run)
>>>
>>> Anyways, most maintainers agreed to dropping fixups
>>> and generalizing checks or generalizing fix up for convenience
>>> of users frameworks are out of scope of this series.
>>>
>>> Someone can work on these refactorings later on
>>> (assuming someone is enough interested in it)
>>>
>>> PS:
>>> If you insist on fixing ram_size up, it would still be possible
>>> to do so locally within board. One can take all RAM that user
>>> provided and then create clamped memory region alias.
>>> It's rather ugly but will do the job without affecting generic
>>> code. (though I wouldn't volunteer to write up such a hack)
>>
>> I think from the virtualization point of view, you don't want to
>> allocate unused resources. Is that why you say it is "ugly"?
>>
>>  From the emulation point of view, it is easier to stay as close as
>> possible to the hardware, where sometimes resources are wasted to
>> simplify the design (or reduce production delays).
>> If it matches hardware, clamping doesn't look "ugly" to me.
>>
>
> gist of discussion in this case is that:
>
> bamboo HW supports 4 banks of RAM that could be populated by
> 256/128/64/32/16/8Mb DIMMs

I care more about sam460ex but limitations are similar (with different 
list of supported DIMMs). This comes from how the SoC's memory controller 
stores memory size (that's similar to how SPD data encodes it) so we can't 
represent different size.

> so -m XXX must be multiple of that, otherwise we are not
> able distribute all -m specified size between supported banks.
>
> Current patch then would error out with
>  "Max 4 banks of 256, 128, 64, 32, 16, 8 MB DIMM/bank supported"
>  "Possible valid RAM size: XXX"
> where XXX is clamped value original code uses to after fixup
>
> Argument was that it's not convenient for users do what they
> would do on real hw (i.e. calculate desired size manually
> based on available banks/DIMMs).

On real hardware one cannot have invalid RAM size because you can only 
plug DIMMs of above sizes so the problem does not happen. The argument was 
more that it worked before and now users may find their previously working 
options to error out. Current version of this patch is better as it gives 
valid option so the user who does not know about SoC limitations can just 
use the printed value and doesn't have to calculate valid value.

> I gave an attempt to dumping all possible sizes and it results
> in not practically useful unique ~80 entries list, so that idea
> also goes down the drain.
>
> I agree that it's not convenient, but I think that human user
> can easily adapt to error message and either fix CLI to use
> suggested value (i.e. what they are getting now) or pick desired
> DIMMs number/size and do the math.

This version is acceptable and I can live with it. If every other board is 
happy with returning error then it's OK to do the same here as long as 
it's easy for users to fix their command line by using the printed value.

Regards,
BALATON Zoltan

Patch
diff mbox series

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,