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

[Vladimir Sementsov-Ogievskiy]

On 26.04.23 07:43, Michael S. Tsirkin wrote:
> On Tue, Apr 25, 2023 at 07:14:34PM +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.
>
> let's mention an example error message:
>   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
>
> Something I'd like to clarify is that the comment applies to uses where
> users/distributions supply their own ROM file.  And lots of
> users/distributions seem to have already painted themselves into a
> corner by supplying a mix of ROM files of unmatching sizes -
> basically they don't understand the detail of live migration,
> ROM size interaction with it and with memory layout, etc -
> as a very small number of people does.
> For example, ubuntu doubled ROM file size by padding their ROMs
> with 0xffffffff at some point, breaking migration for all existing machine
> types.
>
> just a web search for
>   Size mismatch: 0000:00:03.0/virtio-net-pci.rom: 0x40000 != 0x80000: Invalid 
> argument
>
> will turn up a bunch of confused distros and users.
>
>
> > 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;
> >       }
>
> CC pbonzini,dgilbert,quintela,armbru : guys, is poking at runstate_check like
> this the right way to figure out we are not going to use the
> device locally before incoming migration will overwrite ROM contents?

RUN_STATE_INMIGRATE is set in the only one place in qemu_init() when we parse 
cmdline option -incoming. VM is not running for sure. And starting the VM comes 
with changing the state. So it's OK.

The possible problem, if we add netcard on target which we didn't have on 
source. I now checked, this works.. But that doesn't seem correct to add device 
that was not present on source - how would it work - it's not guaranteed anyway.

> > @@ -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);
> > +        }
> >       }
>
> it kind of feels weird to ignore

What do you mean we should not ignore?



--
Best regards,
Vladimir