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

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 2 Jan 2020 16:49:00 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/2/20 4:08 PM, Igor Mammedov wrote:
> > On Thu, 2 Jan 2020 15:17:14 +0100
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >   
> >> On 1/2/20 3:12 PM, Igor Mammedov wrote:  
> >>> On Thu, 2 Jan 2020 13:06:33 +0100
> >>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >>>      
> >>>> On 1/2/20 12:31 PM, Helge Deller 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 <imammedo@redhat.com>
> >>>>>>> ---
> >>>>>>>      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.
> >>>>> Even on a phyiscal machine you can only add DIMMs of sizes 2GB, 3GB or 4GB,
> >>>>> but not "3841MB".  
> >>>>
> >>>> Thanks for the explanation. This deserves a comment in the source file
> >>>> IMHO (and displaying a warning to the user that the behavior is changed).
> >>>>
> >>>> I understand the CPU can't access this DRAM area because the ROM is
> >>>> mapped there. What about other devices, can they do DMA access to it?
> >>>>
> >>>> Igor: If this complicates your series too much, I think we can directly
> >>>> allocate up-to 4GiB and not worry about the 256MiB lost.  
> >>>
> >>> Do you mean
> >>> s/"RAM size more than %d is not supported"/"RAM size more than 4Gb is not supported"/  
> >>
> >> Works for me! You can keep my R-b with this change, thanks.  
> > 
> > Well, it's not that simple.
> > Do we map whole 4G in address space, if yes then we have to "unbreak"
> > firmware mapping and use overlap mapping or do we map only portion of it?
> > Both would make code more confusing and all for the sake of user convenience
> > so they won't have to change their CLI?  
> 
> I was thinking about this patch:
it probably should be a bit more complicated.

> 
> -- >8 --  
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index 5d0de26140..fa0b6a9536 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -91,15 +91,16 @@ static void machine_hppa_init(MachineState *machine)
>       }
> 
>       /* Limit main memory. */
> -    if (ram_size > FIRMWARE_START) {
> -        machine->ram_size = ram_size = FIRMWARE_START;
> +    if (ram_size > 4 * GiB) {
> +        error_report("Can not model more than 4GB of RAM");
> +        exit(EXIT_FAILURE);
>       }
> 
>       /* Main memory region. */
>       ram_region = g_new(MemoryRegion, 1);
>       memory_region_allocate_system_memory(ram_region, OBJECT(machine),
>                                            "ram", ram_size);
> -    memory_region_add_subregion(addr_space, 0, ram_region);
> +    memory_region_add_subregion_overlap(addr_space, 0, ram_region, -1);
> 
>       /* Init Dino (PCI host bus chip).  */
>       pci_bus = dino_init(addr_space, &rtc_irq, &serial_irq);

Ok, I give up on trying to convince you to avoid relaxing error check
but I'd make it an extra patch on top of "[PATCH 43/86] hppa: drop RAM size fixup"
as it's a separate change.

> ---
> 
> $ hppa-softmmu/qemu-system-hppa -m 3840m -S -monitor stdio
> QEMU 4.2.50 monitor - type 'help' for more information
> (qemu) info mtree
> address-space: memory
>    0000000000000000-ffffffffffffffff (prio 0, i/o): system
>      0000000000000000-00000000efffffff (prio -1, ram): ram
>      00000000f0000000-00000000f07fffff (prio 0, ram): firmware
>      00000000f9000000-00000000f90007ff (prio 0, i/o): isa-io
>        00000000f9000020-00000000f9000021 (prio 0, i/o): pic
>        00000000f9000070-00000000f9000071 (prio 0, i/o): rtc
>          00000000f9000070-00000000f9000070 (prio 0, i/o): rtc-index
>        00000000f90000a0-00000000f90000a1 (prio 0, i/o): pic
>        00000000f90004d0-00000000f90004d0 (prio 0, i/o): elcr
>        00000000f90004d1-00000000f90004d1 (prio 0, i/o): elcr
>      00000000fff80000-00000000fff80fff (prio 0, i/o): dino
>        00000000fff80064-00000000fff80067 (prio 0, i/o): pci-conf-idx
>        00000000fff80068-00000000fff8006b (prio 0, i/o): pci-conf-data
>      00000000fff83800-00000000fff83807 (prio 0, i/o): serial
>      00000000fffb0000-00000000fffb0003 (prio 0, i/o): cpu0-io-eir
>