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

Commit Message

Vladimir Sementsov-Ogievskiy April 25, 2023, 4:14 p.m. UTC

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 <[e-mail]>
Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
---
 hw/pci/pci.c | 77 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 32 deletions(-)

Comments

Michael S. Tsirkin April 26, 2023, 4:43 a.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> ---
>  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?

> @@ -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 


>      pci_register_bar(pdev, PCI_ROM_SLOT, 0, &pdev->rom);
> -- 
> 2.34.1

Vladimir Sementsov-Ogievskiy April 26, 2023, 8 p.m. UTC | #2

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 <[e-mail]>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
>> ---
>>   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?

Juan Quintela April 28, 2023, 8:30 a.m. UTC | #3

"Michael S. Tsirkin" <[e-mail]> 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 a mess(TM).
And no easy way to fix it.  Everything has its problems.

Ok, I will elaborate.

We have source machine and destination machine.
Easy case, same version of qemu (or at least the same rom files).
The interesting ones is when the sizes are wrong.

Again this splits on two cases:
- target side is bigger
  not big deal, during migration we just don't use all the space.
- target side is smaller
  guess what, not easy way to get this working O:-)

We added some changes on the past for this, but I don't remember the
details.

If I understood his patch correctly, it set seems to try to fix this to
always do the right thing with respect to migration, i.e. using whatever
was on the source.  I think this is nice.

But we still have left out the big elephant on the ROM, what happens
when we reboot.

Right now, when we reboot we still use the rom files for the source.

And I think that in the case of reboot, if the ROM files have changed
(because there was an upgrade or we migrate to a host with a never
version, etc,) we should always do a powerdown + start to let qemu use
the new ROM files.

As far as I know, no management app does that, and especially as we move
to UEFI (i.e. more complex firmware with more posibilities for CVE's) I
think we should considerd this case.

>> @@ -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?

There is only a way to get into RUN_STATE_INMIGRATE, and that is that we
have started the guest with --incoming <something>.  So the check does
what it is intended.

Once told that, I have never been seen it used for this.
/me launches grep on source tree

At least the block layer and usb use it exactly for this.  So I will say
it is the correct way of doing it (or at least I can think of a better
way right now).

The grep also shows this:

static void rom_reset(void *unused)
{
    Rom *rom;

    QTAILQ_FOREACH(rom, &roms, next) {
        if (rom->fw_file) {
            continue;
        }
        /*
         * We don't need to fill in the RAM with ROM data because we'll fill
         * the data in during the next incoming migration in all cases.  Note
         * that some of those RAMs can actually be modified by the guest.
         */
        if (runstate_check(RUN_STATE_INMIGRATE)) {
            if (rom->data && rom->isrom) {
                /*
                 * Free it so that a rom_reset after migration doesn't
                 * overwrite a potentially modified 'rom'.
                 */
                rom_free_data(rom);
            }
            continue;
        }

It is not exactly the problem at hand, but it is related.  I am just
wondering if we can do something common.

Later, Juan.

Vladimir Sementsov-Ogievskiy April 28, 2023, 8:37 p.m. UTC | #4

On 28.04.23 11:30, Juan Quintela wrote:
> "Michael S. Tsirkin" <[e-mail]> 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 a mess(TM).
> And no easy way to fix it.  Everything has its problems.
> 
> Ok, I will elaborate.
> 
> We have source machine and destination machine.
> Easy case, same version of qemu (or at least the same rom files).
> The interesting ones is when the sizes are wrong.
> 
> Again this splits on two cases:
> - target side is bigger
>    not big deal, during migration we just don't use all the space.

But still doesn't work without my patch, as size mismatch -> migration fail.

Or, if you try to set romsize to match source, it fails on realize, when trying to load ROM from file and see that specified size is smaller. (again my patch fixes it).

> - target side is smaller
>    guess what, not easy way to get this working O:-)
> 
> We added some changes on the past for this, but I don't remember the
> details.

romsize parameter is added. Still, it can't help in all cases.

> 
> If I understood his patch correctly, it set seems to try to fix this to
> always do the right thing with respect to migration, i.e. using whatever
> was on the source.  I think this is nice.

Yes. In details:

- If you didn't use romsize before, all you need is to specify correct romsize on target, it will work, no matter which rom files you have on target
- If you already use romsize - just keep same parameters on target, it will work, no matter which rom files you have on target.

> 
> But we still have left out the big elephant on the ROM, what happens
> when we reboot.

Hmm. I now checked, seems nothing happen with these pci ROMs on reboot. They are not reloaded. pci_qdev_realize() is not called again on reboot.. Or what I miss?

> 
> Right now, when we reboot we still use the rom files for the source.
> 
> And I think that in the case of reboot, if the ROM files have changed
> (because there was an upgrade or we migrate to a host with a never
> version, etc,) we should always do a powerdown + start to let qemu use
> the new ROM files.
> 
> As far as I know, no management app does that, and especially as we move
> to UEFI (i.e. more complex firmware with more posibilities for CVE's) I
> think we should considerd this case.
> 
>>> @@ -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?
> 
> There is only a way to get into RUN_STATE_INMIGRATE, and that is that we
> have started the guest with --incoming <something>.  So the check does
> what it is intended.
> 
> Once told that, I have never been seen it used for this.
> /me launches grep on source tree
> 
> At least the block layer and usb use it exactly for this.  So I will say
> it is the correct way of doing it (or at least I can think of a better
> way right now).
> 
> The grep also shows this:
> 
> static void rom_reset(void *unused)
> {
>      Rom *rom;
> 
>      QTAILQ_FOREACH(rom, &roms, next) {
>          if (rom->fw_file) {
>              continue;
>          }
>          /*
>           * We don't need to fill in the RAM with ROM data because we'll fill
>           * the data in during the next incoming migration in all cases.  Note
>           * that some of those RAMs can actually be modified by the guest.
>           */
>          if (runstate_check(RUN_STATE_INMIGRATE)) {
>              if (rom->data && rom->isrom) {
>                  /*
>                   * Free it so that a rom_reset after migration doesn't
>                   * overwrite a potentially modified 'rom'.
>                   */
>                  rom_free_data(rom);
>              }
>              continue;
>          }
> 
> It is not exactly the problem at hand, but it is related.  I am just
> wondering if we can do something common.

Does these roms (of type Rom) from hw/core/loader.c relate to roms in hw/pci/pci.c, which are "MemoryRegion" ?

Michael S. Tsirkin May 2, 2023, 9:48 a.m. UTC | #5

On Wed, Apr 26, 2023 at 11:00:46PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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 <[e-mail]>
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> > > ---
> > >   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.

You can add it on source too while migration is in progress, no?

> > 
> > > @@ -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

Vladimir Sementsov-Ogievskiy May 2, 2023, 9:59 a.m. UTC | #6

On 02.05.23 12:48, Michael S. Tsirkin wrote:
> On Wed, Apr 26, 2023 at 11:00:46PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 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<[e-mail]>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy<[e-mail]>
>>>> ---
>>>>    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.
> You can add it on source too while migration is in progress, no?
> 

No, as INMIGRATE - means incoming migration, it's only on target

Juan Quintela May 2, 2023, 10:11 a.m. UTC | #7

"Michael S. Tsirkin" <[e-mail]> wrote:

>> > 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.
>
> You can add it on source too while migration is in progress, no?

DeviceState *qdev_device_add_from_qdict(const QDict *opts,
                                        bool from_json, Error **errp)
{
    ....
    if (!migration_is_idle()) {
        error_setg(errp, "device_add not allowed while migrating");
        return NULL;
    }

It should be similar for unplug.

We only support hotplug for some devices during migration, and we
shouldn't need any.

What I think he means is that you can add a device on the command line
on destination that don't exist on the source machine, and that will
confuse things.

In that case, I would say that the problem is that you are doing
something not supported.  You are expected that when you run migration
you use the same command line that on source, module whatever
hot[un]plug operations you have done before migration.

Anything else is not supported.
And for instance, if you are using libvirt, it will do the right thing.

Later, Juan.

Vladimir Sementsov-Ogievskiy May 2, 2023, 10:13 a.m. UTC | #8

On 02.05.23 13:11, Juan Quintela wrote:
> "Michael S. Tsirkin" <[e-mail]> wrote:
> 
>>>> 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.
>>
>> You can add it on source too while migration is in progress, no?
> 
> DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>                                          bool from_json, Error **errp)
> {
>      ....
>      if (!migration_is_idle()) {
>          error_setg(errp, "device_add not allowed while migrating");
>          return NULL;
>      }
> 
> It should be similar for unplug.
> 
> We only support hotplug for some devices during migration, and we
> shouldn't need any.
> 
> What I think he means is that you can add a device on the command line
> on destination that don't exist on the source machine, and that will
> confuse things.

Yes, that what I mean

> 
> In that case, I would say that the problem is that you are doing
> something not supported.  You are expected that when you run migration
> you use the same command line that on source, module whatever
> hot[un]plug operations you have done before migration.

Agree

> 
> Anything else is not supported.
> And for instance, if you are using libvirt, it will do the right thing.
> 
> Later, Juan.
> 

Thanks!

Michael S. Tsirkin May 2, 2023, 11:26 a.m. UTC | #9

On Tue, May 02, 2023 at 12:11:38PM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <[e-mail]> wrote:
> 
> >> > 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.
> >
> > You can add it on source too while migration is in progress, no?
> 
> DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>                                         bool from_json, Error **errp)
> {
>     ....
>     if (!migration_is_idle()) {
>         error_setg(errp, "device_add not allowed while migrating");
>         return NULL;
>     }
> 
> It should be similar for unplug.
> 
> We only support hotplug for some devices during migration, and we
> shouldn't need any.
> 
> What I think he means is that you can add a device on the command line
> on destination that don't exist on the source machine, and that will
> confuse things.
> 
> In that case, I would say that the problem is that you are doing
> something not supported.  You are expected that when you run migration
> you use the same command line that on source, module whatever
> hot[un]plug operations you have done before migration.
> 
> Anything else is not supported.
> And for instance, if you are using libvirt, it will do the right thing.
> 
> Later, Juan.

OK, so you ack this patch?

David Hildenbrand May 3, 2023, 9:20 a.m. UTC | #10

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 <[e-mail]>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> ---
>   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.

Vladimir Sementsov-Ogievskiy May 3, 2023, 9:50 a.m. UTC | #11

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 <[e-mail]>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
>> ---
>>   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?

Michael S. Tsirkin May 3, 2023, 10:05 a.m. UTC | #12

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 <[e-mail]>
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> > > ---
> > >   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.

Vladimir Sementsov-Ogievskiy May 3, 2023, 11:39 a.m. UTC | #13

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 <[e-mail]>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
>>>> ---
>>>>    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?

Juan Quintela May 9, 2023, 3:48 p.m. UTC | #14

"Michael S. Tsirkin" <[e-mail]> wrote:
> On Tue, May 02, 2023 at 12:11:38PM +0200, Juan Quintela wrote:
>> "Michael S. Tsirkin" <[e-mail]> wrote:
>> 
>> >> > 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.
>> >
>> > You can add it on source too while migration is in progress, no?
>> 
>> DeviceState *qdev_device_add_from_qdict(const QDict *opts,
>>                                         bool from_json, Error **errp)
>> {
>>     ....
>>     if (!migration_is_idle()) {
>>         error_setg(errp, "device_add not allowed while migrating");
>>         return NULL;
>>     }
>> 
>> It should be similar for unplug.
>> 
>> We only support hotplug for some devices during migration, and we
>> shouldn't need any.
>> 
>> What I think he means is that you can add a device on the command line
>> on destination that don't exist on the source machine, and that will
>> confuse things.
>> 
>> In that case, I would say that the problem is that you are doing
>> something not supported.  You are expected that when you run migration
>> you use the same command line that on source, module whatever
>> hot[un]plug operations you have done before migration.
>> 
>> Anything else is not supported.
>> And for instance, if you are using libvirt, it will do the right thing.
>> 
>> Later, Juan.
>
> OK, so you ack this patch?

Reviewed-by: Juan Quintela <[e-mail]>

It is ok, or should I do it at toplevel?

Later, Juan.

Michael S. Tsirkin May 9, 2023, 3:54 p.m. UTC | #15

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 <[e-mail]>
> > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> > > > > ---
> > > > >    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

David Hildenbrand May 9, 2023, 4:09 p.m. UTC | #16

On 09.05.23 17:54, Michael S. Tsirkin wrote:
> 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 <[e-mail]>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
>>>>>> ---
>>>>>>     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.

In any case, fine with me

Reviewed-by: David Hildenbrand <[e-mail]>

Vladimir Sementsov-Ogievskiy May 10, 2023, 9:38 a.m. UTC | #17

On 09.05.23 18:54, Michael S. Tsirkin wrote:
> 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 <[e-mail]>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
>>>>>> ---
>>>>>>     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.
> 

OK. I don't like extra file read too.

Patch

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);