[PATCH v7 0/8] Mac Old World ROM experiment

[PATCH v7 0/8] Mac Old World ROM experiment

[BALATON Zoltan]

This is now a minimal set of patches needed to make it possible to
experiment with a firmware ROM from real hardware. After finding out
that the board firmware does not probe PCI devices but expects them at
known fixed addresses we only need to change the address of the macio
device to get the firmware correctly map it. This allows dropping
workarounds in previous versions for this and now only the minimal set
of patches are included to get the firmware loaded and do something.
(Also excluded the grackle revision and machine ID pathes for now that
may be needed as the firmware accesses these but seems to go further
without them so until we hit a problem we can live without it,
although I wonder if this causes us unnecessary debugging later so
unless they cause regressions they could be merged).

I still don't get video output but at least it talks to the GPU chip
now so it can be debugged and improved (this will either need
emulating the correct chip the firmware has a driver for or an OF
compliant ROM for the emulated card).

As before the I2C part (patches 6-8) is RFC and unfinished but the
first 5 patches should be good enough now. I hope someone can take
care of I2C, I can look at the ati-vga side later.

Regards,
BALATON Zoltan

BALATON Zoltan (8):
  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
  i2c: Match parameters of i2c_start_transfer and i2c_send_recv
  WIP macio/cuda: Attempt to add i2c support
  mac_oldworld: Add SPD data to cover RAM

 hw/display/sm501.c           |  2 +-
 hw/i2c/core.c                | 34 +++++++-------
 hw/i2c/ppc4xx_i2c.c          |  2 +-
 hw/misc/macio/cuda.c         | 76 ++++++++++++++++++++++++++++++-
 hw/ppc/mac.h                 |  2 -
 hw/ppc/mac_newworld.c        | 22 +++++----
 hw/ppc/mac_oldworld.c        | 86 +++++++++++++++++++++++-------------
 include/hw/i2c/i2c.h         |  4 +-
 include/hw/misc/macio/cuda.h |  1 +
 9 files changed, 167 insertions(+), 62 deletions(-)

-- 
2.21.3

[PATCH v7 6/8] i2c: Match parameters of i2c_start_transfer and i2c_send_recv

[BALATON Zoltan]

These functions have a parameter that decides the direction of
transfer but totally confusingly they don't match but inverted sense.
To avoid frequent mistakes when using these functions change
i2c_send_recv to match i2c_start_transfer. Also use bool in
i2c_start_transfer instead of int to match i2c_send_recv.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
This probably won't be the solution accepted as Philippe has an
alternative series fixing this problem here:
https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg07022.html
but until that or something else is merged this will do for the next
patch. When this gets sorted out I'll send a rebased version.

 hw/display/sm501.c   |  2 +-
 hw/i2c/core.c        | 34 +++++++++++++++++-----------------
 hw/i2c/ppc4xx_i2c.c  |  2 +-
 include/hw/i2c/i2c.h |  4 ++--
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index a7fc08c52b..e714674681 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -1041,7 +1041,7 @@ static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
                                   s->i2c_byte_count + 1, s->i2c_addr >> 1);
                     for (i = 0; i <= s->i2c_byte_count; i++) {
                         res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
-                                            !(s->i2c_addr & 1));
+                                            s->i2c_addr & 1);
                         if (res) {
                             SM501_DPRINTF("sm501 i2c : transfer failed"
                                           " i=%d, res=%d\n", i, res);
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index acf34a12d6..0303fefeaf 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -91,7 +91,7 @@ int i2c_bus_busy(I2CBus *bus)
  * without releasing the bus.  If that fails, the bus is still
  * in a transaction.
  */
-int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv)
+int i2c_start_transfer(I2CBus *bus, uint8_t address, bool recv)
 {
     BusChild *kid;
     I2CSlaveClass *sc;
@@ -175,26 +175,14 @@ void i2c_end_transfer(I2CBus *bus)
     bus->broadcast = false;
 }
 
-int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send)
+int i2c_send_recv(I2CBus *bus, uint8_t *data, bool recv)
 {
     I2CSlaveClass *sc;
     I2CSlave *s;
     I2CNode *node;
     int ret = 0;
 
-    if (send) {
-        QLIST_FOREACH(node, &bus->current_devs, next) {
-            s = node->elt;
-            sc = I2C_SLAVE_GET_CLASS(s);
-            if (sc->send) {
-                trace_i2c_send(s->address, *data);
-                ret = ret || sc->send(s, *data);
-            } else {
-                ret = -1;
-            }
-        }
-        return ret ? -1 : 0;
-    } else {
+    if (recv) {
         ret = 0xff;
         if (!QLIST_EMPTY(&bus->current_devs) && !bus->broadcast) {
             sc = I2C_SLAVE_GET_CLASS(QLIST_FIRST(&bus->current_devs)->elt);
@@ -206,19 +194,31 @@ int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send)
         }
         *data = ret;
         return 0;
+    } else {
+        QLIST_FOREACH(node, &bus->current_devs, next) {
+            s = node->elt;
+            sc = I2C_SLAVE_GET_CLASS(s);
+            if (sc->send) {
+                trace_i2c_send(s->address, *data);
+                ret = ret || sc->send(s, *data);
+            } else {
+                ret = -1;
+            }
+        }
+        return ret ? -1 : 0;
     }
 }
 
 int i2c_send(I2CBus *bus, uint8_t data)
 {
-    return i2c_send_recv(bus, &data, true);
+    return i2c_send_recv(bus, &data, false);
 }
 
 uint8_t i2c_recv(I2CBus *bus)
 {
     uint8_t data = 0xff;
 
-    i2c_send_recv(bus, &data, false);
+    i2c_send_recv(bus, &data, true);
     return data;
 }
 
diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
index c0a8e04567..d3899203a4 100644
--- a/hw/i2c/ppc4xx_i2c.c
+++ b/hw/i2c/ppc4xx_i2c.c
@@ -239,7 +239,7 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
                     }
                 }
                 if (!(i2c->sts & IIC_STS_ERR) &&
-                    i2c_send_recv(i2c->bus, &i2c->mdata[i], !recv)) {
+                    i2c_send_recv(i2c->bus, &i2c->mdata[i], recv)) {
                     i2c->sts |= IIC_STS_ERR;
                     i2c->extsts |= IIC_EXTSTS_XFRA;
                     break;
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index d6e3d85faf..ad2475d6a2 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -72,10 +72,10 @@ struct I2CBus {
 I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
 void i2c_set_slave_address(I2CSlave *dev, uint8_t address);
 int i2c_bus_busy(I2CBus *bus);
-int i2c_start_transfer(I2CBus *bus, uint8_t address, int recv);
+int i2c_start_transfer(I2CBus *bus, uint8_t address, bool recv);
 void i2c_end_transfer(I2CBus *bus);
 void i2c_nack(I2CBus *bus);
-int i2c_send_recv(I2CBus *bus, uint8_t *data, bool send);
+int i2c_send_recv(I2CBus *bus, uint8_t *data, bool recv);
 int i2c_send(I2CBus *bus, uint8_t data);
 uint8_t i2c_recv(I2CBus *bus);
 
-- 
2.21.3

[PATCH v7 5/8] mac_oldworld: Change PCI address of macio to match real hardware

[BALATON Zoltan]

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>
---
 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 4200008851..6276973c95 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -286,7 +286,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), OBJECT(pic_dev), "pic",
-- 
2.21.3

Re: [PATCH v7 5/8] mac_oldworld: Change PCI address of macio to match real hardware

[Mark Cave-Ayland]

On 29/06/2020 19:55, BALATON Zoltan wrote:

> 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>
> ---
>  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 4200008851..6276973c95 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -286,7 +286,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), OBJECT(pic_dev), "pic",

This looks much better!

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH v7 4/8] mac_oldworld: Drop some variables

[BALATON Zoltan]

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>
---
 hw/ppc/mac_oldworld.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index d1c4244b1e..4200008851 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;
@@ -108,8 +105,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));
@@ -159,7 +154,7 @@ static void ppc_heathrow_init(MachineState *machine)
         exit(1);
     }
 
-    if (linux_boot) {
+    if (machine->kernel_filename) {
         uint64_t lowaddr = 0;
         int bswap_needed;
 
@@ -169,30 +164,33 @@ 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, &lowaddr, 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);
@@ -336,9 +334,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

Re: [PATCH v7 4/8] mac_oldworld: Drop some variables

[Philippe Mathieu-Daudé]

On 6/29/20 8:55 PM, BALATON Zoltan wrote:
> 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>
> ---
>   hw/ppc/mac_oldworld.c | 33 ++++++++++++++++-----------------
>   1 file changed, 16 insertions(+), 17 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v7 8/8] mac_oldworld: Add SPD data to cover RAM

[BALATON Zoltan]

OpenBIOS gets RAM size via fw_cfg but rhe original board firmware
detects RAM using SPD data so generate and add SDP eeproms to cover as
much RAM as possible to describe with SPD (this may be less than the
actual ram_size due to SDRAM size constraints).

This patch is more complex as it should be which I intend to fix once
agreement can be made on how to get back the necessary functionality
removed by previous patches. See in this thread:
https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg08710.html

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

diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 6276973c95..6a27287c9f 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -34,6 +34,7 @@
 #include "hw/input/adb.h"
 #include "sysemu/sysemu.h"
 #include "net/net.h"
+#include "hw/i2c/smbus_eeprom.h"
 #include "hw/isa/isa.h"
 #include "hw/pci/pci.h"
 #include "hw/pci/pci_host.h"
@@ -104,6 +105,8 @@ static void ppc_heathrow_init(MachineState *machine)
     DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
     void *fw_cfg;
     uint64_t tbfreq;
+    uint8_t *spd_data[3] = {};
+    I2CBus *i2c_bus;
 
     /* init CPUs */
     for (i = 0; i < smp_cpus; i++) {
@@ -121,8 +124,16 @@ static void ppc_heathrow_init(MachineState *machine)
                      "maximum 2047 MB", ram_size / MiB);
         exit(1);
     }
-
     memory_region_add_subregion(get_system_memory(), 0, machine->ram);
+    for (i = 0; i < 3; i++) {
+        int size_left = ram_size - i * 512 * MiB;
+        if (size_left > 0) {
+            uint32_t s = size_left / MiB;
+            s = (s > 512 ? 512 : s);
+            s = 1U << (31 - clz32(s));
+            spd_data[i] = spd_data_generate(SDR, s * MiB);
+        }
+    }
 
     /* allocate and load firmware ROM */
     memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE,
@@ -302,6 +313,12 @@ static void ppc_heathrow_init(MachineState *machine)
     macio_ide_init_drives(macio_ide, &hd[MAX_IDE_DEVS]);
 
     dev = DEVICE(object_resolve_path_component(OBJECT(macio), "cuda"));
+    i2c_bus = I2C_BUS(qdev_get_child_bus(dev, "i2c"));
+    for (i = 0; i < 3; i++) {
+        if (spd_data[i]) {
+            smbus_eeprom_init_one(i2c_bus, 0x50 + i, spd_data[i]);
+        }
+    }
     adb_bus = qdev_get_child_bus(dev, "adb.0");
     dev = qdev_new(TYPE_ADB_KEYBOARD);
     qdev_realize_and_unref(dev, adb_bus, &error_fatal);
-- 
2.21.3

Re: [PATCH v7 8/8] mac_oldworld: Add SPD data to cover RAM

[Philippe Mathieu-Daudé]

On 6/29/20 8:55 PM, BALATON Zoltan wrote:
> OpenBIOS gets RAM size via fw_cfg but rhe original board firmware

Typo "the".

> detects RAM using SPD data so generate and add SDP eeproms to cover as

EEPROMs?

> much RAM as possible to describe with SPD (this may be less than the
> actual ram_size due to SDRAM size constraints).
> 
> This patch is more complex as it should be which I intend to fix once
> agreement can be made on how to get back the necessary functionality
> removed by previous patches. See in this thread:
> https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg08710.html
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/ppc/mac_oldworld.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index 6276973c95..6a27287c9f 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -34,6 +34,7 @@
>   #include "hw/input/adb.h"
>   #include "sysemu/sysemu.h"
>   #include "net/net.h"
> +#include "hw/i2c/smbus_eeprom.h"
>   #include "hw/isa/isa.h"
>   #include "hw/pci/pci.h"
>   #include "hw/pci/pci_host.h"
> @@ -104,6 +105,8 @@ static void ppc_heathrow_init(MachineState *machine)
>       DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>       void *fw_cfg;
>       uint64_t tbfreq;
> +    uint8_t *spd_data[3] = {};
> +    I2CBus *i2c_bus;
>   
>       /* init CPUs */
>       for (i = 0; i < smp_cpus; i++) {
> @@ -121,8 +124,16 @@ static void ppc_heathrow_init(MachineState *machine)
>                        "maximum 2047 MB", ram_size / MiB);
>           exit(1);
>       }
> -
>       memory_region_add_subregion(get_system_memory(), 0, machine->ram);
> +    for (i = 0; i < 3; i++) {

3 -> ARRAY_SIZE(spd_data)

> +        int size_left = ram_size - i * 512 * MiB;
> +        if (size_left > 0) {
> +            uint32_t s = size_left / MiB;
> +            s = (s > 512 ? 512 : s);

MIN()?

> +            s = 1U << (31 - clz32(s));
> +            spd_data[i] = spd_data_generate(SDR, s * MiB);
> +        }
> +    }
>   
>       /* allocate and load firmware ROM */
>       memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE,
> @@ -302,6 +313,12 @@ static void ppc_heathrow_init(MachineState *machine)
>       macio_ide_init_drives(macio_ide, &hd[MAX_IDE_DEVS]);
>   
>       dev = DEVICE(object_resolve_path_component(OBJECT(macio), "cuda"));
> +    i2c_bus = I2C_BUS(qdev_get_child_bus(dev, "i2c"));
> +    for (i = 0; i < 3; i++) {

3 -> ARRAY_SIZE(spd_data)

> +        if (spd_data[i]) {
> +            smbus_eeprom_init_one(i2c_bus, 0x50 + i, spd_data[i]);
> +        }
> +    }
>       adb_bus = qdev_get_child_bus(dev, "adb.0");
>       dev = qdev_new(TYPE_ADB_KEYBOARD);
>       qdev_realize_and_unref(dev, adb_bus, &error_fatal);
> 

Re: [PATCH v7 8/8] mac_oldworld: Add SPD data to cover RAM

[BALATON Zoltan via]

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

On Tue, 13 Oct 2020, Philippe Mathieu-Daudé wrote:
> On 6/29/20 8:55 PM, BALATON Zoltan wrote:
>> OpenBIOS gets RAM size via fw_cfg but rhe original board firmware
>
> Typo "the".
>
>> detects RAM using SPD data so generate and add SDP eeproms to cover as
>
> EEPROMs?
>
>> much RAM as possible to describe with SPD (this may be less than the
>> actual ram_size due to SDRAM size constraints).
>> 
>> This patch is more complex as it should be which I intend to fix once
>> agreement can be made on how to get back the necessary functionality
>> removed by previous patches. See in this thread:
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg08710.html
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/ppc/mac_oldworld.c | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
>> index 6276973c95..6a27287c9f 100644
>> --- a/hw/ppc/mac_oldworld.c
>> +++ b/hw/ppc/mac_oldworld.c
>> @@ -34,6 +34,7 @@
>>   #include "hw/input/adb.h"
>>   #include "sysemu/sysemu.h"
>>   #include "net/net.h"
>> +#include "hw/i2c/smbus_eeprom.h"
>>   #include "hw/isa/isa.h"
>>   #include "hw/pci/pci.h"
>>   #include "hw/pci/pci_host.h"
>> @@ -104,6 +105,8 @@ static void ppc_heathrow_init(MachineState *machine)
>>       DriveInfo *hd[MAX_IDE_BUS * MAX_IDE_DEVS];
>>       void *fw_cfg;
>>       uint64_t tbfreq;
>> +    uint8_t *spd_data[3] = {};
>> +    I2CBus *i2c_bus;
>>         /* init CPUs */
>>       for (i = 0; i < smp_cpus; i++) {
>> @@ -121,8 +124,16 @@ static void ppc_heathrow_init(MachineState *machine)
>>                        "maximum 2047 MB", ram_size / MiB);
>>           exit(1);
>>       }
>> -
>>       memory_region_add_subregion(get_system_memory(), 0, machine->ram);
>> +    for (i = 0; i < 3; i++) {
>
> 3 -> ARRAY_SIZE(spd_data)
>
>> +        int size_left = ram_size - i * 512 * MiB;
>> +        if (size_left > 0) {
>> +            uint32_t s = size_left / MiB;
>> +            s = (s > 512 ? 512 : s);
>
> MIN()?

Thanks for the review. (You forgot to cc Mark so maybe he missed the other 
R-b tags but hopefully patchwork will pick these up.)

I plan to rewrite this patch eliminating this part and un-breaking 
spd_data_generate() instead to allow it to signal an error again so we 
don't need to duplicate part of it here to avoid aborting. (You may 
remember the thread where this was discussed in, I don't have a link to it 
now.) So just disregard this one patch for now, I'll take your other 
suggestions in account when submitting a rewritten version.

This and the WIP CUDA i2c patches are not yet ready to be merged and you 
had alternative suggestion for [v7,6/7] i2c: Match parameters of 
i2c_start_transfer and i2c_send_recv, but was that ever merged? Do you 
plan to resubmit or send a ping for that? It may have been lost somewhere 
because I have those mails in my mailbox but could not find it in Patchew 
or Patchwork.

Other than these 3 patches in the series others (1-5) should be OK and 
hope most of those could be taken by Mark now in the upcoming Mac fixes 
series to reduce the size of outstanding patches then I can fix up or 
rewrite the remaining ones.

I'm still not sure what to do about the sign extension problem in 
include/hw/elf_ops.h that was holding back patch 1 so if anyone has a 
suggestion please tell me or feel free to propose any fix for it, I'm not 
confident enough to break the elf loader.

Regards,
BALATON Zoltan

>> +            s = 1U << (31 - clz32(s));
>> +            spd_data[i] = spd_data_generate(SDR, s * MiB);
>> +        }
>> +    }
>>         /* allocate and load firmware ROM */
>>       memory_region_init_rom(bios, NULL, "ppc_heathrow.bios", PROM_SIZE,
>> @@ -302,6 +313,12 @@ static void ppc_heathrow_init(MachineState *machine)
>>       macio_ide_init_drives(macio_ide, &hd[MAX_IDE_DEVS]);
>>         dev = DEVICE(object_resolve_path_component(OBJECT(macio), "cuda"));
>> +    i2c_bus = I2C_BUS(qdev_get_child_bus(dev, "i2c"));
>> +    for (i = 0; i < 3; i++) {
>
> 3 -> ARRAY_SIZE(spd_data)
>
>> +        if (spd_data[i]) {
>> +            smbus_eeprom_init_one(i2c_bus, 0x50 + i, spd_data[i]);
>> +        }
>> +    }
>>       adb_bus = qdev_get_child_bus(dev, "adb.0");
>>       dev = qdev_new(TYPE_ADB_KEYBOARD);
>>       qdev_realize_and_unref(dev, adb_bus, &error_fatal);
>> 
>
>
>

Re: [PATCH v7 8/8] mac_oldworld: Add SPD data to cover RAM

[BALATON Zoltan via]

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

On Wed, 14 Oct 2020, BALATON Zoltan via wrote:
> On Tue, 13 Oct 2020, Philippe Mathieu-Daudé wrote:
>> On 6/29/20 8:55 PM, BALATON Zoltan wrote:
>>> This patch is more complex as it should be which I intend to fix once
>>> agreement can be made on how to get back the necessary functionality
>>> removed by previous patches. See in this thread:
>>> https://lists.nongnu.org/archive/html/qemu-devel/2020-06/msg08710.html
>>> 
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>
> I plan to rewrite this patch eliminating this part and un-breaking 
> spd_data_generate() instead to allow it to signal an error again so we don't 
> need to duplicate part of it here to avoid aborting. (You may remember the 
> thread where this was discussed in, I don't have a link to it now.) So just

Oh, I actually have the link in the commit message... Maybe I should also 
read what I write sometimes.

Regards,
BALATON Zoltan

[PATCH v7 3/8] mac_oldworld: Drop a variable, use get_system_memory() directly

[BALATON Zoltan]

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>
---
 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 baf3da6f90..d1c4244b1e 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;
@@ -128,12 +127,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

Re: [PATCH v7 3/8] mac_oldworld: Drop a variable, use get_system_memory() directly

[Philippe Mathieu-Daudé]

On 6/29/20 8:55 PM, BALATON Zoltan wrote:
> 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>
> ---
>   hw/ppc/mac_oldworld.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[RFC PATCH v7 7/8] WIP macio/cuda: Attempt to add i2c support

[BALATON Zoltan]

This is not a final, RFC patch attempt to implement i2c bus in CUDA
needed for firmware to access SPD data of installed RAM. The skeleton
is there but actual implementation of I2C commands need to be refined
because I don't know how this is supposed to work. In my understanding
after an I2C command (at least for combined transfer) CUDA should
enter a mode where reading subsequent values from VIA[SR] should
return bytes received from the i2C device but not sure what ends this
mode or how to model it correctly. So this patch just returns fixed
amount of bytes expected by reading SPD eeproms just to make testing
the firmware ROM possible. Help fixing and finishing this is welcome,
I don't plan to spend more time with this so just submitted it for
whoever picks this up.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/misc/macio/cuda.c         | 76 +++++++++++++++++++++++++++++++++++-
 include/hw/misc/macio/cuda.h |  1 +
 2 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/hw/misc/macio/cuda.c b/hw/misc/macio/cuda.c
index 5bbc7770fa..3fc9773717 100644
--- a/hw/misc/macio/cuda.c
+++ b/hw/misc/macio/cuda.c
@@ -28,6 +28,7 @@
 #include "hw/ppc/mac.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
+#include "hw/i2c/i2c.h"
 #include "hw/input/adb.h"
 #include "hw/misc/mos6522.h"
 #include "hw/misc/macio/cuda.h"
@@ -370,6 +371,75 @@ static bool cuda_cmd_set_time(CUDAState *s,
     return true;
 }
 
+static bool cuda_cmd_get_set_iic(CUDAState *s,
+                                 const uint8_t *in_data, int in_len,
+                                 uint8_t *out_data, int *out_len)
+{
+    int i;
+
+    qemu_log_mask(LOG_UNIMP, "CUDA: unimplemented GET_SET_IIC %s 0x%x %d\n",
+                  (in_data[0] & 1 ? "read" : "write"), in_data[0] >> 1,
+                  in_len);
+    if (i2c_start_transfer(s->i2c_bus, in_data[0] >> 1, in_data[0] & 1)) {
+        return false;
+    }
+    for (i = 0; i < in_len - 3; i++) {
+        if (i2c_send(s->i2c_bus, in_data[i])) {
+            i2c_end_transfer(s->i2c_bus);
+            return false;
+        }
+    }
+    return true;
+}
+
+static bool cuda_cmd_combined_iic(CUDAState *s,
+                                  const uint8_t *in_data, int in_len,
+                                  uint8_t *out_data, int *out_len)
+{
+    int i;
+
+    if (in_len < 3) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "CUDA: COMBINED_FORMAT_IIC too few input bytes\n");
+        return false;
+    }
+    if ((in_data[0] & 0xfe) != (in_data[2] & 0xfe)) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "CUDA: COMBINED_FORMAT_IIC address mismatch\n");
+        return false;
+    }
+
+    uint8_t data = in_data[1];
+    if (i2c_start_transfer(s->i2c_bus, in_data[0] >> 1, in_data[0] & 1) ||
+        i2c_send_recv(s->i2c_bus, &data, in_data[0] & 1)) {
+        return false;
+    }
+    i2c_end_transfer(s->i2c_bus);
+    if (in_data[2] & 1) {
+        if (i2c_start_transfer(s->i2c_bus, in_data[2] >> 1, in_data[2] & 1)) {
+            i2c_end_transfer(s->i2c_bus);
+            return false;
+        }
+        for (i = 0; i < 5; i++) {
+            if (i2c_send_recv(s->i2c_bus, &out_data[i], in_data[2] & 1)) {
+                i2c_end_transfer(s->i2c_bus);
+                return false;
+            }
+        }
+        *out_len = i;
+        i2c_end_transfer(s->i2c_bus);
+    } else {
+        for (i = 0; i < in_len - 3; i++) {
+            data = in_data[3 + i];
+            if (i2c_send_recv(s->i2c_bus, &data, in_data[2] & 1)) {
+                i2c_end_transfer(s->i2c_bus);
+                return false;
+            }
+        }
+    }
+    return true;
+}
+
 static const CudaCommand handlers[] = {
     { CUDA_AUTOPOLL, "AUTOPOLL", cuda_cmd_autopoll },
     { CUDA_SET_AUTO_RATE, "SET_AUTO_RATE",  cuda_cmd_set_autorate },
@@ -382,6 +452,8 @@ static const CudaCommand handlers[] = {
       cuda_cmd_set_power_message },
     { CUDA_GET_TIME, "GET_TIME", cuda_cmd_get_time },
     { CUDA_SET_TIME, "SET_TIME", cuda_cmd_set_time },
+    { CUDA_GET_SET_IIC, "GET_SET_IIC", cuda_cmd_get_set_iic },
+    { CUDA_COMBINED_FORMAT_IIC, "COMBINED_FORMAT_IIC", cuda_cmd_combined_iic },
 };
 
 static void cuda_receive_packet(CUDAState *s,
@@ -549,6 +621,7 @@ static void cuda_init(Object *obj)
 {
     CUDAState *s = CUDA(obj);
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    DeviceState *dev = DEVICE(obj);
 
     object_initialize_child(obj, "mos6522-cuda", &s->mos6522_cuda,
                             TYPE_MOS6522_CUDA);
@@ -557,7 +630,8 @@ static void cuda_init(Object *obj)
     sysbus_init_mmio(sbd, &s->mem);
 
     qbus_create_inplace(&s->adb_bus, sizeof(s->adb_bus), TYPE_ADB_BUS,
-                        DEVICE(obj), "adb.0");
+                        dev, "adb.0");
+    s->i2c_bus = i2c_init_bus(dev, "i2c");
 }
 
 static Property cuda_properties[] = {
diff --git a/include/hw/misc/macio/cuda.h b/include/hw/misc/macio/cuda.h
index a8cf0be1ec..6856ed7704 100644
--- a/include/hw/misc/macio/cuda.h
+++ b/include/hw/misc/macio/cuda.h
@@ -79,6 +79,7 @@ typedef struct CUDAState {
 
     ADBBusState adb_bus;
     MOS6522CUDAState mos6522_cuda;
+    I2CBus *i2c_bus;
 
     uint32_t tick_offset;
     uint64_t tb_frequency;
-- 
2.21.3

[PATCH v7 2/8] mac_newworld: Allow loading binary ROM image

[BALATON Zoltan]

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>
---
Unlike mac_oldworld where the openbios-ppc image loads at end of ROM
region here we only check size and assume ELF image is loaded from
PROM_BASE, Checking the load addr here is tricky because this board is
also be compiled both 64 and 32 bit and load_elf seems to always
return 64 bit value so handling that could become a mess. If this is a
problem then it's a preexisting one so should be fixed in a separate
patch. This one just allows loading ROM binary too otherwise
preserving previous behaviour.

 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 04e498bc57..195967facd 100644
--- a/hw/ppc/mac.h
+++ b/hw/ppc/mac.h
@@ -40,10 +40,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 828c5992ae..c88142af57 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 */
@@ -153,25 +155,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

Re: [PATCH v7 2/8] mac_newworld: Allow loading binary ROM image

[Philippe Mathieu-Daudé]

On 6/29/20 8:55 PM, BALATON Zoltan wrote:
> 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>

> ---
> Unlike mac_oldworld where the openbios-ppc image loads at end of ROM
> region here we only check size and assume ELF image is loaded from
> PROM_BASE, Checking the load addr here is tricky because this board is
> also be compiled both 64 and 32 bit and load_elf seems to always
> return 64 bit value so handling that could become a mess. If this is a
> problem then it's a preexisting one so should be fixed in a separate
> patch. This one just allows loading ROM binary too otherwise
> preserving previous behaviour.
> 
>   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 04e498bc57..195967facd 100644
> --- a/hw/ppc/mac.h
> +++ b/hw/ppc/mac.h
> @@ -40,10 +40,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 828c5992ae..c88142af57 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 */
> @@ -153,25 +155,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);
>       }
> 

[PATCH v7 1/8] mac_oldworld: Allow loading binary ROM image

[BALATON Zoltan]

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>
---
v4: use load address from ELF to check if ROM is too big

 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 f8c204ead7..baf3da6f90 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)
@@ -99,6 +101,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;
@@ -127,24 +130,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);
+        if (bios_size <= 0) {
+            /* or load binary ROM image */
+            bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE);
+            bios_addr = PROM_BASE;
+        } else {
+            /* load_elf sets high 32 bits for some reason, strip those */
+            bios_addr &= 0xffffffffULL;
+        }
         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

Re: [PATCH v7 1/8] mac_oldworld: Allow loading binary ROM image

[Mark Cave-Ayland]

On 29/06/2020 19:55, BALATON Zoltan wrote:

> 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>
> ---
> v4: use load address from ELF to check if ROM is too big
> 
>  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 f8c204ead7..baf3da6f90 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)
> @@ -99,6 +101,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;
> @@ -127,24 +130,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);
> +        if (bios_size <= 0) {
> +            /* or load binary ROM image */
> +            bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE);
> +            bios_addr = PROM_BASE;
> +        } else {
> +            /* load_elf sets high 32 bits for some reason, strip those */
> +            bios_addr &= 0xffffffffULL;

Repeating my earlier comment from v5: something is wrong here if you need to manually
strip the high bits. If you compare with SPARC32 which uses the same approach, there
is no such strip required - have a look there to try and figure out what's going on here.


ATB,

Mark.

Re: [PATCH v7 1/8] mac_oldworld: Allow loading binary ROM image

[BALATON Zoltan]

On Tue, 30 Jun 2020, Mark Cave-Ayland wrote:
> On 29/06/2020 19:55, BALATON Zoltan wrote:
>> 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>
>> ---
>> v4: use load address from ELF to check if ROM is too big
>>
>>  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 f8c204ead7..baf3da6f90 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)
>> @@ -99,6 +101,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;
>> @@ -127,24 +130,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);
>> +        if (bios_size <= 0) {
>> +            /* or load binary ROM image */
>> +            bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE);
>> +            bios_addr = PROM_BASE;
>> +        } else {
>> +            /* load_elf sets high 32 bits for some reason, strip those */
>> +            bios_addr &= 0xffffffffULL;
>
> Repeating my earlier comment from v5: something is wrong here if you need to manually
> strip the high bits. If you compare with SPARC32 which uses the same approach, there
> is no such strip required - have a look there to try and figure out what's going on here.

OK, the problem here is this:

$ gdb qemu-system-ppc
(gdb) b mac_oldworld.c:146
Breakpoint 1 at 0x416770: file hw/ppc/mac_oldworld.c, line 146.
(gdb) r
Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146
146	    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
(gdb) n
147	    if (filename) {
149	        bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr,
151	        if (bios_size <= 0) {
(gdb) p bios_size
$1 = 755500
(gdb) p/x bios_addr
$2 = 0xfffffffffff00000

this happens within load_elf that I don't feel like wanting to debug but 
causes problem when we use it to calculate bios size later here:

-    if (bios_size < 0 || bios_size > BIOS_SIZE) {
+    if (bios_size < 0 || bios_addr - PROM_BASE + bios_size > PROM_SIZE) {

unless we strip it down to expected 32 bits. This could be some unwanted 
size extension or whatnot but I don't have time to dig deeper.

Now lets see what sun4m does:

$ gdb qemu-system-sparc
(gdb) b sun4m.c:721
Breakpoint 1 at 0x1fc0e6: file hw/sparc/sun4m.c, line 721.
(gdb) r
Thread 1 "qemu-system-spa" hit Breakpoint 1, prom_init (addr=1879048192, bios_name=0x555555b3c60d "openbios-sparc32") at hw/sparc/sun4m.c:721
721	    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
(gdb) p/x addr
$1 = 0x70000000
(gdb) n
722	    if (filename) {
723	        ret = load_elf(filename, NULL,
726	        if (ret < 0 || ret > PROM_SIZE_MAX) {
(gdb) p ret
$2 = 847872
(gdb) p/x addr
$3 = 0x70000000

Hmm, does not happen here, the difference is that this calls load_elf with 
addr already initialised so maybe load_elf only sets low 32 bits? By the 
way, sun4m does not use the returned addr so even if it was wrong it would 
not be noticed,

Maybe initialising addr before calling load_elf in mac_oldworld,c could 
fix this so we can get rid of the fix up? Unfortunately not:

--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -98,7 +98,7 @@ static void ppc_heathrow_init(MachineState *machine)
      SysBusDevice *s;
      DeviceState *dev, *pic_dev;
      BusState *adb_bus;
-    uint64_t bios_addr;
+    uint64_t bios_addr = 0;
      int bios_size;
      unsigned int smp_cpus = machine->smp.cpus;
      uint16_t ppc_boot_device;

$ gdb qemu-system-ppc
[...]
Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146
146	    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
(gdb) p bios_addr
$1 = 0
(gdb) n
147	    if (filename) {
149	        bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr,
151	        if (bios_size <= 0) {
(gdb) p/x bios_addr
$2 = 0xfffffffffff00000

Could this be something about openbios-ppc? I don't know. I give up 
investigating further at this point and let someone else find out.
Any ideas?

Regards,
BALATON Zoltan

Re: [PATCH v7 1/8] mac_oldworld: Allow loading binary ROM image

[David Gibson]

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

On Tue, Jun 30, 2020 at 11:45:42PM +0200, BALATON Zoltan wrote:
> On Tue, 30 Jun 2020, Mark Cave-Ayland wrote:
> > On 29/06/2020 19:55, BALATON Zoltan wrote:
> > > 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>
> > > ---
> > > v4: use load address from ELF to check if ROM is too big
> > > 
> > >  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 f8c204ead7..baf3da6f90 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)
> > > @@ -99,6 +101,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;
> > > @@ -127,24 +130,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);
> > > +        if (bios_size <= 0) {
> > > +            /* or load binary ROM image */
> > > +            bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE);
> > > +            bios_addr = PROM_BASE;
> > > +        } else {
> > > +            /* load_elf sets high 32 bits for some reason, strip those */
> > > +            bios_addr &= 0xffffffffULL;
> > 
> > Repeating my earlier comment from v5: something is wrong here if you need to manually
> > strip the high bits. If you compare with SPARC32 which uses the same approach, there
> > is no such strip required - have a look there to try and figure out what's going on here.
> 
> OK, the problem here is this:
> 
> $ gdb qemu-system-ppc
> (gdb) b mac_oldworld.c:146
> Breakpoint 1 at 0x416770: file hw/ppc/mac_oldworld.c, line 146.
> (gdb) r
> Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146
> 146	    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> (gdb) n
> 147	    if (filename) {
> 149	        bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr,
> 151	        if (bios_size <= 0) {
> (gdb) p bios_size
> $1 = 755500
> (gdb) p/x bios_addr
> $2 = 0xfffffffffff00000
> 
> this happens within load_elf that I don't feel like wanting to debug but
> causes problem when we use it to calculate bios size later here:

I think the problem is here, in include/hw/elf_ops.h:

    if (lowaddr)
        *lowaddr = (uint64_t)(elf_sword)low;

"low" is a u64, but for a 32-bit ELF file, which is what I'm guessing
you're dealing with here, elf_sword is an int32_t.  So the first cast
truncates the high bits, but makes it a signed value, so the second
cast sign extends, resulting in those high bits.

Sign extending rather than zero-extending seems a dubious choice here,
so I wonder if that should be (elf_word) instead of (elf_sword).  But
maybe there's some weird other case where we do want the sign
extension here.

> 
> -    if (bios_size < 0 || bios_size > BIOS_SIZE) {
> +    if (bios_size < 0 || bios_addr - PROM_BASE + bios_size > PROM_SIZE) {
> 
> unless we strip it down to expected 32 bits. This could be some unwanted
> size extension or whatnot but I don't have time to dig deeper.
> 
> Now lets see what sun4m does:
> 
> $ gdb qemu-system-sparc
> (gdb) b sun4m.c:721
> Breakpoint 1 at 0x1fc0e6: file hw/sparc/sun4m.c, line 721.
> (gdb) r
> Thread 1 "qemu-system-spa" hit Breakpoint 1, prom_init (addr=1879048192, bios_name=0x555555b3c60d "openbios-sparc32") at hw/sparc/sun4m.c:721
> 721	    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> (gdb) p/x addr
> $1 = 0x70000000
> (gdb) n
> 722	    if (filename) {
> 723	        ret = load_elf(filename, NULL,
> 726	        if (ret < 0 || ret > PROM_SIZE_MAX) {
> (gdb) p ret
> $2 = 847872
> (gdb) p/x addr
> $3 = 0x70000000
> 
> Hmm, does not happen here, the difference is that this calls load_elf with
> addr already initialised so maybe load_elf only sets low 32 bits? By the
> way, sun4m does not use the returned addr so even if it was wrong it would
> not be noticed,
> 
> Maybe initialising addr before calling load_elf in mac_oldworld,c could fix
> this so we can get rid of the fix up? Unfortunately not:
> 
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -98,7 +98,7 @@ static void ppc_heathrow_init(MachineState *machine)
>      SysBusDevice *s;
>      DeviceState *dev, *pic_dev;
>      BusState *adb_bus;
> -    uint64_t bios_addr;
> +    uint64_t bios_addr = 0;
>      int bios_size;
>      unsigned int smp_cpus = machine->smp.cpus;
>      uint16_t ppc_boot_device;
> 
> $ gdb qemu-system-ppc
> [...]
> Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146
> 146	    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
> (gdb) p bios_addr
> $1 = 0
> (gdb) n
> 147	    if (filename) {
> 149	        bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr,
> 151	        if (bios_size <= 0) {
> (gdb) p/x bios_addr
> $2 = 0xfffffffffff00000
> 
> Could this be something about openbios-ppc? I don't know. I give up
> investigating further at this point and let someone else find out.
> Any ideas?
> 
> Regards,
> BALATON Zoltan
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 1/8] mac_oldworld: Allow loading binary ROM image

[BALATON Zoltan]

On Sun, 5 Jul 2020, David Gibson wrote:
> On Tue, Jun 30, 2020 at 11:45:42PM +0200, BALATON Zoltan wrote:
>> On Tue, 30 Jun 2020, Mark Cave-Ayland wrote:
>>> On 29/06/2020 19:55, BALATON Zoltan wrote:
>>>> 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>
>>>> ---
>>>> v4: use load address from ELF to check if ROM is too big
>>>>
>>>>  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 f8c204ead7..baf3da6f90 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)
>>>> @@ -99,6 +101,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;
>>>> @@ -127,24 +130,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);
>>>> +        if (bios_size <= 0) {
>>>> +            /* or load binary ROM image */
>>>> +            bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE);
>>>> +            bios_addr = PROM_BASE;
>>>> +        } else {
>>>> +            /* load_elf sets high 32 bits for some reason, strip those */
>>>> +            bios_addr &= 0xffffffffULL;
>>>
>>> Repeating my earlier comment from v5: something is wrong here if you need to manually
>>> strip the high bits. If you compare with SPARC32 which uses the same approach, there
>>> is no such strip required - have a look there to try and figure out what's going on here.
>>
>> OK, the problem here is this:
>>
>> $ gdb qemu-system-ppc
>> (gdb) b mac_oldworld.c:146
>> Breakpoint 1 at 0x416770: file hw/ppc/mac_oldworld.c, line 146.
>> (gdb) r
>> Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146
>> 146	    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>> (gdb) n
>> 147	    if (filename) {
>> 149	        bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr,
>> 151	        if (bios_size <= 0) {
>> (gdb) p bios_size
>> $1 = 755500
>> (gdb) p/x bios_addr
>> $2 = 0xfffffffffff00000
>>
>> this happens within load_elf that I don't feel like wanting to debug but
>> causes problem when we use it to calculate bios size later here:
>
> I think the problem is here, in include/hw/elf_ops.h:
>
>    if (lowaddr)
>        *lowaddr = (uint64_t)(elf_sword)low;
>
> "low" is a u64, but for a 32-bit ELF file, which is what I'm guessing
> you're dealing with here, elf_sword is an int32_t.  So the first cast
> truncates the high bits, but makes it a signed value, so the second
> cast sign extends, resulting in those high bits.

Thanks for finding this, this looks like a likely reason.

> Sign extending rather than zero-extending seems a dubious choice here,
> so I wonder if that should be (elf_word) instead of (elf_sword).  But
> maybe there's some weird other case where we do want the sign
> extension here.

Looks like most callers which get this value don't even use it, I've 
submitted a patch to clean that up. Of the remaining callers at least 
hppa does similar truncating but uses a cast instead of explicit masking. 
I'll update my mac patches to do the same.

Regards,
BALATON Zoltan

>> -    if (bios_size < 0 || bios_size > BIOS_SIZE) {
>> +    if (bios_size < 0 || bios_addr - PROM_BASE + bios_size > PROM_SIZE) {
>>
>> unless we strip it down to expected 32 bits. This could be some unwanted
>> size extension or whatnot but I don't have time to dig deeper.
>>
>> Now lets see what sun4m does:
>>
>> $ gdb qemu-system-sparc
>> (gdb) b sun4m.c:721
>> Breakpoint 1 at 0x1fc0e6: file hw/sparc/sun4m.c, line 721.
>> (gdb) r
>> Thread 1 "qemu-system-spa" hit Breakpoint 1, prom_init (addr=1879048192, bios_name=0x555555b3c60d "openbios-sparc32") at hw/sparc/sun4m.c:721
>> 721	    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>> (gdb) p/x addr
>> $1 = 0x70000000
>> (gdb) n
>> 722	    if (filename) {
>> 723	        ret = load_elf(filename, NULL,
>> 726	        if (ret < 0 || ret > PROM_SIZE_MAX) {
>> (gdb) p ret
>> $2 = 847872
>> (gdb) p/x addr
>> $3 = 0x70000000
>>
>> Hmm, does not happen here, the difference is that this calls load_elf with
>> addr already initialised so maybe load_elf only sets low 32 bits? By the
>> way, sun4m does not use the returned addr so even if it was wrong it would
>> not be noticed,
>>
>> Maybe initialising addr before calling load_elf in mac_oldworld,c could fix
>> this so we can get rid of the fix up? Unfortunately not:
>>
>> --- a/hw/ppc/mac_oldworld.c
>> +++ b/hw/ppc/mac_oldworld.c
>> @@ -98,7 +98,7 @@ static void ppc_heathrow_init(MachineState *machine)
>>      SysBusDevice *s;
>>      DeviceState *dev, *pic_dev;
>>      BusState *adb_bus;
>> -    uint64_t bios_addr;
>> +    uint64_t bios_addr = 0;
>>      int bios_size;
>>      unsigned int smp_cpus = machine->smp.cpus;
>>      uint16_t ppc_boot_device;
>>
>> $ gdb qemu-system-ppc
>> [...]
>> Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146
>> 146	    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>> (gdb) p bios_addr
>> $1 = 0
>> (gdb) n
>> 147	    if (filename) {
>> 149	        bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr,
>> 151	        if (bios_size <= 0) {
>> (gdb) p/x bios_addr
>> $2 = 0xfffffffffff00000
>>
>> Could this be something about openbios-ppc? I don't know. I give up
>> investigating further at this point and let someone else find out.
>> Any ideas?
>>
>> Regards,
>> BALATON Zoltan
>>
>
>

Re: [PATCH v7 1/8] mac_oldworld: Allow loading binary ROM image

[Mark Cave-Ayland]

On 05/07/2020 08:31, David Gibson wrote:

> On Tue, Jun 30, 2020 at 11:45:42PM +0200, BALATON Zoltan wrote:
>> On Tue, 30 Jun 2020, Mark Cave-Ayland wrote:
>>> On 29/06/2020 19:55, BALATON Zoltan wrote:
>>>> 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>
>>>> ---
>>>> v4: use load address from ELF to check if ROM is too big
>>>>
>>>>  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 f8c204ead7..baf3da6f90 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)
>>>> @@ -99,6 +101,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;
>>>> @@ -127,24 +130,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);
>>>> +        if (bios_size <= 0) {
>>>> +            /* or load binary ROM image */
>>>> +            bios_size = load_image_targphys(filename, PROM_BASE, PROM_SIZE);
>>>> +            bios_addr = PROM_BASE;
>>>> +        } else {
>>>> +            /* load_elf sets high 32 bits for some reason, strip those */
>>>> +            bios_addr &= 0xffffffffULL;
>>>
>>> Repeating my earlier comment from v5: something is wrong here if you need to manually
>>> strip the high bits. If you compare with SPARC32 which uses the same approach, there
>>> is no such strip required - have a look there to try and figure out what's going on here.
>>
>> OK, the problem here is this:
>>
>> $ gdb qemu-system-ppc
>> (gdb) b mac_oldworld.c:146
>> Breakpoint 1 at 0x416770: file hw/ppc/mac_oldworld.c, line 146.
>> (gdb) r
>> Thread 1 "qemu-system-ppc" hit Breakpoint 1, ppc_heathrow_init (machine=0x555556863800) at hw/ppc/mac_oldworld.c:146
>> 146	    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>> (gdb) n
>> 147	    if (filename) {
>> 149	        bios_size = load_elf(filename, NULL, NULL, NULL, NULL, &bios_addr,
>> 151	        if (bios_size <= 0) {
>> (gdb) p bios_size
>> $1 = 755500
>> (gdb) p/x bios_addr
>> $2 = 0xfffffffffff00000
>>
>> this happens within load_elf that I don't feel like wanting to debug but
>> causes problem when we use it to calculate bios size later here:
> 
> I think the problem is here, in include/hw/elf_ops.h:
> 
>     if (lowaddr)
>         *lowaddr = (uint64_t)(elf_sword)low;
> 
> "low" is a u64, but for a 32-bit ELF file, which is what I'm guessing
> you're dealing with here, elf_sword is an int32_t.  So the first cast
> truncates the high bits, but makes it a signed value, so the second
> cast sign extends, resulting in those high bits.
> 
> Sign extending rather than zero-extending seems a dubious choice here,
> so I wonder if that should be (elf_word) instead of (elf_sword).  But
> maybe there's some weird other case where we do want the sign
> extension here.

I agree that sign-extending here feels odd since it will cause problems with 32-bit
values on 64-bit systems like we see here, however I'm not really familiar enough
with the QEMU ELF loader to know what the intent was here.

The original reference for this was SPARC32 which does a similar thing, but
ultimately the resulting address is never used and the ROM is loaded at a fixed
address which is why it doesn't matter here.

Given that this needs to work with both qemu-system-ppc and qemu-system-ppc64 would
casting bios_addr to target_ulong work?


ATB,

Mark.

Re: [PATCH v7 0/8] Mac Old World ROM experiment

[Mark Cave-Ayland]

On 29/06/2020 19:55, BALATON Zoltan wrote:

> This is now a minimal set of patches needed to make it possible to
> experiment with a firmware ROM from real hardware. After finding out
> that the board firmware does not probe PCI devices but expects them at
> known fixed addresses we only need to change the address of the macio
> device to get the firmware correctly map it. This allows dropping
> workarounds in previous versions for this and now only the minimal set
> of patches are included to get the firmware loaded and do something.
> (Also excluded the grackle revision and machine ID pathes for now that
> may be needed as the firmware accesses these but seems to go further
> without them so until we hit a problem we can live without it,
> although I wonder if this causes us unnecessary debugging later so
> unless they cause regressions they could be merged).
> 
> I still don't get video output but at least it talks to the GPU chip
> now so it can be debugged and improved (this will either need
> emulating the correct chip the firmware has a driver for or an OF
> compliant ROM for the emulated card).
> 
> As before the I2C part (patches 6-8) is RFC and unfinished but the
> first 5 patches should be good enough now. I hope someone can take
> care of I2C, I can look at the ati-vga side later.

If you can sort out the issue with masking in patches 1 and 2 then I'd be happy to
take patches 1-5. Obviously there is still some discussion around the i2c part, so I
can wait a few more days to see what the outcome is there: the patches generally seem
okay, the one change I would like to see is to add a comment around the SPD parts
mentioning that these are only used by the real G3 ROM and not OpenBIOS.

My only concern is whether an incomplete i2c implementation could cause OSs that
currently boot to hang, so it is important that you can test a variety of OS images
from MacOS to Linux and BSD to ensure that it doesn't cause any regression.


ATB,

Mark.

Re: [PATCH v7 0/8] Mac Old World ROM experiment

[BALATON Zoltan]

On Tue, 30 Jun 2020, Mark Cave-Ayland wrote:
> On 29/06/2020 19:55, BALATON Zoltan wrote:
>> This is now a minimal set of patches needed to make it possible to
>> experiment with a firmware ROM from real hardware. After finding out
>> that the board firmware does not probe PCI devices but expects them at
>> known fixed addresses we only need to change the address of the macio
>> device to get the firmware correctly map it. This allows dropping
>> workarounds in previous versions for this and now only the minimal set
>> of patches are included to get the firmware loaded and do something.
>> (Also excluded the grackle revision and machine ID pathes for now that
>> may be needed as the firmware accesses these but seems to go further
>> without them so until we hit a problem we can live without it,
>> although I wonder if this causes us unnecessary debugging later so
>> unless they cause regressions they could be merged).
>>
>> I still don't get video output but at least it talks to the GPU chip
>> now so it can be debugged and improved (this will either need
>> emulating the correct chip the firmware has a driver for or an OF
>> compliant ROM for the emulated card).
>>
>> As before the I2C part (patches 6-8) is RFC and unfinished but the
>> first 5 patches should be good enough now. I hope someone can take
>> care of I2C, I can look at the ati-vga side later.
>
> If you can sort out the issue with masking in patches 1 and 2 then I'd be happy to

Patch 2 does not have masking only patch 1, I'll try to have a look and 
reply to that message.

> take patches 1-5. Obviously there is still some discussion around the i2c part, so I
> can wait a few more days to see what the outcome is there: the patches generally seem

The fix to i2c API would be welcome (not sure it will be resolved in a few 
days but we'll see) but it's not the main problem that makes patch 7 RFC. 
Rather the hardcoded returning 5 bytes in cuda_cmd_combined_iic() which is 
probably not how CUDA should work (apart from that I'm also not sure 
cuda_cmd_get_set_iic() is correct) but

1. I don't know how CUDA works
2. Don't have time and interest in larger CUDA model refactoring

so I've only made the changes needed to be able to test the ROM and 
identify what needs to be fixed. This CUDA I2C is one thing that someone 
with more knowledge/interest may want to look at in more detail. If 
there's nobody to make a better patch now maybe we can test if this breaks 
anything and once the I2C API changes settle down I can rebase it as it 
is, maybe also removing i2c attempts from cuda_cmd_get_set_iic() only 
leaving a log there is better as that command only seems to be used to 
access not yet emulated devices and always returns error at the moment 
anyway not reaching any of the i2c calls in there (I think it may be the 
audio mixer device the ROM tries to poke with this command but I'm not 
sure, I've also seen another i2c address, could be some sensor?, maybe 
DingusPPC has some info on these i2c devices but haven't checked it). 
Anyway, this i2c part is not urgent and can come back to it later. If we 
get in the first 5 patches now that would clean my tree sufficiently to 
only leave patches I intend to change later and get rid of the finished 
ones so I have less patches piling up.

> okay, the one change I would like to see is to add a comment around the SPD parts
> mentioning that these are only used by the real G3 ROM and not OpenBIOS.

This patch also has a dependency on spd_data_generate() API that probably 
won't get fixed before the freeze unless I submit a patch but not sure 
I'll have time for that, also it's not useful without the previous I2C 
patch so again we can see where this goes and come back to it with the I2C 
part.

> My only concern is whether an incomplete i2c implementation could cause OSs that
> currently boot to hang, so it is important that you can test a variety of OS images
> from MacOS to Linux and BSD to ensure that it doesn't cause any regression.

I neither have these images nor time to test all of them so I hoped you 
could do this as part of your merge testing or someone else can help out 
here with testing.

Regards,
BALATON Zoltan

Re: [PATCH v7 0/8] Mac Old World ROM experiment

[Howard Spoelstra]

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

If you can sort out the issue with masking in patches 1 and 2 then I'd be
> happy to
> take patches 1-5. Obviously there is still some discussion around the i2c
> part, so I
> can wait a few more days to see what the outcome is there: the patches
> generally seem
> okay, the one change I would like to see is to add a comment around the
> SPD parts
> mentioning that these are only used by the real G3 ROM and not OpenBIOS.
>
> My only concern is whether an incomplete i2c implementation could cause
> OSs that
> currently boot to hang, so it is important that you can test a variety of
> OS images
> from MacOS to Linux and BSD to ensure that it doesn't cause any regression.
>
>
Hi, I tested this patch set both on top of current master and on top of
Mark' screamer branch.

On top of master with mac99 machine
MacOS:
HD boot: all 9.x and all 10.X boot to desktop
CD boot: all 9.x and all 10.X boot to installer
Linux:
HD boot: Fedora 12, Debian 4, Debian Squeeze
CD boot: Debian 10, Lubuntu-16.04 Live boots to desktop
Freebsd tested (Live CD only)
CD boot only: 12.1 boots to black screen (terminal shows: call-method
set-depth failed with error ffffffdf) 11.4 boots to root login.

On top of master with g3beige machine
MacOS:
HD boot: 10.0,10.1 boot to desktop
Linux:
HD boot: Fedora 12 boots to graphical login screen then hangs

On top of screamer branch with mac99 machine
MacOS:
HD boot: 9.0 and 9.1 often hang with audio extension error. 9.2 and all
10.X boot to desktop. Nothing new here.
CD boot: all 9.x and all 10.X boot to installer
Linux:
HD boot: Debian 4 boots to failing X server
CD boot: Lubuntu-16.04 boot to desktop

On top of screamer branch with g3beige machine:
MacOS:
HD boot: 10.0, 10.1 boot to desktop.
CD boot: 10.0 to 10.4 boot to installer
Linux:
HD boot: Fedora 12 boots to graphical login screen then hangs

All in all, I see no regressions.
The boing is beautiful when g3beige/screamer with increased buffer size
boots the G3 rom ;-)

Best,
Howard

[-- Attachment #2: Type: text/html, Size: 2802 bytes --]

Re: [PATCH v7 0/8] Mac Old World ROM experiment

[BALATON Zoltan]

On Thu, 2 Jul 2020, Howard Spoelstra wrote:
> The boing is beautiful when g3beige/screamer with increased buffer size
> boots the G3 rom ;-)

Try it without my RFC I2C patches (checkout the one before i2c which is 
"mac_oldworld: Change PCI address of macio to match real hardware" then 
build that). With that a crashing sound is heard and you get > prompt 
where you can type ? to see a hardware test menu (but most tests don't 
seem to return anything useful and the ROM checksum is failing, seems to 
read from wrong address). I've shown that here:

https://lists.nongnu.org/archive/html/qemu-ppc/2020-06/msg00239.html

I've never seen that on real hardware but I don't have much experience 
with old Macs. I guess you'd get the same if you removed all RAM from a 
machine and connected a serial terminal (in case you have a real beige 
G3). This might also be useful to test emulation if someone wants to 
investigate.

Regards,
BALATON Zoltan

Re: [PATCH v7 0/8] Mac Old World ROM experiment

[BALATON Zoltan]

On Mon, 29 Jun 2020, BALATON Zoltan wrote:
> This is now a minimal set of patches needed to make it possible to
> experiment with a firmware ROM from real hardware. After finding out
> that the board firmware does not probe PCI devices but expects them at
> known fixed addresses we only need to change the address of the macio
> device to get the firmware correctly map it. This allows dropping
> workarounds in previous versions for this and now only the minimal set
> of patches are included to get the firmware loaded and do something.
> (Also excluded the grackle revision and machine ID pathes for now that
> may be needed as the firmware accesses these but seems to go further
> without them so until we hit a problem we can live without it,
> although I wonder if this causes us unnecessary debugging later so
> unless they cause regressions they could be merged).
>
> I still don't get video output but at least it talks to the GPU chip
> now so it can be debugged and improved (this will either need
> emulating the correct chip the firmware has a driver for or an OF
> compliant ROM for the emulated card).
>
> As before the I2C part (patches 6-8) is RFC and unfinished but the
> first 5 patches should be good enough now. I hope someone can take
> care of I2C, I can look at the ati-vga side later.
>
> Regards,
> BALATON Zoltan
>
> BALATON Zoltan (8):
>  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
>  i2c: Match parameters of i2c_start_transfer and i2c_send_recv

Continuing experimenting with getting g3beige work with the original 
firmware ROM here's the current status. The above patches were already 
merged.

>  WIP macio/cuda: Attempt to add i2c support
>  mac_oldworld: Add SPD data to cover RAM

Adding these last two patches on top of Mark's screamer branch and 
increasing SCREAMER_BUFFER_SIZE define to 0x8000 in 
include/hw/audio/screamer.h to avoid a crash and using -memory 256 (as RAM 
size detection seems to be broken maybe due to imcomplete I2C emulation) I 
get the ROM to start but it cannot yet boot. We're past the initial 
OpenFirmware setup, can get a Forth prompt and explore the device tree and 
run Forth and also can start the Toolbox ROM from /AAPL,ROM but then it 
stops somewhere in there without giving any log or diag output. I think it 
may be waiting for some interrupt or missing some of the Heathrow 
emulation but I'm not really sure. I can get these traces:

heathrow_write 0x28 1: 0x80000000
heathrow_read 0x24 1: 0x80000000
heathrow_read 0x2c 1: 0x0
heathrow_write 0x18 0: 0x80000000
heathrow_read 0x14 0: 0x0
heathrow_read 0x1c 0: 0x0
heathrow_write 0x28 1: 0x80000000
heathrow_read 0x24 1: 0x80000000
heathrow_read 0x2c 1: 0x0
heathrow_write 0x18 0: 0x80000000
heathrow_read 0x14 0: 0x0
heathrow_read 0x1c 0: 0x0
portA_write unimplemented
portA_write unimplemented
heathrow_read 0x24 1: 0x80000000
heathrow_write 0x24 1: 0x80000000
heathrow_read 0x14 0: 0x0
heathrow_write 0x14 0: 0x0
heathrow_read 0x24 1: 0x80000000
heathrow_write 0x24 1: 0x80040000
heathrow_set_irq set_irq: num=0x12 level=1
heathrow_write 0x28 1: 0x80000000
heathrow_read 0x24 1: 0x80040000
heathrow_read 0x2c 1: 0x40000
heathrow_write 0x18 0: 0x80000000
heathrow_read 0x14 0: 0x0
heathrow_read 0x1c 0: 0x0
heathrow_write 0x28 1: 0x80000000
heathrow_read 0x24 1: 0x80040000
heathrow_read 0x2c 1: 0x40000
heathrow_write 0x18 0: 0x80000000
heathrow_read 0x14 0: 0x0
heathrow_read 0x1c 0: 0x0
heathrow_write 0x28 1: 0x80000000
heathrow_read 0x24 1: 0x80040000
heathrow_read 0x2c 1: 0x40000
heathrow_write 0x18 0: 0x80000000
heathrow_read 0x14 0: 0x0
heathrow_read 0x1c 0: 0x0

then the last 6 lines are repeating endlessly. Does anybody have an idea 
what these registers are doing and where they are implemented in QEMU? The 
model in hw/intc/heathrow_pic.c seems to be very simple but I'm not sure 
how are these connected to the rest of the mac_oldworld g3beige machine. 
I've checked that the IRQ numbers defined in include/hw/misc/macio/macio.h 
seems to match what's in the device tree generated by the ROM but there 
are some missing devices we don't emulate (such as SWIM floppy, PMAC 
ethernet and MESH SCSI). Yet the above looks like IRQ 0x12 is raised which 
should be CUDA and there were some portA write errors before that so maybe 
something with VIA or CUDA emulation? Any insight on this anyone?

Regards,
BALATON Zoltan

> hw/display/sm501.c           |  2 +-
> hw/i2c/core.c                | 34 +++++++-------
> hw/i2c/ppc4xx_i2c.c          |  2 +-
> hw/misc/macio/cuda.c         | 76 ++++++++++++++++++++++++++++++-
> hw/ppc/mac.h                 |  2 -
> hw/ppc/mac_newworld.c        | 22 +++++----
> hw/ppc/mac_oldworld.c        | 86 +++++++++++++++++++++++-------------
> include/hw/i2c/i2c.h         |  4 +-
> include/hw/misc/macio/cuda.h |  1 +
> 9 files changed, 167 insertions(+), 62 deletions(-)
>
>

Re: [PATCH v7 0/8] Mac Old World ROM experiment

[BALATON Zoltan]

On Sun, 22 Jan 2023, BALATON Zoltan wrote:
> On Mon, 29 Jun 2020, BALATON Zoltan wrote:
>> This is now a minimal set of patches needed to make it possible to
>> experiment with a firmware ROM from real hardware. After finding out
>> that the board firmware does not probe PCI devices but expects them at
>> known fixed addresses we only need to change the address of the macio
>> device to get the firmware correctly map it. This allows dropping
>> workarounds in previous versions for this and now only the minimal set
>> of patches are included to get the firmware loaded and do something.
>> (Also excluded the grackle revision and machine ID pathes for now that
>> may be needed as the firmware accesses these but seems to go further
>> without them so until we hit a problem we can live without it,
>> although I wonder if this causes us unnecessary debugging later so
>> unless they cause regressions they could be merged).
>> 
>> I still don't get video output but at least it talks to the GPU chip
>> now so it can be debugged and improved (this will either need
>> emulating the correct chip the firmware has a driver for or an OF
>> compliant ROM for the emulated card).
>> 
>> As before the I2C part (patches 6-8) is RFC and unfinished but the
>> first 5 patches should be good enough now. I hope someone can take
>> care of I2C, I can look at the ati-vga side later.
>> 
>> Regards,
>> BALATON Zoltan
>> 
>> BALATON Zoltan (8):
>>  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
>>  i2c: Match parameters of i2c_start_transfer and i2c_send_recv
>
> Continuing experimenting with getting g3beige work with the original firmware 
> ROM here's the current status. The above patches were already merged.
>
>>  WIP macio/cuda: Attempt to add i2c support
>>  mac_oldworld: Add SPD data to cover RAM
>
> Adding these last two patches on top of Mark's screamer branch and increasing 
> SCREAMER_BUFFER_SIZE define to 0x8000 in include/hw/audio/screamer.h to avoid 
> a crash and using -memory 256 (as RAM size detection seems to be broken maybe 
> due to imcomplete I2C emulation) I get the ROM to start but it cannot yet 
> boot. We're past the initial OpenFirmware setup, can get a Forth prompt and 
> explore the device tree and run Forth and also can start the Toolbox ROM from 
> /AAPL,ROM but then it stops somewhere in there without giving any log or diag 
> output. I think it may be waiting for some interrupt or missing some of the 
> Heathrow emulation but I'm not really sure. I can get these traces:
>
> heathrow_write 0x28 1: 0x80000000
> heathrow_read 0x24 1: 0x80000000
> heathrow_read 0x2c 1: 0x0
> heathrow_write 0x18 0: 0x80000000
> heathrow_read 0x14 0: 0x0
> heathrow_read 0x1c 0: 0x0
> heathrow_write 0x28 1: 0x80000000
> heathrow_read 0x24 1: 0x80000000
> heathrow_read 0x2c 1: 0x0
> heathrow_write 0x18 0: 0x80000000
> heathrow_read 0x14 0: 0x0
> heathrow_read 0x1c 0: 0x0
> portA_write unimplemented
> portA_write unimplemented
> heathrow_read 0x24 1: 0x80000000
> heathrow_write 0x24 1: 0x80000000
> heathrow_read 0x14 0: 0x0
> heathrow_write 0x14 0: 0x0
> heathrow_read 0x24 1: 0x80000000
> heathrow_write 0x24 1: 0x80040000
> heathrow_set_irq set_irq: num=0x12 level=1
> heathrow_write 0x28 1: 0x80000000
> heathrow_read 0x24 1: 0x80040000
> heathrow_read 0x2c 1: 0x40000
> heathrow_write 0x18 0: 0x80000000
> heathrow_read 0x14 0: 0x0
> heathrow_read 0x1c 0: 0x0
> heathrow_write 0x28 1: 0x80000000
> heathrow_read 0x24 1: 0x80040000
> heathrow_read 0x2c 1: 0x40000
> heathrow_write 0x18 0: 0x80000000
> heathrow_read 0x14 0: 0x0
> heathrow_read 0x1c 0: 0x0
> heathrow_write 0x28 1: 0x80000000
> heathrow_read 0x24 1: 0x80040000
> heathrow_read 0x2c 1: 0x40000
> heathrow_write 0x18 0: 0x80000000
> heathrow_read 0x14 0: 0x0
> heathrow_read 0x1c 0: 0x0

Adding some cuda* traces and info via output in case that helps to tell 
what's happening:

portA_write unimplemented
cuda_delay_set_sr_int
cuda_delay_set_sr_int
cuda_packet_send length 5
cuda_packet_send_data [0] 0x00
cuda_packet_send_data [1] 0x40
cuda_packet_send_data [2] 0x2c
cuda_packet_send_data [3] 0xa4
cuda_packet_send_data [4] 0xff
cuda_delay_set_sr_int
portA_write unimplemented
heathrow_set_irq set_irq: num=0x12 level=1
(qemu) info via
mos6522-cuda:
   Registers:
     ORB :    0x30
     ORA :    0x10
     DDRB:    0x30
     DDRA:    0x58
     T1CL:    0x11
     T1CH:    0x14
     T1LL:    0xff
     T1LH:    0xff
     T2CL:    0x1b
     T2CH:    0x88
     SR  :    0xff
     ACR :    0x1c
     PCR :    0x0
     IFR :    0x60
     IER :    0x20
   Timers:
     Using current time now(ns)=33052813591
     T1 freq(hz)=783333 mode=one-shot counter=0x1411 latch=0xffff
        load_time(ns)=0 next_irq_time(ns)=33131055374
     T2 freq(hz)=1276 mode=one-shot counter=0x881b latch=0x30c
        load_time(ns)=257468378 next_irq_time(ns)=33349161167

> then the last 6 lines are repeating endlessly. Does anybody have an idea what 
> these registers are doing and where they are implemented in QEMU? The model 
> in hw/intc/heathrow_pic.c seems to be very simple but I'm not sure how are 
> these connected to the rest of the mac_oldworld g3beige machine. I've checked 
> that the IRQ numbers defined in include/hw/misc/macio/macio.h seems to match 
> what's in the device tree generated by the ROM but there are some missing 
> devices we don't emulate (such as SWIM floppy, PMAC ethernet and MESH SCSI). 
> Yet the above looks like IRQ 0x12 is raised which should be CUDA and there 
> were some portA write errors before that so maybe something with VIA or CUDA 
> emulation? Any insight on this anyone?
>
> Regards,
> BALATON Zoltan
>
>> hw/display/sm501.c           |  2 +-
>> hw/i2c/core.c                | 34 +++++++-------
>> hw/i2c/ppc4xx_i2c.c          |  2 +-
>> hw/misc/macio/cuda.c         | 76 ++++++++++++++++++++++++++++++-
>> hw/ppc/mac.h                 |  2 -
>> hw/ppc/mac_newworld.c        | 22 +++++----
>> hw/ppc/mac_oldworld.c        | 86 +++++++++++++++++++++++-------------
>> include/hw/i2c/i2c.h         |  4 +-
>> include/hw/misc/macio/cuda.h |  1 +
>> 9 files changed, 167 insertions(+), 62 deletions(-)
>> 
>> 
>