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

[Philippe Mathieu-Daudé]

On 1/2/20 4:35 PM, Igor Mammedov wrote:
> On Thu, 2 Jan 2020 15:45:55 +0100
> Philippe Mathieu-Daudé <address@hidden> wrote:
>
> > On 1/2/20 3:41 PM, Igor Mammedov wrote:
> > > 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)
> >
> > Going from 3841MB to 4GB is not a breaking change, and 4GB is correct I
> under breaking change, I've meant exit with error when it used to work with
> any values.
>
> But the point in removing fixups to make user aware of valid input and
> make clear what's going on when someone reads the code.
>
> QEMU code is already too complex to read, so I'd avoid supporting values
> that are not really supported by current impl. just for the sake of convenience.
> (a wrapper script could do that RAM size clamping if it is really necessary)
>
> > think. The Raspberry Pi machines do the same (allocate more RAM than the
> > CPU is able to access, and DMA is able to use this extra RAM).
> In current impl. raspi accepts RAM upto 1Gb maps all of it and reports
> that size to the guest. QEMU doesn't mask or waste anything.
> If we where to rise limit upto 4Gb, then there should be the code that
> would inform and allow guest utilize that.

For the raspi2/3, 16MB are lost and we don't complain neither change the 
ram_size, see the bcm2835-peripherals block overmapping the ram:

$ qemu-system-aarch64 -M raspi3 -monitor stdio -S
QEMU 3.1.1 monitor - type 'help' for more information
(qemu) info mtree
address-space: memory
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    0000000000000000-000000003fffffff (prio 0, ram): ram
    000000003f000000-000000003fffffff (prio 1, i/o): bcm2835-peripherals
      000000003f007000-000000003f007fff (prio 0, i/o): bcm2835-dma
      000000003f00b200-000000003f00b3ff (prio 0, i/o): bcm2835-ic
      000000003f00b800-000000003f00bbff (prio 0, i/o): bcm2835-mbox
      000000003f104000-000000003f10400f (prio 0, i/o): bcm2835-rng
      000000003f200000-000000003f200fff (prio 0, i/o): bcm2835_gpio
      000000003f201000-000000003f201fff (prio 0, i/o): pl011
      000000003f202000-000000003f202fff (prio 0, i/o): bcm2835-sdhost
      000000003f215000-000000003f2150ff (prio 0, i/o): bcm2835-aux
      000000003f300000-000000003f3000ff (prio 0, i/o): sdhci
      000000003fe05000-000000003fe050ff (prio 0, i/o): bcm2835-dma-chan15
    0000000040000000-00000000400000ff (prio 0, i/o): bcm2836-control