From: Michal Sojka <hidden> Date: 2015-09-04 13:41:17
Dear Christophe,
my MPC5200-based system stopped booting recently. I bisected the problem
to your commit below. If I revert that commit (on top of
807249d3ada1ff28a47c4054ca4edd479421b671 = v4.2-6663-g807249d), my
system boots again.
commit 0b05e2d671c40cfb57e66e4e402320d6e056b2f8
Author: LEROY Christophe [off-list ref]
Date: Tue May 19 12:07:55 2015 +0200
powerpc/32: cacheable_memcpy becomes memcpy
cacheable_memcpy uses dcbz instruction and is more efficient than
memcpy when the destination is in RAM. If the destination is in an
io area, memcpy_toio() is normally used, not memcpy
This patch renames memcpy as generic_memcpy, and renames
cacheable_memcpy as memcpy
On MPC885, we get approximatly 7% increase of the transfer rate
on an FTP reception
Signed-off-by: Christophe Leroy [off-list ref]
Signed-off-by: Scott Wood [off-list ref]
Best regards,
-Michal Sojka
Dear Christophe,
my MPC5200-based system stopped booting recently. I bisected the problem
to your commit below. If I revert that commit (on top of
807249d3ada1ff28a47c4054ca4edd479421b671 = v4.2-6663-g807249d), my
system boots again.
Do you use mainline code only, or do you have home-made code ?
memcpy() is not supposed to be used on non-cacheable memory.
memcpy_toio() is the function to use when copying to non-cacheble area.
When I submitted the patch, I looked for erroneous use of memcpy() and
memset().
I found one wrong use of memset() that I changed to memset_io() but I
didn't find any misuse of memcpy().
But I may have missed one.
Christophe
From: Michal Sojka <hidden> Date: 2015-09-04 14:35:45
On Fri, Sep 04 2015, Christophe LEROY wrote:
Le 04/09/2015 15:33, Michal Sojka a écrit :
quoted
Dear Christophe,
my MPC5200-based system stopped booting recently. I bisected the problem
to your commit below. If I revert that commit (on top of
807249d3ada1ff28a47c4054ca4edd479421b671 = v4.2-6663-g807249d), my
system boots again.
Do you use mainline code only, or do you have home-made code ?
I use mainline only sources with non-mainline device-tree.
memcpy() is not supposed to be used on non-cacheable memory.
memcpy_toio() is the function to use when copying to non-cacheble area.
When I submitted the patch, I looked for erroneous use of memcpy() and
memset().
I found one wrong use of memset() that I changed to memset_io() but I
didn't find any misuse of memcpy().
But I may have missed one.
I attach my .config, if it helps. I have there
CONFIG_PPC_MPC52xx=y
CONFIG_PPC_MPC5200_SIMPLE=y
so arch/powerpc/platforms/52xx is probably the directory to look. Do you
see any mempcy misuse there?
Thanks,
-Michal
Dear Christophe,
my MPC5200-based system stopped booting recently. I bisected the problem
to your commit below. If I revert that commit (on top of
807249d3ada1ff28a47c4054ca4edd479421b671 = v4.2-6663-g807249d), my
system boots again.
Do you use mainline code only, or do you have home-made code ?
I use mainline only sources with non-mainline device-tree.
quoted
memcpy() is not supposed to be used on non-cacheable memory.
memcpy_toio() is the function to use when copying to non-cacheble area.
When I submitted the patch, I looked for erroneous use of memcpy() and
memset().
I found one wrong use of memset() that I changed to memset_io() but I
didn't find any misuse of memcpy().
But I may have missed one.
I attach my .config, if it helps. I have there
CONFIG_PPC_MPC52xx=y
CONFIG_PPC_MPC5200_SIMPLE=y
so arch/powerpc/platforms/52xx is probably the directory to look. Do you
see any mempcy misuse there?
I only found one suspect use of memcpy() in arch/powerpc/platforms/52xx/
It is in mpc52xx_pm.c but it's linked to CONFIG_PM which is not selected
by your .config
I'll check in the drivers selected by your .config
In parallele, are you able to try with CONFIG_PPC_EARLY_DEBUG in order
to try and locate the blocking point ?
Christophe
---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus
From: Michal Sojka <hidden> Date: 2015-09-04 19:49:44
On 4.9.2015 20:10, christophe leroy wrote:
Le 04/09/2015 16:35, Michal Sojka a écrit :
quoted
On Fri, Sep 04 2015, Christophe LEROY wrote:
quoted
Le 04/09/2015 15:33, Michal Sojka a écrit :
quoted
Dear Christophe,
my MPC5200-based system stopped booting recently. I bisected the
problem
to your commit below. If I revert that commit (on top of
807249d3ada1ff28a47c4054ca4edd479421b671 = v4.2-6663-g807249d), my
system boots again.
Do you use mainline code only, or do you have home-made code ?
I use mainline only sources with non-mainline device-tree.
quoted
memcpy() is not supposed to be used on non-cacheable memory.
memcpy_toio() is the function to use when copying to non-cacheble area.
When I submitted the patch, I looked for erroneous use of memcpy() and
memset().
I found one wrong use of memset() that I changed to memset_io() but I
didn't find any misuse of memcpy().
But I may have missed one.
I attach my .config, if it helps. I have there
CONFIG_PPC_MPC52xx=y
CONFIG_PPC_MPC5200_SIMPLE=y
so arch/powerpc/platforms/52xx is probably the directory to look. Do you
see any mempcy misuse there?
I only found one suspect use of memcpy() in arch/powerpc/platforms/52xx/
It is in mpc52xx_pm.c but it's linked to CONFIG_PM which is not
selected by your .config
I'll check in the drivers selected by your .config
In parallele, are you able to try with CONFIG_PPC_EARLY_DEBUG in order
to try and locate the blocking point ?
I don't get any output from the system even with CONFIG_PPC_EARLY_DEBUG.
-Michal
From: Michal Sojka <hidden> Date: 2015-09-05 00:08:46
On 4.9.2015 21:49, Michal Sojka wrote:
On 4.9.2015 20:10, christophe leroy wrote:
quoted
Le 04/09/2015 16:35, Michal Sojka a écrit :
quoted
On Fri, Sep 04 2015, Christophe LEROY wrote:
quoted
Le 04/09/2015 15:33, Michal Sojka a écrit :
quoted
Dear Christophe,
my MPC5200-based system stopped booting recently. I bisected the
problem
to your commit below. If I revert that commit (on top of
807249d3ada1ff28a47c4054ca4edd479421b671 = v4.2-6663-g807249d), my
system boots again.
Do you use mainline code only, or do you have home-made code ?
I use mainline only sources with non-mainline device-tree.
quoted
memcpy() is not supposed to be used on non-cacheable memory.
memcpy_toio() is the function to use when copying to non-cacheble
area.
When I submitted the patch, I looked for erroneous use of memcpy() and
memset().
I found one wrong use of memset() that I changed to memset_io() but I
didn't find any misuse of memcpy().
But I may have missed one.
I attach my .config, if it helps. I have there
CONFIG_PPC_MPC52xx=y
CONFIG_PPC_MPC5200_SIMPLE=y
so arch/powerpc/platforms/52xx is probably the directory to look. Do
you
see any mempcy misuse there?
I only found one suspect use of memcpy() in arch/powerpc/platforms/52xx/
It is in mpc52xx_pm.c but it's linked to CONFIG_PM which is not
selected by your .config
I'll check in the drivers selected by your .config
In parallele, are you able to try with CONFIG_PPC_EARLY_DEBUG in
order to try and locate the blocking point ?
I don't get any output from the system even with CONFIG_PPC_EARLY_DEBUG.
Hmm, there is no udbg console for MPC5200. I hacked something up and the
earliest place I was able to initialize it is after early_init_devtree()
in setup_32.c. Even with this console, I got no output when the
problematic patch was applied. So the problem is somewhere earlier.
-Michal
Dear Christophe,
my MPC5200-based system stopped booting recently. I bisected the
problem
to your commit below. If I revert that commit (on top of
807249d3ada1ff28a47c4054ca4edd479421b671 = v4.2-6663-g807249d), my
system boots again.
Do you use mainline code only, or do you have home-made code ?
I use mainline only sources with non-mainline device-tree.
quoted
memcpy() is not supposed to be used on non-cacheable memory.
memcpy_toio() is the function to use when copying to non-cacheble
area.
When I submitted the patch, I looked for erroneous use of memcpy()
and
memset().
I found one wrong use of memset() that I changed to memset_io() but I
didn't find any misuse of memcpy().
But I may have missed one.
I attach my .config, if it helps. I have there
CONFIG_PPC_MPC52xx=y
CONFIG_PPC_MPC5200_SIMPLE=y
so arch/powerpc/platforms/52xx is probably the directory to look.
Do you
see any mempcy misuse there?
I only found one suspect use of memcpy() in
arch/powerpc/platforms/52xx/
It is in mpc52xx_pm.c but it's linked to CONFIG_PM which is not
selected by your .config
I'll check in the drivers selected by your .config
In parallele, are you able to try with CONFIG_PPC_EARLY_DEBUG in
order to try and locate the blocking point ?
I don't get any output from the system even with CONFIG_PPC_EARLY_DEBUG.
Hmm, there is no udbg console for MPC5200. I hacked something up and
the earliest place I was able to initialize it is after
early_init_devtree() in setup_32.c. Even with this console, I got no
output when the problematic patch was applied. So the problem is
somewhere earlier.
In early_init() in setup_32.c, there is the following comment:
/* First zero the BSS -- use memset_io, some platforms don't have caches
on yet */
In that case, when does cache get activated ?
In move_device_tree(), called from early_init_devtree(), there is a call
to memcpy().
Can you try replacing it by memcpy_io() ?
Christophe
---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus
From: Michal Sojka <hidden> Date: 2015-09-06 19:05:26
On Sun, Sep 06 2015, christophe leroy wrote:
Le 05/09/2015 02:08, Michal Sojka a =C3=A9crit :
quoted
On 4.9.2015 21:49, Michal Sojka wrote:
quoted
On 4.9.2015 20:10, christophe leroy wrote:
quoted
Le 04/09/2015 16:35, Michal Sojka a =C3=A9crit :
quoted
On Fri, Sep 04 2015, Christophe LEROY wrote:
quoted
Le 04/09/2015 15:33, Michal Sojka a =C3=A9crit :
quoted
Dear Christophe,
my MPC5200-based system stopped booting recently. I bisected the=20
problem
to your commit below. If I revert that commit (on top of
807249d3ada1ff28a47c4054ca4edd479421b671 =3D v4.2-6663-g807249d), my
system boots again.
Do you use mainline code only, or do you have home-made code ?
I use mainline only sources with non-mainline device-tree.
quoted
memcpy() is not supposed to be used on non-cacheable memory.
memcpy_toio() is the function to use when copying to non-cacheble=20
area.
When I submitted the patch, I looked for erroneous use of memcpy()=20
and
memset().
I found one wrong use of memset() that I changed to memset_io() but I
didn't find any misuse of memcpy().
But I may have missed one.
I attach my .config, if it helps. I have there
CONFIG_PPC_MPC52xx=3Dy
CONFIG_PPC_MPC5200_SIMPLE=3Dy
so arch/powerpc/platforms/52xx is probably the directory to look.=20
Do you
see any mempcy misuse there?
I only found one suspect use of memcpy() in=20
arch/powerpc/platforms/52xx/
It is in mpc52xx_pm.c but it's linked to CONFIG_PM which is not=20
selected by your .config
I'll check in the drivers selected by your .config
In parallele, are you able to try with CONFIG_PPC_EARLY_DEBUG in=20
order to try and locate the blocking point ?
I don't get any output from the system even with CONFIG_PPC_EARLY_DEBUG.
Hmm, there is no udbg console for MPC5200. I hacked something up and=20
the earliest place I was able to initialize it is after=20
early_init_devtree() in setup_32.c. Even with this console, I got no=20
output when the problematic patch was applied. So the problem is=20
somewhere earlier.
In early_init() in setup_32.c, there is the following comment:
/* First zero the BSS -- use memset_io, some platforms don't have caches=
=20
on yet */
In that case, when does cache get activated ?
In move_device_tree(), called from early_init_devtree(), there is a call=
=20
to memcpy().
Can you try replacing it by memcpy_io() ?
I tried replacing it by memcpy_toio(), memcpy_fromio() and by
generic_memcpy(). Nothing helped :(
-Michal
From: Michal Sojka <hidden> Date: 2015-09-06 21:01:27
On Sun, Sep 06 2015, Michal Sojka wrote:
On Sun, Sep 06 2015, christophe leroy wrote:
quoted
Le 05/09/2015 02:08, Michal Sojka a =C3=A9crit :
quoted
On 4.9.2015 21:49, Michal Sojka wrote:
quoted
On 4.9.2015 20:10, christophe leroy wrote:
quoted
Le 04/09/2015 16:35, Michal Sojka a =C3=A9crit :
quoted
On Fri, Sep 04 2015, Christophe LEROY wrote:
quoted
Le 04/09/2015 15:33, Michal Sojka a =C3=A9crit :
quoted
Dear Christophe,
my MPC5200-based system stopped booting recently. I bisected the=20
problem
to your commit below. If I revert that commit (on top of
807249d3ada1ff28a47c4054ca4edd479421b671 =3D v4.2-6663-g807249d), =
my
quoted
quoted
quoted
quoted
quoted
quoted
quoted
system boots again.
Do you use mainline code only, or do you have home-made code ?
I use mainline only sources with non-mainline device-tree.
quoted
memcpy() is not supposed to be used on non-cacheable memory.
memcpy_toio() is the function to use when copying to non-cacheble=20
area.
When I submitted the patch, I looked for erroneous use of memcpy()=
=20
quoted
quoted
quoted
quoted
quoted
quoted
and
memset().
I found one wrong use of memset() that I changed to memset_io() but=
I
quoted
quoted
quoted
quoted
quoted
quoted
didn't find any misuse of memcpy().
But I may have missed one.
I attach my .config, if it helps. I have there
CONFIG_PPC_MPC52xx=3Dy
CONFIG_PPC_MPC5200_SIMPLE=3Dy
so arch/powerpc/platforms/52xx is probably the directory to look.=20
Do you
see any mempcy misuse there?
I only found one suspect use of memcpy() in=20
arch/powerpc/platforms/52xx/
It is in mpc52xx_pm.c but it's linked to CONFIG_PM which is not=20
selected by your .config
I'll check in the drivers selected by your .config
In parallele, are you able to try with CONFIG_PPC_EARLY_DEBUG in=20
order to try and locate the blocking point ?
I don't get any output from the system even with CONFIG_PPC_EARLY_DEBU=
G.
quoted
quoted
Hmm, there is no udbg console for MPC5200. I hacked something up and=20
the earliest place I was able to initialize it is after=20
early_init_devtree() in setup_32.c. Even with this console, I got no=20
output when the problematic patch was applied. So the problem is=20
somewhere earlier.
In early_init() in setup_32.c, there is the following comment:
/* First zero the BSS -- use memset_io, some platforms don't have caches=
=20
quoted
on yet */
In that case, when does cache get activated ?
In move_device_tree(), called from early_init_devtree(), there is a call=
=20
quoted
to memcpy().
Can you try replacing it by memcpy_io() ?
I tried replacing it by memcpy_toio(), memcpy_fromio() and by
generic_memcpy(). Nothing helped :(
I found the problem. The compiler replaces an assignment with a call to
memcpy. The following patch fixes the problem for me. However, I'm not
sure whether this is the real solution. I guess the compiler is free to
generate a call to memcpy wherever it wants so other compilers or other
optimization levels may need fixes at other places. What do others
think?
-Michal
---------------8<----------------------
From: Michal Sojka <redacted>
Date: Sun, 6 Sep 2015 22:44:55 +0200
Subject: [PATCH] powerpc: Fix unbootable system after memcpy optimization
This fixes the problem caused by commit
0b05e2d671c40cfb57e66e4e402320d6e056b2f8. On MPC5200, when using the
following version of GCC:
powerpc-603e-linux-gnu-gcc (OSELAS.Toolchain-2012.12.1) 4.7.2
the system was ubootable.
Signed-off-by: Michal Sojka <redacted>
---
arch/powerpc/kernel/cputable.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ed long offset,
old =3D *t;
=20
/* Copy everything, then do fixups */
- *t =3D *s;
+ memcpy_toio(t, s, sizeof(struct cpu_spec));
=20
/*
* If we are overriding a previous value derived from the real
--=20
2.5.0
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-09-07 01:14:10
Hi Michal,
Thanks for finding the problem.
On Sun, 2015-09-06 at 23:01 +0200, Michal Sojka wrote:
I found the problem. The compiler replaces an assignment with a call to
memcpy. The following patch fixes the problem for me. However, I'm not
sure whether this is the real solution. I guess the compiler is free to
generate a call to memcpy wherever it wants so other compilers or other
optimization levels may need fixes at other places. What do others
think?
I think you're right that it's not a good solution, the compiler could generate
other calls to memcpy depending on various factors, and people will add new
code that causes memcpy to get called and it will break your platform.
Christophe, am I right that the problem here is that your new memcpy() doesn't
work until later in boot when caches are enabled?
cheers
Hi Michael
Le 07/09/2015 03:14, Michael Ellerman a écrit :
Hi Michal,
Thanks for finding the problem.
On Sun, 2015-09-06 at 23:01 +0200, Michal Sojka wrote:
quoted
I found the problem. The compiler replaces an assignment with a call to
memcpy. The following patch fixes the problem for me. However, I'm not
sure whether this is the real solution. I guess the compiler is free to
generate a call to memcpy wherever it wants so other compilers or other
optimization levels may need fixes at other places. What do others
think?
I think you're right that it's not a good solution, the compiler could generate
other calls to memcpy depending on various factors, and people will add new
code that causes memcpy to get called and it will break your platform.
Christophe, am I right that the problem here is that your new memcpy() doesn't
work until later in boot when caches are enabled?
That's right, memset() and memcpy() are for setting/copying data into
cacheable RAM.
They are using dczb instruction in order to avoid wasting time loading
the cacheline with data that will be overwritten.
memset_io() and memcpy_toio() are the functions to use when using not
cacheable memory.
The issue identified by Michal is in function setup_cpu_spec() which is
called by identify_cpu(). identify_cpu() is called from early_init().
In the begining of early_init(), there is (code from Paul in 2005)
/* First zero the BSS -- use memset_io, some platforms don't have
* caches on yet */
memset_io((void __iomem *)PTRRELOC(&__bss_start), 0,
__bss_stop - __bss_start);
It shows that it is already expected that the cache is not active yet
and standard memset() shall not be used yet. That's the same with memcpy().
I think GCC uses memcpy() in well known situations like initialising
structures or copying structures.
Shouldn't we just avoid this kind of actions in the very few early init
functions ?
Christophe
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-09-07 08:40:50
On Mon, 2015-09-07 at 09:08 +0200, Christophe LEROY wrote:
Hi Michael
Le 07/09/2015 03:14, Michael Ellerman a écrit :
quoted
On Sun, 2015-09-06 at 23:01 +0200, Michal Sojka wrote:
quoted
I found the problem. The compiler replaces an assignment with a call to
memcpy. The following patch fixes the problem for me. However, I'm not
sure whether this is the real solution. I guess the compiler is free to
generate a call to memcpy wherever it wants so other compilers or other
optimization levels may need fixes at other places. What do others
think?
I think you're right that it's not a good solution, the compiler could generate
other calls to memcpy depending on various factors, and people will add new
code that causes memcpy to get called and it will break your platform.
Christophe, am I right that the problem here is that your new memcpy() doesn't
work until later in boot when caches are enabled?
That's right, memset() and memcpy() are for setting/copying data into
cacheable RAM.
They are using dczb instruction in order to avoid wasting time loading
the cacheline with data that will be overwritten.
memset_io() and memcpy_toio() are the functions to use when using not
cacheable memory.
The issue identified by Michal is in function setup_cpu_spec() which is
called by identify_cpu(). identify_cpu() is called from early_init().
In the begining of early_init(), there is (code from Paul in 2005)
/* First zero the BSS -- use memset_io, some platforms don't have
* caches on yet */
memset_io((void __iomem *)PTRRELOC(&__bss_start), 0,
__bss_stop - __bss_start);
It shows that it is already expected that the cache is not active yet
and standard memset() shall not be used yet. That's the same with memcpy().
Thanks for the explanation.
I think GCC uses memcpy() in well known situations like initialising
structures or copying structures.
Shouldn't we just avoid this kind of actions in the very few early init
functions ?
Which are the "very few" early init functions? Can you make a list, for 32-bit
and 64-bit? And can we keep it updated over time and not introduce regressions?
cheers
From: Michal Sojka <hidden> Date: 2015-09-07 09:46:02
On 7.9.2015 10:40, Michael Ellerman wrote:
On Mon, 2015-09-07 at 09:08 +0200, Christophe LEROY wrote:
quoted
Hi Michael
Le 07/09/2015 03:14, Michael Ellerman a écrit :
quoted
On Sun, 2015-09-06 at 23:01 +0200, Michal Sojka wrote:
quoted
I found the problem. The compiler replaces an assignment with a call to
memcpy. The following patch fixes the problem for me. However, I'm not
sure whether this is the real solution. I guess the compiler is free to
generate a call to memcpy wherever it wants so other compilers or other
optimization levels may need fixes at other places. What do others
think?
I think you're right that it's not a good solution, the compiler could generate
other calls to memcpy depending on various factors, and people will add new
code that causes memcpy to get called and it will break your platform.
Christophe, am I right that the problem here is that your new memcpy() doesn't
work until later in boot when caches are enabled?
That's right, memset() and memcpy() are for setting/copying data into
cacheable RAM.
They are using dczb instruction in order to avoid wasting time loading
the cacheline with data that will be overwritten.
memset_io() and memcpy_toio() are the functions to use when using not
cacheable memory.
The issue identified by Michal is in function setup_cpu_spec() which is
called by identify_cpu(). identify_cpu() is called from early_init().
In the begining of early_init(), there is (code from Paul in 2005)
/* First zero the BSS -- use memset_io, some platforms don't have
* caches on yet */
memset_io((void __iomem *)PTRRELOC(&__bss_start), 0,
__bss_stop - __bss_start);
It shows that it is already expected that the cache is not active yet
and standard memset() shall not be used yet. That's the same with memcpy().
Thanks for the explanation.
quoted
I think GCC uses memcpy() in well known situations like initialising
structures or copying structures.
Shouldn't we just avoid this kind of actions in the very few early init
functions ?
Which are the "very few" early init functions? Can you make a list, for 32-bit
and 64-bit? And can we keep it updated over time and not introduce regressions?
If the code that runs without caches is concentrated in few files, we
may either modify the buildsystem to check whether there is a call to
memcpy from these files (e.g. by using nm) or these files can be
"prelinked" with special version of memcpy that doesn't require caches.
Would any of these be acceptable?
-Michal
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-09-08 03:54:22
On Mon, 2015-09-07 at 10:59 +0000, David Laight wrote:
From: Michal Sojka
quoted
quoted
quoted
I think GCC uses memcpy() in well known situations like initialising
structures or copying structures.
Shouldn't we just avoid this kind of actions in the very few early init
functions ?
Which are the "very few" early init functions? Can you make a list, for 32-bit
and 64-bit? And can we keep it updated over time and not introduce regressions?
If the code that runs without caches is concentrated in few files, we
may either modify the buildsystem to check whether there is a call to
memcpy from these files (e.g. by using nm) or these files can be
"prelinked" with special version of memcpy that doesn't require caches.
Would any of these be acceptable?
What about run-time patching memcpy() after the caches are initialised?
Yeah, that's the solution we use on 64-bit.
It also means you can have cpu specific optimisations, which can be patched in
or out using the cpu feature patching.
cheers