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

[Maran Wilson <maran.wilson@oracle.com>]

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

>       return r;
>   }
>