qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v2] hppa: allow max ram size upto 4Gb
Date: Mon, 6 Jan 2020 12:05:00 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 
On 1/6/20 11:48 AM, Igor Mammedov wrote:

On Sat, 4 Jan 2020 16:00:19 +0100
Philippe Mathieu-Daudé <address@hidden> wrote:


On 1/3/20 10:54 AM, Igor Mammedov wrote:

On Thu, 2 Jan 2020 21:22:12 +0100
Helge Deller <address@hidden> 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 <address@hidden>
Signed-off-by: Igor Mammedov <address@hidden>
---
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.

I'm not adding 4Gb support here (it's out of scope of this series),
all this patch does is making pre-existing global ram_size fixup,
this board only business and naming it clamped_ram_size to match
its current usage (along with explicitly documenting the deviation from
other boards why it was requested to keep fixup 'for user convenience'
instead of going for correct and simpler error message telling
how much RAM user could specify on CLI).


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?


Well, board was fixing up global ram_size and then used it to
  - pass clamped value to guest via register
  - pass it to load load_image_targphys()
  - perform various checks
  - affects reset sequence


You are correct. 'clamped_ram_size' is a good name.


This patch keeps all of that in the same state
(I'd suspect changing above points, would break guest)


It makes sense.


If you have an alternative patch that enables to use full 4Gb,
I'd include on respin as far as it doesn't change user supplied
global ram_size && ms->ram_size && uses generic ms->ram &&
preferably on top of
  "[PATCH 44/86] hppa: use memdev for RAM"
so it would be easier to drop it if there would opposition to it
without affecting series.


No, now than I understood your explication:

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

Thanks for insisting :)


---
   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. */
reply via email to
[Prev in Thread] Current Thread [Next in Thread]