| Message ID | 20180328153024.23039-3-rkagan@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 28, 2018 at 06:30:24PM +0300, Roman Kagan wrote: > In order to guarantee compatibility on migration, QEMU should have > complete control over the features it announces to the guest via CPUID. > > However, for a number of Hyper-V-related cpu properties, if the > corresponding feature is not supported by the underlying KVM, the > propery is silently ignored and the feature is not announced to the > guest. > > Refuse to start with an error instead. > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> Something I didn't consider before: Will this block migration before it even starts, or will crash the VM only after all migration data was sent to the destination? I didn't test it, but kvm_arch_init_vcpu() seems to be too late to block an invalid/unsupport configuration. Maybe we can simply call hyperv_handle_properties() earlier, inside x86_cpu_realizefn()? (I know it's very late for this kind of intrusive change in v2.12, but I still think it's a good idea to fix this as soon as possible.) > --- > v1 -> v2: > - indicate what flag requested the feature that can't be enabled in the > error message > - fix a typo in the error message for VP_RUNTIME > > target/i386/kvm.c | 32 ++++++++++++++++++++++++++++---- > 1 file changed, 28 insertions(+), 4 deletions(-) > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index b35623ae24..113926aff2 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -659,17 +659,41 @@ static int hyperv_handle_properties(CPUState *cs) > env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS; > env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE; > } > - if (cpu->hyperv_crash && has_msr_hv_crash) { > + if (cpu->hyperv_crash) { > + if (!has_msr_hv_crash) { > + fprintf(stderr, "Hyper-V crash MSRs " > + "(requested by 'hv-crash' cpu flag) " > + "are not supported by kernel\n"); > + return -ENOSYS; > + } > env->features[FEAT_HYPERV_EDX] |= HV_GUEST_CRASH_MSR_AVAILABLE; > } > env->features[FEAT_HYPERV_EDX] |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE; > - if (cpu->hyperv_reset && has_msr_hv_reset) { > + if (cpu->hyperv_reset) { > + if (!has_msr_hv_reset) { > + fprintf(stderr, "Hyper-V reset MSR " > + "(requested by 'hv-reset' cpu flag) " > + "is not supported by kernel\n"); > + return -ENOSYS; > + } > env->features[FEAT_HYPERV_EAX] |= HV_RESET_AVAILABLE; > } > - if (cpu->hyperv_vpindex && has_msr_hv_vpindex) { > + if (cpu->hyperv_vpindex) { > + if (!has_msr_hv_vpindex) { > + fprintf(stderr, "Hyper-V VP_INDEX MSR " > + "(requested by 'hv-vpindex' cpu flag) " > + "is not supported by kernel\n"); > + return -ENOSYS; > + } > env->features[FEAT_HYPERV_EAX] |= HV_VP_INDEX_AVAILABLE; > } > - if (cpu->hyperv_runtime && has_msr_hv_runtime) { > + if (cpu->hyperv_runtime) { > + if (!has_msr_hv_runtime) { > + fprintf(stderr, "Hyper-V VP_RUNTIME MSR " > + "(requested by 'hv-runtime' cpu flag) " > + "is not supported by kernel\n"); > + return -ENOSYS; > + } > env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE; > } > if (cpu->hyperv_synic) { > -- > 2.14.3 >
On Wed, Mar 28, 2018 at 03:53:31PM -0300, Eduardo Habkost wrote: > On Wed, Mar 28, 2018 at 06:30:24PM +0300, Roman Kagan wrote: > > In order to guarantee compatibility on migration, QEMU should have > > complete control over the features it announces to the guest via CPUID. > > > > However, for a number of Hyper-V-related cpu properties, if the > > corresponding feature is not supported by the underlying KVM, the > > propery is silently ignored and the feature is not announced to the > > guest. > > > > Refuse to start with an error instead. > > > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > > Something I didn't consider before: > > Will this block migration before it even starts, or will crash > the VM only after all migration data was sent to the destination? > > I didn't test it, but kvm_arch_init_vcpu() seems to be too late > to block an invalid/unsupport configuration. I just did a simple test, force-failing one of the checks and starting QEMU with -incoming defer. It refused to start with the expected error message. IOW in the migration case the destination will abort before the source have started to send any data. > Maybe we can simply call hyperv_handle_properties() earlier, > inside x86_cpu_realizefn()? Now it's ... x86_cpu_realizefn qemu_init_vcpu qemu_kvm_start_vcpu qemu_thread_create(qemu_kvm_cpu_thread_fn) [in vcpu thread] qemu_kvm_cpu_thread_fn kvm_init_vcpu kvm_arch_init_vcpu hyperv_handle_properties [error return] [error return] [error return] exit(1) > (I know it's very late for this kind of intrusive change in > v2.12, but I still think it's a good idea to fix this as soon as > possible.) I agree that the current hyperv flag handling begs for cleanup but I think this can wait for post-2.12. > > --- > > v1 -> v2: > > - indicate what flag requested the feature that can't be enabled in the > > error message > > - fix a typo in the error message for VP_RUNTIME I just noticed that I missed hv-time being silently cleared, too (just in a slightly different pattern), so I'll have to respin. Thanks, Roman.
On Thu, Mar 29, 2018 at 12:47:42PM +0300, Roman Kagan wrote: > On Wed, Mar 28, 2018 at 03:53:31PM -0300, Eduardo Habkost wrote: > > On Wed, Mar 28, 2018 at 06:30:24PM +0300, Roman Kagan wrote: > > > In order to guarantee compatibility on migration, QEMU should have > > > complete control over the features it announces to the guest via CPUID. > > > > > > However, for a number of Hyper-V-related cpu properties, if the > > > corresponding feature is not supported by the underlying KVM, the > > > propery is silently ignored and the feature is not announced to the > > > guest. > > > > > > Refuse to start with an error instead. > > > > > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > > > > Something I didn't consider before: > > > > Will this block migration before it even starts, or will crash > > the VM only after all migration data was sent to the destination? > > > > I didn't test it, but kvm_arch_init_vcpu() seems to be too late > > to block an invalid/unsupport configuration. > > I just did a simple test, force-failing one of the checks and starting > QEMU with -incoming defer. It refused to start with the expected error > message. IOW in the migration case the destination will abort before > the source have started to send any data. > > > Maybe we can simply call hyperv_handle_properties() earlier, > > inside x86_cpu_realizefn()? > > Now it's > > ... > x86_cpu_realizefn > qemu_init_vcpu > qemu_kvm_start_vcpu > qemu_thread_create(qemu_kvm_cpu_thread_fn) > [in vcpu thread] > qemu_kvm_cpu_thread_fn Oh, this is the part that I missed. I thought the vCPU thread wouldn't start until migration was finished. This means the patch is OK as-is. Sorry for the confusion. > kvm_init_vcpu > kvm_arch_init_vcpu > hyperv_handle_properties > [error return] > [error return] > [error return] > exit(1) > > > (I know it's very late for this kind of intrusive change in > > v2.12, but I still think it's a good idea to fix this as soon as > > possible.) > > I agree that the current hyperv flag handling begs for cleanup but I > think this can wait for post-2.12. > > > > --- > > > v1 -> v2: > > > - indicate what flag requested the feature that can't be enabled in the > > > error message > > > - fix a typo in the error message for VP_RUNTIME > > I just noticed that I missed hv-time being silently cleared, too (just > in a slightly different pattern), so I'll have to respin. > > Thanks, > Roman.
diff --git a/target/i386/kvm.c b/target/i386/kvm.c index b35623ae24..113926aff2 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -659,17 +659,41 @@ static int hyperv_handle_properties(CPUState *cs) env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS; env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE; } - if (cpu->hyperv_crash && has_msr_hv_crash) { + if (cpu->hyperv_crash) { + if (!has_msr_hv_crash) { + fprintf(stderr, "Hyper-V crash MSRs " + "(requested by 'hv-crash' cpu flag) " + "are not supported by kernel\n"); + return -ENOSYS; + } env->features[FEAT_HYPERV_EDX] |= HV_GUEST_CRASH_MSR_AVAILABLE; } env->features[FEAT_HYPERV_EDX] |= HV_CPU_DYNAMIC_PARTITIONING_AVAILABLE; - if (cpu->hyperv_reset && has_msr_hv_reset) { + if (cpu->hyperv_reset) { + if (!has_msr_hv_reset) { + fprintf(stderr, "Hyper-V reset MSR " + "(requested by 'hv-reset' cpu flag) " + "is not supported by kernel\n"); + return -ENOSYS; + } env->features[FEAT_HYPERV_EAX] |= HV_RESET_AVAILABLE; } - if (cpu->hyperv_vpindex && has_msr_hv_vpindex) { + if (cpu->hyperv_vpindex) { + if (!has_msr_hv_vpindex) { + fprintf(stderr, "Hyper-V VP_INDEX MSR " + "(requested by 'hv-vpindex' cpu flag) " + "is not supported by kernel\n"); + return -ENOSYS; + } env->features[FEAT_HYPERV_EAX] |= HV_VP_INDEX_AVAILABLE; } - if (cpu->hyperv_runtime && has_msr_hv_runtime) { + if (cpu->hyperv_runtime) { + if (!has_msr_hv_runtime) { + fprintf(stderr, "Hyper-V VP_RUNTIME MSR " + "(requested by 'hv-runtime' cpu flag) " + "is not supported by kernel\n"); + return -ENOSYS; + } env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE; } if (cpu->hyperv_synic) {
In order to guarantee compatibility on migration, QEMU should have complete control over the features it announces to the guest via CPUID. However, for a number of Hyper-V-related cpu properties, if the corresponding feature is not supported by the underlying KVM, the propery is silently ignored and the feature is not announced to the guest. Refuse to start with an error instead. Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> --- v1 -> v2: - indicate what flag requested the feature that can't be enabled in the error message - fix a typo in the error message for VP_RUNTIME target/i386/kvm.c | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-)