[PATCH V1] migration: cpr breaks SNP guest

[PATCH V1] migration: cpr breaks SNP guest

[Steve Sistare]

With aux-ram-share=off, booting an SNP guest fails with:

  ../util/error.c:68: error_setv: Assertion `*errp == NULL' failed.

This is because a CPR blocker for the guest_memfd ramblock is added
twice, once in ram_block_add_cpr_blocker because aux-ram-share=off so
rb->fd < 0, and once in ram_block_add for a specific guest_memfd blocker.

To fix, add the guest_memfd blocker iff a generic one would not be
added by ram_block_add_cpr_blocker.

Fixes: 094a3dbc55df ("migration: ram block cpr blockers")
Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
Reported-by: Michael Roth <michael.roth@amd.com>
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 system/physmem.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/system/physmem.c b/system/physmem.c
index e97de3e..cfafb06 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -1908,13 +1908,18 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
             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);
+        /*
+         * Add a specific guest_memfd blocker if a generic one would not be
+         * added by ram_block_add_cpr_blocker.
+         */
+        if (new_block->fd >= 0 && qemu_ram_is_shared(new_block)) {
+            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;
-- 
1.8.3.1

Re: [PATCH V1] migration: cpr breaks SNP guest

[Tom Lendacky]

On 3/27/25 08:36, Steve Sistare wrote:
> With aux-ram-share=off, booting an SNP guest fails with:
> 
>   ../util/error.c:68: error_setv: Assertion `*errp == NULL' failed.
> 
> This is because a CPR blocker for the guest_memfd ramblock is added
> twice, once in ram_block_add_cpr_blocker because aux-ram-share=off so
> rb->fd < 0, and once in ram_block_add for a specific guest_memfd blocker.
> 
> To fix, add the guest_memfd blocker iff a generic one would not be
> added by ram_block_add_cpr_blocker.
> 
> Fixes: 094a3dbc55df ("migration: ram block cpr blockers")
> Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
> Reported-by: Michael Roth <michael.roth@amd.com>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>

SNP guest launch works for me with this fix. Thanks, Steve!

Tested-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
>  system/physmem.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index e97de3e..cfafb06 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -1908,13 +1908,18 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
>              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);
> +        /*
> +         * Add a specific guest_memfd blocker if a generic one would not be
> +         * added by ram_block_add_cpr_blocker.
> +         */
> +        if (new_block->fd >= 0 && qemu_ram_is_shared(new_block)) {
> +            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;

Re: [PATCH V1] migration: cpr breaks SNP guest

[Fabiano Rosas]

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

> With aux-ram-share=off, booting an SNP guest fails with:
>
>   ../util/error.c:68: error_setv: Assertion `*errp == NULL' failed.
>
> This is because a CPR blocker for the guest_memfd ramblock is added
> twice, once in ram_block_add_cpr_blocker because aux-ram-share=off so
> rb->fd < 0, and once in ram_block_add for a specific guest_memfd blocker.
>
> To fix, add the guest_memfd blocker iff a generic one would not be
> added by ram_block_add_cpr_blocker.
>
> Fixes: 094a3dbc55df ("migration: ram block cpr blockers")
> Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
> Reported-by: Michael Roth <michael.roth@amd.com>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  system/physmem.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/system/physmem.c b/system/physmem.c
> index e97de3e..cfafb06 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -1908,13 +1908,18 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
>              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);
> +        /*
> +         * Add a specific guest_memfd blocker if a generic one would not be
> +         * added by ram_block_add_cpr_blocker.
> +         */
> +        if (new_block->fd >= 0 && qemu_ram_is_shared(new_block)) {

Can we avoid having two instances of the same condition that will need
to be kept in sync? What about:

/*
 * Preserving guest_memfd may be sufficient for CPR, but it has not
 * been tested yet.
 */
if (ram_is_cpr_compatible(new_block)) {
...

> +            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;

Re: [PATCH V1] migration: cpr breaks SNP guest

[Steven Sistare]

On 3/27/2025 10:21 AM, Fabiano Rosas wrote:
> Steve Sistare <steven.sistare@oracle.com> writes:
> 
>> With aux-ram-share=off, booting an SNP guest fails with:
>>
>>    ../util/error.c:68: error_setv: Assertion `*errp == NULL' failed.
>>
>> This is because a CPR blocker for the guest_memfd ramblock is added
>> twice, once in ram_block_add_cpr_blocker because aux-ram-share=off so
>> rb->fd < 0, and once in ram_block_add for a specific guest_memfd blocker.
>>
>> To fix, add the guest_memfd blocker iff a generic one would not be
>> added by ram_block_add_cpr_blocker.
>>
>> Fixes: 094a3dbc55df ("migration: ram block cpr blockers")
>> Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
>> Reported-by: Michael Roth <michael.roth@amd.com>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>   system/physmem.c | 19 ++++++++++++-------
>>   1 file changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/system/physmem.c b/system/physmem.c
>> index e97de3e..cfafb06 100644
>> --- a/system/physmem.c
>> +++ b/system/physmem.c
>> @@ -1908,13 +1908,18 @@ static void ram_block_add(RAMBlock *new_block, Error **errp)
>>               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);
>> +        /*
>> +         * Add a specific guest_memfd blocker if a generic one would not be
>> +         * added by ram_block_add_cpr_blocker.
>> +         */
>> +        if (new_block->fd >= 0 && qemu_ram_is_shared(new_block)) {
> 
> Can we avoid having two instances of the same condition that will need
> to be kept in sync? What about:
> 
> /*
>   * Preserving guest_memfd may be sufficient for CPR, but it has not
>   * been tested yet.
>   */
> if (ram_is_cpr_compatible(new_block)) {
> ...
> 

Sure, that is better. I will send V2 shortly.

- Steve

>> +            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;