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

[Philippe Mathieu-Daudé]

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.