| Message ID | 20240108192013.272112-2-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | nubus: add nubus-virtio-mmio device | expand |
On 8/1/24 20:20, 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(). > > 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, since Nubus > ROM images are unusual in that they are aligned to the end of the slot address > space. > > 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; Both are 'size_t'. > 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); memory_region_get_ram_ptr() returns a 'void *' so this looks dubious. Maybe use a local variable to ease offset calculation? char *rombase = memory_region_get_ram_ptr(&nd->decl_rom); ret = load_image_size(path, rombase + align_size - size, size); Otherwise KISS but ugly: ret = load_image_size(path, (void *)((uintptr_t)memory_region_get_ram_ptr(&nd->decl_rom) + 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 23:06, Philippe Mathieu-Daudé wrote: > On 8/1/24 20:20, 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(). >> >> 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, since Nubus >> ROM images are unusual in that they are aligned to the end of the slot address >> space. >> >> 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; > > Both are 'size_t'. I had a look at include/hw/loader.h, and the function signature for get_image_size() returns int64_t. Does it not make sense to keep int64_t here and use uintptr_t for the pointer arithmetic as below so that everything matches? >> 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); > > memory_region_get_ram_ptr() returns a 'void *' so this looks dubious. > Maybe use a local variable to ease offset calculation? > > char *rombase = memory_region_get_ram_ptr(&nd->decl_rom); > ret = load_image_size(path, rombase + align_size - size, size); > > Otherwise KISS but ugly: > > ret = load_image_size(path, > (void *)((uintptr_t)memory_region_get_ram_ptr(&nd->decl_rom) > + align_size - size), size); I prefer the first approach, but with uint8_t instead of char since it clarifies that it is a pointer to an arbitrary set of bytes as opposed to a string. Does that seem reasonable? >> 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 9/1/24 22:53, Mark Cave-Ayland wrote: > On 08/01/2024 23:06, Philippe Mathieu-Daudé wrote: > >> On 8/1/24 20:20, 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(). >>> >>> 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, >>> since Nubus >>> ROM images are unusual in that they are aligned to the end of the >>> slot address >>> space. >>> >>> 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; >> >> Both are 'size_t'. > > I had a look at include/hw/loader.h, and the function signature for > get_image_size() returns int64_t. Does it not make sense to keep int64_t > here and use uintptr_t for the pointer arithmetic as below so that > everything matches? Oh you are right: $ git grep -E '(get_image_size|qemu_target_page_size|load_image_size)\(' include include/exec/target_page.h:17:size_t qemu_target_page_size(void); include/hw/loader.h:13:int64_t get_image_size(const char *filename); include/hw/loader.h:30:ssize_t load_image_size(const char *filename, void *addr, size_t size); So I guess int64_t is safer. >>> 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); >> >> memory_region_get_ram_ptr() returns a 'void *' so this looks dubious. >> Maybe use a local variable to ease offset calculation? >> >> char *rombase = memory_region_get_ram_ptr(&nd->decl_rom); >> ret = load_image_size(path, rombase + align_size - size, size); >> >> Otherwise KISS but ugly: >> >> ret = load_image_size(path, >> (void *)((uintptr_t)memory_region_get_ram_ptr(&nd->decl_rom) >> + align_size - size), size); > > I prefer the first approach, but with uint8_t instead of char since it > clarifies that it is a pointer to an arbitrary set of bytes as opposed > to a string. Does that seem reasonable? Sure! Then with that: Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >>> 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 11/01/2024 06:22, Philippe Mathieu-Daudé wrote: > On 9/1/24 22:53, Mark Cave-Ayland wrote: >> On 08/01/2024 23:06, Philippe Mathieu-Daudé wrote: >> >>> On 8/1/24 20:20, 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(). >>>> >>>> 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, since Nubus >>>> ROM images are unusual in that they are aligned to the end of the slot address >>>> space. >>>> >>>> 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; >>> >>> Both are 'size_t'. >> >> I had a look at include/hw/loader.h, and the function signature for >> get_image_size() returns int64_t. Does it not make sense to keep int64_t here and >> use uintptr_t for the pointer arithmetic as below so that everything matches? > > Oh you are right: > > $ git grep -E '(get_image_size|qemu_target_page_size|load_image_size)\(' include > include/exec/target_page.h:17:size_t qemu_target_page_size(void); > include/hw/loader.h:13:int64_t get_image_size(const char *filename); > include/hw/loader.h:30:ssize_t load_image_size(const char *filename, void *addr, > size_t size); > > So I guess int64_t is safer. Okay. >>>> 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); >>> >>> memory_region_get_ram_ptr() returns a 'void *' so this looks dubious. >>> Maybe use a local variable to ease offset calculation? >>> >>> char *rombase = memory_region_get_ram_ptr(&nd->decl_rom); >>> ret = load_image_size(path, rombase + align_size - size, size); >>> >>> Otherwise KISS but ugly: >>> >>> ret = load_image_size(path, >>> (void *)((uintptr_t)memory_region_get_ram_ptr(&nd->decl_rom) >>> + align_size - size), size); >> >> I prefer the first approach, but with uint8_t instead of char since it clarifies >> that it is a pointer to an arbitrary set of bytes as opposed to a string. Does that >> seem reasonable? > > Sure! Then with that: > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Thanks! I've also had an off-list request from Elliot to increase the maximum Declaration ROM size as enabling full debug can hit the existing 128k limit. I'll add this simple change into the series and repost as v3. > >> >>>> 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, since Nubus ROM images are unusual in that they are aligned to the end of the slot address space. 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(-)