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

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

[Philippe Mathieu-Daudé]

Following the discussion of Igor's patch "hppa: allow max
ram size upto 4Gb" [*], I tried to simplify the current
code so Igor's series doesn't change the CLI with this
machine.

Last patch is tagged RFC because it is odd to limit a machine
to 4095MB instead of 4G, but this is a current limitation of
SeaBIOS. This shouldn't be a concern if we could load a Linux
kernel directly.

https://www.mail-archive.com/qemu-devel@nongnu.org/msg667903.html

Philippe Mathieu-Daudé (3):
  hw/hppa/machine: Correctly check the firmware is in PDC range
  hw/hppa/machine: Do not limit the RAM to 3840MB
  hw/hppa/machine: Warn when using more than 4095MB of RAM

 hw/hppa/machine.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

-- 
2.21.1

[PATCH 1/3] hw/hppa/machine: Correctly check the firmware is in PDC range

[Philippe Mathieu-Daudé]

The firmware has to reside in the PDC range. If the Elf file
expects to load it below FIRMWARE_START, it is incorrect,
regardless the RAM size.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Note we define FIRMWARE_END=0xf0800000 but in the specs
the PDC ends at 0xf1000000.
---
 hw/hppa/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 5d0de26140..6775d879f8 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -155,7 +155,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 < FIRMWARE_START || firmware_high >= FIRMWARE_END) {
         error_report("Firmware overlaps with memory or IO space");
         exit(1);
     }
-- 
2.21.1

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

[Philippe Mathieu-Daudé]

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);
+    }
     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);
-- 
2.21.1

[RFC PATCH 3/3] hw/hppa/machine: Warn when using more than 4095MB of RAM

[Philippe Mathieu-Daudé]

We use SeaBIOS as firmware, and pass the RAM size in a 32-bit
wide register. When using 4GB of RAM, the register is truncated
to 0, SeaBIOS is confused and halts the machine:

  $ qemu-system-hppa -m 4g -serial stdio

  SeaBIOS: Machine configured with too little memory (0 MB), minimum is 16 MB.

  SeaBIOS wants SYSTEM HALT.

Display a warning in case the user is not looking at the serial
console.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/hppa/machine.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index d10c967d06..e74aafea2f 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -159,6 +159,10 @@ static void machine_hppa_init(MachineState *machine)
         exit(1);
     }
     g_free(firmware_filename);
+    if (machine->ram_size > 4095 * MiB) {
+        /* FIXME As we use 32-bit registers, 4GiB is truncated to 0 */
+        warn_report("Firmware might misbehave with 4GB of RAM");
+    }
 
     rom_region = g_new(MemoryRegion, 1);
     memory_region_init_ram(rom_region, NULL, "firmware",
-- 
2.21.1

Re: [PATCH 1/3] hw/hppa/machine: Correctly check the firmware is in PDC range

[Helge Deller]

On 08.01.20 19:14, Philippe Mathieu-Daudé wrote:
> The firmware has to reside in the PDC range. If the Elf file
> expects to load it below FIRMWARE_START, it is incorrect,
> regardless the RAM size.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Acked-by: Helge Deller <deller@gmx.de>

> ---
> Note we define FIRMWARE_END=0xf0800000 but in the specs
> the PDC ends at 0xf1000000.
> ---
>  hw/hppa/machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index 5d0de26140..6775d879f8 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -155,7 +155,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 < FIRMWARE_START || firmware_high >= FIRMWARE_END) {
>          error_report("Firmware overlaps with memory or IO space");
>          exit(1);
>      }
>

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

[Helge Deller]

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.
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.

>      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?

> -    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

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

[Philippe Mathieu-Daudé]

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

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

[Igor Mammedov]

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)

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.

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

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

[Philippe Mathieu-Daudé]

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.

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

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

[Igor Mammedov]

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