Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Eduardo Habkost wrote: > On Thu, Jun 14, 2012 at 07:02:03PM +, 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. OK, so I will coding based on current KVM_CAP_TSC_DEADLINE_TIMER method. Thanks for clarifying! > > >> 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 emulat
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On Thu, Jun 14, 2012 at 07:02:03PM +, 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 s
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
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'? Thanks, Jinsong 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 > k
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
(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
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2012-04-23 22:02, Eduardo Habkost wrote: > On Mon, Apr 23, 2012 at 06:31:25PM +0200, Jan Kiszka wrote: >> On 2012-04-23 16:48, Eduardo Habkost wrote: >>> Trying to summarize the points above: >>> >>> Groups (A) and (B) are: >>> >>> A) a feature that KVM supports and emulate by itself and can be enabled >>>by userspace blindly, without requiring any additional userspace >>>code to work. >>> B) a feature that KVM supports but need support from userspace to work. >>> >>> We have to differentiate those two groups somehow, otherwise "-cpu host" >>> will always risk being unstable (in case we can't identify group (B) and >>> end up enabling a feature that will break) or useless (if group (A) is >>> considered always empty). >>> >>> (If you think this two-group model is not sufficient, please let me know.) >>> >>> Note that I am discussing two things above: >>> >>> - Whether GET_SUPPORTED_CPUID should expose only features from group >>> (A), or group (B) too. >>> - One problem here is that today GET_SUPPORTED_CPUIDS have many >>> examples of (B) features inside it. Should we stop doing that? >> >> We have exactly two for the category that I was concerned about: TSC >> deadline and X2APIC. The latter is already exposed unconditionally, even >> if the kernel does not provide emulation. So, you are right, TSC >> deadline is not a new scenario. > > Oh, that explains why you were seing it differently: if the kernel > doesn't control anything about the feature exposure, there was no > obvious need to add it to GET_SUPPORTED_CPUID. > >> >>> - TSC-deadline is the first case where we are _not_ doing that. It >>> is the first CPU feature in KVM that can be enabled by userspace >>> (as long as userspace does the proper setup), but it's not >>> included on GET_SUPPORTED_CPUIDs. >>> - Even the current documentation implies that group (B) is included: >>> >>> "This ioctl returns x86 cpuid features which are supported by both >>> the hardware and kvm. Userspace can use the information returned by >>> this ioctl to construct cpuid information (for KVM_SET_CPUID2) that >>> is consistent with hardware, kernel, and userspace capabilities, and >>> ^^ >>> with user requirements (for example, the user may wish to constrain >>> cpuid to emulate older hardware, or for feature consistency across a >>> cluster)." >>> >>> In the specific case of TSC-deadline, I consider "Qemu knowing that >>> TSC-deadline can be enabled only if in-kernel irqchip is enabled" as >>> an "userpace capability". >>> >>> - How to precisely define the groups (A) and (B)? >>> - "requires additional code only if migration is required" qualifies >>> as (B) or (A)? >>> - "requires in-kernel irqchip to be enabled to work" qualifies as (B), >>> doesn't it? >> >> My problem is that features like X2APIC and TSC deadline are exposed by >> the kernel without "contributing" to them (if in-kernel irqchip is off). > > They are not simply "exposed by the kernel": they are enabled by > userspace, after userspace deciding whether it's safe or not (based on > multiple factors). > > >> 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. > > 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 alway
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On Mon, Apr 23, 2012 at 06:31:25PM +0200, Jan Kiszka wrote: > On 2012-04-23 16:48, Eduardo Habkost wrote: > > Trying to summarize the points above: > > > > Groups (A) and (B) are: > > > > A) a feature that KVM supports and emulate by itself and can be enabled > >by userspace blindly, without requiring any additional userspace > >code to work. > > B) a feature that KVM supports but need support from userspace to work. > > > > We have to differentiate those two groups somehow, otherwise "-cpu host" > > will always risk being unstable (in case we can't identify group (B) and > > end up enabling a feature that will break) or useless (if group (A) is > > considered always empty). > > > > (If you think this two-group model is not sufficient, please let me know.) > > > > Note that I am discussing two things above: > > > > - Whether GET_SUPPORTED_CPUID should expose only features from group > > (A), or group (B) too. > > - One problem here is that today GET_SUPPORTED_CPUIDS have many > > examples of (B) features inside it. Should we stop doing that? > > We have exactly two for the category that I was concerned about: TSC > deadline and X2APIC. The latter is already exposed unconditionally, even > if the kernel does not provide emulation. So, you are right, TSC > deadline is not a new scenario. Oh, that explains why you were seing it differently: if the kernel doesn't control anything about the feature exposure, there was no obvious need to add it to GET_SUPPORTED_CPUID. > > > - TSC-deadline is the first case where we are _not_ doing that. It > > is the first CPU feature in KVM that can be enabled by userspace > > (as long as userspace does the proper setup), but it's not > > included on GET_SUPPORTED_CPUIDs. > > - Even the current documentation implies that group (B) is included: > > > > "This ioctl returns x86 cpuid features which are supported by both > > the hardware and kvm. Userspace can use the information returned by > > this ioctl to construct cpuid information (for KVM_SET_CPUID2) that > > is consistent with hardware, kernel, and userspace capabilities, and > > ^^ > > with user requirements (for example, the user may wish to constrain > > cpuid to emulate older hardware, or for feature consistency across a > > cluster)." > > > > In the specific case of TSC-deadline, I consider "Qemu knowing that > > TSC-deadline can be enabled only if in-kernel irqchip is enabled" as > > an "userpace capability". > > > > - How to precisely define the groups (A) and (B)? > > - "requires additional code only if migration is required" qualifies > > as (B) or (A)? > > - "requires in-kernel irqchip to be enabled to work" qualifies as (B), > > doesn't it? > > My problem is that features like X2APIC and TSC deadline are exposed by > the kernel without "contributing" to them (if in-kernel irqchip is off). They are not simply "exposed by the kernel": they are enabled by userspace, after userspace deciding whether it's safe or not (based on multiple factors). > 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? 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." -- Eduardo
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2012-04-23 16:48, Eduardo Habkost wrote: > Trying to summarize the points above: > > Groups (A) and (B) are: > > A) a feature that KVM supports and emulate by itself and can be enabled >by userspace blindly, without requiring any additional userspace >code to work. > B) a feature that KVM supports but need support from userspace to work. > > We have to differentiate those two groups somehow, otherwise "-cpu host" > will always risk being unstable (in case we can't identify group (B) and > end up enabling a feature that will break) or useless (if group (A) is > considered always empty). > > (If you think this two-group model is not sufficient, please let me know.) > > Note that I am discussing two things above: > > - Whether GET_SUPPORTED_CPUID should expose only features from group > (A), or group (B) too. > - One problem here is that today GET_SUPPORTED_CPUIDS have many > examples of (B) features inside it. Should we stop doing that? We have exactly two for the category that I was concerned about: TSC deadline and X2APIC. The latter is already exposed unconditionally, even if the kernel does not provide emulation. So, you are right, TSC deadline is not a new scenario. > - TSC-deadline is the first case where we are _not_ doing that. It > is the first CPU feature in KVM that can be enabled by userspace > (as long as userspace does the proper setup), but it's not > included on GET_SUPPORTED_CPUIDs. > - Even the current documentation implies that group (B) is included: > > "This ioctl returns x86 cpuid features which are supported by both > the hardware and kvm. Userspace can use the information returned by > this ioctl to construct cpuid information (for KVM_SET_CPUID2) that > is consistent with hardware, kernel, and userspace capabilities, and > ^^ > with user requirements (for example, the user may wish to constrain > cpuid to emulate older hardware, or for feature consistency across a > cluster)." > > In the specific case of TSC-deadline, I consider "Qemu knowing that > TSC-deadline can be enabled only if in-kernel irqchip is enabled" as > an "userpace capability". > > - How to precisely define the groups (A) and (B)? > - "requires additional code only if migration is required" qualifies > as (B) or (A)? > - "requires in-kernel irqchip to be enabled to work" qualifies as (B), > doesn't it? My problem is that features like X2APIC and TSC deadline are exposed by the kernel without "contributing" to them (if in-kernel irqchip is off). 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. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On Sat, Apr 21, 2012 at 09:23:50AM +0200, Jan Kiszka wrote: > On 2012-04-20 17:36, Eduardo Habkost wrote: > > On Fri, Apr 20, 2012 at 05:19:17PM +0200, Jan Kiszka wrote: > >> On 2012-04-20 17:00, Eduardo Habkost wrote: > >>> On Fri, Apr 20, 2012 at 12:12:38PM +0200, Jan Kiszka wrote: > On 2012-04-19 22:03, Eduardo Habkost wrote: > > Jan/Avi: ping? > > > > I would like to get this ABI detail clarified so it can be implemented > > the right way on Qemu and KVM. > > > > My proposal is to simply add tsc-deadline to the data returned by > > GET_SUPPORTED_CPUID, making KVM_CAP_TSC_DEADLINE_TIMER unnecessary. > > > > IIRC, there were problems with this model to exclude that the feature > gets reported this way without ensuring that in-kernel irqchip is > actually activated. Please browse the discussions, it should be > documented there. > >>> > >>> The flag was never added to GET_SUPPORTED_CPUID on any of the git > >>> repositories I have checked, and I don't see that being done anywhere on > >>> my KVM mailing list archives, either. So I don't see how we could have > >>> had issues with GET_SUPPORTED_CPUID, if it was never present on the > >>> code. > >>> > >>> What was present on the code before the fix, was coded that > >>> unconditinally enabled the flag even if Qemu never asked for it, and > >>> that really was wrong. > >>> > >>> GET_SUPPORTED_CPUID doesn't enable any flag: it just tells userspace > >>> that the hardware and KVM supports the feature (and, surprise, this is > >>> exactly what KVM_CAP_TSC_DEADLINE_TIMER means too, isn't it?). It's up > >>> to userspace to enable the CPUID bits according to user requirements and > >>> userspace capabilities (in other words: only when userspace knows it is > >>> safe because the in-kernel irqchip is enabled). > >>> > >>> If the above is not what GET_SUPPORTED_CPUID means, I would like to get > >>> that clarified, so I can submit an updated to KVM API documentation. > >> > >> Does old userspace filter out flags from GET_SUPPORTED_CPUID that it > >> does not understand? > > > > It's even more strict than that: it only enables what was explicitly > > enabled on the CPU model asked by the user. > > > > But: > > > > The only exception is "-cpu host", that tries to enable everything, even > > flags Qemu never heard of, and that is something that may require some > > changes on the API design (or at least documentation clarifications). > > > > Today "-cpu host" can't differentiate (A) "a feature that KVM supports > > and emulate by itself and can be enabled without any support from > > userspace" and (B) "a feature that KVM supports but need support from > > userspace to be enabled". I am sure we will be able to find multiple > > examples of (B) inside the flags returned by GET_SUPPORTED_CPUID today. > > The problem remains that exposing TSC_DEADLINE via SUPPORTED_CPUID is > wrong in case userspace doesn't select the in-kernel APIC. The kernel > does _nothing_ about emulating the flag if userspace provides the APIC, > so it must not expose this feature. Even if this had no practical impact > (but it has: old qemu[-kvm] + userspace APIC + -cpu host), it would > still be semantically broken. How is that different from any other feature that requires userspace to "do the right thing" to make the feature work? GET_SUPPORTED_CPUID just tells userspace that the flag is supported, but userspace has to be sure it will really work, before enabling it. In other words, I always assumed that features from the (B) group were always included on GET_SUPPORTED_CPUID, too. Yes, that risks breaking -cpu host, so we will probably need a better interface to let "-cpu host" know what can be enabled or not, anyway. > > > > > We could decide to never add any new flag to GET_SUPPORTED_CPUID if it > > requires additional userspace support to work, from now on, and create > > new KVM_CAP_* flags for them. But, we really want to do that for almost > > every new CPUID feature bit in the future? > > Most CPU features do not depend on our in-kernel irqchips. But if there > is something to simplify in retrieving information about such > "conditional features", let's do it. But many CPU features on GET_SUPPORTED_CPUID depend on other userspace capabilities, need userspace to do "the right thing" to make it work, and won't work if userspace doesn't do what's required. Why configuring the in-kernel irqchip is special? > > > > > We also have the problem of defining what "requires support from > > userspace to work" means: I am not sure this has the same meaning for > > everybody. Many new features require userspace support only for > > migration, and nothing else, but some users don't need migration to > > work. Do those features qualify as (A), or as (B)? > > "Works for most user" also means "breaks for some". And that is a bug, > isn't it? It is, surely, but -cpu host is the only case where CPU features are enabled blindl
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2012-04-20 17:36, Eduardo Habkost wrote: > On Fri, Apr 20, 2012 at 05:19:17PM +0200, Jan Kiszka wrote: >> On 2012-04-20 17:00, Eduardo Habkost wrote: >>> On Fri, Apr 20, 2012 at 12:12:38PM +0200, Jan Kiszka wrote: On 2012-04-19 22:03, Eduardo Habkost wrote: > Jan/Avi: ping? > > I would like to get this ABI detail clarified so it can be implemented > the right way on Qemu and KVM. > > My proposal is to simply add tsc-deadline to the data returned by > GET_SUPPORTED_CPUID, making KVM_CAP_TSC_DEADLINE_TIMER unnecessary. > IIRC, there were problems with this model to exclude that the feature gets reported this way without ensuring that in-kernel irqchip is actually activated. Please browse the discussions, it should be documented there. >>> >>> The flag was never added to GET_SUPPORTED_CPUID on any of the git >>> repositories I have checked, and I don't see that being done anywhere on >>> my KVM mailing list archives, either. So I don't see how we could have >>> had issues with GET_SUPPORTED_CPUID, if it was never present on the >>> code. >>> >>> What was present on the code before the fix, was coded that >>> unconditinally enabled the flag even if Qemu never asked for it, and >>> that really was wrong. >>> >>> GET_SUPPORTED_CPUID doesn't enable any flag: it just tells userspace >>> that the hardware and KVM supports the feature (and, surprise, this is >>> exactly what KVM_CAP_TSC_DEADLINE_TIMER means too, isn't it?). It's up >>> to userspace to enable the CPUID bits according to user requirements and >>> userspace capabilities (in other words: only when userspace knows it is >>> safe because the in-kernel irqchip is enabled). >>> >>> If the above is not what GET_SUPPORTED_CPUID means, I would like to get >>> that clarified, so I can submit an updated to KVM API documentation. >> >> Does old userspace filter out flags from GET_SUPPORTED_CPUID that it >> does not understand? > > It's even more strict than that: it only enables what was explicitly > enabled on the CPU model asked by the user. > > But: > > The only exception is "-cpu host", that tries to enable everything, even > flags Qemu never heard of, and that is something that may require some > changes on the API design (or at least documentation clarifications). > > Today "-cpu host" can't differentiate (A) "a feature that KVM supports > and emulate by itself and can be enabled without any support from > userspace" and (B) "a feature that KVM supports but need support from > userspace to be enabled". I am sure we will be able to find multiple > examples of (B) inside the flags returned by GET_SUPPORTED_CPUID today. The problem remains that exposing TSC_DEADLINE via SUPPORTED_CPUID is wrong in case userspace doesn't select the in-kernel APIC. The kernel does _nothing_ about emulating the flag if userspace provides the APIC, so it must not expose this feature. Even if this had no practical impact (but it has: old qemu[-kvm] + userspace APIC + -cpu host), it would still be semantically broken. > > We could decide to never add any new flag to GET_SUPPORTED_CPUID if it > requires additional userspace support to work, from now on, and create > new KVM_CAP_* flags for them. But, we really want to do that for almost > every new CPUID feature bit in the future? Most CPU features do not depend on our in-kernel irqchips. But if there is something to simplify in retrieving information about such "conditional features", let's do it. > > We also have the problem of defining what "requires support from > userspace to work" means: I am not sure this has the same meaning for > everybody. Many new features require userspace support only for > migration, and nothing else, but some users don't need migration to > work. Do those features qualify as (A), or as (B)? "Works for most user" also means "breaks for some". And that is a bug, isn't it? Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On Fri, Apr 20, 2012 at 05:19:17PM +0200, Jan Kiszka wrote: > On 2012-04-20 17:00, Eduardo Habkost wrote: > > On Fri, Apr 20, 2012 at 12:12:38PM +0200, Jan Kiszka wrote: > >> On 2012-04-19 22:03, Eduardo Habkost wrote: > >>> Jan/Avi: ping? > >>> > >>> I would like to get this ABI detail clarified so it can be implemented > >>> the right way on Qemu and KVM. > >>> > >>> My proposal is to simply add tsc-deadline to the data returned by > >>> GET_SUPPORTED_CPUID, making KVM_CAP_TSC_DEADLINE_TIMER unnecessary. > >>> > >> > >> IIRC, there were problems with this model to exclude that the feature > >> gets reported this way without ensuring that in-kernel irqchip is > >> actually activated. Please browse the discussions, it should be > >> documented there. > > > > The flag was never added to GET_SUPPORTED_CPUID on any of the git > > repositories I have checked, and I don't see that being done anywhere on > > my KVM mailing list archives, either. So I don't see how we could have > > had issues with GET_SUPPORTED_CPUID, if it was never present on the > > code. > > > > What was present on the code before the fix, was coded that > > unconditinally enabled the flag even if Qemu never asked for it, and > > that really was wrong. > > > > GET_SUPPORTED_CPUID doesn't enable any flag: it just tells userspace > > that the hardware and KVM supports the feature (and, surprise, this is > > exactly what KVM_CAP_TSC_DEADLINE_TIMER means too, isn't it?). It's up > > to userspace to enable the CPUID bits according to user requirements and > > userspace capabilities (in other words: only when userspace knows it is > > safe because the in-kernel irqchip is enabled). > > > > If the above is not what GET_SUPPORTED_CPUID means, I would like to get > > that clarified, so I can submit an updated to KVM API documentation. > > Does old userspace filter out flags from GET_SUPPORTED_CPUID that it > does not understand? It's even more strict than that: it only enables what was explicitly enabled on the CPU model asked by the user. But: The only exception is "-cpu host", that tries to enable everything, even flags Qemu never heard of, and that is something that may require some changes on the API design (or at least documentation clarifications). Today "-cpu host" can't differentiate (A) "a feature that KVM supports and emulate by itself and can be enabled without any support from userspace" and (B) "a feature that KVM supports but need support from userspace to be enabled". I am sure we will be able to find multiple examples of (B) inside the flags returned by GET_SUPPORTED_CPUID today. We could decide to never add any new flag to GET_SUPPORTED_CPUID if it requires additional userspace support to work, from now on, and create new KVM_CAP_* flags for them. But, we really want to do that for almost every new CPUID feature bit in the future? We also have the problem of defining what "requires support from userspace to work" means: I am not sure this has the same meaning for everybody. Many new features require userspace support only for migration, and nothing else, but some users don't need migration to work. Do those features qualify as (A), or as (B)? -- Eduardo
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2012-04-20 17:00, Eduardo Habkost wrote: > On Fri, Apr 20, 2012 at 12:12:38PM +0200, Jan Kiszka wrote: >> On 2012-04-19 22:03, Eduardo Habkost wrote: >>> Jan/Avi: ping? >>> >>> I would like to get this ABI detail clarified so it can be implemented >>> the right way on Qemu and KVM. >>> >>> My proposal is to simply add tsc-deadline to the data returned by >>> GET_SUPPORTED_CPUID, making KVM_CAP_TSC_DEADLINE_TIMER unnecessary. >>> >> >> IIRC, there were problems with this model to exclude that the feature >> gets reported this way without ensuring that in-kernel irqchip is >> actually activated. Please browse the discussions, it should be >> documented there. > > The flag was never added to GET_SUPPORTED_CPUID on any of the git > repositories I have checked, and I don't see that being done anywhere on > my KVM mailing list archives, either. So I don't see how we could have > had issues with GET_SUPPORTED_CPUID, if it was never present on the > code. > > What was present on the code before the fix, was coded that > unconditinally enabled the flag even if Qemu never asked for it, and > that really was wrong. > > GET_SUPPORTED_CPUID doesn't enable any flag: it just tells userspace > that the hardware and KVM supports the feature (and, surprise, this is > exactly what KVM_CAP_TSC_DEADLINE_TIMER means too, isn't it?). It's up > to userspace to enable the CPUID bits according to user requirements and > userspace capabilities (in other words: only when userspace knows it is > safe because the in-kernel irqchip is enabled). > > If the above is not what GET_SUPPORTED_CPUID means, I would like to get > that clarified, so I can submit an updated to KVM API documentation. Does old userspace filter out flags from GET_SUPPORTED_CPUID that it does not understand? Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On Fri, Apr 20, 2012 at 12:12:38PM +0200, Jan Kiszka wrote: > On 2012-04-19 22:03, Eduardo Habkost wrote: > > Jan/Avi: ping? > > > > I would like to get this ABI detail clarified so it can be implemented > > the right way on Qemu and KVM. > > > > My proposal is to simply add tsc-deadline to the data returned by > > GET_SUPPORTED_CPUID, making KVM_CAP_TSC_DEADLINE_TIMER unnecessary. > > > > IIRC, there were problems with this model to exclude that the feature > gets reported this way without ensuring that in-kernel irqchip is > actually activated. Please browse the discussions, it should be > documented there. The flag was never added to GET_SUPPORTED_CPUID on any of the git repositories I have checked, and I don't see that being done anywhere on my KVM mailing list archives, either. So I don't see how we could have had issues with GET_SUPPORTED_CPUID, if it was never present on the code. What was present on the code before the fix, was coded that unconditinally enabled the flag even if Qemu never asked for it, and that really was wrong. GET_SUPPORTED_CPUID doesn't enable any flag: it just tells userspace that the hardware and KVM supports the feature (and, surprise, this is exactly what KVM_CAP_TSC_DEADLINE_TIMER means too, isn't it?). It's up to userspace to enable the CPUID bits according to user requirements and userspace capabilities (in other words: only when userspace knows it is safe because the in-kernel irqchip is enabled). If the above is not what GET_SUPPORTED_CPUID means, I would like to get that clarified, so I can submit an updated to KVM API documentation. -- Eduardo
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2012-04-19 22:03, Eduardo Habkost wrote: > Jan/Avi: ping? > > I would like to get this ABI detail clarified so it can be implemented > the right way on Qemu and KVM. > > My proposal is to simply add tsc-deadline to the data returned by > GET_SUPPORTED_CPUID, making KVM_CAP_TSC_DEADLINE_TIMER unnecessary. > IIRC, there were problems with this model to exclude that the feature gets reported this way without ensuring that in-kernel irqchip is actually activated. Please browse the discussions, it should be documented there. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Jan/Avi: ping? I would like to get this ABI detail clarified so it can be implemented the right way on Qemu and KVM. My proposal is to simply add tsc-deadline to the data returned by GET_SUPPORTED_CPUID, making KVM_CAP_TSC_DEADLINE_TIMER unnecessary. On Fri, Mar 23, 2012 at 02:17:52PM +, Liu, Jinsong wrote: > Eduardo Habkost wrote: > > On Fri, Mar 23, 2012 at 03:49:27AM +, Liu, Jinsong wrote: > >> Eduardo Habkost wrote: > >>> [1] From Documentation/virtual/kvm/api.txt: > >>> > >>> "KVM_GET_SUPPORTED_CPUID > >>> [...] > >>> This ioctl returns x86 cpuid features which are supported by both > >>> the hardware and kvm. Userspace can use the information returned > >>> by this ioctl to construct cpuid information (for KVM_SET_CPUID2) > >>> that is consistent with hardware, kernel, and userspace > >>> capabilities, and with > >>> ^^ > >>> user requirements (for example, the user may wish to constrain cpuid > >>> to emulate older hardware, or for feature consistency across a > >>> cluster)." > >> > >> The fixbug patch is implemented by Jan and Avi, I reply per my > >> understanding. > > > > No problem. I hope Jan or Avi can clarify this. > > > >> > >> I think for tsc deadline timer feature, KVM_CAP_TSC_DEADLINE_TIMER is > >> slightly better than KVM_GET_SUPPORTED_CPUID. If use > >> KVM_GET_SUPPORTED_CPUID, it means tsc deadline features bind to host > >> cpuid, while it fact it could be pure software emulated by kvm > >> (though currently it implemented as bound to hareware). For the sake > >> of > >> extension, it choose KVM_CAP_TSC_DEADLINE_TIMER. > > > > There's no requirement for GET_SUPPORTED_CPUID to be a subset of the > > host CPU features. If KVM can completely emulate the feature by > > software, then it can return the feature on GET_SUPPORTED_CPUID even > > if the host CPU doesn't have the feature. That's the case for x2apic, > > for example (see commit 0d1de2d901f4ba0972a3886496a44fb1d3300dbd). > > > Jan/Avi, > > Could you elaborate more your thought? > > Thanks, > Jinsong -- Eduardo
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Eduardo Habkost wrote:
> On Fri, Mar 09, 2012 at 09:52:29PM +0100, Jan Kiszka wrote:
>> On 2012-03-09 20:09, Liu, Jinsong wrote:
>>> Jan Kiszka wrote:
On 2012-03-09 19:27, Liu, Jinsong wrote:
> Jan Kiszka wrote:
>> On 2012-03-06 08:49, Liu, Jinsong wrote:
>>> Jan,
>>>
>>> Any comments? I feel some confused about your point 'disable
>>> cpuid feature for older machine types by default': are you
>>> planning a common approach for this common issue, or, you just
>>> ask me a specific solution for the tsc deadline timer case?
>>
>> I think a generic solution for this can be as simple as passing a
>> feature exclusion mask to cpu_init. You could simple append a
>> string of "-feature1,-feature2" to the cpu model that is
>> specified on creation. And that string could be defined in the
>> compat machine descriptions. Does this make sense?
>>
>
> Jan, to prevent misunderstanding, I elaborate my understanding of
> your points below (if any misunderstanding please point out to
> me): = Your target is, to migrate from A(old
> qemu) to B(new qemu) by
> 1. at A: qemu-version-A [-cpu whatever] // currently the
> default machine type is pc-A
> 2. at B: qemu-version-B -machine pc-A [-cpu whatever] -feature1
> -feature2
>
> B run new qemu-version-B (w/ new features 'feature1' and
> 'feature2'), but when B runs w/ compat '-machine pc-A', vm should
> not see 'feature1' and 'feature2', so commandline append string to
> cpu model '-cpu whatever -feature1 -feature2' to hidden new
> feature1 and feature2 to vm, hence vm can see same cpuid features
> (at B) as those at A (which means, no feature1, no feature2)
> =
>
> If my understanding of your thoughts is right, I think currently
> qemu has satisfied your target, code refer to
> pc_cpus_init(cpu_model) .. cpu_init(cpu_model)
> --> cpu_x86_register(*env, cpu_model)
> --> cpu_x86_find_by_name(*def, cpu_model) // parse '+/-
>
> features', generate feature masks plus_features... // and
> minus_features...(this is feature exclusion masks you want)
> I think your point 'define in the compat machine description' is
> unnecessary.
The user would have to specify the new feature as exclusions
*manually* on the command line if -machine pc-A doesn't inject them
*automatically*. So it is necessary to enhance qemu in this regard.
>>>
>>> ... You suggest 'append a string of "-feature1,-feature2" to the
>>> cpu model that is specified on creation' at your last email. Could
>>> you tell me other way user exclude features? I only know qemu
>>> command line :-(
>>
>> I was thinking of something like
>>
>> diff --git a/hw/boards.h b/hw/boards.h
>> index 667177d..2bae071 100644
>> --- a/hw/boards.h
>> +++ b/hw/boards.h
>> @@ -28,6 +28,7 @@ typedef struct QEMUMachine {
>> int is_default;
>> const char *default_machine_opts;
>> GlobalProperty *compat_props;
>> +const char *compat_cpu_features;
>> struct QEMUMachine *next;
>> } QEMUMachine;
>>
>> diff --git a/hw/pc.c b/hw/pc.c
>> index bb9867b..4d11559 100644
>> --- a/hw/pc.c
>> +++ b/hw/pc.c
>> @@ -949,8 +949,9 @@ static CPUState *pc_new_cpu(const char
>> *cpu_model) return env; }
>>
>> -void pc_cpus_init(const char *cpu_model)
>> +void pc_cpus_init(const char *cpu_model, const char
>> *append_features) { +char *model_and_features;
>> int i;
>>
>> /* init CPUs */
>> @@ -961,10 +962,13 @@ void pc_cpus_init(const char *cpu_model)
>> cpu_model = "qemu32";
>> #endif
>> }
>> +model_and_features = g_strconcat(cpu_model, ",",
>> append_features, NULL);
>>
>> for(i = 0; i < smp_cpus; i++) {
>> -pc_new_cpu(cpu_model);
>> +pc_new_cpu(model_and_features);
>> }
>> +
>> +g_free(model_and_features);
>> }
>>
>> void pc_memory_init(MemoryRegion *system_memory,
>>
>>
>> However, getting machine.compat_cpu_features to pc_cpus_init is
>> rather
>> ugly. And we will have CPU devices with real properties soon. Then
>> the compat feature string could be passed that way, without changing
>> any
>> machine init function.
>
> What if one cpudef had the wrong flags set but another cpudef was
> correct, and we had to fix it on Qemu 1.1 for only one model? What if
> the user _really_ wanted to edit the config file to add or remove a
> given flag?
>
> I think the best approach would be:
> - Having versioned CPU model names;
> - Specifying per-machine-type aliases.
>
> See also the "[libvirt] Modern CPU models cannot be used with libvirt"
> for related discussion.
>
> The config file would look like:
>
> [cpudef]
> name = "Westmere-1.0"
> features = "[...]" # no tsc-deadline
> [...]
>
> [cpudef]
> name = "Westmere-1.1"
> # so we don't have to copy everything from Westmere-1.0 manually:
> bas
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Eduardo Habkost wrote: > On Fri, Mar 23, 2012 at 03:49:27AM +, Liu, Jinsong wrote: >> Eduardo Habkost wrote: >>> [1] From Documentation/virtual/kvm/api.txt: >>> >>> "KVM_GET_SUPPORTED_CPUID >>> [...] >>> This ioctl returns x86 cpuid features which are supported by both >>> the hardware and kvm. Userspace can use the information returned >>> by this ioctl to construct cpuid information (for KVM_SET_CPUID2) >>> that is consistent with hardware, kernel, and userspace >>> capabilities, and with >>> ^^ >>> user requirements (for example, the user may wish to constrain cpuid >>> to emulate older hardware, or for feature consistency across a >>> cluster)." >> >> The fixbug patch is implemented by Jan and Avi, I reply per my >> understanding. > > No problem. I hope Jan or Avi can clarify this. > >> >> I think for tsc deadline timer feature, KVM_CAP_TSC_DEADLINE_TIMER is >> slightly better than KVM_GET_SUPPORTED_CPUID. If use >> KVM_GET_SUPPORTED_CPUID, it means tsc deadline features bind to host >> cpuid, while it fact it could be pure software emulated by kvm >> (though currently it implemented as bound to hareware). For the sake >> of >> extension, it choose KVM_CAP_TSC_DEADLINE_TIMER. > > There's no requirement for GET_SUPPORTED_CPUID to be a subset of the > host CPU features. If KVM can completely emulate the feature by > software, then it can return the feature on GET_SUPPORTED_CPUID even > if the host CPU doesn't have the feature. That's the case for x2apic, > for example (see commit 0d1de2d901f4ba0972a3886496a44fb1d3300dbd). Jan/Avi, Could you elaborate more your thought? Thanks, Jinsong
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On Fri, Mar 23, 2012 at 03:49:27AM +, Liu, Jinsong wrote: > Eduardo Habkost wrote: > > [1] From Documentation/virtual/kvm/api.txt: > > > > "KVM_GET_SUPPORTED_CPUID > > [...] > > This ioctl returns x86 cpuid features which are supported by both the > > hardware and kvm. Userspace can use the information returned by this > > ioctl to construct cpuid information (for KVM_SET_CPUID2) that is > > consistent with hardware, kernel, and userspace capabilities, and with > > ^^ > > user requirements (for example, the user may wish to constrain cpuid > > to emulate older hardware, or for feature consistency across a > > cluster)." > > The fixbug patch is implemented by Jan and Avi, I reply per my understanding. No problem. I hope Jan or Avi can clarify this. > > I think for tsc deadline timer feature, KVM_CAP_TSC_DEADLINE_TIMER is > slightly better than KVM_GET_SUPPORTED_CPUID. If use > KVM_GET_SUPPORTED_CPUID, it means tsc deadline features bind to host > cpuid, while it fact it could be pure software emulated by kvm (though > currently it implemented as bound to hareware). For the sake of > extension, it choose KVM_CAP_TSC_DEADLINE_TIMER. There's no requirement for GET_SUPPORTED_CPUID to be a subset of the host CPU features. If KVM can completely emulate the feature by software, then it can return the feature on GET_SUPPORTED_CPUID even if the host CPU doesn't have the feature. That's the case for x2apic, for example (see commit 0d1de2d901f4ba0972a3886496a44fb1d3300dbd). -- Eduardo
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Eduardo Habkost wrote:
> On Tue, Mar 20, 2012 at 12:53:57PM +, Liu, Jinsong wrote:
>> Rik van Riel wrote:
>>> On 03/09/2012 01:27 PM, Liu, Jinsong wrote:
>>>
As for 'tsc deadline' feature exposing, my patch (as attached) just
obey qemu general cpuid exposing method, and also satisfied your
target I think.
>>>
>>> One question.
>>>
>>> Why is TSC_DEADLINE not exposed in the cpuid allowed feature
>>> bits in do_cpuid_ent() in arch/x86/kvm/x86.c ?
>>>
>>> /* cpuid 1.ecx */
>>> const u32 kvm_supported_word4_x86_features =
>>> F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
>>> 0 /* DS-CPL, VMX, SMX, EST */ |
>>> 0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /*
>>> Reserved */ | F(FMA) | F(CX16) | 0 /* xTPR Update,
>>> PDCM */ | 0 /* Reserved, DCA */ | F(XMM4_1) |
>>> F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) |
>>> 0 /* Reserved*/ | F(AES) | F(XSAVE) | 0 /* OSXSAVE
>>> */ | F(AVX) | F(F16C) | F(RDRAND);
>>>
>>> Would it make sense to expose F(TSC_DEADLINE) above?
>>>
>>> Or is there something truly special about tsc deadline
>>> that means it should be different from everything else?
>>
>> Because the feature depends on KVM_CREATE_IRQCHIP, which we cannot
>> guarantee will be called, we expose it via a
>> KVM_CAP_TSC_DEADLINE_TIMER and not KVM_GET_SUPPORTED_CPUID.
>
> We have many other features that depend on proper support from
> userspace otherwise they wouldn't work, but are listed on
> GET_SUPPORTED_CPUID, don't we? Why is TSC-deadline special?
>
> GET_SUPPORTED_CPUID just means "KVM supports it as long as userspace
> supports it too and enables it", it doesn't mean "CPUID bit that will
> be enabled by default"[1].
>
>> Refer changeset 4d25a066b69fb749a39d0d4c610689dd765a0b0e.
>
> That changeset was necessary because the kernel was doing this on
> update_cpu
>
> if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> best->function == 0x1) {
> best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER);
>
> And that was really wrong, because it enabled the bit unconditionally.
> But I don't understand why KVM_CAP_TSC_DEADLINE_TIMER was created if
> we already have KVM_GET_SUPPORTED_CPUID to tell userspace which bits
> are supported by KVM.
>
Yes, exactly. That's why we need this patch.
>
> [1] From Documentation/virtual/kvm/api.txt:
>
> "KVM_GET_SUPPORTED_CPUID
> [...]
> This ioctl returns x86 cpuid features which are supported by both the
> hardware and kvm. Userspace can use the information returned by this
> ioctl to construct cpuid information (for KVM_SET_CPUID2) that is
> consistent with hardware, kernel, and userspace capabilities, and with
> ^^
> user requirements (for example, the user may wish to constrain cpuid
> to emulate older hardware, or for feature consistency across a
> cluster)."
The fixbug patch is implemented by Jan and Avi, I reply per my understanding.
I think for tsc deadline timer feature, KVM_CAP_TSC_DEADLINE_TIMER is slightly
better than KVM_GET_SUPPORTED_CPUID. If use KVM_GET_SUPPORTED_CPUID, it means
tsc deadline features bind to host cpuid, while it fact it could be pure
software emulated by kvm (though currently it implemented as bound to
hareware). For the sake of extension, it choose KVM_CAP_TSC_DEADLINE_TIMER.
Thanks,
Jinsong
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On Tue, Mar 20, 2012 at 12:53:57PM +, Liu, Jinsong wrote:
> Rik van Riel wrote:
> > On 03/09/2012 01:27 PM, Liu, Jinsong wrote:
> >
> >> As for 'tsc deadline' feature exposing, my patch (as attached) just
> >> obey qemu general cpuid exposing method, and also satisfied your
> >> target I think.
> >
> > One question.
> >
> > Why is TSC_DEADLINE not exposed in the cpuid allowed feature
> > bits in do_cpuid_ent() in arch/x86/kvm/x86.c ?
> >
> > /* cpuid 1.ecx */
> > const u32 kvm_supported_word4_x86_features =
> > F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ |
> > 0 /* DS-CPL, VMX, SMX, EST */ |
> > 0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /*
> > Reserved */ |
> > F(FMA) | F(CX16) | 0 /* xTPR Update, PDCM */ |
> > 0 /* Reserved, DCA */ | F(XMM4_1) |
> > F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) |
> > 0 /* Reserved*/ | F(AES) | F(XSAVE) | 0 /* OSXSAVE
> > */ | F(AVX) |
> > F(F16C) | F(RDRAND);
> >
> > Would it make sense to expose F(TSC_DEADLINE) above?
> >
> > Or is there something truly special about tsc deadline
> > that means it should be different from everything else?
>
> Because the feature depends on KVM_CREATE_IRQCHIP, which we cannot guarantee
> will be called, we expose it via a KVM_CAP_TSC_DEADLINE_TIMER and not
> KVM_GET_SUPPORTED_CPUID.
We have many other features that depend on proper support from userspace
otherwise they wouldn't work, but are listed on GET_SUPPORTED_CPUID,
don't we? Why is TSC-deadline special?
GET_SUPPORTED_CPUID just means "KVM supports it as long as userspace
supports it too and enables it", it doesn't mean "CPUID bit that will be
enabled by default"[1].
> Refer changeset 4d25a066b69fb749a39d0d4c610689dd765a0b0e.
That changeset was necessary because the kernel was doing this on
update_cpu
if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
best->function == 0x1) {
best->ecx |= bit(X86_FEATURE_TSC_DEADLINE_TIMER);
And that was really wrong, because it enabled the bit unconditionally.
But I don't understand why KVM_CAP_TSC_DEADLINE_TIMER was created if we
already have KVM_GET_SUPPORTED_CPUID to tell userspace which bits are
supported by KVM.
[1] From Documentation/virtual/kvm/api.txt:
"KVM_GET_SUPPORTED_CPUID
[...]
This ioctl returns x86 cpuid features which are supported by both the
hardware and kvm. Userspace can use the information returned by this
ioctl to construct cpuid information (for KVM_SET_CPUID2) that is
consistent with hardware, kernel, and userspace capabilities, and with
^^
user requirements (for example, the user may wish to constrain cpuid to
emulate older hardware, or for feature consistency across a cluster)."
--
Eduardo
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Rik van Riel wrote: > On 03/09/2012 01:27 PM, Liu, Jinsong wrote: > >> As for 'tsc deadline' feature exposing, my patch (as attached) just >> obey qemu general cpuid exposing method, and also satisfied your >> target I think. > > One question. > > Why is TSC_DEADLINE not exposed in the cpuid allowed feature > bits in do_cpuid_ent() in arch/x86/kvm/x86.c ? > > /* cpuid 1.ecx */ > const u32 kvm_supported_word4_x86_features = > F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ | > 0 /* DS-CPL, VMX, SMX, EST */ | > 0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* > Reserved */ | > F(FMA) | F(CX16) | 0 /* xTPR Update, PDCM */ | > 0 /* Reserved, DCA */ | F(XMM4_1) | > F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) | > 0 /* Reserved*/ | F(AES) | F(XSAVE) | 0 /* OSXSAVE > */ | F(AVX) | > F(F16C) | F(RDRAND); > > Would it make sense to expose F(TSC_DEADLINE) above? > > Or is there something truly special about tsc deadline > that means it should be different from everything else? Because the feature depends on KVM_CREATE_IRQCHIP, which we cannot guarantee will be called, we expose it via a KVM_CAP_TSC_DEADLINE_TIMER and not KVM_GET_SUPPORTED_CPUID. Refer changeset 4d25a066b69fb749a39d0d4c610689dd765a0b0e. BTW, your question remind me qemu-kvm side patch, and I update it a little (would be sent out later). Thanks, Jinsong
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 03/09/2012 01:27 PM, Liu, Jinsong wrote: As for 'tsc deadline' feature exposing, my patch (as attached) just obey qemu general cpuid exposing method, and also satisfied your target I think. One question. Why is TSC_DEADLINE not exposed in the cpuid allowed feature bits in do_cpuid_ent() in arch/x86/kvm/x86.c ? /* cpuid 1.ecx */ const u32 kvm_supported_word4_x86_features = F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ | 0 /* DS-CPL, VMX, SMX, EST */ | 0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ | F(FMA) | F(CX16) | 0 /* xTPR Update, PDCM */ | 0 /* Reserved, DCA */ | F(XMM4_1) | F(XMM4_2) | F(X2APIC) | F(MOVBE) | F(POPCNT) | 0 /* Reserved*/ | F(AES) | F(XSAVE) | 0 /* OSXSAVE */ | F(AVX) | F(F16C) | F(RDRAND); Would it make sense to expose F(TSC_DEADLINE) above? Or is there something truly special about tsc deadline that means it should be different from everything else? -- All rights reversed
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On Fri, Mar 09, 2012 at 09:52:29PM +0100, Jan Kiszka wrote:
> On 2012-03-09 20:09, Liu, Jinsong wrote:
> > Jan Kiszka wrote:
> >> On 2012-03-09 19:27, Liu, Jinsong wrote:
> >>> Jan Kiszka wrote:
> On 2012-03-06 08:49, Liu, Jinsong wrote:
> > Jan,
> >
> > Any comments? I feel some confused about your point 'disable cpuid
> > feature for older machine types by default': are you planning a
> > common approach for this common issue, or, you just ask me a
> > specific solution for the tsc deadline timer case?
>
> I think a generic solution for this can be as simple as passing a
> feature exclusion mask to cpu_init. You could simple append a string
> of "-feature1,-feature2" to the cpu model that is specified on
> creation. And that string could be defined in the compat machine
> descriptions. Does this make sense?
>
> >>>
> >>> Jan, to prevent misunderstanding, I elaborate my understanding of
> >>> your points below (if any misunderstanding please point out to me):
> >>> =
> >>> Your target is, to migrate from A(old qemu) to B(new qemu) by
> >>> 1. at A: qemu-version-A [-cpu whatever] // currently the
> >>> default machine type is pc-A
> >>> 2. at B: qemu-version-B -machine pc-A [-cpu whatever] -feature1
> >>> -feature2
> >>>
> >>> B run new qemu-version-B (w/ new features 'feature1' and
> >>> 'feature2'), but when B runs w/ compat '-machine pc-A', vm should
> >>> not see 'feature1' and 'feature2', so commandline append string to
> >>> cpu model '-cpu whatever -feature1 -feature2' to hidden new feature1
> >>> and feature2 to vm, hence vm can see same cpuid features (at B) as
> >>> those at A (which means, no feature1, no feature2)
> >>> =
> >>>
> >>> If my understanding of your thoughts is right, I think currently
> >>> qemu has satisfied your target, code refer to
> >>> pc_cpus_init(cpu_model) .. cpu_init(cpu_model)
> >>> --> cpu_x86_register(*env, cpu_model)
> >>> --> cpu_x86_find_by_name(*def, cpu_model) // parse '+/-
> >>>
> >>> features', generate feature masks plus_features... // and
> >>> minus_features...(this is feature exclusion masks you want)
> >>> I think your point 'define in the compat machine description' is
> >>> unnecessary.
> >>
> >> The user would have to specify the new feature as exclusions
> >> *manually* on the command line if -machine pc-A doesn't inject them
> >> *automatically*. So it is necessary to enhance qemu in this regard.
> >>
> >
> > ... You suggest 'append a string of "-feature1,-feature2" to the cpu model
> > that is specified on creation' at your last email.
> > Could you tell me other way user exclude features? I only know qemu command
> > line :-(
>
> I was thinking of something like
>
> diff --git a/hw/boards.h b/hw/boards.h
> index 667177d..2bae071 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -28,6 +28,7 @@ typedef struct QEMUMachine {
> int is_default;
> const char *default_machine_opts;
> GlobalProperty *compat_props;
> +const char *compat_cpu_features;
> struct QEMUMachine *next;
> } QEMUMachine;
>
> diff --git a/hw/pc.c b/hw/pc.c
> index bb9867b..4d11559 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -949,8 +949,9 @@ static CPUState *pc_new_cpu(const char *cpu_model)
> return env;
> }
>
> -void pc_cpus_init(const char *cpu_model)
> +void pc_cpus_init(const char *cpu_model, const char *append_features)
> {
> +char *model_and_features;
> int i;
>
> /* init CPUs */
> @@ -961,10 +962,13 @@ void pc_cpus_init(const char *cpu_model)
> cpu_model = "qemu32";
> #endif
> }
> +model_and_features = g_strconcat(cpu_model, ",", append_features, NULL);
>
> for(i = 0; i < smp_cpus; i++) {
> -pc_new_cpu(cpu_model);
> +pc_new_cpu(model_and_features);
> }
> +
> +g_free(model_and_features);
> }
>
> void pc_memory_init(MemoryRegion *system_memory,
>
>
> However, getting machine.compat_cpu_features to pc_cpus_init is rather
> ugly. And we will have CPU devices with real properties soon. Then the
> compat feature string could be passed that way, without changing any
> machine init function.
What if one cpudef had the wrong flags set but another cpudef was
correct, and we had to fix it on Qemu 1.1 for only one model? What if
the user _really_ wanted to edit the config file to add or remove a
given flag?
I think the best approach would be:
- Having versioned CPU model names;
- Specifying per-machine-type aliases.
See also the "[libvirt] Modern CPU models cannot be used with libvirt"
for related discussion.
The config file would look like:
[cpudef]
name = "Westmere-1.0"
features = "[...]" # no tsc-deadline
[...]
[cpudef]
name = "Westmere-1.1"
# so we don't have to copy everything from Westmere-1.0 manually:
base_cpudef = "Westemere-1.0"
# we could simply copy &
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Jan Kiszka wrote:
> On 2012-03-09 20:09, Liu, Jinsong wrote:
>> Jan Kiszka wrote:
>>> On 2012-03-09 19:27, Liu, Jinsong wrote:
Jan Kiszka wrote:
> On 2012-03-06 08:49, Liu, Jinsong wrote:
>> Jan,
>>
>> Any comments? I feel some confused about your point 'disable
>> cpuid feature for older machine types by default': are you
>> planning a common approach for this common issue, or, you just
>> ask me a specific solution for the tsc deadline timer case?
>
> I think a generic solution for this can be as simple as passing a
> feature exclusion mask to cpu_init. You could simple append a
> string of "-feature1,-feature2" to the cpu model that is
> specified on creation. And that string could be defined in the
> compat machine descriptions. Does this make sense?
>
Jan, to prevent misunderstanding, I elaborate my understanding of
your points below (if any misunderstanding please point out to
me): = Your target is, to migrate from A(old
qemu) to B(new qemu) by
1. at A: qemu-version-A [-cpu whatever] // currently the
default machine type is pc-A
2. at B: qemu-version-B -machine pc-A [-cpu whatever] -feature1
-feature2
B run new qemu-version-B (w/ new features 'feature1' and
'feature2'), but when B runs w/ compat '-machine pc-A', vm should
not see 'feature1' and 'feature2', so commandline append string to
cpu model '-cpu whatever -feature1 -feature2' to hidden new
feature1 and feature2 to vm, hence vm can see same cpuid features
(at B) as those at A (which means, no feature1, no feature2)
=
If my understanding of your thoughts is right, I think currently
qemu has satisfied your target, code refer to
pc_cpus_init(cpu_model) .. cpu_init(cpu_model)
--> cpu_x86_register(*env, cpu_model)
--> cpu_x86_find_by_name(*def, cpu_model) // parse '+/-
features', generate feature masks plus_features... // and
minus_features...(this is feature exclusion masks you want)
I think your point 'define in the compat machine description' is
unnecessary.
>>>
>>> The user would have to specify the new feature as exclusions
>>> *manually* on the command line if -machine pc-A doesn't inject them
>>> *automatically*. So it is necessary to enhance qemu in this regard.
>>>
>>
>> ... You suggest 'append a string of "-feature1,-feature2" to the cpu
>> model that is specified on creation' at your last email. Could you
>> tell me other way user exclude features? I only know qemu command
>> line :-(
>
> I was thinking of something like
>
> diff --git a/hw/boards.h b/hw/boards.h
> index 667177d..2bae071 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -28,6 +28,7 @@ typedef struct QEMUMachine {
> int is_default;
> const char *default_machine_opts;
> GlobalProperty *compat_props;
> +const char *compat_cpu_features;
> struct QEMUMachine *next;
> } QEMUMachine;
>
> diff --git a/hw/pc.c b/hw/pc.c
> index bb9867b..4d11559 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -949,8 +949,9 @@ static CPUState *pc_new_cpu(const char *cpu_model)
> return env;
> }
>
> -void pc_cpus_init(const char *cpu_model)
> +void pc_cpus_init(const char *cpu_model, const char *append_features)
> {
> +char *model_and_features;
> int i;
>
> /* init CPUs */
> @@ -961,10 +962,13 @@ void pc_cpus_init(const char *cpu_model)
> cpu_model = "qemu32";
> #endif
> }
> +model_and_features = g_strconcat(cpu_model, ",",
> append_features, NULL);
>
> for(i = 0; i < smp_cpus; i++) {
> -pc_new_cpu(cpu_model);
> +pc_new_cpu(model_and_features);
> }
> +
> +g_free(model_and_features);
> }
>
> void pc_memory_init(MemoryRegion *system_memory,
>
>
> However, getting machine.compat_cpu_features to pc_cpus_init is rather
> ugly. And we will have CPU devices with real properties soon. Then the
> compat feature string could be passed that way, without changing any
> machine init function.
>
> Andreas, do you expect CPU devices to be ready for qemu 1.1? We would
> need them to pass a feature exclusion mask from machine.compat_props
> to
> the (x86) CPU init code.
cpu devices is just another format of current cpu_model. It helps nothing to
our problem.
Again, the point is, by what method the feature exclusion mask can be
generated, if user not give hint manually?
Thanks,
Jinsong
>
> Well, given that introducing some intermediate solution for this would
> be complex and hacky and that there is a way to configure tsc_deadline
> for old machines away, though only an explicit one, I could live with
> postponing the feature mask after the CPU device conversion. But the
> last word will have the maintainers.
>
> Jan
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 Am 09.03.2012 21:52, schrieb Jan Kiszka: > Andreas, do you expect CPU devices to be ready for qemu 1.1? We > would need them to pass a feature exclusion mask from > machine.compat_props to the (x86) CPU init code. I was sure hoping to! Marcelo and Avi both going into vacation have let me down with the kvmclock patch that you wanted to go through uq/master. http://patchwork.ozlabs.org/patch/141721/ After it didn't make it into Avi's next PULL, I resubmitted it as part of the qom-user series. That's still on the list. Review and tags appreciated. http://patchwork.ozlabs.org/patch/144529/ Also on the list is a patch for a new object_class_get_list() function for sensible -cpu ? handling. http://patchwork.ozlabs.org/patch/143076/ I have been waiting for progress on these prerequisites before posting huge follow-on batches, but I can spam the list right away... The remaining issue was how to solve the struct CPU vs. CPU() issue - my solution on qom-cpu branch is to free the identifier CPUState in a quite invasive but half-scripted renaming. Based on struct CPUState, all targets are long successfully converted and keep needing to be rebased whenever someone moves code to a new microblaze file, adds a new arm board or in-kernel device using "CPUState", etc. Since there's a configure change involved in *-user QOM support, virtually all files get rebuilt on each rebase and that takes quite some time, so getting that in soonish would be appreciated. http://repo.or.cz/w/qemu/afaerber.git/shortlog/refs/heads/qom-cpu Andreas - -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.18 (GNU/Linux) iQIcBAEBAgAGBQJPWqljAAoJEPou0S0+fgE/u+kQAInqv/OYlL2zU+hyeKXdJcZ4 RJ9Ne7J1+MOYcOI0p2T4lTImawHmZbF1HQyM9gTYv3EcGoSLm/SgYnHhFOulvPaE dbpuwk3KPHU+1ekvMVhEX89F62rXvMTz8JxO7hSmNAwIU2fIeKKWEAo0tED2tY16 OC2ExnE46+xLpandGJmfnx8vUJ1/PaO6fpd/FLOZbACT75zmdDxjDpF85e6NNsct ysMQ3kNMWcc0RfTU02dPgrhCPc5irEbalaX6BtOnquVQZDH4ypwzKHOZpMfoTF4u QWyxy+NaOVMVBHgHJeGek0wQ4nbihIhrQAn+Hg/h0P+NPuxQ8s7zX4xwqlYwhAHn cdDfN1sVbIvRWkGCXSRJR5eIkPKV+81Afm7/pL26eNlVtSrVV9UnsOJyfyDrtFs/ tSE90n8ZecjARGXUaACboZRalt7Pxl4EGWJwBhsNd5+3xaYvc9/WuTYP1FK1nltk KiBETQXnSY5aTh13erKLjbtYGR5YexXiLIOXvnBD0qQ/LDryHNjslgM8HcShrx4S U9LFAqEq78fgpCXZlzwtnzed341Wuerw3z4jELrSW8GH+R21jMfT9L8lwaHexabh 7YlulgdSpsmCxzhHOUCMeUTFFY6TlPxVk2NhEhQxlzDXdDmm6nd67R9qJIf7d42z CPzkIX+OpKBmorMQREIg =a+ZQ -END PGP SIGNATURE-
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2012-03-09 20:09, Liu, Jinsong wrote:
> Jan Kiszka wrote:
>> On 2012-03-09 19:27, Liu, Jinsong wrote:
>>> Jan Kiszka wrote:
On 2012-03-06 08:49, Liu, Jinsong wrote:
> Jan,
>
> Any comments? I feel some confused about your point 'disable cpuid
> feature for older machine types by default': are you planning a
> common approach for this common issue, or, you just ask me a
> specific solution for the tsc deadline timer case?
I think a generic solution for this can be as simple as passing a
feature exclusion mask to cpu_init. You could simple append a string
of "-feature1,-feature2" to the cpu model that is specified on
creation. And that string could be defined in the compat machine
descriptions. Does this make sense?
>>>
>>> Jan, to prevent misunderstanding, I elaborate my understanding of
>>> your points below (if any misunderstanding please point out to me):
>>> =
>>> Your target is, to migrate from A(old qemu) to B(new qemu) by
>>> 1. at A: qemu-version-A [-cpu whatever] // currently the
>>> default machine type is pc-A
>>> 2. at B: qemu-version-B -machine pc-A [-cpu whatever] -feature1
>>> -feature2
>>>
>>> B run new qemu-version-B (w/ new features 'feature1' and
>>> 'feature2'), but when B runs w/ compat '-machine pc-A', vm should
>>> not see 'feature1' and 'feature2', so commandline append string to
>>> cpu model '-cpu whatever -feature1 -feature2' to hidden new feature1
>>> and feature2 to vm, hence vm can see same cpuid features (at B) as
>>> those at A (which means, no feature1, no feature2)
>>> =
>>>
>>> If my understanding of your thoughts is right, I think currently
>>> qemu has satisfied your target, code refer to
>>> pc_cpus_init(cpu_model) .. cpu_init(cpu_model)
>>> --> cpu_x86_register(*env, cpu_model)
>>> --> cpu_x86_find_by_name(*def, cpu_model) // parse '+/-
>>>
>>> features', generate feature masks plus_features... // and
>>> minus_features...(this is feature exclusion masks you want)
>>> I think your point 'define in the compat machine description' is
>>> unnecessary.
>>
>> The user would have to specify the new feature as exclusions
>> *manually* on the command line if -machine pc-A doesn't inject them
>> *automatically*. So it is necessary to enhance qemu in this regard.
>>
>
> ... You suggest 'append a string of "-feature1,-feature2" to the cpu model
> that is specified on creation' at your last email.
> Could you tell me other way user exclude features? I only know qemu command
> line :-(
I was thinking of something like
diff --git a/hw/boards.h b/hw/boards.h
index 667177d..2bae071 100644
--- a/hw/boards.h
+++ b/hw/boards.h
@@ -28,6 +28,7 @@ typedef struct QEMUMachine {
int is_default;
const char *default_machine_opts;
GlobalProperty *compat_props;
+const char *compat_cpu_features;
struct QEMUMachine *next;
} QEMUMachine;
diff --git a/hw/pc.c b/hw/pc.c
index bb9867b..4d11559 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -949,8 +949,9 @@ static CPUState *pc_new_cpu(const char *cpu_model)
return env;
}
-void pc_cpus_init(const char *cpu_model)
+void pc_cpus_init(const char *cpu_model, const char *append_features)
{
+char *model_and_features;
int i;
/* init CPUs */
@@ -961,10 +962,13 @@ void pc_cpus_init(const char *cpu_model)
cpu_model = "qemu32";
#endif
}
+model_and_features = g_strconcat(cpu_model, ",", append_features, NULL);
for(i = 0; i < smp_cpus; i++) {
-pc_new_cpu(cpu_model);
+pc_new_cpu(model_and_features);
}
+
+g_free(model_and_features);
}
void pc_memory_init(MemoryRegion *system_memory,
However, getting machine.compat_cpu_features to pc_cpus_init is rather
ugly. And we will have CPU devices with real properties soon. Then the
compat feature string could be passed that way, without changing any
machine init function.
Andreas, do you expect CPU devices to be ready for qemu 1.1? We would
need them to pass a feature exclusion mask from machine.compat_props to
the (x86) CPU init code.
Well, given that introducing some intermediate solution for this would
be complex and hacky and that there is a way to configure tsc_deadline
for old machines away, though only an explicit one, I could live with
postponing the feature mask after the CPU device conversion. But the
last word will have the maintainers.
Jan
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Liu, Jinsong wrote: > Jan Kiszka wrote: >> On 2012-03-06 08:49, Liu, Jinsong wrote: >>> Jan, >>> >>> Any comments? I feel some confused about your point 'disable cpuid >>> feature for older machine types by default': are you planning a >>> common approach for this common issue, or, you just ask me a >>> specific solution for the tsc deadline timer case? >> >> I think a generic solution for this can be as simple as passing a >> feature exclusion mask to cpu_init. You could simple append a string >> of "-feature1,-feature2" to the cpu model that is specified on >> creation. And that string could be defined in the compat machine >> descriptions. Does this make sense? >> > > Jan, to prevent misunderstanding, I elaborate my understanding of > your points below (if any misunderstanding please point out to me): > = > Your target is, to migrate from A(old qemu) to B(new qemu) by > 1. at A: qemu-version-A [-cpu whatever] // currently the default > machine type is pc-A > 2. at B: qemu-version-B -machine pc-A [-cpu whatever] -feature1 > -feature2 > > B run new qemu-version-B (w/ new features 'feature1' and 'feature2'), > but when B runs w/ compat '-machine pc-A', vm should not see > 'feature1' and 'feature2', so commandline append string to cpu model > '-cpu whatever -feature1 -feature2' to hidden new feature1 and > feature2 to vm, hence vm can see same cpuid features (at B) as those > at A (which means, no feature1, no feature2) = > BTW, any misunderstanding or something wrong about my understanding of your target? please help me confirm. I want to make sure we are talking same thing. Thanks, Jinsong > If my understanding of your thoughts is right, I think currently qemu > has satisfied your target, code refer to pc_cpus_init(cpu_model) > .. > cpu_init(cpu_model) > --> cpu_x86_register(*env, cpu_model) > --> cpu_x86_find_by_name(*def, cpu_model) // parse '+/- > > features', generate feature masks plus_features... // and > minus_features...(this is feature exclusion masks you want) > I think your point 'define in the compat machine description' is > unnecessary. > > As for 'tsc deadline' feature exposing, my patch (as attached) just > obey qemu general cpuid exposing method, and also satisfied your > target I think. > > > Thanks, > Jinsong
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Jan Kiszka wrote: > On 2012-03-09 19:27, Liu, Jinsong wrote: >> Jan Kiszka wrote: >>> On 2012-03-06 08:49, Liu, Jinsong wrote: Jan, Any comments? I feel some confused about your point 'disable cpuid feature for older machine types by default': are you planning a common approach for this common issue, or, you just ask me a specific solution for the tsc deadline timer case? >>> >>> I think a generic solution for this can be as simple as passing a >>> feature exclusion mask to cpu_init. You could simple append a string >>> of "-feature1,-feature2" to the cpu model that is specified on >>> creation. And that string could be defined in the compat machine >>> descriptions. Does this make sense? >>> >> >> Jan, to prevent misunderstanding, I elaborate my understanding of >> your points below (if any misunderstanding please point out to me): >> = >> Your target is, to migrate from A(old qemu) to B(new qemu) by >> 1. at A: qemu-version-A [-cpu whatever] // currently the >> default machine type is pc-A >> 2. at B: qemu-version-B -machine pc-A [-cpu whatever] -feature1 >> -feature2 >> >> B run new qemu-version-B (w/ new features 'feature1' and >> 'feature2'), but when B runs w/ compat '-machine pc-A', vm should >> not see 'feature1' and 'feature2', so commandline append string to >> cpu model '-cpu whatever -feature1 -feature2' to hidden new feature1 >> and feature2 to vm, hence vm can see same cpuid features (at B) as >> those at A (which means, no feature1, no feature2) >> = >> >> If my understanding of your thoughts is right, I think currently >> qemu has satisfied your target, code refer to >> pc_cpus_init(cpu_model) .. cpu_init(cpu_model) >> --> cpu_x86_register(*env, cpu_model) >> --> cpu_x86_find_by_name(*def, cpu_model) // parse '+/- >> >> features', generate feature masks plus_features... // and >> minus_features...(this is feature exclusion masks you want) >> I think your point 'define in the compat machine description' is >> unnecessary. > > The user would have to specify the new feature as exclusions > *manually* on the command line if -machine pc-A doesn't inject them > *automatically*. So it is necessary to enhance qemu in this regard. > ... You suggest 'append a string of "-feature1,-feature2" to the cpu model that is specified on creation' at your last email. Could you tell me other way user exclude features? I only know qemu command line :-( Thanks, Jinsong
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2012-03-09 19:27, Liu, Jinsong wrote: > Jan Kiszka wrote: >> On 2012-03-06 08:49, Liu, Jinsong wrote: >>> Jan, >>> >>> Any comments? I feel some confused about your point 'disable cpuid >>> feature for older machine types by default': are you planning a >>> common approach for this common issue, or, you just ask me a >>> specific solution for the tsc deadline timer case? >> >> I think a generic solution for this can be as simple as passing a >> feature exclusion mask to cpu_init. You could simple append a string >> of "-feature1,-feature2" to the cpu model that is specified on >> creation. And that string could be defined in the compat machine >> descriptions. Does this make sense? >> > > Jan, to prevent misunderstanding, I elaborate my understanding of your points > below (if any misunderstanding please point out to me): > = > Your target is, to migrate from A(old qemu) to B(new qemu) by > 1. at A: qemu-version-A [-cpu whatever] // currently the default machine > type is pc-A > 2. at B: qemu-version-B -machine pc-A [-cpu whatever] -feature1 -feature2 > > B run new qemu-version-B (w/ new features 'feature1' and 'feature2'), but > when B runs w/ compat '-machine pc-A', vm should not see 'feature1' and > 'feature2', so commandline append string to cpu model '-cpu whatever > -feature1 -feature2' to hidden new feature1 and feature2 to vm, hence vm can > see same cpuid features (at B) as those at A (which means, no feature1, no > feature2) > = > > If my understanding of your thoughts is right, I think currently qemu has > satisfied your target, code refer to > pc_cpus_init(cpu_model) > .. > cpu_init(cpu_model) > --> cpu_x86_register(*env, cpu_model) > --> cpu_x86_find_by_name(*def, cpu_model) // parse '+/- features', > generate feature masks plus_features... > // and > minus_features...(this is feature exclusion masks you want) > I think your point 'define in the compat machine description' is unnecessary. The user would have to specify the new feature as exclusions *manually* on the command line if -machine pc-A doesn't inject them *automatically*. So it is necessary to enhance qemu in this regard. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Jan Kiszka wrote: > On 2012-03-06 08:49, Liu, Jinsong wrote: >> Jan, >> >> Any comments? I feel some confused about your point 'disable cpuid >> feature for older machine types by default': are you planning a >> common approach for this common issue, or, you just ask me a >> specific solution for the tsc deadline timer case? > > I think a generic solution for this can be as simple as passing a > feature exclusion mask to cpu_init. You could simple append a string > of "-feature1,-feature2" to the cpu model that is specified on > creation. And that string could be defined in the compat machine > descriptions. Does this make sense? > Jan, to prevent misunderstanding, I elaborate my understanding of your points below (if any misunderstanding please point out to me): = Your target is, to migrate from A(old qemu) to B(new qemu) by 1. at A: qemu-version-A [-cpu whatever] // currently the default machine type is pc-A 2. at B: qemu-version-B -machine pc-A [-cpu whatever] -feature1 -feature2 B run new qemu-version-B (w/ new features 'feature1' and 'feature2'), but when B runs w/ compat '-machine pc-A', vm should not see 'feature1' and 'feature2', so commandline append string to cpu model '-cpu whatever -feature1 -feature2' to hidden new feature1 and feature2 to vm, hence vm can see same cpuid features (at B) as those at A (which means, no feature1, no feature2) = If my understanding of your thoughts is right, I think currently qemu has satisfied your target, code refer to pc_cpus_init(cpu_model) .. cpu_init(cpu_model) --> cpu_x86_register(*env, cpu_model) --> cpu_x86_find_by_name(*def, cpu_model) // parse '+/- features', generate feature masks plus_features... // and minus_features...(this is feature exclusion masks you want) I think your point 'define in the compat machine description' is unnecessary. As for 'tsc deadline' feature exposing, my patch (as attached) just obey qemu general cpuid exposing method, and also satisfied your target I think. Thanks, Jinsong 0001-Expose-tsc-deadline-timer-feature-to-guest.patch Description: 0001-Expose-tsc-deadline-timer-feature-to-guest.patch
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2012-03-06 08:49, Liu, Jinsong wrote: > Jan, > > Any comments? I feel some confused about your point 'disable cpuid feature > for older machine types by default': are you planning a common approach for > this common issue, or, you just ask me a specific solution for the tsc > deadline timer case? I think a generic solution for this can be as simple as passing a feature exclusion mask to cpu_init. You could simple append a string of "-feature1,-feature2" to the cpu model that is specified on creation. And that string could be defined in the compat machine descriptions. Does this make sense? Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Jan,
Any comments? I feel some confused about your point 'disable cpuid feature for
older machine types by default': are you planning a common approach for this
common issue, or, you just ask me a specific solution for the tsc deadline
timer case?
Thanks,
Jinsong
Liu, Jinsong wrote:
>> My point is that
>>
>> qemu-version-A [-cpu whatever]
>>
>> should provide the same VM as
>>
>> qemu-version-B -machine pc-A [-cpu whatever]
>>
>> specifically if you leave out the cpu specification.
>>
>> So the compat machine could establish a feature mask (e.g. append
>> some "-tsc_deadline" in this case). But, indeed, we need a new
>> channel for this.
>>
>
> Yes, if such requirement need to be satisfied, I agree we need a new
> channel to solve this kind of common issue.
>
> As for tsc deadline timer feature exposing, I write an updated patch
> as attached. 1). It exposes tsc deadline timer feature to guest if
> in-kernel irqchip is used and kvm has emulated tsc deadline timer;
> 2). It also authorizes user to control the feature exposing via a cpu
> feature flag;
>
> Thanks,
> Jinsong
>
>
> From 5b7d5f459b621686e78e437010ce34748bcb9e8e Mon Sep 17 00:00:00 2001
> From: Liu, Jinsong
> Date: Wed, 29 Feb 2012 01:53:15 +0800
> Subject: [PATCH] Expose tsc deadline timer feature to guest
>
> It exposes tsc deadline timer feature to guest if in-kernel irqchip
> is used
> and kvm has emulated tsc deadline timer.
> It also authorizes user to control the feature exposing via a cpu
> feature flag.
>
> Signed-off-by: Liu, Jinsong
> ---
> target-i386/cpu.h |1 +
> target-i386/cpuid.c |2 +-
> target-i386/kvm.c |4
> 3 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index d92be5d..3409afe 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -399,6 +399,7 @@
> #define CPUID_EXT_X2APIC (1 << 21)
> #define CPUID_EXT_MOVBE(1 << 22)
> #define CPUID_EXT_POPCNT (1 << 23)
> +#define CPUID_EXT_TSC_DEADLINE_TIMER (1 << 24)
> #define CPUID_EXT_XSAVE(1 << 26)
> #define CPUID_EXT_OSXSAVE (1 << 27)
> #define CPUID_EXT_HYPERVISOR (1 << 31)
> diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
> index b9bfeaf..ac4b79c 100644
> --- a/target-i386/cpuid.c
> +++ b/target-i386/cpuid.c
> @@ -50,7 +50,7 @@ static const char *ext_feature_name[] = {
> "fma", "cx16", "xtpr", "pdcm",
> NULL, NULL, "dca", "sse4.1|sse4_1",
> "sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
> -NULL, "aes", "xsave", "osxsave",
> +"tsc_deadline", "aes", "xsave", "osxsave",
> "avx", NULL, NULL, "hypervisor",
> };
> static const char *ext2_feature_name[] = {
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 7079e87..2639699 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -370,6 +370,10 @@ int kvm_arch_init_vcpu(CPUState *env)
> i = env->cpuid_ext_features & CPUID_EXT_HYPERVISOR;
> env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0,
> R_ECX); env->cpuid_ext_features |= i;
> +if (!kvm_irqchip_in_kernel() ||
> +!kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
> +env->cpuid_ext_features &= ~CPUID_EXT_TSC_DEADLINE_TIMER;
> +}
>
> env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s,
>
> 0x8001, 0, R_EDX);
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
>
> My point is that
>
> qemu-version-A [-cpu whatever]
>
> should provide the same VM as
>
> qemu-version-B -machine pc-A [-cpu whatever]
>
> specifically if you leave out the cpu specification.
>
> So the compat machine could establish a feature mask (e.g. append some
> "-tsc_deadline" in this case). But, indeed, we need a new channel for
> this.
>
Yes, if such requirement need to be satisfied, I agree we need a new channel to
solve this kind of common issue.
As for tsc deadline timer feature exposing, I write an updated patch as
attached.
1). It exposes tsc deadline timer feature to guest if in-kernel irqchip is used
and kvm has emulated tsc deadline timer;
2). It also authorizes user to control the feature exposing via a cpu feature
flag;
Thanks,
Jinsong
>From 5b7d5f459b621686e78e437010ce34748bcb9e8e Mon Sep 17 00:00:00 2001
From: Liu, Jinsong
Date: Wed, 29 Feb 2012 01:53:15 +0800
Subject: [PATCH] Expose tsc deadline timer feature to guest
It exposes tsc deadline timer feature to guest if in-kernel irqchip is used
and kvm has emulated tsc deadline timer.
It also authorizes user to control the feature exposing via a cpu feature flag.
Signed-off-by: Liu, Jinsong
---
target-i386/cpu.h |1 +
target-i386/cpuid.c |2 +-
target-i386/kvm.c |4
3 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index d92be5d..3409afe 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -399,6 +399,7 @@
#define CPUID_EXT_X2APIC (1 << 21)
#define CPUID_EXT_MOVBE(1 << 22)
#define CPUID_EXT_POPCNT (1 << 23)
+#define CPUID_EXT_TSC_DEADLINE_TIMER (1 << 24)
#define CPUID_EXT_XSAVE(1 << 26)
#define CPUID_EXT_OSXSAVE (1 << 27)
#define CPUID_EXT_HYPERVISOR (1 << 31)
diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index b9bfeaf..ac4b79c 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -50,7 +50,7 @@ static const char *ext_feature_name[] = {
"fma", "cx16", "xtpr", "pdcm",
NULL, NULL, "dca", "sse4.1|sse4_1",
"sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
-NULL, "aes", "xsave", "osxsave",
+"tsc_deadline", "aes", "xsave", "osxsave",
"avx", NULL, NULL, "hypervisor",
};
static const char *ext2_feature_name[] = {
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 7079e87..2639699 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -370,6 +370,10 @@ int kvm_arch_init_vcpu(CPUState *env)
i = env->cpuid_ext_features & CPUID_EXT_HYPERVISOR;
env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
env->cpuid_ext_features |= i;
+if (!kvm_irqchip_in_kernel() ||
+!kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
+env->cpuid_ext_features &= ~CPUID_EXT_TSC_DEADLINE_TIMER;
+}
env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s, 0x8001,
0, R_EDX);
--
1.7.1
0001-Expose-tsc-deadline-timer-feature-to-guest.patch
Description: 0001-Expose-tsc-deadline-timer-feature-to-guest.patch
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Jan Kiszka wrote:
> On 2012-01-07 19:23, Liu, Jinsong wrote:
>> Jan Kiszka wrote:
>>> On 2012-01-05 18:07, Liu, Jinsong wrote:
> Sorry, it remains bogus to expose the tsc deadline timer feature
> on machines < pc-1.1. That's just like we introduced kvmclock
> only to pc-0.14 onward. The reason is that guest OSes so far
> running on qemu-1.0 or older without deadline timer support must
> not find that feature when being migrated to a host with qemu-1.1
> in pc-1.0 compat mode. Yes, the user can explicitly disable it,
> but that is not the idea of legacy machine models. They should
> provide the very same environment that older qemu versions
> offered.
>
Not quite clear about this point.
Per my understanding, if a kvm guest running on an older qemu
without tsc deadline timer support,
then after migrate, the guest would still cannot find tsc deadline
feature, no matter older or newer host/qemu/pc-xx it migrate to.
>>>
>>> What should prevent this? The feature flags are not part of the
>>> vmstate. They are part of the vm configuration which is not migrated
>>> but defined by starting qemu on the target host.
>>>
>>
>> Thanks! understand this point ("They are part of the vm
>> configuration which is not migrated but defined by starting qemu on
>> the target host").
>>
>> But kvmclock example still cannot satisfy the purpose "guest running
>> on old qemu/pc-0.13 without kvmclock support must not find kvmclock
>> feature when being migrated to a host with new qemu/pc-0.13 compat
>> mode". After migration, guest can possibily find kvmclock
>> feature CPUID.0x4001.KVM_FEATURE_CLOCKSOURCE: pc_init1(...,
>> kvmclock_enabled) { pc_cpus_init(cpu_model);// the point to
>> decide and expose cpuid features to guest
>>
>> if (kvmclock_enabled) {// the difference point between
>> pc-0.13 vs. pc-0.14, related nothing to cpuid features.
>> kvmclock_create(); } }
>
> Right, not a perfect example: the cpuid feature is not influenced by
> this mechanism, only the fact if a kvmclock device (for save/restore)
> should be created. I guess we ignored this back then, only focusing on
> the more obvious issue of the addition device.
>
>>
>> Seems currently there is no good way to satisfy "guest running on
>> old qemu/pc-xx without feature A support must not find feature A
>> when being migrated to a host with new qemu/pc-xx compat mode", i.e.
>> considering
>> * if running with '-cpu host' then migrate;
>> * each time we add a new cpuid feature it need add one or more new
>> machine model? is it necessary to bind pc-xx with cpuid feature?
>> * logically cpuid features should better be controlled by cpu model,
>> not by machine model.
>
> The compatibility machines define the possible cpu models. If I select
How does machine define possible cpu models?
cpu model defined by qemu option '-cpu ...', while machine model defined by
'-machine ...'
> pc-0.14, e.g. -cpu kvm64 should not give me features that 0.14 was not
> exposing.
>
in such case, it's '-cpu kvm64' who take effect to decide what cpuid features
would exposed to guest, not '-machine pc-0.14'.
IMO, what our patch need to do is to expose a cpuid feature to guest
(CPUID.01H:ECX.TSC_Deadline[bit 24]), it decided by cpu model, not machine
model:
pc_init1(..., cpu_model, ...)
{
pc_cpus_init(cpu_model); // this is the whole logic exposing cpuid
features to guest
...
}
Do I misunderstanding something?
Thanks,
Jinsong
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2012-02-27 17:05, Liu, Jinsong wrote:
> Jan Kiszka wrote:
>> On 2012-01-07 19:23, Liu, Jinsong wrote:
>>> Jan Kiszka wrote:
On 2012-01-05 18:07, Liu, Jinsong wrote:
>> Sorry, it remains bogus to expose the tsc deadline timer feature
>> on machines < pc-1.1. That's just like we introduced kvmclock
>> only to pc-0.14 onward. The reason is that guest OSes so far
>> running on qemu-1.0 or older without deadline timer support must
>> not find that feature when being migrated to a host with qemu-1.1
>> in pc-1.0 compat mode. Yes, the user can explicitly disable it,
>> but that is not the idea of legacy machine models. They should
>> provide the very same environment that older qemu versions
>> offered.
>>
>
> Not quite clear about this point.
> Per my understanding, if a kvm guest running on an older qemu
> without tsc deadline timer support,
> then after migrate, the guest would still cannot find tsc deadline
> feature, no matter older or newer host/qemu/pc-xx it migrate to.
What should prevent this? The feature flags are not part of the
vmstate. They are part of the vm configuration which is not migrated
but defined by starting qemu on the target host.
>>>
>>> Thanks! understand this point ("They are part of the vm
>>> configuration which is not migrated but defined by starting qemu on
>>> the target host").
>>>
>>> But kvmclock example still cannot satisfy the purpose "guest running
>>> on old qemu/pc-0.13 without kvmclock support must not find kvmclock
>>> feature when being migrated to a host with new qemu/pc-0.13 compat
>>> mode". After migration, guest can possibily find kvmclock
>>> feature CPUID.0x4001.KVM_FEATURE_CLOCKSOURCE: pc_init1(...,
>>> kvmclock_enabled) { pc_cpus_init(cpu_model);// the point to
>>> decide and expose cpuid features to guest
>>>
>>> if (kvmclock_enabled) {// the difference point between
>>> pc-0.13 vs. pc-0.14, related nothing to cpuid features.
>>> kvmclock_create(); } }
>>
>> Right, not a perfect example: the cpuid feature is not influenced by
>> this mechanism, only the fact if a kvmclock device (for save/restore)
>> should be created. I guess we ignored this back then, only focusing on
>> the more obvious issue of the addition device.
>>
>>>
>>> Seems currently there is no good way to satisfy "guest running on
>>> old qemu/pc-xx without feature A support must not find feature A
>>> when being migrated to a host with new qemu/pc-xx compat mode", i.e.
>>> considering
>>> * if running with '-cpu host' then migrate;
>>> * each time we add a new cpuid feature it need add one or more new
>>> machine model? is it necessary to bind pc-xx with cpuid feature?
>>> * logically cpuid features should better be controlled by cpu model,
>>> not by machine model.
>>
>> The compatibility machines define the possible cpu models. If I select
>
> How does machine define possible cpu models?
> cpu model defined by qemu option '-cpu ...', while machine model defined by
> '-machine ...'
>
>> pc-0.14, e.g. -cpu kvm64 should not give me features that 0.14 was not
>> exposing.
>>
>
> in such case, it's '-cpu kvm64' who take effect to decide what cpuid features
> would exposed to guest, not '-machine pc-0.14'.
>
> IMO, what our patch need to do is to expose a cpuid feature to guest
> (CPUID.01H:ECX.TSC_Deadline[bit 24]), it decided by cpu model, not machine
> model:
> pc_init1(..., cpu_model, ...)
> {
> pc_cpus_init(cpu_model); // this is the whole logic exposing cpuid
> features to guest
> ...
> }
>
> Do I misunderstanding something?
My point is that
qemu-version-A [-cpu whatever]
should provide the same VM as
qemu-version-B -machine pc-A [-cpu whatever]
specifically if you leave out the cpu specification.
So the compat machine could establish a feature mask (e.g. append some
"-tsc_deadline" in this case). But, indeed, we need a new channel for this.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2012-01-07 19:23, Liu, Jinsong wrote:
> Jan Kiszka wrote:
>> On 2012-01-05 18:07, Liu, Jinsong wrote:
Sorry, it remains bogus to expose the tsc deadline timer feature on
machines < pc-1.1. That's just like we introduced kvmclock only to
pc-0.14 onward. The reason is that guest OSes so far running on
qemu-1.0 or older without deadline timer support must not find that
feature when being migrated to a host with qemu-1.1 in pc-1.0 compat
mode. Yes, the user can explicitly disable it, but that is not the
idea of legacy machine models. They should provide the very same
environment that older qemu versions offered.
>>>
>>> Not quite clear about this point.
>>> Per my understanding, if a kvm guest running on an older qemu
>>> without tsc deadline timer support,
>>> then after migrate, the guest would still cannot find tsc deadline
>>> feature, no matter older or newer host/qemu/pc-xx it migrate to.
>>
>> What should prevent this? The feature flags are not part of the
>> vmstate. They are part of the vm configuration which is not migrated
>> but defined by starting qemu on the target host.
>>
>
> Thanks! understand this point ("They are part of the vm configuration which
> is not migrated but defined by starting qemu on the target host").
>
> But kvmclock example still cannot satisfy the purpose "guest running on old
> qemu/pc-0.13 without kvmclock support must not find kvmclock feature when
> being migrated to a host with new qemu/pc-0.13 compat mode". After migration,
> guest can possibily find kvmclock feature
> CPUID.0x4001.KVM_FEATURE_CLOCKSOURCE:
> pc_init1(..., kvmclock_enabled)
> {
> pc_cpus_init(cpu_model);// the point to decide and expose cpuid
> features to guest
>
> if (kvmclock_enabled) {// the difference point between pc-0.13
> vs. pc-0.14, related nothing to cpuid features.
> kvmclock_create();
> }
> }
Right, not a perfect example: the cpuid feature is not influenced by
this mechanism, only the fact if a kvmclock device (for save/restore)
should be created. I guess we ignored this back then, only focusing on
the more obvious issue of the addition device.
>
> Seems currently there is no good way to satisfy "guest running on old
> qemu/pc-xx without feature A support must not find feature A when being
> migrated to a host with new qemu/pc-xx compat mode", i.e. considering
> * if running with '-cpu host' then migrate;
> * each time we add a new cpuid feature it need add one or more new machine
> model? is it necessary to bind pc-xx with cpuid feature?
> * logically cpuid features should better be controlled by cpu model, not by
> machine model.
The compatibility machines define the possible cpu models. If I select
pc-0.14, e.g. -cpu kvm64 should not give me features that 0.14 was not
exposing.
Jan
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Jan Kiszka wrote:
> On 2012-01-05 18:07, Liu, Jinsong wrote:
>>> Sorry, it remains bogus to expose the tsc deadline timer feature on
>>> machines < pc-1.1. That's just like we introduced kvmclock only to
>>> pc-0.14 onward. The reason is that guest OSes so far running on
>>> qemu-1.0 or older without deadline timer support must not find that
>>> feature when being migrated to a host with qemu-1.1 in pc-1.0 compat
>>> mode. Yes, the user can explicitly disable it, but that is not the
>>> idea of legacy machine models. They should provide the very same
>>> environment that older qemu versions offered.
>>>
>>
>> Not quite clear about this point.
>> Per my understanding, if a kvm guest running on an older qemu
>> without tsc deadline timer support,
>> then after migrate, the guest would still cannot find tsc deadline
>> feature, no matter older or newer host/qemu/pc-xx it migrate to.
>
> What should prevent this? The feature flags are not part of the
> vmstate. They are part of the vm configuration which is not migrated
> but defined by starting qemu on the target host.
>
Thanks! understand this point ("They are part of the vm configuration which is
not migrated but defined by starting qemu on the target host").
But kvmclock example still cannot satisfy the purpose "guest running on old
qemu/pc-0.13 without kvmclock support must not find kvmclock feature when being
migrated to a host with new qemu/pc-0.13 compat mode". After migration, guest
can possibily find kvmclock feature CPUID.0x4001.KVM_FEATURE_CLOCKSOURCE:
pc_init1(..., kvmclock_enabled)
{
pc_cpus_init(cpu_model);// the point to decide and expose cpuid
features to guest
if (kvmclock_enabled) {// the difference point between pc-0.13 vs.
pc-0.14, related nothing to cpuid features.
kvmclock_create();
}
}
Seems currently there is no good way to satisfy "guest running on old
qemu/pc-xx without feature A support must not find feature A when being
migrated to a host with new qemu/pc-xx compat mode", i.e. considering
* if running with '-cpu host' then migrate;
* each time we add a new cpuid feature it need add one or more new machine
model? is it necessary to bind pc-xx with cpuid feature?
* logically cpuid features should better be controlled by cpu model, not by
machine model.
Regards,
Jinsong
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2012-01-05 18:07, Liu, Jinsong wrote: >> Sorry, it remains bogus to expose the tsc deadline timer feature on >> machines < pc-1.1. That's just like we introduced kvmclock only to >> pc-0.14 onward. The reason is that guest OSes so far running on >> qemu-1.0 or older without deadline timer support must not find that >> feature when being migrated to a host with qemu-1.1 in pc-1.0 compat >> mode. Yes, the user can explicitly disable it, but that is not the >> idea of legacy machine models. They should provide the very same >> environment that older qemu versions offered. >> > > Not quite clear about this point. > Per my understanding, if a kvm guest running on an older qemu without tsc > deadline timer support, > then after migrate, the guest would still cannot find tsc deadline feature, > no matter older or newer host/qemu/pc-xx it migrate to. What should prevent this? The feature flags are not part of the vmstate. They are part of the vm configuration which is not migrated but defined by starting qemu on the target host. Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
> This requires some logic change and then rewording:
>
> - enable TSC deadline timer support by default if in-kernel irqchip is
> used
> - disable it on user request via a cpu feature flag
Yes, the logic has been implemented by the former patch as:
+if (env->tsc_deadline_timer_enabled) { // user control, default is to
authorize tsc deadline timer feature
+if (kvm_irqchip_in_kernel() && // in-kerenl irqchip is used
+kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
+env->cpuid_ext_features |= CPUID_EXT_TSC_DEADLINE_TIMER;
+}
+}
> - disable it for older machine types (see below) by default
>
> TSC deadline timer emulation in user space is a different story to be
> told once we have a patch for it.
>
>>
>> Signed-off-by: Liu, Jinsong ---
>> target-i386/cpu.h |2 ++
>> target-i386/cpuid.c |7 ++-
>> target-i386/kvm.c | 13 +
>> 3 files changed, 21 insertions(+), 1 deletions(-)
>>
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index 177d8aa..f2d0ad5 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -399,6 +399,7 @@
>> #define CPUID_EXT_X2APIC (1 << 21)
>> #define CPUID_EXT_MOVBE(1 << 22)
>> #define CPUID_EXT_POPCNT (1 << 23)
>> +#define CPUID_EXT_TSC_DEADLINE_TIMER (1 << 24)
>> #define CPUID_EXT_XSAVE(1 << 26)
>> #define CPUID_EXT_OSXSAVE (1 << 27)
>> #define CPUID_EXT_HYPERVISOR (1 << 31)
>> @@ -693,6 +694,7 @@ typedef struct CPUX86State {
>>
>> uint64_t tsc;
>> uint64_t tsc_deadline;
>> +bool tsc_deadline_timer_enabled;
>>
>> uint64_t mcg_status;
>> uint64_t msr_ia32_misc_enable;
>> diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
>> index 0b3af90..fe749e0 100644
>> --- a/target-i386/cpuid.c
>> +++ b/target-i386/cpuid.c
>> @@ -48,7 +48,7 @@ static const char *ext_feature_name[] = {
>> "fma", "cx16", "xtpr", "pdcm",
>> NULL, NULL, "dca", "sse4.1|sse4_1",
>> "sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
>> -NULL, "aes", "xsave", "osxsave",
>> +"tsc_deadline", "aes", "xsave", "osxsave",
>> "avx", NULL, NULL, "hypervisor",
>> };
>> static const char *ext2_feature_name[] = {
>> @@ -225,6 +225,7 @@ typedef struct x86_def_t {
>> int model;
>> int stepping;
>> int tsc_khz;
>> +bool tsc_deadline_timer_enabled;
>> uint32_t features, ext_features, ext2_features, ext3_features;
>> uint32_t kvm_features, svm_features;
>> uint32_t xlevel;
>> @@ -742,6 +743,9 @@ static int cpu_x86_find_by_name(x86_def_t
>> *x86_cpu_def, const char *cpu_model) x86_cpu_def->ext3_features
>> &= ~minus_ext3_features; x86_cpu_def->kvm_features &=
>> ~minus_kvm_features; x86_cpu_def->svm_features &=
>> ~minus_svm_features; +/* Defaultly user don't against
>> tsc_deadline_timer */ +x86_cpu_def->tsc_deadline_timer_enabled =
>> +!(minus_ext_features & CPUID_EXT_TSC_DEADLINE_TIMER);
>> if (check_cpuid) { if
>> (check_features_against_host(x86_cpu_def) && enforce_cpuid)
>> goto error; @@ -885,6 +889,7 @@ int cpu_x86_register
>> (CPUX86State *env, const char *cpu_model)
>> env->cpuid_ext4_features = def->ext4_features;
>> env->cpuid_xlevel2 = def->xlevel2; env->tsc_khz = def->tsc_khz; +
>> env->tsc_deadline_timer_enabled =
>> def->tsc_deadline_timer_enabled; if (!kvm_enabled()) {
>> env->cpuid_features &= TCG_FEATURES; env->cpuid_ext_features &=
>> TCG_EXT_FEATURES;
>> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
>> index d50de90..79baf0b 100644
>> --- a/target-i386/kvm.c
>> +++ b/target-i386/kvm.c
>> @@ -370,6 +370,19 @@ int kvm_arch_init_vcpu(CPUState *env)
>> i = env->cpuid_ext_features & CPUID_EXT_HYPERVISOR;
>> env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1,
>> 0, R_ECX); env->cpuid_ext_features |= i;
>> +/*
>> + * 1. Considering live migration, user enable/disable tsc
>> deadline timer; + * 2. If guest use kvm apic and kvm emulate tsc
>> deadline timer, expose it; + * 3. If in the future qemu support
>> tsc deadline timer emulation, + *and guest use qemu apic,
>> add cpuid exposing case then. + */
>
> See above. Also, I don't think this comment applies very well to this
> function.
Yes, the comment is indeed ambiguous. Would elaborate more clear.
>
>> +env->cpuid_ext_features &= ~CPUID_EXT_TSC_DEADLINE_TIMER;
>
> Can that feature possibly be set in cpuid_ext_features? I thought the
> kernel now refrains from this.
Yes, it's possible. Kernel didn't refrain it, just let qemu to make decision.
>
>> +if (env->tsc_deadline_timer_enabled) {
>> +if (kvm_irqchip_in_kernel() &&
>> +kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
>> +env->cpuid_ext_features |=
>> CPUID_EXT_TSC_DEADLINE_TIMER; +} +}
>>
>> env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s,
>>
Re: [Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
On 2011-12-28 19:55, Liu, Jinsong wrote:
>>From 3a78adf8006ec6189bfe2f55f7ae213e75bf3815 Mon Sep 17 00:00:00 2001
> From: Liu Jinsong
> Date: Thu, 29 Dec 2011 05:28:12 +0800
> Subject: [PATCH 2/2] Expose tsc deadline timer cpuid to guest
>
> Depend on several factors:
> 1. Considering live migration, user enable/disable tsc deadline timer;
> 2. If guest use kvm apic and kvm emulate tsc deadline timer, expose it;
> 3. If in the future qemu support tsc deadline timer emulation,
>and guest use qemu apic, add cpuid exposing case then.
This requires some logic change and then rewording:
- enable TSC deadline timer support by default if in-kernel irqchip is
used
- disable it on user request via a cpu feature flag
- disable it for older machine types (see below) by default
TSC deadline timer emulation in user space is a different story to be
told once we have a patch for it.
>
> Signed-off-by: Liu, Jinsong
> ---
> target-i386/cpu.h |2 ++
> target-i386/cpuid.c |7 ++-
> target-i386/kvm.c | 13 +
> 3 files changed, 21 insertions(+), 1 deletions(-)
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 177d8aa..f2d0ad5 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -399,6 +399,7 @@
> #define CPUID_EXT_X2APIC (1 << 21)
> #define CPUID_EXT_MOVBE(1 << 22)
> #define CPUID_EXT_POPCNT (1 << 23)
> +#define CPUID_EXT_TSC_DEADLINE_TIMER (1 << 24)
> #define CPUID_EXT_XSAVE(1 << 26)
> #define CPUID_EXT_OSXSAVE (1 << 27)
> #define CPUID_EXT_HYPERVISOR (1 << 31)
> @@ -693,6 +694,7 @@ typedef struct CPUX86State {
>
> uint64_t tsc;
> uint64_t tsc_deadline;
> +bool tsc_deadline_timer_enabled;
>
> uint64_t mcg_status;
> uint64_t msr_ia32_misc_enable;
> diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
> index 0b3af90..fe749e0 100644
> --- a/target-i386/cpuid.c
> +++ b/target-i386/cpuid.c
> @@ -48,7 +48,7 @@ static const char *ext_feature_name[] = {
> "fma", "cx16", "xtpr", "pdcm",
> NULL, NULL, "dca", "sse4.1|sse4_1",
> "sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
> -NULL, "aes", "xsave", "osxsave",
> +"tsc_deadline", "aes", "xsave", "osxsave",
> "avx", NULL, NULL, "hypervisor",
> };
> static const char *ext2_feature_name[] = {
> @@ -225,6 +225,7 @@ typedef struct x86_def_t {
> int model;
> int stepping;
> int tsc_khz;
> +bool tsc_deadline_timer_enabled;
> uint32_t features, ext_features, ext2_features, ext3_features;
> uint32_t kvm_features, svm_features;
> uint32_t xlevel;
> @@ -742,6 +743,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def,
> const char *cpu_model)
> x86_cpu_def->ext3_features &= ~minus_ext3_features;
> x86_cpu_def->kvm_features &= ~minus_kvm_features;
> x86_cpu_def->svm_features &= ~minus_svm_features;
> +/* Defaultly user don't against tsc_deadline_timer */
> +x86_cpu_def->tsc_deadline_timer_enabled =
> +!(minus_ext_features & CPUID_EXT_TSC_DEADLINE_TIMER);
> if (check_cpuid) {
> if (check_features_against_host(x86_cpu_def) && enforce_cpuid)
> goto error;
> @@ -885,6 +889,7 @@ int cpu_x86_register (CPUX86State *env, const char
> *cpu_model)
> env->cpuid_ext4_features = def->ext4_features;
> env->cpuid_xlevel2 = def->xlevel2;
> env->tsc_khz = def->tsc_khz;
> +env->tsc_deadline_timer_enabled = def->tsc_deadline_timer_enabled;
> if (!kvm_enabled()) {
> env->cpuid_features &= TCG_FEATURES;
> env->cpuid_ext_features &= TCG_EXT_FEATURES;
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index d50de90..79baf0b 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -370,6 +370,19 @@ int kvm_arch_init_vcpu(CPUState *env)
> i = env->cpuid_ext_features & CPUID_EXT_HYPERVISOR;
> env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
> env->cpuid_ext_features |= i;
> +/*
> + * 1. Considering live migration, user enable/disable tsc deadline timer;
> + * 2. If guest use kvm apic and kvm emulate tsc deadline timer, expose
> it;
> + * 3. If in the future qemu support tsc deadline timer emulation,
> + *and guest use qemu apic, add cpuid exposing case then.
> + */
See above. Also, I don't think this comment applies very well to this
function.
> +env->cpuid_ext_features &= ~CPUID_EXT_TSC_DEADLINE_TIMER;
Can that feature possibly be set in cpuid_ext_features? I thought the
kernel now refrains from this.
> +if (env->tsc_deadline_timer_enabled) {
> +if (kvm_irqchip_in_kernel() &&
> +kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
> +env->cpuid_ext_features |= CPUID_EXT_TSC_DEADLINE_TIMER;
> +}
> +}
>
> env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s, 0x8001,
> 0, R_EDX);
Sorry, it remains bogus to expose the tsc deadline
[Qemu-devel] [PATCH 2/2] Expose tsc deadline timer cpuid to guest
>From 3a78adf8006ec6189bfe2f55f7ae213e75bf3815 Mon Sep 17 00:00:00 2001
From: Liu Jinsong
Date: Thu, 29 Dec 2011 05:28:12 +0800
Subject: [PATCH 2/2] Expose tsc deadline timer cpuid to guest
Depend on several factors:
1. Considering live migration, user enable/disable tsc deadline timer;
2. If guest use kvm apic and kvm emulate tsc deadline timer, expose it;
3. If in the future qemu support tsc deadline timer emulation,
and guest use qemu apic, add cpuid exposing case then.
Signed-off-by: Liu, Jinsong
---
target-i386/cpu.h |2 ++
target-i386/cpuid.c |7 ++-
target-i386/kvm.c | 13 +
3 files changed, 21 insertions(+), 1 deletions(-)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 177d8aa..f2d0ad5 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -399,6 +399,7 @@
#define CPUID_EXT_X2APIC (1 << 21)
#define CPUID_EXT_MOVBE(1 << 22)
#define CPUID_EXT_POPCNT (1 << 23)
+#define CPUID_EXT_TSC_DEADLINE_TIMER (1 << 24)
#define CPUID_EXT_XSAVE(1 << 26)
#define CPUID_EXT_OSXSAVE (1 << 27)
#define CPUID_EXT_HYPERVISOR (1 << 31)
@@ -693,6 +694,7 @@ typedef struct CPUX86State {
uint64_t tsc;
uint64_t tsc_deadline;
+bool tsc_deadline_timer_enabled;
uint64_t mcg_status;
uint64_t msr_ia32_misc_enable;
diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 0b3af90..fe749e0 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -48,7 +48,7 @@ static const char *ext_feature_name[] = {
"fma", "cx16", "xtpr", "pdcm",
NULL, NULL, "dca", "sse4.1|sse4_1",
"sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
-NULL, "aes", "xsave", "osxsave",
+"tsc_deadline", "aes", "xsave", "osxsave",
"avx", NULL, NULL, "hypervisor",
};
static const char *ext2_feature_name[] = {
@@ -225,6 +225,7 @@ typedef struct x86_def_t {
int model;
int stepping;
int tsc_khz;
+bool tsc_deadline_timer_enabled;
uint32_t features, ext_features, ext2_features, ext3_features;
uint32_t kvm_features, svm_features;
uint32_t xlevel;
@@ -742,6 +743,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def,
const char *cpu_model)
x86_cpu_def->ext3_features &= ~minus_ext3_features;
x86_cpu_def->kvm_features &= ~minus_kvm_features;
x86_cpu_def->svm_features &= ~minus_svm_features;
+/* Defaultly user don't against tsc_deadline_timer */
+x86_cpu_def->tsc_deadline_timer_enabled =
+!(minus_ext_features & CPUID_EXT_TSC_DEADLINE_TIMER);
if (check_cpuid) {
if (check_features_against_host(x86_cpu_def) && enforce_cpuid)
goto error;
@@ -885,6 +889,7 @@ int cpu_x86_register (CPUX86State *env, const char
*cpu_model)
env->cpuid_ext4_features = def->ext4_features;
env->cpuid_xlevel2 = def->xlevel2;
env->tsc_khz = def->tsc_khz;
+env->tsc_deadline_timer_enabled = def->tsc_deadline_timer_enabled;
if (!kvm_enabled()) {
env->cpuid_features &= TCG_FEATURES;
env->cpuid_ext_features &= TCG_EXT_FEATURES;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index d50de90..79baf0b 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -370,6 +370,19 @@ int kvm_arch_init_vcpu(CPUState *env)
i = env->cpuid_ext_features & CPUID_EXT_HYPERVISOR;
env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
env->cpuid_ext_features |= i;
+/*
+ * 1. Considering live migration, user enable/disable tsc deadline timer;
+ * 2. If guest use kvm apic and kvm emulate tsc deadline timer, expose it;
+ * 3. If in the future qemu support tsc deadline timer emulation,
+ *and guest use qemu apic, add cpuid exposing case then.
+ */
+env->cpuid_ext_features &= ~CPUID_EXT_TSC_DEADLINE_TIMER;
+if (env->tsc_deadline_timer_enabled) {
+if (kvm_irqchip_in_kernel() &&
+kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
+env->cpuid_ext_features |= CPUID_EXT_TSC_DEADLINE_TIMER;
+}
+}
env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s, 0x8001,
0, R_EDX);
--
1.6.5.6
0002-Expose-tsc-deadline-timer-cpuid-to-guest.patch
Description: 0002-Expose-tsc-deadline-timer-cpuid-to-guest.patch
