Re: [PATCH v7 03/15] s390x: protvirt: Add migration blocker

[Janosch Frank]

On 3/9/20 2:41 PM, David Hildenbrand wrote:
> On 09.03.20 12:21, Janosch Frank wrote:
>> Migration is not yet supported.
>>
>> Signed-off-by: Janosch Frank <address@hidden>
>> ---
>>  hw/s390x/s390-virtio-ccw.c | 26 ++++++++++++++++++++++++--
>>  1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index f718cfc591..4bb38704ff 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)
>>  {
>> @@ -331,16 +334,33 @@ static void s390_machine_unprotect(S390CcwMachineState 
>> *ms)
>>          }
>>          ms->pv = false;
>>      }
>> +    migrate_del_blocker(pv_mig_blocker);
>> +    error_free(pv_mig_blocker);
>> +    pv_mig_blocker = NULL;
> 
> Maybe use error_free_or_abort(&pv_mig_blocker);

I dislike hiding setting the ptr to NULL with that function.
The assert for NULL in error_setg has caused me a lot of headache initially.

But if that's how it's done I'll fix it.

> 
>>  }
>>  
>>  static int s390_machine_protect(S390CcwMachineState *ms)
>>  {
>> +    static Error *local_err;
>>      CPUState *t;
>>      int rc;
>>  
>> +    error_setg(&pv_mig_blocker,
>> +               "protected VMs are currently not migrateable.");
>> +    rc = migrate_add_blocker(pv_mig_blocker, &local_err);
>> +    if (local_err) {
>> +        error_report_err(local_err);
>> +        error_free(pv_mig_blocker);
>> +        pv_mig_blocker = NULL;
> 
> Dito.
> 
>> +        return rc;
>> +    }
>> +
>>      /* Create SE VM */
>>      rc = s390_pv_vm_enable();
>>      if (rc) {
>> +        error_report_err(local_err);
>> +        error_free(pv_mig_blocker);
>> +        pv_mig_blocker = NULL;
> 
> Dito.
> 
>>          return rc;
>>      }
>>  
>> @@ -470,11 +490,13 @@ static void s390_machine_reset(MachineState *machine)
>>  
>>          if (s390_machine_protect(ms)) {
>>              s390_machine_inject_pv_error(cs);
>> -            s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>> -            return;
>> +            goto pv_err;
>>          }
>>  
>>          run_on_cpu(cs, s390_do_cpu_load_normal, RUN_ON_CPU_NULL);
>> +pv_err:
>> +        /* Continue after the diag308 so the guest knows something went 
>> wrong. */
>> +        s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
>>          break;
> 
> Why does this change *not* belong into path #2? I think I asked this
> already.

Yes, I'll move it

signature.asc
Description: OpenPGP digital signature