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-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? 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-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.