qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v8 4/5] mac_oldworld: Drop some variables
  2020-10-15 23:47 [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) BALATON Zoltan via
@ 2020-10-15 23:47 ` BALATON Zoltan via
  2020-10-15 23:47 ` [PATCH v8 1/5] mac_oldworld: Allow loading binary ROM image BALATON Zoltan via
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: BALATON Zoltan via @ 2020-10-15 23:47 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland

Values not used frequently enough may not worth putting in a local
variable, especially with names almost as long as the original value
because that does not improve readability, to the contrary it makes it
harder to see what value is used. Drop a few such variables.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ppc/mac_oldworld.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 5b30fa0739..13eb9bafa1 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -83,14 +83,11 @@ static void ppc_heathrow_reset(void *opaque)
 static void ppc_heathrow_init(MachineState *machine)
 {
     ram_addr_t ram_size = machine->ram_size;
-    const char *kernel_filename = machine->kernel_filename;
-    const char *kernel_cmdline = machine->kernel_cmdline;
-    const char *initrd_filename = machine->initrd_filename;
     const char *boot_device = machine->boot_order;
     PowerPCCPU *cpu = NULL;
     CPUPPCState *env = NULL;
     char *filename;
-    int linux_boot, i;
+    int i;
     MemoryRegion *bios = g_new(MemoryRegion, 1);
     uint32_t kernel_base, initrd_base, cmdline_base = 0;
     int32_t kernel_size, initrd_size;
@@ -109,8 +106,6 @@ static void ppc_heathrow_init(MachineState *machine)
     void *fw_cfg;
     uint64_t tbfreq;
 
-    linux_boot = (kernel_filename != NULL);
-
     /* init CPUs */
     for (i = 0; i < smp_cpus; i++) {
         cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
@@ -147,7 +142,7 @@ static void ppc_heathrow_init(MachineState *machine)
         bios_addr = (target_ulong)bios_addr;
 
         if (bios_size <= 0) {
-            /* or load binary ROM image */
+            /* or if could not load ELF try loading a binary ROM image */
             bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE);
             bios_addr = PROM_BASE;
         }
@@ -160,7 +155,7 @@ static void ppc_heathrow_init(MachineState *machine)
         exit(1);
     }
 
-    if (linux_boot) {
+    if (machine->kernel_filename) {
         int bswap_needed;
 
 #ifdef BSWAP_NEEDED
@@ -169,29 +164,32 @@ static void ppc_heathrow_init(MachineState *machine)
         bswap_needed = 0;
 #endif
         kernel_base = KERNEL_LOAD_ADDR;
-        kernel_size = load_elf(kernel_filename, NULL,
+        kernel_size = load_elf(machine->kernel_filename, NULL,
                                translate_kernel_address, NULL, NULL, NULL,
                                NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0);
         if (kernel_size < 0)
-            kernel_size = load_aout(kernel_filename, kernel_base,
+            kernel_size = load_aout(machine->kernel_filename, kernel_base,
                                     ram_size - kernel_base, bswap_needed,
                                     TARGET_PAGE_SIZE);
         if (kernel_size < 0)
-            kernel_size = load_image_targphys(kernel_filename,
+            kernel_size = load_image_targphys(machine->kernel_filename,
                                               kernel_base,
                                               ram_size - kernel_base);
         if (kernel_size < 0) {
-            error_report("could not load kernel '%s'", kernel_filename);
+            error_report("could not load kernel '%s'",
+                         machine->kernel_filename);
             exit(1);
         }
         /* load initrd */
-        if (initrd_filename) {
-            initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size + KERNEL_GAP);
-            initrd_size = load_image_targphys(initrd_filename, initrd_base,
+        if (machine->initrd_filename) {
+            initrd_base = TARGET_PAGE_ALIGN(kernel_base + kernel_size +
+                                            KERNEL_GAP);
+            initrd_size = load_image_targphys(machine->initrd_filename,
+                                              initrd_base,
                                               ram_size - initrd_base);
             if (initrd_size < 0) {
                 error_report("could not load initial ram disk '%s'",
-                             initrd_filename);
+                             machine->initrd_filename);
                 exit(1);
             }
             cmdline_base = TARGET_PAGE_ALIGN(initrd_base + initrd_size);
@@ -343,9 +341,10 @@ static void ppc_heathrow_init(MachineState *machine)
     fw_cfg_add_i16(fw_cfg, FW_CFG_MACHINE_ID, ARCH_HEATHROW);
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, kernel_base);
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
-    if (kernel_cmdline) {
+    if (machine->kernel_cmdline) {
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, cmdline_base);
-        pstrcpy_targphys("cmdline", cmdline_base, TARGET_PAGE_SIZE, kernel_cmdline);
+        pstrcpy_targphys("cmdline", cmdline_base, TARGET_PAGE_SIZE,
+                         machine->kernel_cmdline);
     } else {
         fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_CMDLINE, 0);
     }
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v8 3/5] mac_oldworld: Drop a variable, use get_system_memory() directly
  2020-10-15 23:47 [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) BALATON Zoltan via
                   ` (3 preceding siblings ...)
  2020-10-15 23:47 ` [PATCH v8 2/5] mac_newworld: Allow loading binary ROM image BALATON Zoltan via
@ 2020-10-15 23:47 ` BALATON Zoltan via
  2020-10-16  9:58 ` [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) Mark Cave-Ayland
  5 siblings, 0 replies; 14+ messages in thread
From: BALATON Zoltan via @ 2020-10-15 23:47 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland

Half of the occurances already use get_system_memory() directly
instead of sysmem variable, convert the two other uses to
get_system_memory() too which seems to be more common and drop the
variable.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ppc/mac_oldworld.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 0a40769b3e..5b30fa0739 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -87,7 +87,6 @@ static void ppc_heathrow_init(MachineState *machine)
     const char *kernel_cmdline = machine->kernel_cmdline;
     const char *initrd_filename = machine->initrd_filename;
     const char *boot_device = machine->boot_order;
-    MemoryRegion *sysmem = get_system_memory();
     PowerPCCPU *cpu = NULL;
     CPUPPCState *env = NULL;
     char *filename;
@@ -129,12 +128,12 @@ static void ppc_heathrow_init(MachineState *machine)
         exit(1);
     }
 
-    memory_region_add_subregion(sysmem, 0, machine->ram);
+    memory_region_add_subregion(get_system_memory(), 0, machine->ram);
 
     /* allocate and load firmware ROM */
     memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE,
                            &error_fatal);
-    memory_region_add_subregion(sysmem, PROM_BASE, bios);
+    memory_region_add_subregion(get_system_memory(), PROM_BASE, bios);
 
     if (!bios_name) {
         bios_name = PROM_FILENAME;
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v8 5/5] mac_oldworld: Change PCI address of macio to match real hardware
  2020-10-15 23:47 [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) BALATON Zoltan via
  2020-10-15 23:47 ` [PATCH v8 4/5] mac_oldworld: Drop some variables BALATON Zoltan via
  2020-10-15 23:47 ` [PATCH v8 1/5] mac_oldworld: Allow loading binary ROM image BALATON Zoltan via
@ 2020-10-15 23:47 ` BALATON Zoltan via
  2020-10-15 23:47 ` [PATCH v8 2/5] mac_newworld: Allow loading binary ROM image BALATON Zoltan via
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: BALATON Zoltan via @ 2020-10-15 23:47 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland

The board firmware expect these to be at fixed addresses and programs
them without probing, this patch puts the macio device at the expected
PCI address.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/ppc/mac_oldworld.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 13eb9bafa1..bb563e8aab 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -288,7 +288,7 @@ static void ppc_heathrow_init(MachineState *machine)
     ide_drive_get(hd, ARRAY_SIZE(hd));
 
     /* MacIO */
-    macio = pci_new(-1, TYPE_OLDWORLD_MACIO);
+    macio = pci_new(PCI_DEVFN(16, 0), TYPE_OLDWORLD_MACIO);
     dev = DEVICE(macio);
     qdev_prop_set_uint64(dev, "frequency", tbfreq);
     object_property_set_link(OBJECT(macio), "pic", OBJECT(pic_dev),
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)
@ 2020-10-15 23:47 BALATON Zoltan via
  2020-10-15 23:47 ` [PATCH v8 4/5] mac_oldworld: Drop some variables BALATON Zoltan via
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: BALATON Zoltan via @ 2020-10-15 23:47 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland

This is the cut down version of the earlier series omitting unfinished
patches that I plan to rework later and rebased to Mark's qemu-macppc
branch. Compared to v7 the only change is the cast to (target_ulong)
from (uint32_t) as requested by Mark in patch 1.

Regards,
BALATON Zoltan

BALATON Zoltan (5):
  mac_oldworld: Allow loading binary ROM image
  mac_newworld: Allow loading binary ROM image
  mac_oldworld: Drop a variable, use get_system_memory() directly
  mac_oldworld: Drop some variables
  mac_oldworld: Change PCI address of macio to match real hardware

 hw/ppc/mac.h          |  2 --
 hw/ppc/mac_newworld.c | 22 ++++++++------
 hw/ppc/mac_oldworld.c | 67 ++++++++++++++++++++++++-------------------
 3 files changed, 52 insertions(+), 39 deletions(-)

-- 
2.21.3



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v8 2/5] mac_newworld: Allow loading binary ROM image
  2020-10-15 23:47 [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) BALATON Zoltan via
                   ` (2 preceding siblings ...)
  2020-10-15 23:47 ` [PATCH v8 5/5] mac_oldworld: Change PCI address of macio to match real hardware BALATON Zoltan via
@ 2020-10-15 23:47 ` BALATON Zoltan via
  2020-10-15 23:47 ` [PATCH v8 3/5] mac_oldworld: Drop a variable, use get_system_memory() directly BALATON Zoltan via
  2020-10-16  9:58 ` [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) Mark Cave-Ayland
  5 siblings, 0 replies; 14+ messages in thread
From: BALATON Zoltan via @ 2020-10-15 23:47 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland

Fall back to load binary ROM image if loading ELF fails. This also
moves PROM_BASE and PROM_SIZE defines to board as these are matching
the ROM size and address on this board and removes the now unused
PROM_ADDR and BIOS_SIZE defines from common mac.h.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ppc/mac.h          |  2 --
 hw/ppc/mac_newworld.c | 22 ++++++++++++++--------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/mac.h b/hw/ppc/mac.h
index f3976b9a45..22c8408078 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -39,10 +39,8 @@
 /* SMP is not enabled, for now */
 #define MAX_CPUS 1
 
-#define BIOS_SIZE        (1 * MiB)
 #define NVRAM_SIZE        0x2000
 #define PROM_FILENAME    "openbios-ppc"
-#define PROM_ADDR         0xfff00000
 
 #define KERNEL_LOAD_ADDR 0x01000000
 #define KERNEL_GAP       0x00100000
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 7a8dc09c8d..f9a1cc8944 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -82,6 +82,8 @@
 
 #define NDRV_VGA_FILENAME "qemu_vga.ndrv"
 
+#define PROM_BASE 0xfff00000
+#define PROM_SIZE (1 * MiB)
 
 static void fw_cfg_boot_set(void *opaque, const char *boot_device,
                             Error **errp)
@@ -100,7 +102,7 @@ static void ppc_core99_reset(void *opaque)
 
     cpu_reset(CPU(cpu));
     /* 970 CPUs want to get their initial IP as part of their boot protocol */
-    cpu->env.nip = PROM_ADDR + 0x100;
+    cpu->env.nip = PROM_BASE + 0x100;
 }
 
 /* PowerPC Mac99 hardware initialisation */
@@ -154,25 +156,29 @@ static void ppc_core99_init(MachineState *machine)
     /* allocate RAM */
     memory_region_add_subregion(get_system_memory(), 0, machine->ram);
 
-    /* allocate and load BIOS */
-    memory_region_init_rom(bios, NULL, "ppc_core99.bios", BIOS_SIZE,
+    /* allocate and load firmware ROM */
+    memory_region_init_rom(bios, NULL, "ppc_core99.bios", PROM_SIZE,
                            &error_fatal);
+    memory_region_add_subregion(get_system_memory(), PROM_BASE, bios);
 
-    if (bios_name == NULL)
+    if (!bios_name) {
         bios_name = PROM_FILENAME;
+    }
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-    memory_region_add_subregion(get_system_memory(), PROM_ADDR, bios);
-
-    /* Load OpenBIOS (ELF) */
     if (filename) {
+        /* Load OpenBIOS (ELF) */
         bios_size = load_elf(filename, NULL, NULL, NULL, NULL,
                              NULL, NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0);
 
+        if (bios_size <= 0) {
+            /* or load binary ROM image */
+            bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE);
+        }
         g_free(filename);
     } else {
         bios_size = -1;
     }
-    if (bios_size < 0 || bios_size > BIOS_SIZE) {
+    if (bios_size < 0 || bios_size > PROM_SIZE) {
         error_report("could not load PowerPC bios '%s'", bios_name);
         exit(1);
     }
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v8 1/5] mac_oldworld: Allow loading binary ROM image
  2020-10-15 23:47 [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) BALATON Zoltan via
  2020-10-15 23:47 ` [PATCH v8 4/5] mac_oldworld: Drop some variables BALATON Zoltan via
@ 2020-10-15 23:47 ` BALATON Zoltan via
  2020-10-15 23:47 ` [PATCH v8 5/5] mac_oldworld: Change PCI address of macio to match real hardware BALATON Zoltan via
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: BALATON Zoltan via @ 2020-10-15 23:47 UTC (permalink / raw)
  To: qemu-devel, qemu-ppc; +Cc: Mark Cave-Ayland

The beige G3 Power Macintosh has a 4MB firmware ROM. Fix the size of
the rom region and fall back to loading a binary image with -bios if
loading ELF image failed. This allows testing emulation with a ROM
image from real hardware as well as using an ELF OpenBIOS image.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/mac_oldworld.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 05e46ee6fe..0a40769b3e 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -59,6 +59,8 @@
 #define NDRV_VGA_FILENAME "qemu_vga.ndrv"
 
 #define GRACKLE_BASE 0xfec00000
+#define PROM_BASE 0xffc00000
+#define PROM_SIZE (4 * MiB)
 
 static void fw_cfg_boot_set(void *opaque, const char *boot_device,
                             Error **errp)
@@ -100,6 +102,7 @@ static void ppc_heathrow_init(MachineState *machine)
     SysBusDevice *s;
     DeviceState *dev, *pic_dev;
     BusState *adb_bus;
+    uint64_t bios_addr;
     int bios_size;
     unsigned int smp_cpus = machine->smp.cpus;
     uint16_t ppc_boot_device;
@@ -128,24 +131,32 @@ static void ppc_heathrow_init(MachineState *machine)
 
     memory_region_add_subregion(sysmem, 0, machine->ram);
 
-    /* allocate and load BIOS */
-    memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", BIOS_SIZE,
+    /* allocate and load firmware ROM */
+    memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE,
                            &error_fatal);
+    memory_region_add_subregion(sysmem, PROM_BASE, bios);
 
-    if (bios_name == NULL)
+    if (!bios_name) {
         bios_name = PROM_FILENAME;
+    }
     filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
-    memory_region_add_subregion(sysmem, PROM_ADDR, bios);
-
-    /* Load OpenBIOS (ELF) */
     if (filename) {
-        bios_size = load_elf(filename, NULL, 0, NULL, NULL, NULL, NULL, NULL,
-                             1, PPC_ELF_MACHINE, 0, 0);
+        /* Load OpenBIOS (ELF) */
+        bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr,
+                             NULL, NULL, 1, PPC_ELF_MACHINE, 0, 0);
+        /* Unfortunately, load_elf sign-extends reading elf32 */
+        bios_addr = (target_ulong)bios_addr;
+
+        if (bios_size <= 0) {
+            /* or load binary ROM image */
+            bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE);
+            bios_addr = PROM_BASE;
+        }
         g_free(filename);
     } else {
         bios_size = -1;
     }
-    if (bios_size < 0 || bios_size > BIOS_SIZE) {
+    if (bios_size < 0 || bios_addr - PROM_BASE + bios_size > PROM_SIZE) {
         error_report("could not load PowerPC bios '%s'", bios_name);
         exit(1);
     }
-- 
2.21.3



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)
  2020-10-15 23:47 [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) BALATON Zoltan via
                   ` (4 preceding siblings ...)
  2020-10-15 23:47 ` [PATCH v8 3/5] mac_oldworld: Drop a variable, use get_system_memory() directly BALATON Zoltan via
@ 2020-10-16  9:58 ` Mark Cave-Ayland
  2020-10-16 12:19   ` BALATON Zoltan via
  2020-10-16 13:05   ` Philippe Mathieu-Daudé
  5 siblings, 2 replies; 14+ messages in thread
From: Mark Cave-Ayland @ 2020-10-16  9:58 UTC (permalink / raw)
  To: BALATON Zoltan, qemu-devel, qemu-ppc

On 16/10/2020 00:47, BALATON Zoltan via wrote:

> This is the cut down version of the earlier series omitting unfinished
> patches that I plan to rework later and rebased to Mark's qemu-macppc
> branch. Compared to v7 the only change is the cast to (target_ulong)
> from (uint32_t) as requested by Mark in patch 1.

FWIW the reason for suggesting the cast to target_ulong is so that the same code 
works for both qemu-system-ppc and qemu-system-ppc64. For qemu-system-ppc that should 
correctly drop the sign extension from 32-bit, whilst still allowing someone to load 
a 64-bit ELF into qemu-system-ppc64 if requested.

Can you confirm that the sign extension behaviour is still correct for both 
qemu-system-ppc and qemu-system-ppc64? If so I'm happy to give it a R-B tag.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)
  2020-10-16  9:58 ` [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) Mark Cave-Ayland
@ 2020-10-16 12:19   ` BALATON Zoltan via
  2020-10-17 12:54     ` Mark Cave-Ayland
  2020-10-16 13:05   ` Philippe Mathieu-Daudé
  1 sibling, 1 reply; 14+ messages in thread
From: BALATON Zoltan via @ 2020-10-16 12:19 UTC (permalink / raw)
  To: Mark Cave-Ayland; +Cc: qemu-ppc, qemu-devel

On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
> On 16/10/2020 00:47, BALATON Zoltan via wrote:
>> This is the cut down version of the earlier series omitting unfinished
>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>> branch. Compared to v7 the only change is the cast to (target_ulong)
>> from (uint32_t) as requested by Mark in patch 1.
>
> FWIW the reason for suggesting the cast to target_ulong is so that the same 
> code works for both qemu-system-ppc and qemu-system-ppc64. For 
> qemu-system-ppc that should correctly drop the sign extension from 32-bit, 
> whilst still allowing someone to load a 64-bit ELF into qemu-system-ppc64 if 
> requested.
>
> Can you confirm that the sign extension behaviour is still correct for both 
> qemu-system-ppc and qemu-system-ppc64? If so I'm happy to give it a R-B tag.

I've tried it now again with both ppc and ppc64: both OpenBIOS and a G3 
beige ROM can be loaded with qemu-system-ppc but qemu-system-ppc64 fails 
with OpenBIOS when casting to target_ulong (i think because target_ulong 
is 64 bit there but g3beige is still 32 bit but I haven't throughly 
debugged it). But everything works with my original uint32_t cast, so 
ditch it and use my original version. Should I resubmit or you can fix up? 
(I think I wait until it's clear if this will be taken by David or you and 
send a fixed version cc-ing David if this is decided to go through the PPC 
queue.)

Regards,
BALATON Zoltan


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)
  2020-10-16  9:58 ` [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) Mark Cave-Ayland
  2020-10-16 12:19   ` BALATON Zoltan via
@ 2020-10-16 13:05   ` Philippe Mathieu-Daudé
  2020-10-17 16:39     ` BALATON Zoltan via
  1 sibling, 1 reply; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-16 13:05 UTC (permalink / raw)
  To: Mark Cave-Ayland, BALATON Zoltan, qemu-devel, qemu-ppc

On 10/16/20 11:58 AM, Mark Cave-Ayland wrote:
> On 16/10/2020 00:47, BALATON Zoltan via wrote:
> 
>> This is the cut down version of the earlier series omitting unfinished
>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>> branch. Compared to v7 the only change is the cast to (target_ulong)
>> from (uint32_t) as requested by Mark in patch 1.
> 
> FWIW the reason for suggesting the cast to target_ulong is so that the 
> same code works for both qemu-system-ppc and qemu-system-ppc64. For 
> qemu-system-ppc that should correctly drop the sign extension from 
> 32-bit, whilst still allowing someone to load a 64-bit ELF into 
> qemu-system-ppc64 if requested.

IMO this is part of a bigger design problem. Not all
machines main bus is 64-bit. I did some experiments
but changing that involves a lot of work.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)
  2020-10-16 12:19   ` BALATON Zoltan via
@ 2020-10-17 12:54     ` Mark Cave-Ayland
  2020-10-17 15:56       ` BALATON Zoltan via
  0 siblings, 1 reply; 14+ messages in thread
From: Mark Cave-Ayland @ 2020-10-17 12:54 UTC (permalink / raw)
  To: BALATON Zoltan; +Cc: qemu-ppc, qemu-devel

On 16/10/2020 13:19, BALATON Zoltan via wrote:

> On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
>> On 16/10/2020 00:47, BALATON Zoltan via wrote:
>>> This is the cut down version of the earlier series omitting unfinished
>>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>>> branch. Compared to v7 the only change is the cast to (target_ulong)
>>> from (uint32_t) as requested by Mark in patch 1.
>>
>> FWIW the reason for suggesting the cast to target_ulong is so that the same code 
>> works for both qemu-system-ppc and qemu-system-ppc64. For qemu-system-ppc that 
>> should correctly drop the sign extension from 32-bit, whilst still allowing someone 
>> to load a 64-bit ELF into qemu-system-ppc64 if requested.
>>
>> Can you confirm that the sign extension behaviour is still correct for both 
>> qemu-system-ppc and qemu-system-ppc64? If so I'm happy to give it a R-B tag.
> 
> I've tried it now again with both ppc and ppc64: both OpenBIOS and a G3 beige ROM can 
> be loaded with qemu-system-ppc but qemu-system-ppc64 fails with OpenBIOS when casting 
> to target_ulong (i think because target_ulong is 64 bit there but g3beige is still 32 
> bit but I haven't throughly debugged it). But everything works with my original 
> uint32_t cast, so ditch it and use my original version. Should I resubmit or you can 
> fix up? (I think I wait until it's clear if this will be taken by David or you and 
> send a fixed version cc-ing David if this is decided to go through the PPC queue.)

Hmmm yes I see that qemu-system-ppc64 fails because target_ulong will always 
represent a 64-bit quantity, even if you request a 32-bit CPU. Rather than add some 
CPU-specific code let's keep the uint32_t cast for now as I would hope it is unlikely 
someone would load an ELF in high memory, and perhaps the sign-extended address bug 
will get fixed later.

With the cast reverted to uint32_t then for patches 1 and 2:
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

If you can send a v9 with the cast fixed I'll apply this to my qemu-macppc branch 
right away.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)
  2020-10-17 12:54     ` Mark Cave-Ayland
@ 2020-10-17 15:56       ` BALATON Zoltan via
  2020-10-18 16:01         ` Mark Cave-Ayland
  0 siblings, 1 reply; 14+ messages in thread
From: BALATON Zoltan via @ 2020-10-17 15:56 UTC (permalink / raw)
  To: Mark Cave-Ayland; +Cc: qemu-ppc, qemu-devel

On Sat, 17 Oct 2020, Mark Cave-Ayland wrote:
> On 16/10/2020 13:19, BALATON Zoltan via wrote:
>> On Fri, 16 Oct 2020, Mark Cave-Ayland wrote:
>>> On 16/10/2020 00:47, BALATON Zoltan via wrote:
>>>> This is the cut down version of the earlier series omitting unfinished
>>>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>>>> branch. Compared to v7 the only change is the cast to (target_ulong)
>>>> from (uint32_t) as requested by Mark in patch 1.
>>> 
>>> FWIW the reason for suggesting the cast to target_ulong is so that the 
>>> same code works for both qemu-system-ppc and qemu-system-ppc64. For 
>>> qemu-system-ppc that should correctly drop the sign extension from 32-bit, 
>>> whilst still allowing someone to load a 64-bit ELF into qemu-system-ppc64 
>>> if requested.
>>> 
>>> Can you confirm that the sign extension behaviour is still correct for 
>>> both qemu-system-ppc and qemu-system-ppc64? If so I'm happy to give it a 
>>> R-B tag.
>> 
>> I've tried it now again with both ppc and ppc64: both OpenBIOS and a G3 
>> beige ROM can be loaded with qemu-system-ppc but qemu-system-ppc64 fails 
>> with OpenBIOS when casting to target_ulong (i think because target_ulong is 
>> 64 bit there but g3beige is still 32 bit but I haven't throughly debugged 
>> it). But everything works with my original uint32_t cast, so ditch it and 
>> use my original version. Should I resubmit or you can fix up? (I think I 
>> wait until it's clear if this will be taken by David or you and send a 
>> fixed version cc-ing David if this is decided to go through the PPC queue.)
>
> Hmmm yes I see that qemu-system-ppc64 fails because target_ulong will always 
> represent a 64-bit quantity, even if you request a 32-bit CPU. Rather than 
> add some CPU-specific code let's keep the uint32_t cast for now as I would 
> hope it is unlikely someone would load an ELF in high memory, and perhaps the 
> sign-extended address bug will get fixed later.
>
> With the cast reverted to uint32_t then for patches 1 and 2:
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
> If you can send a v9 with the cast fixed I'll apply this to my qemu-macppc 
> branch right away.

You could've really just edit the single cast in patch 1 before applying 
to change the it back but I've resent the changed patch 1 as v9 also 
adding your R-b for your convenience. Other patches are unchanged so you 
can take the v8 for those, I haven't resent those, let me know if you want 
the whole series but this is really getting much more work that it should 
be for such a simple change. (There is no cast in patch 2 as I've already 
stated several times.)

Regards,
BALATON Zoltan


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)
  2020-10-16 13:05   ` Philippe Mathieu-Daudé
@ 2020-10-17 16:39     ` BALATON Zoltan via
  2020-10-17 17:44       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 14+ messages in thread
From: BALATON Zoltan via @ 2020-10-17 16:39 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: qemu-ppc, Mark Cave-Ayland, qemu-devel

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

On Fri, 16 Oct 2020, Philippe Mathieu-Daudé wrote:
> On 10/16/20 11:58 AM, Mark Cave-Ayland wrote:
>> On 16/10/2020 00:47, BALATON Zoltan via wrote:
>> 
>>> This is the cut down version of the earlier series omitting unfinished
>>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>>> branch. Compared to v7 the only change is the cast to (target_ulong)
>>> from (uint32_t) as requested by Mark in patch 1.
>> 
>> FWIW the reason for suggesting the cast to target_ulong is so that the same 
>> code works for both qemu-system-ppc and qemu-system-ppc64. For 
>> qemu-system-ppc that should correctly drop the sign extension from 32-bit, 
>> whilst still allowing someone to load a 64-bit ELF into qemu-system-ppc64 
>> if requested.
>
> IMO this is part of a bigger design problem. Not all
> machines main bus is 64-bit. I did some experiments
> but changing that involves a lot of work.

Did not want to reply to this to not bring it to your attention before 
patch gets in finally but it's too late...

Not sure what you refer to but in this particular case the problem only 
seems to be load_elf loading 32 bit ELF files returning sign extended 64 
bit address which looks bogus but since this function is widely used I did 
not feel confident enough to propose a patch to load_elf.

By the way, also the parameters of load_elf could take a clean up to 
remove all the mostly NULL values as I've pointed out before:

https://lists.nongnu.org/archive/html/qemu-devel/2019-01/msg03427.html

but all this could wait until later, these don't seem to be urgent 
problems to prevent moving mac machines forward now and could all be 
addressen in separate elf loading series. So just note the problem and 
move on for now please.

Reagards.
BALATON Zoltan

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)
  2020-10-17 16:39     ` BALATON Zoltan via
@ 2020-10-17 17:44       ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 14+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-17 17:44 UTC (permalink / raw)
  To: BALATON Zoltan; +Cc: Mark Cave-Ayland, qemu-ppc, qemu-devel

On 10/17/20 6:39 PM, BALATON Zoltan via wrote:
> On Fri, 16 Oct 2020, Philippe Mathieu-Daudé wrote:
>> On 10/16/20 11:58 AM, Mark Cave-Ayland wrote:
>>> On 16/10/2020 00:47, BALATON Zoltan via wrote:
>>>
>>>> This is the cut down version of the earlier series omitting unfinished
>>>> patches that I plan to rework later and rebased to Mark's qemu-macppc
>>>> branch. Compared to v7 the only change is the cast to (target_ulong)
>>>> from (uint32_t) as requested by Mark in patch 1.
>>>
>>> FWIW the reason for suggesting the cast to target_ulong is so that 
>>> the same code works for both qemu-system-ppc and qemu-system-ppc64. 
>>> For qemu-system-ppc that should correctly drop the sign extension 
>>> from 32-bit, whilst still allowing someone to load a 64-bit ELF into 
>>> qemu-system-ppc64 if requested.
>>
>> IMO this is part of a bigger design problem. Not all
>> machines main bus is 64-bit. I did some experiments
>> but changing that involves a lot of work.
> 
> Did not want to reply to this to not bring it to your attention before 
> patch gets in finally but it's too late...
> 
> Not sure what you refer to

I refer to having machines with a N-bit main bus using a N-bit main bus
(currently all main busses are 64-bit wide).

Some 32-bit machines have access to 64-bit busses, some don't.

I have been wondering about it because of the AVR CPU which
uses a pair of busses, each less than 32-bit.

If one day I can finish my work there, the Old World mac might
benefit from it.

> but in this particular case the problem only 
> seems to be load_elf loading 32 bit ELF files returning sign extended 64 
> bit address which looks bogus but since this function is widely used I 
> did not feel confident enough to propose a patch to load_elf.
> 
> By the way, also the parameters of load_elf could take a clean up to 
> remove all the mostly NULL values as I've pointed out before:
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2019-01/msg03427.html
> 
> but all this could wait until later, these don't seem to be urgent 
> problems to prevent moving mac machines forward now and could all be 
> addressen in separate elf loading series. So just note the problem and 
> move on for now please.
> 
> Reagards.
> BALATON Zoltan


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM)
  2020-10-17 15:56       ` BALATON Zoltan via
@ 2020-10-18 16:01         ` Mark Cave-Ayland
  0 siblings, 0 replies; 14+ messages in thread
From: Mark Cave-Ayland @ 2020-10-18 16:01 UTC (permalink / raw)
  To: BALATON Zoltan; +Cc: qemu-ppc, qemu-devel

On 17/10/2020 16:56, BALATON Zoltan via wrote:

>> If you can send a v9 with the cast fixed I'll apply this to my qemu-macppc branch 
>> right away.
> 
> You could've really just edit the single cast in patch 1 before applying to change 
> the it back but I've resent the changed patch 1 as v9 also adding your R-b for your 
> convenience. Other patches are unchanged so you can take the v8 for those, I haven't 
> resent those, let me know if you want the whole series but this is really getting 
> much more work that it should be for such a simple change. (There is no cast in patch 
> 2 as I've already stated several times.)

Thanks - this has been included in the PR I just sent.


ATB,

Mark.


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-10-18 16:09 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-15 23:47 [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) BALATON Zoltan via
2020-10-15 23:47 ` [PATCH v8 4/5] mac_oldworld: Drop some variables BALATON Zoltan via
2020-10-15 23:47 ` [PATCH v8 1/5] mac_oldworld: Allow loading binary ROM image BALATON Zoltan via
2020-10-15 23:47 ` [PATCH v8 5/5] mac_oldworld: Change PCI address of macio to match real hardware BALATON Zoltan via
2020-10-15 23:47 ` [PATCH v8 2/5] mac_newworld: Allow loading binary ROM image BALATON Zoltan via
2020-10-15 23:47 ` [PATCH v8 3/5] mac_oldworld: Drop a variable, use get_system_memory() directly BALATON Zoltan via
2020-10-16  9:58 ` [PATCH v8 0/5] Mac Old World ROM experiment (ppc/mac_* clean ups and loading binary ROM) Mark Cave-Ayland
2020-10-16 12:19   ` BALATON Zoltan via
2020-10-17 12:54     ` Mark Cave-Ayland
2020-10-17 15:56       ` BALATON Zoltan via
2020-10-18 16:01         ` Mark Cave-Ayland
2020-10-16 13:05   ` Philippe Mathieu-Daudé
2020-10-17 16:39     ` BALATON Zoltan via
2020-10-17 17:44       ` Philippe Mathieu-Daudé

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).