[PATCH 0/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[PATCH 0/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[Paolo Bonzini]

Patch 1 ensures that all aspects of MPX are disabled when eager FPU
is disabled on the host.  Patch 2 is just a cleanup.

Paolo Bonzini (2):
  KVM: x86: disable MPX if host did not enable MPX XSAVE features
  KVM: x86: remove eager_fpu field of struct kvm_vcpu_arch

 arch/x86/include/asm/kvm_host.h |  1 -
 arch/x86/kvm/cpuid.c            | 14 ++++++++++----
 arch/x86/kvm/cpuid.h            |  9 +--------
 arch/x86/kvm/vmx.c              | 13 ++++++-------
 arch/x86/kvm/x86.c              |  2 +-
 5 files changed, 18 insertions(+), 21 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[Paolo Bonzini]

When eager FPU is disabled, KVM will still see the MPX bit in CPUID and
presumably the MPX vmentry and vmexit controls.  However, it will not
be able to expose the MPX XSAVE features to the guest, because the guest's
accessible XSAVE features are always a subset of host_xcr0.

In this case, we should disable the MPX CPUID bit, the BNDCFGS MSR,
and the MPX vmentry and vmexit controls for nested virtualization.
It is then unnecessary to enable guest eager FPU if the guest has the
MPX CPUID bit set.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/cpuid.c | 13 ++++++++++---
 arch/x86/kvm/cpuid.h |  9 +--------
 arch/x86/kvm/vmx.c   | 13 ++++++-------
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 6525e926f566..fa241d4fda98 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -46,11 +46,18 @@ static u32 xstate_required_size(u64 xstate_bv, bool compacted)
 	return ret;
 }
 
+bool kvm_mpx_supported(void)
+{
+	return ((host_xcr0 & (XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR))
+		 && kvm_x86_ops->mpx_supported());
+}
+EXPORT_SYMBOL_GPL(kvm_mpx_supported);
+
 u64 kvm_supported_xcr0(void)
 {
 	u64 xcr0 = KVM_SUPPORTED_XCR0 & host_xcr0;
 
-	if (!kvm_x86_ops->mpx_supported())
+	if (!kvm_mpx_supported())
 		xcr0 &= ~(XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR);
 
 	return xcr0;
@@ -97,7 +104,7 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 	if (best && (best->eax & (F(XSAVES) | F(XSAVEC))))
 		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
 
-	vcpu->arch.eager_fpu = use_eager_fpu() || guest_cpuid_has_mpx(vcpu);
+	vcpu->arch.eager_fpu = use_eager_fpu();
 	if (vcpu->arch.eager_fpu)
 		kvm_x86_ops->fpu_activate(vcpu);
 
@@ -295,7 +302,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 #endif
 	unsigned f_rdtscp = kvm_x86_ops->rdtscp_supported() ? F(RDTSCP) : 0;
 	unsigned f_invpcid = kvm_x86_ops->invpcid_supported() ? F(INVPCID) : 0;
-	unsigned f_mpx = kvm_x86_ops->mpx_supported() ? F(MPX) : 0;
+	unsigned f_mpx = kvm_mpx_supported() ? F(MPX) : 0;
 	unsigned f_xsaves = kvm_x86_ops->xsaves_supported() ? F(XSAVES) : 0;
 
 	/* cpuid 1.edx */
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index c8eda1498121..66a6581724ad 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -5,6 +5,7 @@
 #include <asm/cpu.h>
 
 int kvm_update_cpuid(struct kvm_vcpu *vcpu);
+bool kvm_mpx_supported(void);
 struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
 					      u32 function, u32 index);
 int kvm_dev_ioctl_get_cpuid(struct kvm_cpuid2 *cpuid,
@@ -135,14 +136,6 @@ static inline bool guest_cpuid_has_rtm(struct kvm_vcpu *vcpu)
 	return best && (best->ebx & bit(X86_FEATURE_RTM));
 }
 
-static inline bool guest_cpuid_has_mpx(struct kvm_vcpu *vcpu)
-{
-	struct kvm_cpuid_entry2 *best;
-
-	best = kvm_find_cpuid_entry(vcpu, 7, 0);
-	return best && (best->ebx & bit(X86_FEATURE_MPX));
-}
-
 static inline bool guest_cpuid_has_pcommit(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 46154dac71e6..e512aa7ed874 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -861,7 +861,6 @@ static unsigned long nested_ept_get_cr3(struct kvm_vcpu *vcpu);
 static u64 construct_eptp(unsigned long root_hpa);
 static void kvm_cpu_vmxon(u64 addr);
 static void kvm_cpu_vmxoff(void);
-static bool vmx_mpx_supported(void);
 static bool vmx_xsaves_supported(void);
 static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr);
 static void vmx_set_segment(struct kvm_vcpu *vcpu,
@@ -2595,7 +2594,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
 		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER |
 		VM_EXIT_SAVE_VMX_PREEMPTION_TIMER | VM_EXIT_ACK_INTR_ON_EXIT;
 
-	if (vmx_mpx_supported())
+	if (kvm_mpx_supported())
 		vmx->nested.nested_vmx_exit_ctls_high |= VM_EXIT_CLEAR_BNDCFGS;
 
 	/* We support free control of debug control saving. */
@@ -2616,7 +2615,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
 		VM_ENTRY_LOAD_IA32_PAT;
 	vmx->nested.nested_vmx_entry_ctls_high |=
 		(VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR | VM_ENTRY_LOAD_IA32_EFER);
-	if (vmx_mpx_supported())
+	if (kvm_mpx_supported())
 		vmx->nested.nested_vmx_entry_ctls_high |= VM_ENTRY_LOAD_BNDCFGS;
 
 	/* We support free control of debug control loading. */
@@ -2860,7 +2859,7 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		msr_info->data = vmcs_readl(GUEST_SYSENTER_ESP);
 		break;
 	case MSR_IA32_BNDCFGS:
-		if (!vmx_mpx_supported())
+		if (!kvm_mpx_supported())
 			return 1;
 		msr_info->data = vmcs_read64(GUEST_BNDCFGS);
 		break;
@@ -2937,7 +2936,7 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		vmcs_writel(GUEST_SYSENTER_ESP, data);
 		break;
 	case MSR_IA32_BNDCFGS:
-		if (!vmx_mpx_supported())
+		if (!kvm_mpx_supported())
 			return 1;
 		vmcs_write64(GUEST_BNDCFGS, data);
 		break;
@@ -3410,7 +3409,7 @@ static void init_vmcs_shadow_fields(void)
 	for (i = j = 0; i < max_shadow_read_write_fields; i++) {
 		switch (shadow_read_write_fields[i]) {
 		case GUEST_BNDCFGS:
-			if (!vmx_mpx_supported())
+			if (!kvm_mpx_supported())
 				continue;
 			break;
 		default:
@@ -10265,7 +10264,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12,
 	vmcs12->guest_sysenter_cs = vmcs_read32(GUEST_SYSENTER_CS);
 	vmcs12->guest_sysenter_esp = vmcs_readl(GUEST_SYSENTER_ESP);
 	vmcs12->guest_sysenter_eip = vmcs_readl(GUEST_SYSENTER_EIP);
-	if (vmx_mpx_supported())
+	if (kvm_mpx_supported())
 		vmcs12->guest_bndcfgs = vmcs_read64(GUEST_BNDCFGS);
 	if (nested_cpu_has_xsaves(vmcs12))
 		vmcs12->xss_exit_bitmap = vmcs_read64(XSS_EXIT_BITMAP);
-- 
1.8.3.1

[PATCH 2/2] KVM: x86: remove eager_fpu field of struct kvm_vcpu_arch

[Paolo Bonzini]

It is now equal to use_eager_fpu(), which simply tests a cpufeature bit.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/include/asm/kvm_host.h | 1 -
 arch/x86/kvm/cpuid.c            | 3 +--
 arch/x86/kvm/x86.c              | 2 +-
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d110dc44d6c2..01c8b501cb6d 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -503,7 +503,6 @@ struct kvm_vcpu_arch {
 	struct kvm_mmu_memory_cache mmu_page_header_cache;
 
 	struct fpu guest_fpu;
-	bool eager_fpu;
 	u64 xcr0;
 	u64 guest_supported_xcr0;
 	u32 guest_xstate_size;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 00aadec50ba3..0b3ed0b27d64 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -97,8 +97,7 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 	if (best && (best->eax & (F(XSAVES) | F(XSAVEC))))
 		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
 
-	vcpu->arch.eager_fpu = use_eager_fpu();
-	if (vcpu->arch.eager_fpu)
+	if (use_eager_fpu())
 		kvm_x86_ops->fpu_activate(vcpu);
 
 	/*
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8daaa0e0830f..08752b8c935c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7330,7 +7330,7 @@ void kvm_put_guest_fpu(struct kvm_vcpu *vcpu)
 	 * Every 255 times fpu_counter rolls over to 0; a guest that uses
 	 * the FPU in bursts will revert to loading it on demand.
 	 */
-	if (!vcpu->arch.eager_fpu) {
+	if (!use_eager_fpu()) {
 		if (++vcpu->fpu_counter < 5)
 			kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
 	}
-- 
1.8.3.1

Re: [PATCH 0/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[Xiao Guangrong]

On 03/08/2016 07:44 PM, Paolo Bonzini wrote:
> Patch 1 ensures that all aspects of MPX are disabled when eager FPU
> is disabled on the host.  Patch 2 is just a cleanup.

It looks good to me.

Reviewed-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>

Now, more and more features depend on eger xsave, e.g, fpu, mpx and
protection-key, maybe it is the time to rename eager-fpu to eager-xsave?

Re: [PATCH 0/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[Paolo Bonzini]

On 10/03/2016 13:28, Xiao Guangrong wrote:
> 
>> Patch 1 ensures that all aspects of MPX are disabled when eager FPU
>> is disabled on the host.  Patch 2 is just a cleanup.
> 
> It looks good to me.
> 
> Reviewed-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>

Thanks very much!

> Now, more and more features depend on eger xsave, e.g, fpu, mpx and
> protection-key, maybe it is the time to rename eager-fpu to eager-xsave?

Yeah, that could be possible.  You can propose it to Ingo Molnar (CCed).
 If you change it in the general purpose kernel code, KVM will of course
follow suit.

Paolo

Re: [PATCH 0/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[Ingo Molnar]

* Paolo Bonzini <pbonzini@redhat.com> wrote:

> > Now, more and more features depend on eger xsave, e.g, fpu, mpx and 
> > protection-key, maybe it is the time to rename eager-fpu to eager-xsave?
> 
> Yeah, that could be possible.  You can propose it to Ingo Molnar (CCed).
>  If you change it in the general purpose kernel code, KVM will of course follow 
> suit.

So we have this queued up for v4.6:

  58122bf1d856 x86/fpu: Default eagerfpu=on on all CPUs

and if all goes fine with that then the plan for v4.7 is to remove the lazy FPU 
restore code altogether.

Thanks,

	Ingo

Re: [PATCH 0/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[Paolo Bonzini]

On 10/03/2016 18:35, Ingo Molnar wrote:
> So we have this queued up for v4.6:
> 
>   58122bf1d856 x86/fpu: Default eagerfpu=on on all CPUs
> 
> and if all goes fine with that then the plan for v4.7 is to remove the lazy FPU 
> restore code altogether.

Whoa.  Unexpected, but I cannot say I'm not happy. :)

Paolo

Re: [PATCH 1/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[Paolo Bonzini]

On 11/03/2016 03:37, Yang Zhang wrote:
>> @@ -97,7 +104,7 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>   	if (best && (best->eax & (F(XSAVES) | F(XSAVEC))))
>>   		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>>   
>> -	vcpu->arch.eager_fpu = use_eager_fpu() || guest_cpuid_has_mpx(vcpu);
>> +	vcpu->arch.eager_fpu = use_eager_fpu();
> 
> Hi Paolo,
> 
> As i mentioned on another thread, force KVM to use eager fpu
> unconditionally may introduce the performance regression. Though the
> cost for eager fpu is very small especially in modern CPU, it still
> cannot be ignored on old platform.

This patch doesn't change anything in that respect.  It doesn't enable
eager FPU in any case where it wasn't already enabled before the patch.

All this patch does is hide MPX completely to the guests (just like it's
hidden on the host) if the host is using lazy FPU.

> And we have observed some performance
> decrease on those platforms according the result from some experiments
> which did several years ago.

Indeed after the merge window I plan to benchmark KVM on old systems
(pre-XSAVE) to see if there is a negative benefit from eager FPU.

Thanks,

Paolo

Re: [PATCH 1/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[Yang Zhang]

On 2016/3/11 20:09, Paolo Bonzini wrote:

Sorry for the late reply.

>
>
> On 11/03/2016 03:37, Yang Zhang wrote:
>>> @@ -97,7 +104,7 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>>    	if (best && (best->eax & (F(XSAVES) | F(XSAVEC))))
>>>    		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>>>
>>> -	vcpu->arch.eager_fpu = use_eager_fpu() || guest_cpuid_has_mpx(vcpu);
>>> +	vcpu->arch.eager_fpu = use_eager_fpu();
>>
>> Hi Paolo,
>>
>> As i mentioned on another thread, force KVM to use eager fpu
>> unconditionally may introduce the performance regression. Though the
>> cost for eager fpu is very small especially in modern CPU, it still
>> cannot be ignored on old platform.
>
> This patch doesn't change anything in that respect.  It doesn't enable
> eager FPU in any case where it wasn't already enabled before the patch.

I mean why not keep the old way that only activate the eager_fpu while 
guest sees the MPX bit in CPUID, like:

vcpu->arch.eager_fpu = use_eager_fpu() && guest_cpuid_has_mpx(vcpu);

Besides, vmx_fpu_activate is called when do vcpu_reset. So it seems no 
need to call fpu_activate() here again.


-- 
best regards
yang

Re: [PATCH 1/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[Paolo Bonzini]

On 24/03/2016 14:06, Yang Zhang wrote:
> 
> I mean why not keep the old way that only activate the eager_fpu while
> guest sees the MPX bit in CPUID, like:
> 
> vcpu->arch.eager_fpu = use_eager_fpu() && guest_cpuid_has_mpx(vcpu);

If the host uses eager FPU you can assume that it's faster than lazy FPU.

Paolo

> Besides, vmx_fpu_activate is called when do vcpu_reset. So it seems no
> need to call fpu_activate() here again.

Re: [PATCH 1/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[Yang Zhang]

On 2016/3/24 21:07, Paolo Bonzini wrote:
>
>
> On 24/03/2016 14:06, Yang Zhang wrote:
>>
>> I mean why not keep the old way that only activate the eager_fpu while
>> guest sees the MPX bit in CPUID, like:
>>
>> vcpu->arch.eager_fpu = use_eager_fpu() && guest_cpuid_has_mpx(vcpu);
>
> If the host uses eager FPU you can assume that it's faster than lazy FPU.
>
> Paolo
>
>> Besides, vmx_fpu_activate is called when do vcpu_reset. So it seems no
>> need to call fpu_activate() here again.

Hi Paolo

The check in kvm_update_cpuid of fpu is strange since nothing relies on 
cpuid now. How about the following change?

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index bbbaa80..da4e6ad 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -114,9 +114,6 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
         if (best && (best->eax & (F(XSAVES) | F(XSAVEC))))
                 best->ebx = xstate_required_size(vcpu->arch.xcr0, true);

-       if (use_eager_fpu())
-               kvm_x86_ops->fpu_activate(vcpu);
-
         /*
          * The existing code assumes virtual address is 48-bit in the 
canonical
          * address checks; exit if it is ever changed.
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 9b7798c..8f57335 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7453,6 +7453,9 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool 
init_event)
         vcpu->arch.regs_avail = ~0;
         vcpu->arch.regs_dirty = ~0;

+       if (use_eager_fpu())
+               kvm_x86_ops->fpu_activate(vcpu);
+
         kvm_x86_ops->vcpu_reset(vcpu, init_event);
  }


-- 
best regards
yang

Re: [PATCH 1/2] KVM: x86: disable MPX if host did not enable MPX XSAVE features

[Paolo Bonzini]

On 13/04/2016 11:47, Yang Zhang wrote:
> On 2016/3/24 21:07, Paolo Bonzini wrote:
>>
>>
>> On 24/03/2016 14:06, Yang Zhang wrote:
>>>
>>> I mean why not keep the old way that only activate the eager_fpu while
>>> guest sees the MPX bit in CPUID, like:
>>>
>>> vcpu->arch.eager_fpu = use_eager_fpu() && guest_cpuid_has_mpx(vcpu);
>>
>> If the host uses eager FPU you can assume that it's faster than lazy FPU.
>>
>> Paolo
>>
>>> Besides, vmx_fpu_activate is called when do vcpu_reset. So it seems no
>>> need to call fpu_activate() here again.
> 
> Hi Paolo
> 
> The check in kvm_update_cpuid of fpu is strange since nothing relies on
> cpuid now. How about the following change?

Ok, please post it generated by git-format-patch and with a commit message.

Thanks,

Paolo

> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index bbbaa80..da4e6ad 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -114,9 +114,6 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>         if (best && (best->eax & (F(XSAVES) | F(XSAVEC))))
>                 best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
> 
> -       if (use_eager_fpu())
> -               kvm_x86_ops->fpu_activate(vcpu);
> -
>         /*
>          * The existing code assumes virtual address is 48-bit in the
> canonical
>          * address checks; exit if it is ever changed.
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9b7798c..8f57335 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7453,6 +7453,9 @@ void kvm_vcpu_reset(struct kvm_vcpu *vcpu, bool
> init_event)
>         vcpu->arch.regs_avail = ~0;
>         vcpu->arch.regs_dirty = ~0;
> 
> +       if (use_eager_fpu())
> +               kvm_x86_ops->fpu_activate(vcpu);
> +
>         kvm_x86_ops->vcpu_reset(vcpu, init_event);
>  }
> 
>