Re: [PATCH] pci: make ROM memory resizable

[Laszlo Ersek <lersek@redhat.com>]

On 4/25/23 09:26, 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.

My working memory has been that we'd build the ROM, such as SeaBIOS, in
multiple configs (resulting in different sizes -- a smaller size for the
old machine type, and a larger size for the new machine type). The new
(large) build would stay "bios.bin", and the old (small) binary would
get a new name. Then the old machine type's compat knobs would include a
setting for loading the old (= small) ROM. This would cause the
destination QEMU to size the ROM area as "small", which would
accommodate the incoming stream just fine.

However, my memory has not been (entirely) correct. Commit bcf2b7d2af7c
("pc: switch 2.0 machine types to large seabios binary", 2013-12-06)
indicates that the *new* (large) binary gets the new name. That confuses
me; it does not seem consistent with how compat knobs usually tie down
old machine types.

It does not change the mechanism, I think, but naming the ROM files (on
host distros) gets more complicated, perhaps. I think Gerd will know
more history.

Laszlo

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