* [PATCH] powerpc: add the missing required isync for the coherent icache flush
@ 2013-08-15 11:56 Kevin Hao
2013-08-19 3:24 ` Wang Dongsheng-B40534
0 siblings, 1 reply; 10+ messages in thread
From: Kevin Hao @ 2013-08-15 11:56 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc
Even we don't need to flush the dcache and invalidate the icache
on the CPU which has coherent icache. But we do need an isync to
discard the prefetched instructions in this case.
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
arch/powerpc/kernel/misc_32.S | 2 ++
arch/powerpc/kernel/misc_64.S | 1 +
2 files changed, 3 insertions(+)
diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
index 777d999..0b84c8d 100644
--- a/arch/powerpc/kernel/misc_32.S
+++ b/arch/powerpc/kernel/misc_32.S
@@ -433,6 +433,7 @@ _GLOBAL(invalidate_dcache_range)
*/
_GLOBAL(__flush_dcache_icache)
BEGIN_FTR_SECTION
+ isync
blr
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
rlwinm r3,r3,0,0,31-PAGE_SHIFT /* Get page base address */
@@ -474,6 +475,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_44x)
*/
_GLOBAL(__flush_dcache_icache_phys)
BEGIN_FTR_SECTION
+ isync
blr /* for 601, do nothing */
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
mfmsr r10
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index 992a78e..d74fefb 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -69,6 +69,7 @@ PPC64_CACHES:
_KPROBE(flush_icache_range)
BEGIN_FTR_SECTION
+ isync
blr
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [PATCH] powerpc: add the missing required isync for the coherent icache flush
2013-08-15 11:56 [PATCH] powerpc: add the missing required isync for the coherent icache flush Kevin Hao
@ 2013-08-19 3:24 ` Wang Dongsheng-B40534
2013-08-19 3:36 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 10+ messages in thread
From: Wang Dongsheng-B40534 @ 2013-08-19 3:24 UTC (permalink / raw)
To: Kevin Hao, Benjamin Herrenschmidt; +Cc: linuxppc
> -----Original Message-----
> From: Linuxppc-dev [mailto:linuxppc-dev-
> bounces+b40534=3Dfreescale.com@lists.ozlabs.org] On Behalf Of Kevin Hao
> Sent: Thursday, August 15, 2013 7:56 PM
> To: Benjamin Herrenschmidt
> Cc: linuxppc
> Subject: [PATCH] powerpc: add the missing required isync for the coherent
> icache flush
>=20
> Even we don't need to flush the dcache and invalidate the icache
> on the CPU which has coherent icache. But we do need an isync to
> discard the prefetched instructions in this case.
>=20
> Signed-off-by: Kevin Hao <haokexin@gmail.com>
> ---
> arch/powerpc/kernel/misc_32.S | 2 ++
> arch/powerpc/kernel/misc_64.S | 1 +
> 2 files changed, 3 insertions(+)
>=20
> diff --git a/arch/powerpc/kernel/misc_32.S
> b/arch/powerpc/kernel/misc_32.S
> index 777d999..0b84c8d 100644
> --- a/arch/powerpc/kernel/misc_32.S
> +++ b/arch/powerpc/kernel/misc_32.S
> @@ -433,6 +433,7 @@ _GLOBAL(invalidate_dcache_range)
> */
> _GLOBAL(__flush_dcache_icache)
> BEGIN_FTR_SECTION
> + isync
There is not touch the icache, why need we to do this?
> blr
> END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> rlwinm r3,r3,0,0,31-PAGE_SHIFT /* Get page base address
> */
> @@ -474,6 +475,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_44x)
> */
> _GLOBAL(__flush_dcache_icache_phys)
> BEGIN_FTR_SECTION
> + isync
> blr /* for 601, do nothing */
> END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> mfmsr r10
> diff --git a/arch/powerpc/kernel/misc_64.S
> b/arch/powerpc/kernel/misc_64.S
> index 992a78e..d74fefb 100644
> --- a/arch/powerpc/kernel/misc_64.S
> +++ b/arch/powerpc/kernel/misc_64.S
> @@ -69,6 +69,7 @@ PPC64_CACHES:
>=20
> _KPROBE(flush_icache_range)
> BEGIN_FTR_SECTION
> + isync
> blr
> END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> /*
> --
> 1.8.3.1
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc: add the missing required isync for the coherent icache flush
2013-08-19 3:24 ` Wang Dongsheng-B40534
@ 2013-08-19 3:36 ` Benjamin Herrenschmidt
2013-08-19 3:37 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 10+ messages in thread
From: Benjamin Herrenschmidt @ 2013-08-19 3:36 UTC (permalink / raw)
To: Wang Dongsheng-B40534; +Cc: linuxppc, Kevin Hao
On Mon, 2013-08-19 at 03:24 +0000, Wang Dongsheng-B40534 wrote:
>
> > -----Original Message-----
> > From: Linuxppc-dev [mailto:linuxppc-dev-
> > bounces+b40534=freescale.com@lists.ozlabs.org] On Behalf Of Kevin Hao
> > Sent: Thursday, August 15, 2013 7:56 PM
> > To: Benjamin Herrenschmidt
> > Cc: linuxppc
> > Subject: [PATCH] powerpc: add the missing required isync for the coherent
> > icache flush
> >
> > Even we don't need to flush the dcache and invalidate the icache
> > on the CPU which has coherent icache. But we do need an isync to
> > discard the prefetched instructions in this case.
> >
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> > ---
> > arch/powerpc/kernel/misc_32.S | 2 ++
> > arch/powerpc/kernel/misc_64.S | 1 +
> > 2 files changed, 3 insertions(+)
> >
> > diff --git a/arch/powerpc/kernel/misc_32.S
> > b/arch/powerpc/kernel/misc_32.S
> > index 777d999..0b84c8d 100644
> > --- a/arch/powerpc/kernel/misc_32.S
> > +++ b/arch/powerpc/kernel/misc_32.S
> > @@ -433,6 +433,7 @@ _GLOBAL(invalidate_dcache_range)
> > */
> > _GLOBAL(__flush_dcache_icache)
> > BEGIN_FTR_SECTION
> > + isync
> There is not touch the icache, why need we to do this?
The semantic of this function is to make data executable. Even if the
implementation has a snooping icache, it *still* needs to make sure
prefetched code is tossed out of the pipeline which is what isync
should provide.
The architecture actually specifies that.
Cheers,
Ben.
> > blr
> > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > rlwinm r3,r3,0,0,31-PAGE_SHIFT /* Get page base address
> > */
> > @@ -474,6 +475,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_44x)
> > */
> > _GLOBAL(__flush_dcache_icache_phys)
> > BEGIN_FTR_SECTION
> > + isync
> > blr /* for 601, do nothing */
> > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > mfmsr r10
> > diff --git a/arch/powerpc/kernel/misc_64.S
> > b/arch/powerpc/kernel/misc_64.S
> > index 992a78e..d74fefb 100644
> > --- a/arch/powerpc/kernel/misc_64.S
> > +++ b/arch/powerpc/kernel/misc_64.S
> > @@ -69,6 +69,7 @@ PPC64_CACHES:
> >
> > _KPROBE(flush_icache_range)
> > BEGIN_FTR_SECTION
> > + isync
> > blr
> > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > /*
> > --
> > 1.8.3.1
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc: add the missing required isync for the coherent icache flush
2013-08-19 3:36 ` Benjamin Herrenschmidt
@ 2013-08-19 3:37 ` Benjamin Herrenschmidt
2013-08-19 11:50 ` Kevin Hao
0 siblings, 1 reply; 10+ messages in thread
From: Benjamin Herrenschmidt @ 2013-08-19 3:37 UTC (permalink / raw)
To: Wang Dongsheng-B40534; +Cc: linuxppc, Kevin Hao
On Mon, 2013-08-19 at 13:36 +1000, Benjamin Herrenschmidt wrote:
> The semantic of this function is to make data executable. Even if the
> implementation has a snooping icache, it *still* needs to make sure
> prefetched code is tossed out of the pipeline which is what isync
> should provide.
>
> The architecture actually specifies that.
In fact, on P5 and later, I think we are supposed to do a single dummy
icbi followed by sync and isync.
Cheers,
Ben.
> Cheers,
> Ben.
>
> > > blr
> > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > rlwinm r3,r3,0,0,31-PAGE_SHIFT /* Get page base address
> > > */
> > > @@ -474,6 +475,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_44x)
> > > */
> > > _GLOBAL(__flush_dcache_icache_phys)
> > > BEGIN_FTR_SECTION
> > > + isync
> > > blr /* for 601, do nothing */
> > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > mfmsr r10
> > > diff --git a/arch/powerpc/kernel/misc_64.S
> > > b/arch/powerpc/kernel/misc_64.S
> > > index 992a78e..d74fefb 100644
> > > --- a/arch/powerpc/kernel/misc_64.S
> > > +++ b/arch/powerpc/kernel/misc_64.S
> > > @@ -69,6 +69,7 @@ PPC64_CACHES:
> > >
> > > _KPROBE(flush_icache_range)
> > > BEGIN_FTR_SECTION
> > > + isync
> > > blr
> > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > /*
> > > --
> > > 1.8.3.1
> > >
> > > _______________________________________________
> > > Linuxppc-dev mailing list
> > > Linuxppc-dev@lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/linuxppc-dev
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc: add the missing required isync for the coherent icache flush
2013-08-19 3:37 ` Benjamin Herrenschmidt
@ 2013-08-19 11:50 ` Kevin Hao
2013-08-19 12:45 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 10+ messages in thread
From: Kevin Hao @ 2013-08-19 11:50 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Wang Dongsheng-B40534, linuxppc
[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]
On Mon, Aug 19, 2013 at 01:37:17PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2013-08-19 at 13:36 +1000, Benjamin Herrenschmidt wrote:
>
> > The semantic of this function is to make data executable. Even if the
> > implementation has a snooping icache, it *still* needs to make sure
> > prefetched code is tossed out of the pipeline which is what isync
> > should provide.
> >
> > The architecture actually specifies that.
>
> In fact, on P5 and later, I think we are supposed to do a single dummy
> icbi followed by sync and isync.
Do you mean something like this? But why?
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index d74fefb..8fcdec7 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -69,6 +69,8 @@ PPC64_CACHES:
_KPROBE(flush_icache_range)
BEGIN_FTR_SECTION
+ icbi 0,r3
+ sync
isync
blr
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
@@ -209,6 +211,8 @@ _GLOBAL(flush_inval_dcache_range)
*/
_GLOBAL(__flush_dcache_icache)
BEGIN_FTR_SECTION
+ icbi 0,r3
+ sync
isync
blr
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
Thanks,
Kevin
>
> Cheers,
> Ben.
>
> > Cheers,
> > Ben.
> >
> > > > blr
> > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > rlwinm r3,r3,0,0,31-PAGE_SHIFT /* Get page base address
> > > > */
> > > > @@ -474,6 +475,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_44x)
> > > > */
> > > > _GLOBAL(__flush_dcache_icache_phys)
> > > > BEGIN_FTR_SECTION
> > > > + isync
> > > > blr /* for 601, do nothing */
> > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > mfmsr r10
> > > > diff --git a/arch/powerpc/kernel/misc_64.S
> > > > b/arch/powerpc/kernel/misc_64.S
> > > > index 992a78e..d74fefb 100644
> > > > --- a/arch/powerpc/kernel/misc_64.S
> > > > +++ b/arch/powerpc/kernel/misc_64.S
> > > > @@ -69,6 +69,7 @@ PPC64_CACHES:
> > > >
> > > > _KPROBE(flush_icache_range)
> > > > BEGIN_FTR_SECTION
> > > > + isync
> > > > blr
> > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > /*
> > > > --
> > > > 1.8.3.1
> > > >
> > > > _______________________________________________
> > > > Linuxppc-dev mailing list
> > > > Linuxppc-dev@lists.ozlabs.org
> > > > https://lists.ozlabs.org/listinfo/linuxppc-dev
> > >
> >
>
>
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc: add the missing required isync for the coherent icache flush
2013-08-19 11:50 ` Kevin Hao
@ 2013-08-19 12:45 ` Benjamin Herrenschmidt
2013-08-20 12:16 ` Kevin Hao
0 siblings, 1 reply; 10+ messages in thread
From: Benjamin Herrenschmidt @ 2013-08-19 12:45 UTC (permalink / raw)
To: Kevin Hao; +Cc: Wang Dongsheng-B40534, linuxppc
On Mon, 2013-08-19 at 19:50 +0800, Kevin Hao wrote:
> On Mon, Aug 19, 2013 at 01:37:17PM +1000, Benjamin Herrenschmidt wrote:
> > On Mon, 2013-08-19 at 13:36 +1000, Benjamin Herrenschmidt wrote:
> >
> > > The semantic of this function is to make data executable. Even if the
> > > implementation has a snooping icache, it *still* needs to make sure
> > > prefetched code is tossed out of the pipeline which is what isync
> > > should provide.
> > >
> > > The architecture actually specifies that.
> >
> > In fact, on P5 and later, I think we are supposed to do a single dummy
> > icbi followed by sync and isync.
>
> Do you mean something like this? But why?
It has to do with making sure prefetched and/or speculated instructions
are properly tossed.
My understanding is that icbi basically tells the ifetch buffers to drop
it, sync orders the icbi and isync ensures its execution has been
synchronized. At least I *think* that's the required sequence, I have to
dbl check the arch, maybe tomorrow. I wouldn't be surprise if we also
need a sync before the icbi to order the actual stores to memory that
might have modified instructions with the icbi.
Cheers,
Ben.
> diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> index d74fefb..8fcdec7 100644
> --- a/arch/powerpc/kernel/misc_64.S
> +++ b/arch/powerpc/kernel/misc_64.S
> @@ -69,6 +69,8 @@ PPC64_CACHES:
>
> _KPROBE(flush_icache_range)
> BEGIN_FTR_SECTION
> + icbi 0,r3
> + sync
> isync
> blr
> END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> @@ -209,6 +211,8 @@ _GLOBAL(flush_inval_dcache_range)
> */
> _GLOBAL(__flush_dcache_icache)
> BEGIN_FTR_SECTION
> + icbi 0,r3
> + sync
> isync
> blr
> END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
>
> Thanks,
> Kevin
>
> >
> > Cheers,
> > Ben.
> >
> > > Cheers,
> > > Ben.
> > >
> > > > > blr
> > > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > > rlwinm r3,r3,0,0,31-PAGE_SHIFT /* Get page base address
> > > > > */
> > > > > @@ -474,6 +475,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_44x)
> > > > > */
> > > > > _GLOBAL(__flush_dcache_icache_phys)
> > > > > BEGIN_FTR_SECTION
> > > > > + isync
> > > > > blr /* for 601, do nothing */
> > > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > > mfmsr r10
> > > > > diff --git a/arch/powerpc/kernel/misc_64.S
> > > > > b/arch/powerpc/kernel/misc_64.S
> > > > > index 992a78e..d74fefb 100644
> > > > > --- a/arch/powerpc/kernel/misc_64.S
> > > > > +++ b/arch/powerpc/kernel/misc_64.S
> > > > > @@ -69,6 +69,7 @@ PPC64_CACHES:
> > > > >
> > > > > _KPROBE(flush_icache_range)
> > > > > BEGIN_FTR_SECTION
> > > > > + isync
> > > > > blr
> > > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > > /*
> > > > > --
> > > > > 1.8.3.1
> > > > >
> > > > > _______________________________________________
> > > > > Linuxppc-dev mailing list
> > > > > Linuxppc-dev@lists.ozlabs.org
> > > > > https://lists.ozlabs.org/listinfo/linuxppc-dev
> > > >
> > >
> >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc: add the missing required isync for the coherent icache flush
2013-08-19 12:45 ` Benjamin Herrenschmidt
@ 2013-08-20 12:16 ` Kevin Hao
2013-08-20 21:28 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 10+ messages in thread
From: Kevin Hao @ 2013-08-20 12:16 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Wang Dongsheng-B40534, linuxppc
[-- Attachment #1: Type: text/plain, Size: 3700 bytes --]
On Mon, Aug 19, 2013 at 10:45:35PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2013-08-19 at 19:50 +0800, Kevin Hao wrote:
> > On Mon, Aug 19, 2013 at 01:37:17PM +1000, Benjamin Herrenschmidt wrote:
> > > On Mon, 2013-08-19 at 13:36 +1000, Benjamin Herrenschmidt wrote:
> > >
> > > > The semantic of this function is to make data executable. Even if the
> > > > implementation has a snooping icache, it *still* needs to make sure
> > > > prefetched code is tossed out of the pipeline which is what isync
> > > > should provide.
> > > >
> > > > The architecture actually specifies that.
> > >
> > > In fact, on P5 and later, I think we are supposed to do a single dummy
> > > icbi followed by sync and isync.
> >
> > Do you mean something like this? But why?
>
> It has to do with making sure prefetched and/or speculated instructions
> are properly tossed.
>
> My understanding is that icbi basically tells the ifetch buffers to drop
> it
Dummy question: What does the ifetch buffers mean? The instruction fetch
pipeline or instruction dispatch pipeline? Shouldn't all the prefetched
instructions in these buffers be discarded by isync?
>, sync orders the icbi and isync ensures its execution has been
> synchronized. At least I *think* that's the required sequence, I have to
> dbl check the arch, maybe tomorrow. I wouldn't be surprise if we also
> need a sync before the icbi to order the actual stores to memory that
> might have modified instructions with the icbi.
Doesn't the coherence between icache and dcache be maintained by the snooping?
Thanks,
Kevin
>
> Cheers,
> Ben.
>
> > diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> > index d74fefb..8fcdec7 100644
> > --- a/arch/powerpc/kernel/misc_64.S
> > +++ b/arch/powerpc/kernel/misc_64.S
> > @@ -69,6 +69,8 @@ PPC64_CACHES:
> >
> > _KPROBE(flush_icache_range)
> > BEGIN_FTR_SECTION
> > + icbi 0,r3
> > + sync
> > isync
> > blr
> > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > @@ -209,6 +211,8 @@ _GLOBAL(flush_inval_dcache_range)
> > */
> > _GLOBAL(__flush_dcache_icache)
> > BEGIN_FTR_SECTION
> > + icbi 0,r3
> > + sync
> > isync
> > blr
> > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> >
> > Thanks,
> > Kevin
> >
> > >
> > > Cheers,
> > > Ben.
> > >
> > > > Cheers,
> > > > Ben.
> > > >
> > > > > > blr
> > > > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > > > rlwinm r3,r3,0,0,31-PAGE_SHIFT /* Get page base address
> > > > > > */
> > > > > > @@ -474,6 +475,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_44x)
> > > > > > */
> > > > > > _GLOBAL(__flush_dcache_icache_phys)
> > > > > > BEGIN_FTR_SECTION
> > > > > > + isync
> > > > > > blr /* for 601, do nothing */
> > > > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > > > mfmsr r10
> > > > > > diff --git a/arch/powerpc/kernel/misc_64.S
> > > > > > b/arch/powerpc/kernel/misc_64.S
> > > > > > index 992a78e..d74fefb 100644
> > > > > > --- a/arch/powerpc/kernel/misc_64.S
> > > > > > +++ b/arch/powerpc/kernel/misc_64.S
> > > > > > @@ -69,6 +69,7 @@ PPC64_CACHES:
> > > > > >
> > > > > > _KPROBE(flush_icache_range)
> > > > > > BEGIN_FTR_SECTION
> > > > > > + isync
> > > > > > blr
> > > > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > > > /*
> > > > > > --
> > > > > > 1.8.3.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > Linuxppc-dev mailing list
> > > > > > Linuxppc-dev@lists.ozlabs.org
> > > > > > https://lists.ozlabs.org/listinfo/linuxppc-dev
> > > > >
> > > >
> > >
> > >
>
>
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc: add the missing required isync for the coherent icache flush
2013-08-20 12:16 ` Kevin Hao
@ 2013-08-20 21:28 ` Benjamin Herrenschmidt
2013-08-21 11:49 ` Kevin Hao
0 siblings, 1 reply; 10+ messages in thread
From: Benjamin Herrenschmidt @ 2013-08-20 21:28 UTC (permalink / raw)
To: Kevin Hao; +Cc: Wang Dongsheng-B40534, linuxppc
On Tue, 2013-08-20 at 20:16 +0800, Kevin Hao wrote:
> Dummy question: What does the ifetch buffers mean? The instruction fetch
> pipeline or instruction dispatch pipeline? Shouldn't all the prefetched
> instructions in these buffers be discarded by isync?
Architecturally isync doesn't have to toss prefetch completely. It only
needs to make sense that context changes performed by previous
instructions (and interrupts/traps) happen at the point of the isync,
for example, it will ensure that a trap conditional is fully evaluated
before subsequent instruction execution, etc....
So in this case, it makes sure the icbi has been executed and it's the
icbi that invalidates the prefetched instructions.
>
> >, sync orders the icbi and isync ensures its execution has been
> > synchronized. At least I *think* that's the required sequence, I have to
> > dbl check the arch, maybe tomorrow. I wouldn't be surprise if we also
> > need a sync before the icbi to order the actual stores to memory that
> > might have modified instructions with the icbi.
>
> Doesn't the coherence between icache and dcache be maintained by the snooping?
The icache yes, but not the ifetch buffers (basically think of them as
buffering stages between icache and dispatch). At least that's my
understanding of the implementation.
Cheers,
Ben.
> Thanks,
> Kevin
>
> >
> > Cheers,
> > Ben.
> >
> > > diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> > > index d74fefb..8fcdec7 100644
> > > --- a/arch/powerpc/kernel/misc_64.S
> > > +++ b/arch/powerpc/kernel/misc_64.S
> > > @@ -69,6 +69,8 @@ PPC64_CACHES:
> > >
> > > _KPROBE(flush_icache_range)
> > > BEGIN_FTR_SECTION
> > > + icbi 0,r3
> > > + sync
> > > isync
> > > blr
> > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > @@ -209,6 +211,8 @@ _GLOBAL(flush_inval_dcache_range)
> > > */
> > > _GLOBAL(__flush_dcache_icache)
> > > BEGIN_FTR_SECTION
> > > + icbi 0,r3
> > > + sync
> > > isync
> > > blr
> > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > >
> > > Thanks,
> > > Kevin
> > >
> > > >
> > > > Cheers,
> > > > Ben.
> > > >
> > > > > Cheers,
> > > > > Ben.
> > > > >
> > > > > > > blr
> > > > > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > > > > rlwinm r3,r3,0,0,31-PAGE_SHIFT /* Get page base address
> > > > > > > */
> > > > > > > @@ -474,6 +475,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_44x)
> > > > > > > */
> > > > > > > _GLOBAL(__flush_dcache_icache_phys)
> > > > > > > BEGIN_FTR_SECTION
> > > > > > > + isync
> > > > > > > blr /* for 601, do nothing */
> > > > > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > > > > mfmsr r10
> > > > > > > diff --git a/arch/powerpc/kernel/misc_64.S
> > > > > > > b/arch/powerpc/kernel/misc_64.S
> > > > > > > index 992a78e..d74fefb 100644
> > > > > > > --- a/arch/powerpc/kernel/misc_64.S
> > > > > > > +++ b/arch/powerpc/kernel/misc_64.S
> > > > > > > @@ -69,6 +69,7 @@ PPC64_CACHES:
> > > > > > >
> > > > > > > _KPROBE(flush_icache_range)
> > > > > > > BEGIN_FTR_SECTION
> > > > > > > + isync
> > > > > > > blr
> > > > > > > END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
> > > > > > > /*
> > > > > > > --
> > > > > > > 1.8.3.1
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > Linuxppc-dev mailing list
> > > > > > > Linuxppc-dev@lists.ozlabs.org
> > > > > > > https://lists.ozlabs.org/listinfo/linuxppc-dev
> > > > > >
> > > > >
> > > >
> > > >
> >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] powerpc: add the missing required isync for the coherent icache flush
2013-08-20 21:28 ` Benjamin Herrenschmidt
@ 2013-08-21 11:49 ` Kevin Hao
2013-08-22 1:30 ` [PATCH v2] powerpc: purge all the prefetched instructions " Kevin Hao
0 siblings, 1 reply; 10+ messages in thread
From: Kevin Hao @ 2013-08-21 11:49 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Wang Dongsheng-B40534, linuxppc
[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]
On Wed, Aug 21, 2013 at 07:28:54AM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2013-08-20 at 20:16 +0800, Kevin Hao wrote:
>
> > Dummy question: What does the ifetch buffers mean? The instruction fetch
> > pipeline or instruction dispatch pipeline? Shouldn't all the prefetched
> > instructions in these buffers be discarded by isync?
>
> Architecturally isync doesn't have to toss prefetch completely. It only
> needs to make sense that context changes performed by previous
> instructions (and interrupts/traps) happen at the point of the isync,
> for example, it will ensure that a trap conditional is fully evaluated
> before subsequent instruction execution, etc....
>
> So in this case, it makes sure the icbi has been executed and it's the
> icbi that invalidates the prefetched instructions.
>
> >
> > >, sync orders the icbi and isync ensures its execution has been
> > > synchronized. At least I *think* that's the required sequence, I have to
> > > dbl check the arch, maybe tomorrow. I wouldn't be surprise if we also
> > > need a sync before the icbi to order the actual stores to memory that
> > > might have modified instructions with the icbi.
> >
> > Doesn't the coherence between icache and dcache be maintained by the snooping?
>
> The icache yes, but not the ifetch buffers (basically think of them as
> buffering stages between icache and dispatch). At least that's my
> understanding of the implementation.
OK. Thanks for the explanation. I will update the patch according to your
suggestions.
Thanks,
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] powerpc: purge all the prefetched instructions for the coherent icache flush
2013-08-21 11:49 ` Kevin Hao
@ 2013-08-22 1:30 ` Kevin Hao
0 siblings, 0 replies; 10+ messages in thread
From: Kevin Hao @ 2013-08-22 1:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc
As Benjamin Herrenschmidt has indicated, we still need a dummy icbi to
purge all the prefetched instructions from the ifetch buffers for the
snooping icache. We also need a sync before the icbi to order the
actual stores to memory that might have modified instructions with
the icbi.
Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
arch/powerpc/include/asm/cache.h | 14 +++++++++++++-
arch/powerpc/kernel/misc_32.S | 4 +++-
arch/powerpc/kernel/misc_64.S | 3 ++-
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/cache.h b/arch/powerpc/include/asm/cache.h
index 9e495c9..ed0afc1 100644
--- a/arch/powerpc/include/asm/cache.h
+++ b/arch/powerpc/include/asm/cache.h
@@ -41,8 +41,20 @@ struct ppc64_caches {
extern struct ppc64_caches ppc64_caches;
#endif /* __powerpc64__ && ! __ASSEMBLY__ */
-#if !defined(__ASSEMBLY__)
+#if defined(__ASSEMBLY__)
+/*
+ * For a snooping icache, we still need a dummy icbi to purge all the
+ * prefetched instructions from the ifetch buffers. We also need a sync
+ * before the icbi to order the the actual stores to memory that might
+ * have modified instructions with the icbi.
+ */
+#define PURGE_PREFETCHED_INS \
+ sync; \
+ icbi 0,r3; \
+ sync; \
+ isync
+#else
#define __read_mostly __attribute__((__section__(".data..read_mostly")))
#ifdef CONFIG_6xx
diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
index 777d999..d1e819b 100644
--- a/arch/powerpc/kernel/misc_32.S
+++ b/arch/powerpc/kernel/misc_32.S
@@ -329,7 +329,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_UNIFIED_ID_CACHE)
*/
_KPROBE(flush_icache_range)
BEGIN_FTR_SECTION
- isync
+ PURGE_PREFETCHED_INS
blr /* for 601, do nothing */
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
li r5,L1_CACHE_BYTES-1
@@ -433,6 +433,7 @@ _GLOBAL(invalidate_dcache_range)
*/
_GLOBAL(__flush_dcache_icache)
BEGIN_FTR_SECTION
+ PURGE_PREFETCHED_INS
blr
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
rlwinm r3,r3,0,0,31-PAGE_SHIFT /* Get page base address */
@@ -474,6 +475,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_44x)
*/
_GLOBAL(__flush_dcache_icache_phys)
BEGIN_FTR_SECTION
+ PURGE_PREFETCHED_INS
blr /* for 601, do nothing */
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
mfmsr r10
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index 992a78e..f9e8770 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -69,6 +69,7 @@ PPC64_CACHES:
_KPROBE(flush_icache_range)
BEGIN_FTR_SECTION
+ PURGE_PREFETCHED_INS
blr
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
/*
@@ -208,7 +209,7 @@ _GLOBAL(flush_inval_dcache_range)
*/
_GLOBAL(__flush_dcache_icache)
BEGIN_FTR_SECTION
- isync
+ PURGE_PREFETCHED_INS
blr
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
/*
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-08-22 1:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-15 11:56 [PATCH] powerpc: add the missing required isync for the coherent icache flush Kevin Hao
2013-08-19 3:24 ` Wang Dongsheng-B40534
2013-08-19 3:36 ` Benjamin Herrenschmidt
2013-08-19 3:37 ` Benjamin Herrenschmidt
2013-08-19 11:50 ` Kevin Hao
2013-08-19 12:45 ` Benjamin Herrenschmidt
2013-08-20 12:16 ` Kevin Hao
2013-08-20 21:28 ` Benjamin Herrenschmidt
2013-08-21 11:49 ` Kevin Hao
2013-08-22 1:30 ` [PATCH v2] powerpc: purge all the prefetched instructions " Kevin Hao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).