Patchwork [v2,3/4] kvm/ppc: Call trace_hardirqs_on before entry

login
register
mail settings
Submitter Scott Wood
Date May 10, 2013, 3:09 a.m.
Message ID <1368155384-11035-4-git-send-email-scottwood@freescale.com>
Download mbox | patch
Permalink /patch/242895/
State Superseded
Headers show

Comments

Scott Wood - May 10, 2013, 3:09 a.m.
Currently this is only being done on 64-bit.  Rather than just move it
out of the 64-bit ifdef, move it to kvm_lazy_ee_enable() so that it is
consistent with lazy ee state, and so that we don't track more host
code as interrupts-enabled than necessary.

Rename kvm_lazy_ee_enable() to kvm_fix_ee_before_entry() to reflect
that this function now has a role on 32-bit as well.

Signed-off-by: Scott Wood <scottwood@freescale.com>
---
 arch/powerpc/include/asm/kvm_ppc.h |   11 ++++++++---
 arch/powerpc/kvm/book3s_pr.c       |    4 ++--
 arch/powerpc/kvm/booke.c           |    4 ++--
 arch/powerpc/kvm/powerpc.c         |    2 --
 4 files changed, 12 insertions(+), 9 deletions(-)
Bharat Bhushan - May 10, 2013, 3:34 a.m.
> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of
> Scott Wood
> Sent: Friday, May 10, 2013 8:40 AM
> To: Alexander Graf; Benjamin Herrenschmidt
> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> Wood Scott-B07421
> Subject: [PATCH v2 3/4] kvm/ppc: Call trace_hardirqs_on before entry
> 
> Currently this is only being done on 64-bit.  Rather than just move it
> out of the 64-bit ifdef, move it to kvm_lazy_ee_enable() so that it is
> consistent with lazy ee state, and so that we don't track more host
> code as interrupts-enabled than necessary.
> 
> Rename kvm_lazy_ee_enable() to kvm_fix_ee_before_entry() to reflect
> that this function now has a role on 32-bit as well.
> 
> Signed-off-by: Scott Wood <scottwood@freescale.com>
> ---
>  arch/powerpc/include/asm/kvm_ppc.h |   11 ++++++++---
>  arch/powerpc/kvm/book3s_pr.c       |    4 ++--
>  arch/powerpc/kvm/booke.c           |    4 ++--
>  arch/powerpc/kvm/powerpc.c         |    2 --
>  4 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h
> b/arch/powerpc/include/asm/kvm_ppc.h
> index a5287fe..6885846 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -394,10 +394,15 @@ static inline void kvmppc_mmu_flush_icache(pfn_t pfn)
>  	}
>  }
> 
> -/* Please call after prepare_to_enter. This function puts the lazy ee state
> -   back to normal mode, without actually enabling interrupts. */
> -static inline void kvmppc_lazy_ee_enable(void)
> +/*
> + * Please call after prepare_to_enter. This function puts the lazy ee and irq
> + * disabled tracking state back to normal mode, without actually enabling
> + * interrupts.
> + */
> +static inline void kvmppc_fix_ee_before_entry(void)
>  {
> +	trace_hardirqs_on();
> +
>  #ifdef CONFIG_PPC64
>  	/* Only need to enable IRQs by hard enabling them after this */
>  	local_paca->irq_happened = 0;
> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
> index bdc40b8..0b97ce4 100644
> --- a/arch/powerpc/kvm/book3s_pr.c
> +++ b/arch/powerpc/kvm/book3s_pr.c
> @@ -890,7 +890,7 @@ program_interrupt:
>  			local_irq_enable();
>  			r = s;
>  		} else {
> -			kvmppc_lazy_ee_enable();
> +			kvmppc_fix_ee_before_entry();
>  		}
>  	}
> 
> @@ -1161,7 +1161,7 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
> kvm_vcpu *vcpu)
>  	if (vcpu->arch.shared->msr & MSR_FP)
>  		kvmppc_handle_ext(vcpu, BOOK3S_INTERRUPT_FP_UNAVAIL, MSR_FP);
> 
> -	kvmppc_lazy_ee_enable();
> +	kvmppc_fix_ee_before_entry();
> 
>  	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
> 
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 705fc5c..eb89b83 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -673,7 +673,7 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu
> *vcpu)
>  		ret = s;
>  		goto out;
>  	}
> -	kvmppc_lazy_ee_enable();
> +	kvmppc_fix_ee_before_entry();

local_irq_disable() is called before kvmppc_prepare_to_enter().
Now we put the irq_happend and soft_enabled back to previous state without checking for any interrupt happened in between. If any interrupt happens in between, will not that be lost?

-Bharat

> 
>  	kvm_guest_enter();
> 
> @@ -1154,7 +1154,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
> kvm_vcpu *vcpu,
>  			local_irq_enable();
>  			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
>  		} else {
> -			kvmppc_lazy_ee_enable();
> +			kvmppc_fix_ee_before_entry();
>  		}
>  	}
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 6316ee3..4e05f8c 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -117,8 +117,6 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>  			kvm_guest_exit();
>  			continue;
>  		}
> -
> -		trace_hardirqs_on();
>  #endif
> 
>  		kvm_guest_enter();
> --
> 1.7.10.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tiejun Chen - May 10, 2013, 4:40 a.m.
On 05/10/2013 11:34 AM, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On Behalf Of
>> Scott Wood
>> Sent: Friday, May 10, 2013 8:40 AM
>> To: Alexander Graf; Benjamin Herrenschmidt
>> Cc: kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
>> Wood Scott-B07421
>> Subject: [PATCH v2 3/4] kvm/ppc: Call trace_hardirqs_on before entry
>>
>> Currently this is only being done on 64-bit.  Rather than just move it
>> out of the 64-bit ifdef, move it to kvm_lazy_ee_enable() so that it is
>> consistent with lazy ee state, and so that we don't track more host
>> code as interrupts-enabled than necessary.
>>
>> Rename kvm_lazy_ee_enable() to kvm_fix_ee_before_entry() to reflect
>> that this function now has a role on 32-bit as well.
>>
>> Signed-off-by: Scott Wood <scottwood@freescale.com>
>> ---
>>   arch/powerpc/include/asm/kvm_ppc.h |   11 ++++++++---
>>   arch/powerpc/kvm/book3s_pr.c       |    4 ++--
>>   arch/powerpc/kvm/booke.c           |    4 ++--
>>   arch/powerpc/kvm/powerpc.c         |    2 --
>>   4 files changed, 12 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/kvm_ppc.h
>> b/arch/powerpc/include/asm/kvm_ppc.h
>> index a5287fe..6885846 100644
>> --- a/arch/powerpc/include/asm/kvm_ppc.h
>> +++ b/arch/powerpc/include/asm/kvm_ppc.h
>> @@ -394,10 +394,15 @@ static inline void kvmppc_mmu_flush_icache(pfn_t pfn)
>>   	}
>>   }
>>
>> -/* Please call after prepare_to_enter. This function puts the lazy ee state
>> -   back to normal mode, without actually enabling interrupts. */
>> -static inline void kvmppc_lazy_ee_enable(void)
>> +/*
>> + * Please call after prepare_to_enter. This function puts the lazy ee and irq
>> + * disabled tracking state back to normal mode, without actually enabling
>> + * interrupts.
>> + */
>> +static inline void kvmppc_fix_ee_before_entry(void)
>>   {
>> +	trace_hardirqs_on();
>> +
>>   #ifdef CONFIG_PPC64
>>   	/* Only need to enable IRQs by hard enabling them after this */
>>   	local_paca->irq_happened = 0;
>> diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
>> index bdc40b8..0b97ce4 100644
>> --- a/arch/powerpc/kvm/book3s_pr.c
>> +++ b/arch/powerpc/kvm/book3s_pr.c
>> @@ -890,7 +890,7 @@ program_interrupt:
>>   			local_irq_enable();
>>   			r = s;
>>   		} else {
>> -			kvmppc_lazy_ee_enable();
>> +			kvmppc_fix_ee_before_entry();
>>   		}
>>   	}
>>
>> @@ -1161,7 +1161,7 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct
>> kvm_vcpu *vcpu)
>>   	if (vcpu->arch.shared->msr & MSR_FP)
>>   		kvmppc_handle_ext(vcpu, BOOK3S_INTERRUPT_FP_UNAVAIL, MSR_FP);
>>
>> -	kvmppc_lazy_ee_enable();
>> +	kvmppc_fix_ee_before_entry();
>>
>>   	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
>>
>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>> index 705fc5c..eb89b83 100644
>> --- a/arch/powerpc/kvm/booke.c
>> +++ b/arch/powerpc/kvm/booke.c
>> @@ -673,7 +673,7 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu
>> *vcpu)
>>   		ret = s;
>>   		goto out;
>>   	}
>> -	kvmppc_lazy_ee_enable();
>> +	kvmppc_fix_ee_before_entry();
>
> local_irq_disable() is called before kvmppc_prepare_to_enter().

In patch 4, we call hard_irq_disable() once enter kvmppc_prepare_to_enter().

Tiejun

> Now we put the irq_happend and soft_enabled back to previous state without checking for any interrupt happened in between. If any interrupt happens in between, will not that be lost?
>
> -Bharat
>
>>
>>   	kvm_guest_enter();
>>
>> @@ -1154,7 +1154,7 @@ int kvmppc_handle_exit(struct kvm_run *run, struct
>> kvm_vcpu *vcpu,
>>   			local_irq_enable();
>>   			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
>>   		} else {
>> -			kvmppc_lazy_ee_enable();
>> +			kvmppc_fix_ee_before_entry();
>>   		}
>>   	}
>>
>> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> index 6316ee3..4e05f8c 100644
>> --- a/arch/powerpc/kvm/powerpc.c
>> +++ b/arch/powerpc/kvm/powerpc.c
>> @@ -117,8 +117,6 @@ int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
>>   			kvm_guest_exit();
>>   			continue;
>>   		}
>> -
>> -		trace_hardirqs_on();
>>   #endif
>>
>>   		kvm_guest_enter();
>> --
>> 1.7.10.4
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
>
Scott Wood - May 10, 2013, 10:47 p.m.
On 05/09/2013 11:40:08 PM, tiejun.chen wrote:
> On 05/10/2013 11:34 AM, Bhushan Bharat-R65777 wrote:
>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>> index 705fc5c..eb89b83 100644
>>> --- a/arch/powerpc/kvm/booke.c
>>> +++ b/arch/powerpc/kvm/booke.c
>>> @@ -673,7 +673,7 @@ int kvmppc_vcpu_run(struct kvm_run *kvm_run,  
>>> struct kvm_vcpu
>>> *vcpu)
>>>   		ret = s;
>>>   		goto out;
>>>   	}
>>> -	kvmppc_lazy_ee_enable();
>>> +	kvmppc_fix_ee_before_entry();
>> 
>> local_irq_disable() is called before kvmppc_prepare_to_enter().
> 
> In patch 4, we call hard_irq_disable() once enter  
> kvmppc_prepare_to_enter().

And before patch 4, we have the code near the end of  
kvmppc_prepare_to_enter() that checks lazy_irq_pending() and aborts  
guest entry if there was a race.  If I'd known about that bit of code  
beforehand, I probably wouldn't have bothered with most of patch 4/4,  
but now that it's been done it seems cleaner.

-Scott

Patch

diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index a5287fe..6885846 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -394,10 +394,15 @@  static inline void kvmppc_mmu_flush_icache(pfn_t pfn)
 	}
 }
 
-/* Please call after prepare_to_enter. This function puts the lazy ee state
-   back to normal mode, without actually enabling interrupts. */
-static inline void kvmppc_lazy_ee_enable(void)
+/*
+ * Please call after prepare_to_enter. This function puts the lazy ee and irq
+ * disabled tracking state back to normal mode, without actually enabling
+ * interrupts.
+ */
+static inline void kvmppc_fix_ee_before_entry(void)
 {
+	trace_hardirqs_on();
+
 #ifdef CONFIG_PPC64
 	/* Only need to enable IRQs by hard enabling them after this */
 	local_paca->irq_happened = 0;
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index bdc40b8..0b97ce4 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -890,7 +890,7 @@  program_interrupt:
 			local_irq_enable();
 			r = s;
 		} else {
-			kvmppc_lazy_ee_enable();
+			kvmppc_fix_ee_before_entry();
 		}
 	}
 
@@ -1161,7 +1161,7 @@  int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	if (vcpu->arch.shared->msr & MSR_FP)
 		kvmppc_handle_ext(vcpu, BOOK3S_INTERRUPT_FP_UNAVAIL, MSR_FP);
 
-	kvmppc_lazy_ee_enable();
+	kvmppc_fix_ee_before_entry();
 
 	ret = __kvmppc_vcpu_run(kvm_run, vcpu);
 
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 705fc5c..eb89b83 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -673,7 +673,7 @@  int kvmppc_vcpu_run(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 		ret = s;
 		goto out;
 	}
-	kvmppc_lazy_ee_enable();
+	kvmppc_fix_ee_before_entry();
 
 	kvm_guest_enter();
 
@@ -1154,7 +1154,7 @@  int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 			local_irq_enable();
 			r = (s << 2) | RESUME_HOST | (r & RESUME_FLAG_NV);
 		} else {
-			kvmppc_lazy_ee_enable();
+			kvmppc_fix_ee_before_entry();
 		}
 	}
 
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 6316ee3..4e05f8c 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -117,8 +117,6 @@  int kvmppc_prepare_to_enter(struct kvm_vcpu *vcpu)
 			kvm_guest_exit();
 			continue;
 		}
-
-		trace_hardirqs_on();
 #endif
 
 		kvm_guest_enter();