[MODERATED] [PATCH v4 8/8] [PATCH v4 8/8] Linux Patch #8

[MODERATED] [PATCH v4 8/8] [PATCH v4 8/8] Linux Patch #8

[konrad.wilk]

If the module parameter is to flush the L1D cache on every VMENTER
then we can optimize by using the MSR save list to have the CPU
poke the MSR with the proper value right at VMENTER boundary.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v3: Actually engage the MSR save list
    Move it to function that frobs VMCS
---
 arch/x86/kvm/vmx.c | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 9b18848ccaba..020145adc546 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2649,15 +2649,22 @@ static void vmx_save_host_state(struct kvm_vcpu *vcpu)
 				   vmx->guest_msrs[i].mask);
 }
 
-static void vmx_prepare_guest_switch(struct kvm_vcpu *vcpu)
+static bool vmx_l1d_cache_flush_req(struct kvm_vcpu *vcpu)
 {
-	vmx_save_host_state(vcpu);
-
 	if (!enable_ept || static_cpu_has(X86_FEATURE_HYPERVISOR) ||
 	    !static_cpu_has(X86_BUG_L1TF)) {
 		vcpu->arch.flush_cache_req = false;
-		return;
+		return false;
 	}
+	return true;
+}
+
+static void vmx_prepare_guest_switch(struct kvm_vcpu *vcpu)
+{
+	vmx_save_host_state(vcpu);
+
+	if (!vmx_l1d_cache_flush_req(vcpu))
+		return;
 
 	switch (vmentry_l1d_flush) {
 	case 0:
@@ -6352,6 +6359,15 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
 		vmcs_write64(PML_ADDRESS, page_to_phys(vmx->pml_pg));
 		vmcs_write16(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
 	}
+
+	/*
+	 * If we enforce flushing the L1D cache on every VMENTER lets use the
+	 * MSR save list.
+	 */
+	if (vmx_l1d_cache_flush_req(&vmx->vcpu))
+		if (vmentry_l1d_flush == 2)
+			add_atomic_switch_msr(vmx, MSR_IA32_FLUSH_CMD,
+					      L1D_FLUSH, 0, true);
 }
 
 static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
@@ -10079,7 +10095,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
 	evmcs_rsp = static_branch_unlikely(&enable_evmcs) ?
 		(unsigned long)&current_evmcs->host_rsp : 0;
 
-	if (vcpu->arch.flush_cache_req)
+	if (vcpu->arch.flush_cache_req && vmentry_l1d_flush != 1)
 		kvm_l1d_flush();
 
 	asm(
-- 
2.14.3

[MODERATED] Re: [PATCH v4 8/8] [PATCH v4 8/8] Linux Patch #8

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

On 23/06/2018 15:54, speck for konrad.wilk_at_oracle.com wrote:
> x86/KVM/VMX: Use MSR save list for IA32_FLUSH_CMD if required.
> 
> If the module parameter is to flush the L1D cache on every VMENTER
> then we can optimize by using the MSR save list to have the CPU
> poke the MSR with the proper value right at VMENTER boundary.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> v3: Actually engage the MSR save list
>     Move it to function that frobs VMCS
> ---
>  arch/x86/kvm/vmx.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 9b18848ccaba..020145adc546 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2649,15 +2649,22 @@ static void vmx_save_host_state(struct kvm_vcpu *vcpu)
>  				   vmx->guest_msrs[i].mask);
>  }
>  
> -static void vmx_prepare_guest_switch(struct kvm_vcpu *vcpu)
> +static bool vmx_l1d_cache_flush_req(struct kvm_vcpu *vcpu)
>  {
> -	vmx_save_host_state(vcpu);
> -
>  	if (!enable_ept || static_cpu_has(X86_FEATURE_HYPERVISOR) ||
>  	    !static_cpu_has(X86_BUG_L1TF)) {
>  		vcpu->arch.flush_cache_req = false;

This assignment is strange, the function would be pure if it was not for it.

Let's remove it, since vmx_vcpu_setup doesn't need it, and rename the
function to vmx_has_bug_l1tf.

Thanks,

Paolo

> -		return;
> +		return false;
>  	}
> +	return true;
> +}
> +
> +static void vmx_prepare_guest_switch(struct kvm_vcpu *vcpu)
> +{
> +	vmx_save_host_state(vcpu);
> +
> +	if (!vmx_l1d_cache_flush_req(vcpu))
> +		return;
>  
>  	switch (vmentry_l1d_flush) {
>  	case 0:> @@ -6352,6 +6359,15 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
>  		vmcs_write64(PML_ADDRESS, page_to_phys(vmx->pml_pg));
>  		vmcs_write16(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
>  	}
> +
> +	/*
> +	 * If we enforce flushing the L1D cache on every VMENTER lets use the
> +	 * MSR save list.
> +	 */
> +	if (vmx_l1d_cache_flush_req(&vmx->vcpu))
> +		if (vmentry_l1d_flush == 2)
> +			add_atomic_switch_msr(vmx, MSR_IA32_FLUSH_CMD,
> +					      L1D_FLUSH, 0, true);
>  }
>  
>  static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> @@ -10079,7 +10095,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
>  	evmcs_rsp = static_branch_unlikely(&enable_evmcs) ?
>  		(unsigned long)&current_evmcs->host_rsp : 0;
>  
> -	if (vcpu->arch.flush_cache_req)
> +	if (vcpu->arch.flush_cache_req && vmentry_l1d_flush != 1)
>  		kvm_l1d_flush();
>  

Re: [PATCH v4 8/8] [PATCH v4 8/8] Linux Patch #8

[Thomas Gleixner]

On Sat, 23 Jun 2018, speck for konrad.wilk_at_oracle.com wrote:
> x86/KVM/VMX: Use MSR save list for IA32_FLUSH_CMD if required.
> +
> +	/*
> +	 * If we enforce flushing the L1D cache on every VMENTER lets use the
> +	 * MSR save list.
> +	 */
> +	if (vmx_l1d_cache_flush_req(&vmx->vcpu))
> +		if (vmentry_l1d_flush == 2)
> +			add_atomic_switch_msr(vmx, MSR_IA32_FLUSH_CMD,
> +					      L1D_FLUSH, 0, true);

That's broken when the CPU does not have the FLUSH MSR, unless I'm missing
something.

>  static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> @@ -10079,7 +10095,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
>  	evmcs_rsp = static_branch_unlikely(&enable_evmcs) ?
>  		(unsigned long)&current_evmcs->host_rsp : 0;
>  
> -	if (vcpu->arch.flush_cache_req)
> +	if (vcpu->arch.flush_cache_req && vmentry_l1d_flush != 1)
>  		kvm_l1d_flush();

Huch? So above you use the MSR list for vmentry_l1d_flush == 2 and here you
do the direct flush for vmentry_l1d_flush != 1. That should be == 1, right?

Thanks,

	tglx

[MODERATED] Re: [PATCH v4 8/8] [PATCH v4 8/8] Linux Patch #8

[Konrad Rzeszutek Wilk]

On Wed, Jun 27, 2018 at 03:05:02PM +0200, speck for Thomas Gleixner wrote:
> On Sat, 23 Jun 2018, speck for konrad.wilk_at_oracle.com wrote:
> > x86/KVM/VMX: Use MSR save list for IA32_FLUSH_CMD if required.
> > +
> > +	/*
> > +	 * If we enforce flushing the L1D cache on every VMENTER lets use the
> > +	 * MSR save list.
> > +	 */
> > +	if (vmx_l1d_cache_flush_req(&vmx->vcpu))
> > +		if (vmentry_l1d_flush == 2)
> > +			add_atomic_switch_msr(vmx, MSR_IA32_FLUSH_CMD,
> > +					      L1D_FLUSH, 0, true);
> 
> That's broken when the CPU does not have the FLUSH MSR, unless I'm missing
> something.

<nods>
It should have the check for the static_cpu_has(X86_FEATURE_FLUSH_L1D).

> 
> >  static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
> > @@ -10079,7 +10095,7 @@ static void __noclone vmx_vcpu_run(struct kvm_vcpu *vcpu)
> >  	evmcs_rsp = static_branch_unlikely(&enable_evmcs) ?
> >  		(unsigned long)&current_evmcs->host_rsp : 0;
> >  
> > -	if (vcpu->arch.flush_cache_req)
> > +	if (vcpu->arch.flush_cache_req && vmentry_l1d_flush != 1)
> >  		kvm_l1d_flush();
> 
> Huch? So above you use the MSR list for vmentry_l1d_flush == 2 and here you
> do the direct flush for vmentry_l1d_flush != 1. That should be == 1, right?

Yes in fact I vividely remember fixing this up as my debug patch has this
this extra logic (so that I could turn on/off the MSR list):

    if (vcpu->arch.flush_cache_req) {
+               if ((vmentry_l1d_flush == 2) && (!use_msr_save))
+                       kvm_l1d_flush();
+               else if (vmentry_l1d_flush == 1)
+                       kvm_l1d_flush();

and yet I managed to post the stale version. Arggh.


> 
> Thanks,
> 
> 	tglx

Re: [PATCH v4 8/8] [PATCH v4 8/8] Linux Patch #8

[Thomas Gleixner]

On Wed, 27 Jun 2018, speck for Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 27, 2018 at 03:05:02PM +0200, speck for Thomas Gleixner wrote:
> > Huch? So above you use the MSR list for vmentry_l1d_flush == 2 and here you
> > do the direct flush for vmentry_l1d_flush != 1. That should be == 1, right?
> 
> Yes in fact I vividely remember fixing this up as my debug patch has this
> this extra logic (so that I could turn on/off the MSR list):
> 
>     if (vcpu->arch.flush_cache_req) {
> +               if ((vmentry_l1d_flush == 2) && (!use_msr_save))
> +                       kvm_l1d_flush();
> +               else if (vmentry_l1d_flush == 1)
> +                       kvm_l1d_flush();
> 
> and yet I managed to post the stale version. Arggh.

Duh, that's pretty horrible. I had a stab on it. Compiles, but
untested. You get the idea.

If that works, parts of that should be folded back of course (enum, static
ket, etc,)

Thanks,

	tglx

8<--------------------
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -194,6 +194,14 @@ module_param(ple_window_max, uint, 0444)
 
 extern const ulong vmx_return;
 
+static DEFINE_STATIC_KEY_FALSE(vmx_l1d_should_flush);
+
+enum vmx_l1d_flush_state {
+	VMENTER_L1D_FLUSH_NEVER,
+	VMENTER_L1D_FLUSH_COND,
+	VMENTER_L1D_FLUSH_ALWAYS,
+};
+
 struct kvm_vmx {
 	struct kvm kvm;
 
@@ -2653,38 +2661,10 @@ static void vmx_prepare_guest_switch(str
 {
 	vmx_save_host_state(vcpu);
 
-	if (!enable_ept || static_cpu_has(X86_FEATURE_HYPERVISOR) ||
-	    !static_cpu_has(X86_BUG_L1TF)) {
-		vcpu->arch.flush_cache_req = false;
-		return;
-	}
+	if (static_branch_unlikely(&vmx_l1d_should_flush)) {
+		bool force = vmentry_l1d_flush == VMENTER_L1D_FLUSH_ALWAYS;
 
-	switch (vmentry_l1d_flush) {
-	case 0:
-		vcpu->arch.flush_cache_req = false;
-		break;
-	case 1:
-		/*
-		 * If vmentry_l1d_flush is 1, each vmexit handler is responsible for
-		 * setting vcpu->arch.vcpu_unconfined.  Currently this happens in the
-		 * following cases:
-		 * - vmlaunch/vmresume: we do not want the cache to be cleared by a
-		 *   nested hypervisor *and* by KVM on bare metal, so we just do it
-		 *   on every nested entry.  Nested hypervisors do not bother clearing
-		 *   the cache.
-		 * - anything that runs the emulator (the slow paths for EPT misconfig
-		 *   or I/O instruction)
-		 * - anything that can cause get_user_pages (EPT violation, and again
-		 *   the slow paths for EPT misconfig or I/O instruction)
-		 * - anything that can run code outside KVM (external interrupt,
-		 *   which can run interrupt handlers or irqs; or the sched_in
-		 *   preempt notifier)
-		 */
-		break;
-	case 2:
-	default:
-		vcpu->arch.flush_cache_req = true;
-		break;
+		vcpu->arch.flush_cache_req = force;
 	}
 }
 
@@ -6231,6 +6211,16 @@ static void ept_set_mmio_spte_mask(void)
 				   VMX_EPT_MISCONFIG_WX_VALUE);
 }
 
+static bool vmx_l1d_use_msr_save_list(void)
+{
+	if (!enable_ept || !boot_cpu_has_bug(X86_BUG_L1TF) ||
+	    static_cpu_has(X86_FEATURE_HYPERVISOR) ||
+	    !static_cpu_has(X86_FEATURE_FLUSH_L1D))
+		return false;
+
+	return vmentry_l1d_flush == VMENTER_L1D_FLUSH_ALWAYS;
+}
+
 #define VMX_XSS_EXIT_BITMAP 0
 /*
  * Sets up the vmcs for emulated real mode.
@@ -6352,6 +6342,13 @@ static void vmx_vcpu_setup(struct vcpu_v
 		vmcs_write64(PML_ADDRESS, page_to_phys(vmx->pml_pg));
 		vmcs_write16(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
 	}
+
+	/*
+	 * If flushing the L1D cache on every VMENTER is enforced and the
+	 * MSR is available, use the MSR save list.
+	 */
+	if (vmx_l1d_use_msr_save_list())
+		add_atomic_switch_msr(vmx, MSR_IA32_FLUSH_CMD, L1D_FLUSH, 0, true);
 }
 
 static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
@@ -10079,8 +10076,10 @@ static void __noclone vmx_vcpu_run(struc
 	evmcs_rsp = static_branch_unlikely(&enable_evmcs) ?
 		(unsigned long)&current_evmcs->host_rsp : 0;
 
-	if (vcpu->arch.flush_cache_req)
-		kvm_l1d_flush();
+	if (static_branch_unlikely(&vmx_l1d_should_flush)) {
+		if (vcpu->arch.flush_cache_req)
+			kvm_l1d_flush();
+	}
 
 	asm(
 		/* Store host registers */
@@ -13136,6 +13135,15 @@ static struct kvm_x86_ops vmx_x86_ops __
 	.enable_smi_window = enable_smi_window,
 };
 
+static void __init vmx_setup_l1d_flush(void)
+{
+	if (vmentry_l1d_flush == VMENTER_L1D_FLUSH_NEVER ||
+	    !boot_cpu_has_bug(X86_BUG_L1TF) ||
+	    vmx_l1d_use_msr_save_list())
+		return;
+	static_branch_enable(&vmx_l1d_should_flush);
+}
+
 static int __init vmx_init(void)
 {
 	int r;
@@ -13169,6 +13177,8 @@ static int __init vmx_init(void)
 	}
 #endif
 
+	vmx_setup_l1d_flush();
+
 	r = kvm_init(&vmx_x86_ops, sizeof(struct vcpu_vmx),
                      __alignof__(struct vcpu_vmx), THIS_MODULE);
 	if (r)

[MODERATED] Re: [PATCH v4 8/8] [PATCH v4 8/8] Linux Patch #8

[Konrad Rzeszutek Wilk]

> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -194,6 +194,14 @@ module_param(ple_window_max, uint, 0444)
>  
>  extern const ulong vmx_return;
>  
> +static DEFINE_STATIC_KEY_FALSE(vmx_l1d_should_flush);
> +
> +enum vmx_l1d_flush_state {
> +	VMENTER_L1D_FLUSH_NEVER,
> +	VMENTER_L1D_FLUSH_COND,
> +	VMENTER_L1D_FLUSH_ALWAYS,
> +};
> +
>  struct kvm_vmx {
>  	struct kvm kvm;
>  
> @@ -2653,38 +2661,10 @@ static void vmx_prepare_guest_switch(str
>  {
>  	vmx_save_host_state(vcpu);
>  
> -	if (!enable_ept || static_cpu_has(X86_FEATURE_HYPERVISOR) ||
> -	    !static_cpu_has(X86_BUG_L1TF)) {
> -		vcpu->arch.flush_cache_req = false;
> -		return;
> -	}
> +	if (static_branch_unlikely(&vmx_l1d_should_flush)) {
> +		bool force = vmentry_l1d_flush == VMENTER_L1D_FLUSH_ALWAYS;
>  
> -	switch (vmentry_l1d_flush) {
> -	case 0:
> -		vcpu->arch.flush_cache_req = false;
> -		break;
> -	case 1:
> -		/*
> -		 * If vmentry_l1d_flush is 1, each vmexit handler is responsible for
> -		 * setting vcpu->arch.vcpu_unconfined.  Currently this happens in the
> -		 * following cases:
> -		 * - vmlaunch/vmresume: we do not want the cache to be cleared by a
> -		 *   nested hypervisor *and* by KVM on bare metal, so we just do it
> -		 *   on every nested entry.  Nested hypervisors do not bother clearing
> -		 *   the cache.
> -		 * - anything that runs the emulator (the slow paths for EPT misconfig
> -		 *   or I/O instruction)
> -		 * - anything that can cause get_user_pages (EPT violation, and again
> -		 *   the slow paths for EPT misconfig or I/O instruction)
> -		 * - anything that can run code outside KVM (external interrupt,
> -		 *   which can run interrupt handlers or irqs; or the sched_in
> -		 *   preempt notifier)
> -		 */
> -		break;
> -	case 2:
> -	default:
> -		vcpu->arch.flush_cache_req = true;
> -		break;
> +		vcpu->arch.flush_cache_req = force;

This ought to be:

		if (force)
			vcpu->arch.flush_cache_req = force;

or perhaps:

		if (vmentry_l1d_flush == VMENTER_L1D_FLUSH_ALWAYS)
			vcpu->arch.flush_cache_req = true;

The problem is that if we are in 'vmentry_l1d_flush=1' we should
not modify vcpu->arch.flush_cache_req as the vcpu_enter_guest does:

7464                                                                                 
7465         vcpu->arch.flush_cache_req = vcpu->arch.vcpu_unconfined;                 <===
7466         kvm_x86_ops->prepare_guest_switch(vcpu);                                
7467         vcpu->arch.vcpu_unconfined = false;                                     
7468         if (vcpu->arch.flush_cache_req)                                         
7469                 vcpu->stat.l1d_flush++;      

Rethinking this, maybe rip the above from vcpu_enter_guest and in this function
will do:

		vcpu->arch.flush_cache_req = (vmentry_l1d_flush == VMENTER_L1D_FLUSH_ALWAYS) ? true : vcpu->arch.vcpu_unconfined;

or so?

Let me try that out.