hw/hppa/machine.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
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 <deller@gmx.de>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
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);
/* 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. */
--
2.7.4
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 <deller@gmx.de>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> 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.
>
> /* 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. */
>
On Thu, 2 Jan 2020 18:15:02 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> 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 <deller@gmx.de>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > 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. */
> >
>
>
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.
should be applied on top of:
"hppa: drop RAM size fixup"
---
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;
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. */
--
2.7.4
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.
>
> 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.
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)?
Helge
> ---
> 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;
> 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. */
>
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.
> >
> > 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).
> Helge
>
>
> > ---
> > 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;
> > 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. */
> >
>
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. */
>>>
>>
>
On Sat, 4 Jan 2020 16:00:19 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 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.
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
This patch keeps all of that in the same state
(I'd suspect changing above points, would break guest)
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.
> >>> ---
> >>> 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. */
> >>>
> >>
> >
>
On 06.01.20 11:48, Igor Mammedov wrote:
> On Sat, 4 Jan 2020 16:00:19 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
>> 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.
> 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
Ok.
> (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).
No, here you are wrong.
This machine, same as all 32-bit x86 based machines, expect users
to insert memory modules of e.g. 1GB, 2GB and so on.
And ROM memory and other regions overlap the RAM in some regions
if required.
So, it's not "user convenience", but it's correct behaviour of the
code to simply allow 4GB and then don't blend in the memory which can't
be accessed.
Giving a error message that "you can only insert 3841MB" would be WRONG.
>> 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
... which is all OK, because it mimics the real hardware.
> This patch keeps all of that in the same state
> (I'd suspect changing above points, would break guest)
Yep (unless I change the SeaBIOS ROM code).
Anyway, I'm tired to discuss this.
Your patch isn't wrong by itself, I just think it's changing unnecessary
too much code and uses wrong wording for the commit message.
But just apply it as long as it doesn't break anything.
Helge
> 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.
>
>>>>> ---
>>>>> 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. */
>>>>>
>>>>
>>>
>>
>
On Mon, 6 Jan 2020 12:28:51 +0100
Helge Deller <deller@gmx.de> wrote:
> On 06.01.20 11:48, Igor Mammedov wrote:
> > On Sat, 4 Jan 2020 16:00:19 +0100
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >
> >> 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.
> > 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
>
> Ok.
>
> > (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).
>
> No, here you are wrong.
> This machine, same as all 32-bit x86 based machines, expect users
> to insert memory modules of e.g. 1GB, 2GB and so on.
> And ROM memory and other regions overlap the RAM in some regions
> if required.
> So, it's not "user convenience", but it's correct behaviour of the
> code to simply allow 4GB and then don't blend in the memory which can't
> be accessed.
> Giving a error message that "you can only insert 3841MB" would be WRONG.
>
> >> 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
>
> ... which is all OK, because it mimics the real hardware.
>
> > This patch keeps all of that in the same state
> > (I'd suspect changing above points, would break guest)
>
> Yep (unless I change the SeaBIOS ROM code).
Does real HW pass in cpu[0]->env.gr[26] full 4Gb or clamped size?
> Anyway, I'm tired to discuss this.
> Your patch isn't wrong by itself, I just think it's changing unnecessary
> too much code aram_sizend uses wrong wording for the commit message.
Let me prepare v3 which hopefully will make is smaller and
with commit message corrected.
How about following wording:
"
hppa: allow max ram size upto 4Gb
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 currently supported
value.
However real hardware allows to plug in up to 4Gb RAM
into memory slots. So allow user specify up to 4Gb and
map all of it into guest address space.
PS:
guest will still see 3840m being reported in
cpu[0]->env.gr[26] and loose ~248Mb, as it doesn't
have other means to discover RAM above firmware ROM.
"
> But just apply it as long as it doesn't break anything.
>
> Helge
>
> > 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.
> >
> >>>>> ---
> >>>>> 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. */
> >>>>>
> >>>>
> >>>
> >>
> >
>
>
On 06.01.20 17:24, Igor Mammedov wrote:
> On Mon, 6 Jan 2020 12:28:51 +0100
> Helge Deller <deller@gmx.de> wrote:
>
>> On 06.01.20 11:48, Igor Mammedov wrote:
>>> On Sat, 4 Jan 2020 16:00:19 +0100
>>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>
>>>> 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.
>>> 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
>>
>> Ok.
>>
>>> (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).
>>
>> No, here you are wrong.
>> This machine, same as all 32-bit x86 based machines, expect users
>> to insert memory modules of e.g. 1GB, 2GB and so on.
>> And ROM memory and other regions overlap the RAM in some regions
>> if required.
>> So, it's not "user convenience", but it's correct behaviour of the
>> code to simply allow 4GB and then don't blend in the memory which can't
>> be accessed.
>> Giving a error message that "you can only insert 3841MB" would be WRONG.
>>
>>>> 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
>>
>> ... which is all OK, because it mimics the real hardware.
>>
>>> This patch keeps all of that in the same state
>>> (I'd suspect changing above points, would break guest)
>>
>> Yep (unless I change the SeaBIOS ROM code).
> Does real HW pass in cpu[0]->env.gr[26] full 4Gb or clamped size?
No, because of the simple reason that real hardware uses a
propriatary HP Firmware, and that it's not started from some emulation
layer like qemu. So it has to find way itself to detect how much RAM
was plugged into the machine.
The functionality of giving the ram size in %r26 was done by me,
otherwise I don't know how SeaBIOS should detect how much RAM the
user wanted the machine to have. That's changeable if you have another
idea. Doesn't on x86 the ram size is given in some emulated RTC register?
>> Anyway, I'm tired to discuss this.
>> Your patch isn't wrong by itself, I just think it's changing unnecessary
>> too much code and uses wrong wording for the commit message.
> Let me prepare v3 which hopefully will make is smaller and
> with commit message corrected.
> How about following wording:
> "
> hppa: allow max ram size upto 4Gb
>
> 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 currently supported
> value.
>
> However real hardware allows to plug in up to 4Gb RAM
> into memory slots. So allow user specify up to 4Gb and
> map all of it into guest address space.
>
> PS:
> guest will still see 3840m being reported in
> cpu[0]->env.gr[26] and loose ~248Mb, as it doesn't
> have other means to discover RAM above firmware ROM.
> "
Why make it so complicated?
I see:
+ if (machine->ram_size > 4 * GiB) {
+ error_report("RAM size more than 4Gb is not supported");
what type is machine->ram_size?
If it's a 32bit unsigned integer this check is useless, and if it's a 64bit
integer it would be too big for a 32bit-only platform anyway.
So, I'd suggest to drop your wrong patch #43.
If you don't want to drop it, my suggestion for a commit message is:
hppa: Revert last wrong patch and give warning if user specified > 4GB RAM
Helge
>
>> But just apply it as long as it doesn't break anything.
>>
>> Helge
>>
>>> 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.
>>>
>>>>>>> ---
>>>>>>> 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. */
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>
On Mon, 6 Jan 2020 18:03:49 +0100
Helge Deller <deller@gmx.de> wrote:
> On 06.01.20 17:24, Igor Mammedov wrote:
> > On Mon, 6 Jan 2020 12:28:51 +0100
> > Helge Deller <deller@gmx.de> wrote:
> >
> >> On 06.01.20 11:48, Igor Mammedov wrote:
> >>> On Sat, 4 Jan 2020 16:00:19 +0100
> >>> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> >>>
> >>>> 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.
> >>> 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
> >>
> >> Ok.
> >>
> >>> (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).
> >>
> >> No, here you are wrong.
> >> This machine, same as all 32-bit x86 based machines, expect users
> >> to insert memory modules of e.g. 1GB, 2GB and so on.
> >> And ROM memory and other regions overlap the RAM in some regions
> >> if required.
> >> So, it's not "user convenience", but it's correct behaviour of the
> >> code to simply allow 4GB and then don't blend in the memory which can't
> >> be accessed.
> >> Giving a error message that "you can only insert 3841MB" would be WRONG.
> >>
> >>>> 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
> >>
> >> ... which is all OK, because it mimics the real hardware.
> >>
> >>> This patch keeps all of that in the same state
> >>> (I'd suspect changing above points, would break guest)
> >>
> >> Yep (unless I change the SeaBIOS ROM code).
>
> > Does real HW pass in cpu[0]->env.gr[26] full 4Gb or clamped size?
>
> No, because of the simple reason that real hardware uses a
> propriatary HP Firmware, and that it's not started from some emulation
> layer like qemu. So it has to find way itself to detect how much RAM
> was plugged into the machine.
>
> The functionality of giving the ram size in %r26 was done by me,
> otherwise I don't know how SeaBIOS should detect how much RAM the
> user wanted the machine to have. That's changeable if you have another
> idea. Doesn't on x86 the ram size is given in some emulated RTC register?
In x86 case, there is RTC value and there is memory map provided via
paravirt fwcfg.
Using register is fine for passing total RAM size and
Since it's QEMU invention, we could pass full 4Gb in register as
a starting point to support 4Gb of RAM (which removes fixup) and
let SeaBIOS to deduce memory map from that.
Then this patch cloud be amended to:
"
hpp: support 4G of RAM
Real hardware allows to plug in up to 4Gb of RAM
into memory slots, add support for this.
"
> >> Anyway, I'm tired to discuss this.
> >> Your patch isn't wrong by itself, I just think it's changing unnecessary
> >> too much code and uses wrong wording for the commit message.
>
> > Let me prepare v3 which hopefully will make is smaller and
> > with commit message corrected.
> > How about following wording:
> > "
> > hppa: allow max ram size upto 4Gb
> >
> > 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 currently supported
> > value.
> >
> > However real hardware allows to plug in up to 4Gb RAM
> > into memory slots. So allow user specify up to 4Gb and
> > map all of it into guest address space.
> >
> > PS:
> > guest will still see 3840m being reported in
> > cpu[0]->env.gr[26] and loose ~248Mb, as it doesn't
> > have other means to discover RAM above firmware ROM.
> > "
>
> Why make it so complicated?
It describes what exactly patch does without masking any
consequences of used approach (since we are not really telling
guest that it has more RAM and guest has other means to discover
it).
If you are asking why I'm making it hard error, see cover letter.
(in short main RAM allocation is total mess and series fixes
that by consolidating, de-duplicating and replacing ad hoc allocation
with a centralized approach that allows user to specify used
RAM backend in generic case).
> I see:
> + if (machine->ram_size > 4 * GiB) {
> + error_report("RAM size more than 4Gb is not supported");
>
> what type is machine->ram_size?
> If it's a 32bit unsigned integer this check is useless, and if it's a 64bit
> integer it would be too big for a 32bit-only platform anyway.
it's ram_addr_t, which is mutable depending on the host
it could be 32-bit or 64-bit.
Currently QEMU doesn't have a generic code to do such
checks and QEMU accumulated a whole zoo where some boards
do checks, some mask incorrect -m values and some do don't
do any checks. But cleaning up that is out of scope of
this already big enough re-factoring.
Hence I'm adding checks/removing fixups where they were
missing/present as it's a minimum effort needed to generalize
main RAM allocation.
> So, I'd suggest to drop your wrong patch #43.
As you noted in your first reply, patch is correct.
All it's doing is validating user input versus RAM size
actually supported by the current code, telling user
current supported limit and enforcing it.
I agree it's inconvenience for the users since they
won't be able to specify non-sense values and still
get board running, but that's clear user error and
should be corrected on user side and not by QEMU
magically masking wrong CLI values.
Since it could be fixed on user side, I care less
about user convenience when it comes to correctness
and unified code.
> If you don't want to drop it, my suggestion for a commit message is:
>
> hppa: Revert last wrong patch and give warning if user specified > 4GB RAM
I have to disagree on definition of wrong here,
in my opinion covering up user errors is wrong especially
when all users have to do is to adapt their CLI.
Supporting 4Gb is another story, which is extending current
impl. and could be done on top of the ram_size check.
> Helge
>
> >
> >> But just apply it as long as it doesn't break anything.
> >>
> >> Helge
> >>
> >>> 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.
> >>>
> >>>>>>> ---
> >>>>>>> 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. */
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >>
> >
>
On 07.01.20 12:21, Igor Mammedov wrote: > On Mon, 6 Jan 2020 18:03:49 +0100 >> So, I'd suggest to drop your wrong patch #43. > As you noted in your first reply, patch is correct. You probably got me wrong. Your patch #43 is wrong, and your fixup patch to some degree reverts it back again. In patch #43 you error out and stop, which real hardware wouldn't do. Real hardware simply ignores the memory which wouldn't be used. > All it's doing is validating user input versus RAM size > actually supported by the current code, telling user> current supported limit and enforcing it. Real hardware would not tell user. > I agree it's inconvenience for the users since they > won't be able to specify non-sense values and still > get board running, but that's clear user error and > should be corrected on user side and not by QEMU > magically masking wrong CLI values. I disagree. Everything worked as expected before, but with *your* change now people might need to modify their CLI. 4GB is a valid amount of memory which can be plugged into the virtual and physical machine. It's not magic, it's how the architecture works and you changed it. > Since it could be fixed on user side, I care less > about user convenience when it comes to correctness > and unified code. IMHO, you should care about that the emulation works the same way as physical machine. Not, how you want to educate end users. >> If you don't want to drop it, my suggestion for a commit message is: >> >> hppa: Revert last wrong patch and give warning if user specified > 4GB RAM > > I have to disagree on definition of wrong here, > in my opinion covering up user errors is wrong especially > when all users have to do is to adapt their CLI. See above. Nobody ever needs to adapt anything unless your patches gets applied. Helge
On Tue, 7 Jan 2020 12:53:32 +0100 Helge Deller <deller@gmx.de> wrote: Even though I disagree and it would waste ~256Mb 4Gb of RAM, I think I should be able to replace #43 with "hppa: allow max ram size upto 4Gb" as it still removes fix up at mapped address space level, removes fix up of global ram_size variable and adds max size check, which lets hppa board get out of the way of re-factoring generic RAM allocation and making what board does with provided RAM its own business. > On 07.01.20 12:21, Igor Mammedov wrote: > > On Mon, 6 Jan 2020 18:03:49 +0100 > >> So, I'd suggest to drop your wrong patch #43. > > As you noted in your first reply, patch is correct. > > You probably got me wrong. whichever way I read it https://www.mail-archive.com/qemu-devel@nongnu.org/msg667855.html states user convenience as a reason. > Your patch #43 is wrong, and your fixup patch to some degree reverts it back again. > > > In patch #43 you error out and stop, which real hardware wouldn't do. > Real hardware simply ignores the memory which wouldn't be used. > > All it's doing is validating user input versus RAM size > > actually supported by the current code, telling user> current supported limit and enforcing it. > > Real hardware would not tell user. > > > I agree it's inconvenience for the users since they > > won't be able to specify non-sense values and still > > get board running, but that's clear user error and > > should be corrected on user side and not by QEMU > > magically masking wrong CLI values. > > I disagree. > Everything worked as expected before, but with *your* change now people > might need to modify their CLI. > 4GB is a valid amount of memory which can be plugged into > the virtual and physical machine. > It's not magic, it's how the architecture works and you changed it. > > > Since it could be fixed on user side, I care less > > about user convenience when it comes to correctness > > and unified code. > > IMHO, you should care about that the emulation works the same > way as physical machine. As for correctness wrt real hardware questions are: * is one able to stuff hardware with unsupported 4Gb or more DIMMs, will system even work? * what real hardware does with top 256Gb of 4Gb RAM if present? Is it addressable/accessible in some way by CPUs or devices? * how does real firmware discovers amount of installed RAM [...]
On 1/6/20 11:48 AM, Igor Mammedov wrote:
> On Sat, 4 Jan 2020 16:00:19 +0100
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
>> 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.
> 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é <philmd@redhat.com>
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. */
>>>>>
>>>>
>>>
>>
>
© 2016 - 2024 Red Hat, Inc.