[PATCH 0/4] ppc/amigaone patches

[PATCH 0/4] ppc/amigaone patches

[BALATON Zoltan]

Hello,

This series adds NVRAM and support for -kernel, -initrd and -append
options to the amigaone machine. This makes it easier to boot AmigaOS
and avoids a crash in the guest when it tries to access NVRAM.

While the -kernel option emulates what U-Boot passes to the kernel,
old Linux kernels for amigaone may not work with it because of two
reasons: these come in legacy U-Boot Multi-File image format that QEMU
cannot read and even after unpacking that and creating a kernel uimage
it won't find PCI devices because it does not initialise them
correctly. This works when booted from U-Boot because U-Boot inits PCI
devices. So does my BBoot loader which can be used to load AmigaOS so
I don't intend to emulate that part of U-Boot.

I'd like this to be merged for the next release please. When merging
please update https://wiki.qemu.org/ChangeLog/10.0 with the following:

amigaone

Added support for NVRAM and -kernel, -initrd, -append command line
options. By default the NVRAM contents are not preserved between
sessions. To make it persistent create a backing file with 'qemu-image
create -f raw nvram.bin 4k' and add -drive
if=mtd,format=raw,file=nvram.bin to keep NVRAM contents in the backing
file so settings stored in it will be preserved between sessions.

To run AmigaOS with BBoot using the -kernel option at least BBoot
version 0.8 is needed. Older BBoot versions only work with -device
loader and cannot be used with -kernel on amigaone.

Regards,

BALATON Zoltan (4):
  ppc/amigaone: Simplify replacement dummy_fw
  ppc/amigaone: Implement NVRAM emulation
  ppc/amigaone: Add default environment
  ppc/amigaone: Add kernel and initrd support

 hw/ppc/amigaone.c | 269 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 261 insertions(+), 8 deletions(-)

-- 
2.30.9

[PATCH 1/4] ppc/amigaone: Simplify replacement dummy_fw

[BALATON Zoltan]

There's no need to do shift in a loop, doing it in one instruction
works just as well, only the result is used.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/amigaone.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
index b02792221c..4290d58613 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -41,10 +41,7 @@
 
 /* AmigaOS calls this routine from ROM, use this if no firmware loaded */
 static const char dummy_fw[] = {
-    0x38, 0x00, 0x00, 0x08, /* li      r0,8 */
-    0x7c, 0x09, 0x03, 0xa6, /* mtctr   r0 */
-    0x54, 0x63, 0xf8, 0x7e, /* srwi    r3,r3,1 */
-    0x42, 0x00, 0xff, 0xfc, /* bdnz    0x8 */
+    0x54, 0x63, 0xc2, 0x3e, /* srwi    r3,r3,8 */
     0x7c, 0x63, 0x18, 0xf8, /* not     r3,r3 */
     0x4e, 0x80, 0x00, 0x20, /* blr */
 };
-- 
2.30.9

[PATCH 2/4] ppc/amigaone: Implement NVRAM emulation

[BALATON Zoltan]

The board has a battery backed NVRAM where U-Boot environment is
stored which is also accessed by AmigaOS and e.g. C:NVGetVar command
crashes without it having at least a valid checksum.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/amigaone.c | 116 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 113 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
index 4290d58613..5273543460 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -21,10 +21,13 @@
 #include "hw/ide/pci.h"
 #include "hw/i2c/smbus_eeprom.h"
 #include "hw/ppc/ppc.h"
+#include "system/block-backend.h"
 #include "system/qtest.h"
 #include "system/reset.h"
 #include "kvm_ppc.h"
 
+#include <zlib.h> /* for crc32 */
+
 #define BUS_FREQ_HZ 100000000
 
 /*
@@ -46,6 +49,103 @@ static const char dummy_fw[] = {
     0x4e, 0x80, 0x00, 0x20, /* blr */
 };
 
+#define NVRAM_ADDR 0xfd0e0000
+#define NVRAM_SIZE (4 * KiB)
+
+#define TYPE_A1_NVRAM "a1-nvram"
+OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
+
+struct A1NVRAMState {
+    SysBusDevice parent_obj;
+
+    MemoryRegion mr;
+    BlockBackend *blk;
+};
+
+/* read callback not used because of romd mode, only here just in case */
+static uint64_t nvram_read(void *opaque, hwaddr addr, unsigned int size)
+{
+    A1NVRAMState *s = opaque;
+    uint8_t *p = memory_region_get_ram_ptr(&s->mr);
+
+    return p[addr];
+}
+
+static void nvram_write(void *opaque, hwaddr addr, uint64_t val,
+                        unsigned int size)
+{
+    A1NVRAMState *s = opaque;
+    uint8_t *p = memory_region_get_ram_ptr(&s->mr);
+
+    p[addr] = val;
+    if (s->blk) {
+        blk_pwrite(s->blk, addr, 1, &val, 0);
+    }
+}
+
+static const MemoryRegionOps nvram_ops = {
+    .read = nvram_read,
+    .write = nvram_write,
+    .endianness = DEVICE_BIG_ENDIAN,
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+};
+
+static void nvram_realize(DeviceState *dev, Error **errp)
+{
+    A1NVRAMState *s = A1_NVRAM(dev);
+    void *p;
+    uint32_t *c;
+
+    memory_region_init_rom_device(&s->mr, NULL, &nvram_ops, s, "nvram",
+                                  NVRAM_SIZE, &error_fatal);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr);
+    c = p = memory_region_get_ram_ptr(&s->mr);
+    if (s->blk) {
+        if (blk_getlength(s->blk) != NVRAM_SIZE) {
+            error_setg(errp, "NVRAM backing file size must be %ld bytes",
+                       NVRAM_SIZE);
+            return;
+        }
+        blk_set_perm(s->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
+                     BLK_PERM_ALL, &error_fatal);
+        if (blk_pread(s->blk, 0, NVRAM_SIZE, p, 0) < 0) {
+            error_setg(errp, "Cannot read NVRAM contents from backing file");
+            return;
+        }
+    }
+    if (*c == 0) {
+        *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
+        if (s->blk) {
+            blk_pwrite(s->blk, 0, 4, p, 0);
+        }
+    }
+}
+
+static const Property nvram_properties[] = {
+    DEFINE_PROP_DRIVE("drive", A1NVRAMState, blk),
+};
+
+static void nvram_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->realize = nvram_realize;
+    device_class_set_props(dc, nvram_properties);
+}
+
+static const TypeInfo nvram_types[] = {
+    {
+        .name = TYPE_A1_NVRAM,
+        .parent = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(A1NVRAMState),
+        .class_init = nvram_class_init,
+    },
+};
+DEFINE_TYPES(nvram_types)
+
 static void amigaone_cpu_reset(void *opaque)
 {
     PowerPCCPU *cpu = opaque;
@@ -72,7 +172,7 @@ static void amigaone_init(MachineState *machine)
     DeviceState *dev;
     I2CBus *i2c_bus;
     uint8_t *spd_data;
-    int i;
+    DriveInfo *di;
 
     /* init CPU */
     cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
@@ -97,6 +197,16 @@ static void amigaone_init(MachineState *machine)
         memory_region_add_subregion(get_system_memory(), 0x40000000, mr);
     }
 
+    /* nvram */
+    dev = qdev_new(TYPE_A1_NVRAM);
+    di = drive_get(IF_MTD, 0, 0);
+    if (di) {
+        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di));
+    }
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
+    memory_region_add_subregion(get_system_memory(), NVRAM_ADDR,
+                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
+
     /* allocate and load firmware */
     rom = g_new(MemoryRegion, 1);
     memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
@@ -136,7 +246,7 @@ static void amigaone_init(MachineState *machine)
     pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
     mr = g_new(MemoryRegion, 1);
     memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", pci_mem,
-                             0, 0x1000000);
+                             0, 0xe0000);
     memory_region_add_subregion(get_system_memory(), 0xfd000000, mr);
     mr = g_new(MemoryRegion, 1);
     memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", pci_mem,
@@ -153,7 +263,7 @@ static void amigaone_init(MachineState *machine)
     qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
                                 qdev_get_gpio_in(DEVICE(cpu),
                                 PPC6xx_INPUT_INT));
-    for (i = 0; i < PCI_NUM_PINS; i++) {
+    for (int i = 0; i < PCI_NUM_PINS; i++) {
         qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
                                                              "pirq", i));
     }
-- 
2.30.9

[PATCH 3/4] ppc/amigaone: Add default environment

[BALATON Zoltan]

Initialise empty NVRAM with default values. This also enables IDE UDMA
mode in AmigaOS that is faster but has to be enabled in environment
due to problems with real hardware but that does not affect emulation
so we can use faster defaults here.

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

diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
index 5273543460..35e4075cc3 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -52,6 +52,28 @@ static const char dummy_fw[] = {
 #define NVRAM_ADDR 0xfd0e0000
 #define NVRAM_SIZE (4 * KiB)
 
+static char default_env[] =
+    "baudrate=115200\0"
+    "stdout=vga\0"
+    "stdin=ps2kbd\0"
+    "bootcmd=boota; menu; run menuboot_cmd\0"
+    "boot1=ide\0"
+    "boot2=cdrom\0"
+    "boota_timeout=3\0"
+    "ide_doreset=on\0"
+    "pci_irqa=9\0"
+    "pci_irqa_select=level\0"
+    "pci_irqb=10\0"
+    "pci_irqb_select=level\0"
+    "pci_irqc=11\0"
+    "pci_irqc_select=level\0"
+    "pci_irqd=7\0"
+    "pci_irqd_select=level\0"
+    "a1ide_irq=1111\0"
+    "a1ide_xfer=FFFF\0";
+#define CRC32_DEFAULT_ENV 0xb5548481
+#define CRC32_ALL_ZEROS   0x603b0489
+
 #define TYPE_A1_NVRAM "a1-nvram"
 OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
 
@@ -97,7 +119,7 @@ static void nvram_realize(DeviceState *dev, Error **errp)
 {
     A1NVRAMState *s = A1_NVRAM(dev);
     void *p;
-    uint32_t *c;
+    uint32_t crc, *c;
 
     memory_region_init_rom_device(&s->mr, NULL, &nvram_ops, s, "nvram",
                                   NVRAM_SIZE, &error_fatal);
@@ -116,12 +138,25 @@ static void nvram_realize(DeviceState *dev, Error **errp)
             return;
         }
     }
+    crc = crc32(0, p + 4, NVRAM_SIZE - 4);
+    if (crc == CRC32_ALL_ZEROS) { /* If env is uninitialized set default */
+        *c = cpu_to_be32(CRC32_DEFAULT_ENV);
+        /* Also copies terminating \0 as env is terminated by \0\0 */
+        memcpy(p + 4, default_env, sizeof(default_env));
+        if (s->blk) {
+            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0);
+        }
+        return;
+    }
     if (*c == 0) {
         *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
         if (s->blk) {
             blk_pwrite(s->blk, 0, 4, p, 0);
         }
     }
+    if (be32_to_cpu(*c) != crc) {
+        warn_report("NVRAM checksum mismatch");
+    }
 }
 
 static const Property nvram_properties[] = {
-- 
2.30.9

[PATCH 4/4] ppc/amigaone: Add kernel and initrd support

[BALATON Zoltan]

Add support for -kernel, -initrd and -append command line options.

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

diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
index 35e4075cc3..94682de1c7 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -25,11 +25,14 @@
 #include "system/qtest.h"
 #include "system/reset.h"
 #include "kvm_ppc.h"
+#include "elf.h"
 
 #include <zlib.h> /* for crc32 */
 
 #define BUS_FREQ_HZ 100000000
 
+#define INITRD_MIN_ADDR 0x600000
+
 /*
  * Firmware binary available at
  * https://www.hyperion-entertainment.com/index.php/downloads?view=files&parent=28
@@ -181,12 +184,68 @@ static const TypeInfo nvram_types[] = {
 };
 DEFINE_TYPES(nvram_types)
 
+struct boot_info {
+    hwaddr entry;
+    hwaddr stack;
+    hwaddr bd_info;
+    hwaddr initrd_start;
+    hwaddr initrd_end;
+    hwaddr cmdline_start;
+    hwaddr cmdline_end;
+};
+
+/* Board info struct from U-Boot */
+struct bd_info {
+    uint32_t bi_memstart;
+    uint32_t bi_memsize;
+    uint32_t bi_flashstart;
+    uint32_t bi_flashsize;
+    uint32_t bi_flashoffset;
+    uint32_t bi_sramstart;
+    uint32_t bi_sramsize;
+    uint32_t bi_bootflags;
+    uint32_t bi_ip_addr;
+    uint8_t  bi_enetaddr[6];
+    uint16_t bi_ethspeed;
+    uint32_t bi_intfreq;
+    uint32_t bi_busfreq;
+    uint32_t bi_baudrate;
+} QEMU_PACKED;
+
+static void create_bd_info(hwaddr addr, ram_addr_t ram_size)
+{
+    struct bd_info *bd = g_new0(struct bd_info, 1);
+
+    bd->bi_memsize =    cpu_to_be32(ram_size);
+    bd->bi_flashstart = cpu_to_be32(PROM_ADDR);
+    bd->bi_flashsize =  cpu_to_be32(1); /* match what U-Boot detects */
+    bd->bi_bootflags =  cpu_to_be32(1);
+    bd->bi_intfreq =    cpu_to_be32(11.5 * BUS_FREQ_HZ);
+    bd->bi_busfreq =    cpu_to_be32(BUS_FREQ_HZ);
+    bd->bi_baudrate =   cpu_to_be32(115200);
+
+    cpu_physical_memory_write(addr, bd, sizeof(*bd));
+}
+
 static void amigaone_cpu_reset(void *opaque)
 {
     PowerPCCPU *cpu = opaque;
+    CPUPPCState *env = &cpu->env;
 
     cpu_reset(CPU(cpu));
-    cpu_ppc_tb_reset(&cpu->env);
+    if (env->load_info) {
+        struct boot_info *bi = env->load_info;
+
+        env->gpr[1] = bi->stack;
+        env->gpr[2] = 1024;
+        env->gpr[3] = bi->bd_info;
+        env->gpr[4] = bi->initrd_start;
+        env->gpr[5] = bi->initrd_end;
+        env->gpr[6] = bi->cmdline_start;
+        env->gpr[7] = bi->cmdline_end;
+        env->nip = bi->entry;
+    }
+    cpu_ppc_tb_reset(env);
 }
 
 static void fix_spd_data(uint8_t *spd)
@@ -208,6 +267,8 @@ static void amigaone_init(MachineState *machine)
     I2CBus *i2c_bus;
     uint8_t *spd_data;
     DriveInfo *di;
+    hwaddr loadaddr;
+    struct boot_info *bi = NULL;
 
     /* init CPU */
     cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
@@ -304,6 +365,56 @@ static void amigaone_init(MachineState *machine)
     }
     pci_ide_create_devs(PCI_DEVICE(object_resolve_path_component(via, "ide")));
     pci_vga_init(pci_bus);
+
+    if (!machine->kernel_filename) {
+        return;
+    }
+
+    /* handle -kernel, -initrd, -append options and emulate U-Boot */
+    bi = g_new0(struct boot_info, 1);
+    cpu->env.load_info = bi;
+
+    loadaddr = MIN(machine->ram_size, 256 * MiB);
+    bi->bd_info = loadaddr - 8 * MiB;
+    create_bd_info(bi->bd_info, machine->ram_size);
+    bi->stack = bi->bd_info - 64 * KiB - 8;
+
+    if (machine->kernel_cmdline && machine->kernel_cmdline[0]) {
+        size_t len = strlen(machine->kernel_cmdline);
+
+        loadaddr = bi->bd_info + 1 * MiB;
+        cpu_physical_memory_write(loadaddr, machine->kernel_cmdline, len + 1);
+        bi->cmdline_start = loadaddr;
+        bi->cmdline_end = loadaddr + len + 1; /* including terminating '\0' */
+    }
+
+    sz = load_elf(machine->kernel_filename, NULL, NULL, NULL,
+                  &bi->entry, &loadaddr, NULL, NULL,
+                  ELFDATA2MSB, PPC_ELF_MACHINE, 0, 0);
+    if (sz <= 0) {
+        sz = load_uimage(machine->kernel_filename, &bi->entry, &loadaddr,
+                         NULL, NULL, NULL);
+    }
+    if (sz <= 0) {
+        error_report("Could not load kernel '%s'",
+                     machine->kernel_filename);
+        exit(1);
+    }
+    loadaddr += sz;
+
+    if (machine->initrd_filename) {
+        loadaddr = ROUND_UP(loadaddr + 4 * MiB, 4 * KiB);
+        loadaddr = MAX(loadaddr, INITRD_MIN_ADDR);
+        sz = load_image_targphys(machine->initrd_filename, loadaddr,
+                                 bi->bd_info - loadaddr);
+        if (sz <= 0) {
+            error_report("Could not load initrd '%s'",
+                         machine->initrd_filename);
+            exit(1);
+        }
+        bi->initrd_start = loadaddr;
+        bi->initrd_end = loadaddr + sz;
+    }
 }
 
 static void amigaone_machine_init(MachineClass *mc)
-- 
2.30.9

Re: [PATCH 0/4] ppc/amigaone patches

[Bernhard Beschow]

Am 22. Februar 2025 17:52:27 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>Hello,
>
>This series adds NVRAM and support for -kernel, -initrd and -append
>options to the amigaone machine. This makes it easier to boot AmigaOS
>and avoids a crash in the guest when it tries to access NVRAM.

Looks like the above information could be added to the changelog. While...

>
>While the -kernel option emulates what U-Boot passes to the kernel,
>old Linux kernels for amigaone may not work with it because of two
>reasons: these come in legacy U-Boot Multi-File image format that QEMU
>cannot read and even after unpacking that and creating a kernel uimage
>it won't find PCI devices because it does not initialise them
>correctly. This works when booted from U-Boot because U-Boot inits PCI
>devices. So does my BBoot loader which can be used to load AmigaOS so
>I don't intend to emulate that part of U-Boot.
>
>I'd like this to be merged for the next release please. When merging
>please update https://wiki.qemu.org/ChangeLog/10.0 with the following:
>
>amigaone
>
>Added support for NVRAM and -kernel, -initrd, -append command line
>options.

... this:

>By default the NVRAM contents are not preserved between
>sessions. To make it persistent create a backing file with 'qemu-image
>create -f raw nvram.bin 4k' and add -drive
>if=mtd,format=raw,file=nvram.bin to keep NVRAM contents in the backing
>file so settings stored in it will be preserved between sessions.
>
>To run AmigaOS with BBoot using the -kernel option at least BBoot
>version 0.8 is needed. Older BBoot versions only work with -device
>loader and cannot be used with -kernel on amigaone.

... information could be added to the amigaone section in the QEMU manual to keep the documentation in one place.

Best regards,
Bernhard

>
>Regards,
>
>BALATON Zoltan (4):
>  ppc/amigaone: Simplify replacement dummy_fw
>  ppc/amigaone: Implement NVRAM emulation
>  ppc/amigaone: Add default environment
>  ppc/amigaone: Add kernel and initrd support
>
> hw/ppc/amigaone.c | 269 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 261 insertions(+), 8 deletions(-)
>

Re: [PATCH 0/4] ppc/amigaone patches

[BALATON Zoltan]

On Mon, 24 Feb 2025, Bernhard Beschow wrote:
> Am 22. Februar 2025 17:52:27 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> Hello,
>>
>> This series adds NVRAM and support for -kernel, -initrd and -append
>> options to the amigaone machine. This makes it easier to boot AmigaOS
>> and avoids a crash in the guest when it tries to access NVRAM.
>
> Looks like the above information could be added to the changelog. While...

I tried to make changelog consise and assume people reading that are 
familiar with QEMU so know what these options do. But whoever will update 
the changelog can decide if this is worth adding, I'm OK with that.

>> While the -kernel option emulates what U-Boot passes to the kernel,
>> old Linux kernels for amigaone may not work with it because of two
>> reasons: these come in legacy U-Boot Multi-File image format that QEMU
>> cannot read and even after unpacking that and creating a kernel uimage
>> it won't find PCI devices because it does not initialise them
>> correctly. This works when booted from U-Boot because U-Boot inits PCI
>> devices. So does my BBoot loader which can be used to load AmigaOS so
>> I don't intend to emulate that part of U-Boot.
>>
>> I'd like this to be merged for the next release please. When merging
>> please update https://wiki.qemu.org/ChangeLog/10.0 with the following:
>>
>> amigaone
>>
>> Added support for NVRAM and -kernel, -initrd, -append command line
>> options.
>
> ... this:
>
>> By default the NVRAM contents are not preserved between
>> sessions. To make it persistent create a backing file with 'qemu-image
>> create -f raw nvram.bin 4k' and add -drive
>> if=mtd,format=raw,file=nvram.bin to keep NVRAM contents in the backing
>> file so settings stored in it will be preserved between sessions.
>>
>> To run AmigaOS with BBoot using the -kernel option at least BBoot
>> version 0.8 is needed. Older BBoot versions only work with -device
>> loader and cannot be used with -kernel on amigaone.
>
> ... information could be added to the amigaone section in the QEMU manual to keep the documentation in one place.

The QEMU doc specifically documents running Linux and Linux does not use 
or rely on NVRAM. But I could mention it there why you likely can't boot 
Linux with -kernel although if you try you'll get an error telling you and 
since the docs don't recommend that it's probably OK to not mention it 
just to not confuse people who only want to know what works.

I have my docs on AmigaOS and BBoot at qmiga.codeberg.page so I'll add 
info relating to these there and keep the QEMU docs for Linux. I've only 
mentioned BBoot in the changelog because people don't read docs so I 
wanted it to be everywhere so they see at least one of those before asking 
me. I've yet to update BBoot and my Qmiga pages but I'll do that for the 
QEMU release.

Regards,
BALATON Zoltan

Re: [PATCH 1/4] ppc/amigaone: Simplify replacement dummy_fw

[Nicholas Piggin]

On Sun Feb 23, 2025 at 3:52 AM AEST, BALATON Zoltan wrote:
> There's no need to do shift in a loop, doing it in one instruction
> works just as well, only the result is used.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Resulting asm looks right to me.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> ---
>  hw/ppc/amigaone.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
> index b02792221c..4290d58613 100644
> --- a/hw/ppc/amigaone.c
> +++ b/hw/ppc/amigaone.c
> @@ -41,10 +41,7 @@
>  
>  /* AmigaOS calls this routine from ROM, use this if no firmware loaded */
>  static const char dummy_fw[] = {
> -    0x38, 0x00, 0x00, 0x08, /* li      r0,8 */
> -    0x7c, 0x09, 0x03, 0xa6, /* mtctr   r0 */
> -    0x54, 0x63, 0xf8, 0x7e, /* srwi    r3,r3,1 */
> -    0x42, 0x00, 0xff, 0xfc, /* bdnz    0x8 */
> +    0x54, 0x63, 0xc2, 0x3e, /* srwi    r3,r3,8 */
>      0x7c, 0x63, 0x18, 0xf8, /* not     r3,r3 */
>      0x4e, 0x80, 0x00, 0x20, /* blr */
>  };

Re: [PATCH 2/4] ppc/amigaone: Implement NVRAM emulation

[Nicholas Piggin]

On Sun Feb 23, 2025 at 3:52 AM AEST, BALATON Zoltan wrote:
> The board has a battery backed NVRAM where U-Boot environment is
> stored which is also accessed by AmigaOS and e.g. C:NVGetVar command
> crashes without it having at least a valid checksum.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ppc/amigaone.c | 116 ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 113 insertions(+), 3 deletions(-)
>
> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
> index 4290d58613..5273543460 100644
> --- a/hw/ppc/amigaone.c
> +++ b/hw/ppc/amigaone.c
> @@ -21,10 +21,13 @@
>  #include "hw/ide/pci.h"
>  #include "hw/i2c/smbus_eeprom.h"
>  #include "hw/ppc/ppc.h"
> +#include "system/block-backend.h"
>  #include "system/qtest.h"
>  #include "system/reset.h"
>  #include "kvm_ppc.h"
>  
> +#include <zlib.h> /* for crc32 */
> +
>  #define BUS_FREQ_HZ 100000000
>  
>  /*
> @@ -46,6 +49,103 @@ static const char dummy_fw[] = {
>      0x4e, 0x80, 0x00, 0x20, /* blr */
>  };
>  
> +#define NVRAM_ADDR 0xfd0e0000
> +#define NVRAM_SIZE (4 * KiB)
> +
> +#define TYPE_A1_NVRAM "a1-nvram"
> +OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
> +
> +struct A1NVRAMState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion mr;
> +    BlockBackend *blk;
> +};
> +
> +/* read callback not used because of romd mode, only here just in case */

Better make it g_assert_not_reached() then.

> +static uint64_t nvram_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    A1NVRAMState *s = opaque;
> +    uint8_t *p = memory_region_get_ram_ptr(&s->mr);
> +
> +    return p[addr];
> +}
> +
> +static void nvram_write(void *opaque, hwaddr addr, uint64_t val,
> +                        unsigned int size)
> +{
> +    A1NVRAMState *s = opaque;
> +    uint8_t *p = memory_region_get_ram_ptr(&s->mr);
> +
> +    p[addr] = val;
> +    if (s->blk) {
> +        blk_pwrite(s->blk, addr, 1, &val, 0);
> +    }
> +}
> +
> +static const MemoryRegionOps nvram_ops = {
> +    .read = nvram_read,
> +    .write = nvram_write,
> +    .endianness = DEVICE_BIG_ENDIAN,
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +};
> +
> +static void nvram_realize(DeviceState *dev, Error **errp)
> +{
> +    A1NVRAMState *s = A1_NVRAM(dev);
> +    void *p;
> +    uint32_t *c;
> +
> +    memory_region_init_rom_device(&s->mr, NULL, &nvram_ops, s, "nvram",
> +                                  NVRAM_SIZE, &error_fatal);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr);
> +    c = p = memory_region_get_ram_ptr(&s->mr);
> +    if (s->blk) {
> +        if (blk_getlength(s->blk) != NVRAM_SIZE) {
> +            error_setg(errp, "NVRAM backing file size must be %ld bytes",
> +                       NVRAM_SIZE);
> +            return;
> +        }
> +        blk_set_perm(s->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
> +                     BLK_PERM_ALL, &error_fatal);
> +        if (blk_pread(s->blk, 0, NVRAM_SIZE, p, 0) < 0) {
> +            error_setg(errp, "Cannot read NVRAM contents from backing file");
> +            return;
> +        }
> +    }
> +    if (*c == 0) {
> +        *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
> +        if (s->blk) {
> +            blk_pwrite(s->blk, 0, 4, p, 0);
> +        }
> +    }
> +}

So, no need for a reset because it's persistent?

> +
> +static const Property nvram_properties[] = {
> +    DEFINE_PROP_DRIVE("drive", A1NVRAMState, blk),
> +};
> +
> +static void nvram_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +
> +    dc->realize = nvram_realize;
> +    device_class_set_props(dc, nvram_properties);
> +}
> +
> +static const TypeInfo nvram_types[] = {
> +    {
> +        .name = TYPE_A1_NVRAM,
> +        .parent = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(A1NVRAMState),
> +        .class_init = nvram_class_init,
> +    },
> +};
> +DEFINE_TYPES(nvram_types)
> +
>  static void amigaone_cpu_reset(void *opaque)
>  {
>      PowerPCCPU *cpu = opaque;
> @@ -72,7 +172,7 @@ static void amigaone_init(MachineState *machine)
>      DeviceState *dev;
>      I2CBus *i2c_bus;
>      uint8_t *spd_data;
> -    int i;
> +    DriveInfo *di;
>  
>      /* init CPU */
>      cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
> @@ -97,6 +197,16 @@ static void amigaone_init(MachineState *machine)
>          memory_region_add_subregion(get_system_memory(), 0x40000000, mr);
>      }
>  
> +    /* nvram */
> +    dev = qdev_new(TYPE_A1_NVRAM);
> +    di = drive_get(IF_MTD, 0, 0);
> +    if (di) {
> +        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di));
> +    }
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    memory_region_add_subregion(get_system_memory(), NVRAM_ADDR,
> +                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
> +
>      /* allocate and load firmware */
>      rom = g_new(MemoryRegion, 1);
>      memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
> @@ -136,7 +246,7 @@ static void amigaone_init(MachineState *machine)
>      pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
>      mr = g_new(MemoryRegion, 1);
>      memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", pci_mem,
> -                             0, 0x1000000);
> +                             0, 0xe0000);
>      memory_region_add_subregion(get_system_memory(), 0xfd000000, mr);

Better make these addresses #defines at the top of the file with
the NVRAM_ADDR?

Thanks,
Nick

>      mr = g_new(MemoryRegion, 1);
>      memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", pci_mem,
> @@ -153,7 +263,7 @@ static void amigaone_init(MachineState *machine)
>      qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
>                                  qdev_get_gpio_in(DEVICE(cpu),
>                                  PPC6xx_INPUT_INT));
> -    for (i = 0; i < PCI_NUM_PINS; i++) {
> +    for (int i = 0; i < PCI_NUM_PINS; i++) {
>          qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
>                                                               "pirq", i));
>      }

Re: [PATCH 3/4] ppc/amigaone: Add default environment

[Nicholas Piggin]

On Sun Feb 23, 2025 at 3:52 AM AEST, BALATON Zoltan wrote:
> Initialise empty NVRAM with default values. This also enables IDE UDMA
> mode in AmigaOS that is faster but has to be enabled in environment
> due to problems with real hardware but that does not affect emulation
> so we can use faster defaults here.

So this overwrites a blank NVRAM file. Okay I suppose if that works.
You could have a property to supply the default environment
alternatively. Anywhere to document this behaviour for users?

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ppc/amigaone.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
> index 5273543460..35e4075cc3 100644
> --- a/hw/ppc/amigaone.c
> +++ b/hw/ppc/amigaone.c
> @@ -52,6 +52,28 @@ static const char dummy_fw[] = {
>  #define NVRAM_ADDR 0xfd0e0000
>  #define NVRAM_SIZE (4 * KiB)
>  
> +static char default_env[] =
> +    "baudrate=115200\0"
> +    "stdout=vga\0"
> +    "stdin=ps2kbd\0"
> +    "bootcmd=boota; menu; run menuboot_cmd\0"
> +    "boot1=ide\0"
> +    "boot2=cdrom\0"
> +    "boota_timeout=3\0"
> +    "ide_doreset=on\0"
> +    "pci_irqa=9\0"
> +    "pci_irqa_select=level\0"
> +    "pci_irqb=10\0"
> +    "pci_irqb_select=level\0"
> +    "pci_irqc=11\0"
> +    "pci_irqc_select=level\0"
> +    "pci_irqd=7\0"
> +    "pci_irqd_select=level\0"
> +    "a1ide_irq=1111\0"
> +    "a1ide_xfer=FFFF\0";
> +#define CRC32_DEFAULT_ENV 0xb5548481
> +#define CRC32_ALL_ZEROS   0x603b0489
> +
>  #define TYPE_A1_NVRAM "a1-nvram"
>  OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
>  
> @@ -97,7 +119,7 @@ static void nvram_realize(DeviceState *dev, Error **errp)
>  {
>      A1NVRAMState *s = A1_NVRAM(dev);
>      void *p;
> -    uint32_t *c;
> +    uint32_t crc, *c;
>  
>      memory_region_init_rom_device(&s->mr, NULL, &nvram_ops, s, "nvram",
>                                    NVRAM_SIZE, &error_fatal);
> @@ -116,12 +138,25 @@ static void nvram_realize(DeviceState *dev, Error **errp)
>              return;
>          }
>      }
> +    crc = crc32(0, p + 4, NVRAM_SIZE - 4);
> +    if (crc == CRC32_ALL_ZEROS) { /* If env is uninitialized set default */
> +        *c = cpu_to_be32(CRC32_DEFAULT_ENV);
> +        /* Also copies terminating \0 as env is terminated by \0\0 */
> +        memcpy(p + 4, default_env, sizeof(default_env));
> +        if (s->blk) {
> +            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0);
> +        }
> +        return;
> +    }
>      if (*c == 0) {
>          *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
>          if (s->blk) {
>              blk_pwrite(s->blk, 0, 4, p, 0);
>          }
>      }
> +    if (be32_to_cpu(*c) != crc) {
> +        warn_report("NVRAM checksum mismatch");
> +    }

Maybe the default environment should be set if there is no CRC? If there
is a CRC already then that seems to indicate a valid rom file was
supplied and user wanted it blank.

This can also be rewritten:

    crc = crc32(0, p + 4, NVRAM_SIZE - 4);
    if (crc == CRC32_ALL_ZEROS) { /* If env is uninitialized set default */
        /* Also copies terminating \0 as env is terminated by \0\0 */
        memcpy(p + 4, default_env, sizeof(default_env));
        crc = CRC32_DEFAULT_ENV;
    }
    if (*c == 0) {
        *c = cpu_to_be32(crc);
        if (s->blk) {
            blk_pwrite(s->blk, 0, 4, p, 0);
        }
    } else if (be32_to_cpu(*c) != crc) {
        warn_report("NVRAM checksum mismatch");
    }

Thanks,
Nick

Re: [PATCH 4/4] ppc/amigaone: Add kernel and initrd support

[Nicholas Piggin]

On Sun Feb 23, 2025 at 3:52 AM AEST, BALATON Zoltan wrote:
> Add support for -kernel, -initrd and -append command line options.

Looks okay. Any test case could be added to test_ppc_amiga.py?

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/ppc/amigaone.c | 113 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 112 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
> index 35e4075cc3..94682de1c7 100644
> --- a/hw/ppc/amigaone.c
> +++ b/hw/ppc/amigaone.c
> @@ -25,11 +25,14 @@
>  #include "system/qtest.h"
>  #include "system/reset.h"
>  #include "kvm_ppc.h"
> +#include "elf.h"
>  
>  #include <zlib.h> /* for crc32 */
>  
>  #define BUS_FREQ_HZ 100000000
>  
> +#define INITRD_MIN_ADDR 0x600000
> +
>  /*
>   * Firmware binary available at
>   * https://www.hyperion-entertainment.com/index.php/downloads?view=files&parent=28
> @@ -181,12 +184,68 @@ static const TypeInfo nvram_types[] = {
>  };
>  DEFINE_TYPES(nvram_types)
>  
> +struct boot_info {
> +    hwaddr entry;
> +    hwaddr stack;
> +    hwaddr bd_info;
> +    hwaddr initrd_start;
> +    hwaddr initrd_end;
> +    hwaddr cmdline_start;
> +    hwaddr cmdline_end;
> +};
> +
> +/* Board info struct from U-Boot */
> +struct bd_info {
> +    uint32_t bi_memstart;
> +    uint32_t bi_memsize;
> +    uint32_t bi_flashstart;
> +    uint32_t bi_flashsize;
> +    uint32_t bi_flashoffset;
> +    uint32_t bi_sramstart;
> +    uint32_t bi_sramsize;
> +    uint32_t bi_bootflags;
> +    uint32_t bi_ip_addr;
> +    uint8_t  bi_enetaddr[6];
> +    uint16_t bi_ethspeed;
> +    uint32_t bi_intfreq;
> +    uint32_t bi_busfreq;
> +    uint32_t bi_baudrate;
> +} QEMU_PACKED;
> +
> +static void create_bd_info(hwaddr addr, ram_addr_t ram_size)
> +{
> +    struct bd_info *bd = g_new0(struct bd_info, 1);
> +
> +    bd->bi_memsize =    cpu_to_be32(ram_size);
> +    bd->bi_flashstart = cpu_to_be32(PROM_ADDR);
> +    bd->bi_flashsize =  cpu_to_be32(1); /* match what U-Boot detects */
> +    bd->bi_bootflags =  cpu_to_be32(1);
> +    bd->bi_intfreq =    cpu_to_be32(11.5 * BUS_FREQ_HZ);
> +    bd->bi_busfreq =    cpu_to_be32(BUS_FREQ_HZ);
> +    bd->bi_baudrate =   cpu_to_be32(115200);
> +
> +    cpu_physical_memory_write(addr, bd, sizeof(*bd));
> +}
> +
>  static void amigaone_cpu_reset(void *opaque)
>  {
>      PowerPCCPU *cpu = opaque;
> +    CPUPPCState *env = &cpu->env;
>  
>      cpu_reset(CPU(cpu));
> -    cpu_ppc_tb_reset(&cpu->env);
> +    if (env->load_info) {
> +        struct boot_info *bi = env->load_info;
> +
> +        env->gpr[1] = bi->stack;
> +        env->gpr[2] = 1024;
> +        env->gpr[3] = bi->bd_info;
> +        env->gpr[4] = bi->initrd_start;
> +        env->gpr[5] = bi->initrd_end;
> +        env->gpr[6] = bi->cmdline_start;
> +        env->gpr[7] = bi->cmdline_end;
> +        env->nip = bi->entry;
> +    }
> +    cpu_ppc_tb_reset(env);
>  }
>  
>  static void fix_spd_data(uint8_t *spd)
> @@ -208,6 +267,8 @@ static void amigaone_init(MachineState *machine)
>      I2CBus *i2c_bus;
>      uint8_t *spd_data;
>      DriveInfo *di;
> +    hwaddr loadaddr;
> +    struct boot_info *bi = NULL;
>  
>      /* init CPU */
>      cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
> @@ -304,6 +365,56 @@ static void amigaone_init(MachineState *machine)
>      }
>      pci_ide_create_devs(PCI_DEVICE(object_resolve_path_component(via, "ide")));
>      pci_vga_init(pci_bus);
> +
> +    if (!machine->kernel_filename) {
> +        return;
> +    }
> +
> +    /* handle -kernel, -initrd, -append options and emulate U-Boot */
> +    bi = g_new0(struct boot_info, 1);
> +    cpu->env.load_info = bi;
> +
> +    loadaddr = MIN(machine->ram_size, 256 * MiB);
> +    bi->bd_info = loadaddr - 8 * MiB;
> +    create_bd_info(bi->bd_info, machine->ram_size);
> +    bi->stack = bi->bd_info - 64 * KiB - 8;
> +
> +    if (machine->kernel_cmdline && machine->kernel_cmdline[0]) {
> +        size_t len = strlen(machine->kernel_cmdline);
> +
> +        loadaddr = bi->bd_info + 1 * MiB;
> +        cpu_physical_memory_write(loadaddr, machine->kernel_cmdline, len + 1);
> +        bi->cmdline_start = loadaddr;
> +        bi->cmdline_end = loadaddr + len + 1; /* including terminating '\0' */
> +    }
> +
> +    sz = load_elf(machine->kernel_filename, NULL, NULL, NULL,
> +                  &bi->entry, &loadaddr, NULL, NULL,
> +                  ELFDATA2MSB, PPC_ELF_MACHINE, 0, 0);
> +    if (sz <= 0) {
> +        sz = load_uimage(machine->kernel_filename, &bi->entry, &loadaddr,
> +                         NULL, NULL, NULL);
> +    }
> +    if (sz <= 0) {
> +        error_report("Could not load kernel '%s'",
> +                     machine->kernel_filename);
> +        exit(1);
> +    }
> +    loadaddr += sz;
> +
> +    if (machine->initrd_filename) {
> +        loadaddr = ROUND_UP(loadaddr + 4 * MiB, 4 * KiB);
> +        loadaddr = MAX(loadaddr, INITRD_MIN_ADDR);
> +        sz = load_image_targphys(machine->initrd_filename, loadaddr,
> +                                 bi->bd_info - loadaddr);
> +        if (sz <= 0) {
> +            error_report("Could not load initrd '%s'",
> +                         machine->initrd_filename);
> +            exit(1);
> +        }
> +        bi->initrd_start = loadaddr;
> +        bi->initrd_end = loadaddr + sz;
> +    }
>  }
>  
>  static void amigaone_machine_init(MachineClass *mc)

Re: [PATCH 2/4] ppc/amigaone: Implement NVRAM emulation

[BALATON Zoltan]

On Thu, 27 Feb 2025, Nicholas Piggin wrote:
> On Sun Feb 23, 2025 at 3:52 AM AEST, BALATON Zoltan wrote:
>> The board has a battery backed NVRAM where U-Boot environment is
>> stored which is also accessed by AmigaOS and e.g. C:NVGetVar command
>> crashes without it having at least a valid checksum.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ppc/amigaone.c | 116 ++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 113 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
>> index 4290d58613..5273543460 100644
>> --- a/hw/ppc/amigaone.c
>> +++ b/hw/ppc/amigaone.c
>> @@ -21,10 +21,13 @@
>>  #include "hw/ide/pci.h"
>>  #include "hw/i2c/smbus_eeprom.h"
>>  #include "hw/ppc/ppc.h"
>> +#include "system/block-backend.h"
>>  #include "system/qtest.h"
>>  #include "system/reset.h"
>>  #include "kvm_ppc.h"
>>
>> +#include <zlib.h> /* for crc32 */
>> +
>>  #define BUS_FREQ_HZ 100000000
>>
>>  /*
>> @@ -46,6 +49,103 @@ static const char dummy_fw[] = {
>>      0x4e, 0x80, 0x00, 0x20, /* blr */
>>  };
>>
>> +#define NVRAM_ADDR 0xfd0e0000
>> +#define NVRAM_SIZE (4 * KiB)
>> +
>> +#define TYPE_A1_NVRAM "a1-nvram"
>> +OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
>> +
>> +struct A1NVRAMState {
>> +    SysBusDevice parent_obj;
>> +
>> +    MemoryRegion mr;
>> +    BlockBackend *blk;
>> +};
>> +
>> +/* read callback not used because of romd mode, only here just in case */
>
> Better make it g_assert_not_reached() then.

There is a memory_region_rom_device_set_romd() function. It's not called 
here so a read function should not be needed but it's also trivial and 
would work if romd mode is turned off for some reason in the future so 
adding it seems safer to me. The comment is just to note it's a romd 
region so reads normally don't go through this function unless romd mode 
is turned off.

>> +static uint64_t nvram_read(void *opaque, hwaddr addr, unsigned int size)
>> +{
>> +    A1NVRAMState *s = opaque;
>> +    uint8_t *p = memory_region_get_ram_ptr(&s->mr);
>> +
>> +    return p[addr];
>> +}
>> +
>> +static void nvram_write(void *opaque, hwaddr addr, uint64_t val,
>> +                        unsigned int size)
>> +{
>> +    A1NVRAMState *s = opaque;
>> +    uint8_t *p = memory_region_get_ram_ptr(&s->mr);
>> +
>> +    p[addr] = val;
>> +    if (s->blk) {
>> +        blk_pwrite(s->blk, addr, 1, &val, 0);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps nvram_ops = {
>> +    .read = nvram_read,
>> +    .write = nvram_write,
>> +    .endianness = DEVICE_BIG_ENDIAN,
>> +    .impl = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 1,
>> +    },
>> +};
>> +
>> +static void nvram_realize(DeviceState *dev, Error **errp)
>> +{
>> +    A1NVRAMState *s = A1_NVRAM(dev);
>> +    void *p;
>> +    uint32_t *c;
>> +
>> +    memory_region_init_rom_device(&s->mr, NULL, &nvram_ops, s, "nvram",
>> +                                  NVRAM_SIZE, &error_fatal);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr);
>> +    c = p = memory_region_get_ram_ptr(&s->mr);
>> +    if (s->blk) {
>> +        if (blk_getlength(s->blk) != NVRAM_SIZE) {
>> +            error_setg(errp, "NVRAM backing file size must be %ld bytes",
>> +                       NVRAM_SIZE);
>> +            return;
>> +        }
>> +        blk_set_perm(s->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
>> +                     BLK_PERM_ALL, &error_fatal);
>> +        if (blk_pread(s->blk, 0, NVRAM_SIZE, p, 0) < 0) {
>> +            error_setg(errp, "Cannot read NVRAM contents from backing file");
>> +            return;
>> +        }
>> +    }
>> +    if (*c == 0) {
>> +        *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
>> +        if (s->blk) {
>> +            blk_pwrite(s->blk, 0, 4, p, 0);
>> +        }
>> +    }
>> +}
>
> So, no need for a reset because it's persistent?

It's either written to the backing file when changed or stays unchanged on 
reboot so no need to reset. The idea is to provide a default if there's no 
backing file but allow the user to change it for the session but not keep 
changes between sessions. With backing file it's like real machine keeping 
settings between reboots. I don't think it needs anything in reset.

>> +
>> +static const Property nvram_properties[] = {
>> +    DEFINE_PROP_DRIVE("drive", A1NVRAMState, blk),
>> +};
>> +
>> +static void nvram_class_init(ObjectClass *oc, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>> +
>> +    dc->realize = nvram_realize;
>> +    device_class_set_props(dc, nvram_properties);
>> +}
>> +
>> +static const TypeInfo nvram_types[] = {
>> +    {
>> +        .name = TYPE_A1_NVRAM,
>> +        .parent = TYPE_SYS_BUS_DEVICE,
>> +        .instance_size = sizeof(A1NVRAMState),
>> +        .class_init = nvram_class_init,
>> +    },
>> +};
>> +DEFINE_TYPES(nvram_types)
>> +
>>  static void amigaone_cpu_reset(void *opaque)
>>  {
>>      PowerPCCPU *cpu = opaque;
>> @@ -72,7 +172,7 @@ static void amigaone_init(MachineState *machine)
>>      DeviceState *dev;
>>      I2CBus *i2c_bus;
>>      uint8_t *spd_data;
>> -    int i;
>> +    DriveInfo *di;
>>
>>      /* init CPU */
>>      cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
>> @@ -97,6 +197,16 @@ static void amigaone_init(MachineState *machine)
>>          memory_region_add_subregion(get_system_memory(), 0x40000000, mr);
>>      }
>>
>> +    /* nvram */
>> +    dev = qdev_new(TYPE_A1_NVRAM);
>> +    di = drive_get(IF_MTD, 0, 0);
>> +    if (di) {
>> +        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di));
>> +    }
>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> +    memory_region_add_subregion(get_system_memory(), NVRAM_ADDR,
>> +                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
>> +
>>      /* allocate and load firmware */
>>      rom = g_new(MemoryRegion, 1);
>>      memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
>> @@ -136,7 +246,7 @@ static void amigaone_init(MachineState *machine)
>>      pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
>>      mr = g_new(MemoryRegion, 1);
>>      memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", pci_mem,
>> -                             0, 0x1000000);
>> +                             0, 0xe0000);
>>      memory_region_add_subregion(get_system_memory(), 0xfd000000, mr);
>
> Better make these addresses #defines at the top of the file with
> the NVRAM_ADDR?

I don't have defines for these as these are single use constants to set up 
memory map and with defines it's less obvious and has to be looked up 
where these are while this way I can see it directly without having to 
scroll up so I prefer this. I've added defines where the constant is used 
more than once where it makes sense to keep consistency.

Regards,
BALATON Zoltan

> Thanks,
> Nick
>
>>      mr = g_new(MemoryRegion, 1);
>>      memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", pci_mem,
>> @@ -153,7 +263,7 @@ static void amigaone_init(MachineState *machine)
>>      qdev_connect_gpio_out_named(DEVICE(via), "intr", 0,
>>                                  qdev_get_gpio_in(DEVICE(cpu),
>>                                  PPC6xx_INPUT_INT));
>> -    for (i = 0; i < PCI_NUM_PINS; i++) {
>> +    for (int i = 0; i < PCI_NUM_PINS; i++) {
>>          qdev_connect_gpio_out(dev, i, qdev_get_gpio_in_named(DEVICE(via),
>>                                                               "pirq", i));
>>      }
>
>

Re: [PATCH 3/4] ppc/amigaone: Add default environment

[BALATON Zoltan]

On Thu, 27 Feb 2025, Nicholas Piggin wrote:
> On Sun Feb 23, 2025 at 3:52 AM AEST, BALATON Zoltan wrote:
>> Initialise empty NVRAM with default values. This also enables IDE UDMA
>> mode in AmigaOS that is faster but has to be enabled in environment
>> due to problems with real hardware but that does not affect emulation
>> so we can use faster defaults here.
>
> So this overwrites a blank NVRAM file. Okay I suppose if that works.

We're emulating what U-Boot does. If it does not find a valid environment 
it will overwrite with defaults. These defaults are to get the same 
behaviour and additionally to enable UDMA for IDE driver that until now 
had to be done manually to get the same speed as with pegasos2 where this 
is enabled by default. (That's because these VIA VT82C686B chips had some 
issues with DMA even in PCs but the emulated devices work so can be 
enabled and that's faster on QEMU too.)

> You could have a property to supply the default environment
> alternatively.

U-Boot has the defaults hard coded. I set and use it from the same file so 
I don't see the need to send this through a property. (Properties are also 
listed in QEMU Monitor with info qtree and I don't know how a long string 
with embedded zeros would look there so I don't think that's a good idea.)

> Anywhere to document this behaviour for users?

I've added docs in the cover letter that I hope would end up in the 
changelog and I have a separate page for this which I'll update at 
qmiga.codeberg.page (Haven't done that yet but would do it by the 
release once this is merged.)

>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/ppc/amigaone.c | 37 ++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
>> index 5273543460..35e4075cc3 100644
>> --- a/hw/ppc/amigaone.c
>> +++ b/hw/ppc/amigaone.c
>> @@ -52,6 +52,28 @@ static const char dummy_fw[] = {
>>  #define NVRAM_ADDR 0xfd0e0000
>>  #define NVRAM_SIZE (4 * KiB)
>>
>> +static char default_env[] =
>> +    "baudrate=115200\0"
>> +    "stdout=vga\0"
>> +    "stdin=ps2kbd\0"
>> +    "bootcmd=boota; menu; run menuboot_cmd\0"
>> +    "boot1=ide\0"
>> +    "boot2=cdrom\0"
>> +    "boota_timeout=3\0"
>> +    "ide_doreset=on\0"
>> +    "pci_irqa=9\0"
>> +    "pci_irqa_select=level\0"
>> +    "pci_irqb=10\0"
>> +    "pci_irqb_select=level\0"
>> +    "pci_irqc=11\0"
>> +    "pci_irqc_select=level\0"
>> +    "pci_irqd=7\0"
>> +    "pci_irqd_select=level\0"
>> +    "a1ide_irq=1111\0"
>> +    "a1ide_xfer=FFFF\0";
>> +#define CRC32_DEFAULT_ENV 0xb5548481
>> +#define CRC32_ALL_ZEROS   0x603b0489
>> +
>>  #define TYPE_A1_NVRAM "a1-nvram"
>>  OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
>>
>> @@ -97,7 +119,7 @@ static void nvram_realize(DeviceState *dev, Error **errp)
>>  {
>>      A1NVRAMState *s = A1_NVRAM(dev);
>>      void *p;
>> -    uint32_t *c;
>> +    uint32_t crc, *c;
>>
>>      memory_region_init_rom_device(&s->mr, NULL, &nvram_ops, s, "nvram",
>>                                    NVRAM_SIZE, &error_fatal);
>> @@ -116,12 +138,25 @@ static void nvram_realize(DeviceState *dev, Error **errp)
>>              return;
>>          }
>>      }
>> +    crc = crc32(0, p + 4, NVRAM_SIZE - 4);
>> +    if (crc == CRC32_ALL_ZEROS) { /* If env is uninitialized set default */
>> +        *c = cpu_to_be32(CRC32_DEFAULT_ENV);
>> +        /* Also copies terminating \0 as env is terminated by \0\0 */
>> +        memcpy(p + 4, default_env, sizeof(default_env));
>> +        if (s->blk) {
>> +            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0);
>> +        }
>> +        return;
>> +    }
>>      if (*c == 0) {
>>          *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
>>          if (s->blk) {
>>              blk_pwrite(s->blk, 0, 4, p, 0);
>>          }
>>      }
>> +    if (be32_to_cpu(*c) != crc) {
>> +        warn_report("NVRAM checksum mismatch");
>> +    }
>
> Maybe the default environment should be set if there is no CRC? If there
> is a CRC already then that seems to indicate a valid rom file was
> supplied and user wanted it blank.

The idea is to allow supplying an environment via the backing file or let 
users edit it and clear the checksum in which case it will take the 
supplied data and only fix the checksum. Unlike U-Boot it will only 
replace empty backing file with defaults. (This is kind of undocumented 
for advanced users only but that's why the above code looks like that.) 
Having an empty NVRAM does not make much sense as U-Boot always has some 
values there so it's never empty. If it finds empty NVRAM then installs 
default env which is to help new users so they only need to create an 
empty file to get started. It also works without backing file in which 
case we want to use defaults and keep it persistent only for the session.

> This can also be rewritten:
>
>    crc = crc32(0, p + 4, NVRAM_SIZE - 4);
>    if (crc == CRC32_ALL_ZEROS) { /* If env is uninitialized set default */
>        /* Also copies terminating \0 as env is terminated by \0\0 */
>        memcpy(p + 4, default_env, sizeof(default_env));
>        crc = CRC32_DEFAULT_ENV;
>    }
>    if (*c == 0) {
>        *c = cpu_to_be32(crc);
>        if (s->blk) {
>            blk_pwrite(s->blk, 0, 4, p, 0);
>        }
>    } else if (be32_to_cpu(*c) != crc) {
>        warn_report("NVRAM checksum mismatch");
>    }

I have the checksum check in separate if to warn if there is a checksum 
but it's wrong because I only want it to fix checksum if it's zero but not 
touch it otherwise. Basically it will only overwrite zero values but leave 
nonzero values there. (Might be useful for testing what does the guest do 
if the checksum is wrong, although on real machine U-Boot would have 
replaced the env with default in that case so this would only happen if 
the NVRAM is changed during runtime without updating the checksum.)

Regards,
BALATON Zoltan

Re: [PATCH 2/4] ppc/amigaone: Implement NVRAM emulation

[Nicholas Piggin]

On Thu Feb 27, 2025 at 11:48 AM AEST, BALATON Zoltan wrote:
> On Thu, 27 Feb 2025, Nicholas Piggin wrote:
>> On Sun Feb 23, 2025 at 3:52 AM AEST, BALATON Zoltan wrote:
>>> The board has a battery backed NVRAM where U-Boot environment is
>>> stored which is also accessed by AmigaOS and e.g. C:NVGetVar command
>>> crashes without it having at least a valid checksum.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/ppc/amigaone.c | 116 ++++++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 113 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
>>> index 4290d58613..5273543460 100644
>>> --- a/hw/ppc/amigaone.c
>>> +++ b/hw/ppc/amigaone.c
>>> @@ -21,10 +21,13 @@
>>>  #include "hw/ide/pci.h"
>>>  #include "hw/i2c/smbus_eeprom.h"
>>>  #include "hw/ppc/ppc.h"
>>> +#include "system/block-backend.h"
>>>  #include "system/qtest.h"
>>>  #include "system/reset.h"
>>>  #include "kvm_ppc.h"
>>>
>>> +#include <zlib.h> /* for crc32 */
>>> +
>>>  #define BUS_FREQ_HZ 100000000
>>>
>>>  /*
>>> @@ -46,6 +49,103 @@ static const char dummy_fw[] = {
>>>      0x4e, 0x80, 0x00, 0x20, /* blr */
>>>  };
>>>
>>> +#define NVRAM_ADDR 0xfd0e0000
>>> +#define NVRAM_SIZE (4 * KiB)
>>> +
>>> +#define TYPE_A1_NVRAM "a1-nvram"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
>>> +
>>> +struct A1NVRAMState {
>>> +    SysBusDevice parent_obj;
>>> +
>>> +    MemoryRegion mr;
>>> +    BlockBackend *blk;
>>> +};
>>> +
>>> +/* read callback not used because of romd mode, only here just in case */
>>
>> Better make it g_assert_not_reached() then.
>
> There is a memory_region_rom_device_set_romd() function. It's not called 
> here so a read function should not be needed but it's also trivial and 
> would work if romd mode is turned off for some reason in the future so 
> adding it seems safer to me. The comment is just to note it's a romd 
> region so reads normally don't go through this function unless romd mode 
> is turned off.

It's trivial to add back if you do turn it off. Adding dead code is no
good. The memory API default won't get changed underneath you without
proper code audit or deprecating the API so that's no problem.

>>> +static uint64_t nvram_read(void *opaque, hwaddr addr, unsigned int size)
>>> +{
>>> +    A1NVRAMState *s = opaque;
>>> +    uint8_t *p = memory_region_get_ram_ptr(&s->mr);
>>> +
>>> +    return p[addr];
>>> +}
>>> +
>>> +static void nvram_write(void *opaque, hwaddr addr, uint64_t val,
>>> +                        unsigned int size)
>>> +{
>>> +    A1NVRAMState *s = opaque;
>>> +    uint8_t *p = memory_region_get_ram_ptr(&s->mr);
>>> +
>>> +    p[addr] = val;
>>> +    if (s->blk) {
>>> +        blk_pwrite(s->blk, addr, 1, &val, 0);
>>> +    }
>>> +}
>>> +
>>> +static const MemoryRegionOps nvram_ops = {
>>> +    .read = nvram_read,
>>> +    .write = nvram_write,
>>> +    .endianness = DEVICE_BIG_ENDIAN,
>>> +    .impl = {
>>> +        .min_access_size = 1,
>>> +        .max_access_size = 1,
>>> +    },
>>> +};
>>> +
>>> +static void nvram_realize(DeviceState *dev, Error **errp)
>>> +{
>>> +    A1NVRAMState *s = A1_NVRAM(dev);
>>> +    void *p;
>>> +    uint32_t *c;
>>> +
>>> +    memory_region_init_rom_device(&s->mr, NULL, &nvram_ops, s, "nvram",
>>> +                                  NVRAM_SIZE, &error_fatal);
>>> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mr);
>>> +    c = p = memory_region_get_ram_ptr(&s->mr);
>>> +    if (s->blk) {
>>> +        if (blk_getlength(s->blk) != NVRAM_SIZE) {
>>> +            error_setg(errp, "NVRAM backing file size must be %ld bytes",
>>> +                       NVRAM_SIZE);
>>> +            return;
>>> +        }
>>> +        blk_set_perm(s->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
>>> +                     BLK_PERM_ALL, &error_fatal);
>>> +        if (blk_pread(s->blk, 0, NVRAM_SIZE, p, 0) < 0) {
>>> +            error_setg(errp, "Cannot read NVRAM contents from backing file");
>>> +            return;
>>> +        }
>>> +    }
>>> +    if (*c == 0) {
>>> +        *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
>>> +        if (s->blk) {
>>> +            blk_pwrite(s->blk, 0, 4, p, 0);
>>> +        }
>>> +    }
>>> +}
>>
>> So, no need for a reset because it's persistent?
>
> It's either written to the backing file when changed or stays unchanged on 
> reboot so no need to reset. The idea is to provide a default if there's no 
> backing file but allow the user to change it for the session but not keep 
> changes between sessions. With backing file it's like real machine keeping 
> settings between reboots. I don't think it needs anything in reset.

Yep, makes sense I think.

>
>>> +
>>> +static const Property nvram_properties[] = {
>>> +    DEFINE_PROP_DRIVE("drive", A1NVRAMState, blk),
>>> +};
>>> +
>>> +static void nvram_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    DeviceClass *dc = DEVICE_CLASS(oc);
>>> +
>>> +    dc->realize = nvram_realize;
>>> +    device_class_set_props(dc, nvram_properties);
>>> +}
>>> +
>>> +static const TypeInfo nvram_types[] = {
>>> +    {
>>> +        .name = TYPE_A1_NVRAM,
>>> +        .parent = TYPE_SYS_BUS_DEVICE,
>>> +        .instance_size = sizeof(A1NVRAMState),
>>> +        .class_init = nvram_class_init,
>>> +    },
>>> +};
>>> +DEFINE_TYPES(nvram_types)
>>> +
>>>  static void amigaone_cpu_reset(void *opaque)
>>>  {
>>>      PowerPCCPU *cpu = opaque;
>>> @@ -72,7 +172,7 @@ static void amigaone_init(MachineState *machine)
>>>      DeviceState *dev;
>>>      I2CBus *i2c_bus;
>>>      uint8_t *spd_data;
>>> -    int i;
>>> +    DriveInfo *di;
>>>
>>>      /* init CPU */
>>>      cpu = POWERPC_CPU(cpu_create(machine->cpu_type));
>>> @@ -97,6 +197,16 @@ static void amigaone_init(MachineState *machine)
>>>          memory_region_add_subregion(get_system_memory(), 0x40000000, mr);
>>>      }
>>>
>>> +    /* nvram */
>>> +    dev = qdev_new(TYPE_A1_NVRAM);
>>> +    di = drive_get(IF_MTD, 0, 0);
>>> +    if (di) {
>>> +        qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di));
>>> +    }
>>> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>> +    memory_region_add_subregion(get_system_memory(), NVRAM_ADDR,
>>> +                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
>>> +
>>>      /* allocate and load firmware */
>>>      rom = g_new(MemoryRegion, 1);
>>>      memory_region_init_rom(rom, NULL, "rom", PROM_SIZE, &error_fatal);
>>> @@ -136,7 +246,7 @@ static void amigaone_init(MachineState *machine)
>>>      pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
>>>      mr = g_new(MemoryRegion, 1);
>>>      memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", pci_mem,
>>> -                             0, 0x1000000);
>>> +                             0, 0xe0000);
>>>      memory_region_add_subregion(get_system_memory(), 0xfd000000, mr);
>>
>> Better make these addresses #defines at the top of the file with
>> the NVRAM_ADDR?
>
> I don't have defines for these as these are single use constants to set up 
> memory map and with defines it's less obvious and has to be looked up 
> where these are while this way I can see it directly without having to 
> scroll up so I prefer this. I've added defines where the constant is used 
> more than once where it makes sense to keep consistency.

But now you have to lok in two different places anyway because you have
the NVRAM_ADDR etc defines at the top of the file. Seems like a good
time to move all defines there so you can easily see the memory map in
one place.

Thanks,
Nick

Re: [PATCH 3/4] ppc/amigaone: Add default environment

[Nicholas Piggin]

On Thu Feb 27, 2025 at 12:18 PM AEST, BALATON Zoltan wrote:
> On Thu, 27 Feb 2025, Nicholas Piggin wrote:
>> On Sun Feb 23, 2025 at 3:52 AM AEST, BALATON Zoltan wrote:
>>> Initialise empty NVRAM with default values. This also enables IDE UDMA
>>> mode in AmigaOS that is faster but has to be enabled in environment
>>> due to problems with real hardware but that does not affect emulation
>>> so we can use faster defaults here.
>>
>> So this overwrites a blank NVRAM file. Okay I suppose if that works.
>
> We're emulating what U-Boot does. If it does not find a valid environment 
> it will overwrite with defaults.

AFAIKS u-boot provides a default environment if the CRC does not match.

If all-zeros env was created with correct CRC, IMO it should be
accepted.

Does u-boot write back a default environment or corrected CRC to NVRAM
if it was missing/bad?

> These defaults are to get the same 
> behaviour and additionally to enable UDMA for IDE driver that until now 
> had to be done manually to get the same speed as with pegasos2 where this 
> is enabled by default. (That's because these VIA VT82C686B chips had some 
> issues with DMA even in PCs but the emulated devices work so can be 
> enabled and that's faster on QEMU too.)
>
>> You could have a property to supply the default environment
>> alternatively.
>
> U-Boot has the defaults hard coded. I set and use it from the same file so 
> I don't see the need to send this through a property. (Properties are also 
> listed in QEMU Monitor with info qtree and I don't know how a long string 
> with embedded zeros would look there so I don't think that's a good idea.)
>
>> Anywhere to document this behaviour for users?
>
> I've added docs in the cover letter that I hope would end up in the 
> changelog and I have a separate page for this which I'll update at 
> qmiga.codeberg.page (Haven't done that yet but would do it by the 
> release once this is merged.)

Okay. It would still be better to add QEMU options to docs/ files and
point the amiga pages to that rather than the other way around, since
this is QEMU specific stuff. Doc in cover letter unfortunately is
not very good since it  just gets lost.

>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/ppc/amigaone.c | 37 ++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
>>> index 5273543460..35e4075cc3 100644
>>> --- a/hw/ppc/amigaone.c
>>> +++ b/hw/ppc/amigaone.c
>>> @@ -52,6 +52,28 @@ static const char dummy_fw[] = {
>>>  #define NVRAM_ADDR 0xfd0e0000
>>>  #define NVRAM_SIZE (4 * KiB)
>>>
>>> +static char default_env[] =
>>> +    "baudrate=115200\0"
>>> +    "stdout=vga\0"
>>> +    "stdin=ps2kbd\0"
>>> +    "bootcmd=boota; menu; run menuboot_cmd\0"
>>> +    "boot1=ide\0"
>>> +    "boot2=cdrom\0"
>>> +    "boota_timeout=3\0"
>>> +    "ide_doreset=on\0"
>>> +    "pci_irqa=9\0"
>>> +    "pci_irqa_select=level\0"
>>> +    "pci_irqb=10\0"
>>> +    "pci_irqb_select=level\0"
>>> +    "pci_irqc=11\0"
>>> +    "pci_irqc_select=level\0"
>>> +    "pci_irqd=7\0"
>>> +    "pci_irqd_select=level\0"

Hmm, the u-boot default env (before it was removed) selected
edge for these. Was that wrong?

>>> +    "a1ide_irq=1111\0"
>>> +    "a1ide_xfer=FFFF\0";
>>> +#define CRC32_DEFAULT_ENV 0xb5548481
>>> +#define CRC32_ALL_ZEROS   0x603b0489
>>> +
>>>  #define TYPE_A1_NVRAM "a1-nvram"
>>>  OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
>>>
>>> @@ -97,7 +119,7 @@ static void nvram_realize(DeviceState *dev, Error **errp)
>>>  {
>>>      A1NVRAMState *s = A1_NVRAM(dev);
>>>      void *p;
>>> -    uint32_t *c;
>>> +    uint32_t crc, *c;
>>>
>>>      memory_region_init_rom_device(&s->mr, NULL, &nvram_ops, s, "nvram",
>>>                                    NVRAM_SIZE, &error_fatal);
>>> @@ -116,12 +138,25 @@ static void nvram_realize(DeviceState *dev, Error **errp)
>>>              return;
>>>          }
>>>      }
>>> +    crc = crc32(0, p + 4, NVRAM_SIZE - 4);
>>> +    if (crc == CRC32_ALL_ZEROS) { /* If env is uninitialized set default */
>>> +        *c = cpu_to_be32(CRC32_DEFAULT_ENV);
>>> +        /* Also copies terminating \0 as env is terminated by \0\0 */
>>> +        memcpy(p + 4, default_env, sizeof(default_env));
>>> +        if (s->blk) {
>>> +            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0);
>>> +        }
>>> +        return;
>>> +    }
>>>      if (*c == 0) {
>>>          *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
>>>          if (s->blk) {
>>>              blk_pwrite(s->blk, 0, 4, p, 0);
>>>          }
>>>      }
>>> +    if (be32_to_cpu(*c) != crc) {
>>> +        warn_report("NVRAM checksum mismatch");
>>> +    }
>>
>> Maybe the default environment should be set if there is no CRC? If there
>> is a CRC already then that seems to indicate a valid rom file was
>> supplied and user wanted it blank.
>
> The idea is to allow supplying an environment via the backing file or let 
> users edit it and clear the checksum in which case it will take the 
> supplied data and only fix the checksum. Unlike U-Boot it will only 
> replace empty backing file with defaults. (This is kind of undocumented 
> for advanced users only but that's why the above code looks like that.) 
> Having an empty NVRAM does not make much sense as U-Boot always has some 
> values there so it's never empty. If it finds empty NVRAM then installs 
> default env which is to help new users so they only need to create an 
> empty file to get started. It also works without backing file in which 
> case we want to use defaults and keep it persistent only for the session.
>
>> This can also be rewritten:
>>
>>    crc = crc32(0, p + 4, NVRAM_SIZE - 4);
>>    if (crc == CRC32_ALL_ZEROS) { /* If env is uninitialized set default */
>>        /* Also copies terminating \0 as env is terminated by \0\0 */
>>        memcpy(p + 4, default_env, sizeof(default_env));
>>        crc = CRC32_DEFAULT_ENV;
>>    }
>>    if (*c == 0) {
>>        *c = cpu_to_be32(crc);
>>        if (s->blk) {
>>            blk_pwrite(s->blk, 0, 4, p, 0);
>>        }
>>    } else if (be32_to_cpu(*c) != crc) {
>>        warn_report("NVRAM checksum mismatch");
>>    }
>
> I have the checksum check in separate if to warn if there is a checksum 
> but it's wrong because I only want it to fix checksum if it's zero but not 
> touch it otherwise. Basically it will only overwrite zero values but leave 
> nonzero values there. (Might be useful for testing what does the guest do 
> if the checksum is wrong, although on real machine U-Boot would have 
> replaced the env with default in that case so this would only happen if 
> the NVRAM is changed during runtime without updating the checksum.)

Right, you can just rewrite it a bit simpler to have only one exit point
and only do the blk_pwrite once etc I think (although my version might
have had a bug it was a bit hasty but you get the idea).

Thanks,
Nick

Re: [PATCH 2/4] ppc/amigaone: Implement NVRAM emulation

[BALATON Zoltan]

On Thu, 27 Feb 2025, Nicholas Piggin wrote:
> On Thu Feb 27, 2025 at 11:48 AM AEST, BALATON Zoltan wrote:
>> On Thu, 27 Feb 2025, Nicholas Piggin wrote:
>>> On Sun Feb 23, 2025 at 3:52 AM AEST, BALATON Zoltan wrote:
>>>> The board has a battery backed NVRAM where U-Boot environment is
>>>> stored which is also accessed by AmigaOS and e.g. C:NVGetVar command
>>>> crashes without it having at least a valid checksum.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/ppc/amigaone.c | 116 ++++++++++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 113 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
>>>> index 4290d58613..5273543460 100644
>>>> --- a/hw/ppc/amigaone.c
>>>> +++ b/hw/ppc/amigaone.c
>>>> @@ -21,10 +21,13 @@
>>>>  #include "hw/ide/pci.h"
>>>>  #include "hw/i2c/smbus_eeprom.h"
>>>>  #include "hw/ppc/ppc.h"
>>>> +#include "system/block-backend.h"
>>>>  #include "system/qtest.h"
>>>>  #include "system/reset.h"
>>>>  #include "kvm_ppc.h"
>>>>
>>>> +#include <zlib.h> /* for crc32 */
>>>> +
>>>>  #define BUS_FREQ_HZ 100000000
>>>>
>>>>  /*
>>>> @@ -46,6 +49,103 @@ static const char dummy_fw[] = {
>>>>      0x4e, 0x80, 0x00, 0x20, /* blr */
>>>>  };
>>>>
>>>> +#define NVRAM_ADDR 0xfd0e0000
>>>> +#define NVRAM_SIZE (4 * KiB)
>>>> +
>>>> +#define TYPE_A1_NVRAM "a1-nvram"
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
>>>> +
>>>> +struct A1NVRAMState {
>>>> +    SysBusDevice parent_obj;
>>>> +
>>>> +    MemoryRegion mr;
>>>> +    BlockBackend *blk;
>>>> +};
>>>> +
>>>> +/* read callback not used because of romd mode, only here just in case */
>>>
>>> Better make it g_assert_not_reached() then.
>>
>> There is a memory_region_rom_device_set_romd() function. It's not called
>> here so a read function should not be needed but it's also trivial and
>> would work if romd mode is turned off for some reason in the future so
>> adding it seems safer to me. The comment is just to note it's a romd
>> region so reads normally don't go through this function unless romd mode
>> is turned off.
>
> It's trivial to add back if you do turn it off. Adding dead code is no
> good. The memory API default won't get changed underneath you without
> proper code audit or deprecating the API so that's no problem.

OK, done in v2.

>>> Better make these addresses #defines at the top of the file with
>>> the NVRAM_ADDR?
>>
>> I don't have defines for these as these are single use constants to set up
>> memory map and with defines it's less obvious and has to be looked up
>> where these are while this way I can see it directly without having to
>> scroll up so I prefer this. I've added defines where the constant is used
>> more than once where it makes sense to keep consistency.
>
> But now you have to lok in two different places anyway because you have
> the NVRAM_ADDR etc defines at the top of the file. Seems like a good
> time to move all defines there so you can easily see the memory map in
> one place.

I don't think this makes it more readable at all but I've added a patch in 
v2 to add defines for these.

Regards,
BALATON Zoltan

Re: [PATCH 3/4] ppc/amigaone: Add default environment

[BALATON Zoltan]

On Thu, 27 Feb 2025, Nicholas Piggin wrote:
> On Thu Feb 27, 2025 at 12:18 PM AEST, BALATON Zoltan wrote:
>> On Thu, 27 Feb 2025, Nicholas Piggin wrote:
>>> On Sun Feb 23, 2025 at 3:52 AM AEST, BALATON Zoltan wrote:
>>>> Initialise empty NVRAM with default values. This also enables IDE UDMA
>>>> mode in AmigaOS that is faster but has to be enabled in environment
>>>> due to problems with real hardware but that does not affect emulation
>>>> so we can use faster defaults here.
>>>
>>> So this overwrites a blank NVRAM file. Okay I suppose if that works.
>>
>> We're emulating what U-Boot does. If it does not find a valid environment
>> it will overwrite with defaults.
>
> AFAIKS u-boot provides a default environment if the CRC does not match.
>
> If all-zeros env was created with correct CRC, IMO it should be
> accepted.
>
> Does u-boot write back a default environment or corrected CRC to NVRAM
> if it was missing/bad?

No, U-Boot replaces the in memory copy with the default, the NVRAM is only 
changed if saveenv command is issued. I don't want to 100% emulate U-Boot 
but make it easier for users, see below.

>> These defaults are to get the same
>> behaviour and additionally to enable UDMA for IDE driver that until now
>> had to be done manually to get the same speed as with pegasos2 where this
>> is enabled by default. (That's because these VIA VT82C686B chips had some
>> issues with DMA even in PCs but the emulated devices work so can be
>> enabled and that's faster on QEMU too.)
>>
>>> You could have a property to supply the default environment
>>> alternatively.
>>
>> U-Boot has the defaults hard coded. I set and use it from the same file so
>> I don't see the need to send this through a property. (Properties are also
>> listed in QEMU Monitor with info qtree and I don't know how a long string
>> with embedded zeros would look there so I don't think that's a good idea.)
>>
>>> Anywhere to document this behaviour for users?
>>
>> I've added docs in the cover letter that I hope would end up in the
>> changelog and I have a separate page for this which I'll update at
>> qmiga.codeberg.page (Haven't done that yet but would do it by the
>> release once this is merged.)
>
> Okay. It would still be better to add QEMU options to docs/ files and
> point the amiga pages to that rather than the other way around, since
> this is QEMU specific stuff. Doc in cover letter unfortunately is
> not very good since it  just gets lost.

I hope that part of the cover letter would end up in the QEMU changelog so 
it will be in QEMU docs. The amigang.rst doc specifically documents how to 
run Linux on these machines and these patches don't affect Linux nor work 
with it so I left them out to not confuse readers with options they don't 
need.

>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>>  hw/ppc/amigaone.c | 37 ++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
>>>> index 5273543460..35e4075cc3 100644
>>>> --- a/hw/ppc/amigaone.c
>>>> +++ b/hw/ppc/amigaone.c
>>>> @@ -52,6 +52,28 @@ static const char dummy_fw[] = {
>>>>  #define NVRAM_ADDR 0xfd0e0000
>>>>  #define NVRAM_SIZE (4 * KiB)
>>>>
>>>> +static char default_env[] =
>>>> +    "baudrate=115200\0"
>>>> +    "stdout=vga\0"
>>>> +    "stdin=ps2kbd\0"
>>>> +    "bootcmd=boota; menu; run menuboot_cmd\0"
>>>> +    "boot1=ide\0"
>>>> +    "boot2=cdrom\0"
>>>> +    "boota_timeout=3\0"
>>>> +    "ide_doreset=on\0"
>>>> +    "pci_irqa=9\0"
>>>> +    "pci_irqa_select=level\0"
>>>> +    "pci_irqb=10\0"
>>>> +    "pci_irqb_select=level\0"
>>>> +    "pci_irqc=11\0"
>>>> +    "pci_irqc_select=level\0"
>>>> +    "pci_irqd=7\0"
>>>> +    "pci_irqd_select=level\0"
>
> Hmm, the u-boot default env (before it was removed) selected
> edge for these. Was that wrong?

What was in upstream U-Boot wasn't what was in the binary that came with 
the machine. The binary has these defaults and AmigaOS depends on it, it 
does not work with edge as found for the pegasos2 where the default was 
edge and that resulted in missed IRQs.

>>>> +    "a1ide_irq=1111\0"
>>>> +    "a1ide_xfer=FFFF\0";
>>>> +#define CRC32_DEFAULT_ENV 0xb5548481
>>>> +#define CRC32_ALL_ZEROS   0x603b0489
>>>> +
>>>>  #define TYPE_A1_NVRAM "a1-nvram"
>>>>  OBJECT_DECLARE_SIMPLE_TYPE(A1NVRAMState, A1_NVRAM)
>>>>
>>>> @@ -97,7 +119,7 @@ static void nvram_realize(DeviceState *dev, Error **errp)
>>>>  {
>>>>      A1NVRAMState *s = A1_NVRAM(dev);
>>>>      void *p;
>>>> -    uint32_t *c;
>>>> +    uint32_t crc, *c;
>>>>
>>>>      memory_region_init_rom_device(&s->mr, NULL, &nvram_ops, s, "nvram",
>>>>                                    NVRAM_SIZE, &error_fatal);
>>>> @@ -116,12 +138,25 @@ static void nvram_realize(DeviceState *dev, Error **errp)
>>>>              return;
>>>>          }
>>>>      }
>>>> +    crc = crc32(0, p + 4, NVRAM_SIZE - 4);
>>>> +    if (crc == CRC32_ALL_ZEROS) { /* If env is uninitialized set default */
>>>> +        *c = cpu_to_be32(CRC32_DEFAULT_ENV);
>>>> +        /* Also copies terminating \0 as env is terminated by \0\0 */
>>>> +        memcpy(p + 4, default_env, sizeof(default_env));
>>>> +        if (s->blk) {
>>>> +            blk_pwrite(s->blk, 0, sizeof(crc) + sizeof(default_env), p, 0);
>>>> +        }
>>>> +        return;
>>>> +    }
>>>>      if (*c == 0) {
>>>>          *c = cpu_to_be32(crc32(0, p + 4, NVRAM_SIZE - 4));
>>>>          if (s->blk) {
>>>>              blk_pwrite(s->blk, 0, 4, p, 0);
>>>>          }
>>>>      }
>>>> +    if (be32_to_cpu(*c) != crc) {
>>>> +        warn_report("NVRAM checksum mismatch");
>>>> +    }
>>>
>>> Maybe the default environment should be set if there is no CRC? If there
>>> is a CRC already then that seems to indicate a valid rom file was
>>> supplied and user wanted it blank.
>>
>> The idea is to allow supplying an environment via the backing file or let
>> users edit it and clear the checksum in which case it will take the
>> supplied data and only fix the checksum. Unlike U-Boot it will only
>> replace empty backing file with defaults. (This is kind of undocumented
>> for advanced users only but that's why the above code looks like that.)
>> Having an empty NVRAM does not make much sense as U-Boot always has some
>> values there so it's never empty. If it finds empty NVRAM then installs
>> default env which is to help new users so they only need to create an
>> empty file to get started. It also works without backing file in which
>> case we want to use defaults and keep it persistent only for the session.
>>
>>> This can also be rewritten:
>>>
>>>    crc = crc32(0, p + 4, NVRAM_SIZE - 4);
>>>    if (crc == CRC32_ALL_ZEROS) { /* If env is uninitialized set default */
>>>        /* Also copies terminating \0 as env is terminated by \0\0 */
>>>        memcpy(p + 4, default_env, sizeof(default_env));
>>>        crc = CRC32_DEFAULT_ENV;
>>>    }
>>>    if (*c == 0) {
>>>        *c = cpu_to_be32(crc);
>>>        if (s->blk) {
>>>            blk_pwrite(s->blk, 0, 4, p, 0);
>>>        }
>>>    } else if (be32_to_cpu(*c) != crc) {
>>>        warn_report("NVRAM checksum mismatch");
>>>    }
>>
>> I have the checksum check in separate if to warn if there is a checksum
>> but it's wrong because I only want it to fix checksum if it's zero but not
>> touch it otherwise. Basically it will only overwrite zero values but leave
>> nonzero values there. (Might be useful for testing what does the guest do
>> if the checksum is wrong, although on real machine U-Boot would have
>> replaced the env with default in that case so this would only happen if
>> the NVRAM is changed during runtime without updating the checksum.)
>
> Right, you can just rewrite it a bit simpler to have only one exit point
> and only do the blk_pwrite once etc I think (although my version might
> have had a bug it was a bit hasty but you get the idea).

It would not be simpler because I'd need another variable to keep how much 
of the NVRAM to rewrite. Note that the area to write in the two blk_pwrite 
calls are different. We have 3 cases:

1. User just created an empty NVRAM file to allow persisting settings in 
it for future runs. Something has to put defaults there but if not running 
with -bios firmware.bin because using -kernel to boot instead, it would 
stay empty. Also firmware.bin has defaults that does not autoboot so needs 
manual intervention and disable UDMA so it will be slow. In this case we 
init NVRAM with sensible defaults which is done in the first if. (This is 
also used when no NVRAM file is provided.)

2. The NVRAM has some values in it either because the user supplied their 
preferred values or edited previous content directly and not within the 
guest. To make their life easier and allow them to leave checksum empty I 
allow setting crc to 0 in which case it will just update the crc and use 
the contents. This is handled in the second if.

3. There is content in NVRAM with a crc. In this case only the crc is 
checked and nothing is replaced. This is done in the third if and will 
never fail in one of the first two cases but checks NVRAM in the third 
case. When checksum does not match U-Boot would use defaults for the 
memory copy used only while U-Boot is running but unless user issues 
saveenv it still passes wrong NVRAM contents to the guest so without 
U-Boot we get the same behaviour here.

Regards,
BALATON Zoltan