diff mbox series

[v3,06/17] s390x: protvirt: Add migration blocker

Message ID 20200214151636.8764-7-frankja@linux.ibm.com
State New
Headers show
Series s390x: Protected Virtualization support | expand

Commit Message

Janosch Frank Feb. 14, 2020, 3:16 p.m. UTC 
Migration is not yet supported.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Cornelia Huck Feb. 20, 2020, 10:48 a.m. UTC | #1 
On Fri, 14 Feb 2020 10:16:25 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Migration is not yet supported.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 5fa4372083..d64724af91 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -42,6 +42,9 @@
>  #include "hw/s390x/tod.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/s390x/pv.h"
> +#include "migration/blocker.h"
> +
> +static Error *pv_mig_blocker;
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> @@ -373,6 +376,7 @@ static void s390_machine_reset(MachineState *machine)
>      CPUState *cs, *t;
>      S390CPU *cpu;
>      S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
> +    static Error *local_err;
>  
>      /* get the reset parameters, reset them once done */
>      s390_ipl_get_reset_request(&cs, &reset_type);
> @@ -422,6 +426,17 @@ static void s390_machine_reset(MachineState *machine)
>          }
>          run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL);
>  
> +        if (!pv_mig_blocker) {
> +            error_setg(&pv_mig_blocker,
> +                       "protected VMs are currently not migrateable.");
> +        }
> +        migrate_add_blocker(pv_mig_blocker, &local_err);

If I'm not lost in the context, that's during PV_RESET. I'm a bit
confused why you'd add the blocker here?

> +        if (local_err) {
> +            error_report_err(local_err);
> +            error_free(pv_mig_blocker);
> +            exit(1);

Why the exit()? Can't you fail the call?

> +        }
> +
>          if (s390_machine_pv_secure(ms)) {
>              CPU_FOREACH(t) {
>                  s390_pv_vcpu_destroy(t);
> @@ -430,6 +445,7 @@ static void s390_machine_reset(MachineState *machine)
>              ms->pv = false;
>  
>              s390_machine_inject_pv_error(cs);
> +            migrate_del_blocker(pv_mig_blocker);
>              s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>              return;
>          }
Janosch Frank Feb. 20, 2020, 11:24 a.m. UTC | #2 
On 2/20/20 11:48 AM, Cornelia Huck wrote:
> On Fri, 14 Feb 2020 10:16:25 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Migration is not yet supported.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 5fa4372083..d64724af91 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -42,6 +42,9 @@
>>  #include "hw/s390x/tod.h"
>>  #include "sysemu/sysemu.h"
>>  #include "hw/s390x/pv.h"
>> +#include "migration/blocker.h"
>> +
>> +static Error *pv_mig_blocker;
>>  
>>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>  {
>> @@ -373,6 +376,7 @@ static void s390_machine_reset(MachineState *machine)
>>      CPUState *cs, *t;
>>      S390CPU *cpu;
>>      S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
>> +    static Error *local_err;
>>  
>>      /* get the reset parameters, reset them once done */
>>      s390_ipl_get_reset_request(&cs, &reset_type);
>> @@ -422,6 +426,17 @@ static void s390_machine_reset(MachineState *machine)
>>          }
>>          run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL);
>>  
>> +        if (!pv_mig_blocker) {
>> +            error_setg(&pv_mig_blocker,
>> +                       "protected VMs are currently not migrateable.");
>> +        }
>> +        migrate_add_blocker(pv_mig_blocker, &local_err);
> 
> If I'm not lost in the context, that's during PV_RESET. I'm a bit
> confused why you'd add the blocker here?

Where would you want me to add it?
It's here where we switch into secure mode and I need to block before
switching and unblock if it fails.

When having the blocker in diag.c, I'd have a hard time unblocking on a
PV switch fail.

> 
>> +        if (local_err) {
>> +            error_report_err(local_err);
>> +            error_free(pv_mig_blocker);
>> +            exit(1);
> 
> Why the exit()? Can't you fail the call?

Well, if that fails and we go protected, I wouldn't be protected agains
migrations, right?

> 
>> +        }
>> +
>>          if (s390_machine_pv_secure(ms)) {
>>              CPU_FOREACH(t) {
>>                  s390_pv_vcpu_destroy(t);
>> @@ -430,6 +445,7 @@ static void s390_machine_reset(MachineState *machine)
>>              ms->pv = false;
>>  
>>              s390_machine_inject_pv_error(cs);
>> +            migrate_del_blocker(pv_mig_blocker);
>>              s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>              return;
>>          }
> 
>
Cornelia Huck Feb. 20, 2020, 11:39 a.m. UTC | #3 
On Thu, 20 Feb 2020 12:24:23 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 2/20/20 11:48 AM, Cornelia Huck wrote:
> > On Fri, 14 Feb 2020 10:16:25 -0500
> > Janosch Frank <frankja@linux.ibm.com> wrote:
> >   
> >> Migration is not yet supported.
> >>
> >> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> >> ---
> >>  hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++++++
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> >> index 5fa4372083..d64724af91 100644
> >> --- a/hw/s390x/s390-virtio-ccw.c
> >> +++ b/hw/s390x/s390-virtio-ccw.c
> >> @@ -42,6 +42,9 @@
> >>  #include "hw/s390x/tod.h"
> >>  #include "sysemu/sysemu.h"
> >>  #include "hw/s390x/pv.h"
> >> +#include "migration/blocker.h"
> >> +
> >> +static Error *pv_mig_blocker;
> >>  
> >>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> >>  {
> >> @@ -373,6 +376,7 @@ static void s390_machine_reset(MachineState *machine)
> >>      CPUState *cs, *t;
> >>      S390CPU *cpu;
> >>      S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
> >> +    static Error *local_err;
> >>  
> >>      /* get the reset parameters, reset them once done */
> >>      s390_ipl_get_reset_request(&cs, &reset_type);
> >> @@ -422,6 +426,17 @@ static void s390_machine_reset(MachineState *machine)
> >>          }
> >>          run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL);
> >>  
> >> +        if (!pv_mig_blocker) {
> >> +            error_setg(&pv_mig_blocker,
> >> +                       "protected VMs are currently not migrateable.");
> >> +        }
> >> +        migrate_add_blocker(pv_mig_blocker, &local_err);  
> > 
> > If I'm not lost in the context, that's during PV_RESET. I'm a bit
> > confused why you'd add the blocker here?  
> 
> Where would you want me to add it?
> It's here where we switch into secure mode and I need to block before
> switching and unblock if it fails.
> 
> When having the blocker in diag.c, I'd have a hard time unblocking on a
> PV switch fail.
> 
> >   
> >> +        if (local_err) {
> >> +            error_report_err(local_err);
> >> +            error_free(pv_mig_blocker);
> >> +            exit(1);  
> > 
> > Why the exit()? Can't you fail the call?  
> 
> Well, if that fails and we go protected, I wouldn't be protected agains
> migrations, right?

No, I meant not go protected, if that's possible.

> 
> >   
> >> +        }
> >> +
> >>          if (s390_machine_pv_secure(ms)) {

Ok, I think what confuses me is this call: it reads as if you actually
tear down things if the machine is secure. Call it
s390_machine_pv_make_secure() to make sure it is actively doing
something and not checking a previously set value?

> >>              CPU_FOREACH(t) {
> >>                  s390_pv_vcpu_destroy(t);
> >> @@ -430,6 +445,7 @@ static void s390_machine_reset(MachineState *machine)
> >>              ms->pv = false;
> >>  
> >>              s390_machine_inject_pv_error(cs);
> >> +            migrate_del_blocker(pv_mig_blocker);
> >>              s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> >>              return;
> >>          }  
> > 
> >   
> 
>
Janosch Frank Feb. 20, 2020, 11:42 a.m. UTC | #4 
On 2/20/20 12:39 PM, Cornelia Huck wrote:
> On Thu, 20 Feb 2020 12:24:23 +0100
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On 2/20/20 11:48 AM, Cornelia Huck wrote:
>>> On Fri, 14 Feb 2020 10:16:25 -0500
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>   
>>>> Migration is not yet supported.
>>>>
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>  hw/s390x/s390-virtio-ccw.c | 16 ++++++++++++++++
>>>>  1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index 5fa4372083..d64724af91 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -42,6 +42,9 @@
>>>>  #include "hw/s390x/tod.h"
>>>>  #include "sysemu/sysemu.h"
>>>>  #include "hw/s390x/pv.h"
>>>> +#include "migration/blocker.h"
>>>> +
>>>> +static Error *pv_mig_blocker;
>>>>  
>>>>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>>>  {
>>>> @@ -373,6 +376,7 @@ static void s390_machine_reset(MachineState *machine)
>>>>      CPUState *cs, *t;
>>>>      S390CPU *cpu;
>>>>      S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
>>>> +    static Error *local_err;
>>>>  
>>>>      /* get the reset parameters, reset them once done */
>>>>      s390_ipl_get_reset_request(&cs, &reset_type);
>>>> @@ -422,6 +426,17 @@ static void s390_machine_reset(MachineState *machine)
>>>>          }
>>>>          run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL);
>>>>  
>>>> +        if (!pv_mig_blocker) {
>>>> +            error_setg(&pv_mig_blocker,
>>>> +                       "protected VMs are currently not migrateable.");
>>>> +        }
>>>> +        migrate_add_blocker(pv_mig_blocker, &local_err);  
>>>
>>> If I'm not lost in the context, that's during PV_RESET. I'm a bit
>>> confused why you'd add the blocker here?  
>>
>> Where would you want me to add it?
>> It's here where we switch into secure mode and I need to block before
>> switching and unblock if it fails.
>>
>> When having the blocker in diag.c, I'd have a hard time unblocking on a
>> PV switch fail.
>>
>>>   
>>>> +        if (local_err) {
>>>> +            error_report_err(local_err);
>>>> +            error_free(pv_mig_blocker);
>>>> +            exit(1);  
>>>
>>> Why the exit()? Can't you fail the call?  
>>
>> Well, if that fails and we go protected, I wouldn't be protected agains
>> migrations, right?
> 
> No, I meant not go protected, if that's possible.

That would be an option, now that we have a proper d308 rc for such a thing.

Will add!

> 
>>
>>>   
>>>> +        }
>>>> +
>>>>          if (s390_machine_pv_secure(ms)) {
> 
> Ok, I think what confuses me is this call: it reads as if you actually
> tear down things if the machine is secure. Call it
> s390_machine_pv_make_secure() to make sure it is actively doing
> something and not checking a previously set value?

Ok, will use something along these lines

> 
>>>>              CPU_FOREACH(t) {
>>>>                  s390_pv_vcpu_destroy(t);
>>>> @@ -430,6 +445,7 @@ static void s390_machine_reset(MachineState *machine)
>>>>              ms->pv = false;
>>>>  
>>>>              s390_machine_inject_pv_error(cs);
>>>> +            migrate_del_blocker(pv_mig_blocker);
>>>>              s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>>>              return;
>>>>          }  
>>>
>>>   
>>
>>
>
diff mbox series

Patch

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 5fa4372083..d64724af91 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -42,6 +42,9 @@ 
 #include "hw/s390x/tod.h"
 #include "sysemu/sysemu.h"
 #include "hw/s390x/pv.h"
+#include "migration/blocker.h"
+
+static Error *pv_mig_blocker;
 
 S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
 {
@@ -373,6 +376,7 @@  static void s390_machine_reset(MachineState *machine)
     CPUState *cs, *t;
     S390CPU *cpu;
     S390CcwMachineState *ms = S390_CCW_MACHINE(machine);
+    static Error *local_err;
 
     /* get the reset parameters, reset them once done */
     s390_ipl_get_reset_request(&cs, &reset_type);
@@ -422,6 +426,17 @@  static void s390_machine_reset(MachineState *machine)
         }
         run_on_cpu(cs, s390_do_cpu_reset, RUN_ON_CPU_NULL);
 
+        if (!pv_mig_blocker) {
+            error_setg(&pv_mig_blocker,
+                       "protected VMs are currently not migrateable.");
+        }
+        migrate_add_blocker(pv_mig_blocker, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            error_free(pv_mig_blocker);
+            exit(1);
+        }
+
         if (s390_machine_pv_secure(ms)) {
             CPU_FOREACH(t) {
                 s390_pv_vcpu_destroy(t);
@@ -430,6 +445,7 @@  static void s390_machine_reset(MachineState *machine)
             ms->pv = false;
 
             s390_machine_inject_pv_error(cs);
+            migrate_del_blocker(pv_mig_blocker);
             s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
             return;
         }