Re: [PATCH 66/86] ppc:ppc440_bamboo/sam460ex: drop RAM size fixup

[BALATON Zoltan]

On Thu, 2 Jan 2020, Igor Mammedov wrote:
> On Wed, 1 Jan 2020 12:54:37 +0100 (CET)
> BALATON Zoltan <address@hidden> wrote:
> > On Tue, 31 Dec 2019, Igor Mammedov wrote:
> > > If user provided non-sense RAM size, board will complain and
> > > continue running with max RAM size supported.
> > > 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.
> > >
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > > include/hw/ppc/ppc4xx.h |  9 ++++-----
> > > hw/ppc/ppc440_bamboo.c  | 11 ++++-------
> > > hw/ppc/ppc4xx_devs.c    | 26 ++++++++++++++++----------
> > > hw/ppc/sam460ex.c       |  5 ++---
> > > 4 files changed, 26 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/include/hw/ppc/ppc4xx.h b/include/hw/ppc/ppc4xx.h
> > > index 7d82259..1a28127 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_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[]);
> >
> > With this change this function does not adjust ram size any more so it may
> > need to be renamed, e.g. ppc4xx_sdram_banks or something else.
> >
> > A better patch title may be
> >
> > ppc/{ppc440_bamboo,sam460x}: drop RAM size fixup
> >
> > (or without curly braces at your preference).
> I'll rename and use this subj as you suggest on v2.
>
> > This is inconvenient for the user because it worked whatever number
> > they've given but now they have to do the math. So it suggests that what
> > you're replacing this with may not support all the existing use cases. If
> > that can't be fixed to allow checking and changing ram size (maybe via a
> > callback in board code similar to above adjust function returning adjusted
> > size) it may be OK to drop this convenience for the sake of cleaning up
> > code elsewhere.
>
> There were few boards that did fix up and in all cases it was to cover up
> invalid CLI input.
> Creating callback for fixing user mistake doesn't seems to me justified,
> I'd much prefer to have a hard error and consistent behavior across all
> the boards versus being lax on error checking.
>
> [...]
>
>
> > > @@ -699,10 +698,19 @@ 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);
> > > +        char *s = g_strdup("");
> > > +        for (i = 0; sdram_bank_sizes[i]; i++) {
> > > +            char *t = g_strdup_printf("%s%" PRIi64 "%s", s, 
> > > sdram_bank_sizes[i],
> > > +                                      sdram_bank_sizes[i + 1] ? " ," : "");
> > > +            g_free(s);
> > > +            s = t;
> > > +        }
> > > +        error_report("Invalid RAM size, unable to fit all RAM into RAM banks"
> > > +                     " (unassigned RAM: %" PRIi64 ")",  size_left);
> > > +        error_report("Supported: %d banks and sizes/bank: %s", nr_banks, s);
>
> Do you have any suggestions how to make error message better?
> (maybe do calculation here and dump all valid -m variants instead of 
> "#bank,size/bank")

Listing the valid values would certainly help users who don't know what 
the constraints of the SoC or SPD ROMs are (which I think most users don't 
have a clue about and we should not expect them to know). I've also seen 
similar concerns in another response for hppa machines so maybe having a 
callback to allow adjusting user input to board constraints is not a bad 
idea. It's not lax error checking but convenience for the average user 
where board has specific constraints and cannot handle any mem size.

Regards,
BALATON Zoltan