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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 2 Jan 2020 16:52:50 +0100 (CET)
BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On Thu, 2 Jan 2020, Igor Mammedov wrote:
> > On Wed, 1 Jan 2020 12:54:37 +0100 (CET)
> > BALATON Zoltan <balaton@eik.bme.hu> 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 <imammedo@redhat.com>
> >>> ---
> >>> 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 
ok, I'll go ahead with it.

> 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.
It could be usefull to generalize and probably introspect valid/supported
RAM sizes but I doubt it would be easy to sell a callback for fixing up
invalid user input vs just a hard error.
Anyways it looks out of scope of this series and could be done on top if
there is demand for that.

> Regards,
> BALATON Zoltan
>