[PATCH 0/4] KVM: x86: kvm_mwait_in_guest() cleanup and fixes

[PATCH 0/4] KVM: x86: kvm_mwait_in_guest() cleanup and fixes

[Radim Krčmář]

kvm_mwait_in_guest() was overcomplicated and also missed one AMD bug
that should prevent MWAIT pass through.

This series ignores errata that don't have any Linux bug defined;
I know of two minor (not affecting the host) Core 2 errata:
  AG36.  Split Locked Stores May not Trigger the Monitoring Hardware
  AG106.  A REP STOS/MOVS to a MONITOR/MWAIT Address Range May Prevent
          Triggering of the Monitoring Hardware

None of them are really worthy of a new condition if Linux never hit
them ... we still have the OS X bug that Gabriel is hitting, but I'm ok
with the original approach that sacrificed it for "greater good".


Radim Krčmář (4):
  KVM: svm: prevent MWAIT in guest with erratum 400
  KVM: x86: prevent MWAIT in guest with buggy MONITOR
  KVM: x86: drop bogus MWAIT check
  KVM: x86: simplify kvm_mwait_in_guest()

 arch/x86/kvm/x86.h | 33 +++------------------------------
 1 file changed, 3 insertions(+), 30 deletions(-)

-- 
2.12.2

[PATCH 1/4] KVM: svm: prevent MWAIT in guest with erratum 400

[Radim Krčmář]

The host might miss APIC timer interrupts if the guest enters a specific
C-state.  Quoting the erratum:

  400 APIC Timer Interrupt Does Not Occur in Processor C-States

  Description

  An APIC timer interrupt that becomes pending in low-power states C1E
  or C3 will not cause the processor to enter the C0 state even if the
  interrupt is enabled by Timer Local Vector Table Entry[Mask],
  APIC320[16]). APIC timer functionality is otherwise unaffected.

  Potential Effect on System

  System hang may occur provided that the operating system has not
  configured another interrupt source.  APIC timer interrupts may be
  delayed or, when the APIC timer is configured in rollover mode
  (APIC320[17]), the APIC timer may roll over multiple times in the
  low-power state with only one interrupt presented after the processor
  resumes. The standard use of the APIC timer does not make this effect
  significant.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/x86.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 612067074905..3ed7dd8737ab 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -223,8 +223,7 @@ static inline bool kvm_mwait_in_guest(void)
 
 	switch (boot_cpu_data.x86_vendor) {
 	case X86_VENDOR_AMD:
-		/* All AMD CPUs have a working MWAIT implementation */
-		return true;
+		return !boot_cpu_has_bug(X86_BUG_AMD_E400);
 	case X86_VENDOR_INTEL:
 		/* Handle Intel below */
 		break;
-- 
2.12.2

[PATCH 2/4] KVM: x86: prevent MWAIT in guest with buggy MONITOR

[Radim Krčmář]

The bug prevents MWAIT from waking up after a write to the monitored
cache line.
KVM might emulate a CPU model that shouldn't have the bug, so the guest
would not employ a workaround and possibly miss wakeups.
Better to avoid the situation.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/x86.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 3ed7dd8737ab..63d5fb65ea30 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -231,6 +231,9 @@ static inline bool kvm_mwait_in_guest(void)
 		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
-- 
2.12.2

[PATCH 3/4] KVM: x86: drop bogus MWAIT check

[Radim Krčmář]

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(-)

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
-- 
2.12.2

[PATCH 4/4] KVM: x86: simplify kvm_mwait_in_guest()

[Radim Krčmář]

If Intel/AMD implements MWAIT, we expect that it works well and only
reject known bugs;  no reason to do it the other way around for minor
vendors.  (Not that they are relevant ATM.)

This allows further simplification of kvm_mwait_in_guest().

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/kvm/x86.h | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 8ea4e80c24d1..b49add75ea35 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -216,17 +216,9 @@ static inline u64 nsec_to_cycles(struct kvm_vcpu *vcpu, u64 nsec)
 
 static inline bool kvm_mwait_in_guest(void)
 {
-	if (!cpu_has(&boot_cpu_data, X86_FEATURE_MWAIT))
-		return false;
-
-	switch (boot_cpu_data.x86_vendor) {
-	case X86_VENDOR_AMD:
-		return !boot_cpu_has_bug(X86_BUG_AMD_E400);
-	case X86_VENDOR_INTEL:
-		return !boot_cpu_has_bug(X86_BUG_MONITOR);
-	default:
-		return false;
-	}
+	return boot_cpu_has(X86_FEATURE_MWAIT) &&
+		!boot_cpu_has_bug(X86_BUG_AMD_E400) &&
+		!boot_cpu_has_bug(X86_BUG_MONITOR);
 }
 
 #endif
-- 
2.12.2

Re: [PATCH 0/4] KVM: x86: kvm_mwait_in_guest() cleanup and fixes

[Alexander Graf]

On 03.05.17 21:37, Radim Krčmář wrote:
> kvm_mwait_in_guest() was overcomplicated and also missed one AMD bug
> that should prevent MWAIT pass through.
>
> This series ignores errata that don't have any Linux bug defined;
> I know of two minor (not affecting the host) Core 2 errata:
>   AG36.  Split Locked Stores May not Trigger the Monitoring Hardware
>   AG106.  A REP STOS/MOVS to a MONITOR/MWAIT Address Range May Prevent
>           Triggering of the Monitoring Hardware
>
> None of them are really worthy of a new condition if Linux never hit
> them ... we still have the OS X bug that Gabriel is hitting, but I'm ok
> with the original approach that sacrificed it for "greater good".

I like the series :)

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

Re: [PATCH 1/4] KVM: svm: prevent MWAIT in guest with erratum 400

[Borislav Petkov]

On Wed, May 03, 2017 at 09:37:30PM +0200, Radim Krčmář wrote:
> The host might miss APIC timer interrupts if the guest enters a specific
> C-state.  Quoting the erratum:
> 
>   400 APIC Timer Interrupt Does Not Occur in Processor C-States
> 
>   Description
> 
>   An APIC timer interrupt that becomes pending in low-power states C1E
>   or C3 will not cause the processor to enter the C0 state even if the
>   interrupt is enabled by Timer Local Vector Table Entry[Mask],
>   APIC320[16]). APIC timer functionality is otherwise unaffected.
> 
>   Potential Effect on System
> 
>   System hang may occur provided that the operating system has not
>   configured another interrupt source.  APIC timer interrupts may be
>   delayed or, when the APIC timer is configured in rollover mode
>   (APIC320[17]), the APIC timer may roll over multiple times in the
>   low-power state with only one interrupt presented after the processor
>   resumes. The standard use of the APIC timer does not make this effect
>   significant.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/kvm/x86.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 612067074905..3ed7dd8737ab 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -223,8 +223,7 @@ static inline bool kvm_mwait_in_guest(void)
>  
>  	switch (boot_cpu_data.x86_vendor) {
>  	case X86_VENDOR_AMD:
> -		/* All AMD CPUs have a working MWAIT implementation */
> -		return true;
> +		return !boot_cpu_has_bug(X86_BUG_AMD_E400);

Well, this looks wrong: it is X86_BUG_AMD_APIC_C1E, which actually
denotes that we must enable the E400 workaround because the platform
actually goes into C1E.

X86_BUG_AMD_E400 gets set only on the affected f/m/s range but if the
BIOS doesn't put the CPU in C1E, we don't hit the erratum and all is
peachy.

Also, what do APIC timer interrupts even have to do with MWAIT-ing in
the guest, especially if we enable the workaround and switch to HPET on
the host? Maybe I'm missing something here...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 3/4] KVM: x86: drop bogus MWAIT check

[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:

- 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
> 

Re: [PATCH 1/4] KVM: svm: prevent MWAIT in guest with erratum 400

[Radim Krčmář]

2017-05-03 22:11+0200, Borislav Petkov:
> On Wed, May 03, 2017 at 09:37:30PM +0200, Radim Krčmář wrote:
>> The host might miss APIC timer interrupts if the guest enters a specific
>> C-state.  Quoting the erratum:
>> 
>>   400 APIC Timer Interrupt Does Not Occur in Processor C-States
>> 
>>   Description
>> 
>>   An APIC timer interrupt that becomes pending in low-power states C1E
>>   or C3 will not cause the processor to enter the C0 state even if the
>>   interrupt is enabled by Timer Local Vector Table Entry[Mask],
>>   APIC320[16]). APIC timer functionality is otherwise unaffected.
>> 
>>   Potential Effect on System
>> 
>>   System hang may occur provided that the operating system has not
>>   configured another interrupt source.  APIC timer interrupts may be
>>   delayed or, when the APIC timer is configured in rollover mode
>>   (APIC320[17]), the APIC timer may roll over multiple times in the
>>   low-power state with only one interrupt presented after the processor
>>   resumes. The standard use of the APIC timer does not make this effect
>>   significant.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> ---
>>  arch/x86/kvm/x86.h | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>> index 612067074905..3ed7dd8737ab 100644
>> --- a/arch/x86/kvm/x86.h
>> +++ b/arch/x86/kvm/x86.h
>> @@ -223,8 +223,7 @@ static inline bool kvm_mwait_in_guest(void)
>>  
>>  	switch (boot_cpu_data.x86_vendor) {
>>  	case X86_VENDOR_AMD:
>> -		/* All AMD CPUs have a working MWAIT implementation */
>> -		return true;
>> +		return !boot_cpu_has_bug(X86_BUG_AMD_E400);
> 
> Well, this looks wrong: it is X86_BUG_AMD_APIC_C1E, which actually
> denotes that we must enable the E400 workaround because the platform
> actually goes into C1E.

X86_BUG_AMD_APIC_C1E doesn't cover C3, which is why I used
X86_BUG_AMD_E400.

> X86_BUG_AMD_E400 gets set only on the affected f/m/s range but if the
> BIOS doesn't put the CPU in C1E, we don't hit the erratum and all is
> peachy.
> 
> Also, what do APIC timer interrupts even have to do with MWAIT-ing in
> the guest, especially if we enable the workaround and switch to HPET on
> the host? Maybe I'm missing something here...

The host uses APIC timer when entering a guest and I assumed that MWAIT
can change C states, but it seems that affected AMD models do not even
support MWAIT hints and the package is in C0 the whole time.

I'll drop this patch if this is what you meant, thanks.

Re: [PATCH 3/4] KVM: x86: drop bogus MWAIT check

[Radim Krčmář]

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.)

Re: [PATCH 1/4] KVM: svm: prevent MWAIT in guest with erratum 400

[Borislav Petkov]

On Thu, May 04, 2017 at 04:02:44PM +0200, Radim Krčmář wrote:
> X86_BUG_AMD_APIC_C1E doesn't cover C3, which is why I used
> X86_BUG_AMD_E400.

Practically, the CPUs which even support C3 with E400 are single-core
devices and C3 support was removed with revision C3 (yap, the same as
the ACPI state) so I think we can safely ignore C3 here.

I mean, otherwise, we would be seeing E400 in action as our workaround
would not be sufficient. And even then, I don't think we ever go into C3
on those machines as we do HLT which enters C1.

> The host uses APIC timer when entering a guest and I assumed that MWAIT
> can change C states, but it seems that affected AMD models do not even
> support MWAIT hints and the package is in C0 the whole time.

Right, we never do MWAIT when idle on AMD but HLT.

I notice now that MWAIT text has received recent additions talking
about C-state hints but we'd need to evaluate that first and how power
consumption behaves and it probably would work the same way as it does
on Intel.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 0/4] KVM: x86: kvm_mwait_in_guest() cleanup and fixes

[Gabriel L. Somlo]

Hi Radim,

On Wed, May 03, 2017 at 09:37:29PM +0200, Radim Krčmář wrote:
> kvm_mwait_in_guest() was overcomplicated and also missed one AMD bug
> that should prevent MWAIT pass through.
> 
> This series ignores errata that don't have any Linux bug defined;
> I know of two minor (not affecting the host) Core 2 errata:
>   AG36.  Split Locked Stores May not Trigger the Monitoring Hardware
>   AG106.  A REP STOS/MOVS to a MONITOR/MWAIT Address Range May Prevent
>           Triggering of the Monitoring Hardware
> 
> None of them are really worthy of a new condition if Linux never hit
> them ... we still have the OS X bug that Gabriel is hitting, but I'm ok
> with the original approach that sacrificed it for "greater good".

If I wanted to test this (e.g. with OS X 10.8 guests on several of my older
Mac boxes running Fedora), which git repo would you have me use? (The series
won't apply directly on top of git://git.kernel.org/pub/scm/virt/kvm/kvm.git).

Thanks much,
--Gabriel

> Radim Krčmář (4):
>   KVM: svm: prevent MWAIT in guest with erratum 400
>   KVM: x86: prevent MWAIT in guest with buggy MONITOR
>   KVM: x86: drop bogus MWAIT check
>   KVM: x86: simplify kvm_mwait_in_guest()
> 
>  arch/x86/kvm/x86.h | 33 +++------------------------------
>  1 file changed, 3 insertions(+), 30 deletions(-)
> 
> -- 
> 2.12.2
> 

Re: [PATCH 0/4] KVM: x86: kvm_mwait_in_guest() cleanup and fixes

[Radim Krčmář]

2017-05-04 13:56-0400, Gabriel L. Somlo:
> If I wanted to test this (e.g. with OS X 10.8 guests on several of my older
> Mac boxes running Fedora), which git repo would you have me use? (The series
> won't apply directly on top of git://git.kernel.org/pub/scm/virt/kvm/kvm.git).

The queue branch of that repo.  This series depends on a patch that is
applied there:

  668fffa3f838 kvm: better MWAIT emulation for guests

I forgot to mention that, sorry.

Re: [PATCH 3/4] KVM: x86: drop bogus MWAIT check

[Michael S. Tsirkin]

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
> > 

Re: [PATCH 3/4] KVM: x86: drop bogus MWAIT check

[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 :)

> (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.)
-- 
MST

Re: [PATCH 3/4] KVM: x86: drop bogus MWAIT check

[Radim Krčmář]

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.)

Re: [PATCH 0/4] KVM: x86: kvm_mwait_in_guest() cleanup and fixes

[Gabriel L. Somlo]

On Thu, May 04, 2017 at 08:07:15PM +0200, Radim Krčmář wrote:
> 2017-05-04 13:56-0400, Gabriel L. Somlo:
> > If I wanted to test this (e.g. with OS X 10.8 guests on several of my older
> > Mac boxes running Fedora), which git repo would you have me use? (The series
> > won't apply directly on top of git://git.kernel.org/pub/scm/virt/kvm/kvm.git).
> 
> The queue branch of that repo.  This series depends on a patch that is
> applied there:
> 
>   668fffa3f838 kvm: better MWAIT emulation for guests
> 
> I forgot to mention that, sorry.

Thanks; right now, I get this:

# modprobe -v kvm-intel
insmod /lib/modules/4.11.0-rc3+/kernel/virt/lib/irqbypass.ko 
insmod /lib/modules/4.11.0-rc3+/kernel/arch/x86/kvm/kvm.ko 
insmod /lib/modules/4.11.0-rc3+/kernel/arch/x86/kvm/kvm-intel.ko 
modprobe: ERROR: could not insert 'kvm_intel': Input/output error

but that appears to have nothing to do with the MWAIT patches. I'm
bisecting to find the root cause, but it's really slow...

I'll follow up when I know more... Thanks,
--Gabriel

Re: [PATCH 0/4] KVM: x86: kvm_mwait_in_guest() cleanup and fixes

[Gabriel L. Somlo]

On Thu, May 04, 2017 at 08:07:15PM +0200, Radim Krčmář wrote:
> 2017-05-04 13:56-0400, Gabriel L. Somlo:
> > If I wanted to test this (e.g. with OS X 10.8 guests on several of my older
> > Mac boxes running Fedora), which git repo would you have me use? (The series
> > won't apply directly on top of git://git.kernel.org/pub/scm/virt/kvm/kvm.git).
> 
> The queue branch of that repo.  This series depends on a patch that is
> applied there:
> 
>   668fffa3f838 kvm: better MWAIT emulation for guests
> 
> I forgot to mention that, sorry.

OK, here's where I'm at right now:

With this series applied on top of 'queue', my MacbookAir4,2 running
F25 (with the kvm/queue kernel) works fine, i.e. loads the kvm-intel
module successfully, and mwaits in L1 guest mode, reporting 400% cpu
but staying cool (guest started with -smp 4).

So far, so good.

On the MacPro1,1, I first had to revert 2c82878b0cb38fd
("KVM: VMX: require virtual NMI support") to get around this error:

# modprobe -v kvm-intel
insmod /lib/modules/4.11.0-rc3+/kernel/virt/lib/irqbypass.ko
insmod /lib/modules/4.11.0-rc3+/kernel/arch/x86/kvm/kvm.ko
insmod /lib/modules/4.11.0-rc3+/kernel/arch/x86/kvm/kvm-intel.ko
modprobe: ERROR: could not insert 'kvm_intel': Input/output error

Next, it turns out that on the MacPro1,1 kvm_mwait_in_guest() returns
TRUE, which causes OS X 10.7 (the one that mwaits without checking
CPUID) to misbehave. Forcing the function to return 0 (FALSE) solves
the problem:

diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index b49add7..249362c 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -216,9 +216,12 @@ static inline u64 nsec_to_cycles(struct kvm_vcpu *vcpu, u64 nsec)
 
 static inline bool kvm_mwait_in_guest(void)
 {
-       return boot_cpu_has(X86_FEATURE_MWAIT) &&
+       bool ret;
+       ret =  boot_cpu_has(X86_FEATURE_MWAIT) &&
                !boot_cpu_has_bug(X86_BUG_AMD_E400) &&
                !boot_cpu_has_bug(X86_BUG_MONITOR);
+       printk(KERN_INFO "kvm_mwait_in_guest: %d\n", ret);
+       return 0;
 }
 
 #endif

After this change, I get:

[ 1201.529002] kvm_mwait_in_guest: 1
[ 1201.529024] kvm_mwait_in_guest: 1
[ 1201.529029] kvm_mwait_in_guest: 1
[ 1201.529038] kvm_mwait_in_guest: 1
[ 1201.529047] kvm_mwait_in_guest: 1
[ 1225.150235] kvm: MONITOR instruction emulated as NOP!
[ 1225.150240] kvm: MWAIT instruction emulated as NOP!

indicating that it *would* have returned TRUE if I let it :)

This is a 2x dual-core Xeon, cca 2006 vintage, and the last (4th) CPU
in /proc/cpuinfo returns:

processor       : 3
vendor_id       : GenuineIntel
cpu family      : 6
model           : 15
model name      : Intel(R) Xeon(R) CPU            5150  @ 2.66GHz
stepping        : 6
microcode       : 0xd2
cpu MHz         : 2659.977
cache size      : 4096 KB
physical id     : 3
siblings        : 2
core id         : 0
cpu cores       : 2
apicid          : 6
initial apicid  : 6
fpu             : yes
fpu_exception   : yes
cpuid level     : 10
wp              : yes
flags           : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx lm constant_tsc arch_perfmon pebs bts rep_good nopl cpuid aperfmperf pni dtes64 monitor ds_cpl vmx est tm2 ssse3 cx16 xtpr pdcm dca lahf_lm tpr_shadow dtherm
bugs            :
bogomips        : 5320.03
clflush size    : 64
cache_alignment : 64
address sizes   : 36 bits physical, 48 bits virtual
power management:

So, in conclusion; it's not important to *me* that this old machine
keeps working, I'm just volunteering test data points. So please don't
feel obligated in any way to go out of your way on my account. OTOH,
I'm happy to provide feedback as long as you would like me to.

Along the same lines: Paolo, as the author of commit 2c82878b0cb38fd,
is the Xeon chip listed above one of the "obsolete for virtualization"
models ? In that case, it makes no sense for me to keep using it for
tests, and the fact that it misbehaves with L1 MWAIT should also not
matter at all.

Let me know what you all think.

Thanks,
--Gabriel

Re: [PATCH 0/4] KVM: x86: kvm_mwait_in_guest() cleanup and fixes

[Paolo Bonzini]

On 06/05/2017 18:48, Gabriel L. Somlo wrote:
> So, in conclusion; it's not important to *me* that this old machine
> keeps working, I'm just volunteering test data points. So please don't
> feel obligated in any way to go out of your way on my account. OTOH,
> I'm happy to provide feedback as long as you would like me to.
> 
> Along the same lines: Paolo, as the author of commit 2c82878b0cb38fd,
> is the Xeon chip listed above one of the "obsolete for virtualization"
> models ?

Yes - I hadn't tested this model in particular, and this one is a little
less obsolete compared to the ones I found without NMI support (a 64-bit
Prescott and a 32-bit Yonah), but I still believe it's saner to treat
them as obsolete.

Can you please run vmxcap (from QEMU's git repository) on that processor
and include the output?

Paolo

> In that case, it makes no sense for me to keep using it for
> tests, and the fact that it misbehaves with L1 MWAIT should also not
> matter at all.