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

[Janosch Frank]

On 3/4/20 6:13 PM, David Hildenbrand wrote:
> On 04.03.20 12:42, Janosch Frank wrote:
>> Migration is not yet supported.
>>
>> Signed-off-by: Janosch Frank <address@hidden>
>> ---
>>  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.

It's rather more of personal style choice to do a "if return" to not
encapsulate everything in ifs.

> 
> 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.

You mean the error check for migrate_add_blocker()?
I'm still metabolizing my coffee...

> 
> 
>>  }
>>  
>>  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 ...

Ack

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

To also return the rc code, I changed it to:

    rc = migrate_add_blocker(pv_mig_blocker, &local_err);
    if (local_err) {
        error_free(pv_mig_blocker);
        return rc;
    }


> 
>> +    }
>>  
>>      /* 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)

Yeah, not too proud about this one, but doing it at diag308 time is not
possible, since we need to reset the devices before attempting to protect.

> 
>> -            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?

The error needs to be communicated to the guest, so we need to resume
CPU operation instead of doing a load which we would do on success.

> 
>>          }
>>  
>>          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();
>>
> 
>

signature.asc
Description: OpenPGP digital signature