* [Qemu-devel] [PATCH for-2.12 0/2] i386/hyperv: fully control Hyper-V features in CPUID @ 2018-03-23 12:58 Roman Kagan 2018-03-23 12:58 ` [Qemu-devel] [PATCH for-2.12 1/2] i386/hyperv: add hv-frequencies cpu property Roman Kagan ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: Roman Kagan @ 2018-03-23 12:58 UTC (permalink / raw) To: qemu-devel, Paolo Bonzini, Eduardo Habkost, Vitaly Kuznetsov; +Cc: qemu-stable In order to guarantee compatibility on migration, QEMU should have complete control over the features it announces to the guest via CPUID. However, a number of Hyper-V-related features happen to depend on the support in the underlying KVM, with no regard to QEMU configuration. Make QEMU regain control over what Hyper-V features it announces to the guest. Note: the patches are also being proposed for stable-2.11, even though one of them introduces a new cpu property. This is done to minimize the number of published QEMU releases where the behavior of the features is unpredictable, with potentially fatal consequences for the guest. Note #2: there are other problems in the surrounding code, like ugly error reporting or inconsistent population of MSRs. I think this can be put off to post-2.12. Roman Kagan (2): i386/hyperv: add hv-frequencies cpu property i386/hyperv: error out if features requested but unsupported Cc: qemu-stable@nongnu.org target/i386/cpu.h | 1 + target/i386/cpu.c | 1 + target/i386/kvm.c | 37 +++++++++++++++++++++++++++++-------- 3 files changed, 31 insertions(+), 8 deletions(-) -- 2.14.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH for-2.12 1/2] i386/hyperv: add hv-frequencies cpu property 2018-03-23 12:58 [Qemu-devel] [PATCH for-2.12 0/2] i386/hyperv: fully control Hyper-V features in CPUID Roman Kagan @ 2018-03-23 12:58 ` Roman Kagan 2018-03-23 19:41 ` Eduardo Habkost 2018-03-23 12:58 ` [Qemu-devel] [PATCH for-2.12 2/2] i386/hyperv: error out if features requested but unsupported Roman Kagan 2018-03-23 14:02 ` [Qemu-devel] [PATCH for-2.12 0/2] i386/hyperv: fully control Hyper-V features in CPUID no-reply 2 siblings, 1 reply; 8+ messages in thread From: Roman Kagan @ 2018-03-23 12:58 UTC (permalink / raw) To: qemu-devel, Paolo Bonzini, Eduardo Habkost, Vitaly Kuznetsov; +Cc: qemu-stable In order to guarantee compatibility on migration, QEMU should have complete control over the features it announces to the guest via CPUID. However, the declared availability of Hyper-V frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and HV_X64_MSR_APIC_FREQUENCY) depends solely on the support for them in the underlying KVM. Introduce "hv-frequencies" cpu property (off by default) which gives QEMU full control over whether these MSRs are announced. While at this, drop the redundant check of the cpu tsc frequency, and decouple this feature from hv-time. Cc: qemu-stable@nongnu.org Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> --- Note: this patch introduces a new cpu property, which is not what we normally do in stable branches. However, this appears to be the minimal effort/churn approach to reduce the number of published QEMU releases where the behavior of the feature is unpredictable, with potentially fatal consequences for the guest. target/i386/cpu.h | 1 + target/i386/cpu.c | 1 + target/i386/kvm.c | 12 ++++++++---- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/target/i386/cpu.h b/target/i386/cpu.h index 78db1b833a..1b219fafc4 100644 --- a/target/i386/cpu.h +++ b/target/i386/cpu.h @@ -1296,6 +1296,7 @@ struct X86CPU { bool hyperv_runtime; bool hyperv_synic; bool hyperv_stimer; + bool hyperv_frequencies; bool check_cpuid; bool enforce_cpuid; bool expose_kvm; diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 555ae79d29..1a6b082b6f 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -4761,6 +4761,7 @@ static Property x86_cpu_properties[] = { DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false), DEFINE_PROP_BOOL("hv-synic", X86CPU, hyperv_synic, false), DEFINE_PROP_BOOL("hv-stimer", X86CPU, hyperv_stimer, false), + DEFINE_PROP_BOOL("hv-frequencies", X86CPU, hyperv_frequencies, false), DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true), DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false), DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true), diff --git a/target/i386/kvm.c b/target/i386/kvm.c index d23fff12f5..fb20ff18c2 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -648,11 +648,15 @@ static int hyperv_handle_properties(CPUState *cs) env->features[FEAT_HYPERV_EAX] |= HV_HYPERCALL_AVAILABLE; env->features[FEAT_HYPERV_EAX] |= HV_TIME_REF_COUNT_AVAILABLE; env->features[FEAT_HYPERV_EAX] |= HV_REFERENCE_TSC_AVAILABLE; - - if (has_msr_hv_frequencies && tsc_is_stable_and_known(env)) { - env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS; - env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE; + } + if (cpu->hyperv_frequencies) { + if (!has_msr_hv_frequencies) { + fprintf(stderr, + "Hyper-V frequency MSRs are not supported by kernel\n"); + return -ENOSYS; } + 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) { env->features[FEAT_HYPERV_EDX] |= HV_GUEST_CRASH_MSR_AVAILABLE; -- 2.14.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 1/2] i386/hyperv: add hv-frequencies cpu property 2018-03-23 12:58 ` [Qemu-devel] [PATCH for-2.12 1/2] i386/hyperv: add hv-frequencies cpu property Roman Kagan @ 2018-03-23 19:41 ` Eduardo Habkost 0 siblings, 0 replies; 8+ messages in thread From: Eduardo Habkost @ 2018-03-23 19:41 UTC (permalink / raw) To: Roman Kagan; +Cc: qemu-devel, Paolo Bonzini, Vitaly Kuznetsov, qemu-stable On Fri, Mar 23, 2018 at 03:58:07PM +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, the declared availability of Hyper-V frequency MSRs > (HV_X64_MSR_TSC_FREQUENCY and HV_X64_MSR_APIC_FREQUENCY) depends solely > on the support for them in the underlying KVM. So this problem was introduced fairly recently (in v2.11). This makes the decision to break compatibility of (some[1]) existing configurations that didn't specify "hv-frequency" a bit easier. > Introduce "hv-frequencies" cpu property (off by default) which gives > QEMU full control over whether these MSRs are announced. > So, as we have two possible results when running QEMU-2.11, we need to make a guess and choose which half of our users will be affected: a) People running "-machine pc-2.11 -cpu ...,+hv-time" on Linux 4.14+ (including commit 72c139bacfa3), that have hv-frequencies enabled automatically. b) People running "-machine pc-2.11 -cpu ...,+hv-time" on Linux 4.13 and older (without commit 72c139bacfa3), that have hv-frequencies disabled. If we set hv-frequencies=off by default on pc-2.11 (this patch), we will inconvenience group (a). The consequence for them is having hv-frequencies disabled suddenly on CPUID after updating QEMU. The MSRs will still be available to the guest, however (so the guest won't crash), and they can add hv-frequencies=on to their configuration manually. If we set hv-frequencies=on by default on pc-2.11, we will inconvenience group (b). The consequence for them is having the VM not being runnable anymore until they change the machine-type or add hv-frequencies=off to their configuration. So it looks like this patch is the safest solution, but I will get back to the "[PATCH v3 2/2] i386/kvm: lower requirements for Hyper-V frequency MSRs exposure" thread to be sure we are not missing anything. > While at this, drop the redundant check of the cpu tsc frequency, and > decouple this feature from hv-time. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > --- > Note: this patch introduces a new cpu property, which is not what we > normally do in stable branches. However, this appears to be the minimal > effort/churn approach to reduce the number of published QEMU releases > where the behavior of the feature is unpredictable, with potentially > fatal consequences for the guest. > > target/i386/cpu.h | 1 + > target/i386/cpu.c | 1 + > target/i386/kvm.c | 12 ++++++++---- > 3 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h > index 78db1b833a..1b219fafc4 100644 > --- a/target/i386/cpu.h > +++ b/target/i386/cpu.h > @@ -1296,6 +1296,7 @@ struct X86CPU { > bool hyperv_runtime; > bool hyperv_synic; > bool hyperv_stimer; > + bool hyperv_frequencies; > bool check_cpuid; > bool enforce_cpuid; > bool expose_kvm; > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 555ae79d29..1a6b082b6f 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -4761,6 +4761,7 @@ static Property x86_cpu_properties[] = { > DEFINE_PROP_BOOL("hv-runtime", X86CPU, hyperv_runtime, false), > DEFINE_PROP_BOOL("hv-synic", X86CPU, hyperv_synic, false), > DEFINE_PROP_BOOL("hv-stimer", X86CPU, hyperv_stimer, false), > + DEFINE_PROP_BOOL("hv-frequencies", X86CPU, hyperv_frequencies, false), > DEFINE_PROP_BOOL("check", X86CPU, check_cpuid, true), > DEFINE_PROP_BOOL("enforce", X86CPU, enforce_cpuid, false), > DEFINE_PROP_BOOL("kvm", X86CPU, expose_kvm, true), > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index d23fff12f5..fb20ff18c2 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -648,11 +648,15 @@ static int hyperv_handle_properties(CPUState *cs) > env->features[FEAT_HYPERV_EAX] |= HV_HYPERCALL_AVAILABLE; > env->features[FEAT_HYPERV_EAX] |= HV_TIME_REF_COUNT_AVAILABLE; > env->features[FEAT_HYPERV_EAX] |= HV_REFERENCE_TSC_AVAILABLE; > - > - if (has_msr_hv_frequencies && tsc_is_stable_and_known(env)) { > - env->features[FEAT_HYPERV_EAX] |= HV_ACCESS_FREQUENCY_MSRS; > - env->features[FEAT_HYPERV_EDX] |= HV_FREQUENCY_MSRS_AVAILABLE; > + } > + if (cpu->hyperv_frequencies) { > + if (!has_msr_hv_frequencies) { > + fprintf(stderr, > + "Hyper-V frequency MSRs are not supported by kernel\n"); > + return -ENOSYS; > } > + 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) { > env->features[FEAT_HYPERV_EDX] |= HV_GUEST_CRASH_MSR_AVAILABLE; > -- > 2.14.3 > -- Eduardo ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH for-2.12 2/2] i386/hyperv: error out if features requested but unsupported 2018-03-23 12:58 [Qemu-devel] [PATCH for-2.12 0/2] i386/hyperv: fully control Hyper-V features in CPUID Roman Kagan 2018-03-23 12:58 ` [Qemu-devel] [PATCH for-2.12 1/2] i386/hyperv: add hv-frequencies cpu property Roman Kagan @ 2018-03-23 12:58 ` Roman Kagan 2018-03-23 19:56 ` Eduardo Habkost 2018-03-23 14:02 ` [Qemu-devel] [PATCH for-2.12 0/2] i386/hyperv: fully control Hyper-V features in CPUID no-reply 2 siblings, 1 reply; 8+ messages in thread From: Roman Kagan @ 2018-03-23 12:58 UTC (permalink / raw) To: qemu-devel, Paolo Bonzini, Eduardo Habkost, Vitaly Kuznetsov; +Cc: qemu-stable 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. Cc: qemu-stable@nongnu.org Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> --- target/i386/kvm.c | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/target/i386/kvm.c b/target/i386/kvm.c index fb20ff18c2..c9c359241c 100644 --- a/target/i386/kvm.c +++ b/target/i386/kvm.c @@ -658,17 +658,34 @@ 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 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 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 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_INDEX is not supported by kernel\n"); + return -ENOSYS; + } env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE; } if (cpu->hyperv_synic) { -- 2.14.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 2/2] i386/hyperv: error out if features requested but unsupported 2018-03-23 12:58 ` [Qemu-devel] [PATCH for-2.12 2/2] i386/hyperv: error out if features requested but unsupported Roman Kagan @ 2018-03-23 19:56 ` Eduardo Habkost 2018-03-26 15:06 ` Roman Kagan 0 siblings, 1 reply; 8+ messages in thread From: Eduardo Habkost @ 2018-03-23 19:56 UTC (permalink / raw) To: Roman Kagan; +Cc: qemu-devel, Paolo Bonzini, Vitaly Kuznetsov, qemu-stable On Fri, Mar 23, 2018 at 03:58:08PM +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. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> I wonder if we should make these just warnings on -stable, and make them fatal errors only on 2.12. I wouldn't want to make existing running VMs not runnable on a stable update. > --- > target/i386/kvm.c | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > index fb20ff18c2..c9c359241c 100644 > --- a/target/i386/kvm.c > +++ b/target/i386/kvm.c > @@ -658,17 +658,34 @@ 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 are not supported by kernel\n"); I would mention the corresponding "hv-..." -cpu option explicitly, for clarity. > + 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 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 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_INDEX is not supported by kernel\n"); > + return -ENOSYS; > + } > env->features[FEAT_HYPERV_EAX] |= HV_VP_RUNTIME_AVAILABLE; > } > if (cpu->hyperv_synic) { > -- > 2.14.3 > -- Eduardo ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 2/2] i386/hyperv: error out if features requested but unsupported 2018-03-23 19:56 ` Eduardo Habkost @ 2018-03-26 15:06 ` Roman Kagan 2018-03-28 14:18 ` Eduardo Habkost 0 siblings, 1 reply; 8+ messages in thread From: Roman Kagan @ 2018-03-26 15:06 UTC (permalink / raw) To: Eduardo Habkost; +Cc: qemu-devel, Paolo Bonzini, Vitaly Kuznetsov, qemu-stable On Fri, Mar 23, 2018 at 04:56:21PM -0300, Eduardo Habkost wrote: > On Fri, Mar 23, 2018 at 03:58:08PM +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. > > > > Cc: qemu-stable@nongnu.org > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > > I wonder if we should make these just warnings on -stable, and > make them fatal errors only on 2.12. I wouldn't want to make > existing running VMs not runnable on a stable update. OK let's follow your approach and consider who would suffer more: a) users who started a VM on a newer kernel and then migrated to an older one, and got a guest crash on an unsupported MSR (I'm not even sure they'd be able to see the warnings) b) users who had a VM configuration with features which didn't actually work (but that wasn't apparently a problem for them), and suddenly couldn't start their VM after QEMU upgrade (the problem is not only cold-restarting per se, but also live migration to the upgraded version: dunno if the management layer would allow to adjust the VM config to migrate successfully). I don't have a strong opinion on this, will follow whatever direction you advise. > > target/i386/kvm.c | 25 +++++++++++++++++++++---- > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > > index fb20ff18c2..c9c359241c 100644 > > --- a/target/i386/kvm.c > > +++ b/target/i386/kvm.c > > @@ -658,17 +658,34 @@ 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 are not supported by kernel\n"); > > I would mention the corresponding "hv-..." -cpu option > explicitly, for clarity. This sounds like a good idea, but I wonder if it should better be done uniformly for all hv_* options, together with transitioning to error_report(), and whether we want to do this at this point in the release cycle. Thanks, Roman. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 2/2] i386/hyperv: error out if features requested but unsupported 2018-03-26 15:06 ` Roman Kagan @ 2018-03-28 14:18 ` Eduardo Habkost 0 siblings, 0 replies; 8+ messages in thread From: Eduardo Habkost @ 2018-03-28 14:18 UTC (permalink / raw) To: Roman Kagan, qemu-devel, Paolo Bonzini, Vitaly Kuznetsov, qemu-stable On Mon, Mar 26, 2018 at 06:06:16PM +0300, Roman Kagan wrote: > On Fri, Mar 23, 2018 at 04:56:21PM -0300, Eduardo Habkost wrote: > > On Fri, Mar 23, 2018 at 03:58:08PM +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. > > > > > > Cc: qemu-stable@nongnu.org > > > Signed-off-by: Roman Kagan <rkagan@virtuozzo.com> > > > > I wonder if we should make these just warnings on -stable, and > > make them fatal errors only on 2.12. I wouldn't want to make > > existing running VMs not runnable on a stable update. > > OK let's follow your approach and consider who would suffer more: > > a) users who started a VM on a newer kernel and then migrated to an > older one, and got a guest crash on an unsupported MSR (I'm not even > sure they'd be able to see the warnings) > > b) users who had a VM configuration with features which didn't actually > work (but that wasn't apparently a problem for them), and suddenly > couldn't start their VM after QEMU upgrade (the problem is not only > cold-restarting per se, but also live migration to the upgraded > version: dunno if the management layer would allow to adjust the VM > config to migrate successfully). > > I don't have a strong opinion on this, will follow whatever direction > you advise. (a) is an existing bug (and rare, it seems: I didn't see it being reported anywhere). (b) would be a regression in a -stable update. I'd prefer to be conservative on -stable. > > > > > target/i386/kvm.c | 25 +++++++++++++++++++++---- > > > 1 file changed, 21 insertions(+), 4 deletions(-) > > > > > > diff --git a/target/i386/kvm.c b/target/i386/kvm.c > > > index fb20ff18c2..c9c359241c 100644 > > > --- a/target/i386/kvm.c > > > +++ b/target/i386/kvm.c > > > @@ -658,17 +658,34 @@ 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 are not supported by kernel\n"); > > > > I would mention the corresponding "hv-..." -cpu option > > explicitly, for clarity. > > This sounds like a good idea, but I wonder if it should better be done > uniformly for all hv_* options, together with transitioning to > error_report(), and whether we want to do this at this point in the > release cycle. I don't think it will be a problem if we mention the property names in the new messages, but not try to reword the existing ones. -- Eduardo ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.12 0/2] i386/hyperv: fully control Hyper-V features in CPUID 2018-03-23 12:58 [Qemu-devel] [PATCH for-2.12 0/2] i386/hyperv: fully control Hyper-V features in CPUID Roman Kagan 2018-03-23 12:58 ` [Qemu-devel] [PATCH for-2.12 1/2] i386/hyperv: add hv-frequencies cpu property Roman Kagan 2018-03-23 12:58 ` [Qemu-devel] [PATCH for-2.12 2/2] i386/hyperv: error out if features requested but unsupported Roman Kagan @ 2018-03-23 14:02 ` no-reply 2 siblings, 0 replies; 8+ messages in thread From: no-reply @ 2018-03-23 14:02 UTC (permalink / raw) To: rkagan; +Cc: famz, qemu-devel, pbonzini, ehabkost, vkuznets, qemu-stable Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20180323125808.4479-1-rkagan@virtuozzo.com Subject: [Qemu-devel] [PATCH for-2.12 0/2] i386/hyperv: fully control Hyper-V features in CPUID === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 2efbb18fe2 i386/hyperv: error out if features requested but unsupported 31e7467f4e i386/hyperv: add hv-frequencies cpu property === OUTPUT BEGIN === Checking PATCH 1/2: i386/hyperv: add hv-frequencies cpu property... Checking PATCH 2/2: i386/hyperv: error out if features requested but unsupported... ERROR: suspect code indent for conditional statements (4, 7) #39: FILE: target/i386/kvm.c:670: + if (cpu->hyperv_reset) { + if (!has_msr_hv_reset) { total: 1 errors, 0 warnings, 38 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-03-28 14:18 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-23 12:58 [Qemu-devel] [PATCH for-2.12 0/2] i386/hyperv: fully control Hyper-V features in CPUID Roman Kagan 2018-03-23 12:58 ` [Qemu-devel] [PATCH for-2.12 1/2] i386/hyperv: add hv-frequencies cpu property Roman Kagan 2018-03-23 19:41 ` Eduardo Habkost 2018-03-23 12:58 ` [Qemu-devel] [PATCH for-2.12 2/2] i386/hyperv: error out if features requested but unsupported Roman Kagan 2018-03-23 19:56 ` Eduardo Habkost 2018-03-26 15:06 ` Roman Kagan 2018-03-28 14:18 ` Eduardo Habkost 2018-03-23 14:02 ` [Qemu-devel] [PATCH for-2.12 0/2] i386/hyperv: fully control Hyper-V features in CPUID no-reply
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.