From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37850) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1f1BuC-0001Jj-Rn for qemu-devel@nongnu.org; Wed, 28 Mar 2018 10:18:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1f1Bu9-0000dH-Kq for qemu-devel@nongnu.org; Wed, 28 Mar 2018 10:18:24 -0400 Date: Wed, 28 Mar 2018 11:18:18 -0300 From: Eduardo Habkost Message-ID: <20180328141818.GG5046@localhost.localdomain> References: <20180323125808.4479-1-rkagan@virtuozzo.com> <20180323125808.4479-3-rkagan@virtuozzo.com> <20180323195621.GN3417@localhost.localdomain> <20180326150615.GD2401@rkaganb.sw.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180326150615.GD2401@rkaganb.sw.ru> Subject: Re: [Qemu-devel] [PATCH for-2.12 2/2] i386/hyperv: error out if features requested but unsupported List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Kagan , qemu-devel@nongnu.org, Paolo Bonzini , Vitaly Kuznetsov , qemu-stable@nongnu.org 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 > > > > 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