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