Re: [PATCH V2 06/11] migration: fix mismatched GPAs during cpr

[Peter Xu <peterx@redhat.com>]

On Fri, Aug 16, 2024 at 01:10:02PM -0400, Steven Sistare wrote:
> On 8/16/2024 10:43 AM, Peter Xu wrote:
> > On Thu, Aug 15, 2024 at 04:54:58PM -0400, Steven Sistare wrote:
> > > On 8/13/2024 4:43 PM, Peter Xu wrote:
> > > > On Wed, Aug 07, 2024 at 05:04:26PM -0400, Steven Sistare wrote:
> > > > > On 7/19/2024 12:28 PM, Peter Xu wrote:
> > > > > > On Sun, Jun 30, 2024 at 12:40:29PM -0700, Steve Sistare wrote:
> > > > > > > For new cpr modes, ramblock_is_ignored will always be true, because the
> > > > > > > memory is preserved in place rather than copied.  However, for an ignored
> > > > > > > block, parse_ramblock currently requires that the received address of the
> > > > > > > block must match the address of the statically initialized region on the
> > > > > > > target.  This fails for a PCI rom block, because the memory region address
> > > > > > > is set when the guest writes to a BAR on the source, which does not occur
> > > > > > > on the target, causing a "Mismatched GPAs" error during cpr migration.
> > > > > > 
> > > > > > Is this a common fix with/without cpr mode?
> > > > > > 
> > > > > > It looks to me mr->addr (for these ROMs) should only be set in PCI config
> > > > > > region updates as you mentioned.  But then I didn't figure out when they're
> > > > > > updated on dest in live migration: the ramblock info was sent at the
> > > > > > beginning of migration, so it doesn't even have PCI config space migrated;
> > > > > > I thought the real mr->addr should be in there.
> > > > > > 
> > > > > > I also failed to understand yet on why the mr->addr check needs to be done
> > > > > > by ignore-shared only.  Some explanation would be greatly helpful around
> > > > > > this area..
> > > > > 
> > > > > The error_report does not bite for normal migration because migrate_ram_is_ignored()
> > > > > is false for the problematic blocks, so the block->mr->addr check is not
> > > > > performed.  However, mr->addr is never fixed up in this case, which is a
> > > > > quiet potential bug, and this patch fixes that with the "has_addr" check.
> > > > > 
> > > > > For cpr-exec, migrate_ram_is_ignored() is true for all blocks,
> > > > > because we do not copy the contents over the migration stream, we preserve the
> > > > > memory in place.  So we fall into the block->mr->addr sanity check and fail
> > > > > with the original code.
> > > > 
> > > > OK I get your point now.  However this doesn't look right, instead I start
> > > > to question why we need to send mr->addr at all..
> > > > 
> > > > As I said previously, AFAIU mr->addr should only be updated when there's
> > > > some PCI config space updates so that it moves the MR around in the address
> > > > space based on how guest drivers / BIOS (?) set things up.  Now after these
> > > > days not looking, and just started to look at this again, I think the only
> > > > sane place to do this update is during a post_load().
> > > > 
> > > > And if we start to check some of the memory_region_set_address() users,
> > > > that's exactly what happened..
> > > > 
> > > >     - ich9_pm_iospace_update(), update addr for ICH9LPCPMRegs.io, where
> > > >       ich9_pm_post_load() also invokes it.
> > > > 
> > > >     - pm_io_space_update(), updates PIIX4PMState.io, where
> > > >       vmstate_acpi_post_load() also invokes it.
> > > > 
> > > > I stopped here just looking at the initial two users, it looks all sane to
> > > > me that it only got updated there, because the update requires pci config
> > > > space being migrated first.
> > > > 
> > > > IOW, I don't think having mismatched mr->addr is wrong at this stage.
> > > > Instead, I don't see why we should send mr->addr at all in this case during
> > > > as early as SETUP, and I don't see anything justifies the mr->addr needs to
> > > > be verified in parse_ramblock() since ignore-shared introduced by Yury in
> > > > commit fbd162e629aaf8 in 2019.
> > > > 
> > > > We can't drop mr->addr now when it's on-wire, but I think we should drop
> > > > the error report and addr check, instead of this patch.
> > > 
> > > As it turns out, my test case triggers this bug because it sets x-ignore-shared,
> > > but x-ignore-shared is not needed for cpr-exec, because migrate_ram_is_ignored
> > > is true for all blocks when mode==cpr-exec.  So, the best fix for the GPAs bug
> > > for me is to stop setting x-ignore-shared.  I will drop this patch.
> > > 
> > > I agree that post_load is the right place to restore mr->addr, and I don't
> > > understand why commit fbd162e629aaf8 added the error report, but I am going
> > > to leave it as is.
> > 
> > Ah, I didn't notice that cpr special cased migrate_ram_is_ignored()..
> > 
> > Shall we stick with the old check, but always require cpr to rely on
> > ignore-shared?
> > 
> > Then we replace this patch with removing the error_report, probably
> > together with not caring about whatever is received at all.. would that be
> > cleaner?
> 
> migrate_ram_is_ignored() is called in many places and must return true for
> cpr-exec/cpr-transfer, independently of migrate_ignore_shared.  That logic
> must remain as is.

Is this because cpr can fail some ramblock in qemu_ram_is_named_file()?

It's not obvious in this case, maybe some re-strcture would be nice.  Would
something like this look nicer and easier to understand?

===8<===
diff --git a/migration/ram.c b/migration/ram.c
index 1e1e05e859..ace635b167 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -214,14 +214,29 @@ static bool postcopy_preempt_active(void)
     return migrate_postcopy_preempt() && migration_in_postcopy();
 }
 
-bool migrate_ram_is_ignored(RAMBlock *block)
+/* Whether the destination QEMU can share the access on this ramblock? */
+bool migrate_ram_is_shared(RAMBlock *block)
 {
     MigMode mode = migrate_mode();
+
+    /* Private ram is never share-able */
+    if (!qemu_ram_is_shared(block)) {
+        return false;
+    }
+
+    /* Named file ram is always assumed to be share-able */
+    if (qemu_ram_is_named_file(block)) {
+        return true;
+    }
+
+    /* It's a private fd, only cpr mode can share it (by sharing fd) */
+    return (mode == MIG_MODE_CPR_EXEC) || (mode == MIG_MODE_CPR_TRANSFER);
+}
+
+bool migrate_ram_is_ignored(RAMBlock *block)
+{
     return !qemu_ram_is_migratable(block) ||
-           mode == MIG_MODE_CPR_EXEC ||
-           mode == MIG_MODE_CPR_TRANSFER ||
-           (migrate_ignore_shared() && qemu_ram_is_shared(block)
-                                    && qemu_ram_is_named_file(block));
+           (migrate_ignore_shared() && migrate_ram_is_shared(block));
 }
===8<===

Please feel free to squash this to your patch in whatever way if it looks
reasonable to you.

> 
> The cleanest change is no change, just dropping this patch.  I was just confused
> when I set x-ignore-shared for the test.
> 
> However, if an unsuspecting user sets x-ignore-shared, it will trigger this error,
> so perhaps I should delete the error_report.

Yes, feel free to send that as a separate patch if you want, since we
digged it this far it'll be nice we fix it even if it's not relevant now.

Thanks,

-- 
Peter Xu