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

[Vladimir Sementsov-Ogievskiy]

On 28.04.23 11:30, Juan Quintela wrote:
> "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.

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

--
Best regards,
Vladimir