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

Commit Message

Steven Sistare June 30, 2024, 7:40 p.m. UTC

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.

To fix, unconditionally set the target's address to the source's address
if the target region does not have an address yet.

Signed-off-by: Steve Sistare <[e-mail]>
Reviewed-by: Fabiano Rosas <[e-mail]>
---
 include/exec/memory.h | 12 ++++++++++++
 migration/ram.c       | 15 +++++++++------
 system/memory.c       | 10 ++++++++--
 3 files changed, 29 insertions(+), 8 deletions(-)

Comments

Peter Xu July 19, 2024, 4:28 p.m. UTC | #1

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

Thanks,

Steven Sistare July 20, 2024, 9:28 p.m. UTC | #2

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 does not occur during normal migration.

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

I will continue this thread later and explain more fully.

- Steve

Steven Sistare Aug. 7, 2024, 9:04 p.m. UTC | #3

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.

I will add this to the commit message.

- Steve

Peter Xu Aug. 13, 2024, 8:43 p.m. UTC | #4

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.

Thanks,

Steven Sistare Aug. 15, 2024, 8:54 p.m. UTC | #5

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.

Thanks for reviewing this.

- Steve

Peter Xu Aug. 16, 2024, 2:43 p.m. UTC | #6

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,

Steven Sistare Aug. 16, 2024, 5:10 p.m. UTC | #7

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.

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.

- Steve

Peter Xu Aug. 21, 2024, 4:57 p.m. UTC | #8

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,

Patch

diff --git a/include/exec/memory.h b/include/exec/memory.h
index c26ede3..227169e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -811,6 +811,7 @@  struct MemoryRegion {
     bool ram_device;
     bool enabled;
     bool warning_printed; /* For reservations */
+    bool has_addr;
     uint8_t vga_logging_count;
     MemoryRegion *alias;
     hwaddr alias_offset;
@@ -2408,6 +2409,17 @@  void memory_region_set_enabled(MemoryRegion *mr, bool enabled);
 void memory_region_set_address(MemoryRegion *mr, hwaddr addr);
 
 /*
+ * memory_region_set_address_only: set the address of a region.
+ *
+ * Same as memory_region_set_address, but without causing transaction side
+ * effects.
+ *
+ * @mr: the region to be updated
+ * @addr: new address, relative to container region
+ */
+void memory_region_set_address_only(MemoryRegion *mr, hwaddr addr);
+
+/*
  * memory_region_set_size: dynamically update the size of a region.
  *
  * Dynamically updates the size of a region.
diff --git a/migration/ram.c b/migration/ram.c
index edec1a2..eaf3151 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4059,12 +4059,15 @@  static int parse_ramblock(QEMUFile *f, RAMBlock *block, ram_addr_t length)
     }
     if (migrate_ignore_shared()) {
         hwaddr addr = qemu_get_be64(f);
-        if (migrate_ram_is_ignored(block) &&
-            block->mr->addr != addr) {
-            error_report("Mismatched GPAs for block %s "
-                         "%" PRId64 "!= %" PRId64, block->idstr,
-                         (uint64_t)addr, (uint64_t)block->mr->addr);
-            return -EINVAL;
+        if (migrate_ram_is_ignored(block)) {
+            if (!block->mr->has_addr) {
+                memory_region_set_address_only(block->mr, addr);
+            } else if (block->mr->addr != addr) {
+                error_report("Mismatched GPAs for block %s "
+                             "%" PRId64 "!= %" PRId64, block->idstr,
+                             (uint64_t)addr, (uint64_t)block->mr->addr);
+                return -EINVAL;
+            }
         }
     }
     ret = rdma_block_notification_handle(f, block->idstr);
diff --git a/system/memory.c b/system/memory.c
index 28a837d..b7548bf 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -2655,7 +2655,7 @@  static void memory_region_add_subregion_common(MemoryRegion *mr,
     for (alias = subregion->alias; alias; alias = alias->alias) {
         alias->mapped_via_alias++;
     }
-    subregion->addr = offset;
+    memory_region_set_address_only(subregion, offset);
     memory_region_update_container_subregions(subregion);
 }
 
@@ -2735,10 +2735,16 @@  static void memory_region_readd_subregion(MemoryRegion *mr)
     }
 }
 
+void memory_region_set_address_only(MemoryRegion *mr, hwaddr addr)
+{
+    mr->addr = addr;
+    mr->has_addr = true;
+}
+
 void memory_region_set_address(MemoryRegion *mr, hwaddr addr)
 {
     if (addr != mr->addr) {
-        mr->addr = addr;
+        memory_region_set_address_only(mr, addr);
         memory_region_readd_subregion(mr);
     }
 }