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-454ae9c11...@linaro.org/T/ https://patchew.org/QEMU/20231208020619.117-1-zhiwei._5f...@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-ayl...@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.
