Re: [Qemu-devel] [QEMU PATCH v4 01/10] target/i386: kvm: Delete VMX migration blocker on vCPU init failure

[Liran Alon <liran.alon@oracle.com>]

> On 19 Jun 2019, at 23:30, Maran Wilson <maran.wilson@oracle.com> wrote:
> 
> On 6/19/2019 9:21 AM, Liran Alon wrote:
>> Commit d98f26073beb ("target/i386: kvm: add VMX migration blocker")
>> added migration blocker for vCPU exposed with Intel VMX because QEMU
>> doesn't yet contain code to support migration of nested virtualization
>> workloads.
>> 
>> However, that commit missed adding deletion of the migration blocker in
>> case init of vCPU failed. Similar to invtsc_mig_blocker. This commit fix
>> that issue.
>> 
>> Fixes: d98f26073beb ("target/i386: kvm: add VMX migration blocker")
>> Signed-off-by: Liran Alon <liran.alon@oracle.com>
>> ---
>>  target/i386/kvm.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>> 
>> diff --git a/target/i386/kvm.c b/target/i386/kvm.c
>> index 3b29ce5c0d08..7aa7914a498c 100644
>> --- a/target/i386/kvm.c
>> +++ b/target/i386/kvm.c
>> @@ -940,7 +940,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>        r = kvm_arch_set_tsc_khz(cs);
>>      if (r < 0) {
>> -        goto fail;
>> +        return r;
>>      }
>>        /* vcpu's TSC frequency is either specified by user, or following
>> @@ -1295,7 +1295,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>              if (local_err) {
>>                  error_report_err(local_err);
>>                  error_free(invtsc_mig_blocker);
>> -                return r;
>> +                goto fail2;
>>              }
>>          }
>>      }
>> @@ -1346,6 +1346,9 @@ int kvm_arch_init_vcpu(CPUState *cs)
>>     fail:
>>      migrate_del_blocker(invtsc_mig_blocker);
>> + fail2:
>> +    migrate_del_blocker(vmx_mig_blocker);
>> +
> 
> At the risk of being a bit pedantic...
> 
> Your changes don't introduce this problem, but they do make it worse -- Since [vmx|invtsc]_mig_blocker are both global in scope, isn't it possible you end up deleting one or both valid blockers that were created by a previous invocation of kvm_arch_init_vcpu() ?  Seems to me that you would need something like an additional pair of local boolean variables named created_[vmx|invtsc]_mig_blocker and condition the calls to migrate_del_blocker() accordingly. On the positive side, that would simplify some of the logic around when and if it's ok to jump to "fail" (and you wouldn't need the "fail2").
> 
> Thanks,
> -Maran

I actually thought about this as-well when I encountered this issue.
In general one can argue that every vCPU should introduce it’s own migration blocker on init (if required) and remove it’s own migration blocker on deletion (on vCPU destroy).
On 99% of the time, all of this shouldn’t matter as all vCPUs of a given VM have exactly the same properties.
Anyway, I decided that this is entirely not relevant for this patch-series and therefore if this should be addressed further, let it be another unrelated patch-series.
QEMU have too many issues to fix all at once :P. I need to filter.

-Liran

> 
>>      return r;
>>  }
>>