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? Thanks, -- Peter Xu
