Re: [PATCH] hw/ppc/ppc4xx: Only accept (combination of) pow2 DDR sizes

[BALATON Zoltan]

On Mon, 29 Jun 2020, Philippe Mathieu-Daudé wrote:
> Use popcount instruction to count the number of bits set in
> the RAM size. Allow at most 1 bit for each bank. This avoid
> using invalid hardware configurations.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/ppc/ppc4xx_devs.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index f1651e04d9..c2484a5695 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -687,6 +687,15 @@ void ppc4xx_sdram_banks(MemoryRegion *ram, int nr_banks,
>     int i;
>     int j;
>
> +    if (ctpop64(size_left) > nr_banks) {
> +        if (nr_banks) {
> +            error_report("RAM size must be a power of 2");
> +        } else {
> +            error_report("RAM size must be the combination of %d powers of 2",
> +                         nr_banks);
> +        }
> +        exit(1);

What is this supposed to fix? Is it a good idea to exit() from a helper? I 
don't think so because the board code should be in control in my opinion 
to decide what it can work with or what it cannot handle and wants to 
abort. So maybe it's better to return error in some way and let board code 
handle it. (We already exit from this function but that was added in 
commit a0258e4afa1 when the size fix up was removed due to memdev. That 
exit uses EXIT_FAILURE constant.)

Regards,
BALATON Zoltan

> +    }
>     for (i = 0; i < nr_banks; i++) {
>         for (j = 0; sdram_bank_sizes[j] != 0; j++) {
>             bank_size = sdram_bank_sizes[j];