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(+)
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);
+}
--
2.35.3On 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/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 >
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
© 2016 - 2025 Red Hat, Inc.