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

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

[Philippe Mathieu-Daudé]

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);
+    }
     for (i = 0; i < nr_banks; i++) {
         for (j = 0; sdram_bank_sizes[j] != 0; j++) {
             bank_size = sdram_bank_sizes[j];
-- 
2.21.3

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

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];
>

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

[Markus Armbruster]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> 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?

The commit message doesn't really tell.  It should.

I suspect this new check is redundant.  What am I missing?

The loop that follows it finds a split of @ram's size guided by
@nr_banks and sdram_bank_sizes[].  The conditional after the loop fails
the function when no such split can be found.

In other words, the function fails unless @ram's size is the sum of
@nr_banks elements of sdram_bank_sizes[].

The actual sdram_bank_sizes[] contain only powers of two.  Anything else
would be deeply weird.

ctpop64(size_left) > nr_banks is weaker than that, isn't it?

To prove me wrong, show me a scenario where the new check fails, and the
existing check doesn't.

>                               Is it a good idea to exit() from a
> helper?

Depends on what exactly is being helped.

>         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.

Complicate your helper when you genuinely need to.  But YAGNI.

>                That exit uses EXIT_FAILURE constant.)

I consider exit(EXIT_FAILURE) a case of portability virtue signalling ;)

But yes, local consistency should be maintained.

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