Re: [PATCH 2/3] hw/hppa/machine: Do not limit the RAM to 3840MB

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 9 Jan 2020 12:09:26 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/9/20 11:15 AM, Igor Mammedov wrote:
> > On Wed,  8 Jan 2020 19:14:24 +0100
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >   
> >> The hardware expects DIMM slots of 1 or 2 GB, allowing up to
> >> 4 GB of memory. Accept the same amount of memory the hardware
> >> can deal with.
> >>
> >> The CPU doesn't have access to the RAM mapped in the
> >> [0xf0000000 - 0xf1000000] range because this is the PDC area
> >> (Processor Dependent Code) where the firmware is loaded.
> >> To keep this region with higher priority than the RAM, lower
> >> the RAM priority. The PDC will overlap it.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>  
> > Looks good to me (since board doesn't fix up ram_size and uses
> > whatever user specified, proper support for 4Gb could be done on top later).
> >   
> >> ---
> >>   hw/hppa/machine.c | 11 +++++------
> >>   1 file changed, 5 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> >> index 6775d879f8..d10c967d06 100644
> >> --- a/hw/hppa/machine.c
> >> +++ b/hw/hppa/machine.c
> >> @@ -90,16 +90,15 @@ static void machine_hppa_init(MachineState *machine)
> >>           g_free(name);
> >>       }
> >>   
> >> -    /* Limit main memory. */
> >> -    if (ram_size > FIRMWARE_START) {
> >> -        machine->ram_size = ram_size = FIRMWARE_START;
> >> -    }
> >> -
> >>       /* Main memory region. */
> >> +    if (machine->ram_size > 4 * GiB) {  
> > Could it break a build on 32-bit mingw host?
> > (machine->ram_size is 32-bit on that host and condition would be
> > always false, tripping -Werror)  
> 
> By following Helge tip to restrict to 3GB, v2 of this series doesn't 
> have this problem :)
> 
> > that's why I've worked around it using local uint64_t in the last version
> >   "[PATCH v3 43/86] hppa: allow max ram size upto 4Gb"
> > coincidentally that would get rid of global ram_size usage
> > and leave only machine->ram_size on this board.  
> 
> Since I was not sure how you wanted to clean this, I haven't modified 
> it. We can add it on top but I'd rather do a whole codebase cleanup.
with 3Gb there is no actual need to do that,
Like you said having a separate tree wide clean up would be a better approach.

> 
> Note: you also need to modify hppa_machine_reset() by using ms->ram_size 
> instead.
> 
> >> +        error_report("RAM size of 4GB or more is not supported");
> >> +        exit(EXIT_FAILURE);
> >> +    }
> >>       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);  
> > 
> >   
>