Re: [PATCH v6 04/18] s390x: protvirt: Add migration blocker

[David Hildenbrand <david@redhat.com>]

On 04.03.20 12:42, Janosch Frank wrote:
> Migration is not yet supported.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index dd39890f89..272531a9ee 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -43,6 +43,9 @@
>  #include "sysemu/sysemu.h"
>  #include "hw/s390x/pv.h"
>  #include <linux/kvm.h>
> +#include "migration/blocker.h"
> +
> +static Error *pv_mig_blocker;
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> @@ -324,19 +327,30 @@ static void s390_machine_unprotect(S390CcwMachineState *ms)
>  {
>      CPUState *t;
>  
> -    if (!ms->pv)
> -        return;
> -    s390_pv_vm_disable();
> -    CPU_FOREACH(t) {
> -        S390_CPU(t)->env.pv = false;
> +    if (ms->pv) {
> +        s390_pv_vm_disable();
> +        CPU_FOREACH(t) {
> +            S390_CPU(t)->env.pv = false;
> +        }
> +        ms->pv = false;
>      }
> -    ms->pv = false;
> +    migrate_del_blocker(pv_mig_blocker);

Is it just me or is this unnecessary code churn that wants to challenge
my reviewing capabilities? ;) Please perform that rework in the previous
patch already.

Also, I do wonder why the blocker is always removed (IOW, how we could
end up unprotecting, although there is nothing to unprotect).

This needs a comment in the patch description.


>  }
>  
>  static int s390_machine_protect(S390CcwMachineState *ms)
>  {
> +    static Error *local_err;
>      CPUState *t;
> -    int rc;
> +    int rc = -1;
> +
> +    if (!pv_mig_blocker) {

I think we should instead error_free() when unprotecting ...

> +        error_setg(&pv_mig_blocker,
> +                   "protected VMs are currently not migrateable.");
> +    }
> +    migrate_add_blocker(pv_mig_blocker, &local_err);
> +    if (local_err) {
> +        goto out_err;

... and on the error path.

> +    }
>  
>      /* Create SE VM */
>      rc = s390_pv_vm_enable();
> @@ -440,11 +454,12 @@ static void s390_machine_reset(MachineState *machine)
>  
>          if (s390_machine_protect(ms)) {
>              s390_machine_inject_pv_error(cs);

ew, injecting an exception during a reset. (I know this belongs into the
previous patch)

> -            s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
> -            return;
> +            goto pv_err;

I have no idea why this hunk is in this patch. What am I missing?

>          }
>  
>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
> +pv_err:
> +        s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>          break;
>      default:
>          g_assert_not_reached();
> 


-- 
Thanks,

David / dhildenb