Re: [PULL 2/8] migration: ram block cpr blockers

[Fabiano Rosas <farosas@suse.de>]

Michael Roth <michael.roth@amd.com> writes:

> Quoting Tom Lendacky (2025-03-26 14:21:31)
>> On 3/26/25 13:46, Tom Lendacky wrote:
>> > On 3/7/25 12:15, Fabiano Rosas wrote:
>> >> From: Steve Sistare <steven.sistare@oracle.com>
>> >>
>> >> Unlike cpr-reboot mode, cpr-transfer mode cannot save volatile ram blocks
>> >> in the migration stream file and recreate them later, because the physical
>> >> memory for the blocks is pinned and registered for vfio.  Add a blocker
>> >> for volatile ram blocks.
>> >>
>> >> Also add a blocker for RAM_GUEST_MEMFD.  Preserving guest_memfd may be
>> >> sufficient for CPR, but it has not been tested yet.
>> >>
>> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> >> Reviewed-by: Fabiano Rosas <farosas@suse.de>
>> >> Reviewed-by: Peter Xu <peterx@redhat.com>
>> >> Reviewed-by: David Hildenbrand <david@redhat.com>
>> >> Message-ID: <1740667681-257312-1-git-send-email-steven.sistare@oracle.com>
>> >> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> >> ---
>> >>  include/exec/memory.h   |  3 ++
>> >>  include/exec/ramblock.h |  1 +
>> >>  migration/savevm.c      |  2 ++
>> >>  system/physmem.c        | 66 +++++++++++++++++++++++++++++++++++++++++
>> >>  4 files changed, 72 insertions(+)
>> > 
>> > This patch breaks booting an SNP guest as it triggers the following
>> > assert:
>> > 
>> > qemu-system-x86_64: ../util/error.c:68: error_setv: Assertion `*errp == NULL' failed.
>> > 

Usually this means the error has already been set previously, which is
not allowed.

>> > I tracked it to the err_setg() call in ram_block_add_cpr_blocker().
>> > It looks like the error message is unable to be printed because
>> > rb->cpr_blocker is NULL.
>> > 
>> > Adding aux-ram-share=on to the -machine object gets me past the error and
>> > therefore the assertion, but isn't that an incompatible change to how an
>> > SNP guest has to be started?
>> 
>> If I update the err_setg() call to use the errp parameter that is passed
>> into ram_block_add_cpr_blocker(), I get the following message and then
>> the guest launch terminates:
>> 

The usage at ram_block_add_cpr_blocker() is correct, the cpr_blocker
gets initialized and registered as a migration blocker. The errp only
becomes relevant later when migration starts and the error condition is
met.

>> qemu-system-x86_64: Memory region pc.bios is not compatible with CPR.
>> share=on is required for memory-backend objects, and aux-ram-share=on is
>> required.

Since errp is an &error_fatal, it causes QEMU to exit, so this^ error
message is bogus.

>> 
>> The qemu parameters I used prior to this patch that allowed an SNP guest
>> to launch were:
>> 
>> -machine type=q35,confidential-guest-support=sev0,memory-backend=ram1
>> -object memory-backend-memfd,id=ram1,size=16G,share=true,prealloc=false
>> 
>> With these parameters after this patch, the launch fails.
>
> I think it might be failing because the caller of
> ram_block_add_cpr_blocker() is passing in &error_abort, but if the
> error_setg() is call on a properly initialized cpr_blocker value then
> SNP is still able to boot for me.
> I'm not sure where the best spot is
> to initialize cpr_blocker, it probably needs to be done before either
> ram_block_add_cpr_blocker() or ram_block_del_cpr_blocker() are callable,
> but the following avoids the reported crash at least:
>
> diff --git a/system/physmem.c b/system/physmem.c
> index 44dd129662..bff0fdcaac 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -4176,6 +4176,7 @@ void ram_block_add_cpr_blocker(RAMBlock *rb, Error **errp)
>          return;
>      }
>
> +    rb->cpr_blocker = NULL;

Could it be the cpr_blocker already got set at ram_block_add() in the
RAM_GUEST_MEMFD path?

>      error_setg(&rb->cpr_blocker,
>                 "Memory region %s is not compatible with CPR. share=on is "
>                 "required for memory-backend objects, and aux-ram-share=on is "
>
> -Mike
>
>> 
>> Thanks,
>> Tom
>> 
>> > 
>> > Thanks,
>> > Tom
>> > 
>> >>
>> >> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> >> index 78c4e0aec8..d09af58c97 100644
>> >> --- a/include/exec/memory.h
>> >> +++ b/include/exec/memory.h
>> >> @@ -3203,6 +3203,9 @@ bool ram_block_discard_is_disabled(void);
>> >>   */
>> >>  bool ram_block_discard_is_required(void);
>> >>  
>> >> +void ram_block_add_cpr_blocker(RAMBlock *rb, Error **errp);
>> >> +void ram_block_del_cpr_blocker(RAMBlock *rb);
>> >> +
>> >>  #endif
>> >>  
>> >>  #endif
>> >> diff --git a/include/exec/ramblock.h b/include/exec/ramblock.h
>> >> index 0babd105c0..64484cd821 100644
>> >> --- a/include/exec/ramblock.h
>> >> +++ b/include/exec/ramblock.h
>> >> @@ -39,6 +39,7 @@ struct RAMBlock {
>> >>      /* RCU-enabled, writes protected by the ramlist lock */
>> >>      QLIST_ENTRY(RAMBlock) next;
>> >>      QLIST_HEAD(, RAMBlockNotifier) ramblock_notifiers;
>> >> +    Error *cpr_blocker;
>> >>      int fd;
>> >>      uint64_t fd_offset;
>> >>      int guest_memfd;
>> >> diff --git a/migration/savevm.c b/migration/savevm.c
>> >> index 5c4fdfd95e..ce158c3512 100644
>> >> --- a/migration/savevm.c
>> >> +++ b/migration/savevm.c
>> >> @@ -3514,12 +3514,14 @@ void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
>> >>      qemu_ram_set_idstr(mr->ram_block,
>> >>                         memory_region_name(mr), dev);
>> >>      qemu_ram_set_migratable(mr->ram_block);
>> >> +    ram_block_add_cpr_blocker(mr->ram_block, &error_fatal);
>> >>  }
>> >>  
>> >>  void vmstate_unregister_ram(MemoryRegion *mr, DeviceState *dev)
>> >>  {
>> >>      qemu_ram_unset_idstr(mr->ram_block);
>> >>      qemu_ram_unset_migratable(mr->ram_block);
>> >> +    ram_block_del_cpr_blocker(mr->ram_block);
>> >>  }
>> >>  
>> >>  void vmstate_register_ram_global(MemoryRegion *mr)
>> >> diff --git a/system/physmem.c b/system/physmem.c
>> >> index 8c1736f84e..445981a1b4 100644
>> >> --- a/system/physmem.c
>> >> +++ b/system/physmem.c
>> >> @@ -70,7 +70,10 @@
>> >>  
>> >>  #include "qemu/pmem.h"
>> >>  
>> >> +#include "qapi/qapi-types-migration.h"
>> >> +#include "migration/blocker.h"
>> >>  #include "migration/cpr.h"
>> >> +#include "migration/options.h"
>> >>  #include "migration/vmstate.h"
>> >>  
>> >>  #include "qemu/range.h"
>> >> @@ -1903,6 +1906,14 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
>> >>              qemu_mutex_unlock_ramlist();
>> >>              goto out_free;
>> >>          }
>> >> +
>> >> +        error_setg(&new_block->cpr_blocker,
>> >> +                   "Memory region %s uses guest_memfd, "
>> >> +                   "which is not supported with CPR.",
>> >> +                   memory_region_name(new_block->mr));
>> >> +        migrate_add_blocker_modes(&new_block->cpr_blocker, errp,
>> >> +                                  MIG_MODE_CPR_TRANSFER,
>> >> +                                  -1);
>> >>      }
>> >>  
>> >>      ram_size = (new_block->offset + new_block->max_length) >> TARGET_PAGE_BITS;
>> >> @@ -4094,3 +4105,58 @@ bool ram_block_discard_is_required(void)
>> >>      return qatomic_read(&ram_block_discard_required_cnt) ||
>> >>             qatomic_read(&ram_block_coordinated_discard_required_cnt);
>> >>  }
>> >> +
>> >> +/*
>> >> + * Return true if ram is compatible with CPR.  Do not exclude rom,
>> >> + * because the rom file could change in new QEMU.
>> >> + */
>> >> +static bool ram_is_cpr_compatible(RAMBlock *rb)
>> >> +{
>> >> +    MemoryRegion *mr = rb->mr;
>> >> +
>> >> +    if (!mr || !memory_region_is_ram(mr)) {
>> >> +        return true;
>> >> +    }
>> >> +
>> >> +    /* Ram device is remapped in new QEMU */
>> >> +    if (memory_region_is_ram_device(mr)) {
>> >> +        return true;
>> >> +    }
>> >> +
>> >> +    /*
>> >> +     * A file descriptor is passed to new QEMU and remapped, or its backing
>> >> +     * file is reopened and mapped.  It must be shared to avoid COW.
>> >> +     */
>> >> +    if (rb->fd >= 0 && qemu_ram_is_shared(rb)) {
>> >> +        return true;
>> >> +    }
>> >> +
>> >> +    return false;
>> >> +}
>> >> +
>> >> +/*
>> >> + * Add a blocker for each volatile ram block.  This function should only be
>> >> + * called after we know that the block is migratable.  Non-migratable blocks
>> >> + * are either re-created in new QEMU, or are handled specially, or are covered
>> >> + * by a device-level CPR blocker.
>> >> + */
>> >> +void ram_block_add_cpr_blocker(RAMBlock *rb, Error **errp)
>> >> +{
>> >> +    assert(qemu_ram_is_migratable(rb));
>> >> +
>> >> +    if (ram_is_cpr_compatible(rb)) {
>> >> +        return;
>> >> +    }
>> >> +
>> >> +    error_setg(&rb->cpr_blocker,
>> >> +               "Memory region %s is not compatible with CPR. share=on is "
>> >> +               "required for memory-backend objects, and aux-ram-share=on is "
>> >> +               "required.", memory_region_name(rb->mr));
>> >> +    migrate_add_blocker_modes(&rb->cpr_blocker, errp, MIG_MODE_CPR_TRANSFER,
>> >> +                              -1);
>> >> +}
>> >> +
>> >> +void ram_block_del_cpr_blocker(RAMBlock *rb)
>> >> +{
>> >> +    migrate_del_blocker(&rb->cpr_blocker);
>> >> +}
>>