[PATCH v4 17/31] i386/sev: add migration blockers only once

Ani Sinha posted 31 patches 1 month, 4 weeks ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Zhao Liu <zhao1.liu@intel.com>, "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>, "Michael S. Tsirkin" <mst@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Bernhard Beschow <shentey@gmail.com>, Alex Williamson <alex@shazbot.org>, "Cédric Le Goater" <clg@redhat.com>, Peter Xu <peterx@redhat.com>, Ani Sinha <anisinha@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>, David Woodhouse <dwmw2@infradead.org>, Paul Durrant <paul@xen.org>
There is a newer version of this series
[PATCH v4 17/31] i386/sev: add migration blockers only once
Posted by Ani Sinha 1 month, 4 weeks ago
sev_launch_finish() and sev_snp_launch_finish() could be called multiple times
when the confidential guest is being reset/rebooted. The migration
blockers should not be added multiple times, once per invocation. This change
makes sure that the migration blockers are added only one time by adding the
migration blockers to the vm state change handler when the vm transitions to
the running state. Subsequent reboots do not change the state of the vm.

Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 target/i386/sev.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 66e38ca32e..260d8ef88b 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1421,11 +1421,6 @@ sev_launch_finish(SevCommonState *sev_common)
     }
 
     sev_set_guest_state(sev_common, SEV_STATE_RUNNING);
-
-    /* add migration blocker */
-    error_setg(&sev_mig_blocker,
-               "SEV: Migration is not implemented");
-    migrate_add_blocker(&sev_mig_blocker, &error_fatal);
 }
 
 static int snp_launch_update_data(uint64_t gpa, void *hva, size_t len,
@@ -1608,7 +1603,6 @@ static void
 sev_snp_launch_finish(SevCommonState *sev_common)
 {
     int ret, error;
-    Error *local_err = NULL;
     OvmfSevMetadata *metadata;
     SevLaunchUpdateData *data;
     SevSnpGuestState *sev_snp = SEV_SNP_GUEST(sev_common);
@@ -1655,15 +1649,6 @@ sev_snp_launch_finish(SevCommonState *sev_common)
 
     kvm_mark_guest_state_protected();
     sev_set_guest_state(sev_common, SEV_STATE_RUNNING);
-
-    /* add migration blocker */
-    error_setg(&sev_mig_blocker,
-               "SEV-SNP: Migration is not implemented");
-    ret = migrate_add_blocker(&sev_mig_blocker, &local_err);
-    if (local_err) {
-        error_report_err(local_err);
-        exit(1);
-    }
 }
 
 
@@ -1676,6 +1661,11 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
     if (running) {
         if (!sev_check_state(sev_common, SEV_STATE_RUNNING)) {
             klass->launch_finish(sev_common);
+
+            /* add migration blocker */
+            error_setg(&sev_mig_blocker,
+                       "SEV: Migration is not implemented");
+            migrate_add_blocker(&sev_mig_blocker, &error_fatal);
         }
     }
 }
-- 
2.42.0
Re: [PATCH v4 17/31] i386/sev: add migration blockers only once
Posted by Prasad Pandit 1 month, 4 weeks ago
On Thu, 12 Feb 2026 at 11:58, Ani Sinha <anisinha@redhat.com> wrote:
> sev_launch_finish() and sev_snp_launch_finish() could be called multiple times
> when the confidential guest is being reset/rebooted. The migration
> blockers should not be added multiple times, once per invocation. This change
> makes sure that the migration blockers are added only one time by adding the
> migration blockers to the vm state change handler when the vm transitions to
> the running state. Subsequent reboots do not change the state of the vm.
>
> Signed-off-by: Ani Sinha <anisinha@redhat.com>
> ---
>  target/i386/sev.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 66e38ca32e..260d8ef88b 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -1421,11 +1421,6 @@ sev_launch_finish(SevCommonState *sev_common)
>      }
>
>      sev_set_guest_state(sev_common, SEV_STATE_RUNNING);
> -
> -    /* add migration blocker */
> -    error_setg(&sev_mig_blocker,
> -               "SEV: Migration is not implemented");
> -    migrate_add_blocker(&sev_mig_blocker, &error_fatal);
>  }
>
>  static int snp_launch_update_data(uint64_t gpa, void *hva, size_t len,
> @@ -1608,7 +1603,6 @@ static void
>  sev_snp_launch_finish(SevCommonState *sev_common)
>  {
>      int ret, error;
> -    Error *local_err = NULL;
>      OvmfSevMetadata *metadata;
>      SevLaunchUpdateData *data;
>      SevSnpGuestState *sev_snp = SEV_SNP_GUEST(sev_common);
> @@ -1655,15 +1649,6 @@ sev_snp_launch_finish(SevCommonState *sev_common)
>
>      kvm_mark_guest_state_protected();
>      sev_set_guest_state(sev_common, SEV_STATE_RUNNING);
> -
> -    /* add migration blocker */
> -    error_setg(&sev_mig_blocker,
> -               "SEV-SNP: Migration is not implemented");
> -    ret = migrate_add_blocker(&sev_mig_blocker, &local_err);
> -    if (local_err) {
> -        error_report_err(local_err);
> -        exit(1);
> -    }
>  }
>
>
> @@ -1676,6 +1661,11 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
>      if (running) {
>          if (!sev_check_state(sev_common, SEV_STATE_RUNNING)) {
>              klass->launch_finish(sev_common);
> +
> +            /* add migration blocker */
> +            error_setg(&sev_mig_blocker,
> +                       "SEV: Migration is not implemented");
> +            migrate_add_blocker(&sev_mig_blocker, &error_fatal);
>          }
>      }
>  }
> --

* 'sev_mig_blocker' is a global static variable, so it's the same
blocker (address) added each time, maybe add_blocker() should do a
check to avoid duplicates.

* Otherwise it looks okay.
Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>

Thank you.
---
  - Prasad
Re: [PATCH v4 17/31] i386/sev: add migration blockers only once
Posted by Ani Sinha 1 month, 4 weeks ago

> On 12 Feb 2026, at 1:20 PM, Prasad Pandit <ppandit@redhat.com> wrote:
> 
> On Thu, 12 Feb 2026 at 11:58, Ani Sinha <anisinha@redhat.com> wrote:
>> sev_launch_finish() and sev_snp_launch_finish() could be called multiple times
>> when the confidential guest is being reset/rebooted. The migration
>> blockers should not be added multiple times, once per invocation. This change
>> makes sure that the migration blockers are added only one time by adding the
>> migration blockers to the vm state change handler when the vm transitions to
>> the running state. Subsequent reboots do not change the state of the vm.
>> 
>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>> ---
>> target/i386/sev.c | 20 +++++---------------
>> 1 file changed, 5 insertions(+), 15 deletions(-)
>> 
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 66e38ca32e..260d8ef88b 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -1421,11 +1421,6 @@ sev_launch_finish(SevCommonState *sev_common)
>>     }
>> 
>>     sev_set_guest_state(sev_common, SEV_STATE_RUNNING);
>> -
>> -    /* add migration blocker */
>> -    error_setg(&sev_mig_blocker,
>> -               "SEV: Migration is not implemented");
>> -    migrate_add_blocker(&sev_mig_blocker, &error_fatal);
>> }
>> 
>> static int snp_launch_update_data(uint64_t gpa, void *hva, size_t len,
>> @@ -1608,7 +1603,6 @@ static void
>> sev_snp_launch_finish(SevCommonState *sev_common)
>> {
>>     int ret, error;
>> -    Error *local_err = NULL;
>>     OvmfSevMetadata *metadata;
>>     SevLaunchUpdateData *data;
>>     SevSnpGuestState *sev_snp = SEV_SNP_GUEST(sev_common);
>> @@ -1655,15 +1649,6 @@ sev_snp_launch_finish(SevCommonState *sev_common)
>> 
>>     kvm_mark_guest_state_protected();
>>     sev_set_guest_state(sev_common, SEV_STATE_RUNNING);
>> -
>> -    /* add migration blocker */
>> -    error_setg(&sev_mig_blocker,
>> -               "SEV-SNP: Migration is not implemented");
>> -    ret = migrate_add_blocker(&sev_mig_blocker, &local_err);
>> -    if (local_err) {
>> -        error_report_err(local_err);
>> -        exit(1);
>> -    }
>> }
>> 
>> 
>> @@ -1676,6 +1661,11 @@ sev_vm_state_change(void *opaque, bool running, RunState state)
>>     if (running) {
>>         if (!sev_check_state(sev_common, SEV_STATE_RUNNING)) {
>>             klass->launch_finish(sev_common);
>> +
>> +            /* add migration blocker */
>> +            error_setg(&sev_mig_blocker,
>> +                       "SEV: Migration is not implemented");
>> +            migrate_add_blocker(&sev_mig_blocker, &error_fatal);
>>         }
>>     }
>> }
>> --
> 
> * 'sev_mig_blocker' is a global static variable, so it's the same
> blocker (address) added each time, maybe add_blocker() should do a
> check to avoid duplicates.

Maybe we could do something like this (I have not tested it) but perhaps outside the scope of this change set:

diff --git a/migration/migration.c b/migration/migration.c
index b103a82fc0..7d85821315 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1696,8 +1696,11 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
 {
     for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
         if (modes & BIT(mode)) {
-            migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
-                                                       *reasonp);
+            if (g_slist_index(migration_blockers[mode],
+                                 *reasonp) == -1) {
+                migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
+                                                           *reasonp);
+            }
         }
     }
     return 0;


> 
> * Otherwise it looks okay.
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
> 
> Thank you.
> ---
>  - Prasad
> 
Re: [PATCH v4 17/31] i386/sev: add migration blockers only once
Posted by Prasad Pandit 1 month, 4 weeks ago
On Thu, 12 Feb 2026 at 14:07, Ani Sinha <anisinha@redhat.com> wrote:
> Maybe we could do something like this (I have not tested it) but perhaps outside the scope of this change set:
>
> diff --git a/migration/migration.c b/migration/migration.c
> index b103a82fc0..7d85821315 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1696,8 +1696,11 @@ static int add_blockers(Error **reasonp, unsigned modes, Error **errp)
>  {
>      for (MigMode mode = 0; mode < MIG_MODE__MAX; mode++) {
>          if (modes & BIT(mode)) {
> -            migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
> -                                                       *reasonp);
> +            if (g_slist_index(migration_blockers[mode],
> +                                 *reasonp) == -1) {
> +                migration_blockers[mode] = g_slist_prepend(migration_blockers[mode],
> +                                                           *reasonp);
> +            }
>          }
>      }
>      return 0;

* Yes, and when g_slist_index() returns >= 0,  return -1 OR an
indication that the blocker (address) already exists in the list. That
way other such instances where the same blocker is added multiple
times will be caught.

* But let's make it a separate patch. Restrict current patch to
removing add_clocker calls from sev_*_launch_finish() calls.

Thank you.
---
  - Prasad