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

[David Hildenbrand]

On 05.03.20 10:16, Janosch Frank wrote:
> 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.

Yes, but please do that in the previous patch instead. This makes review
of this patch unnecessary complicated.


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

Why could we get a call to s390_machine_unprotect() and
- have !ms->pv
- still need to do a migrate_del_blocker()

Not clear to me.

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

The point I am making: This change should go into the previous patch
unless I am missing something important.


-- 
Thanks,

David / dhildenb