Re: [PATCH] pci: make ROM memory resizable

[Michael S. Tsirkin]

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 <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);

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