From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38076) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TF36g-0002wa-SF for qemu-devel@nongnu.org; Fri, 21 Sep 2012 09:17:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TF36f-00069d-Bx for qemu-devel@nongnu.org; Fri, 21 Sep 2012 09:17:22 -0400 Received: from hub021-nj-3.exch021.serverdata.net ([206.225.164.218]:40020) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TF36f-00069S-7J for qemu-devel@nongnu.org; Fri, 21 Sep 2012 09:17:21 -0400 Message-ID: <505C68DF.3060801@CloudSwitch.Com> Date: Fri, 21 Sep 2012 09:17:19 -0400 From: Don Slutz MIME-Version: 1.0 References: <1348171412-23669-1-git-send-email-Don@CloudSwitch.com> <1348171412-23669-3-git-send-email-Don@CloudSwitch.com> <20120921103952.34c176c7@thinkpad.mammed.net> <20120921123602.GZ3983@otherpad.lan.raisama.net> In-Reply-To: <20120921123602.GZ3983@otherpad.lan.raisama.net> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 02/17] target-i386: Add missing kvm bits. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: peter.maydell@linaro.org, kvm@vger.kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, avi@redhat.com, anthony@codemonkey.ws, Igor Mammedov , afaerber@suse.de On 09/21/12 08:36, Eduardo Habkost wrote: > On Fri, Sep 21, 2012 at 10:39:52AM +0200, Igor Mammedov wrote: >> On Thu, 20 Sep 2012 16:03:17 -0400 >> Don Slutz wrote: >> >>> Fix duplicate name (kvmclock => kvm_clock2) also. >>> >>> Signed-off-by: Don Slutz >>> --- >>> target-i386/cpu.c | 12 ++++++++---- >>> 1 files changed, 8 insertions(+), 4 deletions(-) >>> >>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c >>> index 0313cf5..5f9866a 100644 >>> --- a/target-i386/cpu.c >>> +++ b/target-i386/cpu.c >>> @@ -87,10 +87,14 @@ static const char *ext3_feature_name[] = { >>> }; >>> >>> static const char *kvm_feature_name[] = { >>> - "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock", "kvm_asyncpf", NULL, "kvm_pv_eoi", NULL, >>> - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, >>> - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, >>> - NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, >>> + "kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvm_clock2", >> before patch if "kvmclock" is specified it would set 0 and 3 bits, >> after patch only bit 0 is set. >> Is it correct/expected behavior? if yes, please add rationale into patch >> description. This is not what I had intended. > The problem here seems to be: > - It would be interesting to make "kvmclock=true" enough to enable the > optimal behavior, instead of requiring users to use "kvm_clock2=true" > explicitly > - We need to allow older machine-types to be backwards compatible (not > enabling the second bit by default), so we need a separate property > to control the second bit. > > I think this is best modelled this way: > > - Having two separate properties: kvmclock and kvmclock2 (or kvm_clock2) > - Older machine-types would have kvmclock2 default to false. Newer > machine-types would kvmclock2 default to true. > - kvmclock=false would disable both bits > > Then: > > - kvmclock=false would not set any bit (it would be surprising to have > kvmclock=false but still have kvmclock enabled) > - kvmclock=true would keep compatible behavior on older machine-types, > (only the first bit set), but would get optimal behavior on newer > machine-types (both bits set) > - kvmclock=true,kvmclock2=true would set both bits > - kvmclock=true,kvmclock2=false would set only the first bit > > It wouldn't be a direct mapping between properties and CPUID bits, but > that's exactly the point. In this case, exposing individual CPUID bits > directly is a too low-level interface. > This does look much better. For the sake of simple changes, this patch will be changed so that -kvmclock (kvmclock=false) will continue to clear both bits. I will look into the right way to fit this into the newer cpu model. >> >>> + "kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", NULL, >>> + NULL, NULL, NULL, NULL, >>> + NULL, NULL, NULL, NULL, >>> + NULL, NULL, NULL, NULL, >>> + NULL, NULL, NULL, NULL, >>> + "kvm_clock_stable", NULL, NULL, NULL, >>> + NULL, NULL, NULL, NULL, >>> }; >>> >>> static const char *svm_feature_name[] = { >>> -- >>> 1.7.1 >>> >> >> -- >> Regards, >> Igor -Don Slutz