Re: [PATCH] pci: make ROM memory resizable

[Michael S. Tsirkin]

On Tue, Apr 25, 2023 at 03:26:54AM -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
> 
> 
> 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?

Actually, ignore this - we do keep old ROMs around specifically to avoid
ROM size changes and have been for ever. E.g.:

commit c45e5b5b30ac1f5505725a7b36e68cedfce4f01f
Author: Gerd Hoffmann <kraxel@redhat.com>
Date:   Tue Feb 26 17:46:11 2013 +0100

    Switch to efi-enabled nic roms by default
    
    All PCI nics are switched to EFI-enabled roms by default.  They are
    composed from three images (legacy, efi ia32 & efi x86), so classic
    pxe booting will continue to work.
    
    Exception: eepro100 is not switched, it uses a single rom for all
    emulated eepro100 variants, then goes patch the rom header on the
    fly with the correct PCI IDs.  I doubt that will work as-is with
    the efi roms.
    
    Keep old roms for 1.4+older machine types via compat properties,
    needed because the efi-enabled roms are larger so the pci rom bar
    size would change.
    
    Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>


So it's downstream messing up with things, overriding the
rom file then changing its size.


On fedora I find both pxe virtio and efi virtio so it gets it right.


> 
> 
> > 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 <vsementsov@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