[PATCH v3 0/3] ROM migration

[PATCH v3 0/3] ROM migration

[Vladimir Sementsov-Ogievskiy]

Hi all!

v3:
01: follow small refactoring advice by David + r-b
02: r-b by Devid
03: add error message example to commit message
    r-bs by Devid and Juan

v2: simply ignore romfile on incoming migration when romsize is
specified.

Here I suggest a 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 | 100 +++++++++++++++++++++++++++------------------------
 1 file changed, 53 insertions(+), 47 deletions(-)

-- 
2.34.1

[PATCH v3 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>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 hw/pci/pci.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 8a87ccc8b0..e26e2a7e65 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2312,10 +2312,9 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
     char name[32];
     const VMStateDescription *vmsd;
 
-    if (!pdev->romfile)
-        return;
-    if (strlen(pdev->romfile) == 0)
+    if (!pdev->romfile || !strlen(pdev->romfile)) {
         return;
+    }
 
     if (!pdev->rom_bar) {
         /*
@@ -2364,7 +2363,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;
@@ -2374,14 +2374,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

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

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> 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>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH v3 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>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 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 e26e2a7e65..3a0107758c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2307,7 +2307,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;
@@ -2349,16 +2349,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) {
@@ -2366,7 +2363,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 {
@@ -2384,10 +2380,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

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

[Juan Quintela]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

nice.

[PATCH v3 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, like this:

    Size mismatch: 0000:00:03.0/virtio-net-pci.rom: 0x40000 != 0x80000: Invalid argument

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

Let's just ignore ROM file when romsize is specified and we are in
incoming migration state. In other words, we need only to preallocate
ROM of specified size, local ROM file is unrelated.

This way:

If romsize was specified on source, we just use same commandline as on
source, and migration will work independently of local ROM files on
target.

If romsize was not specified on source (and we have mismatching local
ROM file on target host), we have to specify romsize on target to match
source romsize. romfile parameter may be kept same as on source or may
be dropped, the file is not loaded anyway.

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

Note: when we don't have romsize parameter on source command line and
need it for target, it may be calculated as aligned up to power of two
size of ROM file on source (if we know, which file is it) or,
alternatively it may be retrieved from source QEMU by QMP qom-get
command, like

  { "execute": "qom-get",
    "arguments": {
      "path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
      "property": "size" } }

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Reviewed-by: David Hildenbrand <david@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 hw/pci/pci.c | 77 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 32 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 3a0107758c..0f0c83c02f 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -36,6 +36,7 @@
 #include "migration/vmstate.h"
 #include "net/net.h"
 #include "sysemu/numa.h"
+#include "sysemu/runstate.h"
 #include "sysemu/sysemu.h"
 #include "hw/loader.h"
 #include "qemu/error-report.h"
@@ -2308,10 +2309,16 @@ 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;
 
+    /*
+     * In case of incoming migration ROM will come with migration stream, no
+     * reason to load the file.  Neither we want to fail if local ROM file
+     * mismatches with specified romsize.
+     */
+    bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
+
     if (!pdev->romfile || !strlen(pdev->romfile)) {
         return;
     }
@@ -2341,32 +2348,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 || pdev->romsize == -1) {
+        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));
@@ -2377,15 +2387,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 v3 3/3] pci: ROM preallocation for incoming migration

[Michael S. Tsirkin]

On Mon, May 15, 2023 at 03:52:29PM +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, like this:
> 
>     Size mismatch: 0000:00:03.0/virtio-net-pci.rom: 0x40000 != 0x80000: Invalid argument
> 
> 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
> 
> Let's just ignore ROM file when romsize is specified and we are in
> incoming migration state. In other words, we need only to preallocate
> ROM of specified size, local ROM file is unrelated.
> 
> This way:
> 
> If romsize was specified on source, we just use same commandline as on
> source, and migration will work independently of local ROM files on
> target.
> 
> If romsize was not specified on source (and we have mismatching local
> ROM file on target host), we have to specify romsize on target to match
> source romsize. romfile parameter may be kept same as on source or may
> be dropped, the file is not loaded anyway.
> 
> As a bonus we avoid extra reading from ROM file on target.
> 
> Note: when we don't have romsize parameter on source command line and
> need it for target, it may be calculated as aligned up to power of two
> size of ROM file on source (if we know, which file is it) or,
> alternatively it may be retrieved from source QEMU by QMP qom-get
> command, like
> 
>   { "execute": "qom-get",
>     "arguments": {
>       "path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
>       "property": "size" } }
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> Reviewed-by: Juan Quintela <quintela@redhat.com>


Breaks build here:

In function ‘pci_add_option_rom’,
    inlined from ‘pci_qdev_realize’ at ../hw/pci/pci.c:2155:5:
../hw/pci/pci.c:2395:13: error: ‘size’ may be used uninitialized [-Werror=maybe-uninitialized]
 2395 |         if (load_image_size(path, ptr, size) < 0) {
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../hw/pci/pci.c: In function ‘pci_qdev_realize’:
../hw/pci/pci.c:2312:13: note: ‘size’ was declared here
 2312 |     int64_t size;
      |             ^~~~



> ---
>  hw/pci/pci.c | 77 ++++++++++++++++++++++++++++++----------------------
>  1 file changed, 45 insertions(+), 32 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 3a0107758c..0f0c83c02f 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -36,6 +36,7 @@
>  #include "migration/vmstate.h"
>  #include "net/net.h"
>  #include "sysemu/numa.h"
> +#include "sysemu/runstate.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/loader.h"
>  #include "qemu/error-report.h"
> @@ -2308,10 +2309,16 @@ 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;
>  
> +    /*
> +     * In case of incoming migration ROM will come with migration stream, no
> +     * reason to load the file.  Neither we want to fail if local ROM file
> +     * mismatches with specified romsize.
> +     */
> +    bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
> +
>      if (!pdev->romfile || !strlen(pdev->romfile)) {
>          return;
>      }
> @@ -2341,32 +2348,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 || pdev->romsize == -1) {
> +        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));
> @@ -2377,15 +2387,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 v3 3/3] pci: ROM preallocation for incoming migration

[Vladimir Sementsov-Ogievskiy]

On 19.05.23 08:34, Michael S. Tsirkin wrote:
> On Mon, May 15, 2023 at 03:52:29PM +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, like this:
>>
>>      Size mismatch: 0000:00:03.0/virtio-net-pci.rom: 0x40000 != 0x80000: Invalid argument
>>
>> 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
>>
>> Let's just ignore ROM file when romsize is specified and we are in
>> incoming migration state. In other words, we need only to preallocate
>> ROM of specified size, local ROM file is unrelated.
>>
>> This way:
>>
>> If romsize was specified on source, we just use same commandline as on
>> source, and migration will work independently of local ROM files on
>> target.
>>
>> If romsize was not specified on source (and we have mismatching local
>> ROM file on target host), we have to specify romsize on target to match
>> source romsize. romfile parameter may be kept same as on source or may
>> be dropped, the file is not loaded anyway.
>>
>> As a bonus we avoid extra reading from ROM file on target.
>>
>> Note: when we don't have romsize parameter on source command line and
>> need it for target, it may be calculated as aligned up to power of two
>> size of ROM file on source (if we know, which file is it) or,
>> alternatively it may be retrieved from source QEMU by QMP qom-get
>> command, like
>>
>>    { "execute": "qom-get",
>>      "arguments": {
>>        "path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
>>        "property": "size" } }
>>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> 
> Breaks build here:
> 
> In function ‘pci_add_option_rom’,
>      inlined from ‘pci_qdev_realize’ at ../hw/pci/pci.c:2155:5:
> ../hw/pci/pci.c:2395:13: error: ‘size’ may be used uninitialized [-Werror=maybe-uninitialized]
>   2395 |         if (load_image_size(path, ptr, size) < 0) {
>        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../hw/pci/pci.c: In function ‘pci_qdev_realize’:
> ../hw/pci/pci.c:2312:13: note: ‘size’ was declared here
>   2312 |     int64_t size;
>        |             ^~~~
> 
> 

Hmm, but works for me. Anyway that's obviously false-positive, if we are here, size is initialized in previous block if (load_file || ..).

So, may be add simply this:

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 0f0c83c02f..075c998284 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -2307,7 +2307,7 @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size)
  static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
                                 Error **errp)
  {
-    int64_t size;
+    int64_t size = 0;  /* fix "uninitialized" false-positive */
      g_autofree char *path = NULL;
      char name[32];
      const VMStateDescription *vmsd;


> 
>> ---
>>   hw/pci/pci.c | 77 ++++++++++++++++++++++++++++++----------------------
>>   1 file changed, 45 insertions(+), 32 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 3a0107758c..0f0c83c02f 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -36,6 +36,7 @@
>>   #include "migration/vmstate.h"
>>   #include "net/net.h"
>>   #include "sysemu/numa.h"
>> +#include "sysemu/runstate.h"
>>   #include "sysemu/sysemu.h"
>>   #include "hw/loader.h"
>>   #include "qemu/error-report.h"
>> @@ -2308,10 +2309,16 @@ 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;
>>   
>> +    /*
>> +     * In case of incoming migration ROM will come with migration stream, no
>> +     * reason to load the file.  Neither we want to fail if local ROM file
>> +     * mismatches with specified romsize.
>> +     */
>> +    bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
>> +
>>       if (!pdev->romfile || !strlen(pdev->romfile)) {
>>           return;
>>       }
>> @@ -2341,32 +2348,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 || pdev->romsize == -1) {
>> +        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));
>> @@ -2377,15 +2387,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
> 

-- 
Best regards,
Vladimir

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

[Michael S. Tsirkin]

On Mon, May 22, 2023 at 11:44:32AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 19.05.23 08:34, Michael S. Tsirkin wrote:
> > On Mon, May 15, 2023 at 03:52:29PM +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, like this:
> > > 
> > >      Size mismatch: 0000:00:03.0/virtio-net-pci.rom: 0x40000 != 0x80000: Invalid argument
> > > 
> > > 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
> > > 
> > > Let's just ignore ROM file when romsize is specified and we are in
> > > incoming migration state. In other words, we need only to preallocate
> > > ROM of specified size, local ROM file is unrelated.
> > > 
> > > This way:
> > > 
> > > If romsize was specified on source, we just use same commandline as on
> > > source, and migration will work independently of local ROM files on
> > > target.
> > > 
> > > If romsize was not specified on source (and we have mismatching local
> > > ROM file on target host), we have to specify romsize on target to match
> > > source romsize. romfile parameter may be kept same as on source or may
> > > be dropped, the file is not loaded anyway.
> > > 
> > > As a bonus we avoid extra reading from ROM file on target.
> > > 
> > > Note: when we don't have romsize parameter on source command line and
> > > need it for target, it may be calculated as aligned up to power of two
> > > size of ROM file on source (if we know, which file is it) or,
> > > alternatively it may be retrieved from source QEMU by QMP qom-get
> > > command, like
> > > 
> > >    { "execute": "qom-get",
> > >      "arguments": {
> > >        "path": "/machine/peripheral/CARD_ID/virtio-net-pci.rom[0]",
> > >        "property": "size" } }
> > > 
> > > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > Reviewed-by: David Hildenbrand <david@redhat.com>
> > > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > 
> > 
> > Breaks build here:
> > 
> > In function ‘pci_add_option_rom’,
> >      inlined from ‘pci_qdev_realize’ at ../hw/pci/pci.c:2155:5:
> > ../hw/pci/pci.c:2395:13: error: ‘size’ may be used uninitialized [-Werror=maybe-uninitialized]
> >   2395 |         if (load_image_size(path, ptr, size) < 0) {
> >        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ../hw/pci/pci.c: In function ‘pci_qdev_realize’:
> > ../hw/pci/pci.c:2312:13: note: ‘size’ was declared here
> >   2312 |     int64_t size;
> >        |             ^~~~
> > 
> > 
> 
> Hmm, but works for me. Anyway that's obviously false-positive, if we are here, size is initialized in previous block if (load_file || ..).
> 
> So, may be add simply this:
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 0f0c83c02f..075c998284 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -2307,7 +2307,7 @@ static void pci_patch_ids(PCIDevice *pdev, uint8_t *ptr, uint32_t size)
>  static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom,
>                                 Error **errp)
>  {
> -    int64_t size;
> +    int64_t size = 0;  /* fix "uninitialized" false-positive */

I'd even drop the comment, we will not remember to remove it.
just mention in commit log.


OK, pls repost with this fix. Minor so include acks posted so far. Thanks!

>      g_autofree char *path = NULL;
>      char name[32];
>      const VMStateDescription *vmsd;
> 
> 
> > 
> > > ---
> > >   hw/pci/pci.c | 77 ++++++++++++++++++++++++++++++----------------------
> > >   1 file changed, 45 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 3a0107758c..0f0c83c02f 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -36,6 +36,7 @@
> > >   #include "migration/vmstate.h"
> > >   #include "net/net.h"
> > >   #include "sysemu/numa.h"
> > > +#include "sysemu/runstate.h"
> > >   #include "sysemu/sysemu.h"
> > >   #include "hw/loader.h"
> > >   #include "qemu/error-report.h"
> > > @@ -2308,10 +2309,16 @@ 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;
> > > +    /*
> > > +     * In case of incoming migration ROM will come with migration stream, no
> > > +     * reason to load the file.  Neither we want to fail if local ROM file
> > > +     * mismatches with specified romsize.
> > > +     */
> > > +    bool load_file = !runstate_check(RUN_STATE_INMIGRATE);
> > > +
> > >       if (!pdev->romfile || !strlen(pdev->romfile)) {
> > >           return;
> > >       }
> > > @@ -2341,32 +2348,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 || pdev->romsize == -1) {
> > > +        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));
> > > @@ -2377,15 +2387,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
> > 
> 
> -- 
> Best regards,
> Vladimir