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-sta...@nongnu.org > > Signed-off-by: Roman Kagan <rka...@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.