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

["Michael S. Tsirkin" <mst@redhat.com>]

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