Re: [PATCH V1 25/26] migration: fix mismatched GPAs during cpr-exec

[Fabiano Rosas <farosas@suse.de>]

Steve Sistare <steven.sistare@oracle.com> writes:

> For cpr-exec mode, ramblock_is_ignored is always true, and the address of
> each migrated memory region must match the address of the statically
> initialized region on the target.  However, for a PCI rom block, the 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-exec
> migration.
>
> To fix, unconditionally set the target's address to the source's address
> if the region does not have an address yet.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>

Just a detail below.

Reviewed-by: Fabiano Rosas <farosas@suse.de>

> ---
>  include/exec/memory.h | 12 ++++++++++++
>  migration/ram.c       | 15 +++++++++------
>  system/memory.c       | 10 ++++++++--
>  3 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index d337737..4f654b0 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -801,6 +801,7 @@ struct MemoryRegion {
>      bool unmergeable;
>      uint8_t dirty_log_mask;
>      bool is_iommu;
> +    bool has_addr;

This field is not used during memory access, maybe move it down below to
preserve the hole for future usage.

>      RAMBlock *ram_block;
>      Object *owner;
>      /* owner as TYPE_DEVICE. Used for re-entrancy checks in MR access hotpath */
> @@ -2402,6 +2403,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 add285b..7b8d7f6 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -4196,12 +4196,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 ca04a0e..3c72504 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -2665,7 +2665,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);
>  }
>  
> @@ -2745,10 +2745,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);
>      }
>  }