On Wed, Mar 26, 2025 at 05:13:50PM -0300, Fabiano Rosas wrote:
> Michael Roth <michael.r...@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.sist...@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.sist...@oracle.com>
> >> >> Reviewed-by: Fabiano Rosas <faro...@suse.de>
> >> >> Reviewed-by: Peter Xu <pet...@redhat.com>
> >> >> Reviewed-by: David Hildenbrand <da...@redhat.com>
> >> >> Message-ID:
> >> >> <1740667681-257312-1-git-send-email-steven.sist...@oracle.com>
> >> >> Signed-off-by: Fabiano Rosas <faro...@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?
That seems to be the case: in some cases ram_block_add() sets cpr_blocker
when (new_block->flags & RAM_GUEST_MEMFD) is true, and then soon after
when ram_block_add_cpr_blocker() is called on the same RAMBlock:
2025-03-26T21:08:15.092427Z qemu-system-x86_64: warning: ram_block_add:
new_block 0x55c247e4c880 new_block->cpr_blocker (nil) name ram1
2025-03-26T21:08:15.124710Z qemu-system-x86_64: warning: ram_block_add:
new_block 0x55c2480fde00 new_block->cpr_blocker (nil) name pc.bios
2025-03-26T21:08:15.126190Z qemu-system-x86_64: warning:
ram_block_add_cpr_blocker: rb 0x55c2480fde00 rb->cpr_blocker 0x55c2480fe050
name pc.bios
2025-03-26T21:08:15.138582Z qemu-system-x86_64: warning: ram_block_add:
new_block 0x55c247e3c1e0 new_block->cpr_blocker (nil) name pc.rom
2025-03-26T21:08:15.138938Z qemu-system-x86_64: warning:
ram_block_add_cpr_blocker: rb 0x55c247e3c1e0 rb->cpr_blocker 0x55c247e3c890
name pc.rom
2025-03-26T21:08:16.185577Z qemu-system-x86_64: warning:
ram_block_add_cpr_blocker: rb 0x55c248db9200 rb->cpr_blocker (nil) name
/rom@etc/acpi/tables
2025-03-26T21:08:16.187140Z qemu-system-x86_64: warning:
ram_block_add_cpr_blocker: rb 0x55c248085620 rb->cpr_blocker (nil) name
/rom@etc/table-loader
2025-03-26T21:08:16.188029Z qemu-system-x86_64: warning:
ram_block_add_cpr_blocker: rb 0x55c2480ce220 rb->cpr_blocker (nil) name
/rom@etc/acpi/rsd
-Mike
>
> > 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);
> >> >> +}
> >>
