[PATCH 0/2] nubus: add nubus-virtio-mmio device

[PATCH 0/2] nubus: add nubus-virtio-mmio device

[Mark Cave-Ayland]

This series introduces a new nubus-virtio-mmio device which can be plugged into
the q800 machine to enable a 68k Classic MacOS guest to access virtio devices
such as virtio-9p-device (host filesharing), virtio-gpu (extended framebuffer
support) and virtio-tablet-device (absolute positioning).

Once the nubus-virtio-mmio device has been plugged into the q800 machine, virtio
devices can be accessed by a Classic MacOS guest using the drivers from the
classicvirtio project at https://github.com/elliotnunn/classicvirtio.

The nubus-virtio-mmio device is purposefully designed to be similar to the
virtio-mmio interface used by the existing 68k virt machine, making use of a
similar memory layout and the goldfish PIC for simple interrupt management. The
main difference is that only a single goldfish PIC is used, however that still
allows up to 32 virtio devices to be connected using a single nubus card.

Patch 1 fixes an alignment bug in the existing nubus-device Declaration ROM code
whereby some ROM images could trigger an assert() in QEMU, whilst patch 2 adds
the nubus-virtio-mmio device itself.

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


Mark Cave-Ayland (2):
  nubus-device: round Declaration ROM memory region address to
    qemu_target_page_size()
  nubus: add nubus-virtio-mmio device

 hw/nubus/meson.build                 |   1 +
 hw/nubus/nubus-device.c              |  16 +++--
 hw/nubus/nubus-virtio-mmio.c         | 102 +++++++++++++++++++++++++++
 include/hw/nubus/nubus-virtio-mmio.h |  36 ++++++++++
 4 files changed, 151 insertions(+), 4 deletions(-)
 create mode 100644 hw/nubus/nubus-virtio-mmio.c
 create mode 100644 include/hw/nubus/nubus-virtio-mmio.h

-- 
2.39.2

[PATCH 1/2] nubus-device: round Declaration ROM memory region address to qemu_target_page_size()

[Mark Cave-Ayland]

Declaration ROM binary images can be any arbitrary size, however if a host ROM
memory region is not aligned to qemu_target_page_size() then we fail the
"assert(!(iotlb & ~TARGET_PAGE_MASK))" check in tlb_set_page_full().

Ensure that the host ROM memory region is aligned to qemu_target_page_size()
and adjust the offset at which the Declaration ROM image is loaded so that the
image is still aligned to the end of the Nubus slot.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/nubus/nubus-device.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
index 49008e4938..e4f824d58b 100644
--- a/hw/nubus/nubus-device.c
+++ b/hw/nubus/nubus-device.c
@@ -10,6 +10,7 @@
 
 #include "qemu/osdep.h"
 #include "qemu/datadir.h"
+#include "exec/target_page.h"
 #include "hw/irq.h"
 #include "hw/loader.h"
 #include "hw/nubus/nubus.h"
@@ -30,7 +31,7 @@ static void nubus_device_realize(DeviceState *dev, Error **errp)
     NubusDevice *nd = NUBUS_DEVICE(dev);
     char *name, *path;
     hwaddr slot_offset;
-    int64_t size;
+    int64_t size, align_size;
     int ret;
 
     /* Super */
@@ -76,16 +77,23 @@ static void nubus_device_realize(DeviceState *dev, Error **errp)
         }
 
         name = g_strdup_printf("nubus-slot-%x-declaration-rom", nd->slot);
-        memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, size,
+
+        /*
+         * Ensure ROM memory region is aligned to target page size regardless
+         * of the size of the Declaration ROM image
+         */
+        align_size = ROUND_UP(size, qemu_target_page_size());
+        memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, align_size,
                                &error_abort);
-        ret = load_image_mr(path, &nd->decl_rom);
+        ret = load_image_size(path, memory_region_get_ram_ptr(&nd->decl_rom) +
+                                    (uintptr_t)align_size - size, size);
         g_free(path);
         g_free(name);
         if (ret < 0) {
             error_setg(errp, "could not load romfile \"%s\"", nd->romfile);
             return;
         }
-        memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - size,
+        memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - align_size,
                                     &nd->decl_rom);
     }
 }
-- 
2.39.2

[PATCH 2/2] nubus: add nubus-virtio-mmio device

[Mark Cave-Ayland]

The nubus-virtio-mmio device is a Nubus card that contains a set of 32 virtio-mmio
devices and a goldfish PIC similar to the m68k virt machine that can be plugged
into the m68k q800 machine.

There are currently a number of drivers under development that can be used in
conjunction with this device to provide accelerated and/or additional hypervisor
services to 68k Classic MacOS.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/nubus/meson.build                 |   1 +
 hw/nubus/nubus-virtio-mmio.c         | 102 +++++++++++++++++++++++++++
 include/hw/nubus/nubus-virtio-mmio.h |  36 ++++++++++
 3 files changed, 139 insertions(+)
 create mode 100644 hw/nubus/nubus-virtio-mmio.c
 create mode 100644 include/hw/nubus/nubus-virtio-mmio.h

diff --git a/hw/nubus/meson.build b/hw/nubus/meson.build
index e7ebda8993..9a7a12ea68 100644
--- a/hw/nubus/meson.build
+++ b/hw/nubus/meson.build
@@ -2,6 +2,7 @@ nubus_ss = ss.source_set()
 nubus_ss.add(files('nubus-device.c'))
 nubus_ss.add(files('nubus-bus.c'))
 nubus_ss.add(files('nubus-bridge.c'))
+nubus_ss.add(files('nubus-virtio-mmio.c'))
 nubus_ss.add(when: 'CONFIG_Q800', if_true: files('mac-nubus-bridge.c'))
 
 system_ss.add_all(when: 'CONFIG_NUBUS', if_true: nubus_ss)
diff --git a/hw/nubus/nubus-virtio-mmio.c b/hw/nubus/nubus-virtio-mmio.c
new file mode 100644
index 0000000000..58a63c84d0
--- /dev/null
+++ b/hw/nubus/nubus-virtio-mmio.c
@@ -0,0 +1,102 @@
+/*
+ * QEMU Macintosh Nubus Virtio MMIO card
+ *
+ * Copyright (c) 2024 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/nubus/nubus-virtio-mmio.h"
+
+
+#define NUBUS_VIRTIO_MMIO_PIC_OFFSET   0
+#define NUBUS_VIRTIO_MMIO_DEV_OFFSET   0x200
+
+
+static void nubus_virtio_mmio_set_input_irq(void *opaque, int n, int level)
+{
+    NubusDevice *nd = NUBUS_DEVICE(opaque);
+
+    nubus_set_irq(nd, level);
+}
+
+static void nubus_virtio_mmio_realize(DeviceState *dev, Error **errp)
+{
+    NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_GET_CLASS(dev);
+    NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(dev);
+    NubusDevice *nd = NUBUS_DEVICE(dev);
+    SysBusDevice *sbd;
+    int i, offset;
+
+    nvmdc->parent_realize(dev, errp);
+    if (*errp) {
+        return;
+    }
+
+    /* Goldfish PIC */
+    sbd = SYS_BUS_DEVICE(&s->pic);
+    if (!sysbus_realize(sbd, errp)) {
+        return;
+    }
+    memory_region_add_subregion(&nd->slot_mem, NUBUS_VIRTIO_MMIO_PIC_OFFSET,
+                                sysbus_mmio_get_region(sbd, 0));
+    sysbus_connect_irq(sbd, 0,
+                       qdev_get_gpio_in_named(dev, "pic-input-irq", 0));
+
+    /* virtio-mmio devices */
+    offset = NUBUS_VIRTIO_MMIO_DEV_OFFSET;
+    for (i = 0; i < NUBUS_VIRTIO_MMIO_NUM_DEVICES; i++) {
+        sbd = SYS_BUS_DEVICE(&s->virtio_mmio[i]);
+        qdev_prop_set_bit(DEVICE(sbd), "force-legacy", false);
+        if (!sysbus_realize_and_unref(sbd, errp)) {
+            return;
+        }
+
+        memory_region_add_subregion(&nd->slot_mem, offset,
+                                    sysbus_mmio_get_region(sbd, 0));
+        offset += 0x200;
+
+        sysbus_connect_irq(sbd, 0, qdev_get_gpio_in(DEVICE(&s->pic), i));
+    }
+}
+
+static void nubus_virtio_mmio_init(Object *obj)
+{
+    NubusVirtioMMIO *s = NUBUS_VIRTIO_MMIO(obj);
+    int i;
+
+    object_initialize_child(obj, "pic", &s->pic, TYPE_GOLDFISH_PIC);
+    for (i = 0; i < NUBUS_VIRTIO_MMIO_NUM_DEVICES; i++) {
+        char *name = g_strdup_printf("virtio-mmio[%d]", i);
+        object_initialize_child(obj, name, &s->virtio_mmio[i],
+                                TYPE_VIRTIO_MMIO);
+        g_free(name);
+    }
+
+    /* Input from goldfish PIC */
+    qdev_init_gpio_in_named(DEVICE(obj), nubus_virtio_mmio_set_input_irq,
+                            "pic-input-irq", 1);
+}
+
+static void nubus_virtio_mmio_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+    NubusVirtioMMIODeviceClass *nvmdc = NUBUS_VIRTIO_MMIO_CLASS(oc);
+
+    device_class_set_parent_realize(dc, nubus_virtio_mmio_realize,
+                                    &nvmdc->parent_realize);
+}
+
+static const TypeInfo nubus_virtio_mmio_types[] = {
+    {
+        .name = TYPE_NUBUS_VIRTIO_MMIO,
+        .parent = TYPE_NUBUS_DEVICE,
+        .instance_init = nubus_virtio_mmio_init,
+        .instance_size = sizeof(NubusVirtioMMIO),
+        .class_init = nubus_virtio_mmio_class_init,
+        .class_size = sizeof(NubusVirtioMMIODeviceClass),
+    },
+};
+
+DEFINE_TYPES(nubus_virtio_mmio_types)
diff --git a/include/hw/nubus/nubus-virtio-mmio.h b/include/hw/nubus/nubus-virtio-mmio.h
new file mode 100644
index 0000000000..de497b7f76
--- /dev/null
+++ b/include/hw/nubus/nubus-virtio-mmio.h
@@ -0,0 +1,36 @@
+/*
+ * QEMU Macintosh Nubus Virtio MMIO card
+ *
+ * Copyright (c) 2023 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef HW_NUBUS_VIRTIO_MMIO_H
+#define HW_NUBUS_VIRTIO_MMIO_H
+
+#include "hw/nubus/nubus.h"
+#include "qom/object.h"
+#include "hw/intc/goldfish_pic.h"
+#include "hw/virtio/virtio-mmio.h"
+
+#define TYPE_NUBUS_VIRTIO_MMIO "nubus-virtio-mmio"
+OBJECT_DECLARE_TYPE(NubusVirtioMMIO, NubusVirtioMMIODeviceClass,
+                    NUBUS_VIRTIO_MMIO)
+
+struct NubusVirtioMMIODeviceClass {
+    DeviceClass parent_class;
+
+    DeviceRealize parent_realize;
+};
+
+#define NUBUS_VIRTIO_MMIO_NUM_DEVICES 32
+
+struct NubusVirtioMMIO {
+    NubusDevice parent_obj;
+
+    GoldfishPICState pic;
+    VirtIOMMIOProxy virtio_mmio[NUBUS_VIRTIO_MMIO_NUM_DEVICES];
+};
+
+#endif
-- 
2.39.2

Re: [PATCH 1/2] nubus-device: round Declaration ROM memory region address to qemu_target_page_size()

[Philippe Mathieu-Daudé]

On 7/1/24 22:25, Mark Cave-Ayland wrote:
> Declaration ROM binary images can be any arbitrary size, however if a host ROM
> memory region is not aligned to qemu_target_page_size() then we fail the
> "assert(!(iotlb & ~TARGET_PAGE_MASK))" check in tlb_set_page_full().

IIUC this isn't specific to NuBus but to any ROM used to execute code
in place.

Shouldn't this be handled in memory_region_init_rom()?

> Ensure that the host ROM memory region is aligned to qemu_target_page_size()
> and adjust the offset at which the Declaration ROM image is loaded so that the
> image is still aligned to the end of the Nubus slot.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/nubus/nubus-device.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
> index 49008e4938..e4f824d58b 100644
> --- a/hw/nubus/nubus-device.c
> +++ b/hw/nubus/nubus-device.c
> @@ -10,6 +10,7 @@
>   
>   #include "qemu/osdep.h"
>   #include "qemu/datadir.h"
> +#include "exec/target_page.h"
>   #include "hw/irq.h"
>   #include "hw/loader.h"
>   #include "hw/nubus/nubus.h"
> @@ -30,7 +31,7 @@ static void nubus_device_realize(DeviceState *dev, Error **errp)
>       NubusDevice *nd = NUBUS_DEVICE(dev);
>       char *name, *path;
>       hwaddr slot_offset;
> -    int64_t size;
> +    int64_t size, align_size;
>       int ret;
>   
>       /* Super */
> @@ -76,16 +77,23 @@ static void nubus_device_realize(DeviceState *dev, Error **errp)
>           }
>   
>           name = g_strdup_printf("nubus-slot-%x-declaration-rom", nd->slot);
> -        memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, size,
> +
> +        /*
> +         * Ensure ROM memory region is aligned to target page size regardless
> +         * of the size of the Declaration ROM image
> +         */
> +        align_size = ROUND_UP(size, qemu_target_page_size());
> +        memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, align_size,
>                                  &error_abort);
> -        ret = load_image_mr(path, &nd->decl_rom);
> +        ret = load_image_size(path, memory_region_get_ram_ptr(&nd->decl_rom) +
> +                                    (uintptr_t)align_size - size, size);
>           g_free(path);
>           g_free(name);
>           if (ret < 0) {
>               error_setg(errp, "could not load romfile \"%s\"", nd->romfile);
>               return;
>           }
> -        memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - size,
> +        memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - align_size,
>                                       &nd->decl_rom);
>       }
>   }

Re: [PATCH 1/2] nubus-device: round Declaration ROM memory region address to qemu_target_page_size()

[Mark Cave-Ayland]

On 08/01/2024 12:01, Philippe Mathieu-Daudé wrote:

> On 7/1/24 22:25, Mark Cave-Ayland wrote:
>> Declaration ROM binary images can be any arbitrary size, however if a host ROM
>> memory region is not aligned to qemu_target_page_size() then we fail the
>> "assert(!(iotlb & ~TARGET_PAGE_MASK))" check in tlb_set_page_full().
> 
> IIUC this isn't specific to NuBus but to any ROM used to execute code
> in place.
> 
> Shouldn't this be handled in memory_region_init_rom()?

There were some previous discussion in the threads here:

https://lore.kernel.org/all/b68ab7d3-d3d3-9f81-569d-454ae9c11b16@linaro.org/T/
https://patchew.org/QEMU/20231208020619.117-1-zhiwei._5Fliu@linux.alibaba.com/

My impression from Richard's last reply in the second thread is that this should be 
fixed in nubus-device instead? The Nubus declaration ROMs are different in that they 
are aligned to the end of the memory region rather than the beginning, which is 
probably quite an unusual use-case.

>> Ensure that the host ROM memory region is aligned to qemu_target_page_size()
>> and adjust the offset at which the Declaration ROM image is loaded so that the
>> image is still aligned to the end of the Nubus slot.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/nubus/nubus-device.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
>> index 49008e4938..e4f824d58b 100644
>> --- a/hw/nubus/nubus-device.c
>> +++ b/hw/nubus/nubus-device.c
>> @@ -10,6 +10,7 @@
>>   #include "qemu/osdep.h"
>>   #include "qemu/datadir.h"
>> +#include "exec/target_page.h"
>>   #include "hw/irq.h"
>>   #include "hw/loader.h"
>>   #include "hw/nubus/nubus.h"
>> @@ -30,7 +31,7 @@ static void nubus_device_realize(DeviceState *dev, Error **errp)
>>       NubusDevice *nd = NUBUS_DEVICE(dev);
>>       char *name, *path;
>>       hwaddr slot_offset;
>> -    int64_t size;
>> +    int64_t size, align_size;
>>       int ret;
>>       /* Super */
>> @@ -76,16 +77,23 @@ static void nubus_device_realize(DeviceState *dev, Error **errp)
>>           }
>>           name = g_strdup_printf("nubus-slot-%x-declaration-rom", nd->slot);
>> -        memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, size,
>> +
>> +        /*
>> +         * Ensure ROM memory region is aligned to target page size regardless
>> +         * of the size of the Declaration ROM image
>> +         */
>> +        align_size = ROUND_UP(size, qemu_target_page_size());
>> +        memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, align_size,
>>                                  &error_abort);
>> -        ret = load_image_mr(path, &nd->decl_rom);
>> +        ret = load_image_size(path, memory_region_get_ram_ptr(&nd->decl_rom) +
>> +                                    (uintptr_t)align_size - size, size);
>>           g_free(path);
>>           g_free(name);
>>           if (ret < 0) {
>>               error_setg(errp, "could not load romfile \"%s\"", nd->romfile);
>>               return;
>>           }
>> -        memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - size,
>> +        memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - align_size,
>>                                       &nd->decl_rom);
>>       }
>>   }


ATB,

Mark.

Re: [PATCH 1/2] nubus-device: round Declaration ROM memory region address to qemu_target_page_size()

[Philippe Mathieu-Daudé]

On 8/1/24 13:46, Mark Cave-Ayland wrote:
> On 08/01/2024 12:01, Philippe Mathieu-Daudé wrote:
> 
>> On 7/1/24 22:25, Mark Cave-Ayland wrote:
>>> Declaration ROM binary images can be any arbitrary size, however if a 
>>> host ROM
>>> memory region is not aligned to qemu_target_page_size() then we fail the
>>> "assert(!(iotlb & ~TARGET_PAGE_MASK))" check in tlb_set_page_full().
>>
>> IIUC this isn't specific to NuBus but to any ROM used to execute code
>> in place.
>>
>> Shouldn't this be handled in memory_region_init_rom()?
> 
> There were some previous discussion in the threads here:
> 
> https://lore.kernel.org/all/b68ab7d3-d3d3-9f81-569d-454ae9c11b16@linaro.org/T/
> https://patchew.org/QEMU/20231208020619.117-1-zhiwei._5Fliu@linux.alibaba.com/
> 
> My impression from Richard's last reply in the second thread is that 
> this should be fixed in nubus-device instead?

Hmm OK. And you need the offset to call load_image_size() at the
proper offset anyway.

Can you add that <...

> The Nubus declaration ROMs 
> are different in that they are aligned to the end of the memory region 
> rather than the beginning, which is probably quite an unusual use-case.

...> info to the description?


> 
>>> Ensure that the host ROM memory region is aligned to 
>>> qemu_target_page_size()
>>> and adjust the offset at which the Declaration ROM image is loaded so 
>>> that the
>>> image is still aligned to the end of the Nubus slot.

 >> should it be that the ROM memory region must be aligned to 
TARGET_PAGE_MASK?

This seems to make sense to me, but I'm out of my comfort zone.

>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/nubus/nubus-device.c | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
>>> index 49008e4938..e4f824d58b 100644
>>> --- a/hw/nubus/nubus-device.c
>>> +++ b/hw/nubus/nubus-device.c
>>> @@ -10,6 +10,7 @@
>>>   #include "qemu/osdep.h"
>>>   #include "qemu/datadir.h"
>>> +#include "exec/target_page.h"
>>>   #include "hw/irq.h"
>>>   #include "hw/loader.h"
>>>   #include "hw/nubus/nubus.h"
>>> @@ -30,7 +31,7 @@ static void nubus_device_realize(DeviceState *dev, 
>>> Error **errp)
>>>       NubusDevice *nd = NUBUS_DEVICE(dev);
>>>       char *name, *path;
>>>       hwaddr slot_offset;
>>> -    int64_t size;
>>> +    int64_t size, align_size;
>>>       int ret;
>>>       /* Super */
>>> @@ -76,16 +77,23 @@ static void nubus_device_realize(DeviceState 
>>> *dev, Error **errp)
>>>           }
>>>           name = g_strdup_printf("nubus-slot-%x-declaration-rom", 
>>> nd->slot);
>>> -        memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, size,
>>> +
>>> +        /*
>>> +         * Ensure ROM memory region is aligned to target page size 
>>> regardless
>>> +         * of the size of the Declaration ROM image
>>> +         */
>>> +        align_size = ROUND_UP(size, qemu_target_page_size());
>>> +        memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, 
>>> align_size,
>>>                                  &error_abort);
>>> -        ret = load_image_mr(path, &nd->decl_rom);
>>> +        ret = load_image_size(path, 
>>> memory_region_get_ram_ptr(&nd->decl_rom) +
>>> +                                    (uintptr_t)align_size - size, 
>>> size);
>>>           g_free(path);
>>>           g_free(name);
>>>           if (ret < 0) {
>>>               error_setg(errp, "could not load romfile \"%s\"", 
>>> nd->romfile);
>>>               return;
>>>           }
>>> -        memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - 
>>> size,
>>> +        memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - 
>>> align_size,
>>>                                       &nd->decl_rom);
>>>       }
>>>   }
> 
> 
> ATB,
> 
> Mark.
> 

Re: [PATCH 1/2] nubus-device: round Declaration ROM memory region address to qemu_target_page_size()

[Mark Cave-Ayland]

On 08/01/2024 13:17, Philippe Mathieu-Daudé wrote:

> On 8/1/24 13:46, Mark Cave-Ayland wrote:
>> On 08/01/2024 12:01, Philippe Mathieu-Daudé wrote:
>>
>>> On 7/1/24 22:25, Mark Cave-Ayland wrote:
>>>> Declaration ROM binary images can be any arbitrary size, however if a host ROM
>>>> memory region is not aligned to qemu_target_page_size() then we fail the
>>>> "assert(!(iotlb & ~TARGET_PAGE_MASK))" check in tlb_set_page_full().
>>>
>>> IIUC this isn't specific to NuBus but to any ROM used to execute code
>>> in place.
>>>
>>> Shouldn't this be handled in memory_region_init_rom()?
>>
>> There were some previous discussion in the threads here:
>>
>> https://lore.kernel.org/all/b68ab7d3-d3d3-9f81-569d-454ae9c11b16@linaro.org/T/
>> https://patchew.org/QEMU/20231208020619.117-1-zhiwei._5Fliu@linux.alibaba.com/
>>
>> My impression from Richard's last reply in the second thread is that this should be 
>> fixed in nubus-device instead?
> 
> Hmm OK. And you need the offset to call load_image_size() at the
> proper offset anyway.
> 
> Can you add that <...
> 
>> The Nubus declaration ROMs are different in that they are aligned to the end of the 
>> memory region rather than the beginning, which is probably quite an unusual use-case.
> 
> ...> info to the description?

No problem, I've just sent a v2 with an updated description for patch 1 to better 
document the unusual alignment behaviour.

>>>> Ensure that the host ROM memory region is aligned to qemu_target_page_size()
>>>> and adjust the offset at which the Declaration ROM image is loaded so that the
>>>> image is still aligned to the end of the Nubus slot.
> 
>  >> should it be that the ROM memory region must be aligned to TARGET_PAGE_MASK?
> 
> This seems to make sense to me, but I'm out of my comfort zone.
> 
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>>   hw/nubus/nubus-device.c | 16 ++++++++++++----
>>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/nubus/nubus-device.c b/hw/nubus/nubus-device.c
>>>> index 49008e4938..e4f824d58b 100644
>>>> --- a/hw/nubus/nubus-device.c
>>>> +++ b/hw/nubus/nubus-device.c
>>>> @@ -10,6 +10,7 @@
>>>>   #include "qemu/osdep.h"
>>>>   #include "qemu/datadir.h"
>>>> +#include "exec/target_page.h"
>>>>   #include "hw/irq.h"
>>>>   #include "hw/loader.h"
>>>>   #include "hw/nubus/nubus.h"
>>>> @@ -30,7 +31,7 @@ static void nubus_device_realize(DeviceState *dev, Error **errp)
>>>>       NubusDevice *nd = NUBUS_DEVICE(dev);
>>>>       char *name, *path;
>>>>       hwaddr slot_offset;
>>>> -    int64_t size;
>>>> +    int64_t size, align_size;
>>>>       int ret;
>>>>       /* Super */
>>>> @@ -76,16 +77,23 @@ static void nubus_device_realize(DeviceState *dev, Error **errp)
>>>>           }
>>>>           name = g_strdup_printf("nubus-slot-%x-declaration-rom", nd->slot);
>>>> -        memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, size,
>>>> +
>>>> +        /*
>>>> +         * Ensure ROM memory region is aligned to target page size regardless
>>>> +         * of the size of the Declaration ROM image
>>>> +         */
>>>> +        align_size = ROUND_UP(size, qemu_target_page_size());
>>>> +        memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, align_size,
>>>>                                  &error_abort);
>>>> -        ret = load_image_mr(path, &nd->decl_rom);
>>>> +        ret = load_image_size(path, memory_region_get_ram_ptr(&nd->decl_rom) +
>>>> +                                    (uintptr_t)align_size - size, size);
>>>>           g_free(path);
>>>>           g_free(name);
>>>>           if (ret < 0) {
>>>>               error_setg(errp, "could not load romfile \"%s\"", nd->romfile);
>>>>               return;
>>>>           }
>>>> -        memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - size,
>>>> +        memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - align_size,
>>>>                                       &nd->decl_rom);
>>>>       }
>>>>   }


ATB,

Mark.