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

[Mark Cave-Ayland]

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.