Re: [PATCH 43/86] hppa: drop RAM size fixup

[Igor Mammedov]

On Thu, 2 Jan 2020 12:31:58 +0100
Helge Deller <address@hidden> wrote:

> On 31.12.19 16:44, Philippe Mathieu-Daudé wrote:
> > On 12/31/19 2:03 PM, 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>
> >> ---
> >>   hw/hppa/machine.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> >> index 5d0de26..25f5afc 100644
> >> --- a/hw/hppa/machine.c
> >> +++ b/hw/hppa/machine.c
> >> @@ -92,7 +92,8 @@ static void machine_hppa_init(MachineState *machine)
> >>         /* Limit main memory. */
> >>       if (ram_size > FIRMWARE_START) {
> >> -        machine->ram_size = ram_size = FIRMWARE_START;
> >> +        error_report("RAM size more than %d is not supported", 
> >> FIRMWARE_START);
> >> +        exit(EXIT_FAILURE);  
> >
> > $ qemu-system-hppa -m 3841m
> > qemu-system-hppa: invalid accelerator kvm
> > qemu-system-hppa: falling back to tcg
> > qemu-system-hppa: RAM size more than -268435456 is not supported
> >
> > Instead of using qemu_strtosz_MiB on FIRMWARE_START or unsigned format, we 
> > can simply use "RAM size more than 3840m is not supported". Is that OK with 
> > you?  
> 
> I don't really like that change.
> 
> We currently only emulate a 32-bit system, and for those 4GB is the maximum.
> So, if I start my machine with "qemu-system-hppa -m 4G", the current code
> then automatically uses the maximum possible of 3841MB (which is limited by
> firmware start address).
> I don't expect users to know the excact 3841MB number.
It's annoying to see a error where it used to work before
with no matter what -m value user have used.

But error message tells exact max size one could use,
so user doesn't have to know max, just fix CLI with provided value.

> Even on a phyiscal machine you can only add DIMMs of sizes 2GB, 3GB or 4GB,
> but not "3841MB".
> 
> So, I think that patch is - although it's more correct - not a
> benefit for the end user.
Sure thing that users dislike when we do breaking changes (removing legacy CLI
options, fixups or adding error checks that weren't there before).
But I'd choose correctness (and consistent codebase) vs convenience.
(it's no like we are hiding max from user)


> Helge
>