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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 1/3/20 10:54 AM, Igor Mammedov wrote:
> On Thu, 2 Jan 2020 21:22:12 +0100
> Helge Deller <deller@gmx.de> wrote:
> 
>> On 02.01.20 18:46, 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 allow using -m 4G vs -m 3840M.
>>>
>>> Requested-by: Helge Deller <deller@gmx.de>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> v2:
>>>    - make main ram -1 prio, so it wouldn't conflict with other regions
>>>      starting from 0xf9000000
>>>
>>> I dislike it but if you feel it's really necessary feel free to ack it.

Hard to find the v2 buried in the other series with my email client.

>>>
>>> should be applied on top of:
>>>    "hppa: drop RAM size fixup"
>>
>> Hello Igor,
>> I appreciate that you are trying to make it more cleaner.
>> But, can't you merge both of your patches to one patch?
>> Right now you have one patch "hppa: drop RAM size fixup", which is
>> what I think is wrong. Then you add another one which somehow
>> fixes it up again and adds other stuff.
> 1st patch bring it in line with other boards adding
> proper error check but without changing RAM size.
> While 2nd is changing device model (mapped RAM size) and
> clearly documents that it's a hack for user convenience,
> Hence I'd prefer to keep both separated.
> 
>> Having everything in one single patch makes your full change more
>> understandable.
>>
>> Is it necessary to introduce clamped_ram_size and not continue with
>> ram_size (even if you would add it as static local variable)?
> it's necessary since ram_size is global which should be kept == MachineState::ram_size.
> Later on I plan to remove the former altogether and maybe
> MachineState::ram_size aa well, since it could be read with
> memory_region_size(MachineState::ram).

Why insist on clamping the ram? We recommend to model what the hardware 
does, and the hardware uses a full DIMM of DRAM, so 4GB, not less.

What are the new problem introduced by using 4GB? I only see advantages 
doing so. This doesn't break your series. This doesn't break the CLI.
Am I missing something?

>>> ---
>>>   hw/hppa/machine.c | 21 +++++++++++----------
>>>   1 file changed, 11 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
>>> index ebbf44f..0302983 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,12 +89,14 @@ 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);
>>> +    memory_region_add_subregion_overlap(addr_space, 0, machine->ram, -1);
>>>
>>>       /* Init Dino (PCI host bus chip).  */
>>>       pci_bus = dino_init(addr_space, &rtc_irq, &serial_irq);
>>> @@ -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);
>>>       }
>>> @@ -204,7 +205,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 +233,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;

Helge, is this the code using this register?

https://github.com/hdeller/seabios-hppa/blob/parisc-qemu-5.0/src/parisc/head.S#L139

>>>       cpu[0]->env.gr[25] = kernel_entry;
>>>
>>>       /* tell firmware how many SMP CPUs to present in inventory table */
>>> @@ -255,11 +256,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. */
>>>   
>>
>