| Message ID | 20180524172949.178885-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2018-05-24 10:29-0700, Jim Mattson: > KVM_GET_SUPPORTED_CPUID should return the x86 cpuid features which are > supported by both the hardware and kvm. Since commit 4d5422cea3b6 > ("KVM: X86: Provide a capability to disable MWAIT intercepts"), it is > possible to configure a VM so that MONITOR/MWAIT are fully supported > in the guest. Userspace still has the option to mask these features > off (e.g. for VMs that don't disable MONITOR/MWAIT exits.) > > Fixes: 4d5422cea3b6 ("KVM: X86: Provide a capability to disable MWAIT intercepts") > Signed-off-by: Jim Mattson <jmattson@google.com> > --- QEMU has a peculiar '-cpu host' mode that enables everything that is reported as supported and this change would regress old userspaces by tricking the guest OS into using a VM exit busy-loop instead of HLT. I think it would be better to say that the userspace should enable MONITOR in CPUID when using KVM_X86_DISABLE_EXITS_MWAIT. > arch/x86/kvm/cpuid.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index 82055b90a8b3..794e8c6adc11 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -359,9 +359,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, > 0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW); > /* cpuid 1.ecx */ > const u32 kvm_cpuid_1_ecx_x86_features = > - /* NOTE: MONITOR (and MWAIT) are emulated as NOP, > - * but *not* advertised to guests via CPUID ! */ > - F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ | > + F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64 */ | F(MONITOR) | > 0 /* DS-CPL, VMX, SMX, EST */ | > 0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ | > F(FMA) | F(CX16) | 0 /* xTPR Update, PDCM */ | > -- > 2.17.0.441.gb46fe60e1d-goog >
> I think it would be better to say that the userspace should enable > MONITOR in CPUID when using KVM_X86_DISABLE_EXITS_MWAIT. The problem with that approach is that it breaks the documented API for KVM_GET_SUPPORTED_CPUID. Specifically, "This ioctl returns x86 cpuid features which are supported by both the hardware and kvm." If, for example, we use KVM_GET_SUPPORTED_CPUID to determine whether or not a target host can support a live migration, then it becomes impossible to migrate a guest that uses KVM_X86_DISABLE_EXITS_MWAIT. Perhaps, like Intel, we have painted ourselves into a corner, and now we need KVM_GET_TRUE_SUPPORTED_CPUID?
2018-05-24 11:18-0700, Jim Mattson: > > I think it would be better to say that the userspace should enable > > MONITOR in CPUID when using KVM_X86_DISABLE_EXITS_MWAIT. > > The problem with that approach is that it breaks the documented API > for KVM_GET_SUPPORTED_CPUID. Specifically, "This ioctl returns x86 > cpuid features which are supported by both the hardware > and kvm." If, for example, we use KVM_GET_SUPPORTED_CPUID to determine > whether or not a target host can support a live migration, then it > becomes impossible to migrate a guest that uses > KVM_X86_DISABLE_EXITS_MWAIT. The original nop-MWAIT implementation is so bad that we didn't say that MONITOR is supported by KVM, we just had to implement those intercepts as NOP instead of #UD becase of a buggy OS X guest. The nop MWAIT implementation is the default and userspace cannot change it before a query to KVM_GET_SUPPORTED_CPUID, so I think we can still say that MONITOR is not supported by KVM. A guest that enables KVM_X86_DISABLE_EXITS_MWAIT could ignore the MONITOR flag when checking its CPUID against target host's KVM_GET_SUPPORTED_CPUID. The migration check should already take non-CPUID KVM capabilities into account, so the real check would just be elsewhere. > Perhaps, like Intel, we have painted ourselves into a corner, and now > we need KVM_GET_TRUE_SUPPORTED_CPUID? I'd rather regress -cpu host, but isn't this soluble with capability checking?
On Thu, May 24, 2018 at 11:46 AM, Radim Krčmář <rkrcmar@redhat.com> wrote: > 2018-05-24 11:18-0700, Jim Mattson: >> > I think it would be better to say that the userspace should enable >> > MONITOR in CPUID when using KVM_X86_DISABLE_EXITS_MWAIT. >> >> The problem with that approach is that it breaks the documented API >> for KVM_GET_SUPPORTED_CPUID. Specifically, "This ioctl returns x86 >> cpuid features which are supported by both the hardware >> and kvm." If, for example, we use KVM_GET_SUPPORTED_CPUID to determine >> whether or not a target host can support a live migration, then it >> becomes impossible to migrate a guest that uses >> KVM_X86_DISABLE_EXITS_MWAIT. > > The original nop-MWAIT implementation is so bad that we didn't say that > MONITOR is supported by KVM, we just had to implement those intercepts > as NOP instead of #UD becase of a buggy OS X guest. Yes, to abet violations of Apple's EULA. Shame on you! > The nop MWAIT implementation is the default and userspace cannot change > it before a query to KVM_GET_SUPPORTED_CPUID, so I think we can still > say that MONITOR is not supported by KVM. More precisely, MONITOR is not supported by KVM *in its default configuration*. > A guest that enables KVM_X86_DISABLE_EXITS_MWAIT could ignore the > MONITOR flag when checking its CPUID against target host's > KVM_GET_SUPPORTED_CPUID. The migration check should already take > non-CPUID KVM capabilities into account, so the real check would just be > elsewhere. > >> Perhaps, like Intel, we have painted ourselves into a corner, and now >> we need KVM_GET_TRUE_SUPPORTED_CPUID? > > I'd rather regress -cpu host, but isn't this soluble with capability > checking? It is, but at the very least, the documentation needs to be updated to reflect the new reality. This doesn't regress old userspace code, because the old userspace code would not know about KVM_X86_DISABLE_EXITS_MWAIT.
Hi Jim,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v4.17-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jim-Mattson/kvm-x86-MONITOR-MWAIT-are-now-supported/20180526-214733
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: x86_64-randconfig-x015-201820 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
arch/x86/kvm/cpuid.c: In function '__do_cpuid_ent':
>> arch/x86/kvm/cpuid.c:68:18: error: 'X86_FEATURE_MONITOR' undeclared (first use in this function); did you mean 'X86_FEATURE_MTRR'?
#define F(x) bit(X86_FEATURE_##x)
^
arch/x86/kvm/cpuid.c:362:45: note: in expansion of macro 'F'
F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64 */ | F(MONITOR) |
^
arch/x86/kvm/cpuid.c:68:18: note: each undeclared identifier is reported only once for each function it appears in
#define F(x) bit(X86_FEATURE_##x)
^
arch/x86/kvm/cpuid.c:362:45: note: in expansion of macro 'F'
F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64 */ | F(MONITOR) |
^
vim +68 arch/x86/kvm/cpuid.c
4ff41732 Paolo Bonzini 2014-02-24 67
5c404cab Paolo Bonzini 2014-12-03 @68 #define F(x) bit(X86_FEATURE_##x)
5c404cab Paolo Bonzini 2014-12-03 69
:::::: The code at line 68 was first introduced by commit
:::::: 5c404cabd1b5c125653ac573cb9284bdf42b658a KVM: x86: use F() macro throughout cpuid.c
:::::: TO: Paolo Bonzini <pbonzini@redhat.com>
:::::: CC: Paolo Bonzini <pbonzini@redhat.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Jim,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on kvm/linux-next]
[also build test ERROR on v4.17-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jim-Mattson/kvm-x86-MONITOR-MWAIT-are-now-supported/20180526-214733
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git linux-next
config: i386-randconfig-x010-201820 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
arch/x86/kvm/cpuid.c: In function '__do_cpuid_ent':
>> arch/x86/kvm/cpuid.c:68:18: error: 'X86_FEATURE_MONITOR' undeclared (first use in this function); did you mean 'X86_FEATURE_MWAIT'?
#define F(x) bit(X86_FEATURE_##x)
^
arch/x86/kvm/cpuid.c:362:45: note: in expansion of macro 'F'
F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64 */ | F(MONITOR) |
^
arch/x86/kvm/cpuid.c:68:18: note: each undeclared identifier is reported only once for each function it appears in
#define F(x) bit(X86_FEATURE_##x)
^
arch/x86/kvm/cpuid.c:362:45: note: in expansion of macro 'F'
F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64 */ | F(MONITOR) |
^
vim +68 arch/x86/kvm/cpuid.c
4ff41732 Paolo Bonzini 2014-02-24 67
5c404cab Paolo Bonzini 2014-12-03 @68 #define F(x) bit(X86_FEATURE_##x)
5c404cab Paolo Bonzini 2014-12-03 69
:::::: The code at line 68 was first introduced by commit
:::::: 5c404cabd1b5c125653ac573cb9284bdf42b658a KVM: x86: use F() macro throughout cpuid.c
:::::: TO: Paolo Bonzini <pbonzini@redhat.com>
:::::: CC: Paolo Bonzini <pbonzini@redhat.com>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index 82055b90a8b3..794e8c6adc11 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -359,9 +359,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function, 0 /* Reserved */ | f_lm | F(3DNOWEXT) | F(3DNOW); /* cpuid 1.ecx */ const u32 kvm_cpuid_1_ecx_x86_features = - /* NOTE: MONITOR (and MWAIT) are emulated as NOP, - * but *not* advertised to guests via CPUID ! */ - F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64, MONITOR */ | + F(XMM3) | F(PCLMULQDQ) | 0 /* DTES64 */ | F(MONITOR) | 0 /* DS-CPL, VMX, SMX, EST */ | 0 /* TM2 */ | F(SSSE3) | 0 /* CNXT-ID */ | 0 /* Reserved */ | F(FMA) | F(CX16) | 0 /* xTPR Update, PDCM */ |
KVM_GET_SUPPORTED_CPUID should return the x86 cpuid features which are supported by both the hardware and kvm. Since commit 4d5422cea3b6 ("KVM: X86: Provide a capability to disable MWAIT intercepts"), it is possible to configure a VM so that MONITOR/MWAIT are fully supported in the guest. Userspace still has the option to mask these features off (e.g. for VMs that don't disable MONITOR/MWAIT exits.) Fixes: 4d5422cea3b6 ("KVM: X86: Provide a capability to disable MWAIT intercepts") Signed-off-by: Jim Mattson <jmattson@google.com> --- arch/x86/kvm/cpuid.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)