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

[Igor Mammedov]

On Thu, 2 Jan 2020 18:14:32 +0100
Philippe Mathieu-Daudé <address@hidden> wrote:

> On 1/2/20 5:50 PM, Igor Mammedov wrote:
> > On Thu, 2 Jan 2020 16:49:00 +0100
> > Philippe Mathieu-Daudé <address@hidden> wrote:
> >   
> >> On 1/2/20 4:08 PM, Igor Mammedov wrote:  
> >>> On Thu, 2 Jan 2020 15:17:14 +0100
> >>> Philippe Mathieu-Daudé <address@hidden> wrote:
> >>>      
> >>>> On 1/2/20 3:12 PM, Igor Mammedov wrote:  
> >>>>> On Thu, 2 Jan 2020 13:06:33 +0100
> >>>>> Philippe Mathieu-Daudé <address@hidden> 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 <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.
> >>>>>>> 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.  
> 
> I am simply not understanding what you are suggesting...
> Can you share a diff snippet of what you would prefer?
> 

My preference is to error out with:
  "RAM size more than 3840m is not supported"
and let user fix their CLI