qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2 3/3] pci: ROM preallocation for incoming migration
Date: Wed, 3 May 2023 14:39:15 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0

On 03.05.23 13:05, Michael S. Tsirkin wrote:
On Wed, May 03, 2023 at 12:50:09PM +0300, Vladimir Sementsov-Ogievskiy wrote:
On 03.05.23 12:20, David Hildenbrand wrote:
On 25.04.23 18:14, 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

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>
---
   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 a442f8fce1..e2cab622e4 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"
@@ -2293,10 +2294,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) {
           return;
       }
@@ -2329,32 +2336,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));
@@ -2365,15 +2375,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);


So, we'll now never load the file on the migration destination. But if 
"pdev->romsize == -1", we'll use the size of the file to size the region -- but 
not load it.


While that should work (because the ROM content will be migrated), at least I 
would find this easier to digest if we would have

bool use_file = !runstate_check(RUN_STATE_INMIGRATE) ||
          pdev->romsize == -1;

if (use_file) {
      path = qemu_find_file(QEMU_FILE_TYPE_BIOS, pdev->romfile);
      ...
}
...
memory_region_init_rom(&pdev->rom, OBJECT(pdev), name, pdev->romsize ...
...
if (use_file) {
      ptr = memory_region_get_ram_ptr(&pdev->rom);
      if (load_image_size(path, ptr, size) < 0) {
          ...
      }
}


If something about the file is weird (such that reading the size would work but 
loading would fail), it would fail consistently. Sure, we would load once more, 
but who really cares about that.

I wonder, though, if we then also want to handle the "pdev->romfile" checks 
differently, when we're not going to use the file at all ... would maybe make it more 
consistent. If we're not using the file, then ignore if no file is given/available ... 
because we don't need it. The romsize is sufficient in that case on the migration 
destination.


Maybe, we should just deprecate unspecified romsize? And make it necessary in 
future?

That would be quite annoying. The whole problem arises because
downstream decided to override QEMU provided ROM
on the command line. Users that don't do this,
are ok and I do not want to make things harder for them.


OK. Are you agree with Devid's advice to still load file, even on incoming 
migration, when romsize argument is absent?

--
Best regards,
Vladimir




reply via email to

[Prev in Thread] Current Thread [Next in Thread]