LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] KVM: X86: Enable IA32_MSIC_ENABLE MONITOR bit when exposing mwait/monitor
@ 2019-05-13  9:46 Wanpeng Li
  2019-05-13 11:22 ` Konrad Rzeszutek Wilk
  2019-05-13 13:35 ` Radim Krčmář
  0 siblings, 2 replies; 4+ messages in thread
From: Wanpeng Li @ 2019-05-13  9:46 UTC (permalink / raw)
  To: linux-kernel, kvm
  Cc: Paolo Bonzini, Radim Krčmář,
	Sean Christopherson, Liran Alon

From: Wanpeng Li <wanpengli@tencent.com>

MSR IA32_MSIC_ENABLE bit 18, according to SDM:

 | When this bit is set to 0, the MONITOR feature flag is not set (CPUID.01H:ECX[bit 3] = 0). 
 | This indicates that MONITOR/MWAIT are not supported.
 | 
 | Software attempts to execute MONITOR/MWAIT will cause #UD when this bit is 0.
 | 
 | When this bit is set to 1 (default), MONITOR/MWAIT are supported (CPUID.01H:ECX[bit 3] = 1). 

This bit should be set to 1, if BIOS enables MONITOR/MWAIT support on host and 
we intend to expose mwait/monitor to the guest.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Liran Alon <liran.alon@oracle.com>
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 arch/x86/kvm/x86.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1d89cb9..664449e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2723,6 +2723,13 @@ static int get_msr_mce(struct kvm_vcpu *vcpu, u32 msr, u64 *pdata, bool host)
 	return 0;
 }
 
+static inline bool kvm_can_mwait_in_guest(void)
+{
+	return boot_cpu_has(X86_FEATURE_MWAIT) &&
+		!boot_cpu_has_bug(X86_BUG_MONITOR) &&
+		boot_cpu_has(X86_FEATURE_ARAT);
+}
+
 int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 {
 	switch (msr_info->index) {
@@ -2801,6 +2808,8 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = (u64)vcpu->arch.ia32_tsc_adjust_msr;
 		break;
 	case MSR_IA32_MISC_ENABLE:
+		if (kvm_can_mwait_in_guest() && kvm_mwait_in_guest(vcpu->kvm))
+			vcpu->arch.ia32_misc_enable_msr |= MSR_IA32_MISC_ENABLE_MWAIT;
 		msr_info->data = vcpu->arch.ia32_misc_enable_msr;
 		break;
 	case MSR_IA32_SMBASE:
@@ -2984,13 +2993,6 @@ static int msr_io(struct kvm_vcpu *vcpu, struct kvm_msrs __user *user_msrs,
 	return r;
 }
 
-static inline bool kvm_can_mwait_in_guest(void)
-{
-	return boot_cpu_has(X86_FEATURE_MWAIT) &&
-		!boot_cpu_has_bug(X86_BUG_MONITOR) &&
-		boot_cpu_has(X86_FEATURE_ARAT);
-}
-
 int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 {
 	int r = 0;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: X86: Enable IA32_MSIC_ENABLE MONITOR bit when exposing mwait/monitor
  2019-05-13  9:46 [PATCH] KVM: X86: Enable IA32_MSIC_ENABLE MONITOR bit when exposing mwait/monitor Wanpeng Li
@ 2019-05-13 11:22 ` Konrad Rzeszutek Wilk
  2019-05-13 13:35 ` Radim Krčmář
  1 sibling, 0 replies; 4+ messages in thread
From: Konrad Rzeszutek Wilk @ 2019-05-13 11:22 UTC (permalink / raw)
  To: Wanpeng Li, linux-kernel, kvm
  Cc: Paolo Bonzini, Radim Krčmář,
	Sean Christopherson, Liran Alon

On May 13, 2019 5:46:39 AM EDT, Wanpeng Li <kernellwp@gmail.com> wrote:
>From: Wanpeng Li <wanpengli@tencent.com>
>
>MSR IA32_MSIC_ENABLE bit 18, according to SDM:
>

 MSIC? (Also the $subject)


>| When this bit is set to 0, the MONITOR feature flag is not set
>(CPUID.01H:ECX[bit 3] = 0). 
> | This indicates that MONITOR/MWAIT are not supported.
> | 
>| Software attempts to execute MONITOR/MWAIT will cause #UD when this
>bit is 0.
> | 
>| When this bit is set to 1 (default), MONITOR/MWAIT are supported
>(CPUID.01H:ECX[bit 3] = 1). 
>
>This bit should be set to 1, if BIOS enables MONITOR/MWAIT support on
>host and 
>we intend to expose mwait/monitor to the guest.
>
>Cc: Paolo Bonzini <pbonzini@redhat.com>
>Cc: Radim Krčmář <rkrcmar@redhat.com>
>Cc: Sean Christopherson <sean.j.christopherson@intel.com>
>Cc: Liran Alon <liran.alon@oracle.com>
>Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
>---
> arch/x86/kvm/x86.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index 1d89cb9..664449e 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -2723,6 +2723,13 @@ static int get_msr_mce(struct kvm_vcpu *vcpu,
>u32 msr, u64 *pdata, bool host)
> 	return 0;
> }
> 
>+static inline bool kvm_can_mwait_in_guest(void)
>+{
>+	return boot_cpu_has(X86_FEATURE_MWAIT) &&
>+		!boot_cpu_has_bug(X86_BUG_MONITOR) &&
>+		boot_cpu_has(X86_FEATURE_ARAT);
>+}
>+
>int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data
>*msr_info)
> {
> 	switch (msr_info->index) {
>@@ -2801,6 +2808,8 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu,
>struct msr_data *msr_info)
> 		msr_info->data = (u64)vcpu->arch.ia32_tsc_adjust_msr;
> 		break;
> 	case MSR_IA32_MISC_ENABLE:
>+		if (kvm_can_mwait_in_guest() && kvm_mwait_in_guest(vcpu->kvm))
>+			vcpu->arch.ia32_misc_enable_msr |= MSR_IA32_MISC_ENABLE_MWAIT;
> 		msr_info->data = vcpu->arch.ia32_misc_enable_msr;
> 		break;
> 	case MSR_IA32_SMBASE:
>@@ -2984,13 +2993,6 @@ static int msr_io(struct kvm_vcpu *vcpu, struct
>kvm_msrs __user *user_msrs,
> 	return r;
> }
> 
>-static inline bool kvm_can_mwait_in_guest(void)
>-{
>-	return boot_cpu_has(X86_FEATURE_MWAIT) &&
>-		!boot_cpu_has_bug(X86_BUG_MONITOR) &&
>-		boot_cpu_has(X86_FEATURE_ARAT);
>-}
>-
> int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> {
> 	int r = 0;


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: X86: Enable IA32_MSIC_ENABLE MONITOR bit when exposing mwait/monitor
  2019-05-13  9:46 [PATCH] KVM: X86: Enable IA32_MSIC_ENABLE MONITOR bit when exposing mwait/monitor Wanpeng Li
  2019-05-13 11:22 ` Konrad Rzeszutek Wilk
@ 2019-05-13 13:35 ` Radim Krčmář
  2019-05-14  6:13   ` Wanpeng Li
  1 sibling, 1 reply; 4+ messages in thread
From: Radim Krčmář @ 2019-05-13 13:35 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: linux-kernel, kvm, Paolo Bonzini, Sean Christopherson, Liran Alon

2019-05-13 17:46+0800, Wanpeng Li:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> MSR IA32_MSIC_ENABLE bit 18, according to SDM:
> 
>  | When this bit is set to 0, the MONITOR feature flag is not set (CPUID.01H:ECX[bit 3] = 0). 
>  | This indicates that MONITOR/MWAIT are not supported.
>  | 
>  | Software attempts to execute MONITOR/MWAIT will cause #UD when this bit is 0.
>  | 
>  | When this bit is set to 1 (default), MONITOR/MWAIT are supported (CPUID.01H:ECX[bit 3] = 1). 
> 
> This bit should be set to 1, if BIOS enables MONITOR/MWAIT support on host and 
> we intend to expose mwait/monitor to the guest.

The CPUID.01H:ECX[bit 3] ought to mirror the value of the MSR bit and
as userspace has control of them both, I'd argue that it is userspace's
job to configure both bits to match on the initial setup.

Also, CPUID.01H:ECX[bit 3] is a better guard than kvm_mwait_in_guest().
kvm_mwait_in_guest() affects the behavior of MONITOR/MWAIT, not its
guest visibility.
Some weird migration cases might want MONITOR in CPUID without
kvm_mwait_in_guest() and the MSR should be correct there as well.

Missing the MSR bit shouldn't be a big problem for guests, so I am in
favor of fixing the userspace code.

Thanks.

(For extra correctness in KVM, we could implement toggling of the CPUID
 bit based on guest writes to the MSR.)

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: X86: Enable IA32_MSIC_ENABLE MONITOR bit when exposing mwait/monitor
  2019-05-13 13:35 ` Radim Krčmář
@ 2019-05-14  6:13   ` Wanpeng Li
  0 siblings, 0 replies; 4+ messages in thread
From: Wanpeng Li @ 2019-05-14  6:13 UTC (permalink / raw)
  To: Radim Krčmář
  Cc: LKML, kvm, Paolo Bonzini, Sean Christopherson, Liran Alon

On Mon, 13 May 2019 at 21:35, Radim Krčmář <rkrcmar@redhat.com> wrote:
>
> 2019-05-13 17:46+0800, Wanpeng Li:
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > MSR IA32_MSIC_ENABLE bit 18, according to SDM:
> >
> >  | When this bit is set to 0, the MONITOR feature flag is not set (CPUID.01H:ECX[bit 3] = 0).
> >  | This indicates that MONITOR/MWAIT are not supported.
> >  |
> >  | Software attempts to execute MONITOR/MWAIT will cause #UD when this bit is 0.
> >  |
> >  | When this bit is set to 1 (default), MONITOR/MWAIT are supported (CPUID.01H:ECX[bit 3] = 1).
> >
> > This bit should be set to 1, if BIOS enables MONITOR/MWAIT support on host and
> > we intend to expose mwait/monitor to the guest.
>
> The CPUID.01H:ECX[bit 3] ought to mirror the value of the MSR bit and
> as userspace has control of them both, I'd argue that it is userspace's
> job to configure both bits to match on the initial setup.
>
> Also, CPUID.01H:ECX[bit 3] is a better guard than kvm_mwait_in_guest().
> kvm_mwait_in_guest() affects the behavior of MONITOR/MWAIT, not its
> guest visibility.
> Some weird migration cases might want MONITOR in CPUID without
> kvm_mwait_in_guest() and the MSR should be correct there as well.
>
> Missing the MSR bit shouldn't be a big problem for guests, so I am in
> favor of fixing the userspace code.
>
> Thanks.
>
> (For extra correctness in KVM, we could implement toggling of the CPUID
>  bit based on guest writes to the MSR.)

Do it in v2 and a separate qemu patch, thanks for your review.

Regards,
Wanpeng Li

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-05-14  6:12 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-13  9:46 [PATCH] KVM: X86: Enable IA32_MSIC_ENABLE MONITOR bit when exposing mwait/monitor Wanpeng Li
2019-05-13 11:22 ` Konrad Rzeszutek Wilk
2019-05-13 13:35 ` Radim Krčmář
2019-05-14  6:13   ` Wanpeng Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).