On Mon, Apr 24, 2023 at 11:36:47PM +0300, Vladimir Sementsov-Ogievskiy wrote: > On migration, on target we load local ROM file. But actual ROM content > migrates through migration channel. Original ROM content from local > file doesn't matter. But when size mismatch - we have an error like > > Size mismatch: 0000:00:03.0/virtio-net-pci.rom: 0x40000 != 0x80000: Invalid > argument
Oh, this is this old bug then: https://bugs.launchpad.net/ubuntu/+source/qemu/+bug/1713490 People seem to be "fixing" this by downgrading ROMs. Actually, I think the fix is different: we need to build versions of ROMs for old machine types that can fit in the old BAR size. Gerd, Laszlo what's your take on all this? > Let's just allow resizing of ROM memory. This way migration is not > relate on local ROM file on target node which is loaded by default but > is not actually needed. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru> > --- > hw/pci/pci.c | 7 +++++-- > include/exec/memory.h | 26 ++++++++++++++++++++++++++ > softmmu/memory.c | 39 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 70 insertions(+), 2 deletions(-) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index def5000e7b..72ee8f6aea 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -59,6 +59,8 @@ > # define PCI_DPRINTF(format, ...) do { } while (0) > #endif > > +#define MAX_ROM_SIZE (2 * GiB) > + > bool pci_available = true; > > static char *pcibus_get_dev_path(DeviceState *dev); > @@ -2341,7 +2343,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool > is_default_rom, > error_setg(errp, "romfile \"%s\" is empty", pdev->romfile); > g_free(path); > return; > - } else if (size > 2 * GiB) { > + } else if (size > MAX_ROM_SIZE) { > error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 > GiB)", > pdev->romfile); > g_free(path); > @@ -2366,7 +2368,8 @@ static void pci_add_option_rom(PCIDevice *pdev, bool > is_default_rom, > snprintf(name, sizeof(name), "%s.rom", > object_get_typename(OBJECT(pdev))); > } > pdev->has_rom = true; > - memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, pdev->romsize, > &error_fatal); > + memory_region_init_rom_resizable(&pdev->rom, OBJECT(pdev), name, > + pdev->romsize, MAX_ROM_SIZE, > &error_fatal); > ptr = memory_region_get_ram_ptr(&pdev->rom); > if (load_image_size(path, ptr, size) < 0) { > error_setg(errp, "failed to load romfile \"%s\"", pdev->romfile); > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 15ade918ba..ed1e5d9126 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -1453,6 +1453,19 @@ void memory_region_init_rom_nomigrate(MemoryRegion *mr, > uint64_t size, > Error **errp); > > +/* > + * memory_region_init_rom_nomigrate_resizable: same as > + * memory_region_init_rom_nomigrate(), but initialize resizable memory > region. > + * > + * @max_size maximum allowed size. > + */ > +void memory_region_init_rom_nomigrate_resizable(MemoryRegion *mr, > + struct Object *owner, > + const char *name, > + uint64_t size, > + uint64_t max_size, > + Error **errp); > + > /** > * memory_region_init_rom_device_nomigrate: Initialize a ROM memory region. > * Writes are handled via callbacks. > @@ -1562,6 +1575,19 @@ void memory_region_init_rom(MemoryRegion *mr, > uint64_t size, > Error **errp); > > +/* > + * memory_region_init_rom_resizable: same as memory_region_init_rom(), > + * but initialize resizable memory region. > + * > + * @max_size maximum allowed size. > + */ > +void memory_region_init_rom_resizable(MemoryRegion *mr, > + struct Object *owner, > + const char *name, > + uint64_t size, > + uint64_t max_size, > + Error **errp); > + > /** > * memory_region_init_rom_device: Initialize a ROM memory region. > * Writes are handled via callbacks. > diff --git a/softmmu/memory.c b/softmmu/memory.c > index b1a6cae6f5..744d03bc02 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -1701,6 +1701,18 @@ void memory_region_init_rom_nomigrate(MemoryRegion *mr, > mr->readonly = true; > } > > +void memory_region_init_rom_nomigrate_resizable(MemoryRegion *mr, > + struct Object *owner, > + const char *name, > + uint64_t size, > + uint64_t max_size, > + Error **errp) > +{ > + memory_region_init_resizeable_ram(mr, owner, name, size, max_size, NULL, > + errp); > + mr->readonly = true; > +} > + > void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, > Object *owner, > const MemoryRegionOps *ops, > @@ -3580,6 +3592,33 @@ void memory_region_init_rom(MemoryRegion *mr, > vmstate_register_ram(mr, owner_dev); > } > > +void memory_region_init_rom_resizable(MemoryRegion *mr, > + struct Object *owner, > + const char *name, > + uint64_t size, > + uint64_t max_size, > + Error **errp) > +{ > + DeviceState *owner_dev; > + Error *err = NULL; > + > + memory_region_init_rom_nomigrate_resizable(mr, owner, name, size, > max_size, > + &err); > + if (err) { > + error_propagate(errp, err); > + return; > + } > + /* > + * This will assert if owner is neither NULL nor a DeviceState. > + * We only want the owner here for the purposes of defining a > + * unique name for migration. TODO: Ideally we should implement > + * a naming scheme for Objects which are not DeviceStates, in > + * which case we can relax this restriction. > + */ > + owner_dev = DEVICE(owner); > + vmstate_register_ram(mr, owner_dev); > +} > + > void memory_region_init_rom_device(MemoryRegion *mr, > Object *owner, > const MemoryRegionOps *ops, > -- > 2.34.1
