From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eduardo Habkost Subject: Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest Date: Thu, 14 Jun 2012 16:12:47 -0300 Message-ID: <20120614191247.GR7090@otherpad.lan.raisama.net> References: <20120420150005.GW3169@otherpad.lan.raisama.net> <4F917E75.2080003@siemens.com> <20120420153656.GX3169@otherpad.lan.raisama.net> <4F926086.3020307@web.de> <20120423144818.GA3169@otherpad.lan.raisama.net> <4F9583DD.10807@siemens.com> <20120423200214.GG3169@otherpad.lan.raisama.net> <4F96CF9F.9060302@siemens.com> <20120424171925.GT3169@otherpad.lan.raisama.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kiszka , "qemu-devel@nongnu.org" , "kvm@vger.kernel.org" , Avi Kivity , Andre Przywara To: "Liu, Jinsong" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:34349 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756453Ab2FNTMQ (ORCPT ); Thu, 14 Jun 2012 15:12:16 -0400 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Jun 14, 2012 at 07:02:03PM +0000, Liu, Jinsong wrote: > Eduardo, Jan > > I will update tsc deadline timer patch (at qemu-kvm side) recently. > Have you made a final agreement of the issue 'KVM_CAP_TSC_DEADLINE_TIMER' vs. 'GET_SUPPORTED_CPUID'? I don't think there's a final agreement, but I was convinced later that it's probably better to _not_ have TSC-deadline on GET_SUPPORTED_CPUID, at least not by default. Even if this is changed in the future, it's a good idea to make qemu support the KVM_CAP_TSC_DEADLINE_TIMER method if running on older kernels, anyway. > Eduardo Habkost wrote: > > (CCing Andre Przywara, in case he can help to clarify what's the > > expected meaning of "-cpu host") > > > > On Tue, Apr 24, 2012 at 06:06:55PM +0200, Jan Kiszka wrote: > >> On 2012-04-23 22:02, Eduardo Habkost wrote: > >>> On Mon, Apr 23, 2012 at 06:31:25PM +0200, Jan Kiszka wrote: > >>>> However, that was how I interpreted this GET_SUPPORTED_CPUID. In > >>>> fact, > >>>> it is used as "kernel or hardware does not _prevent_" already. And > >>>> in that sense, it's ok to enable even features that are not in > >>>> kernel/hardware hands. We should point out this fact in the > >>>> documentation. > >>> > >>> I see GET_SUPPORTED_CPUID as just a "what userspace can enable > >>> because the kernel and the hardware support it (= don't prevent > >>> it), as long as userspace has the required support" (meaning A+B). > >>> It's a bit like KVM_CHECK_EXTENSION, but with the nice feature that > >>> that the capabilities map directly to CPUID bits. > >>> > >>> So, it's not clear to me: now you are OK with adding TSC_DEADLINE > >>> to GET_SUPPORTED_CPUID? > >>> > >>> But we still have the issue of "-cpu host" not knowing what can be > >>> safely enabled (without userspace feature-specific setup code), or > >>> not. Do you have any suggestion for that? Avi, do you have any > >>> suggestion? > >> > >> First of all, I bet this was already broken with the introduction of > >> x2apic. So TSC deadline won't make it worse. I guess we need to > >> address this in userspace, first by masking those features out, > >> later by actually emulating them. > > > > I am not sure I understand what you are proposing. Let me explain the > > use case I am thinking about: > > > > - Feature FOO is of type (A) (e.g. just a new instruction set that > > doesn't require additional userspace support) > > - User has a Qemu vesion that doesn't know anything about feature FOO > > - User gets a new CPU that supports feature FOO > > - User gets a new kernel that supports feature FOO (i.e. has FOO in > > GET_SUPPORTED_CPUID) > > - User does _not_ upgrade Qemu. > > - User expects to get feature FOO enabled if using "-cpu host", > > without upgrading Qemu. > > > > The problem here is: to support the above use-case, userspace need a > > probing mechanism that can differentiate _new_ (previously unknown) > > features that are in group (A) (safe to blindly enable) from features > > that are in group (B) (that can't be enabled without an userspace > > upgrade). > > > > In short, it becomes a problem if we consider the following case: > > > > - Feature BAR is of type (B) (it can't be enabled without extra > > userspace support) > > - User has a Qemu version that doesn't know anything about feature BAR > > - User gets a new CPU that supports feature BAR > > - User gets a new kernel that supports feature BAR (i.e. has BAR in > > GET_SUPPORTED_CPUID) > > - User does _not_ upgrade Qemu. > > - User simply shouldn't get feature BAR enabled, even if using "-cpu > > host", otherwise Qemu would break. > > > > If userspace always limited itself to features it knows about, it > > would be really easy to implement the feature without any new probing > > mechanism from the kernel. But that's not how I think users expect > > "-cpu host" to work. Maybe I am wrong, I don't know. I am CCing > > Andre, who introduced the "-cpu host" feature, in case he can explain > > what's the expected semantics on the cases above. > > > >> > >>> > >>> And I still don't know the answer to: > >>> > >>>>> - How to precisely define the groups (A) and (B)? > >>>>> - "requires additional code only if migration is required" > >>>>> qualifies as (B) or (A)? > >>> > >>> > >>> Re: documentation, isn't the following paragraph (already present > >>> on api.txt) sufficient? > >>> > >>> "The entries returned are the host cpuid as returned by the cpuid > >>> instruction, with unknown or unsupported features masked out. Some > >>> features (for example, x2apic), may not be present in the host cpu, > >>> but are exposed by kvm if it can emulate them efficiently." > >> > >> That suggests such features are always emulated - which is not true. > >> They are either emulated, or nothing _prevents_ their emulation by > >> user space. > > > > Well... it's a bit more complicated than that: the current semantics > > are a bit more than "doesn't prevent", as in theory every single > > feature can be emulated by userspace, without any help from the > > kernel. So, if "doesn't prevent" were the only criteria, the kernel > > would set every single feature bit on GET_SUPPORTED_CPUID, making it > > not very useful. > > > > At least in the case of x2apic, the kernel is using > > GET_SUPPORTED_CPUID to expose a _capability_ too: when x2apic is > > present on GET_SUPPORTED_CPUID, userspace knows that in addition to > > "not preventing" the feature from being enabled, the kernel is now > > able to emulate x2apic (if proper setup is made by userspace). A > > kernel that can't emulate x2apic (even if userspace was allowed to > > emulate it completely in userspace) would never have x2apic enabled on > > GET_SUPPORTED_CPUID. > > > > Like I said previously, in the end GET_SUPPORTED_CPUID is just a > > capability querying mechanism like KVM_CHECK_EXTENSION (where each > > extension have a specific kernel-capability meaning), but with the > > nice feature of being automatically mapped to CPUID bits (with no > > need for additional KVM_CAP_* => CPUID translation in userspace). > -- Eduardo From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33862) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfFSq-0007Au-QV for qemu-devel@nongnu.org; Thu, 14 Jun 2012 15:12:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SfFSo-0005dE-Az for qemu-devel@nongnu.org; Thu, 14 Jun 2012 15:12:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55941) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfFSo-0005aR-1R for qemu-devel@nongnu.org; Thu, 14 Jun 2012 15:12:14 -0400 Date: Thu, 14 Jun 2012 16:12:47 -0300 From: Eduardo Habkost Message-ID: <20120614191247.GR7090@otherpad.lan.raisama.net> References: <20120420150005.GW3169@otherpad.lan.raisama.net> <4F917E75.2080003@siemens.com> <20120420153656.GX3169@otherpad.lan.raisama.net> <4F926086.3020307@web.de> <20120423144818.GA3169@otherpad.lan.raisama.net> <4F9583DD.10807@siemens.com> <20120423200214.GG3169@otherpad.lan.raisama.net> <4F96CF9F.9060302@siemens.com> <20120424171925.GT3169@otherpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Liu, Jinsong" Cc: Jan Kiszka , Andre Przywara , "qemu-devel@nongnu.org" , "kvm@vger.kernel.org" , Avi Kivity On Thu, Jun 14, 2012 at 07:02:03PM +0000, Liu, Jinsong wrote: > Eduardo, Jan > > I will update tsc deadline timer patch (at qemu-kvm side) recently. > Have you made a final agreement of the issue 'KVM_CAP_TSC_DEADLINE_TIMER' vs. 'GET_SUPPORTED_CPUID'? I don't think there's a final agreement, but I was convinced later that it's probably better to _not_ have TSC-deadline on GET_SUPPORTED_CPUID, at least not by default. Even if this is changed in the future, it's a good idea to make qemu support the KVM_CAP_TSC_DEADLINE_TIMER method if running on older kernels, anyway. > Eduardo Habkost wrote: > > (CCing Andre Przywara, in case he can help to clarify what's the > > expected meaning of "-cpu host") > > > > On Tue, Apr 24, 2012 at 06:06:55PM +0200, Jan Kiszka wrote: > >> On 2012-04-23 22:02, Eduardo Habkost wrote: > >>> On Mon, Apr 23, 2012 at 06:31:25PM +0200, Jan Kiszka wrote: > >>>> However, that was how I interpreted this GET_SUPPORTED_CPUID. In > >>>> fact, > >>>> it is used as "kernel or hardware does not _prevent_" already. And > >>>> in that sense, it's ok to enable even features that are not in > >>>> kernel/hardware hands. We should point out this fact in the > >>>> documentation. > >>> > >>> I see GET_SUPPORTED_CPUID as just a "what userspace can enable > >>> because the kernel and the hardware support it (= don't prevent > >>> it), as long as userspace has the required support" (meaning A+B). > >>> It's a bit like KVM_CHECK_EXTENSION, but with the nice feature that > >>> that the capabilities map directly to CPUID bits. > >>> > >>> So, it's not clear to me: now you are OK with adding TSC_DEADLINE > >>> to GET_SUPPORTED_CPUID? > >>> > >>> But we still have the issue of "-cpu host" not knowing what can be > >>> safely enabled (without userspace feature-specific setup code), or > >>> not. Do you have any suggestion for that? Avi, do you have any > >>> suggestion? > >> > >> First of all, I bet this was already broken with the introduction of > >> x2apic. So TSC deadline won't make it worse. I guess we need to > >> address this in userspace, first by masking those features out, > >> later by actually emulating them. > > > > I am not sure I understand what you are proposing. Let me explain the > > use case I am thinking about: > > > > - Feature FOO is of type (A) (e.g. just a new instruction set that > > doesn't require additional userspace support) > > - User has a Qemu vesion that doesn't know anything about feature FOO > > - User gets a new CPU that supports feature FOO > > - User gets a new kernel that supports feature FOO (i.e. has FOO in > > GET_SUPPORTED_CPUID) > > - User does _not_ upgrade Qemu. > > - User expects to get feature FOO enabled if using "-cpu host", > > without upgrading Qemu. > > > > The problem here is: to support the above use-case, userspace need a > > probing mechanism that can differentiate _new_ (previously unknown) > > features that are in group (A) (safe to blindly enable) from features > > that are in group (B) (that can't be enabled without an userspace > > upgrade). > > > > In short, it becomes a problem if we consider the following case: > > > > - Feature BAR is of type (B) (it can't be enabled without extra > > userspace support) > > - User has a Qemu version that doesn't know anything about feature BAR > > - User gets a new CPU that supports feature BAR > > - User gets a new kernel that supports feature BAR (i.e. has BAR in > > GET_SUPPORTED_CPUID) > > - User does _not_ upgrade Qemu. > > - User simply shouldn't get feature BAR enabled, even if using "-cpu > > host", otherwise Qemu would break. > > > > If userspace always limited itself to features it knows about, it > > would be really easy to implement the feature without any new probing > > mechanism from the kernel. But that's not how I think users expect > > "-cpu host" to work. Maybe I am wrong, I don't know. I am CCing > > Andre, who introduced the "-cpu host" feature, in case he can explain > > what's the expected semantics on the cases above. > > > >> > >>> > >>> And I still don't know the answer to: > >>> > >>>>> - How to precisely define the groups (A) and (B)? > >>>>> - "requires additional code only if migration is required" > >>>>> qualifies as (B) or (A)? > >>> > >>> > >>> Re: documentation, isn't the following paragraph (already present > >>> on api.txt) sufficient? > >>> > >>> "The entries returned are the host cpuid as returned by the cpuid > >>> instruction, with unknown or unsupported features masked out. Some > >>> features (for example, x2apic), may not be present in the host cpu, > >>> but are exposed by kvm if it can emulate them efficiently." > >> > >> That suggests such features are always emulated - which is not true. > >> They are either emulated, or nothing _prevents_ their emulation by > >> user space. > > > > Well... it's a bit more complicated than that: the current semantics > > are a bit more than "doesn't prevent", as in theory every single > > feature can be emulated by userspace, without any help from the > > kernel. So, if "doesn't prevent" were the only criteria, the kernel > > would set every single feature bit on GET_SUPPORTED_CPUID, making it > > not very useful. > > > > At least in the case of x2apic, the kernel is using > > GET_SUPPORTED_CPUID to expose a _capability_ too: when x2apic is > > present on GET_SUPPORTED_CPUID, userspace knows that in addition to > > "not preventing" the feature from being enabled, the kernel is now > > able to emulate x2apic (if proper setup is made by userspace). A > > kernel that can't emulate x2apic (even if userspace was allowed to > > emulate it completely in userspace) would never have x2apic enabled on > > GET_SUPPORTED_CPUID. > > > > Like I said previously, in the end GET_SUPPORTED_CPUID is just a > > capability querying mechanism like KVM_CHECK_EXTENSION (where each > > extension have a specific kernel-capability meaning), but with the > > nice feature of being automatically mapped to CPUID bits (with no > > need for additional KVM_CAP_* => CPUID translation in userspace). > -- Eduardo