[PATCH 0/3] ROM migration

[PATCH 0/3] ROM migration

[Vladimir Sementsov-Ogievskiy]

Hi all!

That's a substitution for my first attempt:
 [PATCH] pci: make ROM memory resizable
 <20230424203647.94614-1-vsementsov@yandex-team.ru>

Here I suggest another way to solve a problem, when we have existing
running QEMU with old option ROM of small size and want to migrate to
new environment where we don't have this ROM file.

All the details are in patch 03; 01-02 are simple code style
improvements.

Vladimir Sementsov-Ogievskiy (3):
  pci: pci_add_option_rom(): improve style
  pci: pci_add_option_rom(): refactor: use g_autofree for path variable
  pci: ROM preallocation for incoming migration

 hw/pci/pci.c | 98 ++++++++++++++++++++++++++++------------------------
 1 file changed, 52 insertions(+), 46 deletions(-)

-- 
2.34.1

[PATCH 1/3] pci: pci_add_option_rom(): improve style

[Vladimir Sementsov-Ogievskiy]

Fix over-80 lines and missing curly brackets for if-operators, which
are required by QEMU coding style.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/pci/pci.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index def5000e7b..4a61c8d24a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2297,10 +2297,12 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
     char name[32];
     const VMStateDescription *vmsd;
 
-    if (!pdev->romfile)
+    if (!pdev->romfile) {
         return;
-    if (strlen(pdev->romfile) == 0)
+    }
+    if (strlen(pdev->romfile) == 0) {
         return;
+    }
 
     if (!pdev->rom_bar) {
         /*
@@ -2349,7 +2351,8 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
     }
     if (pdev->romsize != -1) {
         if (size > pdev->romsize) {
-            error_setg(errp, "romfile \"%s\" (%u bytes) is too large for ROM size %u",
+            error_setg(errp, "romfile \"%s\" (%u bytes) "
+                       "is too large for ROM size %u",
                        pdev->romfile, (uint32_t)size, pdev->romsize);
             g_free(path);
             return;
@@ -2359,14 +2362,13 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
     }
 
     vmsd = qdev_get_vmsd(DEVICE(pdev));
+    snprintf(name, sizeof(name), "%s.rom",
+             vmsd ? vmsd->name : object_get_typename(OBJECT(pdev)));
 
-    if (vmsd) {
-        snprintf(name, sizeof(name), "%s.rom", vmsd->name);
-    } else {
-        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(&pdev->rom, OBJECT(pdev), name, pdev->romsize,
+                           &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);
-- 
2.34.1

[PATCH 2/3] pci: pci_add_option_rom(): refactor: use g_autofree for path variable

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/pci/pci.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 4a61c8d24a..a442f8fce1 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2292,7 +2292,7 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
                                Error **errp)
 {
     int64_t size;
-    char *path;
+    g_autofree char *path = NULL;
     void *ptr;
     char name[32];
     const VMStateDescription *vmsd;
@@ -2337,16 +2337,13 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
     size = get_image_size(path);
     if (size < 0) {
         error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
-        g_free(path);
         return;
     } else if (size == 0) {
         error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
-        g_free(path);
         return;
     } else if (size > 2 * GiB) {
         error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 GiB)",
                    pdev->romfile);
-        g_free(path);
         return;
     }
     if (pdev->romsize != -1) {
@@ -2354,7 +2351,6 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
             error_setg(errp, "romfile \"%s\" (%u bytes) "
                        "is too large for ROM size %u",
                        pdev->romfile, (uint32_t)size, pdev->romsize);
-            g_free(path);
             return;
         }
     } else {
@@ -2372,10 +2368,8 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
     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);
-        g_free(path);
         return;
     }
-    g_free(path);
 
     if (is_default_rom) {
         /* Only the default rom images will be patched (if needed). */
-- 
2.34.1

[PATCH 3/3] pci: ROM preallocation for incoming migration

[Vladimir Sementsov-Ogievskiy]

On incoming migration we have the following sequence to load option
ROM:

1. On device realize we do normal load ROM from the file

2. Than, on incoming migration we rewrite ROM from the incoming RAM
   block. If sizes mismatch we fail.

This is not ideal when we migrate to updated distribution: we have to
keep old ROM files in new distribution and be careful around romfile
property to load correct ROM file. Which is loaded actually just to
allocate the ROM with correct length.

Note, that romsize property doesn't really help: if we try to specify
it when default romfile is larger, it fails with something like:

romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536

This commit brings new behavior for romfile="",romsize=SIZE combination
of options. Current behavior is just ignore romsize and not load or
create any ROM.

Let's instead preallocate ROM, not loading any file. This way we can
migrate old vm to new environment not thinking about ROM files on
destination host:

1. specify romfile="",romsize=SIZE on target, with correct SIZE
   (actually, size of romfile on source aligned up to power of two, or
    just original romsize option on source)

2. On device realize we just preallocate ROM, and not load any file

3. On incoming migration ROM is filled from the migration stream

As a bonus we avoid extra reading from ROM file on target.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
---
 hw/pci/pci.c | 76 +++++++++++++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 33 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index a442f8fce1..039e762b66 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2293,17 +2293,21 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
 {
     int64_t size;
     g_autofree char *path = NULL;
-    void *ptr;
     char name[32];
     const VMStateDescription *vmsd;
+    bool load_file;
 
     if (!pdev->romfile) {
         return;
     }
-    if (strlen(pdev->romfile) == 0) {
+
+    load_file = strlen(pdev->romfile) > 0;
+    if (!load_file && pdev->romsize == -1) {
         return;
     }
 
+    assert(load_file || !is_default_rom);
+
     if (!pdev->rom_bar) {
         /*
          * Load rom via fw_cfg instead of creating a rom bar,
@@ -2329,32 +2333,35 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
         return;
     }
 
-    path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
-    if (path == NULL) {
-        path = g_strdup(pdev->romfile);
-    }
+    if (load_file) {
+        path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
+        if (path == NULL) {
+            path = g_strdup(pdev->romfile);
+        }
 
-    size = get_image_size(path);
-    if (size < 0) {
-        error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
-        return;
-    } else if (size == 0) {
-        error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
-        return;
-    } else if (size > 2 * GiB) {
-        error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 GiB)",
-                   pdev->romfile);
-        return;
-    }
-    if (pdev->romsize != -1) {
-        if (size > pdev->romsize) {
-            error_setg(errp, "romfile \"%s\" (%u bytes) "
-                       "is too large for ROM size %u",
-                       pdev->romfile, (uint32_t)size, pdev->romsize);
+        size = get_image_size(path);
+        if (size < 0) {
+            error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
+            return;
+        } else if (size == 0) {
+            error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
+            return;
+        } else if (size > 2 * GiB) {
+            error_setg(errp,
+                       "romfile \"%s\" too large (size cannot exceed 2 GiB)",
+                       pdev->romfile);
             return;
         }
-    } else {
-        pdev->romsize = pow2ceil(size);
+        if (pdev->romsize != -1) {
+            if (size > pdev->romsize) {
+                error_setg(errp, "romfile \"%s\" (%u bytes) "
+                           "is too large for ROM size %u",
+                           pdev->romfile, (uint32_t)size, pdev->romsize);
+                return;
+            }
+        } else {
+            pdev->romsize = pow2ceil(size);
+        }
     }
 
     vmsd = qdev_get_vmsd(DEVICE(pdev));
@@ -2365,15 +2372,18 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
     memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, pdev->romsize,
                            &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);
-        return;
-    }
+    if (load_file) {
+        void *ptr = memory_region_get_ram_ptr(&pdev->rom);
 
-    if (is_default_rom) {
-        /* Only the default rom images will be patched (if needed). */
-        pci_patch_ids(pdev, ptr, size);
+        if (load_image_size(path, ptr, size) < 0) {
+            error_setg(errp, "failed to load romfile \"%s\"", pdev->romfile);
+            return;
+        }
+
+        if (is_default_rom) {
+            /* Only the default rom images will be patched (if needed). */
+            pci_patch_ids(pdev, ptr, size);
+        }
     }
 
     pci_register_bar(pdev, PCI_ROM_SLOT, 0, &pdev->rom);
-- 
2.34.1

Re: [PATCH 3/3] pci: ROM preallocation for incoming migration

[Michael S. Tsirkin]

On Tue, Apr 25, 2023 at 01:56:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On incoming migration we have the following sequence to load option
> ROM:
> 
> 1. On device realize we do normal load ROM from the file
> 
> 2. Than, on incoming migration we rewrite ROM from the incoming RAM
>    block. If sizes mismatch we fail.
> 
> This is not ideal when we migrate to updated distribution: we have to
> keep old ROM files in new distribution and be careful around romfile
> property to load correct ROM file. Which is loaded actually just to
> allocate the ROM with correct length.
> 
> Note, that romsize property doesn't really help: if we try to specify
> it when default romfile is larger, it fails with something like:
> 
> romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
> 
> This commit brings new behavior for romfile="",romsize=SIZE combination
> of options. Current behavior is just ignore romsize and not load or
> create any ROM.
> 
> Let's instead preallocate ROM, not loading any file. This way we can
> migrate old vm to new environment not thinking about ROM files on
> destination host:
> 
> 1. specify romfile="",romsize=SIZE on target, with correct SIZE
>    (actually, size of romfile on source aligned up to power of two, or
>     just original romsize option on source)
> 
> 2. On device realize we just preallocate ROM, and not load any file
> 
> 3. On incoming migration ROM is filled from the migration stream
> 
> As a bonus we avoid extra reading from ROM file on target.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

why is this a bad idea:
- on source presumably user overrides romfile
- we have a general rule that source and destination flags must match

I propose instead to ignore romfile if qemu is incoming migration
and romsize has been specified.

> ---
>  hw/pci/pci.c | 76 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 43 insertions(+), 33 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index a442f8fce1..039e762b66 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2293,17 +2293,21 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
>  {
>      int64_t size;
>      g_autofree char *path = NULL;
> -    void *ptr;
>      char name[32];
>      const VMStateDescription *vmsd;
> +    bool load_file;
>  
>      if (!pdev->romfile) {
>          return;
>      }
> -    if (strlen(pdev->romfile) == 0) {
> +
> +    load_file = strlen(pdev->romfile) > 0;
> +    if (!load_file && pdev->romsize == -1) {
>          return;
>      }
>  
> +    assert(load_file || !is_default_rom);
> +
>      if (!pdev->rom_bar) {
>          /*
>           * Load rom via fw_cfg instead of creating a rom bar,
> @@ -2329,32 +2333,35 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
>          return;
>      }
>  
> -    path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
> -    if (path == NULL) {
> -        path = g_strdup(pdev->romfile);
> -    }
> +    if (load_file) {
> +        path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
> +        if (path == NULL) {
> +            path = g_strdup(pdev->romfile);
> +        }
>  
> -    size = get_image_size(path);
> -    if (size < 0) {
> -        error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
> -        return;
> -    } else if (size == 0) {
> -        error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
> -        return;
> -    } else if (size > 2 * GiB) {
> -        error_setg(errp, "romfile \"%s\" too large (size cannot exceed 2 GiB)",
> -                   pdev->romfile);
> -        return;
> -    }
> -    if (pdev->romsize != -1) {
> -        if (size > pdev->romsize) {
> -            error_setg(errp, "romfile \"%s\" (%u bytes) "
> -                       "is too large for ROM size %u",
> -                       pdev->romfile, (uint32_t)size, pdev->romsize);
> +        size = get_image_size(path);
> +        if (size < 0) {
> +            error_setg(errp, "failed to find romfile \"%s\"", pdev->romfile);
> +            return;
> +        } else if (size == 0) {
> +            error_setg(errp, "romfile \"%s\" is empty", pdev->romfile);
> +            return;
> +        } else if (size > 2 * GiB) {
> +            error_setg(errp,
> +                       "romfile \"%s\" too large (size cannot exceed 2 GiB)",
> +                       pdev->romfile);
>              return;
>          }
> -    } else {
> -        pdev->romsize = pow2ceil(size);
> +        if (pdev->romsize != -1) {
> +            if (size > pdev->romsize) {
> +                error_setg(errp, "romfile \"%s\" (%u bytes) "
> +                           "is too large for ROM size %u",
> +                           pdev->romfile, (uint32_t)size, pdev->romsize);
> +                return;
> +            }
> +        } else {
> +            pdev->romsize = pow2ceil(size);
> +        }
>      }
>  
>      vmsd = qdev_get_vmsd(DEVICE(pdev));
> @@ -2365,15 +2372,18 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
>      memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, pdev->romsize,
>                             &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);
> -        return;
> -    }
> +    if (load_file) {
> +        void *ptr = memory_region_get_ram_ptr(&pdev->rom);
>  
> -    if (is_default_rom) {
> -        /* Only the default rom images will be patched (if needed). */
> -        pci_patch_ids(pdev, ptr, size);
> +        if (load_image_size(path, ptr, size) < 0) {
> +            error_setg(errp, "failed to load romfile \"%s\"", pdev->romfile);
> +            return;
> +        }
> +
> +        if (is_default_rom) {
> +            /* Only the default rom images will be patched (if needed). */
> +            pci_patch_ids(pdev, ptr, size);
> +        }
>      }
>  
>      pci_register_bar(pdev, PCI_ROM_SLOT, 0, &pdev->rom);
> -- 
> 2.34.1

Re: [PATCH 3/3] pci: ROM preallocation for incoming migration

[Vladimir Sementsov-Ogievskiy]

On 25.04.23 15:43, Michael S. Tsirkin wrote:
> On Tue, Apr 25, 2023 at 01:56:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> On incoming migration we have the following sequence to load option
>> ROM:
>>
>> 1. On device realize we do normal load ROM from the file
>>
>> 2. Than, on incoming migration we rewrite ROM from the incoming RAM
>>     block. If sizes mismatch we fail.
>>
>> This is not ideal when we migrate to updated distribution: we have to
>> keep old ROM files in new distribution and be careful around romfile
>> property to load correct ROM file. Which is loaded actually just to
>> allocate the ROM with correct length.
>>
>> Note, that romsize property doesn't really help: if we try to specify
>> it when default romfile is larger, it fails with something like:
>>
>> romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
>>
>> This commit brings new behavior for romfile="",romsize=SIZE combination
>> of options. Current behavior is just ignore romsize and not load or
>> create any ROM.
>>
>> Let's instead preallocate ROM, not loading any file. This way we can
>> migrate old vm to new environment not thinking about ROM files on
>> destination host:
>>
>> 1. specify romfile="",romsize=SIZE on target, with correct SIZE
>>     (actually, size of romfile on source aligned up to power of two, or
>>      just original romsize option on source)
>>
>> 2. On device realize we just preallocate ROM, and not load any file
>>
>> 3. On incoming migration ROM is filled from the migration stream
>>
>> As a bonus we avoid extra reading from ROM file on target.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
> why is this a bad idea:
> - on source presumably user overrides romfile
> - we have a general rule that source and destination flags must match
> 
> I propose instead to ignore romfile if qemu is incoming migration
> and romsize has been specified.
> 

Hmm, that would work even better, as no additional options needed, thanks. I'll resend

-- 
Best regards,
Vladimir

Re: [PATCH 3/3] pci: ROM preallocation for incoming migration

[Vladimir Sementsov-Ogievskiy]

On 25.04.23 16:07, Vladimir Sementsov-Ogievskiy wrote:
> On 25.04.23 15:43, Michael S. Tsirkin wrote:
>> On Tue, Apr 25, 2023 at 01:56:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> On incoming migration we have the following sequence to load option
>>> ROM:
>>>
>>> 1. On device realize we do normal load ROM from the file
>>>
>>> 2. Than, on incoming migration we rewrite ROM from the incoming RAM
>>>     block. If sizes mismatch we fail.
>>>
>>> This is not ideal when we migrate to updated distribution: we have to
>>> keep old ROM files in new distribution and be careful around romfile
>>> property to load correct ROM file. Which is loaded actually just to
>>> allocate the ROM with correct length.
>>>
>>> Note, that romsize property doesn't really help: if we try to specify
>>> it when default romfile is larger, it fails with something like:
>>>
>>> romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
>>>
>>> This commit brings new behavior for romfile="",romsize=SIZE combination
>>> of options. Current behavior is just ignore romsize and not load or
>>> create any ROM.
>>>
>>> Let's instead preallocate ROM, not loading any file. This way we can
>>> migrate old vm to new environment not thinking about ROM files on
>>> destination host:
>>>
>>> 1. specify romfile="",romsize=SIZE on target, with correct SIZE
>>>     (actually, size of romfile on source aligned up to power of two, or
>>>      just original romsize option on source)
>>>
>>> 2. On device realize we just preallocate ROM, and not load any file
>>>
>>> 3. On incoming migration ROM is filled from the migration stream
>>>
>>> As a bonus we avoid extra reading from ROM file on target.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
>> why is this a bad idea:
>> - on source presumably user overrides romfile
>> - we have a general rule that source and destination flags must match
>>
>> I propose instead to ignore romfile if qemu is incoming migration
>> and romsize has been specified.
>>
> 
> Hmm, that would work even better, as no additional options needed, thanks. I'll resend
> 

romsize needed anyway, of course.

-- 
Best regards,
Vladimir

Re: [PATCH 3/3] pci: ROM preallocation for incoming migration

[Michael S. Tsirkin]

On Tue, Apr 25, 2023 at 04:19:12PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 25.04.23 16:07, Vladimir Sementsov-Ogievskiy wrote:
> > On 25.04.23 15:43, Michael S. Tsirkin wrote:
> > > On Tue, Apr 25, 2023 at 01:56:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > On incoming migration we have the following sequence to load option
> > > > ROM:
> > > > 
> > > > 1. On device realize we do normal load ROM from the file
> > > > 
> > > > 2. Than, on incoming migration we rewrite ROM from the incoming RAM
> > > >     block. If sizes mismatch we fail.
> > > > 
> > > > This is not ideal when we migrate to updated distribution: we have to
> > > > keep old ROM files in new distribution and be careful around romfile
> > > > property to load correct ROM file. Which is loaded actually just to
> > > > allocate the ROM with correct length.
> > > > 
> > > > Note, that romsize property doesn't really help: if we try to specify
> > > > it when default romfile is larger, it fails with something like:
> > > > 
> > > > romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
> > > > 
> > > > This commit brings new behavior for romfile="",romsize=SIZE combination
> > > > of options. Current behavior is just ignore romsize and not load or
> > > > create any ROM.
> > > > 
> > > > Let's instead preallocate ROM, not loading any file. This way we can
> > > > migrate old vm to new environment not thinking about ROM files on
> > > > destination host:
> > > > 
> > > > 1. specify romfile="",romsize=SIZE on target, with correct SIZE
> > > >     (actually, size of romfile on source aligned up to power of two, or
> > > >      just original romsize option on source)
> > > > 
> > > > 2. On device realize we just preallocate ROM, and not load any file
> > > > 
> > > > 3. On incoming migration ROM is filled from the migration stream
> > > > 
> > > > As a bonus we avoid extra reading from ROM file on target.
> > > > 
> > > > Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>
> > > why is this a bad idea:
> > > - on source presumably user overrides romfile
> > > - we have a general rule that source and destination flags must match
> > > 
> > > I propose instead to ignore romfile if qemu is incoming migration
> > > and romsize has been specified.
> > > 
> > 
> > Hmm, that would work even better, as no additional options needed, thanks. I'll resend
> > 
> 
> romsize needed anyway, of course.

yes but it can match on source and dest.

> -- 
> Best regards,
> Vladimir

Re: [PATCH 3/3] pci: ROM preallocation for incoming migration

[Igor Mammedov]

On Tue, 25 Apr 2023 09:32:54 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 25, 2023 at 04:19:12PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > On 25.04.23 16:07, Vladimir Sementsov-Ogievskiy wrote:  
> > > On 25.04.23 15:43, Michael S. Tsirkin wrote:  
> > > > On Tue, Apr 25, 2023 at 01:56:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:  
> > > > > On incoming migration we have the following sequence to load option
> > > > > ROM:
> > > > > 
> > > > > 1. On device realize we do normal load ROM from the file
> > > > > 
> > > > > 2. Than, on incoming migration we rewrite ROM from the incoming RAM
> > > > >     block. If sizes mismatch we fail.
> > > > > 
> > > > > This is not ideal when we migrate to updated distribution: we have to
> > > > > keep old ROM files in new distribution and be careful around romfile
> > > > > property to load correct ROM file. Which is loaded actually just to
> > > > > allocate the ROM with correct length.
> > > > > 
> > > > > Note, that romsize property doesn't really help: if we try to specify
> > > > > it when default romfile is larger, it fails with something like:
> > > > > 
> > > > > romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
> > > > > 
> > > > > This commit brings new behavior for romfile="",romsize=SIZE combination
> > > > > of options. Current behavior is just ignore romsize and not load or
> > > > > create any ROM.
> > > > > 
> > > > > Let's instead preallocate ROM, not loading any file. This way we can
> > > > > migrate old vm to new environment not thinking about ROM files on
> > > > > destination host:
> > > > > 
> > > > > 1. specify romfile="",romsize=SIZE on target, with correct SIZE
> > > > >     (actually, size of romfile on source aligned up to power of two, or
> > > > >      just original romsize option on source)
> > > > > 
> > > > > 2. On device realize we just preallocate ROM, and not load any file
> > > > > 
> > > > > 3. On incoming migration ROM is filled from the migration stream
> > > > > 
> > > > > As a bonus we avoid extra reading from ROM file on target.
> > > > > 
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>  
> > > > why is this a bad idea:
> > > > - on source presumably user overrides romfile
> > > > - we have a general rule that source and destination flags must match
> > > > 
> > > > I propose instead to ignore romfile if qemu is incoming migration
> > > > and romsize has been specified.
> > > >   
> > > 
> > > Hmm, that would work even better, as no additional options needed, thanks. I'll resend
> > >   
> > 
> > romsize needed anyway, of course.  
> 
> yes but it can match on source and dest.

Aren't we pushin issue from QEMU(/how distro packages firmware/)
to mgmt layer(s) going this way? 

> 
> > -- 
> > Best regards,
> > Vladimir  
> 
> 

Re: [PATCH 3/3] pci: ROM preallocation for incoming migration

[Vladimir Sementsov-Ogievskiy]

On 25.04.23 17:55, Igor Mammedov wrote:
> On Tue, 25 Apr 2023 09:32:54 -0400
> "Michael S. Tsirkin"<mst@redhat.com>  wrote:
> 
>> On Tue, Apr 25, 2023 at 04:19:12PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> On 25.04.23 16:07, Vladimir Sementsov-Ogievskiy wrote:
>>>> On 25.04.23 15:43, Michael S. Tsirkin wrote:
>>>>> On Tue, Apr 25, 2023 at 01:56:03PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> On incoming migration we have the following sequence to load option
>>>>>> ROM:
>>>>>>
>>>>>> 1. On device realize we do normal load ROM from the file
>>>>>>
>>>>>> 2. Than, on incoming migration we rewrite ROM from the incoming RAM
>>>>>>      block. If sizes mismatch we fail.
>>>>>>
>>>>>> This is not ideal when we migrate to updated distribution: we have to
>>>>>> keep old ROM files in new distribution and be careful around romfile
>>>>>> property to load correct ROM file. Which is loaded actually just to
>>>>>> allocate the ROM with correct length.
>>>>>>
>>>>>> Note, that romsize property doesn't really help: if we try to specify
>>>>>> it when default romfile is larger, it fails with something like:
>>>>>>
>>>>>> romfile "efi-virtio.rom" (160768 bytes) is too large for ROM size 65536
>>>>>>
>>>>>> This commit brings new behavior for romfile="",romsize=SIZE combination
>>>>>> of options. Current behavior is just ignore romsize and not load or
>>>>>> create any ROM.
>>>>>>
>>>>>> Let's instead preallocate ROM, not loading any file. This way we can
>>>>>> migrate old vm to new environment not thinking about ROM files on
>>>>>> destination host:
>>>>>>
>>>>>> 1. specify romfile="",romsize=SIZE on target, with correct SIZE
>>>>>>      (actually, size of romfile on source aligned up to power of two, or
>>>>>>       just original romsize option on source)
>>>>>>
>>>>>> 2. On device realize we just preallocate ROM, and not load any file
>>>>>>
>>>>>> 3. On incoming migration ROM is filled from the migration stream
>>>>>>
>>>>>> As a bonus we avoid extra reading from ROM file on target.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@yandex-team.ru>   
>>>>> why is this a bad idea:
>>>>> - on source presumably user overrides romfile
>>>>> - we have a general rule that source and destination flags must match
>>>>>
>>>>> I propose instead to ignore romfile if qemu is incoming migration
>>>>> and romsize has been specified.
>>>>>    
>>>> Hmm, that would work even better, as no additional options needed, thanks. I'll resend
>>>>    
>>> romsize needed anyway, of course.
>> yes but it can match on source and dest.
> Aren't we pushin issue from QEMU(/how distro packages firmware/)
> to mgmt layer(s) going this way?
> 

I'm afraid, we can't simply solve the issue on QEMU part, as we need this romsize at realize time. If we want to change the size on load of incoming migration, we'd have to reinitialize the device and memory layout.. This seems too difficult.

On the other hand, it seems correct to force management to specify ROM size to create. Same as RAM size.

Imagine, we always had the following behavior:

  - romsize is necessary parameter when we have rom (absence means no rom, same as romfile="")
  - romfile is ignored on incoming migration

this way, we would never have any migration problems.

So, the proposal is to at least support such behavior:

  - ignore romfile on incoming migration when romsize is specified.

Additionally we can deprecate missing romsize, when we have rom.


-- 
Best regards,
Vladimir