Re: [Qemu-devel] [PATCH for-2.12 2/2] i386/hyperv: error out if features requested but unsupported

[Eduardo Habkost <ehabkost@redhat.com>]

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