From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.9 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B724DC433E0 for ; Thu, 21 Jan 2021 16:26:43 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 2A89522B45 for ; Thu, 21 Jan 2021 16:26:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2A89522B45 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:51898 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1l2cnC-0000ZF-1l for qemu-devel@archiver.kernel.org; Thu, 21 Jan 2021 11:26:42 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:41302) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1l2ckd-00082R-Rt for qemu-devel@nongnu.org; Thu, 21 Jan 2021 11:24:03 -0500 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:52893) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.90_1) (envelope-from ) id 1l2ckb-0007Su-F2 for qemu-devel@nongnu.org; Thu, 21 Jan 2021 11:24:03 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611246240; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=SnnH4/0w3mBJoXY+u6+16cRpCpAYoOwmqc9fe+wgnj4=; b=Lfzq3XPYImGZqZeFBENS9tckQFMADZgDoe6DpfmfAb6gL+0dQbhBWu0j3n9jhjMhFFWJGv 3+cW7EpFUF3LAxO8S6bbnr4tSjeDtnsxYwYznDJNdFqkI+1DdZZgjpHR8vLp4fAGq3TY9R gSe/2Cd2zGwIs236oge3uyCA95CARCk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-565-8bSi8xZRNrub4iWwhNJPNA-1; Thu, 21 Jan 2021 11:23:58 -0500 X-MC-Unique: 8bSi8xZRNrub4iWwhNJPNA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6F80859 for ; Thu, 21 Jan 2021 16:23:57 +0000 (UTC) Received: from localhost (unknown [10.40.208.9]) by smtp.corp.redhat.com (Postfix) with ESMTP id B4E116E515; Thu, 21 Jan 2021 16:23:55 +0000 (UTC) Date: Thu, 21 Jan 2021 17:23:53 +0100 From: Igor Mammedov To: Eduardo Habkost Subject: Re: [PATCH v3 18/19] i386: provide simple 'hv-default=on' option Message-ID: <20210121172353.4649bb18@redhat.com> In-Reply-To: <20210121142704.1a150cac@redhat.com> References: <20210107150640.539239-1-vkuznets@redhat.com> <20210107151449.541062-1-vkuznets@redhat.com> <20210115031142.7c171a7f@redhat.com> <87h7ni7e08.fsf@vitty.brq.redhat.com> <20210120141312.0a1e6c33@redhat.com> <874kjb65cm.fsf@vitty.brq.redhat.com> <20210120200832.40141dc1@redhat.com> <20210120204909.GS1227584@habkost.net> <20210121142704.1a150cac@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=imammedo@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=216.205.24.124; envelope-from=imammedo@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -29 X-Spam_score: -3.0 X-Spam_bar: --- X-Spam_report: (-3.0 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.168, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Paolo Bonzini , Vitaly Kuznetsov , Marcelo Tosatti , qemu-devel@nongnu.org Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On Thu, 21 Jan 2021 14:27:04 +0100 Igor Mammedov wrote: > On Wed, 20 Jan 2021 15:49:09 -0500 > Eduardo Habkost wrote: > > > On Wed, Jan 20, 2021 at 08:08:32PM +0100, Igor Mammedov wrote: > > > On Wed, 20 Jan 2021 15:38:33 +0100 > > > Vitaly Kuznetsov wrote: > > > > > > > Igor Mammedov writes: > > > > > > > > > On Fri, 15 Jan 2021 10:20:23 +0100 > > > > > Vitaly Kuznetsov wrote: > > > > > > > > > >> Igor Mammedov writes: > > > > >> > > > > >> > On Thu, 7 Jan 2021 16:14:49 +0100 > > > > >> > Vitaly Kuznetsov wrote: > > > > >> > > > > > >> >> Enabling Hyper-V emulation for a Windows VM is a tiring experience as it > > > > >> >> requires listing all currently supported enlightenments ("hv-*" CPU > > > > >> >> features) explicitly. We do have 'hv-passthrough' mode enabling > > > > >> >> everything but it can't be used in production as it prevents migration. > > > > >> >> > > > > >> >> Introduce a simple 'hv-default=on' CPU flag enabling all currently supported > > > > >> >> Hyper-V enlightenments. Later, when new enlightenments get implemented, > > > > >> >> compat_props mechanism will be used to disable them for legacy machine types, > > > > >> >> this will keep 'hv-default=on' configurations migratable. > > > > >> >> > > > > >> >> Signed-off-by: Vitaly Kuznetsov > > > > >> >> --- > > > > >> >> docs/hyperv.txt | 16 +++++++++++++--- > > > > >> >> target/i386/cpu.c | 38 ++++++++++++++++++++++++++++++++++++++ > > > > >> >> target/i386/cpu.h | 5 +++++ > > > > >> >> 3 files changed, 56 insertions(+), 3 deletions(-) > > > > >> >> > > > > >> >> diff --git a/docs/hyperv.txt b/docs/hyperv.txt > > > > >> >> index 5df00da54fc4..a54c066cab09 100644 > > > > >> >> --- a/docs/hyperv.txt > > > > >> >> +++ b/docs/hyperv.txt > > > > >> >> @@ -17,10 +17,20 @@ compatible hypervisor and use Hyper-V specific features. > > > > >> >> > > > > >> >> 2. Setup > > > > >> >> ========= > > > > >> >> -No Hyper-V enlightenments are enabled by default by either KVM or QEMU. In > > > > >> >> -QEMU, individual enlightenments can be enabled through CPU flags, e.g: > > > > >> >> +All currently supported Hyper-V enlightenments can be enabled by specifying > > > > >> >> +'hv-default=on' CPU flag: > > > > >> >> > > > > >> >> - qemu-system-x86_64 --enable-kvm --cpu host,hv_relaxed,hv_vpindex,hv_time, ... > > > > >> >> + qemu-system-x86_64 --enable-kvm --cpu host,hv-default ... > > > > >> >> + > > > > >> >> +Alternatively, it is possible to do fine-grained enablement through CPU flags, > > > > >> >> +e.g: > > > > >> >> + > > > > >> >> + qemu-system-x86_64 --enable-kvm --cpu host,hv-relaxed,hv-vpindex,hv-time ... > > > > >> > > > > > >> > I'd put here not '...' but rather recommended list of flags, and update > > > > >> > it every time when new feature added if necessary. > > > > >> > > > > > > > > > > > 1) > > > > > > > > > >> This is an example of fine-grained enablement, there is no point to put > > > > >> all the existing flags there (hv-default is the only recommended way > > > > >> now, the rest is 'expert'/'debugging'). > > > > > so users are kept in dark what hv-default disables/enables (and it might depend > > > > > on machine version on top that). Doesn't look like a good documentation to me > > > > > (sure everyone can go and read source code for it and try to figure out how > > > > > it's supposed to work) > > > > > > > > 'hv-default' enables *all* currently supported enlightenments. When > > > > using with an old machine type, it will enable *all* Hyper-V > > > > enlightenmnets which were supported when the corresponding machine type > > > > was released. I don't think we document all other cases when a machine > > > > type is modified (i.e. where can I read how pc-q35-5.1 is different from > > > > pc-q35-5.0 if I refuse to read the source code?) > > > > > > > > > > > > > >> > > > > >> > (not to mention that if we had it to begin with, then new 'hv-default' won't > > > > >> > be necessary, I still see it as functionality duplication but I will not oppose it) > > > > >> > > > > > >> > > > > >> Unfortunately, upper layer tools don't read this doc and update > > > > >> themselves to enable new features when they appear. > > > > > rant: (just merge all libvirt into QEMU, and make VM configuration less low-level. > > > > > why stop there, just merge with yet another upper layer, it would save us a lot > > > > > on communication protocols and simplify VM creation even more, > > > > > and no one will have to read docs and write anything new on top.) > > > > > There should be limit somewhere, where QEMU job ends and others pile hw abstraction > > > > > layers on top of it. > > > > > > > > We have '-machine q35' and we don't require to list all the devices from > > > > it. We have '-cpu Skylake-Server' and we don't require to configure all > > > > the features manually. Why can't we have similar enablement for Hyper-V > > > > emulation where we can't even see a real need for anything but 'enable > > > > everything' option? > > > > > > > > There is no 'one libvirt to rule them all' (fortunately or > > > > unfortunately). And sometimes QEMU is the uppermost layer and there's no > > > > 'libvirt' on top of it, this is also a perfectly valid use-case. > > > > > > > > > > > > > >> Similarly, if when these tools use '-machine q35' they get all the new features we add > > > > >> automatically, right? > > > > > it depends, in case of CPUs, new features usually 'off' by default > > > > > for existing models. In case of bugs, features sometimes could be > > > > > flipped and versioned machines were used to keep broken CPU models > > > > > on old machine types. > > > > > > > > > > > > > That's why I was saying that Hyper-V enlightenments hardly resemble > > > > 'hardware' CPU features. > > > Well, Microsoft chose to implement them as hardware concept (CPUID leaf), > > > and I prefer to treat them the same way as any other CPUID bits. > > > > > > > > > > > > > > > > >> >> +It is also possible to disable individual enlightenments from the default list, > > > > >> >> +this can be used for debugging purposes: > > > > >> >> + > > > > >> >> + qemu-system-x86_64 --enable-kvm --cpu host,hv-default=on,hv-evmcs=off ... > > > > >> >> > > > > >> >> Sometimes there are dependencies between enlightenments, QEMU is supposed to > > > > >> >> check that the supplied configuration is sane. > > > > >> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c > > > > >> >> index 48007a876e32..99338de00f78 100644 > > > > >> >> --- a/target/i386/cpu.c > > > > >> >> +++ b/target/i386/cpu.c > > > > >> >> @@ -4552,6 +4552,24 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, > > > > >> >> cpu->env.tsc_khz = cpu->env.user_tsc_khz = value / 1000; > > > > >> >> } > > > > >> >> > > > > >> >> +static bool x86_hv_default_get(Object *obj, Error **errp) > > > > >> >> +{ > > > > >> >> + X86CPU *cpu = X86_CPU(obj); > > > > >> >> + > > > > >> >> + return cpu->hyperv_default; > > > > >> >> +} > > > > >> >> + > > > > >> >> +static void x86_hv_default_set(Object *obj, bool value, Error **errp) > > > > >> >> +{ > > > > >> >> + X86CPU *cpu = X86_CPU(obj); > > > > >> >> + > > > > >> >> + cpu->hyperv_default = value; > > > > >> >> + > > > > >> >> + if (value) { > > > > >> >> + cpu->hyperv_features |= cpu->hyperv_default_features; > > > > >> > > > > > >> > s/|="/=/ please, > > > > >> > i.e. no option overrides whatever was specified before to keep semantics consistent. > > > > >> > > > > > >> > > > > >> Hm, > > > > >> > > > > > > > > > >> this doesn't matter for the most recent machine type as > > > > >> hyperv_default_features has all the features but imagine you're running > > > > >> an older machine type which doesn't have 'hv_feature'. Now your > > > > > normally one shouldn't use new feature with old machine type as it makes > > > > > VM non-migratable to older QEMU that has this machine type but not this feature. > > > > > > > > > > nitpicking: > > > > > according to (1) user should not use 'hv_feature' on old machine since > > > > > hv_default should cover all their needs (well they don't know what > > > > > hv_default actually is). > > > > > > > > Normally yes but I can imagine sticking to some old machine type for > > > > other-than-hyperv-enlightenments purposes and still wanting to add a > > > > newly introduced enlightenment. Migration is not always a must. > > > > > > > > > > > > > >> suggestion is > > > > >> > > > > >> if I do: > > > > >> > > > > >> 'hv_default,hv_feature=on' I will get "hyperv_default_features | hv_feature" > > > > >> > > > > >> but if I do > > > > >> > > > > >> 'hv_feature=on,hv_default' I will just get 'hyperv_default_features' > > > > >> (as hv_default enablement will overwrite everything) > > > > >> > > > > >> How is this consistent? > > > > > usual semantics for properties, is that the latest property overwrites, > > > > > the previous property value parsed from left to right. > > > > > (i.e. if one asked for hv_default, one gets it related CPUID bit set/unset, > > > > > if one needs more than that one should add more related features after that. > > > > > > > > > > > > > This semantics probably doesn't apply to 'hv-default' case IMO as my > > > > brain refuses to accept the fact that > > > it's difficult probably because 'hv-default' is 'alias' property > > > that covers all individual hv-foo features in one go and that individual > > > features are exposed to user, but otherwise it is just a property that > > > sets CPUID features or like any other property, and should be treated like such. > > > > > > > 'hv_default,hv_feature' != 'hv_feature,hv_default' > > > > > > > > which should express the same desire 'the default set PLUS the feature I > > > > want'. > > > if hv_default were touching different data, I'd agree. > > > But in the end hv_default boils down to the same CPUID bits as individual > > > features: > > > > > > hv_default,hv_f2 => (hv_f1=on,hv_f2=off),hv_f2=on > > > != > > > hv_f2,hv_default => hv_f2=on,(hv_f1=on,hv_f2=off) > > > > I don't know why you chose to define "hv_default" as > > hv_f1=on,hv_f2=off. If hv_f2 is not enabled by hv_default, it > > doesn't need to be touched by hv_default at all. > > Essentially I was thinking about hv_default=on as setting default value > of hv CPUID leaf i.e. like doc claims, 'all' hv_* features (including > turned off and unused bits) which always sets leaf to its default state. > > Now lets consider following possible situation > using combine' approach (leaf |= some_bits): > > QEMU-6.0: initially we have all possible features enabled > hv_default = (hv_f1=on,hv_f2=on) > > hv_f2=on,hv_default=on == hv_f1=on,hv_f2=on > > QEMU-6.1: disabled hv_f2=off that was causing problems > > hv_default = (hv_f1=on,hv_f2=off) > > however due to ORing hv_default doesn't fix issue for the same CLI > (i.e. it doesn't have expected effect) > > hv_f2=on,hv_default=on => hv_f1=on,hv_f2=on > > if one would use usual 'set' semantics (leaf = all_bits), > then new hv_default value will have desired effect despite of botched CLI, > just by virtue of property following typical 'last set' semantics: > > => hv_f1=on,hv_f2=off > > If we assume that we 'never ever' will need to disable feature bits > than it doesn't matter which approach to use, however a look at > pc_compat arrays shows that features are being enabled/disabled > all the time. Also there should be a good reason for adding new semantics and deviating from typical property behavior.