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

[Michael S. Tsirkin]

On Wed, May 03, 2023 at 02:39:15PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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?

I am not sure why it's ncessary and I don't much like extra file reads
just for the heck of it. If nothing else this attempt to check file
is readable is futile - it might not stay readable until the next
qemu run.

> -- 
> Best regards,
> Vladimir