On Mon, Apr 24, 2023 at 04:42:00PM -0400, Michael S. Tsirkin wrote:
> 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
> >
> > 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>
Also isn't ROM size reflected in config space etc?
I don't remember code that would update that on migration.
> > ---
> > 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);
>
> You know this steals 2GB from address space, yes? This is quite a lot
> ...
>
> > 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
