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

[Juan Quintela <quintela@redhat.com>]

"Michael S. Tsirkin" <mst@redhat.com> 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.