On Thu, 2 Jan 2020 16:52:50 +0100 (CET)
BALATON Zoltan <bala...@eik.bme.hu> wrote:
> On Thu, 2 Jan 2020, Igor Mammedov wrote:
> > On Wed, 1 Jan 2020 12:54:37 +0100 (CET)
> > BALATON Zoltan <bala...@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 <imamm...@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
>
