Re: [Qemu-devel] [QEMU PATCH v4 01/10] target/i386: kvm: Delete VMX migration blocker on vCPU init failure
On 6/19/2019 1:33 PM, Liran Alon wrote:
On 19 Jun 2019, at 23:30, Maran Wilson 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
---
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.
The example I was thinking about is a VM that is created with a bunch of
vCPUs -- all of which require installation of the blocker(s). Then at
some point in the future, a failed CPU hotplug attempt wipes out all the
pre-existing blockers and leaves the VM exposed.
But I agree that the problem wasn't introduced by this patch series and
so it is reasonable to argue that you shouldn't have to fix it here. As
such:
Reviewed-by: Maran Wilson
Thanks,
-Maran
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;
}
Re: [Qemu-devel] [QEMU PATCH v4 01/10] target/i386: kvm: Delete VMX migration blocker on vCPU init failure
> On 19 Jun 2019, at 23:30, Maran Wilson 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
>> ---
>> 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;
>> }
>>
Re: [Qemu-devel] [QEMU PATCH v4 01/10] target/i386: kvm: Delete VMX migration blocker on vCPU init failure
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
---
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;
}
