Re: [PATCH] hppa: allow max ram size upto 4Gb

[Igor Mammedov]

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

> On 1/2/20 6:08 PM, Igor Mammedov wrote:
> > Previous patch drops silent ram_size fixup and makes
> > QEMU error out with:
> > 
> >   "RAM size more than 3840m is not supported"
> > 
> > when user specified -m X more than supported value.
> > 
> > User shouldn't be bothered with starting QEMU with valid CLI,
> > so for the sake of user convenience to allow using -m 4G vs -m 3840M.
> > 
> > Requested-by: Helge Deller <address@hidden>
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > I dislike it but if you feel it's really necessary feel free to ack it.
> > 
> > should be applied on top of:
> >    "hppa: drop RAM size fixup"
> > ---
> >   hw/hppa/machine.c | 22 ++++++++++++----------
> >   1 file changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> > index ebbf44f..7f8c92f 100644
> > --- a/hw/hppa/machine.c
> > +++ b/hw/hppa/machine.c
> > @@ -54,6 +54,7 @@ static uint64_t cpu_hppa_to_phys(void *opaque, uint64_t 
> > addr)
> >   
> >   static HPPACPU *cpu[HPPA_MAX_CPUS];
> >   static uint64_t firmware_entry;
> > +static ram_addr_t clamped_ram_size;
> >   
> >   static void machine_hppa_init(MachineState *machine)
> >   {
> > @@ -74,8 +75,6 @@ static void machine_hppa_init(MachineState *machine)
> >       long i;
> >       unsigned int smp_cpus = machine->smp.cpus;
> >   
> > -    ram_size = machine->ram_size;
> > -
> >       /* Create CPUs.  */
> >       for (i = 0; i < smp_cpus; i++) {
> >           char *name = g_strdup_printf("cpu%ld-io-eir", i);
> > @@ -90,10 +89,12 @@ static void machine_hppa_init(MachineState *machine)
> >       }
> >   
> >       /* Limit main memory. */
> > -    if (ram_size > FIRMWARE_START) {
> > -        error_report("RAM size more than %d is not supported", 
> > FIRMWARE_START);
> > +    if (machine->ram_size > 4 * GiB) {
> > +        error_report("RAM size more than 4Gb is not supported");
> >           exit(EXIT_FAILURE);
> >       }
> > +    clamped_ram_size = machine->ram_size > FIRMWARE_START ?
> > +        FIRMWARE_START : machine->ram_size;
> >   
> >       memory_region_add_subregion(addr_space, 0, machine->ram);
> >   
> > @@ -151,7 +152,7 @@ static void machine_hppa_init(MachineState *machine)
> >       qemu_log_mask(CPU_LOG_PAGE, "Firmware loaded at 0x%08" PRIx64
> >                     "-0x%08" PRIx64 ", entry at 0x%08" PRIx64 ".\n",
> >                     firmware_low, firmware_high, firmware_entry);
> > -    if (firmware_low < ram_size || firmware_high >= FIRMWARE_END) {
> > +    if (firmware_low < clamped_ram_size || firmware_high >= FIRMWARE_END) {
> >           error_report("Firmware overlaps with memory or IO space");
> >           exit(1);
> >       }
> > @@ -160,7 +161,8 @@ static void machine_hppa_init(MachineState *machine)
> >       rom_region = g_new(MemoryRegion, 1);
> >       memory_region_init_ram(rom_region, NULL, "firmware",
> >                              (FIRMWARE_END - FIRMWARE_START), &error_fatal);
> > -    memory_region_add_subregion(addr_space, FIRMWARE_START, rom_region);
> > +    memory_region_add_subregion_overlap(addr_space, FIRMWARE_START, 
> > rom_region,
> > +                                        1);  
> 
> I don't think this is enough because we also have:
> 
> (qemu) info mtree
> address-space: memory
>    00000000f9000000-00000000f90007ff (prio 0, i/o): isa-io
>    00000000fff80000-00000000fff80fff (prio 0, i/o): dino
>    00000000fff83800-00000000fff83807 (prio 0, i/o): serial
>    00000000fffb0000-00000000fffb0003 (prio 0, i/o): cpu0-io-eir
> 
> Which is why I went the other way around, using prio=-1 for the ram.

True
(you see it just snowball of un-obvious changes for user convenience)

> 
> >   
> >       /* Load kernel */
> >       if (kernel_filename) {
> > @@ -204,7 +206,7 @@ static void machine_hppa_init(MachineState *machine)
> >                  (1) Due to sign-extension problems and PDC,
> >                  put the initrd no higher than 1G.
> >                  (2) Reserve 64k for stack.  */
> > -            initrd_base = MIN(ram_size, 1 * GiB);
> > +            initrd_base = MIN(clamped_ram_size, 1 * GiB);
> >               initrd_base = initrd_base - 64 * KiB;
> >               initrd_base = (initrd_base - initrd_size) & TARGET_PAGE_MASK;
> >   
> > @@ -232,7 +234,7 @@ static void machine_hppa_init(MachineState *machine)
> >        * various parameters in registers. After firmware initialization,
> >        * firmware will start the Linux kernel with ramdisk and cmdline.
> >        */
> > -    cpu[0]->env.gr[26] = ram_size;
> > +    cpu[0]->env.gr[26] = clamped_ram_size;
> >       cpu[0]->env.gr[25] = kernel_entry;
> >   
> >       /* tell firmware how many SMP CPUs to present in inventory table */
> > @@ -255,11 +257,11 @@ static void hppa_machine_reset(MachineState *ms)
> >       }
> >   
> >       /* already initialized by machine_hppa_init()? */
> > -    if (cpu[0]->env.gr[26] == ram_size) {
> > +    if (cpu[0]->env.gr[26] == clamped_ram_size) {
> >           return;
> >       }
> >   
> > -    cpu[0]->env.gr[26] = ram_size;
> > +    cpu[0]->env.gr[26] = clamped_ram_size;
> >       cpu[0]->env.gr[25] = 0; /* no firmware boot menu */
> >       cpu[0]->env.gr[24] = 'c';
> >       /* gr22/gr23 unused, no initrd while reboot. */
> >   
> 
>