* [PATCH v3] powerpc32: memset: only use dcbz once cache is enabled
@ 2015-09-14 6:21 Christophe Leroy
2015-09-14 7:15 ` Michal Sojka
2015-09-14 15:20 ` Scott Wood
0 siblings, 2 replies; 6+ messages in thread
From: Christophe Leroy @ 2015-09-14 6:21 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
scottwood, sojkam1
Cc: linux-kernel, linuxppc-dev
memset() uses instruction dcbz to speed up clearing by not wasting time
loading cache line with data that will be overwritten.
Some platform like mpc52xx do no have cache active at startup and
can therefore not use memset(). Allthough no part of the code
explicitly uses memset(), GCC may makes calls to it.
This patch modifies memset() such that at startup, memset()
unconditionally jumps to simple_memset() which doesn't use
the dcbz instruction.
Once the initial MMU is set up, in machine_init() we patch memset()
by replacing this inconditional jump by a NOP
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
This patch goes on to of [v3] powerpc32: memcpy: only use dcbz once cache is enabled
Changes in v2:
was part of [v2] powerpc32: memcpy/memset: only use dcbz once cache is enabled
changes in v3:
Not using anymore feature-fixups
Handling of memcpy() and memset() split in two patches
arch/powerpc/kernel/setup_32.c | 1 +
arch/powerpc/lib/copy_32.S | 15 +++++++++++++++
2 files changed, 16 insertions(+)
diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index 362495f..345ec3a 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -124,6 +124,7 @@ notrace void __init machine_init(u64 dt_ptr)
udbg_early_init();
patch_instruction((unsigned int *)&memcpy, PPC_INST_NOP);
+ patch_instruction((unsigned int *)&memset, PPC_INST_NOP);
/* Do some early initialization based on the flat device tree */
early_init_devtree(__va(dt_ptr));
diff --git a/arch/powerpc/lib/copy_32.S b/arch/powerpc/lib/copy_32.S
index da5847d..68a59d4 100644
--- a/arch/powerpc/lib/copy_32.S
+++ b/arch/powerpc/lib/copy_32.S
@@ -73,8 +73,13 @@ CACHELINE_MASK = (L1_CACHE_BYTES-1)
* Use dcbz on the complete cache lines in the destination
* to set them to zero. This requires that the destination
* area is cacheable. -- paulus
+ *
+ * During early init, cache might not be active yet, so dcbz cannot be used.
+ * We therefore jump to simple_memset which doesn't use dcbz. This jump is
+ * replaced by a nop once cache is active. This is done in machine_init()
*/
_GLOBAL(memset)
+ b simple_memset
rlwimi r4,r4,8,16,23
rlwimi r4,r4,16,0,15
@@ -122,6 +127,16 @@ _GLOBAL(memset)
bdnz 8b
blr
+/* Simple version of memset used during early boot until cache is enabled */
+simple_memset:
+ cmplwi cr0,r5,0
+ addi r6,r3,-1
+ beqlr
+ mtctr r5
+1: stbu r4,1(r6)
+ bdnz 1b
+ blr
+
/*
* This version uses dcbz on the complete cache lines in the
* destination area to reduce memory traffic. This requires that
--
2.1.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] powerpc32: memset: only use dcbz once cache is enabled
2015-09-14 6:21 [PATCH v3] powerpc32: memset: only use dcbz once cache is enabled Christophe Leroy
@ 2015-09-14 7:15 ` Michal Sojka
2015-09-14 15:20 ` Scott Wood
1 sibling, 0 replies; 6+ messages in thread
From: Michal Sojka @ 2015-09-14 7:15 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, scottwood
Cc: linux-kernel, linuxppc-dev
On Mon, Sep 14 2015, Christophe Leroy wrote:
> memset() uses instruction dcbz to speed up clearing by not wasting time
> loading cache line with data that will be overwritten.
> Some platform like mpc52xx do no have cache active at startup and
> can therefore not use memset(). Allthough no part of the code
> explicitly uses memset(), GCC may makes calls to it.
>
> This patch modifies memset() such that at startup, memset()
> unconditionally jumps to simple_memset() which doesn't use
> the dcbz instruction.
I tested both v3 patches (memcpy and memset) and they work for me.
Thanks
-Michal
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] powerpc32: memset: only use dcbz once cache is enabled
2015-09-14 6:21 [PATCH v3] powerpc32: memset: only use dcbz once cache is enabled Christophe Leroy
2015-09-14 7:15 ` Michal Sojka
@ 2015-09-14 15:20 ` Scott Wood
2015-09-14 15:44 ` Christophe LEROY
1 sibling, 1 reply; 6+ messages in thread
From: Scott Wood @ 2015-09-14 15:20 UTC (permalink / raw)
To: Christophe Leroy
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, sojkam1,
linux-kernel, linuxppc-dev
On Mon, 2015-09-14 at 08:21 +0200, Christophe Leroy wrote:
> memset() uses instruction dcbz to speed up clearing by not wasting time
> loading cache line with data that will be overwritten.
> Some platform like mpc52xx do no have cache active at startup and
> can therefore not use memset(). Allthough no part of the code
> explicitly uses memset(), GCC may makes calls to it.
>
> This patch modifies memset() such that at startup, memset()
> unconditionally jumps to simple_memset() which doesn't use
> the dcbz instruction.
>
> Once the initial MMU is set up, in machine_init() we patch memset()
> by replacing this inconditional jump by a NOP
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> This patch goes on to of [v3] powerpc32: memcpy: only use dcbz once cache
> is enabled
>
> Changes in v2:
> was part of [v2] powerpc32: memcpy/memset: only use dcbz once cache is
> enabled
> changes in v3:
> Not using anymore feature-fixups
> Handling of memcpy() and memset() split in two patches
>
> arch/powerpc/kernel/setup_32.c | 1 +
> arch/powerpc/lib/copy_32.S | 15 +++++++++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
> index 362495f..345ec3a 100644
> --- a/arch/powerpc/kernel/setup_32.c
> +++ b/arch/powerpc/kernel/setup_32.c
> @@ -124,6 +124,7 @@ notrace void __init machine_init(u64 dt_ptr)
> udbg_early_init();
>
> patch_instruction((unsigned int *)&memcpy, PPC_INST_NOP);
> + patch_instruction((unsigned int *)&memset, PPC_INST_NOP);
>
> /* Do some early initialization based on the flat device tree */
> early_init_devtree(__va(dt_ptr));
> diff --git a/arch/powerpc/lib/copy_32.S b/arch/powerpc/lib/copy_32.S
> index da5847d..68a59d4 100644
> --- a/arch/powerpc/lib/copy_32.S
> +++ b/arch/powerpc/lib/copy_32.S
> @@ -73,8 +73,13 @@ CACHELINE_MASK = (L1_CACHE_BYTES-1)
> * Use dcbz on the complete cache lines in the destination
> * to set them to zero. This requires that the destination
> * area is cacheable. -- paulus
> + *
> + * During early init, cache might not be active yet, so dcbz cannot be
> used.
> + * We therefore jump to simple_memset which doesn't use dcbz. This jump is
> + * replaced by a nop once cache is active. This is done in machine_init()
> */
> _GLOBAL(memset)
> + b simple_memset
> rlwimi r4,r4,8,16,23
> rlwimi r4,r4,16,0,15
>
> @@ -122,6 +127,16 @@ _GLOBAL(memset)
> bdnz 8b
> blr
>
> +/* Simple version of memset used during early boot until cache is enabled
> */
> +simple_memset:
> + cmplwi cr0,r5,0
> + addi r6,r3,-1
> + beqlr
> + mtctr r5
> +1: stbu r4,1(r6)
> + bdnz 1b
> + blr
Instead couldn't you use the generic memset at label 2: and patch the "bne
2f"?
-Scott
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] powerpc32: memset: only use dcbz once cache is enabled
2015-09-14 15:20 ` Scott Wood
@ 2015-09-14 15:44 ` Christophe LEROY
2015-09-14 16:13 ` Scott Wood
0 siblings, 1 reply; 6+ messages in thread
From: Christophe LEROY @ 2015-09-14 15:44 UTC (permalink / raw)
To: Scott Wood
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, sojkam1,
linux-kernel, linuxppc-dev
Le 14/09/2015 17:20, Scott Wood a écrit :
> On Mon, 2015-09-14 at 08:21 +0200, Christophe Leroy wrote:
>> memset() uses instruction dcbz to speed up clearing by not wasting time
>> loading cache line with data that will be overwritten.
>> Some platform like mpc52xx do no have cache active at startup and
>> can therefore not use memset(). Allthough no part of the code
>> explicitly uses memset(), GCC may makes calls to it.
>>
>> This patch modifies memset() such that at startup, memset()
>> unconditionally jumps to simple_memset() which doesn't use
>> the dcbz instruction.
>>
>> Once the initial MMU is set up, in machine_init() we patch memset()
>> by replacing this inconditional jump by a NOP
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> This patch goes on to of [v3] powerpc32: memcpy: only use dcbz once cache
>> is enabled
>>
>> Changes in v2:
>> was part of [v2] powerpc32: memcpy/memset: only use dcbz once cache is
>> enabled
>> changes in v3:
>> Not using anymore feature-fixups
>> Handling of memcpy() and memset() split in two patches
>>
>> arch/powerpc/kernel/setup_32.c | 1 +
>> arch/powerpc/lib/copy_32.S | 15 +++++++++++++++
>> 2 files changed, 16 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
>> index 362495f..345ec3a 100644
>> --- a/arch/powerpc/kernel/setup_32.c
>> +++ b/arch/powerpc/kernel/setup_32.c
>> @@ -124,6 +124,7 @@ notrace void __init machine_init(u64 dt_ptr)
>> udbg_early_init();
>>
>> patch_instruction((unsigned int *)&memcpy, PPC_INST_NOP);
>> + patch_instruction((unsigned int *)&memset, PPC_INST_NOP);
>>
>> /* Do some early initialization based on the flat device tree */
>> early_init_devtree(__va(dt_ptr));
>> diff --git a/arch/powerpc/lib/copy_32.S b/arch/powerpc/lib/copy_32.S
>> index da5847d..68a59d4 100644
>> --- a/arch/powerpc/lib/copy_32.S
>> +++ b/arch/powerpc/lib/copy_32.S
>> @@ -73,8 +73,13 @@ CACHELINE_MASK = (L1_CACHE_BYTES-1)
>> * Use dcbz on the complete cache lines in the destination
>> * to set them to zero. This requires that the destination
>> * area is cacheable. -- paulus
>> + *
>> + * During early init, cache might not be active yet, so dcbz cannot be
>> used.
>> + * We therefore jump to simple_memset which doesn't use dcbz. This jump is
>> + * replaced by a nop once cache is active. This is done in machine_init()
>> */
>> _GLOBAL(memset)
>> + b simple_memset
>> rlwimi r4,r4,8,16,23
>> rlwimi r4,r4,16,0,15
>>
>> @@ -122,6 +127,16 @@ _GLOBAL(memset)
>> bdnz 8b
>> blr
>>
>> +/* Simple version of memset used during early boot until cache is enabled
>> */
>> +simple_memset:
>> + cmplwi cr0,r5,0
>> + addi r6,r3,-1
>> + beqlr
>> + mtctr r5
>> +1: stbu r4,1(r6)
>> + bdnz 1b
>> + blr
> Instead couldn't you use the generic memset at label 2: and patch the "bne
> 2f"?
Yes I could but it means adding a global symbol there at the "bne 2f". I
thought it was what Michael didn't like in my v1 of memcpy().
What name could I give to that symbol ? Something like
memcpy_nocache_patch: ?
What would be the best ? Having b 2f, and replacing it with bne 2f ? Or
have b 2f just above and replace it by nop once cache is up ?
Christophe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] powerpc32: memset: only use dcbz once cache is enabled
2015-09-14 15:44 ` Christophe LEROY
@ 2015-09-14 16:13 ` Scott Wood
2015-09-14 23:52 ` Michael Ellerman
0 siblings, 1 reply; 6+ messages in thread
From: Scott Wood @ 2015-09-14 16:13 UTC (permalink / raw)
To: Christophe LEROY
Cc: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman, sojkam1,
linux-kernel, linuxppc-dev
On Mon, 2015-09-14 at 17:44 +0200, Christophe LEROY wrote:
> Le 14/09/2015 17:20, Scott Wood a écrit :
> > On Mon, 2015-09-14 at 08:21 +0200, Christophe Leroy wrote:
> > > memset() uses instruction dcbz to speed up clearing by not wasting time
> > > loading cache line with data that will be overwritten.
> > > Some platform like mpc52xx do no have cache active at startup and
> > > can therefore not use memset(). Allthough no part of the code
> > > explicitly uses memset(), GCC may makes calls to it.
> > >
> > > This patch modifies memset() such that at startup, memset()
> > > unconditionally jumps to simple_memset() which doesn't use
> > > the dcbz instruction.
> > >
> > > Once the initial MMU is set up, in machine_init() we patch memset()
> > > by replacing this inconditional jump by a NOP
> > >
> > > Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > > ---
> > > This patch goes on to of [v3] powerpc32: memcpy: only use dcbz once
> > > cache
> > > is enabled
> > >
> > > Changes in v2:
> > > was part of [v2] powerpc32: memcpy/memset: only use dcbz once cache
> > > is
> > > enabled
> > > changes in v3:
> > > Not using anymore feature-fixups
> > > Handling of memcpy() and memset() split in two patches
> > >
> > > arch/powerpc/kernel/setup_32.c | 1 +
> > > arch/powerpc/lib/copy_32.S | 15 +++++++++++++++
> > > 2 files changed, 16 insertions(+)
> > >
> > > diff --git a/arch/powerpc/kernel/setup_32.c
> > > b/arch/powerpc/kernel/setup_32.c
> > > index 362495f..345ec3a 100644
> > > --- a/arch/powerpc/kernel/setup_32.c
> > > +++ b/arch/powerpc/kernel/setup_32.c
> > > @@ -124,6 +124,7 @@ notrace void __init machine_init(u64 dt_ptr)
> > > udbg_early_init();
> > >
> > > patch_instruction((unsigned int *)&memcpy, PPC_INST_NOP);
> > > + patch_instruction((unsigned int *)&memset, PPC_INST_NOP);
> > >
> > > /* Do some early initialization based on the flat device tree */
> > > early_init_devtree(__va(dt_ptr));
> > > diff --git a/arch/powerpc/lib/copy_32.S b/arch/powerpc/lib/copy_32.S
> > > index da5847d..68a59d4 100644
> > > --- a/arch/powerpc/lib/copy_32.S
> > > +++ b/arch/powerpc/lib/copy_32.S
> > > @@ -73,8 +73,13 @@ CACHELINE_MASK = (L1_CACHE_BYTES-1)
> > > * Use dcbz on the complete cache lines in the destination
> > > * to set them to zero. This requires that the destination
> > > * area is cacheable. -- paulus
> > > + *
> > > + * During early init, cache might not be active yet, so dcbz cannot be
> > > used.
> > > + * We therefore jump to simple_memset which doesn't use dcbz. This
> > > jump is
> > > + * replaced by a nop once cache is active. This is done in
> > > machine_init()
> > > */
> > > _GLOBAL(memset)
> > > + b simple_memset
> > > rlwimi r4,r4,8,16,23
> > > rlwimi r4,r4,16,0,15
> > >
> > > @@ -122,6 +127,16 @@ _GLOBAL(memset)
> > > bdnz 8b
> > > blr
> > >
> > > +/* Simple version of memset used during early boot until cache is
> > > enabled
> > > */
> > > +simple_memset:
> > > + cmplwi cr0,r5,0
> > > + addi r6,r3,-1
> > > + beqlr
> > > + mtctr r5
> > > +1: stbu r4,1(r6)
> > > + bdnz 1b
> > > + blr
> > Instead couldn't you use the generic memset at label 2: and patch the "bne
> > 2f"?
>
> Yes I could but it means adding a global symbol there at the "bne 2f". I
> thought it was what Michael didn't like in my v1 of memcpy().
> What name could I give to that symbol ? Something like
> memcpy_nocache_patch: ?
> What would be the best ? Having b 2f, and replacing it with bne 2f ? Or
> have b 2f just above and replace it by nop once cache is up ?
I'm not sure why a global symbol for an instruction to be patched would be a
big deal (similar to kvm_emul.S)... I thought Michael just wanted existing
infrastructure to be used if it's practical to do so.
-Scott
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] powerpc32: memset: only use dcbz once cache is enabled
2015-09-14 16:13 ` Scott Wood
@ 2015-09-14 23:52 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2015-09-14 23:52 UTC (permalink / raw)
To: Scott Wood
Cc: Christophe LEROY, Benjamin Herrenschmidt, Paul Mackerras, sojkam1,
linux-kernel, linuxppc-dev
On Mon, 2015-09-14 at 11:13 -0500, Scott Wood wrote:
> On Mon, 2015-09-14 at 17:44 +0200, Christophe LEROY wrote:
> > Le 14/09/2015 17:20, Scott Wood a écrit :
> > > On Mon, 2015-09-14 at 08:21 +0200, Christophe Leroy wrote:
> > > > diff --git a/arch/powerpc/lib/copy_32.S b/arch/powerpc/lib/copy_32.S
> > > > index da5847d..68a59d4 100644
> > > > --- a/arch/powerpc/lib/copy_32.S
> > > > +++ b/arch/powerpc/lib/copy_32.S
> > > > @@ -73,8 +73,13 @@ CACHELINE_MASK = (L1_CACHE_BYTES-1)
> > > > @@ -122,6 +127,16 @@ _GLOBAL(memset)
> > > > bdnz 8b
> > > > blr
> > > >
> > > > +/* Simple version of memset used during early boot until cache is
> > > > enabled
> > > > */
> > > > +simple_memset:
> > > > + cmplwi cr0,r5,0
> > > > + addi r6,r3,-1
> > > > + beqlr
> > > > + mtctr r5
> > > > +1: stbu r4,1(r6)
> > > > + bdnz 1b
> > > > + blr
> > > Instead couldn't you use the generic memset at label 2: and patch the "bne
> > > 2f"?
> >
> > Yes I could but it means adding a global symbol there at the "bne 2f". I
> > thought it was what Michael didn't like in my v1 of memcpy().
> > What name could I give to that symbol ? Something like
> > memcpy_nocache_patch: ?
> > What would be the best ? Having b 2f, and replacing it with bne 2f ? Or
> > have b 2f just above and replace it by nop once cache is up ?
>
> I'm not sure why a global symbol for an instruction to be patched would be a
> big deal (similar to kvm_emul.S)... I thought Michael just wanted existing
> infrastructure to be used if it's practical to do so.
Yeah, I don't mind adding a global if you need it.
I'm not wedded to either approach, I'm just trying to find the least intrusive
option that uses the most existing code, and is the most maintainable into the
future.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-14 23:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-14 6:21 [PATCH v3] powerpc32: memset: only use dcbz once cache is enabled Christophe Leroy
2015-09-14 7:15 ` Michal Sojka
2015-09-14 15:20 ` Scott Wood
2015-09-14 15:44 ` Christophe LEROY
2015-09-14 16:13 ` Scott Wood
2015-09-14 23:52 ` Michael Ellerman
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).