| Message ID | 20170503193733.13409-4-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/05/2017 21:37, Radim Krčmář wrote: > The guest can call MWAIT with ECX = 0 even if we enforce > CPUID5_ECX_INTERRUPT_BREAK; the call would have the exactly the same > effect as if the host didn't have CPUID5_ECX_INTERRUPT_BREAK. > > The check was added in some iteration while trying to fix a reported > OS X on Core 2 bug, but the CPU had CPUID5_ECX_INTERRUPT_BREAK and the > bug is elsewhere. The reason for this, as I understood it, is that we have historically not published leaf 5 information via KVM_GET_SUPPORTED_CPUID. For this reason, QEMU is publishing CPUID5_ECX_INTERRUPT_BREAK. Then if: - the host doesn't have ECX[0]=1 support - the guest sets ECX[0] you get a #GP in the guest. So wrong comment but right thing to do. Paolo > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > arch/x86/kvm/x86.h | 23 +---------------------- > 1 file changed, 1 insertion(+), 22 deletions(-) > > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h > index 63d5fb65ea30..8ea4e80c24d1 100644 > --- a/arch/x86/kvm/x86.h > +++ b/arch/x86/kvm/x86.h > @@ -216,8 +216,6 @@ static inline u64 nsec_to_cycles(struct kvm_vcpu *vcpu, u64 nsec) > > static inline bool kvm_mwait_in_guest(void) > { > - unsigned int eax, ebx, ecx, edx; > - > if (!cpu_has(&boot_cpu_data, X86_FEATURE_MWAIT)) > return false; > > @@ -225,29 +223,10 @@ static inline bool kvm_mwait_in_guest(void) > case X86_VENDOR_AMD: > return !boot_cpu_has_bug(X86_BUG_AMD_E400); > case X86_VENDOR_INTEL: > - /* Handle Intel below */ > - break; > + return !boot_cpu_has_bug(X86_BUG_MONITOR); > default: > return false; > } > - > - if (boot_cpu_has_bug(X86_BUG_MONITOR)) > - return false; > - > - /* > - * Intel CPUs without CPUID5_ECX_INTERRUPT_BREAK are problematic as > - * they would allow guest to stop the CPU completely by disabling > - * interrupts then invoking MWAIT. > - */ > - if (boot_cpu_data.cpuid_level < CPUID_MWAIT_LEAF) > - return false; > - > - cpuid(CPUID_MWAIT_LEAF, &eax, &ebx, &ecx, &edx); > - > - if (!(ecx & CPUID5_ECX_INTERRUPT_BREAK)) > - return false; > - > - return true; > } > > #endif >
2017-05-04 12:58+0200, Paolo Bonzini: > On 03/05/2017 21:37, Radim Krčmář wrote: >> The guest can call MWAIT with ECX = 0 even if we enforce >> CPUID5_ECX_INTERRUPT_BREAK; the call would have the exactly the same >> effect as if the host didn't have CPUID5_ECX_INTERRUPT_BREAK. >> >> The check was added in some iteration while trying to fix a reported >> OS X on Core 2 bug, but the CPU had CPUID5_ECX_INTERRUPT_BREAK and the >> bug is elsewhere. > > The reason for this, as I understood it, is that we have historically > not published leaf 5 information via KVM_GET_SUPPORTED_CPUID. For this > reason, QEMU is publishing CPUID5_ECX_INTERRUPT_BREAK. Then if: I see, it was added to QEMU in e737b32a3688 ("Core 2 Duo specification (Alexander Graf)"). > - the host doesn't have ECX[0]=1 support > > - the guest sets ECX[0] > > you get a #GP in the guest. So wrong comment but right thing to do. That userspace didn't set CPUID.01H:ECX.MONITOR[bit 3], so a guest should get #UD instead, but MWAIT couldn't be expected to work. I think that the guest bug is very unlikely, so I'd get rid of the condition anyway ... we have also recently killed support for pre-Core 2 hosts and AFAIK, all newer Intels have it. (Not so sure about AMDs, which share the same problem, so we do need to do more than just comment it better in any case.)
On Thu, May 04, 2017 at 12:58:05PM +0200, Paolo Bonzini wrote: > > > On 03/05/2017 21:37, Radim Krčmář wrote: > > The guest can call MWAIT with ECX = 0 even if we enforce > > CPUID5_ECX_INTERRUPT_BREAK; the call would have the exactly the same > > effect as if the host didn't have CPUID5_ECX_INTERRUPT_BREAK. > > > > The check was added in some iteration while trying to fix a reported > > OS X on Core 2 bug, but the CPU had CPUID5_ECX_INTERRUPT_BREAK and the > > bug is elsewhere. > > The reason for this, as I understood it, is that we have historically > not published leaf 5 information via KVM_GET_SUPPORTED_CPUID. For this > reason, QEMU is publishing CPUID5_ECX_INTERRUPT_BREAK. Then if: > > - the host doesn't have ECX[0]=1 support > > - the guest sets ECX[0] > > you get a #GP in the guest. So wrong comment but right thing to do. > > Paolo Exactly. And I agree the comment isn't a good one. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > > --- > > arch/x86/kvm/x86.h | 23 +---------------------- > > 1 file changed, 1 insertion(+), 22 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h > > index 63d5fb65ea30..8ea4e80c24d1 100644 > > --- a/arch/x86/kvm/x86.h > > +++ b/arch/x86/kvm/x86.h > > @@ -216,8 +216,6 @@ static inline u64 nsec_to_cycles(struct kvm_vcpu *vcpu, u64 nsec) > > > > static inline bool kvm_mwait_in_guest(void) > > { > > - unsigned int eax, ebx, ecx, edx; > > - > > if (!cpu_has(&boot_cpu_data, X86_FEATURE_MWAIT)) > > return false; > > > > @@ -225,29 +223,10 @@ static inline bool kvm_mwait_in_guest(void) > > case X86_VENDOR_AMD: > > return !boot_cpu_has_bug(X86_BUG_AMD_E400); > > case X86_VENDOR_INTEL: > > - /* Handle Intel below */ > > - break; > > + return !boot_cpu_has_bug(X86_BUG_MONITOR); > > default: > > return false; > > } > > - > > - if (boot_cpu_has_bug(X86_BUG_MONITOR)) > > - return false; > > - > > - /* > > - * Intel CPUs without CPUID5_ECX_INTERRUPT_BREAK are problematic as > > - * they would allow guest to stop the CPU completely by disabling > > - * interrupts then invoking MWAIT. > > - */ > > - if (boot_cpu_data.cpuid_level < CPUID_MWAIT_LEAF) > > - return false; > > - > > - cpuid(CPUID_MWAIT_LEAF, &eax, &ebx, &ecx, &edx); > > - > > - if (!(ecx & CPUID5_ECX_INTERRUPT_BREAK)) > > - return false; > > - > > - return true; > > } > > > > #endif > >
On Thu, May 04, 2017 at 04:33:28PM +0200, Radim Krčmář wrote: > 2017-05-04 12:58+0200, Paolo Bonzini: > > On 03/05/2017 21:37, Radim Krčmář wrote: > >> The guest can call MWAIT with ECX = 0 even if we enforce > >> CPUID5_ECX_INTERRUPT_BREAK; the call would have the exactly the same > >> effect as if the host didn't have CPUID5_ECX_INTERRUPT_BREAK. > >> > >> The check was added in some iteration while trying to fix a reported > >> OS X on Core 2 bug, but the CPU had CPUID5_ECX_INTERRUPT_BREAK and the > >> bug is elsewhere. > > > > The reason for this, as I understood it, is that we have historically > > not published leaf 5 information via KVM_GET_SUPPORTED_CPUID. For this > > reason, QEMU is publishing CPUID5_ECX_INTERRUPT_BREAK. Then if: > > I see, it was added to QEMU in e737b32a3688 ("Core 2 Duo specification > (Alexander Graf)"). > > > - the host doesn't have ECX[0]=1 support > > > > - the guest sets ECX[0] > > > > you get a #GP in the guest. So wrong comment but right thing to do. > > That userspace didn't set CPUID.01H:ECX.MONITOR[bit 3], so a guest > should get #UD instead, but MWAIT couldn't be expected to work. > > I think that the guest bug is very unlikely, so I'd get rid of the > condition anyway ... we have also recently killed support for pre-Core 2 > hosts and AFAIK, all newer Intels have it. That's a strange approach. If other software followed the same logic, it would say all newer intels have MWAIT support without checking the MWAIT leaf :) > (Not so sure about AMDs, which share the same problem, so we do need to > do more than just comment it better in any case.)
2017-05-04 21:29+0300, Michael S. Tsirkin: > On Thu, May 04, 2017 at 04:33:28PM +0200, Radim Krčmář wrote: >> 2017-05-04 12:58+0200, Paolo Bonzini: >> > On 03/05/2017 21:37, Radim Krčmář wrote: >> >> The guest can call MWAIT with ECX = 0 even if we enforce >> >> CPUID5_ECX_INTERRUPT_BREAK; the call would have the exactly the same >> >> effect as if the host didn't have CPUID5_ECX_INTERRUPT_BREAK. >> >> >> >> The check was added in some iteration while trying to fix a reported >> >> OS X on Core 2 bug, but the CPU had CPUID5_ECX_INTERRUPT_BREAK and the >> >> bug is elsewhere. >> > >> > The reason for this, as I understood it, is that we have historically >> > not published leaf 5 information via KVM_GET_SUPPORTED_CPUID. For this >> > reason, QEMU is publishing CPUID5_ECX_INTERRUPT_BREAK. Then if: >> >> I see, it was added to QEMU in e737b32a3688 ("Core 2 Duo specification >> (Alexander Graf)"). >> >> > - the host doesn't have ECX[0]=1 support >> > >> > - the guest sets ECX[0] >> > >> > you get a #GP in the guest. So wrong comment but right thing to do. >> >> That userspace didn't set CPUID.01H:ECX.MONITOR[bit 3], so a guest >> should get #UD instead, but MWAIT couldn't be expected to work. >> >> I think that the guest bug is very unlikely, so I'd get rid of the >> condition anyway ... we have also recently killed support for pre-Core 2 >> hosts and AFAIK, all newer Intels have it. > > That's a strange approach. If other software followed the same logic, > it would say all newer intels have MWAIT support without > checking the MWAIT leaf :) I'd make an analogy for the condition with CPU that cannot disable a feature because software is not checking for its presence correctly, but I wanted to convey something different. :) The condition is catching a combination of a questionable QEMU behavior and a very unlikely guest bug (only old OS X is known to use MWAIT when it should #UD). I think that handling it in KVM doesn't make sense, like with other obvious guest/QEMU bugs -- if we started from scratch, there would be no reason to have this condition. Still, we fear regressions, which is where Intel's support of that feature comes in. The KVM code can be simpler/better at no real cost. (If we keep the condition, I'd also fix Gabriel's real bug as it is far more important.)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h index 63d5fb65ea30..8ea4e80c24d1 100644 --- a/arch/x86/kvm/x86.h +++ b/arch/x86/kvm/x86.h @@ -216,8 +216,6 @@ static inline u64 nsec_to_cycles(struct kvm_vcpu *vcpu, u64 nsec) static inline bool kvm_mwait_in_guest(void) { - unsigned int eax, ebx, ecx, edx; - if (!cpu_has(&boot_cpu_data, X86_FEATURE_MWAIT)) return false; @@ -225,29 +223,10 @@ static inline bool kvm_mwait_in_guest(void) case X86_VENDOR_AMD: return !boot_cpu_has_bug(X86_BUG_AMD_E400); case X86_VENDOR_INTEL: - /* Handle Intel below */ - break; + return !boot_cpu_has_bug(X86_BUG_MONITOR); default: return false; } - - if (boot_cpu_has_bug(X86_BUG_MONITOR)) - return false; - - /* - * Intel CPUs without CPUID5_ECX_INTERRUPT_BREAK are problematic as - * they would allow guest to stop the CPU completely by disabling - * interrupts then invoking MWAIT. - */ - if (boot_cpu_data.cpuid_level < CPUID_MWAIT_LEAF) - return false; - - cpuid(CPUID_MWAIT_LEAF, &eax, &ebx, &ecx, &edx); - - if (!(ecx & CPUID5_ECX_INTERRUPT_BREAK)) - return false; - - return true; } #endif
The guest can call MWAIT with ECX = 0 even if we enforce CPUID5_ECX_INTERRUPT_BREAK; the call would have the exactly the same effect as if the host didn't have CPUID5_ECX_INTERRUPT_BREAK. The check was added in some iteration while trying to fix a reported OS X on Core 2 bug, but the CPU had CPUID5_ECX_INTERRUPT_BREAK and the bug is elsewhere. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- arch/x86/kvm/x86.h | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-)