[PATCH] pci: make ROM memory resizable

[PATCH] pci: make ROM memory resizable

[Vladimir Sementsov-Ogievskiy]

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

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

Re: [PATCH] pci: make ROM memory resizable

[Michael S. Tsirkin]

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

Re: [PATCH] pci: make ROM memory resizable

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 23:45, Michael S. Tsirkin wrote:
> 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<vsementsov@yandex-team.ru>
> Also isn't ROM size reflected in config space etc?
> I don't remember code that would update that on migration.
> 
> 

Thanks a lot for fast answers!

Hmm. I'm a newbie in these things.

But yes, I noted, that my patch helps, if, for example jump from 200K to 500K zero-filled ROM file. But if jump to 600K, migration fails with

(qemu) qemu: get_pci_config_device: Bad config data: i=0x32 read: b8 device: 0 cmask: ff wmask: f0 w1cmask:0
qemu: Failed to load PCIDevice:config
qemu: Failed to load virtio-net:virtio
qemu: error while loading state for instance 0x0 of device '0000:00:03.0/virtio-net'
qemu: load of migration failed: Invalid argument


I thought, that, maybe, romfile for this device just mustn't be more than 512K where config starts. But now I think that it's exactly the problem you are saying about.


I know also, that there were another step around this problem: 08b1df8ff463e72b087 "pci: add romsize property".. But it doesn't help when you already have a running instance with small ROM and want to migrate it to the node where you have corresponding local ROM file updated to new package with bigger size.


Hmm. So, simply reuse "resizable" memory blocks doesn't help. And I need more precise reinitialization of device on load of incoming migration..

-- 
Best regards,
Vladimir

Re: [PATCH] pci: make ROM memory resizable

[Michael S. Tsirkin]

On Tue, Apr 25, 2023 at 12:02:49AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 24.04.23 23:45, Michael S. Tsirkin wrote:
> > 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<vsementsov@yandex-team.ru>
> > Also isn't ROM size reflected in config space etc?
> > I don't remember code that would update that on migration.
> > 
> > 
> 
> Thanks a lot for fast answers!
> 
> Hmm. I'm a newbie in these things.
> 
> But yes, I noted, that my patch helps, if, for example jump from 200K to 500K zero-filled ROM file. But if jump to 600K, migration fails with
> 
> (qemu) qemu: get_pci_config_device: Bad config data: i=0x32 read: b8 device: 0 cmask: ff wmask: f0 w1cmask:0
> qemu: Failed to load PCIDevice:config
> qemu: Failed to load virtio-net:virtio
> qemu: error while loading state for instance 0x0 of device '0000:00:03.0/virtio-net'
> qemu: load of migration failed: Invalid argument
> 
> 
> I thought, that, maybe, romfile for this device just mustn't be more than 512K where config starts. But now I think that it's exactly the problem you are saying about.
> 
> 
> I know also, that there were another step around this problem: 08b1df8ff463e72b087 "pci: add romsize property".. But it doesn't help when you already have a running instance with small ROM and want to migrate it to the node where you have corresponding local ROM file updated to new package with bigger size.
> 

set romsize on destination?

> Hmm. So, simply reuse "resizable" memory blocks doesn't help. And I need more precise reinitialization of device on load of incoming migration..
> 
> -- 
> Best regards,
> Vladimir

Re: [PATCH] pci: make ROM memory resizable

[Vladimir Sementsov-Ogievskiy]

On 25.04.23 00:06, Michael S. Tsirkin wrote:
> On Tue, Apr 25, 2023 at 12:02:49AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On 24.04.23 23:45, Michael S. Tsirkin wrote:
>>> 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<vsementsov@yandex-team.ru>
>>> Also isn't ROM size reflected in config space etc?
>>> I don't remember code that would update that on migration.
>>>
>>>
>>
>> Thanks a lot for fast answers!
>>
>> Hmm. I'm a newbie in these things.
>>
>> But yes, I noted, that my patch helps, if, for example jump from 200K to 500K zero-filled ROM file. But if jump to 600K, migration fails with
>>
>> (qemu) qemu: get_pci_config_device: Bad config data: i=0x32 read: b8 device: 0 cmask: ff wmask: f0 w1cmask:0
>> qemu: Failed to load PCIDevice:config
>> qemu: Failed to load virtio-net:virtio
>> qemu: error while loading state for instance 0x0 of device '0000:00:03.0/virtio-net'
>> qemu: load of migration failed: Invalid argument
>>
>>
>> I thought, that, maybe, romfile for this device just mustn't be more than 512K where config starts. But now I think that it's exactly the problem you are saying about.
>>
>>
>> I know also, that there were another step around this problem: 08b1df8ff463e72b087 "pci: add romsize property".. But it doesn't help when you already have a running instance with small ROM and want to migrate it to the node where you have corresponding local ROM file updated to new package with bigger size.
>>
> 
> set romsize on destination?

This does not work if you try to set smaller size, it say:

romfile "b" (409600 bytes) is too large for ROM size 262144

so we need additional option like romalloc=on, that say: don't load any file but just allocate ROM by romsize option. Or just handle romfile="",romsize=SOME_SIZE in this way.

But I'm still interested in possibility to avoid any additional option on target.

> 
>> Hmm. So, simply reuse "resizable" memory blocks doesn't help. And I need more precise reinitialization of device on load of incoming migration..
>>
>> -- 
>> Best regards,
>> Vladimir
> 

-- 
Best regards,
Vladimir

Re: [PATCH] pci: make ROM memory resizable

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 23:41, Michael S. Tsirkin wrote:
>> @@ -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

Oops no, I didn't.

-- 
Best regards,
Vladimir

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


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

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

Re: [PATCH] pci: make ROM memory resizable

[Vladimir Sementsov-Ogievskiy]

On 25.04.23 10:43, Michael S. Tsirkin wrote:
> 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.
> 
> 

Yes I understand that distribution may work-around the problem just having all needed roms on target and specifying correct romfile= argument.

But this is not ideal: having the file only to get its size, to not mismatch with incoming RAM block. There should be way to migrate ROMs automatically without extra files on target.

-- 
Best regards,
Vladimir

Re: [PATCH] pci: make ROM memory resizable

[Michael S. Tsirkin]

On Tue, Apr 25, 2023 at 11:34:09AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 25.04.23 10:43, Michael S. Tsirkin wrote:
> > 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.
> > 
> > 
> 
> Yes I understand that distribution may work-around the problem just having all needed roms on target and specifying correct romfile= argument.
> 
> But this is not ideal: having the file only to get its size, to not mismatch with incoming RAM block. There should be way to migrate ROMs automatically without extra files on target.

This not "only to get its size". It is so you can start old machine type
on the new box and get the same memory layout.

Fundamentally, the approach QEMU takes is that it does not matter
how you migrate: live, or by killing and restarting guest.
What we try to do as best we can, is make the box look the same.

-- 
MST

Re: [PATCH] pci: make ROM memory resizable

[Laszlo Ersek]

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
> 

Re: [PATCH] pci: make ROM memory resizable

[Gerd Hoffmann]

On Tue, Apr 25, 2023 at 03:26:50AM -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, a 512k rom.  Where does that come from?

The ones shipped by qemu are all between 128k and 256k in size,
which gets rounded up to 256k (0x40000).  Has not changed since
we added efi boot rom support ...

If you supply your own versions for some reason you must make sure
they have identical size on all host machines.

take care,
  Gerd

Re: [PATCH] pci: make ROM memory resizable

[Vladimir Sementsov-Ogievskiy]

On 25.04.23 15:10, Gerd Hoffmann wrote:
> On Tue, Apr 25, 2023 at 03:26:50AM -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, a 512k rom.  Where does that come from?
> 
> The ones shipped by qemu are all between 128k and 256k in size,
> which gets rounded up to 256k (0x40000).  Has not changed since
> we added efi boot rom support ...
> 
> If you supply your own versions for some reason you must make sure
> they have identical size on all host machines.
> 

on my ubuntu 22.04:

dpkg -L ipxe-qemu | grep efi-virtio
/usr/lib/ipxe/qemu/efi-virtio.rom

ls -lthr /usr/lib/ipxe/qemu/efi-virtio.rom
-rw-r--r-- 1 root root 512K Jan 13  2022 /usr/lib/ipxe/qemu/efi-virtio.rom

If look inside the file, it's filled with ffff starting from 0x32400

-- 
Best regards,
Vladimir

Re: [PATCH] pci: make ROM memory resizable

[Gerd Hoffmann]

  Hi,

> > If you supply your own versions for some reason you must make sure
> > they have identical size on all host machines.
> 
> on my ubuntu 22.04:
> 
> dpkg -L ipxe-qemu | grep efi-virtio
> /usr/lib/ipxe/qemu/efi-virtio.rom
> 
> ls -lthr /usr/lib/ipxe/qemu/efi-virtio.rom
> -rw-r--r-- 1 root root 512K Jan 13  2022 /usr/lib/ipxe/qemu/efi-virtio.rom
> 
> If look inside the file, it's filled with ffff starting from 0x32400

So it would actually fit into 256k without problems.
Strange ...

I guess that one is for ubuntu to sort out.

take care,
  Gerd

Re: [PATCH] pci: make ROM memory resizable

[Vladimir Sementsov-Ogievskiy]

On 24.04.23 23:36, 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>

Seems, automatically reconfigure the memory on incoming migration is not so easy.. ROM size really influence config of the device, and after some digging in the code I still cannot say where it happens.

So, I'm going to suggest another way, new series "[PATCH 0/3] ROM migration" will substitute this patch.

-- 
Best regards,
Vladimir

Re: [PATCH] pci: make ROM memory resizable

[Igor Mammedov]

On Tue, 25 Apr 2023 13:55:55 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:

> On 24.04.23 23:36, 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>  
> 
> Seems, automatically reconfigure the memory on incoming migration is not so easy.. ROM size really influence config of the device, and after some digging in the code I still cannot say where it happens.
You can't change PCI device bars on migration as it practically
will mess up memory mapping (if you manage to do so), but
guest will still think that it has the old one. So you'd need
a guest side that will notice migration and deal with remapping.

(one way to work around it could be unplug nic, and once it's gone
migrate/replug on target side)

> 
> So, I'm going to suggest another way, new series "[PATCH 0/3] ROM migration" will substitute this patch.
>