| Message ID | 20240107212538.227627-2-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | nubus: add nubus-virtio-mmio device | expand |
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); > } > }
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.
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. >
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.
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); } }
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(-)