| Message ID | 20250307181551.19887-3-farosas@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,1/8] migration: Fix UAF for incoming migration on MigrationState | expand |
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. 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? 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); > +}
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. > > 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: 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. 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. 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); >> +}
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. > > > > 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: > > 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. > > 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; 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); > >> +} >
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); >> >> +} >>
On Wed, Mar 26, 2025 at 05:13:50PM -0300, Fabiano Rosas wrote: > 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? 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); > >> >> +} > >>
On 3/26/2025 5:34 PM, Michael Roth wrote: > On Wed, Mar 26, 2025 at 05:13:50PM -0300, Fabiano Rosas wrote: >> 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? > > 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 Thanks everyone for debugging this. To summarize, ram_block_add_cpr_blocker already blocks guest_memfd, because rb->fd < 0. The fix is to delete this redundant code in ram_block_add: 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, MIG_MODE_CPR_EXEC, -1); I will submit a fix (unless Tom or Michael would prefer to author it). - Steve
On 3/27/25 07:27, Steven Sistare wrote: > On 3/26/2025 5:34 PM, Michael Roth wrote: >> On Wed, Mar 26, 2025 at 05:13:50PM -0300, Fabiano Rosas wrote: >>> 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> > > Thanks everyone for debugging this. To summarize, > ram_block_add_cpr_blocker already blocks > guest_memfd, because rb->fd < 0. The fix is to delete this redundant > code in ram_block_add: > > 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, MIG_MODE_CPR_EXEC, > -1); > > I will submit a fix (unless Tom or Michael would prefer to author it). Steve, please do submit the fix. Thanks! Tom > > - Steve
On 3/27/2025 8:27 PM, Steven Sistare wrote: > On 3/26/2025 5:34 PM, Michael Roth wrote: >> On Wed, Mar 26, 2025 at 05:13:50PM -0300, Fabiano Rosas wrote: >>> 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? >> >> 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 > > Thanks everyone for debugging this. To summarize, > ram_block_add_cpr_blocker already blocks > guest_memfd, because rb->fd < 0. The fix is to delete this redundant > code in ram_block_add: > > 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, > MIG_MODE_CPR_EXEC, > -1); I just encountered the same issue with TDX guest, after rebasing TDX code to 10.0.0-rc0. thank you all for the reporting and quick solution for it. > I will submit a fix (unless Tom or Michael would prefer to author it). > > - Steve >
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); +}