Patchwork [v5] KVM: PPC: Book3E HV: call RECONCILE_IRQ_STATE to sync the software state

login
register
mail settings
Submitter Tiejun Chen
Date Oct. 23, 2013, 1:26 a.m.
Message ID <1382491608-4535-1-git-send-email-tiejun.chen@windriver.com>
Download mbox | patch
Permalink /patch/285507/
State New
Headers show

Comments

Tiejun Chen - Oct. 23, 2013, 1:26 a.m.
We enter with interrupts disabled in hardware, but we need to
call RECONCILE_IRQ_STATE anyway to ensure that the software state
is kept in sync instead of calling hard_irq_disable() directly.

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
v5:

Fix one typo in the comment.

v4:

Fix one typo in the patch description.

v3:

Base on the latest tree, now we can use RECONCILE_IRQ_STATE instead of SOFT_DISABLE_INTS.

v2:

Move SOFT_DISABLE_INTS[1] earlier to avoid clobbering the arguments we want to pass to kvmppc_handle_exit. 

 arch/powerpc/kvm/booke.c              |   11 -----------
 arch/powerpc/kvm/bookehv_interrupts.S |   11 +++++++++++
 2 files changed, 11 insertions(+), 11 deletions(-)
Tiejun Chen - Nov. 18, 2013, 8:34 a.m.
On 10/23/2013 09:26 AM, Tiejun Chen wrote:
> We enter with interrupts disabled in hardware, but we need to
> call RECONCILE_IRQ_STATE anyway to ensure that the software state
> is kept in sync instead of calling hard_irq_disable() directly.
>
> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
> v5:
>
> Fix one typo in the comment.

Alex,

I already addressed Scott's comment, any further feedback?

Tiejun

>
> v4:
>
> Fix one typo in the patch description.
>
> v3:
>
> Base on the latest tree, now we can use RECONCILE_IRQ_STATE instead of SOFT_DISABLE_INTS.
>
> v2:
>
> Move SOFT_DISABLE_INTS[1] earlier to avoid clobbering the arguments we want to pass to kvmppc_handle_exit.
>
>   arch/powerpc/kvm/booke.c              |   11 -----------
>   arch/powerpc/kvm/bookehv_interrupts.S |   11 +++++++++++
>   2 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index 15d0149..0d211ff 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -899,17 +899,6 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>   	int s;
>   	int idx;
>
> -#ifdef CONFIG_PPC64
> -	WARN_ON(local_paca->irq_happened != 0);
> -#endif
> -
> -	/*
> -	 * We enter with interrupts disabled in hardware, but
> -	 * we need to call hard_irq_disable anyway to ensure that
> -	 * the software state is kept in sync.
> -	 */
> -	hard_irq_disable();
> -
>   	/* update before a new last_exit_type is rewritten */
>   	kvmppc_update_timing_stats(vcpu);
>
> diff --git a/arch/powerpc/kvm/bookehv_interrupts.S b/arch/powerpc/kvm/bookehv_interrupts.S
> index e8ed7d6..191c32b 100644
> --- a/arch/powerpc/kvm/bookehv_interrupts.S
> +++ b/arch/powerpc/kvm/bookehv_interrupts.S
> @@ -33,6 +33,8 @@
>
>   #ifdef CONFIG_64BIT
>   #include <asm/exception-64e.h>
> +#include <asm/hw_irq.h>
> +#include <asm/irqflags.h>
>   #else
>   #include "../kernel/head_booke.h" /* for THREAD_NORMSAVE() */
>   #endif
> @@ -465,6 +467,15 @@ _GLOBAL(kvmppc_resume_host)
>   	mtspr	SPRN_EPCR, r3
>   	isync
>
> +#ifdef CONFIG_64BIT
> +	/*
> +	 * We enter with interrupts disabled in hardware, but
> +	 * we need to call RECONCILE_IRQ_STATE anyway to ensure
> +	 * that the software state is kept in sync.
> +	 */
> +	RECONCILE_IRQ_STATE(r3,r5)
> +#endif
> +
>   	/* Switch to kernel stack and jump to handler. */
>   	PPC_LL	r3, HOST_RUN(r1)
>   	mr	r5, r14 /* intno */
>

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - Nov. 18, 2013, 9:09 p.m.
On 18.11.2013, at 03:34, “tiejun.chen” <tiejun.chen@windriver.com> wrote:

> On 10/23/2013 09:26 AM, Tiejun Chen wrote:
>> We enter with interrupts disabled in hardware, but we need to
>> call RECONCILE_IRQ_STATE anyway to ensure that the software state
>> is kept in sync instead of calling hard_irq_disable() directly.

Why didn't this happen before? What is this patch fixing?

>> 
>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>> ---
>> v5:
>> 
>> Fix one typo in the comment.
> 
> Alex,
> 
> I already addressed Scott's comment, any further feedback?

Apart from the fact that we don't build PR for 64bit and only 64bit does do lazy EE, is there any reason this code moves from booke.c into HV only paths?


Oh, and sorry for the late reply :).

Alex

> 
> Tiejun
> 
>> 
>> v4:
>> 
>> Fix one typo in the patch description.
>> 
>> v3:
>> 
>> Base on the latest tree, now we can use RECONCILE_IRQ_STATE instead of SOFT_DISABLE_INTS.
>> 
>> v2:
>> 
>> Move SOFT_DISABLE_INTS[1] earlier to avoid clobbering the arguments we want to pass to kvmppc_handle_exit.
>> 
>>  arch/powerpc/kvm/booke.c              |   11 -----------
>>  arch/powerpc/kvm/bookehv_interrupts.S |   11 +++++++++++
>>  2 files changed, 11 insertions(+), 11 deletions(-)
>> 
>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>> index 15d0149..0d211ff 100644
>> --- a/arch/powerpc/kvm/booke.c
>> +++ b/arch/powerpc/kvm/booke.c
>> @@ -899,17 +899,6 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>  	int s;
>>  	int idx;
>> 
>> -#ifdef CONFIG_PPC64
>> -	WARN_ON(local_paca->irq_happened != 0);
>> -#endif
>> -
>> -	/*
>> -	 * We enter with interrupts disabled in hardware, but
>> -	 * we need to call hard_irq_disable anyway to ensure that
>> -	 * the software state is kept in sync.
>> -	 */
>> -	hard_irq_disable();
>> -
>>  	/* update before a new last_exit_type is rewritten */
>>  	kvmppc_update_timing_stats(vcpu);
>> 
>> diff --git a/arch/powerpc/kvm/bookehv_interrupts.S b/arch/powerpc/kvm/bookehv_interrupts.S
>> index e8ed7d6..191c32b 100644
>> --- a/arch/powerpc/kvm/bookehv_interrupts.S
>> +++ b/arch/powerpc/kvm/bookehv_interrupts.S
>> @@ -33,6 +33,8 @@
>> 
>>  #ifdef CONFIG_64BIT
>>  #include <asm/exception-64e.h>
>> +#include <asm/hw_irq.h>
>> +#include <asm/irqflags.h>
>>  #else
>>  #include "../kernel/head_booke.h" /* for THREAD_NORMSAVE() */
>>  #endif
>> @@ -465,6 +467,15 @@ _GLOBAL(kvmppc_resume_host)
>>  	mtspr	SPRN_EPCR, r3
>>  	isync
>> 
>> +#ifdef CONFIG_64BIT
>> +	/*
>> +	 * We enter with interrupts disabled in hardware, but
>> +	 * we need to call RECONCILE_IRQ_STATE anyway to ensure
>> +	 * that the software state is kept in sync.
>> +	 */
>> +	RECONCILE_IRQ_STATE(r3,r5)
>> +#endif
>> +
>>  	/* Switch to kernel stack and jump to handler. */
>>  	PPC_LL	r3, HOST_RUN(r1)
>>  	mr	r5, r14 /* intno */
>> 
> 

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - Nov. 18, 2013, 11:49 p.m.
On Mon, 2013-11-18 at 16:09 -0500, Alexander Graf wrote:
> On 18.11.2013, at 03:34, “tiejun.chen” <tiejun.chen@windriver.com> wrote:
> 
> > On 10/23/2013 09:26 AM, Tiejun Chen wrote:
> >> We enter with interrupts disabled in hardware, but we need to
> >> call RECONCILE_IRQ_STATE anyway to ensure that the software state
> >> is kept in sync instead of calling hard_irq_disable() directly.
> 
> Why didn't this happen before? What is this patch fixing?

It's cleanup, not a fix.  It makes things more consistent with other
64-bit kernel entry code.

-Scott



--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - Nov. 19, 2013, 9:51 a.m.
Am 19.11.2013 um 00:49 schrieb Scott Wood <scottwood@freescale.com>:

> On Mon, 2013-11-18 at 16:09 -0500, Alexander Graf wrote:
>> On 18.11.2013, at 03:34, “tiejun.chen” <tiejun.chen@windriver.com> wrote:
>> 
>>> On 10/23/2013 09:26 AM, Tiejun Chen wrote:
>>>> We enter with interrupts disabled in hardware, but we need to
>>>> call RECONCILE_IRQ_STATE anyway to ensure that the software state
>>>> is kept in sync instead of calling hard_irq_disable() directly.
>> 
>> Why didn't this happen before? What is this patch fixing?
> 
> It's cleanup, not a fix.  It makes things more consistent with other
> 64-bit kernel entry code.

Could we please note this in the comit message so that whoever stumbles over the patch later knows that this is effectively a no-op (and just prepones the lazy sync)?

Also, I'm still wary of lazy breakage in the pr code path, but I guess since it's completely untested today already it's ok to ignore.


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - Nov. 19, 2013, 6:32 p.m.
On Tue, 2013-11-19 at 10:51 +0100, Alexander Graf wrote:
> 
> Am 19.11.2013 um 00:49 schrieb Scott Wood <scottwood@freescale.com>:
> 
> > On Mon, 2013-11-18 at 16:09 -0500, Alexander Graf wrote:
> >> On 18.11.2013, at 03:34, “tiejun.chen” <tiejun.chen@windriver.com> wrote:
> >> 
> >>> On 10/23/2013 09:26 AM, Tiejun Chen wrote:
> >>>> We enter with interrupts disabled in hardware, but we need to
> >>>> call RECONCILE_IRQ_STATE anyway to ensure that the software state
> >>>> is kept in sync instead of calling hard_irq_disable() directly.
> >> 
> >> Why didn't this happen before? What is this patch fixing?
> > 
> > It's cleanup, not a fix.  It makes things more consistent with other
> > 64-bit kernel entry code.
> 
> Could we please note this in the comit message so that whoever stumbles over the patch later knows that this is effectively a no-op (and just prepones the lazy sync)?
> 
> Also, I'm still wary of lazy breakage in the pr code path, but I guess since it's completely untested today already it's ok to ignore.

PR doesn't support 64-bit at all (e.g. it's all stw/lwz rather than some
word-size abstraction).  In the unlikely event that lazy EE is ever
extended to 32-bit, then fixing up booke_interrupts.S would be just one
part of the task of updating all the 32-bit asm (e.g. you'll find
RECONCILE_IRQ_STATE in entry_64.S but not entry_32.S).

-Scott



--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tiejun Chen - Nov. 29, 2013, 2:01 a.m.
Alex,

I suppose Scott already elaborate anything you want to know.

Tiejun

On 11/19/2013 05:09 AM, Alexander Graf wrote:
>
> On 18.11.2013, at 03:34, �tiejun.chen� <tiejun.chen@windriver.com> wrote:
>
>> On 10/23/2013 09:26 AM, Tiejun Chen wrote:
>>> We enter with interrupts disabled in hardware, but we need to
>>> call RECONCILE_IRQ_STATE anyway to ensure that the software state
>>> is kept in sync instead of calling hard_irq_disable() directly.
>
> Why didn't this happen before? What is this patch fixing?
>
>>>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>>> ---
>>> v5:
>>>
>>> Fix one typo in the comment.
>>
>> Alex,
>>
>> I already addressed Scott's comment, any further feedback?
>
> Apart from the fact that we don't build PR for 64bit and only 64bit does do lazy EE, is there any reason this code moves from booke.c into HV only paths?
>
>
> Oh, and sorry for the late reply :).
>
> Alex
>
>>
>> Tiejun
>>
>>>
>>> v4:
>>>
>>> Fix one typo in the patch description.
>>>
>>> v3:
>>>
>>> Base on the latest tree, now we can use RECONCILE_IRQ_STATE instead of SOFT_DISABLE_INTS.
>>>
>>> v2:
>>>
>>> Move SOFT_DISABLE_INTS[1] earlier to avoid clobbering the arguments we want to pass to kvmppc_handle_exit.
>>>
>>>   arch/powerpc/kvm/booke.c              |   11 -----------
>>>   arch/powerpc/kvm/bookehv_interrupts.S |   11 +++++++++++
>>>   2 files changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
>>> index 15d0149..0d211ff 100644
>>> --- a/arch/powerpc/kvm/booke.c
>>> +++ b/arch/powerpc/kvm/booke.c
>>> @@ -899,17 +899,6 @@ int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
>>>   	int s;
>>>   	int idx;
>>>
>>> -#ifdef CONFIG_PPC64
>>> -	WARN_ON(local_paca->irq_happened != 0);
>>> -#endif
>>> -
>>> -	/*
>>> -	 * We enter with interrupts disabled in hardware, but
>>> -	 * we need to call hard_irq_disable anyway to ensure that
>>> -	 * the software state is kept in sync.
>>> -	 */
>>> -	hard_irq_disable();
>>> -
>>>   	/* update before a new last_exit_type is rewritten */
>>>   	kvmppc_update_timing_stats(vcpu);
>>>
>>> diff --git a/arch/powerpc/kvm/bookehv_interrupts.S b/arch/powerpc/kvm/bookehv_interrupts.S
>>> index e8ed7d6..191c32b 100644
>>> --- a/arch/powerpc/kvm/bookehv_interrupts.S
>>> +++ b/arch/powerpc/kvm/bookehv_interrupts.S
>>> @@ -33,6 +33,8 @@
>>>
>>>   #ifdef CONFIG_64BIT
>>>   #include <asm/exception-64e.h>
>>> +#include <asm/hw_irq.h>
>>> +#include <asm/irqflags.h>
>>>   #else
>>>   #include "../kernel/head_booke.h" /* for THREAD_NORMSAVE() */
>>>   #endif
>>> @@ -465,6 +467,15 @@ _GLOBAL(kvmppc_resume_host)
>>>   	mtspr	SPRN_EPCR, r3
>>>   	isync
>>>
>>> +#ifdef CONFIG_64BIT
>>> +	/*
>>> +	 * We enter with interrupts disabled in hardware, but
>>> +	 * we need to call RECONCILE_IRQ_STATE anyway to ensure
>>> +	 * that the software state is kept in sync.
>>> +	 */
>>> +	RECONCILE_IRQ_STATE(r3,r5)
>>> +#endif
>>> +
>>>   	/* Switch to kernel stack and jump to handler. */
>>>   	PPC_LL	r3, HOST_RUN(r1)
>>>   	mr	r5, r14 /* intno */
>>>
>>
>
>

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 15d0149..0d211ff 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -899,17 +899,6 @@  int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	int s;
 	int idx;
 
-#ifdef CONFIG_PPC64
-	WARN_ON(local_paca->irq_happened != 0);
-#endif
-
-	/*
-	 * We enter with interrupts disabled in hardware, but
-	 * we need to call hard_irq_disable anyway to ensure that
-	 * the software state is kept in sync.
-	 */
-	hard_irq_disable();
-
 	/* update before a new last_exit_type is rewritten */
 	kvmppc_update_timing_stats(vcpu);
 
diff --git a/arch/powerpc/kvm/bookehv_interrupts.S b/arch/powerpc/kvm/bookehv_interrupts.S
index e8ed7d6..191c32b 100644
--- a/arch/powerpc/kvm/bookehv_interrupts.S
+++ b/arch/powerpc/kvm/bookehv_interrupts.S
@@ -33,6 +33,8 @@ 
 
 #ifdef CONFIG_64BIT
 #include <asm/exception-64e.h>
+#include <asm/hw_irq.h>
+#include <asm/irqflags.h>
 #else
 #include "../kernel/head_booke.h" /* for THREAD_NORMSAVE() */
 #endif
@@ -465,6 +467,15 @@  _GLOBAL(kvmppc_resume_host)
 	mtspr	SPRN_EPCR, r3
 	isync
 
+#ifdef CONFIG_64BIT
+	/*
+	 * We enter with interrupts disabled in hardware, but
+	 * we need to call RECONCILE_IRQ_STATE anyway to ensure
+	 * that the software state is kept in sync.
+	 */
+	RECONCILE_IRQ_STATE(r3,r5)
+#endif
+
 	/* Switch to kernel stack and jump to handler. */
 	PPC_LL	r3, HOST_RUN(r1)
 	mr	r5, r14 /* intno */