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

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

On Wed, Jan 8, 2020 at 10:39 PM Helge Deller <deller@gmx.de> wrote:
> On 08.01.20 19:14, Philippe Mathieu-Daudé 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>
> > ---
> >  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) {
> > +        error_report("RAM size of 4GB or more is not supported");
> > +        exit(EXIT_FAILURE);
> > +    }
>
> My suggestion is to initially then limit it to max. 3GB, e.g.
> > +    if (machine->ram_size > 3 * GiB) {
> > +        error_report("RAM size of 3GB or more is not supported");
> > +        exit(EXIT_FAILURE);
>
> That way you don't need to work around the 4GB SeaBIOS limitation
> in your other RFC patch.

If you are happy with this outcome, I'm happy too, this is simpler :)

> So, people can start it with:
> qemu-system-hppa -m 3g -serial stdio
>
> Later then we can fix SeaBIOS, at least if 64bit support gets added later on.

Agreed.

> >      ram_region = g_new(MemoryRegion, 1);
> >      memory_region_allocate_system_memory(ram_region, OBJECT(machine),
> >                                           "ram", ram_size);
>
> ^^^ here is still "ram_size". Do you need to change it?

No, because it is not modified, it still contains the size requested
from the user, and the size has been validated.
Hopefully it will simplify Igor series.

> > -    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);
> >
>
> Helge