| Message ID | 1382491608-4535-1-git-send-email-tiejun.chen@windriver.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
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(-)