Re: powerpc32: fix warning from include/linux/mm.h

[leroy christophe]

Le 21/03/2015 00:52, Scott Wood a écrit :

On Fri, Dec 05, 2014 at 12:20:20PM +0100, LEROY Christophe wrote:

include/linux/mm.h: In function 'is_vmalloc_addr':
include/linux/mm.h:367:14: warning: comparison between signed and unsigned 
integer expressions [-Wsign-compare]
   return addr >= VMALLOC_START && addr < VMALLOC_END;
   ^

Signed-off-by: Christophe Leroy 
---
  arch/powerpc/include/asm/pgtable-ppc32.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

That warning doesn't appear to be enabled.  What config are you seeing
this with?
I'm used to adding EXTRA_CFLAGS=-Wextra  when checking my own drivers, 
as it helps finding additional bugs.


When doing that, the only warnings I get outside of my own code are this 
one, and the other one in my proposed patch identified "powerpc32: fix 
warning from include/asm-generic/termios-base.h"


I though it would be worth fixing those two warnings in order to get a 
perfectly clean code.


Christophe

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 11/11] powerpc/8xx: Add support for TASK_SIZE greater than 0x80000000

[leroy christophe]

Le 21/03/2015 01:47, Scott Wood a écrit :

On Tue, 2015-01-20 at 10:57 +0100, Christophe Leroy wrote:

By default, TASK_SIZE is set to 0x8000 for PPC_8xx, which is most likely
sufficient for most cases. However, kernel configuration allows to set TASK_SIZE
to another value, so the 8xx shall handle it.

Signed-off-by: Christophe Leroy 

---
v2: no change

  arch/powerpc/kernel/head_8xx.S | 25 +++--
  1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index ead6448..92a6456 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -48,6 +48,19 @@
mtspr   spr, reg
  #endif
  
+/* Macro to test if an address is a kernel address */

+#if CONFIG_TASK_SIZE <= 0x8000
+#define IS_KERNEL(tmp, addr)   \
+   andis.  tmp, addr, 0x8000   /* Address >= 0x8000 */
+#define BRANCH_UNLESS_KERNEL(label)beq label
+#else

This works if CONFIG_TASK_SIZE == 0x8000, but what if it's less, and
you have a kernel address < 0x8000?


You are right, I didn't realise that PAGE_OFFSET was also configurable 
and could be different from 0xC000.
andis. (which is what is always used in the current kernel) can be used 
if CONFIG_TASK_SIZE <= 0x8000 AND PAGE_OFFSET >= 0x8000.

I will resubmit the patch accordingly.

Christophe

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 0/8] powerpc/8xx: Getting rid of CONFIG_8xx

[leroy christophe]

Le 25/03/2015 01:45, Scott Wood a écrit :

On Fri, 2015-03-13 at 10:34 +1100, Michael Ellerman wrote:

On Thu, 2015-03-12 at 16:24 +0100, Christophe Leroy wrote:

Two config options exist to define powerpc MPC8xx:
* CONFIG_PPC_8xx
* CONFIG_8xx
In addition, CONFIG_PPC_8xx also defines CONFIG_CPM1 as
communication co-processor

arch/powerpc/platforms/Kconfig.cputype has contained the following
comment about CONFIG_8xx item for some years:
"# this is temp to handle compat with arch=ppc"

It looks like not many places still have that old CONFIG_8xx used,
so it is likely to be a good time to get rid of it completely ?

Patchset is composed of the following patches:
[1/8] powerpc: replace CONFIG_8xx by CONFIG_PPC_8xx
[2/8] um: replace CONFIG_8xx by CONFIG_PPC_8xx
[3/8] video: replace CONFIG_8xx by CONFIG_PPC_8xx
[4/8] net: freescale: replace CONFIG_8xx by CONFIG_PPC_8xx
[5/8] tty: cpm_uart: replace CONFIG_8xx by CONFIG_CPM1
[6/8] mtd: replace CONFIG_8xx by CONFIG_PPC_8xx
[7/8] isdn: replace CONFIG_8xx by CONFIG_PPC_8xx
[8/8] powerpc: get rid of CONFIG_8xx

All but the last one are independant and can be applied in any
order. Only the 8th one requires the first 7 patches to be applied.

Sounds good.

I only see 0, 4 and 7 on linuxppc-dev for some reason.

I see them all in linuxppc-dev patchwork.


You'll need to collect ACKs, or get the individual patches merged, and then we
can take patch 8 through the powerpc tree once those are all in - probably for
4.2.

It looks like CONFIG_8xx is used a lot more than CONFIG_PPC_8xx, so it
would be less churn to get rid of the latter (plus, we also have
CONFIG_4xx, CONFIG_6xx, etc).  The only use of PPC_8xx I see outside
arch/powerpc is in drivers/watchdog/Kconfig.


Ok, we can do that. But when outside of arch/powerpc/, isn't is more 
explicit with CONFIG_PPC_8xx rather that CONFIG_8xx ?


Now that I have submitted the first set of patch, don't we have a risk 
that now it is already merged by some other maintainers ?


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v3, 01/11] powerpc/8xx: remove remaining unnecessary code in FixupDAR

[leroy christophe]

Le 26/03/2015 22:32, Scott Wood a écrit :

On Tue, Feb 03, 2015 at 12:38:16PM +0100, LEROY Christophe wrote:

Since commit 33fb845a6f01 ("powerpc/8xx: Don't use MD_TWC for walk"), MD_EPN and
MD_TWC are not writen anymore in FixupDAR so saving r3 has become useless.

Signed-off-by: Christophe Leroy 
---
v2: no change
v3: no change

This doesn't apply cleanly.


You already applied part of that patchset it in your next tree, 
including that one (commit 2374d0a).
You told me to re-submit a patchset with only the remaining ones, 
therefore I sent v4 on the 4th of Feb, based on your tree.


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v3, 01/11] powerpc/8xx: remove remaining unnecessary code in FixupDAR

[leroy christophe]

Le 13/04/2015 22:26, Scott Wood a écrit :

On Sun, 2015-04-12 at 18:16 +0200, leroy christophe wrote:

Le 26/03/2015 22:32, Scott Wood a écrit :

On Tue, Feb 03, 2015 at 12:38:16PM +0100, LEROY Christophe wrote:

Since commit 33fb845a6f01 ("powerpc/8xx: Don't use MD_TWC for walk"), MD_EPN and
MD_TWC are not writen anymore in FixupDAR so saving r3 has become useless.

Signed-off-by: Christophe Leroy 
---
v2: no change
v3: no change

This doesn't apply cleanly.



You already applied part of that patchset it in your next tree,
including that one (commit 2374d0a).
You told me to re-submit a patchset with only the remaining ones,
therefore I sent v4 on the 4th of Feb, based on your tree.

OK.  I applied v2, and didn't remember that when I came across v3 in
patchwork.


What about v4 (the remaining ones) ? You got comments on the last one of 
the set, have you applied the other ones or shall I re-sumbit a full v5 ?


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 00/17] crypto: talitos - Add support for SEC1

[leroy christophe]

Oops, this is the first time I use directly the output of git 
format-patch into sendmail, and

it looks like the mails are dated with the commit date, not today's date.

I will resend now with today's date. Sorry for the noise.

Christophe

Le 17/04/2015 15:47, Christophe Leroy a écrit :

The purpose of this set of patchs is to add to talitos crypto driver
the support for the SEC1 version of the security engine, which is
found in mpc885 and mpc8272 processors.

v3 is a complete rework of the patchset. Since a kernel can be built
with support for both MPC82xx and MPC83xx at the same time, talitos
driver shall support both SEC1 and SEC2+ at the same time.

Based on cryptodev-2.6 tree

Christophe Leroy (17):
   crypto: talitos - Use zero entry to init descriptors ptrs to zero
   crypto: talitos - Refactor the sg in/out chain allocation
   crypto: talitos - talitos_ptr renamed ptr for more lisibility
   crypto: talitos - Add a helper function to clear j_extent field
   crypto: talitos - remove param 'extent' in map_single_talitos_ptr()
   crypto: talitos - helper function for ptr len
   crypto: talitos - enhanced talitos_desc struct for SEC1
   crypto: talitos - add sub-choice in talitos CONFIG for SEC1
   crypto: talitos - Add a feature to tag SEC1
   crypto: talitos - fill in talitos descriptor iaw SEC1 or SEC2+
   crypto: talitos - adaptation of talitos_submit() for SEC1
   crypto: talitos - base address for Execution Units
   crypto: talitos - adapt interrupts and reset functions to SEC1
   crypto: talitos - implement scatter/gather copy for SEC1
   crypto: talitos - SEC1 bugs on 0 data hash
   crypto: talitos - Add fsl,sec1.0 compatible
   crypto: talitos - Update DT bindings with SEC1

  .../devicetree/bindings/crypto/fsl-sec2.txt|   6 +-
  drivers/crypto/Kconfig |  18 +
  drivers/crypto/talitos.c   | 727 +++--
  drivers/crypto/talitos.h   | 153 +++--
  4 files changed, 644 insertions(+), 260 deletions(-)



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 03/17] crypto: talitos - talitos_ptr renamed ptr for more lisibility

[leroy christophe]

Le 17/04/2015 17:14, David Laight a écrit :

From: Christophe Leroy

Linux CodyingStyle recommends to use short variables for local
variables. ptr is just good enough for those 3 lines functions.
It helps keep single lines shorter than 80 characters.

...

-static void to_talitos_ptr(struct talitos_ptr *talitos_ptr, dma_addr_t 
dma_addr)
+static void to_talitos_ptr(struct talitos_ptr *ptr, dma_addr_t dma_addr)
  {
-   talitos_ptr->ptr = cpu_to_be32(lower_32_bits(dma_addr));
-   talitos_ptr->eptr = upper_32_bits(dma_addr);
+   ptr->ptr = cpu_to_be32(lower_32_bits(dma_addr));
+   ptr->eptr = upper_32_bits(dma_addr);
  }

...

Maybe, but 'ptr' isn't a good choice.



Any suggestion ?

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 00/11] powerpc8xx: Further optimisation of TLB handling

[leroy christophe]

Le 20/04/2015 07:26, Christophe Leroy a écrit :

This patchset provides a further optimisation of TLB handling in the 8xx.
Main changes are based on:
- Using processor handling of PGD/PTE Validity bits instead of testing ourselves
the entries validity
- Aligning PGD address to allow direct bit manipulation
- Not saving registers like CR when not needed

It also adds support to any TASK_SIZE

Patchset:
01 - powerpc/8xx: remove remaining unnecessary code in FixupDAR
02 - powerpc/8xx: remove tests on PGDIR entry validity
03 - powerpc32: Use kmem_cache memory for PGDIR
04 - powerpc/8xx: Take benefit of aligned PGDIR
05 - powerpc/8xx: Optimise access to swapper_pg_dir
06 - powerpc/8xx: Remove duplicated code in set_context()
07 - powerpc/8xx: macro for handling CPU15 errata
08 - powerpc/8xx: Handle CR out of exception PROLOG/EPILOG
09 - powerpc/8xx: dont save CR in SCRATCH registers
10 - powerpc/8xx: Use SPRG2 instead of DAR for saving r3
11 - powerpc/8xx: Add support for TASK_SIZE greater than 0x8000

All changes have been successfully tested on MPC885

Signed-off-by: Christophe Leroy 
Tested-by: Christophe Leroy 
Sorry, forget than one, my finger slipped and I resent a very old one 
already applied


Christophe


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 07/11] powerpc/8xx: macro for handling CPU15 errata

[leroy christophe]

Le 20/04/2015 13:40, David Laight a écrit :

From: Christophe Leroy

Sent: 20 April 2015 06:27
Having a macro will help keep clear code.

...

   * We have to use the MD_xxx registers for the tablewalk because the
   * equivalent MI_xxx registers only perform the attribute functions.
   */
+
+#ifdef CONFIG_8xx_CPU15
+#define DO_8xx_CPU15(tmp, addr)\
+   additmp, addr, PAGE_SIZE;   \
+   tlbie   tmp;\
+   additmp, addr, PAGE_SIZE;   \
+   tlbie   tmp
+#else
+#define DO_8xx_CPU15(tmp, addr)
+#endif

I'm sure I've spotted the same obvious error in the above before.

I'd also suggest calling it 'invalidate_adjacent_pages' - since that it
what it does.

I also guess that the execution time of 'tlbie' is non-trivial.
So you might as well get rid of the temporary register and put an
'addi' to reset 'addr' at the end.

David



Forget it, I did a big mistake this morning, involontarily resent an old 
patch.

Sorry for the noise.

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v2,2/2] powerpc32: add support for csum_add()

[leroy christophe]

Le 05/05/2015 00:10, Segher Boessenkool a écrit :

On Fri, May 01, 2015 at 08:00:14PM -0500, Scott Wood wrote:

On Tue, 2015-04-28 at 21:01 +0200, christophe leroy wrote:

The generated code is most likely different on ppc64. I have no ppc64
compiler

For reference: yes you do.  Just add -m64.

[root@localhost knl]# LANG= ppc-linux-gcc -m64 test.c
test.c:1:0: error: -m64 not supported in this configuration

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: Linux 3.16: all my drivers on SPI bus report WARNING: at drivers/base/dd.c:286

[leroy christophe]

I've been able to identify the origin of the issue. It happens since the below 
commit.
Do you know what should be done to fix that ?

Christophe


From 7a40054361162d2f78f332aa868fed137beb7901 Mon Sep 17 00:00:00 2001
From: Axel Lin 
Date: Sun, 30 Mar 2014 16:42:57 +0800
Subject: spi: fsl-spi: Fix memory leak

mpc8xxx_spi_probe() has set master->cleanup = mpc8xxx_spi_cleanup,
however current code overrides the setting in fsl_spi_probe() and set
master->cleanup = fsl_spi_cleanup.
Thus the memory allocated for cs is not freed anywhere.
Convert to use devm_kzalloc to fix the memory leak.

Signed-off-by: Axel Lin 
Signed-off-by: Mark Brown 

diff --git a/drivers/spi/spi-fsl-spi.c b/drivers/spi/spi-fsl-spi.c
index b3e7775..98ccd23 100644
--- a/drivers/spi/spi-fsl-spi.c
+++ b/drivers/spi/spi-fsl-spi.c
@@ -431,7 +431,7 @@ static int fsl_spi_setup(struct spi_device *spi)
return -EINVAL;
 
 	if (!cs) {

-   cs = kzalloc(sizeof *cs, GFP_KERNEL);
+   cs = devm_kzalloc(&spi->dev, sizeof(*cs), GFP_KERNEL);
if (!cs)
return -ENOMEM;
spi->controller_state = cs;
--
cgit v0.10.1


Le 19/08/2014 11:21, leroy christophe a écrit :
Since Linux 3.16, for all drivers tied to SPI bus, I get the following 
warning on a PowerPC 8xx.

It doesn't happen with Linux 3.15

What can be the reason / what should I look at ?

[3.086957] device: 'spi32766.1': device_add
[3.087179] bus: 'spi': add device spi32766.1
[3.087653] bus: 'spi': driver_probe_device: matched device 
spi32766.1 with driver lm70
[3.087743] bus: 'spi': really_probe: probing driver lm70 with 
device spi32766.1

[3.088014] [ cut here ]
[3.092348] WARNING: at drivers/base/dd.c:286
[3.096637] Modules linked in:
[3.099697] CPU: 0 PID: 25 Comm: kworker/u2:1 Tainted: G W 
3.16.1-s3k-drv-999-svn5771_knld-999 #158

[3.109610] Workqueue: deferwq deferred_probe_work_func
[3.114736] task: c787f020 ti: c790c000 task.ti: c790c000
[3.120062] NIP: c01df158 LR: c01df144 CTR: 
[3.124983] REGS: c790db30 TRAP: 0700   Tainted: GW 
(3.16.1-s3k-drv-999-svn5771_knld-999)

[3.134162] MSR: 00029032   CR: 22002082 XER: 2000
[3.140703]
[3.140703] GPR00: 0001 c790dbe0 c787f020 0044 0054 
0308 c056da0e 20737069
[3.140703] GPR08: 33323736 000ebfe0 0308 000ebfdf 22002082 
 c046c5a0 c046c608
[3.140703] GPR16: c046c614 c046c620 c046c62c c046c638 c046c648 
c046c654 c046c68c c046c6c4
[3.140703] GPR24:   0003 c0401aa0 c0596638 
c059662c c054e7a8 c7996800

[3.170102] NIP [c01df158] driver_probe_device+0xf8/0x334
[3.175431] LR [c01df144] driver_probe_device+0xe4/0x334
[3.180633] Call Trace:
[3.183093] [c790dbe0] [c01df144] driver_probe_device+0xe4/0x334 
(unreliable)

[3.190147] [c790dc10] [c01dd15c] bus_for_each_drv+0x7c/0xc0
[3.195741] [c790dc40] [c01df5fc] device_attach+0xcc/0xf8
[3.201076] [c790dc60] [c01dd6d4] bus_probe_device+0xb4/0xc4
[3.20] [c790dc80] [c01db9f8] device_add+0x270/0x564
[3.211923] [c790dcc0] [c0219e84] spi_add_device+0xc0/0x190
[3.217427] [c790dce0] [c021a79c] spi_register_master+0x720/0x834
[3.223455] [c790dd40] [c021cb48] of_fsl_spi_probe+0x55c/0x614
[3.229234] [c790dda0] [c01e0d2c] platform_drv_probe+0x30/0x74
[3.234987] [c790ddb0] [c01df18c] driver_probe_device+0x12c/0x334
[3.241008] [c790dde0] [c01dd15c] bus_for_each_drv+0x7c/0xc0
[3.246602] [c790de10] [c01df5fc] device_attach+0xcc/0xf8
[3.251937] [c790de30] [c01dd6d4] bus_probe_device+0xb4/0xc4
[3.257536] [c790de50] [c01de9d8] deferred_probe_work_func+0x98/0xe0
[3.263816] [c790de70] [c00305b8] process_one_work+0x18c/0x440
[3.269577] [c790dea0] [c0030a00] worker_thread+0x194/0x67c
[3.275105] [c790def0] [c0039198] kthread+0xd0/0xe4
[3.279911] [c790df40] [c000c6d0] ret_from_kernel_thread+0x5c/0x64
[3.285970] Instruction dump:
[3.288900] 80de 419e01d0 3b7b0038 3c60c046 7f65db78 38635264 
48211b99 813f00a0
[3.296559] 381f00a0 7d290278 3169 7c0b4910 <0f00> 93df0044 
7fe3fb78 4bfffd4d

[3.304401] ---[ end trace c75d3461bf9e2961 ]---
[3.309598] device: 'hwmon1': device_add
[3.310246] driver: 'lm70': driver_bound: bound to device 'spi32766.1'
[3.310342] bus: 'spi': really_probe: bound device spi32766.1 to 
driver lm70


Christophe


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] spi: fsl: Don't use devm_kzalloc in master->setup callback

[leroy christophe]

Le 31/08/2014 06:44, Axel Lin a écrit :

device_add() expects that any memory allocated via devm_* API is only
done in the device's probe function.

Fix below boot warning:
[3.092348] WARNING: at drivers/base/dd.c:286
[3.096637] Modules linked in:
[3.099697] CPU: 0 PID: 25 Comm: kworker/u2:1 Tainted: G W 
3.16.1-s3k-drv-999-svn5771_knld-999 #158
[ 3.109610] Workqueue: deferwq deferred_probe_work_func
[3.114736] task: c787f020 ti: c790c000 task.ti: c790c000
[3.120062] NIP: c01df158 LR: c01df144 CTR: 
[3.124983] REGS: c790db30 TRAP: 0700   Tainted: GW 
(3.16.1-s3k-drv-999-svn5771_knld-999)
[3.134162] MSR: 00029032   CR: 22002082 XER: 2000
[3.140703]
[3.140703] GPR00: 0001 c790dbe0 c787f020 0044 0054 0308 
c056da0e 20737069
[3.140703] GPR08: 33323736 000ebfe0 0308 000ebfdf 22002082  
c046c5a0 c046c608
[3.140703] GPR16: c046c614 c046c620 c046c62c c046c638 c046c648 c046c654 
c046c68c c046c6c4
[3.140703] GPR24:   0003 c0401aa0 c0596638 c059662c 
c054e7a8 c7996800
[3.170102] NIP [c01df158] driver_probe_device+0xf8/0x334
[3.175431] LR [c01df144] driver_probe_device+0xe4/0x334
[3.180633] Call Trace:
[3.183093] [c790dbe0] [c01df144] driver_probe_device+0xe4/0x334 (unreliable)
[3.190147] [c790dc10] [c01dd15c] bus_for_each_drv+0x7c/0xc0
[3.195741] [c790dc40] [c01df5fc] device_attach+0xcc/0xf8
[3.201076] [c790dc60] [c01dd6d4] bus_probe_device+0xb4/0xc4
[3.20] [c790dc80] [c01db9f8] device_add+0x270/0x564
[3.211923] [c790dcc0] [c0219e84] spi_add_device+0xc0/0x190
[3.217427] [c790dce0] [c021a79c] spi_register_master+0x720/0x834
[3.223455] [c790dd40] [c021cb48] of_fsl_spi_probe+0x55c/0x614
[3.229234] [c790dda0] [c01e0d2c] platform_drv_probe+0x30/0x74
[3.234987] [c790ddb0] [c01df18c] driver_probe_device+0x12c/0x334
[3.241008] [c790dde0] [c01dd15c] bus_for_each_drv+0x7c/0xc0
[3.246602] [c790de10] [c01df5fc] device_attach+0xcc/0xf8
[3.251937] [c790de30] [c01dd6d4] bus_probe_device+0xb4/0xc4
[3.257536] [c790de50] [c01de9d8] deferred_probe_work_func+0x98/0xe0
[3.263816] [c790de70] [c00305b8] process_one_work+0x18c/0x440
[3.269577] [c790dea0] [c0030a00] worker_thread+0x194/0x67c
[3.275105] [c790def0] [c0039198] kthread+0xd0/0xe4
[3.279911] [c790df40] [c000c6d0] ret_from_kernel_thread+0x5c/0x64
[3.285970] Instruction dump:
[3.288900] 80de 419e01d0 3b7b0038 3c60c046 7f65db78 38635264 48211b99 
813f00a0
[3.296559] 381f00a0 7d290278 3169 7c0b4910 <0f00> 93df0044 7fe3fb78 
4bfffd4d

Reported-by: leroy christophe 
Signed-off-by: Axel Lin 
---
Hi Leroy,
   Can you test this path?
Thanks,
Axel
  drivers/spi/spi-fsl-espi.c | 15 ---
  drivers/spi/spi-fsl-spi.c  | 10 +++---
  2 files changed, 19 insertions(+), 6 deletions(-)



Tested-by: Christophe Leroy 

Thanks for this patch.
SPI still works ok, no warning anymore. Tested on MPC8xx (spi-fsl-spi).
Not tested the removal of drivers (I don't use modules)

Christophe
(NB: Leroy is my family name)
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 03/21] powerpc/8xx: exception InstructionAccess does not exist on MPC8xx

[leroy christophe]

Le 18/09/2014 17:15, Joakim Tjernlund a écrit :

Christophe Leroy  wrote on 2014/09/17 18:36:57:

Exception InstructionAccess does not exist on MPC8xx. No need to branch

there from somewhere else.

Handling can be done directly in InstructionTLBError Exception.

Signed-off-by: Christophe Leroy 

---
Changes in v2:
- None

Changes in v3:
- arch/powerpc/mm/fault.c uses the vector number, so make sure it

understand

the new ones.

  arch/powerpc/kernel/head_8xx.S | 17 +++--
  arch/powerpc/mm/fault.c|  1 +
  2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/head_8xx.S

b/arch/powerpc/kernel/head_8xx.S

index 3af6db1..fbe5d10 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -234,15 +234,9 @@ DataAccess:
 EXC_XFER_LITE(0x300, handle_page_fault)

  /* Instruction access exception.
- * This is "never generated" by the MPC8xx.  We jump to it for other
- * translation errors.
+ * This is "never generated" by the MPC8xx.
   */
-   . = 0x400
-InstructionAccess:
-   EXCEPTION_PROLOG
-   mr   r4,r12
-   mr   r5,r9
-   EXC_XFER_LITE(0x400, handle_page_fault)
+   EXCEPTION(0x400, InstructionAccess, unknown_exception, EXC_XFER_STD)

  /* External interrupt */
 EXCEPTION(0x500, HardwareInterrupt, do_IRQ, EXC_XFER_LITE)
@@ -382,7 +376,7 @@ InstructionTLBMiss:
  #endif
 mfspr   r10, SPRN_SPRG_SCRATCH2
 EXCEPTION_EPILOG_0
-   b   InstructionAccess
+   b   InstructionTLBError

 . = 0x1200
  DataStoreTLBMiss:
@@ -477,7 +471,10 @@ DataStoreTLBMiss:
   */
 . = 0x1300
  InstructionTLBError:
-   b   InstructionAccess
+   EXCEPTION_PROLOG
+   mr   r4,r12
+   mr   r5,r9
+   EXC_XFER_LITE(0x1300, handle_page_fault)

Still don't like that you change the vector number, every other ppc uses
the
standard number.

Can you not just lie here(EXC_XFER_LITE(0x400, handle_page_fault))?
Move the code to InstructionAccess too and let TLBError branch there.
My issue was that if I do EXC_XFER_LITE(0x400, handle_page_fault), I 
can't leave the
EXCEPTION(0x400, InstructionAccess, unknown_exception, 
EXC_XFER_STD) at address .400

Otherwise, I get twice the same label.

What about the following patch then ? Would this be acceptable ?

@@ -234,15 +234,13 @@ DataAccess:
 EXC_XFER_LITE(0x300, handle_page_fault)

 /* Instruction access exception.
- * This is "never generated" by the MPC8xx.  We jump to it for other
- * translation errors.
+ * This is "never generated" by the MPC8xx.
  */
 . = 0x400
 InstructionAccess:
 EXCEPTION_PROLOG
-mrr4,r12
-mrr5,r9
-EXC_XFER_LITE(0x400, handle_page_fault)
+addir3,r1,STACK_FRAME_OVERHEAD
+EXC_XFER_STD(0x401, unknown_exception)

 /* External interrupt */
 EXCEPTION(0x500, HardwareInterrupt, do_IRQ, EXC_XFER_LITE)
@@ -382,7 +380,7 @@ InstructionTLBMiss:
 #endif
 mfsprr10, SPRN_SPRG_SCRATCH2
 EXCEPTION_EPILOG_0
-bInstructionAccess
+bInstructionTLBError

 . = 0x1200
 DataStoreTLBMiss:
@@ -477,7 +475,11 @@ DataStoreTLBMiss:
  */
 . = 0x1300
 InstructionTLBError:
-bInstructionAccess
+EXCEPTION_PROLOG
+mrr4,r12
+mrr5,r9
+/* 0x400 is InstructionAccess exception, needed by bad_page_fault() */
+EXC_XFER_LITE(0x400, handle_page_fault)

 /* This is the data TLB error on the MPC8xx.  This could be due to
  * many reasons, including a dirty update to a pte.  We can catch that




If not I think you should keep the old/current way?

Same for DataTLB Miss/Error


  /* This is the data TLB error on the MPC8xx.  This could be due to
   * many reasons, including a dirty update to a pte.  We can catch that
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 51ab9e7..4d63c96 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -526,6 +526,7 @@ void bad_page_fault(struct pt_regs *regs, unsigned

long address, int sig)

break;
 case 0x400:
 case 0x480:
+   case 0x1300:
printk(KERN_ALERT "Unable to handle kernel paging request for "
   "instruction fetch\n");
break;
--
2.1.0



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 03/21] powerpc/8xx: exception InstructionAccess does not exist on MPC8xx

[leroy christophe]

Le 18/09/2014 22:02, Joakim Tjernlund a écrit :

christophe leroy  wrote on 2014/09/18 21:11:01:


Le 18/09/2014 20:12, Joakim Tjernlund a écrit :

leroy christophe  wrote on 2014/09/18

18:42:14:

Le 18/09/2014 17:15, Joakim Tjernlund a écrit :

Christophe Leroy  wrote on 2014/09/17

18:36:57:

Exception InstructionAccess does not exist on MPC8xx. No need to

branch

there from somewhere else.

Handling can be done directly in InstructionTLBError Exception.

Signed-off-by: Christophe Leroy 

. = 0x1200
DataStoreTLBMiss:
@@ -477,7 +475,11 @@ DataStoreTLBMiss:
 */
. = 0x1300
InstructionTLBError:
-bInstructionAccess
+EXCEPTION_PROLOG
+mrr4,r12
+mrr5,r9
+/* 0x400 is InstructionAccess exception, needed by

bad_page_fault()

*/

+EXC_XFER_LITE(0x400, handle_page_fault)

You should have the code in TLBMiss and have the TLBError branch there

as

that is the common case.

As far as I remember, I tried it already but we don't have enough space
in TLBmiss for that. We can only have 40 instructions.

Do your other optimizations first, then you might have space :)




Even with the optimisation first, there is no chance to fit ITLBError 
instructions after ITLBMiss and before DTLBMiss.
After optimisation, TLBMiss goes from 0x1100 to 0x1174. TLBError goes 
from 0x1300 to 0x13b4. DTLBMiss is at 0x1200

And this is without CPU6 ERRATA. So this is hopeless I believe.

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Looking for mpc8xx QMC driver

[leroy christophe]

I'm looking for someone having some QMC driver and/or some experience 
with the QMC (QUICC Multichannel Controlleur) on mpc8xx.
This is because we are trying to implement audio interface to codecs via 
the TDM bus using the QUICC. At the time being we are facing a major 
issue which is that after some time, we don't get any interrupts 
anymore, and we are not able to find what we did wrong.


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 1/2] spi: fsl-spi: Fix parameter ram offset setup for CPM1

[leroy christophe]

Le 07/10/2014 02:15, Scott Wood a écrit :

On Sat, 2014-10-04 at 14:02 +0200, christophe leroy wrote:

Le 03/10/2014 22:29, Scott Wood a écrit :

On Fri, 2014-10-03 at 18:49 +0200, Christophe Leroy wrote:

On CPM1, the SPI parameter RAM has a default location. In fsl_spi_cpm_get_pram()
there was a confusion between the SPI_BASE register and the base of the SPI
parameter RAM. Fortunatly, it was working properly with MPC866 and MPC885
because they do set SPI_BASE, but on MPC860 and other old MPC8xx that doesn't
set SPI_BASE, pram_ofs was not properly set. This patch fixes this confusion.

Signed-off-by: Christophe Leroy 

---
Changes from v1 to v2: none

   drivers/spi/spi-fsl-cpm.c | 9 -
   1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-fsl-cpm.c b/drivers/spi/spi-fsl-cpm.c
index 54b0637..0f3a912 100644
--- a/drivers/spi/spi-fsl-cpm.c
+++ b/drivers/spi/spi-fsl-cpm.c
@@ -262,15 +262,14 @@ static unsigned long fsl_spi_cpm_get_pram(struct 
mpc8xxx_spi *mspi)
pram_ofs = cpm_muram_alloc(SPI_PRAM_SIZE, 64);
out_be16(spi_base, pram_ofs);
} else {
-   struct spi_pram __iomem *pram = spi_base;
-   u16 rpbase = in_be16(&pram->rpbase);
+   u16 rpbase = in_be16(spi_base);
   
-		/* Microcode relocation patch applied? */

+   /* Microcode relocation patch applied | rpbase set by default */
if (rpbase) {
pram_ofs = rpbase;
} else {
-   pram_ofs = cpm_muram_alloc(SPI_PRAM_SIZE, 64);
-   out_be16(spi_base, pram_ofs);
+   pram_ofs = offsetof(cpm8xx_t, cp_dparam[PROFF_SPI]) -
+  offsetof(cpm8xx_t, cp_dpmem[0]);
}
}

Why is PROFF_SPI not coming from the device tree?

That's where it starts to become tricky.

PROFF_SPI is defined in cpm1.h which is included by the driver already.

Yes, but those values shouldn't be used.  It's a leftover from the old
way of hardcoding things and describing the hardware with kconfig rather
than the device tree.


It provides the default offset from the start of the parameter RAM.
Previously I had the following in my device tree, and the last part of
the source above (the one for rpbase == 0) could not work.

  spi: spi@a80 {
  cell-index = <0>;
  compatible = "fsl,spi", "fsl,cpm1-spi";
  reg = <0xa80 0x30 0x3d80 0x30>;

First reg area was the area for SPI registers. Second area was the
parameter RAM zone, which was just mapped to get access to the SPI_BASE
pointer (rpbase)

Now I have

  compatible = "fsl,spi", "fsl,cpm1-spi-reloc";
  reg = <0xa80 0x30 0x3dac 0x2>;

First reg area is the area for SPI registers. Second area is the
SPI_BASE, as for the CPM2.

On recent 8xx (885 and 866 at least) it contains the offset (=0x1D80) of
the parameter RAM. But on old ones (860, ...) it contains 0. Therefore
we have to get the default index in another way.
What I wanted was to keep something similar to what's done with CPM2.

What should it look like if that offset had to be in the device tree ?

If the offset is not relocatable or discoverable, it should stay in the
device tree.  If you have an old chip you wouldn't have
fsl,cpm1-spi-reloc and thus you'd still have "0x3d80 0x30" in reg.
This index is from the start of the dual port RAM. It is 0x2000 above 
the start of the CPM area.

In the DTS, we have:

soc@ff00 {
compatible = "fsl,mpc885", "fsl,pq1-soc";
#address-cells = <1>;
#size-cells = <1>;
device_type = "soc";
ranges = <0x0 0xff00 0x28000>;
bus-frequency = <0>;
clock-frequency = <0>;

cpm@9c0 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "fsl,mpc885-cpm", "fsl,cpm1";
ranges;
reg = <0x9c0 0x40>;
brg-frequency = <0>;
interrupts = <0>;// cpm error interrupt
interrupt-parent = <&CPM_PIC>;

muram@2000 {
#address-cells = <1>;
#size-cells = <1>;
ranges = <0x0 0x2000 0x2000>;

data@0 {
compatible = "fsl,cpm-muram-data";
reg = <0x0 0x1c00>;
};
};

spi: spi@a80 {
#address-cells = <1>;
#size-cells = <0>;
cell-index = <0>;
compatible = "fsl,spi", "fsl,cpm1-spi";
reg = <0xa80 0x30 0x3d80 0x30>;
interrupts = <5>;
interrupt-parent = <&CPM_PIC>;
mode = "cpu";


The binding allows me to do an of_iomap() on the parameter RAM, hence to 
get access to the relocation index which is inside it.
But if the relocation index is 0, I have to calculate it by myself 
because th

Re: [PATCH 2/2] spi: fsl-spi: Allow dynamic allocation of CPM1 parameter RAM

[leroy christophe]

Le 07/10/2014 02:19, Scott Wood a écrit :

On Sat, 2014-10-04 at 12:15 +0200, christophe leroy wrote:

Le 03/10/2014 22:24, Scott Wood a écrit :

On Fri, 2014-10-03 at 22:15 +0200, christophe leroy wrote:

Le 03/10/2014 16:44, Mark Brown a écrit :

On Fri, Oct 03, 2014 at 02:56:09PM +0200, Christophe Leroy wrote:


+config CPM1_RELOCSPI
+   bool "Dynamic SPI relocation"
+   default n
+   help
+ On recent MPC8xx (at least MPC866 and MPC885) SPI can be relocated
+ without micropatch. This activates relocation to a dynamically
+ allocated area in the CPM Dual port RAM.
+ When combined with SPI relocation patch (for older MPC8xx) it avoids
+ the "loss" of additional Dual port RAM space just above the patch,
+ which might be needed for example when using the CPM QMC.

Something like this shouldn't be a compile time option.  Either it
should be unconditional or it should be triggered in some system
specific manner (from DT, from knowing about other users or similar).

Can't be unconditional as older versions of mpc8xx (eg MPC860) don't
support relocation without a micropatch.
I have therefore submitted a v2 based on a DTS compatible property.

So the device tree change is about whether relocation is supported, not
whether it is required?

Indeed no, my intension is to say that relocation is requested. Do you
mean that it should then not use a compatible ?

The device tree describes hardware.  It doesn't tell software how to use
that hardware.

Based on one of your other e-mails, I think what you want to say here is
that the old binding didn't describe the registers needed for
relocation, so the new compatible describes the new binding, rather than
requesting that software do a relocation.  Software that sees the new
binding could choose to relocate, or just choose to read the current
offset from the register.

Not exactly.
The old binding does describe the entire default param RAM (0x3d80 size 
0x30). The relocation index is within this param RAM at 0x3dac.

So the old binding is enough to allow relocation.
The issue today with the driver (hence my first patch) is that the 
driver reads the relocation index but takes a wrong decision if the 
index is 0: it assumes that an nul index means that a param RAM shall be 
allocated, which is wrong. A nul index means that the component doesn't 
support relocation, so the default param RAM shall be used. The function 
used for that is supposed to return the index. So when the index is 
null, I need to calculate it.


Now, it can't be the SPI driver by itself that decide if he has to 
relocate or not. Because it depends whether I need to relocate or not. 
There is no point in waisting another area of the dualport RAM if I 
don't need to use SCC2 in a mode that overlaps the SPI parameter RAM.


Today on the old MPC8xx, a microcode patch is needed in order to be able 
to relocate, and relocated address is directly fixed by the code 
handling the patch (sysdev/micropatch.c). The patch loading function is 
call very early in the boot process by cpm_reset() which is call by the 
xxx_setup_arch().

I have two issues with the way it is done today:
1/ the address which in hard coded is the micropatch loading function() 
is within the area for descripters for the QMC, so I would need to use 
another address.
2/ for new MPC8xx which don't need microcode patch, I have no way today 
to relocate.


I have the same issue with the relocation of SMC1. Today when we 
activate SMC1 relocation microcode patch, the loading function has a 
hard coded relocation area for SMC1 which is the area dedicated to the 
MPC8xx DSP. It means that I need to change it as I want to use the DSP.


Would it be acceptable to define a fixed relocation address in the 
Kconfig in which we select microcode patch (arch/powerpc/platforms/8xx), 
instead of having it hardcoded in micropatch.c ?


Or maybe it would be possible to select which microcode patch we 
want/need via the device tree and which address shall be used for 
relocation ? What would you suggest to describe it ?



How about checking for the existing specific-SoC compatibles?

What do you mean ?

Look for "fsl,mpc885-cpm-i2c" etc.  Or, if you didn't follow that
pattern (remember, I can't see your device tree!), look for
"fsl,mpc885-cpm" or "fsl,mpc866-cpm" in the parent node.  It's moot
though, if the device tree also needs to be modified to describe the
register used to relocate.

-Scott


I'm not sure I understood your question.
My full device tree below

Christophe

/*
 * MIA ethernet Device Tree Source
 *
 * Copyright 2011 CSSI, Inc
 */

/dts-v1/;

/ {
model = "MIAE";
compatible = "fsl,cmpc885", "fsl,mod885";
#address-cells = <1>;
#size-cells = <1>;

aliases {
ethernet0 = ð0;
ethernet1 = ð1;
mdio = &phy;
serial0 = &smc1;
};

cpus {
#address-cells = <1>;
#size-cells = <0>;

PowerPC,885@0 {
dev

Re: [PATCH 0/2] net: fs_enet: Remove non NAPI RX and add NAPI for TX

[leroy christophe]

Le 08/10/2014 22:03, David Miller a écrit :

From: Christophe Leroy 
Date: Tue,  7 Oct 2014 15:04:53 +0200 (CEST)


When using a MPC8xx as a router, 'perf' shows a significant time spent in
fs_enet_interrupt() and fs_enet_start_xmit().
'perf annotate' shows that the time spent in fs_enet_start_xmit is indeed spent
between spin_unlock_irqrestore() and the following instruction, hence in
interrupt handling. This is due to the TX complete interrupt that fires after
each transmitted packet.
This patchset first remove all non NAPI handling as NAPI has become the only
mode for RX, then adds NAPI for handling TX complete.
This improves NAT TCP throughput by 21% on MPC885 with FEC.

Tested on MPC885 with FEC.

[PATCH 1/2] net: fs_enet: Remove non NAPI RX
[PATCH 2/2] net: fs_enet: Add NAPI TX

Signed-off-by: Christophe Leroy 

Series applied, thanks.

Any particular reason you didn't just put the TX reclaim calls into
the existing NAPI handler?

Not really. I used the gianfar.c driver as a model.


That's what other drivers do, because TX reclaim can make SKBs
available for RX packet receive on the local cpu.  So generally you
have one NAPI context that first does any pending TX reclaim, then
polls the RX ring for new packets.


Is that a better approach ?

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: powerpc32: add support for csum_add()

[leroy christophe]

Le 12/10/2014 18:22, Jochen Rollwagen a écrit :

This patch

https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-September/121144.html

only compiles after putting an #ifndef ARCH_HAS_CSUM_ADD around the 
definition in include/net/checksum.h


This is missing from the patch




This is already included upstream since May 2014, see patch below

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=07064c6e022ba8dc0c86ce12f7851a1de24e04fc

From 07064c6e022ba8dc0c86ce12f7851a1de24e04fc Mon Sep 17 00:00:00 2001
From: Tom Herbert 
Date: Fri, 2 May 2014 16:28:03 -0700
Subject: net: Allow csum_add to be provided in arch

csum_add is really nothing more then add-with-carry which
can be implemented efficiently in some architectures.
Allow architecture to define this protected by HAVE_ARCH_CSUM_ADD.

Signed-off-by: Tom Herbert 
Signed-off-by: David S. Miller 

diff --git a/include/net/checksum.h b/include/net/checksum.h
index a28f4e0..87cb190 100644
--- a/include/net/checksum.h
+++ b/include/net/checksum.h
@@ -57,12 +57,14 @@ static __inline__ __wsum csum_and_copy_to_user
 }
 #endif
 
+#ifndef HAVE_ARCH_CSUM_ADD

 static inline __wsum csum_add(__wsum csum, __wsum addend)
 {
u32 res = (__force u32)csum;
res += (__force u32)addend;
return (__force __wsum)(res + (res < (__force u32)addend));
 }
+#endif
 
 static inline __wsum csum_sub(__wsum csum, __wsum addend)

 {
--
cgit v0.10.1



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 00/21] powerpc/8xx: Optimise MMU TLB handling and add support of 16k pages

[leroy christophe]

Le 17/09/2014 22:34, Scott Wood a écrit :

On Wed, 2014-09-17 at 22:33 +0200, christophe leroy wrote:

Le 17/09/2014 18:40, Scott Wood a écrit :

On Wed, 2014-09-17 at 18:36 +0200, Christophe Leroy wrote:

This patchset:
1) provides several MMU TLB handling optimisation on MPC8xx.
2) adds support of 16k pages on MPC8xx.
All changes have been successfully tested on a custom board equipped with MPC885

Signed-off-by: Christophe Leroy 
Tested-by: Christophe Leroy 

I've already applied patches 1, 2, 4, 5, 6, 9, and 10 from the previous
patchset -- have they changed?

-Scott


No, only 3, 7, 17 are changed, and 20,21 are new.
I didn't notice you already applied some. How should I then proceed now
for the remaining ones ? Submit a new set ?


No, I'll just skip the ones I've already applied.



I think I took into account all Joakim's and your's comments in v4.
Since I submitted v4, I think I didn't get any new comment.
What's the way forward to get the remaining ones applied ?

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

kernel 3.17 - perf build failure

[leroy christophe]

  LINK perf
libperf.a(skip-callchain-idx.o): In function `arch_skip_callchain_idx':
/root/gen/trunk/knl/tools/perf/arch/powerpc/util/skip-callchain-idx.c:250: 
undefined reference to `pr_debug'

libperf.a(skip-callchain-idx.o): In function `check_return_addr':
/root/gen/trunk/knl/tools/perf/arch/powerpc/util/skip-callchain-idx.c:188: 
undefined reference to `pr_debug'

libperf.a(skip-callchain-idx.o): In function `arch_skip_callchain_idx':
/root/gen/trunk/knl/tools/perf/arch/powerpc/util/skip-callchain-idx.c:250: 
undefined reference to `pr_debug'

libperf.a(skip-callchain-idx.o): In function `check_return_addr':
/root/gen/trunk/knl/tools/perf/arch/powerpc/util/skip-callchain-idx.c:165: 
undefined reference to `pr_debug'
/root/gen/trunk/knl/tools/perf/arch/powerpc/util/skip-callchain-idx.c:171: 
undefined reference to `pr_debug'
libperf.a(skip-callchain-idx.o):/root/gen/trunk/knl/tools/perf/arch/powerpc/util/skip-callchain-idx.c:101: 
more undefined references to `pr_debug' follow

collect2: ld returned 1 exit status
make[1]: *** [perf] Error 1
make: *** [install] Error 2

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

What is the reel purpose of in_beXX() and out_beXX() fonctions ?

[leroy christophe]

Many drivers use in_be16(), in_be32(), out_be16(), out_be32(), etc  
to access to registrers in IO mapped memory.


What is the real purpose of those functions, and are they really needed ?

ioremap() maps the related areas as GUARDED, which means that accesses 
can't be speculative. So what is the benefit of using in_beXX() and 
out_beXX() over simple memory accesses in the area ?


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v4,17/21] powerpc/8xx: set PTE bit 22 off TLBmiss

[leroy christophe]

Le 07/11/2014 04:37, Scott Wood a écrit :

On Fri, Sep 19, 2014 at 10:36:09AM +0200, LEROY Christophe wrote:

No need to re-set this bit at each TLB miss. Let's set it in the PTE.

Signed-off-by: Christophe Leroy 
---
Changes in v2:
- None

Changes in v3:
- Removed PPC405 related macro from PPC8xx specific code
- PTE_NONE_MASK doesn't need PAGE_ACCESSED in Linux 2.6

Changes in v4:
- None

  arch/powerpc/include/asm/pgtable-ppc32.h | 20 
  arch/powerpc/include/asm/pte-8xx.h   |  7 +--
  arch/powerpc/kernel/head_8xx.S   | 10 ++
  3 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/include/asm/pgtable-ppc32.h 
b/arch/powerpc/include/asm/pgtable-ppc32.h
index 47edde8..35a9b44 100644
--- a/arch/powerpc/include/asm/pgtable-ppc32.h
+++ b/arch/powerpc/include/asm/pgtable-ppc32.h
@@ -172,6 +172,25 @@ static inline unsigned long pte_update(pte_t *p,
  #ifdef PTE_ATOMIC_UPDATES
unsigned long old, tmp;
  
+#ifdef CONFIG_PPC_8xx

+   unsigned long tmp2;
+
+   __asm__ __volatile__("\
+1: lwarx   %0,0,%4\n\
+   andc%1,%0,%5\n\
+   or  %1,%1,%6\n\
+   /* 0x200 == Extended encoding, bit 22 */ \
+   /* Bit 22 has to be 1 if neither _PAGE_USER nor _PAGE_RW are set */ \
+   rlwimi  %1,%1,32-2,0x200\n /* get _PAGE_USER */ \
+   rlwinm  %3,%1,32-1,0x200\n /* get _PAGE_RW */ \
+   or  %1,%3,%1\n\
+   xori%1,%1,0x200\n"
+" stwcx.  %1,0,%4\n\
+   bne-1b"

Why do you need this...


diff --git a/arch/powerpc/include/asm/pte-8xx.h 
b/arch/powerpc/include/asm/pte-8xx.h
index d44826e..daa4616 100644
--- a/arch/powerpc/include/asm/pte-8xx.h
+++ b/arch/powerpc/include/asm/pte-8xx.h
@@ -48,19 +48,22 @@
   */
  #define _PAGE_RW  0x0400  /* lsb PP bits, inverted in HW */
  #define _PAGE_USER0x0800  /* msb PP bits */
+/* set when neither _PAGE_USER nor _PAGE_RW are set */
+#define _PAGE_KNLRO0x0200
  
  #define _PMD_PRESENT	0x0001

  #define _PMD_BAD  0x0ff0
  #define _PMD_PAGE_MASK0x000c
  #define _PMD_PAGE_8M  0x000c
  
-#define _PTE_NONE_MASK _PAGE_ACCESSED

+#define _PTE_NONE_MASK _PAGE_KNLRO
  
  /* Until my rework is finished, 8xx still needs atomic PTE updates */

  #define PTE_ATOMIC_UPDATES1
  
  /* We need to add _PAGE_SHARED to kernel pages */

-#define _PAGE_KERNEL_RO(_PAGE_SHARED)
+#define _PAGE_KERNEL_RO(_PAGE_SHARED | _PAGE_KNLRO)
+#define _PAGE_KERNEL_ROX   (_PAGE_EXEC | _PAGE_KNLRO)
  #define _PAGE_KERNEL_RW   (_PAGE_DIRTY | _PAGE_RW | _PAGE_HWWRITE)

...if 0x200 is already being set in the PTE here?

If I understand well the way it works, those defines are used for 
setting the PTE the first time.

Then, pte_update() is used to modify the pte settings on an existing pte

0x200 must be set when and only when the page is a RO kernel page. If 
later on pte_update() is called for instance to set the page to RW, the 
0x200 has to be removed. Same, if pte_update() is called to remove 
_PAGE_RW (ptep_set_wrprotect() does this), 0x200 must be set back.


Christophe

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v2 PATCH 2/2] powerpc/8xx: use _PAGE_RO instead of _PAGE_RW

[leroy christophe]

Le 18/12/2014 03:22, Scott Wood a écrit :

On Wed, 2014-12-17 at 10:14 +0100, Christophe Leroy wrote:

On powerpc 8xx, in TLB entries, 0x400 bit is set to 1 for read-only pages
and is set to 0 for RW pages. So we should use _PAGE_RO instead of _PAGE_RW

Signed-off-by: Christophe Leroy 

---
v2 is a complete rework compared to v1

  arch/powerpc/include/asm/pte-8xx.h | 7 +++
  arch/powerpc/kernel/head_8xx.S | 3 ---
  2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/pte-8xx.h 
b/arch/powerpc/include/asm/pte-8xx.h
index daa4616..4f5583e 100644
--- a/arch/powerpc/include/asm/pte-8xx.h
+++ b/arch/powerpc/include/asm/pte-8xx.h
@@ -46,7 +46,7 @@
   * require a TLB exception handler change.  It is assumed unused bits
   * are always zero.
   */
-#define _PAGE_RW   0x0400  /* lsb PP bits, inverted in HW */
+#define _PAGE_RO   0x0400  /* lsb PP bits */

It looks like pte_update() needs to be updated to match.


  #define _PAGE_USER0x0800  /* msb PP bits */
  /* set when neither _PAGE_USER nor _PAGE_RW are set */

Also update this comment.


  #define _PAGE_KNLRO   0x0200
@@ -62,9 +62,8 @@
  #define PTE_ATOMIC_UPDATES1
  
  /* We need to add _PAGE_SHARED to kernel pages */

-#define _PAGE_KERNEL_RO(_PAGE_SHARED | _PAGE_KNLRO)
-#define _PAGE_KERNEL_ROX   (_PAGE_EXEC | _PAGE_KNLRO)
-#define _PAGE_KERNEL_RW(_PAGE_DIRTY | _PAGE_RW | _PAGE_HWWRITE)
+#define _PAGE_KERNEL_RO(_PAGE_SHARED | _PAGE_RO | _PAGE_KNLRO)
+#define _PAGE_KERNEL_ROX   (_PAGE_EXEC | _PAGE_RO | _PAGE_KNLRO)
  
  #endif /* __KERNEL__ */

  #endif /*  _ASM_POWERPC_PTE_8xx_H */

Where did _PAGE_KERNEL_RW go?



It is already defined like this in pte-common.h:

#ifndef _PAGE_KERNEL_RW
#define _PAGE_KERNEL_RW(_PAGE_DIRTY | _PAGE_RW | _PAGE_HWWRITE)
#endif


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v2 PATCH 1/2] powerpc32: adds handling of _PAGE_RO

[leroy christophe]

Le 18/12/2014 03:14, Scott Wood a écrit :

On Wed, 2014-12-17 at 10:14 +0100, Christophe Leroy wrote:

Some powerpc like the 8xx don't have a RW bit in PTE bits but a RO (Read Only) 
bit.
This patch implements the handling of a _PAGE_RO flag to be used in place of 
_PAGE_RW

Signed-off-by: Christophe Leroy 

---
v2 is a complete rework compared to v1

  arch/powerpc/include/asm/pgtable-ppc32.h | 11 ++-
  arch/powerpc/include/asm/pgtable.h   | 10 +++---
  arch/powerpc/include/asm/pte-common.h| 27 ++-
  arch/powerpc/mm/gup.c|  2 ++
  arch/powerpc/mm/mem.c|  2 +-
  arch/powerpc/mm/pgtable_32.c | 24 
  6 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/include/asm/pgtable-ppc32.h 
b/arch/powerpc/include/asm/pgtable-ppc32.h
index 543bb8e..64ed9e1 100644
--- a/arch/powerpc/include/asm/pgtable-ppc32.h
+++ b/arch/powerpc/include/asm/pgtable-ppc32.h
@@ -125,7 +125,7 @@ extern int icache_44x_need_flush;
  #ifndef __ASSEMBLY__
  
  #define pte_clear(mm, addr, ptep) \

-   do { pte_update(ptep, ~_PAGE_HASHPTE, 0); } while (0)
+   do { pte_update(ptep, ~_PAGE_HASHPTE, _PAGE_RO); } while (0)

Is this really necessary?  It's already clearing the valid bit.

Likewise in several other places that set or check for _PAGE_RO on pages
for which no access is permitted.


@@ -287,8 +287,9 @@ static inline void huge_ptep_set_wrprotect(struct mm_struct 
*mm,
  static inline void __ptep_set_access_flags(pte_t *ptep, pte_t entry)
  {
unsigned long bits = pte_val(entry) &
-   (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_RW | _PAGE_EXEC);
-   pte_update(ptep, 0, bits);
+   (_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_RW | _PAGE_RO |
+_PAGE_EXEC);
+   pte_update(ptep, _PAGE_RO, bits);
  }

You're unconditionally clearing _PAGE_RO, and apparently relying on the
undocumented behavior of pte_update() to clear "clr" before setting
"set".

Instead I'd write this as:

unsigned long set = pte_val(entry) &
(_PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_RW | _PAGE_EXEC);
unsigned long clr = pte_val(entry) & _PAGE_RO;

Don't you mean ?

unsigned long clr = ~pte_val(entry) & _PAGE_RO;

Because, we want to clear _PAGE_RO when _PAGE_RO is not set in entry.

Christophe



pte_update(ptep, clr, set);

-Scott



___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v2 PATCH 1/2] powerpc32: adds handling of _PAGE_RO

[leroy christophe]

Le 18/12/2014 03:14, Scott Wood a écrit :

On Wed, 2014-12-17 at 10:14 +0100, Christophe Leroy wrote:

Some powerpc like the 8xx don't have a RW bit in PTE bits but a RO (Read Only) 
bit.
This patch implements the handling of a _PAGE_RO flag to be used in place of 
_PAGE_RW

Signed-off-by: Christophe Leroy 

---
v2 is a complete rework compared to v1

  arch/powerpc/include/asm/pgtable-ppc32.h | 11 ++-
  arch/powerpc/include/asm/pgtable.h   | 10 +++---
  arch/powerpc/include/asm/pte-common.h| 27 ++-
  arch/powerpc/mm/gup.c|  2 ++
  arch/powerpc/mm/mem.c|  2 +-
  arch/powerpc/mm/pgtable_32.c | 24 
  6 files changed, 54 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/include/asm/pgtable-ppc32.h 
b/arch/powerpc/include/asm/pgtable-ppc32.h
index 543bb8e..64ed9e1 100644
--- a/arch/powerpc/include/asm/pgtable-ppc32.h
+++ b/arch/powerpc/include/asm/pgtable-ppc32.h
@@ -125,7 +125,7 @@ extern int icache_44x_need_flush;
  #ifndef __ASSEMBLY__
  
  #define pte_clear(mm, addr, ptep) \

-   do { pte_update(ptep, ~_PAGE_HASHPTE, 0); } while (0)
+   do { pte_update(ptep, ~_PAGE_HASHPTE, _PAGE_RO); } while (0)

Is this really necessary?  It's already clearing the valid bit.

Likewise in several other places that set or check for _PAGE_RO on pages
for which no access is permitted.


You are right, this is not needed. I needed it because I had defined 
pte_none() as requiring _PAGE_RO set. But we can keep value 0 as 
pte_none. Taken into account in v3


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v3 2/2] powerpc/8xx: use _PAGE_RO instead of _PAGE_RW

[leroy christophe]

Le 05/01/2015 19:12, Joakim Tjernlund a écrit :

On Mon, 2014-12-22 at 11:14 +0100, Christophe Leroy wrote:

On powerpc 8xx, in TLB entries, 0x400 bit is set to 1 for read-only pages
and is set to 0 for RW pages. So we should use _PAGE_RO instead of _PAGE_RW

Signed-off-by: Christophe Leroy 

Hi Christophe, been meaning to look over all you recent 8xx MMU/TLB patches
but got so little time :(

This is very cool (not sure if there will be a performance gain)  but ..

I think every saved cycle is worth it.
Before I did any modification:
* ITLBMiss was 28 instructions.
* DTLBMiss was 32 instructions.
Now, (No MODULES, no CPU6, no CPU15):
* ITLBMiss is 15 instructions
* DTLBMiss is 24 instructions



diff --git a/arch/powerpc/include/asm/pgtable-ppc32.h 
b/arch/powerpc/include/asm/pgtable-ppc32.h
index caf094a..b4e0c3b 100644
--- a/arch/powerpc/include/asm/pgtable-ppc32.h
+++ b/arch/powerpc/include/asm/pgtable-ppc32.h
@@ -178,9 +178,10 @@ static inline unsigned long pte_update(pte_t *p,
 andc%1,%0,%5\n\
 or  %1,%1,%6\n\
 /* 0x200 == Extended encoding, bit 22 */ \
-   /* Bit 22 has to be 1 if neither _PAGE_USER nor _PAGE_RW are set */ \
+   /* Bit 22 has to be 1 when _PAGE_USER is unset and _PAGE_RO is set */ \
 rlwimi  %1,%1,32-2,0x200\n /* get _PAGE_USER */ \
-   rlwinm  %3,%1,32-1,0x200\n /* get _PAGE_RW */ \
+   rlwinm  %3,%1,32-1,0x200\n /* get _PAGE_RO */ \
+   xori%3,%3,0x200\n \
 or  %1,%3,%1\n\
 xori%1,%1,0x200\n"
  "  stwcx.  %1,0,%4\n\

... here I expected to loose the existing xori insn instead of adding one?


Well, I could have xored the PAGE_USER bit instead, but in that case, it 
is not anymore an 'or' but an 'and' that has to be performed between the 
bits, and then all other bits must be set to 1, or the result of the 
'and' shall be inserted using 'rlwimi'. So it would be more 
modifications than just adding an xori, and not less instructions.


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 09/11] powerpc/8xx: dont save CR in SCRATCH registers

[leroy christophe]

Le 05/01/2015 19:30, Joakim Tjernlund a écrit :

On Tue, 2014-12-16 at 16:03 +0100, Christophe Leroy wrote:

CR only needs to be preserved when checking if we are handling a kernel address.
So we can preserve CR in a register:
- In ITLBMiss, check is done only when CONFIG_MODULES is defined. Otherwise we
don't need to do anything at all with CR.
- If CONFIG_8xx_CPU6 is defined, we have r3 available for saving CR
- Otherwise, we use r10, then we reload SRR0/MD_EPN into r10 when CR is restored

I think the #ifdef CPU6 code makes for too much maintenance. Can you
loose the #ifdef CPU6 and adjust code so it works for both cases?
I wanted to take the opportunity to save one cycle, but we are wasting 
so many cycles with CPU6 that it is propably not worth it.

Ok, I will remove the special handling for CPU6.




Signed-off-by: Christophe Leroy 

---
  arch/powerpc/kernel/head_8xx.S | 53 +-
  1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index c89aed9..a073918 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -308,14 +308,10 @@ SystemCall:
  #endif
  
  InstructionTLBMiss:

+   EXCEPTION_PROLOG_0
  #ifdef CONFIG_8xx_CPU6
 mtspr   SPRN_DAR, r3
  #endif
-   EXCEPTION_PROLOG_0
-   mfcrr10
-   mtspr   SPRN_SPRG_SCRATCH2, r10
-   mfspr   r10, SPRN_SRR0/* Get effective address of fault */
-   DO_8xx_CPU15(r11, r10)
  
 /* If we are faulting a kernel address, we have to use the

 * kernel page tables.
@@ -323,14 +319,33 @@ InstructionTLBMiss:
  #ifdef CONFIG_MODULES
 /* Only modules will cause ITLB Misses as we always
 * pin the first 8MB of kernel memory */
+#ifdef CONFIG_8xx_CPU6
+   mfspr   r10, SPRN_SRR0/* Get effective address of fault */
+   DO_8xx_CPU15(r11, r10)
+   mfcrr3
 andis.  r11, r10, 0x8000/* Address >= 0x8000 */
+#else
+   mfspr   r11, SPRN_SRR0/* Get effective address of fault */
+   DO_8xx_CPU15(r10, r11)
+   mfcrr10
+   andis.  r11, r11, 0x8000/* Address >= 0x8000 */
  #endif
 mfspr   r11, SPRN_M_TW/* Get level 1 table base address */
-#ifdef CONFIG_MODULES
 beq 3f
 lis r11, (swapper_pg_dir-PAGE_OFFSET)@ha
  3:
+#ifdef CONFIG_8xx_CPU6
+   mtcrr3
+#else
+   mtcrr10
+   mfspr   r10, SPRN_SRR0/* Get effective address of fault */
  #endif
+#else /* CONFIG_MODULES */
+   mfspr   r10, SPRN_SRR0/* Get effective address of fault */
+   DO_8xx_CPU15(r11, r10)
+   mfspr   r11, SPRN_M_TW/* Get level 1 table base address */
+#endif /* CONFIG_MODULES */
+
 /* Insert level 1 index */
 rlwimi  r11, r10, 32 - ((PAGE_SHIFT - 2) << 1), (PAGE_SHIFT - 2) << 1, 
29
 lwz r11, (swapper_pg_dir-PAGE_OFFSET)@l(r11)/* Get the level 1 
entry */
@@ -362,29 +377,37 @@ InstructionTLBMiss:
 mfspr   r3, SPRN_DAR
 mtspr   SPRN_DAR, r11/* Tag DAR */
  #endif
-   mfspr   r10, SPRN_SPRG_SCRATCH2
-   mtcrr10
 EXCEPTION_EPILOG_0
 rfi
  
 . = 0x1200

  DataStoreTLBMiss:
-#ifdef CONFIG_8xx_CPU6
-   mtspr   SPRN_DAR, r3
-#endif
 EXCEPTION_PROLOG_0
-   mfcrr10
-   mtspr   SPRN_SPRG_SCRATCH2, r10
-   mfspr   r10, SPRN_MD_EPN
  
 /* If we are faulting a kernel address, we have to use the

 * kernel page tables.
 */
+#ifdef CONFIG_8xx_CPU6
+   mtspr   SPRN_DAR, r3
+   mfcrr3
+   mfspr   r10, SPRN_MD_EPN
 andis.  r11, r10, 0x8000
+#else
+   mfcrr10
+   mfspr   r11, SPRN_MD_EPN
+   andis.  r11, r11, 0x8000
+#endif
 mfspr   r11, SPRN_M_TW/* Get level 1 table base address */
 beq 3f
 lis r11, (swapper_pg_dir-PAGE_OFFSET)@ha
  3:
+#ifdef CONFIG_8xx_CPU6
+   mtcrr3
+#else
+   mtcrr10
+   mfspr   r10, SPRN_MD_EPN
+#endif
+
 /* Insert level 1 index */
 rlwimi  r11, r10, 32 - ((PAGE_SHIFT - 2) << 1), (PAGE_SHIFT - 2) << 1, 
29
 lwz r11, (swapper_pg_dir-PAGE_OFFSET)@l(r11)/* Get the level 1 
entry */
@@ -441,8 +464,6 @@ DataStoreTLBMiss:
 mfspr   r3, SPRN_DAR
  #endif
 mtspr   SPRN_DAR, r11/* Tag DAR */
-   mfspr   r10, SPRN_SPRG_SCRATCH2
-   mtcrr10
 EXCEPTION_EPILOG_0
 rfi
  


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 05/11] powerpc/8xx: Optimise access to swapper_pg_dir

[leroy christophe]

Le 06/01/2015 13:08, David Laight a écrit :

On Tue, 2014-12-16 at 16:03 +0100, Christophe Leroy wrote:

All accessed to PGD entries are done via 0(r11).
By using lower part of swapper_pg_dir as load index to r11, we can remove the
ori instruction.

Signed-off-by: Christophe Leroy 

Nice :)
Acked-by: Joakim Tjernlund 


---
  arch/powerpc/kernel/head_8xx.S | 22 ++
  1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/kernel/head_8xx.S b/arch/powerpc/kernel/head_8xx.S
index ae05f28..aa45225 100644
--- a/arch/powerpc/kernel/head_8xx.S
+++ b/arch/powerpc/kernel/head_8xx.S
@@ -322,13 +322,12 @@ InstructionTLBMiss:
 mfspr   r11, SPRN_M_TW/* Get level 1 table base address */
  #ifdef CONFIG_MODULES
 beq 3f
-   lis r11, (swapper_pg_dir-PAGE_OFFSET)@h
-   ori r11, r11, (swapper_pg_dir-PAGE_OFFSET)@l
+   lis r11, (swapper_pg_dir-PAGE_OFFSET)@ha
  3:
  #endif
 /* Insert level 1 index */
 rlwimi  r11, r10, 32 - ((PAGE_SHIFT - 2) << 1), (PAGE_SHIFT - 2) << 1, 
29
-   lwz r11, 0(r11)/* Get the level 1 entry */
+   lwz r11, (swapper_pg_dir-PAGE_OFFSET)@l(r11)/* Get the level 1 
entry */


On the face of it that fragment doesn't look right when CONFIG_MODULES is 
undefined.

David


I'm not sure I understand what you mean.

The other part of the patch adds the following:
+lir5, (swapper_pg_dir-PAGE_OFFSET)@l
+subr4, r4, r5

r4 is the value put into SPRN_M_TW, so I don't see what may be wrong.

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [BUG] mpc8323_rdb platform doesn't boot since kernel 3.16

[leroy christophe]

Le 06/06/2015 17:32, Rob Herring a écrit :

On Sat, Jun 6, 2015 at 6:20 AM, christophe leroy
 wrote:

I've got a MPC8323 RDB evaluation platform from freescale
kernel 4.0 doesn't boot
kernel 3.16 doesn't boot
kernel 3.15 boots ok

I disected the issue down to your commit "of/fdt: Convert FDT functions to
use libfdt" (e6a6928c3ea1d0195ed75a091e345696b916c09b)

Do you have an idea of what the issue could be ?

Is your dtb older version of the dtb format (before 0x10)? libfdt
doesn't work on the older versions.

Yes, dtb has version 0x11, which seems to be the issue (see below)


If not, do you have logs with debug enabled in drivers/of/fdt.c?

I get "unflatten: error -11 processing FDT"

Indeed, Uboot (version 1.1.6 - installed on that evaluation board) adds 
a node called "chosen" at the end of the BLOB.
But Uboot doesn't update the FDT len in the BLOB header, therefore the 
following test fails in fdt_offset_ptr()


if (fdt_version(fdt) >= 0x11)
if (((offset + len) < offset)
|| ((offset + len) > fdt_size_dt_struct(fdt)))
return NULL;


If I comment this test out, Linux 3.16 to 4.0 work properly on my 
evaluation board.


Can we find a proper workaround for this issue ?

Christophe


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [BUG] mpc8323_rdb platform doesn't boot since kernel 3.16

[leroy christophe]

Le 10/06/2015 20:17, Rob Herring a écrit :

On Wed, Jun 10, 2015 at 10:12 AM, leroy christophe
 wrote:

Le 06/06/2015 17:32, Rob Herring a écrit :

On Sat, Jun 6, 2015 at 6:20 AM, christophe leroy
 wrote:

I've got a MPC8323 RDB evaluation platform from freescale
kernel 4.0 doesn't boot
kernel 3.16 doesn't boot
kernel 3.15 boots ok

I disected the issue down to your commit "of/fdt: Convert FDT functions
to
use libfdt" (e6a6928c3ea1d0195ed75a091e345696b916c09b)

Do you have an idea of what the issue could be ?

Is your dtb older version of the dtb format (before 0x10)? libfdt
doesn't work on the older versions.

Yes, dtb has version 0x11, which seems to be the issue (see below)

In that your bootloader doesn't understand 0x11.


If not, do you have logs with debug enabled in drivers/of/fdt.c?

I get "unflatten: error -11 processing FDT"

Can I get the full debug prints.

Here it is:

 -> unflatten_device_tree()
Unflattening device tree:
magic: d00dfeed
size: 20b5
version: 0011
unflatten: error -11 processing FDT
unflatten: error -11 processing FDT
  size is 44dc, allocating...
  unflattening c3ff0b20...
fixed up name for  ->
fixed up name for aliases -> aliases
fixed up name for cpus -> cpus
fixed up name for PowerPC,8323@0 -> PowerPC,8323
fixed up name for memory -> memory
fixed up name for soc8323@e000 -> soc8323
fixed up name for wdt@200 -> wdt
fixed up name for power@b00 -> power
fixed up name for i2c@3000 -> i2c
fixed up name for serial@4500 -> serial
fixed up name for serial@4600 -> serial
fixed up name for dma@82a8 -> dma
fixed up name for dma-channel@0 -> dma-channel
fixed up name for dma-channel@80 -> dma-channel
fixed up name for dma-channel@100 -> dma-channel
fixed up name for dma-channel@180 -> dma-channel
fixed up name for crypto@3 -> crypto
fixed up name for pic@700 -> pic
fixed up name for par_io@1400 -> par_io
fixed up name for gpio-controller@1448 -> gpio-controller
fixed up name for ucc_pin@02 -> ucc_pin
fixed up name for ucc_pin@03 -> ucc_pin
fixed up name for qe@e010 -> qe
fixed up name for muram@1 -> muram
fixed up name for data-only@0 -> data-only
fixed up name for spi@4c0 -> spi
fixed up name for mmc-slot@0 -> mmc-slot
fixed up name for spi@500 -> spi
fixed up name for ucc@3000 -> ucc
fixed up name for ucc@2200 -> ucc
fixed up name for mdio@3120 -> mdio
fixed up name for ethernet-phy@00 -> ethernet-phy
fixed up name for ethernet-phy@04 -> ethernet-phy
fixed up name for interrupt-controller@80 -> interrupt-controller
fixed up name for pci@e0008500 -> pci
unflatten: error -11 processing FDT
unflatten: error -11 processing FDT
 <- unflatten_device_tree()

Christophe

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Oops in 3.18.14 in destroy_inode()

[leroy christophe]

[46796.501487] Unable to handle kernel paging request for data at 
address 0x02dd

[46796.514365] Faulting instruction address: 0xc00c5978
[46796.524217] Oops: Kernel access of bad area, sig: 11 [#1]
[46796.529351] PREEMPT CMPC885
[46796.532144] CPU: 0 PID: 1107 Comm: snmpd Not tainted 3.18.14 #43
[46796.539790] task: c682d340 ti: c6728000 task.ti: c6728000
[46796.545119] NIP: c00c5978 LR: c00c5974 CTR: c00efeb4
[46796.550033] REGS: c6729e00 TRAP: 0300   Not tainted (3.18.14)
[46796.557497] MSR: 9032   CR: 24042424 XER: 2000
[46796.564043] DAR: 02dd DSISR: c000
[46796.564043] GPR00: c00c5974 c6729eb0 c682d340  c5a02734 
0003  00851d4a
[46796.564043] GPR08: 05ae 02b9 9032 01e4 24042424 
1001c8cc 7fc835f8 100ad378
[46796.564043] GPR16:  7fc835f0 7fc835e8 7fc835e0 7fc835d8 
7fc835d0 7fc835c8 7fc835c0
[46796.564043] GPR24: 0fe59f14 02ac c6a44b48 c6056110 c5e03168 
c5a026e0 c6728000 c1a026e0

[46796.596017] NIP [c00c5978] destroy_inode+0x38/0x84
[46796.600736] LR [c00c5974] destroy_inode+0x34/0x84
[46796.605344] Call Trace:
[46796.607793] [c6729eb0] [c00c5974] destroy_inode+0x34/0x84 (unreliable)
[46796.614271] [c6729ec0] [c00c1d90] __dentry_kill+0x2a8/0x304
[46796.619763] [c6729ee0] [c00c27c8] dput+0xd0/0x1d8
[46796.624416] [c6729f00] [c00adf54] __fput+0x134/0x1fc
[46796.629319] [c6729f20] [c002de28] task_work_run+0xac/0xf4
[46796.634655] [c6729f40] [c000bba4] do_user_signal+0x74/0xc4
[46796.640023] Instruction dump:
[46796.642955] 39430078 93e1000c 90010014 7c7f1b78 81230078 7d295278 
7d290034 5529d97e
[46796.650612] 69290001 0f09 4b45 813f0014 <81290024> 81290004 
2f89 419e0020

[46796.658466] ---[ end trace 0abe99599a8bf31d ]---


c00c5940 :
struct inode *inode = container_of(head, struct inode, i_rcu);
kmem_cache_free(inode_cachep, inode);
}

static void destroy_inode(struct inode *inode)
{
c00c5940:7c 08 02 a6 mflrr0
c00c5944:94 21 ff f0 stwur1,-16(r1)
BUG_ON(!list_empty(&inode->i_lru));
c00c5948:39 43 00 78 addir10,r3,120
struct inode *inode = container_of(head, struct inode, i_rcu);
kmem_cache_free(inode_cachep, inode);
}

static void destroy_inode(struct inode *inode)
{
c00c594c:93 e1 00 0c stw r31,12(r1)
c00c5950:90 01 00 14 stw r0,20(r1)
c00c5954:7c 7f 1b 78 mr  r31,r3
BUG_ON(!list_empty(&inode->i_lru));
c00c5958:81 23 00 78 lwz r9,120(r3)
c00c595c:7d 29 52 78 xor r9,r9,r10
c00c5960:7d 29 00 34 cntlzw  r9,r9
c00c5964:55 29 d9 7e rlwinm  r9,r9,27,5,31
c00c5968:69 29 00 01 xorir9,r9,1
c00c596c:0f 09 00 00 twnei   r9,0
__destroy_inode(inode);
c00c5970:4b ff ff 45 bl  c00c58b4 <__destroy_inode>
if (inode->i_sb->s_op->destroy_inode)
c00c5974:81 3f 00 14 lwz r9,20(r31)
==> c00c5978:81 29 00 24 lwz r9,36(r9)
c00c597c:81 29 00 04 lwz r9,4(r9)
c00c5980:2f 89 00 00 cmpwi   cr7,r9,0
c00c5984:41 9e 00 20 beq cr7,c00c59a4 
inode->i_sb->s_op->destroy_inode(inode);
else
call_rcu(&inode->i_rcu, i_callback);
}
c00c5988:80 01 00 14 lwz r0,20(r1)

Looks like inode->i_sb (apparently contained in r9) has value 0x2b9 
which is obviously wrong, hence the bad access at 0x2dd when trying to 
get inode->i_sb->s_op


What else can I look at to investigate this issue ?

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [HELP/RFC] Moving ppc8xx microcode patch from micropatch.c to firmware

[leroy christophe]

Le 30/06/2015 22:38, christophe leroy a écrit :
I'm trying to move the 3 microcode patches included in 
arch/powerpc/sysdev/micropatch.c into the firmware directory in order 
to use request_firmware() and then be able to add additional 
micropatch that I need to relocate SMC2 on my MPC885.
I've now been able to get it compiled in, was due to Makefile item 
written fw_shipped- instead of fw-shipped-


I'm now facing an Oops for NULL pointer in kmem_cache_alloc() after a 
call to kzalloc(sizeof(*firmware), GFP_KERNEL) in 
_request_firmware_prepare() (drivers/base/firmware_class.c)


Is that due to cpm_reset() being called too early ? Shouldn't kzalloc() 
allocate memory from the bootmem pool in that case ?


[0.00] Unable to handle kernel paging request for data at 
address 0x

[0.00] Faulting instruction address: 0xc00a77cc
[0.00] Oops: Kernel access of bad area, sig: 11 [#1]
[0.00] PREEMPT CMPC885
[0.00] CPU: 0 PID: 0 Comm: swapper Not tainted 
3.18.17-local-knld-998-g9c4c6b6-svn-dirty #1022

[0.00] task: c04dc3d0 ti: c04fc000 task.ti: c04fc000
[0.00] NIP: c00a77cc LR: c01bc3b8 CTR: 
[0.00] REGS: c04fde20 TRAP: 0300   Not tainted 
(3.18.17-local-knld-998-g9c4c6b6-svn-dirty)

[0.00] MSR: 1032   CR: 93d55d35  XER: a000fb40
[0.00] DAR:  DSISR: c000
GPR00: c01bc3b8 c04fded0 c04dc3d0  80d0  0009 
01ff
GPR08:  0022 c051 0158 53d55d39   
07ff94e8
GPR16:  07bb5d70  07ff81f4 c0534468 c04fdf68  
0009
GPR24: 07ffb3a0  0001 80d0 07ffb3a0  c04fc000 
c0410878

[0.00] NIP [c00a77cc] kmem_cache_alloc+0x28/0x120
[0.00] LR [c01bc3b8] _request_firmware+0x58/0xa14
[0.00] Call Trace:
[0.00] [c04fded0] [c045f588] ___alloc_bootmem_nopanic+0x34/0x64 
(unreliable)

[0.00] [c04fdef0] [c01bc3b8] _request_firmware+0x58/0xa14
[0.00] [c04fdf60] [c0458678] cpm_load_patch+0x34/0xac
[0.00] [c04fdf80] [c0458620] cpm_reset+0x5c/0x80
[0.00] [c04fdf90] [c0458c1c] cmpc885_setup_arch+0x10/0x30
[0.00] [c04fdfa0] [c0457cbc] setup_arch+0x130/0x168
[0.00] [c04fdfb0] [c045469c] start_kernel+0x84/0x37c
[0.00] [c04fdff0] [c0002214] start_here+0x38/0x98
[0.00] Instruction dump:
[0.00] 7c832378 4e800020 7c0802a6 9421ffe0 bf61000c 90010024 
7c7d1b78 7c9b2378
[0.00] 543e0024 813e000c 39290001 913e000c <83fd> 839f0004 
813e000c 3929

[0.00] ---[ end trace dc8fa200cb88537f ]---





I have written the below patch in order to test the principle, but the 
firmware never gets included in my kernel, allthough I have set the 
below CONFIG items:


# CONFIG_NO_UCODE_PATCH is not set
CONFIG_USB_SOF_UCODE_PATCH=y
# CONFIG_I2C_SPI_UCODE_PATCH is not set
# CONFIG_I2C_SPI_SMC1_UCODE_PATCH is not set
CONFIG_UCODE_PATCH=y
#
# Generic Driver Options
#
# CONFIG_UEVENT_HELPER is not set
CONFIG_DEVTMPFS=y
CONFIG_DEVTMPFS_MOUNT=y
# CONFIG_STANDALONE is not set
# CONFIG_PREVENT_FIRMWARE_BUILD is not set
CONFIG_FW_LOADER=y
CONFIG_FIRMWARE_IN_KERNEL=y
CONFIG_EXTRA_FIRMWARE=""
CONFIG_FW_LOADER_USER_HELPER=y
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y

Can anybody help in finding what's wrong there ?

Christophe

---
 arch/powerpc/sysdev/micropatch.c | 655 
++-

 firmware/Makefile|   3 +
 firmware/freescale/i2c_spi.bin.ihex  | 257 
 firmware/freescale/i2c_spi_smc1.bin.ihex | 257 
 firmware/freescale/usb_sof.bin.ihex  | 513 
 5 files changed, 1065 insertions(+), 620 deletions(-)
 create mode 100644 firmware/freescale/i2c_spi.bin.ihex
 create mode 100644 firmware/freescale/i2c_spi_smc1.bin.ihex
 create mode 100644 firmware/freescale/usb_sof.bin.ihex

diff --git a/arch/powerpc/sysdev/micropatch.c 
b/arch/powerpc/sysdev/micropatch.c

index 6727dc5..c24780c 100644
--- a/arch/powerpc/sysdev/micropatch.c
+++ b/arch/powerpc/sysdev/micropatch.c
@@ -12,6 +12,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -19,652 +21,69 @@
 #include 
 #include 

-/*
- * I2C/SPI relocation patch arrays.
- */
-
-#ifdef CONFIG_I2C_SPI_UCODE_PATCH
-
-static uint patch_2000[] __initdata = {
-0x7FFFEFD9,
-0x3FFD,
-0x7FFB49F7,
-0x7FF9,
-0x5FEFADF7,
-0x5F89ADF7,
-0x5FEFAFF7,
-0x5F89AFF7,
-0x3A9CFBC8,
-0xE7C0EDF0,
-0x77C1E1BB,
-0xF4DC7F1D,
-0xABAD932F,
-0x4E08FDCF,
-0x6E0FAFF8,
-0x7CCF76CF,
-0xFD1FF9CF,
-0xABF88DC6,
-0xAB5679F7,
-0xB0937383,
-0xDFCE79F7,
-0xB091E6BB,
-0xE5BBE74F,
-0xB3FA6F0F,
-0x6FFB76CE,
-0xEE0DF9CF,
-0x2BFBEFEF,
-0xCFEEF9CF,
-0x76CEAD24,
-0x90B2DF9A,
-0x7FDDD0BF,
-0x4BF847FD,
-0x7CCF76CE,
-0xCFEF7E1F,
-0x7F1D7DFD,
-0xF0B6EF71,
-0x7FC17

Re: Missing Linux patches

[leroy christophe]

Le 02/08/2015 21:05, Markus Stockhausen a écrit :

Hi Christophe,

I saw that this patch from you is still missing in Linux mainline:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2014-September/121144.html

Is there any reason for not using it?

Markus


Hi,

I sent v3 of that Patch on 19 May 2015, taking into account all comments 
I have received.


I have not received any further comments so I expect it will be applied.

Christophe

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 2/2] powerpc32: optimise csum_partial() loop

[leroy christophe]

Le 07/08/2015 01:25, Segher Boessenkool a écrit :

On Thu, Aug 06, 2015 at 05:45:45PM -0500, Scott Wood wrote:

If this makes performance non-negligibly worse on other 32-bit chips, and is
an important improvement on 8xx, then we can use an ifdef since 8xx already
requires its own kernel build.  I'd prefer to see a benchmark showing that it
actually does make things worse on those chips, though.

And I'd like to see a benchmark that shows it *does not* hurt performance
on most chips, and does improve things on 8xx, and by how much.  But it
isn't *me* who has to show that, it is not my patch.
Ok, following this discussion I made some additional measurement and it 
looks like:

* There is almost no change on the 885
* There is a non negligeable degradation on the 8323 (19.5 tb ticks 
instead of 15.3)


Thanks for pointing this out, I think my patch is therefore not good.

Christophe

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 2/2] powerpc32: optimise csum_partial() loop

[leroy christophe]

Le 17/08/2015 12:56, leroy christophe a écrit :



Le 07/08/2015 01:25, Segher Boessenkool a écrit :

On Thu, Aug 06, 2015 at 05:45:45PM -0500, Scott Wood wrote:
If this makes performance non-negligibly worse on other 32-bit 
chips, and is
an important improvement on 8xx, then we can use an ifdef since 8xx 
already
requires its own kernel build.  I'd prefer to see a benchmark 
showing that it

actually does make things worse on those chips, though.
And I'd like to see a benchmark that shows it *does not* hurt 
performance

on most chips, and does improve things on 8xx, and by how much. But it
isn't *me* who has to show that, it is not my patch.
Ok, following this discussion I made some additional measurement and 
it looks like:

* There is almost no change on the 885
* There is a non negligeable degradation on the 8323 (19.5 tb ticks 
instead of 15.3)


Thanks for pointing this out, I think my patch is therefore not good.

Oops, I was talking about my other past, the one that was to optimise 
ip_csum_fast.

I still have to measure csum_partial

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 2/2] powerpc32: optimise csum_partial() loop

[leroy christophe]

Le 17/08/2015 13:00, leroy christophe a écrit :



Le 17/08/2015 12:56, leroy christophe a écrit :



Le 07/08/2015 01:25, Segher Boessenkool a écrit :

On Thu, Aug 06, 2015 at 05:45:45PM -0500, Scott Wood wrote:
If this makes performance non-negligibly worse on other 32-bit 
chips, and is
an important improvement on 8xx, then we can use an ifdef since 8xx 
already
requires its own kernel build.  I'd prefer to see a benchmark 
showing that it

actually does make things worse on those chips, though.
And I'd like to see a benchmark that shows it *does not* hurt 
performance

on most chips, and does improve things on 8xx, and by how much. But it
isn't *me* who has to show that, it is not my patch.
Ok, following this discussion I made some additional measurement and 
it looks like:

* There is almost no change on the 885
* There is a non negligeable degradation on the 8323 (19.5 tb ticks 
instead of 15.3)


Thanks for pointing this out, I think my patch is therefore not good.

Oops, I was talking about my other past, the one that was to optimise 
ip_csum_fast.

I still have to measure csum_partial

Now, I have the results for csum_partial(). The measurement is done with 
mftbl() before and after calling the function, with IRQ off to get a 
stable measure. Measurement is done with a transfer of vmlinux file done 
3 times via scp toward the target. We get approximatly 5 calls to 
csum_partial()


On MPC885:
1/ Without the patchset, mean time spent in csum_partial() is 167 tb ticks.
2/ With the patchset, mean time is 150 tb ticks

On MPC8323:
1/ Without the patchset, mean time is 287 tb ticks
2/ With the patchset, mean time is 256 tb ticks

The improvement is approximatly 10% in both cases

So, unlike my patch on ip_fast_csum(), this one is worth it.

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [v2] Enhanced support for MPC8xx/8xxx watchdog

[leroy christophe]

Le 26/06/2013 01:04, Scott Wood a écrit :

On Thu, Feb 28, 2013 at 09:52:22AM +0100, LEROY Christophe wrote:

This patch modifies the behaviour of the MPC8xx/8xxx watchdog. On the MPC8xx,
at 133Mhz, the maximum timeout of the watchdog timer is 1s, which means it must
be pinged twice a second. This is not in line with the Linux watchdog concept
which is based on a default watchdog timeout around 60s.
This patch introduces an intermediate layer between the CPU and the userspace.
The kernel pings the watchdog at the required frequency at the condition that
userspace tools refresh it regularly.
Existing parameter 'timeout' is renamed 'hw_time'.
The new parameter 'timeout' allows to set up the userspace timeout.
The driver also implements the WDIOC_SETTIMEOUT ioctl.

Signed-off-by: Christophe Leroy 


diff -ur linux-3.7.9/drivers/watchdog/mpc8xxx_wdt.c 
linux/drivers/watchdog/mpc8xxx_wdt.c
--- linux-3.7.9/drivers/watchdog/mpc8xxx_wdt.c  2013-02-17 19:53:32.0 
+0100
+++ linux/drivers/watchdog/mpc8xxx_wdt.c2013-02-27 16:00:07.0 
+0100
@@ -52,10 +52,17 @@
  static struct mpc8xxx_wdt __iomem *wd_base;
  static int mpc8xxx_wdt_init_late(void);
  
-static u16 timeout = 0x;

-module_param(timeout, ushort, 0);
+#define WD_TIMO 10 /* Default timeout = 10 seconds */

If the default Linux watchdog timeout is normally 60 seconds, why is it 10
here?
Looks like each driver has its own default value, but agreed, I change 
it to 60 seconds



+static uint timeout = WD_TIMO;
+module_param(timeout, uint, 0);
  MODULE_PARM_DESC(timeout,
-   "Watchdog timeout in ticks. (0
hw_timeout would be more legibile -- this is a public interface.

Ok



  static bool reset = 1;
  module_param(reset, bool, 0);
@@ -72,10 +79,12 @@
   * to 0
   */
  static int prescale = 1;
-static unsigned int timeout_sec;
+static unsigned int hw_timo_sec;
  
+static int wdt_auto = 1;

bool, and add a comment indicating what this means.

Ok



  static unsigned long wdt_is_open;
  static DEFINE_SPINLOCK(wdt_spinlock);
+static unsigned long wdt_last_ping;
  
  static void mpc8xxx_wdt_keepalive(void)

  {
@@ -91,9 +100,20 @@
  
  static void mpc8xxx_wdt_timer_ping(unsigned long arg)

  {
-   mpc8xxx_wdt_keepalive();
-   /* We're pinging it twice faster than needed, just to be sure. */
-   mod_timer(&wdt_timer, jiffies + HZ * timeout_sec / 2);
+   if (wdt_auto)
+   wdt_last_ping = jiffies;
+
+   if (jiffies - wdt_last_ping <= timeout * HZ) {

So timeout cannot be more than UINT_MAX / HZ...  Might want to check for
that, just in case.

Ok.


What happens if there's a race?  If another CPU updates wdt_last_ping in
parallel, then you could see wdt_last_ping greater than the value you
read for jiffies.  Since this is an unsigned comparison, it will fail to
call keepalive.  You might get saved by pinging it twice as often as
necessary, but you shouldn't rely on that.
Euh ... This watchdog is integrated inside the CPU, so there is no 
chance that any external CPU get access to it.



+   mpc8xxx_wdt_keepalive();
+   /* We're pinging it twice faster than needed, to be sure. */
+   mod_timer(&wdt_timer, jiffies + HZ * hw_timo_sec / 2);
+   }
+}
+
+static void mpc8xxx_wdt_sw_keepalive(void)
+{
+   wdt_last_ping = jiffies;
+   mpc8xxx_wdt_timer_ping(0);
  }

This isn't new with this patch, but it looks like
mpc8xxx_wdt_keepalive() can be called either from timer context, or with
interrupts enabled... yet it uses a bare spin_lock() rather than an
irq-safe version.  This should be fixed.
Ok, I'll propose another patch for that. Indeed, is the spin_lock needed 
at all ? If we get two writes interleaved, it will make it anyway.


Christophe

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Feedback wished on possible improvment of CPU15 errata handling on mpc8xx

[leroy christophe]

The mpc8xx powerpc has an errata identified CPU15 which is that whenever 
the last instruction of a page is a conditional branch to the last 
instruction of the next page, the CPU might do crazy things.


To work around this errata, one of the workarounds proposed by freescale is:
"In the ITLB miss exception code, when loading the TLB for an MMU page, 
also invalidate any TLB referring to the next and previous page using 
tlbie. This intentionally forces an ITLB miss exception on every 
execution across sequential MMU page boundaries"


It is that workaround which has been implemented in the kernel. The 
drawback of this workaround is that TLB miss is encountered everytime we 
cross page boundary. On a flat program execution, it means that we get a 
TLB miss every 1000 instructions. A TLB miss handling is around 30/40 
instructions, which means a degradation of about 4% of the performances.

It can be even worse if the program has a loop astride two pages.

In the errata document from freescale, there is an example where they 
only invalidate the TLB when the page has the actual issue, in extenso 
when the page has the offending instruction at offset 0xffc, and they 
suggest to use the available PTE bits to tag pages in advance.


I checked in asm/pte-8xx.h : we still have one SW bit available 
(0x0080). So I was thinking about using that bit to mark pages 
CPU15_SAFE when loading them if they don't have the offending instruction.


Then, in the ITLBmiss handler, instead of always invalidating preceeding 
and following pages, we would check SW bit in the PTE and invalidate 
following page only if current page is not marked CPU15_SAFE, then check 
the PTE of preceeding page and invalidate it only if it is not marked 
CPU15_SAFE


I believe this would improve the CPU15 errata handling and would reduce 
the overhead introduced by the handling of this errata.


Do you see anything wrong with my proposal ?

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: Feedback wished on possible improvment of CPU15 errata handling on mpc8xx

[leroy christophe]

Le 29/08/2013 19:57, Joakim Tjernlund a écrit :

"Linuxppc-dev"

wrote on 2013/08/29 19:11:48:

The mpc8xx powerpc has an errata identified CPU15 which is that whenever
the last instruction of a page is a conditional branch to the last
instruction of the next page, the CPU might do crazy things.

To work around this errata, one of the workarounds proposed by freescale

is:

"In the ITLB miss exception code, when loading the TLB for an MMU page,
also invalidate any TLB referring to the next and previous page using
tlbie. This intentionally forces an ITLB miss exception on every
execution across sequential MMU page boundaries"

It is that workaround which has been implemented in the kernel. The
drawback of this workaround is that TLB miss is encountered everytime we
cross page boundary. On a flat program execution, it means that we get a
TLB miss every 1000 instructions. A TLB miss handling is around 30/40
instructions, which means a degradation of about 4% of the performances.
It can be even worse if the program has a loop astride two pages.

In the errata document from freescale, there is an example where they
only invalidate the TLB when the page has the actual issue, in extenso
when the page has the offending instruction at offset 0xffc, and they
suggest to use the available PTE bits to tag pages in advance.

I checked in asm/pte-8xx.h : we still have one SW bit available
(0x0080). So I was thinking about using that bit to mark pages
CPU15_SAFE when loading them if they don't have the offending

instruction.

Then, in the ITLBmiss handler, instead of always invalidating preceeding
and following pages, we would check SW bit in the PTE and invalidate
following page only if current page is not marked CPU15_SAFE, then check
the PTE of preceeding page and invalidate it only if it is not marked
CPU15_SAFE

I believe this would improve the CPU15 errata handling and would reduce
the overhead introduced by the handling of this errata.

Do you see anything wrong with my proposal ?

Just that you are using up the last bit of the pte which will be needed at
some point.
Have you run into CPU15? We have been using 8xx for more than 10 years on
kernel 2.4 and I
don't think we ever run into this problem.
Ok, indeed I have activated the CPU15 errata in the kernel because I 
know my CPU has the bug.

Do you think it can be deactivated without much risk though ?

If you go forward with this I suggest you use the WRITETHRU bit instead
and make
it so the user can choose which to use.

If you want to optimize TLB misses you might want to add support for 8MB
pages, I got
the TLB and kernel memory done in my 2.4 kernel. You could start with that
and
add 8MB user space page.
In 2.6 Kernel we have CONFIG_PIN_TLB which pins the first 8Mbytes in 
ITLB and pins the first 24Mbytes in DTLB as far as I understand. Do we 
need more for the kernel ? I so, yes I would be interested in porting 
your code to 2.6


Wouldn't we waste memory by using 8Mbytes pages in user mode ?
I read somewhere that Transparent Huge Pages have been ported on powerpc 
in future kernel 3.11. Therefore I was thinking about maybe adding 
support for hugepages into 8xx.
8xx has 512kbytes hugepages, I was thinking that maybe it would be more 
appropriate than 8Mbytes pages.
Do you think it would be feasible and usefull to do this for embeddeds 
system having let say 32 to 128Mbytes RAM ?


Christophe

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc 8xx: Fixing issue with CONFIG_PIN_TLB

[leroy christophe]

Le 12/09/2013 02:15, Benjamin Herrenschmidt a écrit :

On Wed, 2013-09-11 at 17:36 -0500, Scott Wood wrote:

I wonder why we don't start from entry 31 so we can actually make use of
that autodecrement.  What will happen when we load the first normal TLB
entry later on?  I don't see any setting of SPRN_MD_CTR after this code,
so won't it overwrite entry 30 (the middle 8M) in the CONFIG_PIN_TLB
case?

Ben, would patches like this be considered bugfixes as far as merging
goes, or would they be for next given that it's something that's never
really worked right and hasn't been touched in years?

Since they don't affect anything outside of 8xx, I'm happy to take them
until around -rc2 or 3. But it's your call really.



Scott, you're right, I didn't see that other consequence.
I'll come with a more complete patch this afternoon.
Thanks
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] powerpc 8xx: Fixing issue with CONFIG_PIN_TLB

[leroy christophe]

Le 12/09/2013 20:44, Scott Wood a écrit :

On Thu, 2013-09-12 at 20:25 +0200, Christophe Leroy wrote:

This is a reorganisation of the setup of the TLB at kernel startup, in order
to handle the CONFIG_PIN_TLB case in accordance with chapter 8.10.3 of MPC866
and MPC885 reference manuals.

Signed-off-by: Christophe Leroy 

diff -ur linux-3.11.org/arch/powerpc/kernel/head_8xx.S 
linux-3.11/arch/powerpc/kernel/head_8xx.S
--- linux-3.11.org/arch/powerpc/kernel/head_8xx.S   2013-09-02 
22:46:10.0 +0200
+++ linux-3.11/arch/powerpc/kernel/head_8xx.S   2013-09-09 11:28:54.0 
+0200
@@ -785,27 +785,24 @@
   * these mappings is mapped by page tables.
   */
  initial_mmu:
-   tlbia   /* Invalidate all TLB entries */
-/* Always pin the first 8 MB ITLB to prevent ITLB
-   misses while mucking around with SRR0/SRR1 in asm
-*/
-   lis r8, MI_RSV4I@h
-   ori r8, r8, 0x1c00
-
+   lis r8, MI_RESETVAL@h
mtspr   SPRN_MI_CTR, r8 /* Set instruction MMU control */
  
-#ifdef CONFIG_PIN_TLB

-   lis r10, (MD_RSV4I | MD_RESETVAL)@h
-   ori r10, r10, 0x1c00
-   mr  r8, r10
-#else
lis r10, MD_RESETVAL@h
-#endif
  #ifndef CONFIG_8xx_COPYBACK
orisr10, r10, MD_WTDEF@h
  #endif
mtspr   SPRN_MD_CTR, r10/* Set data TLB control */
  
+	tlbia			/* Invalidate all TLB entries */

Is this change to make sure we invalidate everything even if the
bootloader set RSV4I?
Most probably. It is step 2 of the process defined in MPC866 and MPC885 
Reference Manuals:


§8.10.3 Loading Locked TLB Entries:
The process of loading a single reserved entry in the TLB is as follows:
1. Disable the TLB by clearing MSR[IR] or MSR[DR] as needed.
2. Clear MI_CTR[RSV4I] (MD_CTR[RSV4D]).
3. Invalidate the EA of the reserved page by using tlbia or tlbie.
4. Set MI_CTR[ITLB_INDX] (MD_CTR[DTLB_INDX]) to the appropriate value 
(between 27 and 31).
5. Load Mx_EPN with the effective page number, the ASID of the reserved 
page, and set EV.
6. Run software tablewalk code to load the appropriate entry into the 
translation lookaside buffer. See Section 8.10.1.1, “Translation Reload 
Examples.”

7. Repeat steps 4–6 to load other TLB entries.
8. Set MI_CTR[RSV4I] (MD_CTR[RSV4D])



+   ori r8, r8, 0x1c00
+   mtspr   SPRN_MI_CTR, r8 /* Set instruction MMU control */
+#ifdef CONFIG_PIN_TLB
+   ori r10, r10, 0x1c00
+   mtspr   SPRN_MD_CTR, r10/* Set data TLB control */
+#endif

Still 0x1c00?

Yes, I kept the same entries in order to limit modifications:
* 28 = First 8Mbytes page
* 29 = IMMR
* 30 = Second 8Mbytes page
* 31 = Third 8Mbytes page



/* Now map the lower 8 Meg into the TLBs.  For this quick hack,
 * we can load the instruction and data TLB registers with the
 * same values.
@@ -825,6 +822,12 @@
mtspr   SPRN_MI_AP, r8
mtspr   SPRN_MD_AP, r8
  
+	/* Always pin the first 8 MB ITLB to prevent ITLB

+* misses while mucking around with SRR0/SRR1 in asm
+*/
+   lis r8, (MI_RSV4I | MI_RESETVAL)@h
+   mtspr   SPRN_MI_CTR, r8 /* Set instruction MMU control */

Entry 0 is not pinnable.
Here we are not trying to pin entry 0. We are at step 8, we are setting 
MI_RSV4I. At the same time, we set MD_CTR to 0 which is off the pinned 
range, to be sure that we won't overwrite one of the pinned entries.


The main difference compared to the previous implementation is that 
before, we were setting the RSV4I bit before loading the TLB entries. 
Now, as defined in the Reference Manuals, we are doing it at the end.


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] powerpc 8xx: Fixing issue with CONFIG_PIN_TLB

[leroy christophe]

Le 16/09/2013 23:02, Scott Wood a écrit :

On Fri, 2013-09-13 at 07:04 +0200, leroy christophe wrote:

Le 12/09/2013 20:44, Scott Wood a écrit :

On Thu, 2013-09-12 at 20:25 +0200, Christophe Leroy wrote:

This is a reorganisation of the setup of the TLB at kernel startup, in order
to handle the CONFIG_PIN_TLB case in accordance with chapter 8.10.3 of MPC866
and MPC885 reference manuals.

Signed-off-by: Christophe Leroy 

diff -ur linux-3.11.org/arch/powerpc/kernel/head_8xx.S 
linux-3.11/arch/powerpc/kernel/head_8xx.S
--- linux-3.11.org/arch/powerpc/kernel/head_8xx.S   2013-09-02 
22:46:10.0 +0200
+++ linux-3.11/arch/powerpc/kernel/head_8xx.S   2013-09-09 11:28:54.0 
+0200
@@ -785,27 +785,24 @@
* these mappings is mapped by page tables.
*/
   initial_mmu:
-   tlbia   /* Invalidate all TLB entries */
-/* Always pin the first 8 MB ITLB to prevent ITLB
-   misses while mucking around with SRR0/SRR1 in asm
-*/
-   lis r8, MI_RSV4I@h
-   ori r8, r8, 0x1c00
-
+   lis r8, MI_RESETVAL@h
mtspr   SPRN_MI_CTR, r8 /* Set instruction MMU control */
   
-#ifdef CONFIG_PIN_TLB

-   lis r10, (MD_RSV4I | MD_RESETVAL)@h
-   ori r10, r10, 0x1c00
-   mr  r8, r10
-#else
lis r10, MD_RESETVAL@h
-#endif
   #ifndef CONFIG_8xx_COPYBACK
orisr10, r10, MD_WTDEF@h
   #endif
mtspr   SPRN_MD_CTR, r10/* Set data TLB control */
   
+	tlbia			/* Invalidate all TLB entries */

Is this change to make sure we invalidate everything even if the
bootloader set RSV4I?

Most probably. It is step 2 of the process defined in MPC866 and MPC885
Reference Manuals:

§8.10.3 Loading Locked TLB Entries:
The process of loading a single reserved entry in the TLB is as follows:

To minimize code churn we should just fix actual problems, rather than
shuffle things around to conform to a suggested sequence.  After all,
we're not just trying to load a single entry.

Ok, I'll try again.



+   ori r8, r8, 0x1c00
+   mtspr   SPRN_MI_CTR, r8 /* Set instruction MMU control */
+#ifdef CONFIG_PIN_TLB
+   ori r10, r10, 0x1c00
+   mtspr   SPRN_MD_CTR, r10/* Set data TLB control */
+#endif

Still 0x1c00?

Yes, I kept the same entries in order to limit modifications:
* 28 = First 8Mbytes page
* 29 = IMMR
* 30 = Second 8Mbytes page
* 31 = Third 8Mbytes page

If you actually want to program them in increasing order then it looks
like you're still missing a write to CTR between the last two 8M entries
-- thus you'll overwrite the IMMR with the last 8M entry.  That was the
same problem that v1 fixed -- did that change get lost accidentally?
Oops, no, in fact I diffed from the version which was including it 
already. My mistake.


The hardware wants to decrement; why fight it?

I see your point.
However it is not clear in the documentation if the decrement is done 
really after the update, or at xTLB interrupt. So I propose to still set 
the CTR ourself as described in the reference Manual and not assume that 
the HW decrements it.



/* Now map the lower 8 Meg into the TLBs.  For this quick hack,
 * we can load the instruction and data TLB registers with the
 * same values.
@@ -825,6 +822,12 @@
mtspr   SPRN_MI_AP, r8
mtspr   SPRN_MD_AP, r8
   
+	/* Always pin the first 8 MB ITLB to prevent ITLB

+* misses while mucking around with SRR0/SRR1 in asm
+*/
+   lis r8, (MI_RSV4I | MI_RESETVAL)@h
+   mtspr   SPRN_MI_CTR, r8 /* Set instruction MMU control */

Entry 0 is not pinnable.

Here we are not trying to pin entry 0.

Sorry, misread the patch.


We are at step 8, we are setting
MI_RSV4I. At the same time, we set MD_CTR to 0 which is off the pinned
range, to be sure that we won't overwrite one of the pinned entries.

The main difference compared to the previous implementation is that
before, we were setting the RSV4I bit before loading the TLB entries.
Now, as defined in the Reference Manuals, we are doing it at the end.

Have you seen any evidence that it matters?

Not really.

Ok, propose a new patch in a few minutes.

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] powerpc 8xx: Fixing issue with CONFIG_PIN_TLB

[leroy christophe]

Le 20/09/2013 23:22, Scott Wood a écrit :

The hardware wants to decrement; why fight it?

>I see your point.
>However it is not clear in the documentation if the decrement is done
>really after the update, or at xTLB interrupt. So I propose to still set
>the CTR ourself as described in the reference Manual and not assume that
>the HW decrements it.

It says "every update" -- do you have any reason to believe that's
wrong?  It could be tested...


Ok. I just test it,  and I observe the following: As we have set the 
RSV4x bit, the CPU sets Mx_CTR to a value below 0x1c after each update:

* After writing entry 0x1c, Mx_CTR has value 0x1b
* After writing entry 0x1d, Mx_CTR has value 0x18
* After writing entry 0x1e, Mx_CTR has value 0x19
* After writing entry 0x1f, Mx_CTR has value 0x1a

Indeed the first version of my patch was complete, only the description 
was not fully correct.
So, in order to minimise code churn, I will re-submit my initial patch 
with a modified description.


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc 8xx: Fixing memory init issue with CONFIG_PIN_TLB

[leroy christophe]

Le 11/10/2013 17:13, Joakim Tjernlund a écrit :

"Linuxppc-dev"

wrote on 2013/10/11 14:56:40:

Activating CONFIG_PIN_TLB allows access to the 24 first Mbytes of memory

at

bootup instead of 8. It is needed for "big" kernels for instance when

activating

CONFIG_LOCKDEP_SUPPORT. This needs to be taken into account in init_32

too,

otherwise memory allocation soon fails after startup.

Signed-off-by: Christophe Leroy 

diff -ur linux-3.11.org/arch/powerpc/kernel/head_8xx.S

linux-3.11/arch/powerpc/kernel/head_8xx.S

--- linux-3.11.org/arch/powerpc/mm/init_32.c   2013-09-02

22:46:10.0 +0200

+++ linux-3.11/arch/powerpc/mm/init_32.c   2013-09-09 11:28:54.0

+0200

@@ -213,7 +213,12 @@
  */
 BUG_ON(first_memblock_base != 0);

+#ifdef CONFIG_PIN_TLB
+   /* 8xx can only access 24MB at the moment */
+   memblock_set_current_limit(min_t(u64, first_memblock_size,

0x0180));

+#else
 /* 8xx can only access 8MB at the moment */
 memblock_set_current_limit(min_t(u64, first_memblock_size,

0x0080));

+#endif
  }
  #endif /* CONFIG_8xx */

hmm, I think you should always map 24 MB (or less if RAM < 24 MB) and do
the same
in head_8xx.S.

Or to keep it simple, just always map at least 16 MB here and in
head_8xx.S, assuming
that 16 MB is min RAM for any 8xx system running 3.x kernels.
Yes we could do a more elaborated modification in the future. However it 
also has an impact on the boot loader, so I'm not sure we should make it 
the default without thinking twice.


In the meantime, my patch does take into account the existing situation 
where you have 8Mb by default and 24Mb when you activate CONFIG_PIN_TLB.
I see it as a bug fix and I believe we should include it at least in 
order to allow including in the stable releases.


Do you see any issue with this approach ?



Much of the need for pinning would go away if you adopted my 8MB pages
from 2.4 to 3.x

Jocke


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] powerpc 8xx: Fixing memory init issue with CONFIG_PIN_TLB

[leroy christophe]

Le 15/10/2013 22:33, Scott Wood a écrit :

On Tue, 2013-10-15 at 18:27 +0200, leroy christophe wrote:

Le 11/10/2013 17:13, Joakim Tjernlund a écrit :

"Linuxppc-dev"

wrote on 2013/10/11 14:56:40:

Activating CONFIG_PIN_TLB allows access to the 24 first Mbytes of memory

at

bootup instead of 8. It is needed for "big" kernels for instance when

activating

CONFIG_LOCKDEP_SUPPORT. This needs to be taken into account in init_32

too,

otherwise memory allocation soon fails after startup.

Signed-off-by: Christophe Leroy 

diff -ur linux-3.11.org/arch/powerpc/kernel/head_8xx.S

linux-3.11/arch/powerpc/kernel/head_8xx.S

--- linux-3.11.org/arch/powerpc/mm/init_32.c   2013-09-02

22:46:10.0 +0200

+++ linux-3.11/arch/powerpc/mm/init_32.c   2013-09-09 11:28:54.0

+0200

@@ -213,7 +213,12 @@
   */
  BUG_ON(first_memblock_base != 0);

+#ifdef CONFIG_PIN_TLB
+   /* 8xx can only access 24MB at the moment */
+   memblock_set_current_limit(min_t(u64, first_memblock_size,

0x0180));

+#else
  /* 8xx can only access 8MB at the moment */
  memblock_set_current_limit(min_t(u64, first_memblock_size,

0x0080));

+#endif
   }
   #endif /* CONFIG_8xx */

hmm, I think you should always map 24 MB (or less if RAM < 24 MB) and do
the same
in head_8xx.S.

Or to keep it simple, just always map at least 16 MB here and in
head_8xx.S, assuming
that 16 MB is min RAM for any 8xx system running 3.x kernels.

Yes we could do a more elaborated modification in the future. However it
also has an impact on the boot loader, so I'm not sure we should make it
the default without thinking twice.

In the meantime, my patch does take into account the existing situation
where you have 8Mb by default and 24Mb when you activate CONFIG_PIN_TLB.
I see it as a bug fix and I believe we should include it at least in
order to allow including in the stable releases.

Do you see any issue with this approach ?

The patch is fine, but I don't think it's stable material (BTW, if it
were, you should have marked it as such when submitting).  If I
understand the situation correctly, there's no regression, and nothing
fails to work with CONFIG_PIN_TLB that would have worked without it.
It's just making CONFIG_PIN_TLB more useful.



Yes the patch is definitly stable. How should I have mark it ?

The situation is that in 2010, I discovered that I was not able to start 
a big Kernel because of the 8Mb limit.
You told me (see attached mail) that in order to get rid of that limit I 
shall use CONFIG_PIN_TLB: it was the first step, it helped pass the 
memory zeroize at init, but it was not enough as I then got problems 
with the Device Tree being erased because being put inside the first 8Mb 
area too. Then I temporarely gave up at that time.


Recently I started again. After fixing my bootloader to get the device 
tree somewhere else, I discovered this 8Mb limit hardcoded in 
mm/init_32.c for the 8xx

With the patch I submitted I can now boot a kernel which is bigger than 8Mb.

So, I'm a bit lost here on what to do.
--- Begin Message ---
On Tue, 2013-10-15 at 18:27 +0200, leroy christophe wrote:
> Le 11/10/2013 17:13, Joakim Tjernlund a écrit :
> > "Linuxppc-dev"
> > 
> > wrote on 2013/10/11 14:56:40:
> >> Activating CONFIG_PIN_TLB allows access to the 24 first Mbytes of memory
> > at
> >> bootup instead of 8. It is needed for "big" kernels for instance when
> > activating
> >> CONFIG_LOCKDEP_SUPPORT. This needs to be taken into account in init_32
> > too,
> >> otherwise memory allocation soon fails after startup.
> >>
> >> Signed-off-by: Christophe Leroy 
> >>
> >> diff -ur linux-3.11.org/arch/powerpc/kernel/head_8xx.S
> > linux-3.11/arch/powerpc/kernel/head_8xx.S
> >> --- linux-3.11.org/arch/powerpc/mm/init_32.c   2013-09-02
> > 22:46:10.0 +0200
> >> +++ linux-3.11/arch/powerpc/mm/init_32.c   2013-09-09 11:28:54.0
> > +0200
> >> @@ -213,7 +213,12 @@
> >>   */
> >>  BUG_ON(first_memblock_base != 0);
> >>
> >> +#ifdef CONFIG_PIN_TLB
> >> +   /* 8xx can only access 24MB at the moment */
> >> +   memblock_set_current_limit(min_t(u64, first_memblock_size,
> > 0x0180));
> >> +#else
> >>  /* 8xx can only access 8MB at the moment */
> >>  memblock_set_current_limit(min_t(u64, first_memblock_size,
> > 0x0080));
> >> +#endif
> >>   }
> >>   #endif /* CONFIG_8xx */
> > hmm, I think you should always map 24 MB (or less if RAM < 24 MB) and do
> > the same
> > in head_8xx.S.
> >
> > Or to keep it simple, just always map at least 16 MB here and in
> > head_8xx.S, assuming
> > that 16 MB is min RAM for any 8xx syst

Re: [PATCH] Enhanced support for MPC8xx/8xxx watchdog

[leroy christophe]

Hi Wim,

Le 27/02/2013 20:52, Wim Van Sebroeck a écrit :

The rest of the code is OK and when above comments are corrected,
I will add the patch to improve the userspace experience.

Kind regards,
Wim.


Ok, I'll fix and re-submit my patch according to your comments.

Best regards
Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Linux 3.16: all my drivers on SPI bus report WARNING: at drivers/base/dd.c:286

[leroy christophe]

Since Linux 3.16, for all drivers tied to SPI bus, I get the following 
warning on a PowerPC 8xx.

It doesn't happen with Linux 3.15

What can be the reason / what should I look at ?

[3.086957] device: 'spi32766.1': device_add
[3.087179] bus: 'spi': add device spi32766.1
[3.087653] bus: 'spi': driver_probe_device: matched device 
spi32766.1 with driver lm70
[3.087743] bus: 'spi': really_probe: probing driver lm70 with device 
spi32766.1

[3.088014] [ cut here ]
[3.092348] WARNING: at drivers/base/dd.c:286
[3.096637] Modules linked in:
[3.099697] CPU: 0 PID: 25 Comm: kworker/u2:1 Tainted: G W 
3.16.1-s3k-drv-999-svn5771_knld-999 #158

[3.109610] Workqueue: deferwq deferred_probe_work_func
[3.114736] task: c787f020 ti: c790c000 task.ti: c790c000
[3.120062] NIP: c01df158 LR: c01df144 CTR: 
[3.124983] REGS: c790db30 TRAP: 0700   Tainted: GW 
(3.16.1-s3k-drv-999-svn5771_knld-999)

[3.134162] MSR: 00029032   CR: 22002082 XER: 2000
[3.140703]
[3.140703] GPR00: 0001 c790dbe0 c787f020 0044 0054 
0308 c056da0e 20737069
[3.140703] GPR08: 33323736 000ebfe0 0308 000ebfdf 22002082 
 c046c5a0 c046c608
[3.140703] GPR16: c046c614 c046c620 c046c62c c046c638 c046c648 
c046c654 c046c68c c046c6c4
[3.140703] GPR24:   0003 c0401aa0 c0596638 
c059662c c054e7a8 c7996800

[3.170102] NIP [c01df158] driver_probe_device+0xf8/0x334
[3.175431] LR [c01df144] driver_probe_device+0xe4/0x334
[3.180633] Call Trace:
[3.183093] [c790dbe0] [c01df144] driver_probe_device+0xe4/0x334 
(unreliable)

[3.190147] [c790dc10] [c01dd15c] bus_for_each_drv+0x7c/0xc0
[3.195741] [c790dc40] [c01df5fc] device_attach+0xcc/0xf8
[3.201076] [c790dc60] [c01dd6d4] bus_probe_device+0xb4/0xc4
[3.20] [c790dc80] [c01db9f8] device_add+0x270/0x564
[3.211923] [c790dcc0] [c0219e84] spi_add_device+0xc0/0x190
[3.217427] [c790dce0] [c021a79c] spi_register_master+0x720/0x834
[3.223455] [c790dd40] [c021cb48] of_fsl_spi_probe+0x55c/0x614
[3.229234] [c790dda0] [c01e0d2c] platform_drv_probe+0x30/0x74
[3.234987] [c790ddb0] [c01df18c] driver_probe_device+0x12c/0x334
[3.241008] [c790dde0] [c01dd15c] bus_for_each_drv+0x7c/0xc0
[3.246602] [c790de10] [c01df5fc] device_attach+0xcc/0xf8
[3.251937] [c790de30] [c01dd6d4] bus_probe_device+0xb4/0xc4
[3.257536] [c790de50] [c01de9d8] deferred_probe_work_func+0x98/0xe0
[3.263816] [c790de70] [c00305b8] process_one_work+0x18c/0x440
[3.269577] [c790dea0] [c0030a00] worker_thread+0x194/0x67c
[3.275105] [c790def0] [c0039198] kthread+0xd0/0xe4
[3.279911] [c790df40] [c000c6d0] ret_from_kernel_thread+0x5c/0x64
[3.285970] Instruction dump:
[3.288900] 80de 419e01d0 3b7b0038 3c60c046 7f65db78 38635264 
48211b99 813f00a0
[3.296559] 381f00a0 7d290278 3169 7c0b4910 <0f00> 93df0044 
7fe3fb78 4bfffd4d

[3.304401] ---[ end trace c75d3461bf9e2961 ]---
[3.309598] device: 'hwmon1': device_add
[3.310246] driver: 'lm70': driver_bound: bound to device 'spi32766.1'
[3.310342] bus: 'spi': really_probe: bound device spi32766.1 to 
driver lm70


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

BUG: Patch "Convert some mftb/mftbu into mfspr" breaks MPC885

[leroy christophe]

Scott,

The patch "Convert some mftb/mftbu into mfspr" 
(beb2dc0a7a84be003ce54e98b95d65cc66e6e536) breaks startup on MPC885.


The CPU traps (SoftwareEmulation trap) at sched_clock() when trying to 
read TBU with mfspr.


Reverting the patch solves the issue.

What's the prefered way to fix this ?

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

BUG: Patch "Convert some mftb/mftbu into mfspr" breaks MPC885

[leroy christophe]

Scott,

The patch "Convert some mftb/mftbu into mfspr" 
(beb2dc0a7a84be003ce54e98b95d65cc66e6e536 
) 
breaks startup on MPC885.


The CPU traps (SoftwareEmulation trap) at sched_clock() when trying to 
read TBU with mfspr.


Reverting the patch solves the issue.

What's the prefered way to fix this ?

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] watchdog: mpc8xxx_wdt convert to watchdog core

[leroy christophe]

Le 01/12/2013 20:38, Guenter Roeck a écrit :

On 11/30/2013 07:33 AM, Christophe Leroy wrote:

Convert mpc8xxx_wdt.c to the new watchdog API.

Signed-off-by: Christophe Leroy 

diff -ur a/drivers/watchdog/mpc8xxx_wdt.c 
b/drivers/watchdog/mpc8xxx_wdt.c
--- a/drivers/watchdog/mpc8xxx_wdt.c2013-05-11 22:57:46.0 
+0200
+++ b/drivers/watchdog/mpc8xxx_wdt.c2013-11-30 16:14:53.803495472 
+0100

@@ -72,28 +72,31 @@
   * to 0
   */
  static int prescale = 1;
-static unsigned int timeout_sec;

-static unsigned long wdt_is_open;
  static DEFINE_SPINLOCK(wdt_spinlock);

-static void mpc8xxx_wdt_keepalive(void)
+static int mpc8xxx_wdt_ping(struct watchdog_device *w)
  {
  /* Ping the WDT */
  spin_lock(&wdt_spinlock);
  out_be16(&wd_base->swsrr, 0x556c);
  out_be16(&wd_base->swsrr, 0xaa39);
  spin_unlock(&wdt_spinlock);
+return 0;


The return code is never checked, so you can make this function void.

watchdog .h expects an int function.
Wouldn't it be an error to make it void, if for instance in the future 
the return code is checked by the core ?



  }

+static struct watchdog_device mpc8xxx_wdt_dev;
  static void mpc8xxx_wdt_timer_ping(unsigned long arg);
-static DEFINE_TIMER(wdt_timer, mpc8xxx_wdt_timer_ping, 0, 0);
+static DEFINE_TIMER(wdt_timer, mpc8xxx_wdt_timer_ping, 0,
+(unsigned long)&mpc8xxx_wdt_dev);

  static void mpc8xxx_wdt_timer_ping(unsigned long arg)
  {
-mpc8xxx_wdt_keepalive();
+struct watchdog_device *w = (struct watchdog_device *)arg;
+
+mpc8xxx_wdt_ping(w);
  /* We're pinging it twice faster than needed, just to be sure. */
-mod_timer(&wdt_timer, jiffies + HZ * timeout_sec / 2);
+mod_timer(&wdt_timer, jiffies + HZ * w->timeout / 2);
  }

  static void mpc8xxx_wdt_pr_warn(const char *msg)
@@ -102,19 +105,9 @@
  reset ? "reset" : "machine check exception");
  }

-static ssize_t mpc8xxx_wdt_write(struct file *file, const char 
__user *buf,

- size_t count, loff_t *ppos)
-{
-if (count)
-mpc8xxx_wdt_keepalive();
-return count;
-}
-
-static int mpc8xxx_wdt_open(struct inode *inode, struct file *file)
+static int mpc8xxx_wdt_start(struct watchdog_device *w)
  {
  u32 tmp = SWCRR_SWEN;
-if (test_and_set_bit(0, &wdt_is_open))
-return -EBUSY;

  /* Once we start the watchdog we can't stop it */
  if (nowayout)


This code and the subsequent module_get can be removed; it is handled 
by the infrastructure.




@@ -132,59 +125,30 @@

  del_timer_sync(&wdt_timer);

-return nonseekable_open(inode, file);
+return 0;
  }

-static int mpc8xxx_wdt_release(struct inode *inode, struct file *file)
+static int mpc8xxx_wdt_stop(struct watchdog_device *w)
  {
-if (!nowayout)
-mpc8xxx_wdt_timer_ping(0);
-else
-mpc8xxx_wdt_pr_warn("watchdog closed");
-clear_bit(0, &wdt_is_open);
+mpc8xxx_wdt_timer_ping((unsigned long)w);


I really dislike this typecasting back and forth.

I think it would be better to move the mod_timer() call from 
mpc8xxx_wdt_timer_ping
into mpc8xxx_wdt_ping and call mpc8xxx_wdt_ping directly whenever 
possible.
From mpc8xxx_wdt_timer_ping you can then just call mpc8xxx_wdt_ping 
with the

typecasted parameter.

I'm not sure I understand what you mean.
mpc8xxx_wdt_ping() is called by the core when the user app writes into 
/dev/watchdog.

We don't want the set the timer in that case.
The timer is only used for when no user app has opened /dev/watchdog yet.




  return 0;
  }

-static long mpc8xxx_wdt_ioctl(struct file *file, unsigned int cmd,
-unsigned long arg)
-{
-void __user *argp = (void __user *)arg;
-int __user *p = argp;
-static const struct watchdog_info ident = {
-.options = WDIOF_KEEPALIVEPING,
-.firmware_version = 1,
-.identity = "MPC8xxx",
-};
-
-switch (cmd) {
-case WDIOC_GETSUPPORT:
-return copy_to_user(argp, &ident, sizeof(ident)) ? -EFAULT : 0;
-case WDIOC_GETSTATUS:
-case WDIOC_GETBOOTSTATUS:
-return put_user(0, p);
-case WDIOC_KEEPALIVE:
-mpc8xxx_wdt_keepalive();
-return 0;
-case WDIOC_GETTIMEOUT:
-return put_user(timeout_sec, p);
-default:
-return -ENOTTY;
-}
-}
+static struct watchdog_info mpc8xxx_wdt_info = {
+.options = WDIOF_KEEPALIVEPING,
+.firmware_version = 1,
+.identity = "MPC8xxx",
+};

-static const struct file_operations mpc8xxx_wdt_fops = {
-.owner= THIS_MODULE,
-.llseek= no_llseek,
-.write= mpc8xxx_wdt_write,
-.unlocked_ioctl= mpc8xxx_wdt_ioctl,
-.open= mpc8xxx_wdt_open,
-.release= mpc8xxx_wdt_release,
+static struct watchdog_ops mpc8xxx_wdt_ops = {
+.owner = THIS_MODULE,
+.start = mpc8xxx_wdt_start,
+.stop = mpc8xxx_wdt_stop,
  };

-static struct miscdevice mpc8xxx_wdt_miscdev = {
-.minor= WATCHDOG_MINOR,
-.name= "watchdog",
-.fops  

Re: [PATCH v2] powerpc 8xx: Loading kernels over 8Mbytes without CONFIG_PIN_TLB

[leroy christophe]

Le 10/12/2013 23:24, Scott Wood a écrit :

On Tue, 2013-12-10 at 12:29 +0100, Christophe Leroy wrote:

Today, the only way to load kernels whose size is greater than 8Mbytes is to
activate CONFIG_PIN_TLB. Otherwise, the physical memory initially mapped is
limited to 8Mbytes. This patch adds the capability to select the size of initial
memory between 8/16/24 Mbytes and this is regardless of whether CONFIG_PIN_TLB
is active or not. It allows to load "big" kernels (for instance when activating
CONFIG_LOCKDEP_SUPPORT) without having to activate CONFIG_PIN_TLB.

Signed-off-by: Christophe Leroy 

diff -ur a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -980,6 +980,29 @@
  config PIN_TLB
bool "Pinned Kernel TLBs (860 ONLY)"
depends on ADVANCED_OPTIONS && 8xx
+
+choice
+   prompt "Initial Data Memory Mapped on 8xx"
+   default 8xx_MAP_8M
+   depends on ADVANCED_OPTIONS && 8xx
+
+config 8xx_INIT_MAP_8M
+   bool "8 Mbytes"
+
+config 8xx_INIT_MAP_16M
+   bool "16 Mbytes"
+
+config 8xx_INIT_MAP_24M
+   bool "24 Mbytes"

Are you working with a loader that passes initial-mapped-area size in r7
as per ePAPR?  If so, we could rely on that at runtime.  If you're using
a non-ancient U-Boot, it should qualify here even if it's not fully
ePAPR compliant (it passes the value of the bootm_mapsize variable in
r7).
Ok, let me check that. But it means that the size of the kernel I can 
boot will depend on the initial memory mapped by uboot ? Isn't it 
limitating ?
Even if uboot only maps 8Mbytes, why couldn't I be allowed to boot a 
kernel having 10 Mbytes data if I have 32 Mbytes mem on the board ?
I don't like the idea of having to change the bootloader just because I 
want to activate CONFIG_LOCKDEP to debug my kernel.



-#ifdef CONFIG_PIN_TLB
+#if defined (CONFIG_8xx_INIT_MAP_16M) || defined (CONFIG_8xx_INIT_MAP_24M)
/* Map two more 8M kernel data pages.
*/
+#ifdef CONFIG_PIN_TLB
addir10, r10, 0x0100
mtspr   SPRN_MD_CTR, r10
+#endif
  
  	lis	r8, KERNELBASE@h	/* Create vaddr for TLB */

addis   r8, r8, 0x0080  /* Add 8M */
@@ -858,15 +860,19 @@
addis   r11, r11, 0x0080/* Add 8M */
mtspr   SPRN_MD_RPN, r11
  
+#ifdef CONFIG_8xx_INIT_MAP_24M

+#ifdef CONFIG_PIN_TLB
addir10, r10, 0x0100
mtspr   SPRN_MD_CTR, r10
+#endif

Are these ifdefs for CONFIG_PIN_TLB really needed?  It shouldn't harm
anything to use those entries even if they're not being pinned.


I'm not sure I understand your comment.
ifdef for CONFIG_PIN_TLB was already there before, but was enclosing the 
whole block, so 24 Mbytes were automatically mapped when you selected 
CONFIG_PIN_TLB and only 8 Mbytes were mapped when you didn't select 
CONFIG_PIN_TLB.
I reduced the scope of those ifdefs so that they now apply on the 
pinning only.


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2] powerpc 8xx: Loading kernels over 8Mbytes without CONFIG_PIN_TLB

[leroy christophe]

Le 11/12/2013 00:18, Scott Wood a écrit :

On Wed, 2013-12-11 at 00:05 +0100, leroy christophe wrote:

Le 10/12/2013 23:24, Scott Wood a écrit :

On Tue, 2013-12-10 at 12:29 +0100, Christophe Leroy wrote:

Today, the only way to load kernels whose size is greater than 8Mbytes is to
activate CONFIG_PIN_TLB. Otherwise, the physical memory initially mapped is
limited to 8Mbytes. This patch adds the capability to select the size of initial
memory between 8/16/24 Mbytes and this is regardless of whether CONFIG_PIN_TLB
is active or not. It allows to load "big" kernels (for instance when activating
CONFIG_LOCKDEP_SUPPORT) without having to activate CONFIG_PIN_TLB.

Signed-off-by: Christophe Leroy 

diff -ur a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -980,6 +980,29 @@
   config PIN_TLB
bool "Pinned Kernel TLBs (860 ONLY)"
depends on ADVANCED_OPTIONS && 8xx
+
+choice
+   prompt "Initial Data Memory Mapped on 8xx"
+   default 8xx_MAP_8M
+   depends on ADVANCED_OPTIONS && 8xx
+
+config 8xx_INIT_MAP_8M
+   bool "8 Mbytes"
+
+config 8xx_INIT_MAP_16M
+   bool "16 Mbytes"
+
+config 8xx_INIT_MAP_24M
+   bool "24 Mbytes"

Are you working with a loader that passes initial-mapped-area size in r7
as per ePAPR?  If so, we could rely on that at runtime.  If you're using
a non-ancient U-Boot, it should qualify here even if it's not fully
ePAPR compliant (it passes the value of the bootm_mapsize variable in
r7).

Ok, let me check that. But it means that the size of the kernel I can
boot will depend on the initial memory mapped by uboot ? Isn't it
limitating ?

The ePAPR IMA is supposed to be large enough to include the OS image,
device tree, etc.


Even if uboot only maps 8Mbytes, why couldn't I be allowed to boot a
kernel having 10 Mbytes data if I have 32 Mbytes mem on the board ?
I don't like the idea of having to change the bootloader just because I
want to activate CONFIG_LOCKDEP to debug my kernel.

Well, as noted, if you're using a non-ancient U-Boot you shouldn't have
to change anything because it already implements r7.  Now, the value of
r7 it passes might be a lie as far as ePAPR is concerned, since it's
supposed to represent what's actually mapped, but that's another matter.

Even fixing that wouldn't mean you have to change U-Boot every time the
kernel size changes; you'd just set it to something reasonable and be
done with it.  I'm not fond of adding kconfigs to hack around a problem
that has already been addressed in the standard that governs the PPC
boot process that U-Boot claims to implement.

Well, ok, that makes sense. I'll investigate around that solution.



-#ifdef CONFIG_PIN_TLB
+#if defined (CONFIG_8xx_INIT_MAP_16M) || defined (CONFIG_8xx_INIT_MAP_24M)
/* Map two more 8M kernel data pages.
*/
+#ifdef CONFIG_PIN_TLB
addir10, r10, 0x0100
mtspr   SPRN_MD_CTR, r10
+#endif
   
   	lis	r8, KERNELBASE@h	/* Create vaddr for TLB */

addis   r8, r8, 0x0080  /* Add 8M */
@@ -858,15 +860,19 @@
addis   r11, r11, 0x0080/* Add 8M */
mtspr   SPRN_MD_RPN, r11
   
+#ifdef CONFIG_8xx_INIT_MAP_24M

+#ifdef CONFIG_PIN_TLB
addir10, r10, 0x0100
mtspr   SPRN_MD_CTR, r10
+#endif

Are these ifdefs for CONFIG_PIN_TLB really needed?  It shouldn't harm
anything to use those entries even if they're not being pinned.

I'm not sure I understand your comment.
ifdef for CONFIG_PIN_TLB was already there before, but was enclosing the
whole block, so 24 Mbytes were automatically mapped when you selected
CONFIG_PIN_TLB and only 8 Mbytes were mapped when you didn't select
CONFIG_PIN_TLB.
I reduced the scope of those ifdefs so that they now apply on the
pinning only.

There wasn't previously an ifdef specifically around the setting of
SPRN_MD_CTR.  That's new.  There was an ifdef around the entire block,
which has gone away because you are now trying to map more than 8M
regardless of CONFIG_PIN_TLB, but that has nothing to do with whether
there should be an ifdef around SPRN_MD_CTR.


Euh, ok, but then we have to fix it in the whole function, not only in 
this block. Do you think it is worth doing it ?
Then we are back to the problem we discussed some months ago which is 
that the 8xx is decrementing the MD_CTR after writting a TLB entry, and 
if pinning is activated it decrements it out of the pinnable area. So it 
would still be needed to:

* Reposition it for each entry for when the pinning is activated
* Make sure we set it out of the area at the end when the pinning is not 
active hence the area not protected.
* Then we should probably reverse the entries, start at 31 and go down 
to 28 instead of going from 28 to 31 as do today.
But is it worth doing such a big change

Re: [PATCH v2] powerpc 8xx: Loading kernels over 8Mbytes without CONFIG_PIN_TLB

[leroy christophe]

Le 16/12/2013 23:57, Scott Wood a écrit :

On Wed, 2013-12-11 at 00:36 +0100, leroy christophe wrote:

Le 11/12/2013 00:18, Scott Wood a écrit :

There wasn't previously an ifdef specifically around the setting of
SPRN_MD_CTR.  That's new.  There was an ifdef around the entire block,
which has gone away because you are now trying to map more than 8M
regardless of CONFIG_PIN_TLB, but that has nothing to do with whether
there should be an ifdef around SPRN_MD_CTR.



Euh, ok, but then we have to fix it in the whole function, not only in
this block. Do you think it is worth doing it ?

Fix what in the whole function?  I was asking what harm there would be
if you just remove all the CONFIG_PIN_TLB ifdefs except around the
actual RSV4I setting -- do we really care what value goes in MD_CTR for
the non-pinned case, as long as it's different for each entry?



MD_CTR is decremented after each entry added.
However, the function populates entry 28, then 29, then 30, then 31. At 
the end MD_CTR has then value 30, ready to overide entry 30 then 29 then 
28 then 27 .


So I will remove all the CONFIG_PIN_TLB, but I'll also have to fix the 
value set in MD_CTR to start from 31, won't I ?


Do you have any comment/recommendation on my tentative v3 patch where I 
have tried to implement based on the use of r7 as you recommended ?


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH v2 07/11] powerpc/8xx: macro for handling CPU15 errata

[leroy christophe]

Le 20/01/2015 12:09, David Laight a écrit :

 From Christophe Leroy

Having a macro will help keep clear code.

It might remove an #if but it doesn't really help.
All it means is that anyone reading the code has to hunt for
the definition before proceeding.

Some comment about what (and why) the extra code is needed
might help.
The main reason is because of patch 09/11 where we have to duplicate 
this code. I prefer to just duplicate one line rather than duplicate the 
whole code (especially because in v1 of the PATCHset, it was duplicated 
twice):


-DO_8xx_CPU15(r11, r10)
[...]
#ifdef CONFIG_MODULES
[...]
+DO_8xx_CPU15(r10, r11)
[...]
+#else
+mfsprr10, SPRN_SRR0/* Get effective address of fault */
+DO_8xx_CPU15(r11, r10)

Is this approach wrong ?




...

+
+#ifdef CONFIG_8xx_CPU15
+#define DO_8xx_CPU15(tmp, addr)\
+   additmp, addr, PAGE_SIZE;   \
+   tlbie   tmp;\
+   additmp, addr, PAGE_SIZE;   \

You've even transcribed this incorrectly.

Oops


Clearly not tested :-)
Indeed it's been tested, but tests can only show that the code is not 
worth than before.

This code is there to fix a chip errata which (almost?) never happens.
In my production version, I have not activated this errata, and he have 
never seen the problem on any of the more than 200 boards that have run 
for at least 4 years.


Christophe


David


+   tlbie   tmp
+#else
+#define DO_8xx_CPU15(tmp, addr)
+#endif
+
  InstructionTLBMiss:
  #ifdef CONFIG_8xx_CPU6
mtspr   SPRN_DAR, r3
@@ -304,12 +315,7 @@ InstructionTLBMiss:
EXCEPTION_PROLOG_0
mtspr   SPRN_SPRG_SCRATCH2, r10
mfspr   r10, SPRN_SRR0  /* Get effective address of fault */
-#ifdef CONFIG_8xx_CPU15
-   addir11, r10, PAGE_SIZE
-   tlbie   r11
-   addir11, r10, -PAGE_SIZE
-   tlbie   r11
-#endif
+   DO_8xx_CPU15(r11, r10)

/* If we are faulting a kernel address, we have to use the
 * kernel page tables.
--
2.1.0

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

cacheable_memcpy() versus memcpy() ==> 8% improvment on FTP throughput

[leroy christophe]

In powerpc32 architecture we have a function called cacheable_memcpy() 
which does same thing as memcpy() but using dcbz/dcbt instructions for 
an optimised copy (just like __copy_tofrom_user())
What seems strange is that it is almost nowhere used (only used in 
drivers/net/ethernet/ibm/emac/core.c)


I replaced all memcpy() in include/linux/skbuff.h and net/core/skbuff.c 
by cacheable_memcpy() and I get around 8% improvement on FTP throughput 
on MPC885.


What could be done to generalise the use of cacheable_memcpy() instead 
of memcpy() whenever possible ?

Indeed, in order to use cacheable_memcpy(), we need
* The destination to be cacheable
* The source and destination to not overlap on the same cachelines

Could we check, when calling memcpy(), whether the destination is 
cacheable or not, and if yes redirect the call to cacheable_memcpy() ?

How can we check that ?

Christophe

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[QUESTION,RFC] cacheable_memcpy() versus memcpy() ==> 8% improvment on FTP throughput

[leroy christophe]

In powerpc32 architecture there is a function called cacheable_memcpy() 
which does same thing as memcpy() but using dcbz/dcbt instructions for 
an optimised copy (just like __copy_tofrom_user())
What seems strange is that it is almost nowhere used (only used in 
drivers/net/ethernet/ibm/emac/core.c)


For a try I replaced all memcpy() in include/linux/skbuff.h and 
net/core/skbuff.c by cacheable_memcpy() and I got around 8% improvement 
on FTP throughput on MPC885.


What could be done to generalise the use of cacheable_memcpy() instead 
of memcpy() whenever possible ?

Indeed, in order to use cacheable_memcpy(), we need
* The destination to be cacheable
* The source and destination to not overlap on the same cachelines

Could we check, when calling memcpy(), whether the destination is 
cacheable or not, and if yes redirect the call to cacheable_memcpy() ?

How can we check that ?

Christophe

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] spi: fsl-spi: use of_iomap() to map parameter ram on CPM1

[leroy christophe]

Le 03/03/2015 19:44, Mark Brown a écrit :

On Thu, Feb 26, 2015 at 05:11:42PM +0100, Christophe Leroy wrote:

On CPM2, the SPI parameter RAM is dynamically allocated in the dualport RAM
whereas in CPM1, it is statically allocated to a default address with
capability to relocate it somewhere else via the use of CPM micropatch.
The address of the parameter RAM is given by the boot loader and expected
to be mapped via of_iomap()

Why are we using of_iomap() rather than a generic I/O mapping function
here?

Euh ...

because all drivers for powerpc seems to be using of_iomap(), as on 
powerpc the HW is described by the bootloader in a OF device tree.
Today, of_iomap() is at least used in FSL SPI, FSL UART, SPI mpc52xx, 
UART mpc52xx, i2c-mpc, i2c-cpm, freescale ethernet drivers, etc 


Is it not correct ?

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 0/17] crypto: talitos - Add support for SEC1

[leroy christophe]

Le 06/03/2015 01:21, Kim Phillips a écrit :

On Thu, 5 Mar 2015 17:46:05 +0100
Christophe Leroy  wrote:


[15/17] crypto: talitos - Implementation of SEC1

...


[16/17] crypto: talitos - SEC1 bugs on 0 data hash
[17/17] crypto: talitos - Update DT bindings with SEC1

This patchseries doesn't apply, at least on top of Herbert's
cryptodev-2.6 tree, as of today:

Applying: crypto: talitos - Implementation of SEC1
error: patch failed: drivers/crypto/talitos.c:655
error: drivers/crypto/talitos.c: patch does not apply

It was applying ok on linux-next as of yesterday.
I will rebase the serie on cryptodev-2.6

Christophe

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 0/17] crypto: talitos - Add support for SEC1

[leroy christophe]

Le 06/03/2015 01:28, Herbert Xu a écrit :

On Thu, Mar 05, 2015 at 06:21:01PM -0600, Kim Phillips wrote:

On Thu, 5 Mar 2015 17:46:05 +0100
Christophe Leroy  wrote:


[15/17] crypto: talitos - Implementation of SEC1

...


[16/17] crypto: talitos - SEC1 bugs on 0 data hash
[17/17] crypto: talitos - Update DT bindings with SEC1

This patchseries doesn't apply, at least on top of Herbert's
cryptodev-2.6 tree, as of today:

Applying: crypto: talitos - Implementation of SEC1
error: patch failed: drivers/crypto/talitos.c:655
error: drivers/crypto/talitos.c: patch does not apply

Also the patches are coming in a random order.  Please send them
one at a time to ensure proper ordering.

Thanks,
Kim, I have now tried on top of cryptodev-2.6 tree, and for me it works 
(see below).

Do I clone cryptodev-2.6 from the wrong place ?
On that clone, the latest commit on talitos.c is commit 
5be4d4c94b1f98b839344fda7a8752a4a09d0ef5 "crypto: replace 
scatterwalk_sg_next with sg_next"


[root@localhost ~]# git clone 
https://www.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git

Cloning into 'cryptodev-2.6'...
remote: Counting objects: 4043448, done.
remote: Compressing objects: 100% (682829/682829), done.
Receiving objects: 100% (4043448/4043448), 893.52 MiB | 258.00 KiB/s, done.
remote: Total 4043448 (delta 3330215), reused 4043104 (delta 3329977)
Resolving deltas: 100% (3330215/3330215), done.
Checking connectivity... done.
Checking out files: 100% (48971/48971), done.
[root@localhost ~]# cd cryptodev-2.6/
[root@localhost cryptodev-2.6]# git branch test
[root@localhost cryptodev-2.6]# git checkout test
Switched to branch 'test'
[root@localhost cryptodev-2.6]# git am 
/root/gen/trunk/submitted_patches/talitos/0*
Applying: crypto: talitos - base address for Execution Units and macro 
for ISR masks

Applying: crypto: talitos - Externalise specific SEC2 reset actions
Applying: crypto: talitos - Use zero entry to init descriptors ptrs to zero
Applying: crypto: talitos - Refactor the sg in/out chain allocation
Applying: crypto: talitos - isolate scatter/gather handling for ahash
Applying: crypto: talitos - Add talitos2.c to isolate SEC2 specific 
functions

Applying: crypto: talitos - Split talitos.h into 2 parts
Applying: crypto: talitos - Deport SEC2 error handling
Applying: crypto: talitos - Move reset/init helpers into talitos2.h
Applying: crypto: talitos - Move interrupt related macros in talitos2.h
Applying: crypto: talitos - Move hash chain handling into talitos2.h
Applying: crypto: talitos - Define compatible in talitos2.h instead of 
talitos.c

Applying: crypto: talitos - move sg_count() helper into talitos.h
Applying: crypto: talitos - Add a helper function to clear j_extent field
Applying: crypto: talitos - Implementation of SEC1
Applying: crypto: talitos - SEC1 bugs on 0 data hash
Applying: crypto: talitos - Update DT bindings with SEC1
[root@localhost cryptodev-2.6]#

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] spi: fsl-spi: use of_iomap() to map parameter ram on CPM1

[leroy christophe]

Le 06/03/2015 12:44, Mark Brown a écrit :

On Wed, Mar 04, 2015 at 09:00:39AM +0100, leroy christophe wrote:

Le 03/03/2015 19:44, Mark Brown a écrit :

Why are we using of_iomap() rather than a generic I/O mapping function
here?

because all drivers for powerpc seems to be using of_iomap(), as on powerpc
the HW is described by the bootloader in a OF device tree.
Today, of_iomap() is at least used in FSL SPI, FSL UART, SPI mpc52xx, UART
mpc52xx, i2c-mpc, i2c-cpm, freescale ethernet drivers, etc 
Is it not correct ?

It's legacy, all that code is really old.  Modern code is written in as
architecture and firmware neutral a fashion as possible to make things
more consistent and maintainable.

This patch is only a small bug fix.
That driver already contains calls to of_iomap() and other related of_ 
functions.

Is it worth rewriting the driver for just a small bug fix ?

Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Strange reports of perf events on powerpc 83xx

[leroy christophe]

Hi,

Has anybody already used 'perf' tool on powerpc MPC83xx ?

I have been succesfully using perf on MPC8xx, but on MPC83xx I get 
something strange.


perf record/report reports addresses on user stack, as if it was mixing 
up D accesses and I accesses.


Any idea of what the problem can be ?

# Samples: 8K of event 'cpu-clock'
# Event count (approx.): 219600
#
# Overhead  Command   Shared Object   Symbol
#     .. 


#
 2.62%  perf_reseau4  libpthread-2.18.so  [.] __libc_send
 2.56%  perf_reseau4  [kernel.kallsyms]   [k] __ip_make_skb
 1.62%  perf_reseau4  [kernel.kallsyms]   [k] __ip_append_data.isra.39
 1.55%  perf_reseau4  [kernel.kallsyms]   [k] ip_finish_output
 1.33%  perf_reseau4  [unknown]   [k] 0x7d94
 1.33%  perf_reseau4  [unknown]   [k] 0x7d95
 1.28%  perf_reseau4  [unknown]   [k] 0x7d97
 1.26%  perf_reseau4  [unknown]   [k] 0x7da3
 1.24%  perf_reseau4  [unknown]   [k] 0x7d98
 1.22%  perf_reseau4  [unknown]   [k] 0x7d92
 1.22%  perf_reseau4  [unknown]   [k] 0x7d9b
 1.22%  perf_reseau4  [unknown]   [k] 0x7daa
 1.21%  perf_reseau4  [unknown]   [k] 0x7d96
 1.18%  perf_reseau4  [unknown]   [k] 0x7da7
 1.17%  perf_reseau4  [unknown]   [k] 0x7d8d
 1.17%  perf_reseau4  [unknown]   [k] 0x7d99
 1.13%  perf_reseau4  [unknown]   [k] 0x7d90
 1.13%  perf_reseau4  [unknown]   [k] 0x7da2
 1.12%  perf_reseau4  [kernel.kallsyms]   [k] __local_bh_enable_ip
 1.12%  perf_reseau4  [unknown]   [k] 0x7d9c
 1.12%  perf_reseau4  [unknown]   [k] 0x7d9e
 1.10%  perf_reseau4  [unknown]   [k] 0x7da0
 1.08%  perf_reseau4  [unknown]   [k] 0x7d9f
 1.08%  perf_reseau4  [unknown]   [k] 0x7da6
 1.05%  perf_reseau4  [unknown]   [k] 0x7da8
 1.02%  perf_reseau4  [unknown]   [k] 0x7d9a
 1.01%  perf_reseau4  [unknown]   [k] 0x7db0
 1.00%  perf_reseau4  [unknown]   [k] 0x7d89
 1.00%  perf_reseau4  [unknown]   [k] 0x7d8b
 1.00%  perf_reseau4  [unknown]   [k] 0x7dac


Christophe
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: powerpc32 boot crash in 5.1-rc1

[LEROY Christophe]

Meelis Roos  a écrit :

While 5.0.0 worked fine on my PowerMac G4, 5.0 + git (unknown rev as  
of Mar 13), 5.0.0-11520-gf261c4e and todays git all fail to boot.


The problem seems to be in page fault handler in load_elf_binary()  
of init process.


The patch at https://patchwork.ozlabs.org/patch/1053385/ should fix it

Christophe



Two different screenshots are attached (let's see if they come through).

--
Meelis Roos 

Re: powerpc32 boot crash in 5.1-rc1

[LEROY Christophe]

Meelis Roos  a écrit :

While 5.0.0 worked fine on my PowerMac G4, 5.0 + git (unknown rev  
as of Mar 13), 5.0.0-11520-gf261c4e and todays git all fail to boot.


The problem seems to be in page fault handler in load_elf_binary()  
of init process.


The patch at https://patchwork.ozlabs.org/patch/1053385/ should fix it


Tested it - yes, it fixes the boot.


Thanks for testing. It will be merged in 5.1-rc2

Christophe



--
Meelis Roos 

Re: [PATCH] compiler: allow all arches to enable CONFIG_OPTIMIZE_INLINING

[LEROY Christophe]

Arnd Bergmann  a écrit :


On Wed, Mar 20, 2019 at 10:41 AM Arnd Bergmann  wrote:


I've added your patch to my randconfig test setup and will let you
know if I see anything noticeable. I'm currently testing clang-arm32,
clang-arm64 and gcc-x86.


This is the only additional bug that has come up so far:

`.exit.text' referenced in section `.alt.smp.init' of
drivers/char/ipmi/ipmi_msghandler.o: defined in discarded section
`exit.text' of drivers/char/ipmi/ipmi_msghandler.o


Wouldn't it be useful to activate -Winline gcc warning to ease finding  
out problematic usage of inline keyword ?


Christophe



diff --git a/arch/arm/kernel/atags.h b/arch/arm/kernel/atags.h
index 201100226301..84b12e33104d 100644
--- a/arch/arm/kernel/atags.h
+++ b/arch/arm/kernel/atags.h
@@ -5,7 +5,7 @@ void convert_to_tag_list(struct tag *tags);
 const struct machine_desc *setup_machine_tags(phys_addr_t __atags_pointer,
unsigned int machine_nr);
 #else
-static inline const struct machine_desc *
+static __always_inline const struct machine_desc *
 setup_machine_tags(phys_addr_t __atags_pointer, unsigned int machine_nr)
 {
early_print("no ATAGS support: can't continue\n");

Re: [PATCH] compiler: allow all arches to enable CONFIG_OPTIMIZE_INLINING

[LEROY Christophe]

Masahiro Yamada  a écrit :


Commit 60a3cdd06394 ("x86: add optimized inlining") introduced
CONFIG_OPTIMIZE_INLINING, but it has been available only for x86.

The idea is obviously arch-agnostic although we need some code fixups.
This commit moves the config entry from arch/x86/Kconfig.debug to
lib/Kconfig.debug so that all architectures (except MIPS for now) can
benefit from it.

At this moment, I added "depends on !MIPS" because fixing 0day bot reports
for MIPS was complex to me.

I tested this patch on my arm/arm64 boards.

This can make a huge difference in kernel image size especially when
CONFIG_OPTIMIZE_FOR_SIZE is enabled.

For example, I got 3.5% smaller arm64 kernel image for v5.1-rc1.

  dec   file
  18983424  arch/arm64/boot/Image.before
  18321920  arch/arm64/boot/Image.after

This also slightly improves the "Kernel hacking" Kconfig menu.
Commit e61aca5158a8 ("Merge branch 'kconfig-diet' from Dave Hansen')
mentioned this config option would be a good fit in the "compiler option"
menu. I did so.

I fixed up some files to avoid build warnings/errors.

[1] arch/arm64/include/asm/cpufeature.h

In file included from ././include/linux/compiler_types.h:68,
 from :
./arch/arm64/include/asm/jump_label.h: In function 'cpus_have_const_cap':
./include/linux/compiler-gcc.h:120:38: warning: asm operand 0  
probably doesn't match constraints

 #define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)
  ^~~
./arch/arm64/include/asm/jump_label.h:32:2: note: in expansion of  
macro 'asm_volatile_goto'

  asm_volatile_goto(
  ^
./include/linux/compiler-gcc.h:120:38: error: impossible constraint in 'asm'
 #define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)
  ^~~
./arch/arm64/include/asm/jump_label.h:32:2: note: in expansion of  
macro 'asm_volatile_goto'

  asm_volatile_goto(
  ^

[2] arch/mips/kernel/cpu-bugs64.c

arch/mips/kernel/cpu-bugs64.c: In function 'mult_sh_align_mod.constprop':
arch/mips/kernel/cpu-bugs64.c:33:2: error: asm operand 1 probably  
doesn't match constraints [-Werror]

  asm volatile(
  ^~~
arch/mips/kernel/cpu-bugs64.c:33:2: error: asm operand 1 probably  
doesn't match constraints [-Werror]

  asm volatile(
  ^~~
arch/mips/kernel/cpu-bugs64.c:33:2: error: impossible constraint in 'asm'
  asm volatile(
  ^~~
arch/mips/kernel/cpu-bugs64.c:33:2: error: impossible constraint in 'asm'
  asm volatile(
  ^~~

[3] arch/powerpc/mm/tlb-radix.c

arch/powerpc/mm/tlb-radix.c: In function '__radix__flush_tlb_range_psize':
arch/powerpc/mm/tlb-radix.c:104:2: error: asm operand 3 probably  
doesn't match constraints [-Werror]

  asm volatile(PPC_TLBIEL(%0, %4, %3, %2, %1)
  ^~~
arch/powerpc/mm/tlb-radix.c:104:2: error: impossible constraint in 'asm'
  CC  arch/powerpc/perf/hv-gpci.o

[4] arch/s390/include/asm/cpacf.h

In file included from arch/s390/crypto/des_s390.c:19:
./arch/s390/include/asm/cpacf.h: In function 'cpacf_query':
./arch/s390/include/asm/cpacf.h:170:2: warning: asm operand 3  
probably doesn't match constraints

  asm volatile(
  ^~~
./arch/s390/include/asm/cpacf.h:170:2: error: impossible constraint in 'asm'

[5] arch/powerpc/kernel/prom_init.c

WARNING: vmlinux.o(.text.unlikely+0x20): Section mismatch in  
reference from the function .prom_getprop() to the function  
.init.text:.call_prom()

The function .prom_getprop() references
the function __init .call_prom().
This is often because .prom_getprop lacks a __init
annotation or the annotation of .call_prom is wrong.

WARNING: vmlinux.o(.text.unlikely+0x3c): Section mismatch in  
reference from the function .prom_getproplen() to the function  
.init.text:.call_prom()

The function .prom_getproplen() references
the function __init .call_prom().
This is often because .prom_getproplen lacks a __init
annotation or the annotation of .call_prom is wrong.

[6] drivers/mtd/nand/raw/vf610_nfc.c

drivers/mtd/nand/raw/vf610_nfc.c: In function ‘vf610_nfc_cmd’:
drivers/mtd/nand/raw/vf610_nfc.c:455:3: warning: ‘offset’ may be  
used uninitialized in this function [-Wmaybe-uninitialized]

   vf610_nfc_rd_from_sram(instr->ctx.data.buf.in + offset,
   ^~~
nfc->regs + NFC_MAIN_AREA(0) + offset,
~~
trfr_sz, !nfc->data_access);
~~~

[7] arch/arm/kernel/smp.c

arch/arm/kernel/smp.c: In function ‘raise_nmi’:
arch/arm/kernel/smp.c:522:2: warning: array subscript is above array  
bounds [-Warray-bounds]

  trace_ipi_raise_rcuidle(target, ipi_types[ipinr]);
  ^

The fixup is not included in this. The patch is available in ML:

http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/409393.html

Signed-off-by: Masahiro Yamada 
---

 arch/arm64/include/asm/cpufeature.h |  4 ++--
 arch/mips/

Re: [PATCH v2] powerpc/tm: Print 64-bits MSR

[LEROY Christophe]

Breno Leitao  a écrit :


Hi,

On 08/07/2018 02:15 PM, Christophe LEROY wrote:

Le 07/08/2018 à 15:35, Breno Leitao a écrit :

On a kernel TM Bad thing program exception, the Machine State Register
(MSR) is not being properly displayed. The exception code dumps a 32-bits
value but MSR is a 64 bits register for all platforms that have HTM
enabled.

This patch dumps the MSR value as a 64-bits value instead of 32 bits. In
order to do so, the 'reason' variable could not be used, since it trimmed
MSR to 32-bits (int).


reason is not always regs->msr, see get_reason(), allthough in your  
case it is.


I think it would be better to change 'reason' to 'unsigned long' instead of
replacing it by regs->msr for the printk.


That was my initial approach, but this code seems to run on 32 bits system,
and I do not want to change the whole 'reason' bit width without having a 32
bits to test, at least.


But 'unsigned long' is still 32 bits on ppc32, so it makes no  
difference with 'unsigned int'

And I will test it for you if needed

Christophe



Also, it is a bit weird doing something as:

printk("(msr 0x%lx)", reason);

I personally think that the follow code is much more readable:

printk(" (msr 0x%lx)...", regs->msr);

Re: [PATCH net] powerpc: use big endian to hash len and proto in csum_ipv6_magic

[LEROY Christophe]

Xin Long  a écrit :


The function csum_ipv6_magic doesn't convert len and proto to big
endian before doing ipv6 csum hash, which is not consistent with
RFC and other arches.

Jianlin found it when ICMPv6 packets from other hosts were dropped
in the powerpc64 system.

This patch is to fix it by using instruction 'lwbrx' to do this
conversion in powerpc32/64 csum_ipv6_magic.

Fixes: e9c4943a107b ("powerpc: Implement csum_ipv6_magic in assembly")
Reported-by: Jianlin Shi 
Signed-off-by: Xin Long 
---
 arch/powerpc/lib/checksum_32.S | 4 
 arch/powerpc/lib/checksum_64.S | 4 
 2 files changed, 8 insertions(+)

diff --git a/arch/powerpc/lib/checksum_32.S b/arch/powerpc/lib/checksum_32.S
index aa22406..7d3446e 100644
--- a/arch/powerpc/lib/checksum_32.S
+++ b/arch/powerpc/lib/checksum_32.S
@@ -325,6 +325,10 @@ _GLOBAL(csum_ipv6_magic)
adder0, r0, r9
lwz r11, 12(r4)
adder0, r0, r10
+   STWX_BE r5, 0, r1
+   lwz r5, 0(r1)
+   STWX_BE r6, 0, r1
+   lwz r6, 0(r1)


PPC32 doesn't support little endian, so nothing to do here.


add r5, r5, r6  /* assumption: len + proto doesn't carry */
adder0, r0, r11
adder0, r0, r5
diff --git a/arch/powerpc/lib/checksum_64.S b/arch/powerpc/lib/checksum_64.S
index 886ed94..302e732 100644
--- a/arch/powerpc/lib/checksum_64.S
+++ b/arch/powerpc/lib/checksum_64.S
@@ -439,6 +439,10 @@ EXPORT_SYMBOL(csum_partial_copy_generic)
 _GLOBAL(csum_ipv6_magic)
ld  r8, 0(r3)
ld  r9, 8(r3)
+   STWX_BE r5, 0, r1
+   lwz r5, 0(r1)
+   STWX_BE r6, 0, r1
+   lwz r6, 0(r1)
add r5, r5, r6


This is overkill. For LE it should be enough to rotate r5 by 8 bits  
after the sum. Best place to do it would be after ld r11 I think.


Christophe


addcr0, r8, r9
ld  r10, 0(r4)
--
2.1.0

Re: [PATCH v3 06/24] powerpc/mm: properly set PAGE_KERNEL flags in ioremap()

[LEROY Christophe]

Michael Ellerman  a écrit :


Christophe Leroy  writes:


Set PAGE_KERNEL directly in the caller and do not rely on a
hack adding PAGE_KERNEL flags when _PAGE_PRESENT is not set.

As already done for PPC64, use pgprot_cache() helpers instead of
_PAGE_XXX flags in PPC32 ioremap() derived functions.

Signed-off-by: Christophe Leroy 


Something in here is breaking my p5020ds (both 32-bit and 64-bit):

# first bad commit: [cb9220cc18345a2a1ec287752e576a2c88fa4a2b]  
powerpc/mm: properly set PAGE_KERNEL flags in ioremap()


Strange, it is calling ioremap_prot(), it should have been calling  
ioremap_cache() hence ioremap() since patch 4  of the serie. Did patch  
4 apply correctly ?


Christophe



64-bit:

  io scheduler mq-deadline registered
  io scheduler kyber registered
  pcieport :00:00.0: AER enabled with IRQ 482
  pcieport 2000:00:00.0: AER enabled with IRQ 480
  Unable to handle kernel paging request for data at address  
0x88008008

  Faulting instruction address: 0xc00152e4
  Oops: Kernel access of bad area, sig: 11 [#1]
  BE SMP NR_CPUS=32 CoreNet Generic
  Modules linked in:
  CPU: 1 PID: 1 Comm: swapper/0 Not tainted  
4.19.0-rc3-gcc-7.3.1-00135-gcb9220cc1834 #16

  NIP:  c00152e4 LR: c05173b8 CTR: 0010
  REGS: c000f30eb420 TRAP: 0300   Not tainted   
(4.19.0-rc3-gcc-7.3.1-00135-gcb9220cc1834)

  MSR:  80029000   CR: 24000224  XER: 
  DEAR: 88008008 ESR: 0080 IRQMASK: 0
  GPR00: c05173a0 c000f30eb6a0 c0f86e00 88008008
  GPR04:  0040 0ffbff04 0008
  GPR08:  0010  0080
  GPR12: 84000422 c00037c0 c000263c 
  GPR16:    
  GPR20:    
  GPR24: c0dbe0a0 c0d41bf8 88008008 c00089a8
  GPR28: c000f3492400 c000f3492410 0040 c0ff0a18
  NIP [c00152e4] ._memset_io+0x6c/0x9c
  LR [c05173b8] .fsl_qman_probe+0x188/0x918
  Call Trace:
  [c000f30eb6a0] [c05173a0] .fsl_qman_probe+0x170/0x918  
(unreliable)

  [c000f30eb740] [c05797dc] .platform_drv_probe+0x58/0xac
  [c000f30eb7c0] [c05772b0] .really_probe+0x2a8/0x380
  [c000f30eb860] [c05776f0] .__driver_attach+0x138/0x13c
  [c000f30eb8f0] [c0574550] .bus_for_each_dev+0x9c/0x110
  [c000f30eb9a0] [c0576a18] .driver_attach+0x24/0x38
  [c000f30eba10] [c05761ec] .bus_add_driver+0x16c/0x2ac
  [c000f30ebab0] [c0578194] .driver_register+0x88/0x178
  [c000f30ebb30] [c0579770] .__platform_driver_register+0x48/0x5c
  [c000f30ebba0] [c0d85744] .fsl_qman_driver_init+0x1c/0x30
  [c000f30ebc10] [c0002374] .do_one_initcall+0x70/0x258
  [c000f30ebcf0] [c0d4a244] .kernel_init_freeable+0x340/0x43c
  [c000f30ebdb0] [c0002658] .kernel_init+0x1c/0x130
  [c000f30ebe30] [c9e4] .ret_from_kernel_thread+0x58/0x74
  Instruction dump:
  4e800020 2ba50003 40dd003c 3925fffc 5488402e 7929f082 7d082378 39290001
  550a801e 7d2903a6 7d4a4378 794a0020 <9143> 38630004 4200fff8 70a50003
  ---[ end trace 372a57fd67efb6fe ]---

32 bit:

  [1.076133] pcieport 2000:02:00.0: AER enabled with IRQ 480
  [1.082106] Unable to handle kernel paging request for data at  
address 0xf110

  [1.089488] Faulting instruction address: 0xc0011c80
  [1.094437] Oops: Kernel access of bad area, sig: 11 [#1]
  [1.099816] BE SMP NR_CPUS=24 CoreNet Generic
  [1.104157] Modules linked in:
  [1.107197] CPU: 0 PID: 1 Comm: swapper/0 Not tainted  
4.19.0-rc3-gcc7x-g8f0c636b0542 #1

  [1.115181] NIP:  c0011c80 LR: c058f970 CTR: 0010
  [1.120216] REGS: e8107c80 TRAP: 0300   Not tainted   
(4.19.0-rc3-gcc7x-g8f0c636b0542)

  [1.128026] MSR:  00029002   CR: 24000284  XER: 
  [1.134190] DEAR: f110 ESR: 0080
  [1.134190] GPR00: c058f958 e8107d30 e8108000 f110   
0040 e8107cd8 e8107d0c
  [1.134190] GPR08:  0010  ff00 24000282  
 c0003340 
  [1.134190] GPR16:     c0d64410  
c0edb34c c0ec6700 0007
  [1.134190] GPR24: c0ef f110 efffcab8 c0ef e8231c00  
e8231c10 0040003f c101d290

  [1.171519] NIP [c0011c80] _memset_io+0x90/0xd0
  [1.176030] LR [c058f970] fsl_qman_probe+0x190/0x8c0
  [1.180975] Call Trace:
  [1.183410] [e8107d30] [c001f6c0] ioremap_prot+0x40/0x50 (unreliable)
  [1.189830] [e8107d40] [c058f958] fsl_qman_probe+0x178/0x8c0
  [1.195475] [e8107d70] [c0600894] platform_drv_probe+0x54/0xb0
  [1.201287] [e8107d90] [c05fe15c] really_probe+0x28c/0x350
  [1.206756] [e8107dc0] [c05fe73c] __dr

Re: [PATCH v3 06/24] powerpc/mm: properly set PAGE_KERNEL flags in ioremap()

[LEROY Christophe]

Michael Ellerman  a écrit :


Michael Ellerman  writes:


Christophe Leroy  writes:


Set PAGE_KERNEL directly in the caller and do not rely on a
hack adding PAGE_KERNEL flags when _PAGE_PRESENT is not set.

As already done for PPC64, use pgprot_cache() helpers instead of
_PAGE_XXX flags in PPC32 ioremap() derived functions.

Signed-off-by: Christophe Leroy 


Something in here is breaking my p5020ds (both 32-bit and 64-bit):


Oh duh.

That's because I didn't take patch 4.

It didn't have any acks, but I guess I'll just merge it rather than
breaking things.


Yes indeed. Maybe should I have followed it more carrefully to ensure  
it gets an ack.


Thanks
Christophe

Re: [PATCH v3 06/24] powerpc/mm: properly set PAGE_KERNEL flags in ioremap()

[LEROY Christophe]

LEROY Christophe  a écrit :


Michael Ellerman  a écrit :


Christophe Leroy  writes:


Set PAGE_KERNEL directly in the caller and do not rely on a
hack adding PAGE_KERNEL flags when _PAGE_PRESENT is not set.

As already done for PPC64, use pgprot_cache() helpers instead of
_PAGE_XXX flags in PPC32 ioremap() derived functions.

Signed-off-by: Christophe Leroy 


Something in here is breaking my p5020ds (both 32-bit and 64-bit):

# first bad commit: [cb9220cc18345a2a1ec287752e576a2c88fa4a2b]  
powerpc/mm: properly set PAGE_KERNEL flags in ioremap()


Strange, it is calling ioremap_prot(), it should have been calling  
ioremap_cache() hence ioremap() since patch 4  of the serie. Did  
patch 4 apply correctly ?


Oops, forget that stupid comment. ioremap_cache() uses ioremap_prot()  
but with the correct flags instead of 0


So patch 4 is needed anyway, because using 0 as flags will now not  
anymore implicitely imply PAGE_KERNEL


Christophe



Christophe



64-bit:

 io scheduler mq-deadline registered
 io scheduler kyber registered
 pcieport :00:00.0: AER enabled with IRQ 482
 pcieport 2000:00:00.0: AER enabled with IRQ 480
 Unable to handle kernel paging request for data at address  
0x88008008

 Faulting instruction address: 0xc00152e4
 Oops: Kernel access of bad area, sig: 11 [#1]
 BE SMP NR_CPUS=32 CoreNet Generic
 Modules linked in:
 CPU: 1 PID: 1 Comm: swapper/0 Not tainted  
4.19.0-rc3-gcc-7.3.1-00135-gcb9220cc1834 #16

 NIP:  c00152e4 LR: c05173b8 CTR: 0010
 REGS: c000f30eb420 TRAP: 0300   Not tainted   
(4.19.0-rc3-gcc-7.3.1-00135-gcb9220cc1834)

 MSR:  80029000   CR: 24000224  XER: 
 DEAR: 88008008 ESR: 0080 IRQMASK: 0
 GPR00: c05173a0 c000f30eb6a0 c0f86e00 88008008
 GPR04:  0040 0ffbff04 0008
 GPR08:  0010  0080
 GPR12: 84000422 c00037c0 c000263c 
 GPR16:    
 GPR20:    
 GPR24: c0dbe0a0 c0d41bf8 88008008 c00089a8
 GPR28: c000f3492400 c000f3492410 0040 c0ff0a18
 NIP [c00152e4] ._memset_io+0x6c/0x9c
 LR [c05173b8] .fsl_qman_probe+0x188/0x918
 Call Trace:
 [c000f30eb6a0] [c05173a0] .fsl_qman_probe+0x170/0x918  
(unreliable)

 [c000f30eb740] [c05797dc] .platform_drv_probe+0x58/0xac
 [c000f30eb7c0] [c05772b0] .really_probe+0x2a8/0x380
 [c000f30eb860] [c05776f0] .__driver_attach+0x138/0x13c
 [c000f30eb8f0] [c0574550] .bus_for_each_dev+0x9c/0x110
 [c000f30eb9a0] [c0576a18] .driver_attach+0x24/0x38
 [c000f30eba10] [c05761ec] .bus_add_driver+0x16c/0x2ac
 [c000f30ebab0] [c0578194] .driver_register+0x88/0x178
 [c000f30ebb30] [c0579770] .__platform_driver_register+0x48/0x5c
 [c000f30ebba0] [c0d85744] .fsl_qman_driver_init+0x1c/0x30
 [c000f30ebc10] [c0002374] .do_one_initcall+0x70/0x258
 [c000f30ebcf0] [c0d4a244] .kernel_init_freeable+0x340/0x43c
 [c000f30ebdb0] [c0002658] .kernel_init+0x1c/0x130
 [c000f30ebe30] [c9e4] .ret_from_kernel_thread+0x58/0x74
 Instruction dump:
 4e800020 2ba50003 40dd003c 3925fffc 5488402e 7929f082 7d082378 39290001
 550a801e 7d2903a6 7d4a4378 794a0020 <9143> 38630004 4200fff8 70a50003
 ---[ end trace 372a57fd67efb6fe ]---

32 bit:

 [1.076133] pcieport 2000:02:00.0: AER enabled with IRQ 480
 [1.082106] Unable to handle kernel paging request for data at  
address 0xf110

 [1.089488] Faulting instruction address: 0xc0011c80
 [1.094437] Oops: Kernel access of bad area, sig: 11 [#1]
 [1.099816] BE SMP NR_CPUS=24 CoreNet Generic
 [1.104157] Modules linked in:
 [1.107197] CPU: 0 PID: 1 Comm: swapper/0 Not tainted  
4.19.0-rc3-gcc7x-g8f0c636b0542 #1

 [1.115181] NIP:  c0011c80 LR: c058f970 CTR: 0010
 [1.120216] REGS: e8107c80 TRAP: 0300   Not tainted   
(4.19.0-rc3-gcc7x-g8f0c636b0542)

 [1.128026] MSR:  00029002   CR: 24000284  XER: 
 [1.134190] DEAR: f110 ESR: 0080
 [1.134190] GPR00: c058f958 e8107d30 e8108000 f110   
0040 e8107cd8 e8107d0c
 [1.134190] GPR08:  0010  ff00 24000282  
 c0003340 
 [1.134190] GPR16:     c0d64410  
c0edb34c c0ec6700 0007
 [1.134190] GPR24: c0ef f110 efffcab8 c0ef e8231c00  
e8231c10 0040003f c101d290

 [1.171519] NIP [c0011c80] _memset_io+0x90/0xd0
 [1.176030] LR [c058f970] fsl_qman_probe+0x190/0x8c0
 [1.180975] Call Trace:
 [1.183410] [e8107d30] [c001f6c0] ioremap_prot+0x40/0x50 (unreliable)
 [1.189830] [e8

Re: [PATCH 3/9] powerpc/mm: Remove extern from function definition

[LEROY Christophe]

Breno Leitao  a écrit :


Function huge_ptep_set_access_flags() has the 'extern' keyword in the
function definition and also in the function declaration. This causes a
warning in 'sparse' since the 'extern' storage class should be used only on
symbol declarations.

	arch/powerpc/mm/pgtable.c:232:12: warning: function  
'huge_ptep_set_access_flags' with external linkage has definition


This patch removes the keyword from the definition part, while keeps it in
the declaration part.


I think checkpatch also says that extern should be avoided in declarations.
Can you remove both ?

Christophe



Signed-off-by: Breno Leitao 
---
 arch/powerpc/mm/pgtable.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index d71c669c..213933b78077 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -229,7 +229,7 @@ int ptep_set_access_flags(struct vm_area_struct  
*vma, unsigned long address,

 }

 #ifdef CONFIG_HUGETLB_PAGE
-extern int huge_ptep_set_access_flags(struct vm_area_struct *vma,
+int huge_ptep_set_access_flags(struct vm_area_struct *vma,
  unsigned long addr, pte_t *ptep,
  pte_t pte, int dirty)
 {
--
2.19.0

Re: [PATCH 3/9] powerpc/mm: Remove extern from function definition

[LEROY Christophe]

Breno Leitao  a écrit :


hi Christophe,

On 10/23/2018 12:38 PM, LEROY Christophe wrote:

Breno Leitao  a écrit :


This patch removes the keyword from the definition part, while keeps
it in
the declaration part.


I think checkpatch also says that extern should be avoided in declarations.


Thanks for the review. I tried to look at this complain, but I didn't see
this behavior on checkpatch.pl from kernel 4.19. I created a commit that adds
a new extern prototype and checked the patch. Take a look:


Use option --strict with checkpatch.pl

Christophe



# git show

commit 720cd4ee7bf3c0607eaea79e209b719bac79508e
Author: Breno Leitao 
Date:   Wed Oct 24 10:31:54 2018 -0400

powerpc/mm: New test function

New test function.

Signed-off-by: Breno Leitao 

diff --git a/arch/powerpc/include/asm/hugetlb.h
b/arch/powerpc/include/asm/hugetlb.h
index 2d00cc530083..4a348e42cab6 100644
--- a/arch/powerpc/include/asm/hugetlb.h
+++ b/arch/powerpc/include/asm/hugetlb.h
	@@ -167,6 +167,8 @@ extern int huge_ptep_set_access_flags(struct  
vm_area_struct *vma,

  unsigned long addr, pte_t *ptep,
  pte_t pte, int dirty);

+extern int test(int foo);
+
static inline pte_t huge_ptep_get(pte_t *ptep)
{
return *ptep;

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 5c390f5a5207..2e8f5f77f7f6 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -3204,6 +3204,11 @@ static void set_huge_ptep_writable(struct
vm_area_struct *vma,
update_mmu_cache(vma, address, ptep);
}

+int test(int foo)
+{
+   return foo;
+}
+
bool is_hugetlb_entry_migration(pte_t pte)
{
swp_entry_t swp;


# scripts/checkpatch.pl -g HEAD

total: 0 errors, 0 warnings, 19 lines checked

Commit 720cd4ee7bf3 ("powerpc/mm: New test function") has no obvious
style problems and is ready for submission.

Re: [PATCH 0/5] Guarded Userspace Access Prevention on Radix

[LEROY Christophe]

Russell Currey  a écrit :


Guarded Userspace Access Prevention is a security mechanism that prevents
the kernel from being able to read and write userspace addresses outside of
the allowed paths, most commonly copy_{to/from}_user().

At present, the only CPU that supports this is POWER9, and only while using
the Radix MMU.  Privileged reads and writes cannot access user data when
key 0 of the AMR is set.  This is described in the "Radix Tree Translation
Storage Protection" section of the POWER ISA as of version 3.0.


It is not right that only power9 can support that.

The 8xx has mmu access protection registers which can serve the same  
purpose. Today on the 8xx kernel space is group 0 and user space is  
group 1. Group 0 is set to "page defined access permission" in MD_AP  
and MI_AP registers, and group 1 is set to "all accesses done with  
supervisor rights". By setting group 1 to "user and supervisor  
interpretation swapped" we can forbid kernel access to user space  
while still allowing user access to it. Then by simply changing group  
1 mode at dedicated places we can lock/unlock kernel access to user  
space.


Could you implement something as generic as possible having that in  
mind for a future patch ?


Christophe



GUAP code sets key 0 of the AMR (thus disabling accesses of user data)
early during boot, and only ever "unlocks" access prior to certain
operations, like copy_{to/from}_user(), futex ops, etc.  Setting this does
not prevent unprivileged access, so userspace can operate fine while access
is locked.

There is a performance impact, although I don't consider it heavy.  Running
a worst-case benchmark of a 1GB copy 1 byte at a time (and thus constant
read(1) write(1) syscalls), I found enabling GUAP to be 3.5% slower than
when disabled.  In most cases, the difference is negligible.  The main
performance impact is the mtspr instruction, which is quite slow.

There are a few caveats with this series that could be improved upon in
future.  Right now there is no saving and restoring of the AMR value -
there is no userspace exploitation of the AMR on Radix in POWER9, but if
this were to change in future, saving and restoring the value would be
necessary.

No attempt to optimise cases of repeated calls - for example, if some
code was repeatedly calling copy_to_user() for small sizes very frequently,
it would be slower than the equivalent of wrapping that code in an unlock
and lock and only having to modify the AMR once.

There are some interesting cases that I've attempted to handle, such as if
the AMR is unlocked (i.e. because a copy_{to_from}_user is in progress)...

- and an exception is taken, the kernel would then be running with the
AMR unlocked and freely able to access userspace again.  I am working
around this by storing a flag in the PACA to indicate if the AMR is
unlocked (to save a costly SPR read), and if so, locking the AMR in
the exception entry path and unlocking it on the way out.

- and gets context switched out, goes into a path that locks the AMR,
then context switches back, access will be disabled and will fault.
As a result, I context switch the AMR between tasks as if it was used
by userspace like hash (which already implements this).

Another consideration is use of the isync instruction.  Without an isync
following the mtspr instruction, there is no guarantee that the change
takes effect.  The issue is that isync is very slow, and so I tried to
avoid them wherever necessary.  In this series, the only place an isync
gets used is after *unlocking* the AMR, because if an access takes place
and access is still prevented, the kernel will fault.

On the flipside, a slight delay in unlocking caused by skipping an isync
potentially allows a small window of vulnerability.  It is my opinion
that this window is practically impossible to exploit, but if someone
thinks otherwise, please do share.

This series is my first attempt at POWER assembly so all feedback is very
welcome.

The official theme song of this series can be found here:
https://www.youtube.com/watch?v=QjTrnKAcYjE

Russell Currey (5):
  powerpc/64s: Guarded Userspace Access Prevention
  powerpc/futex: GUAP support for futex ops
  powerpc/lib: checksum GUAP support
  powerpc/64s: Disable GUAP with nosmap option
  powerpc/64s: Document that PPC supports nosmap

 .../admin-guide/kernel-parameters.txt |  2 +-
 arch/powerpc/include/asm/exception-64e.h  |  3 +
 arch/powerpc/include/asm/exception-64s.h  | 19 ++-
 arch/powerpc/include/asm/futex.h  |  6 ++
 arch/powerpc/include/asm/mmu.h|  7 +++
 arch/powerpc/include/asm/paca.h   |  3 +
 arch/powerpc/include/asm/reg.h|  1 +
 arch/powerpc/include/asm/uaccess.h| 57 ---
 arch/powerpc/kernel/asm-offsets.c |  1 +
 arch/powerpc/kernel/dt_cpu_ftrs.c |  4 ++
 arch/powerpc/kernel/entry_64.S| 17 +

Re: [PATCH 2/5] powerpc/futex: GUAP support for futex ops

[LEROY Christophe]

Russell Currey  a écrit :


Wrap the futex operations in GUAP locks and unlocks.


Does it means futex doesn't work anymore once only patch 1 is applied  
? If so, then you should split patch 1 in two parts and reorder  
patches so that guap can only be activated once all necessary changes  
are done. Otherwise the serie won't be bisectable


Christophe



Signed-off-by: Russell Currey 
---
 arch/powerpc/include/asm/futex.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/include/asm/futex.h  
b/arch/powerpc/include/asm/futex.h

index 94542776a62d..3aed640ee9ef 100644
--- a/arch/powerpc/include/asm/futex.h
+++ b/arch/powerpc/include/asm/futex.h
@@ -35,6 +35,7 @@ static inline int arch_futex_atomic_op_inuser(int  
op, int oparg, int *oval,

 {
int oldval = 0, ret;

+   unlock_user_access();
pagefault_disable();

switch (op) {
@@ -62,6 +63,7 @@ static inline int arch_futex_atomic_op_inuser(int  
op, int oparg, int *oval,

if (!ret)
*oval = oldval;

+   lock_user_access();
return ret;
 }

@@ -75,6 +77,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
return -EFAULT;

+   unlock_user_access();
 __asm__ __volatile__ (
 PPC_ATOMIC_ENTRY_BARRIER
 "1: lwarx   %1,0,%3 # futex_atomic_cmpxchg_inatomic\n\
@@ -95,6 +98,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
 : "cc", "memory");

*uval = prev;
+   lock_user_access();
 return ret;
 }

--
2.19.1

Re: [PATCH 3/5] powerpc/lib: checksum GUAP support

[LEROY Christophe]

Same comment as for futex

Christophe

Russell Currey  a écrit :


Wrap the checksumming code in GUAP locks and unlocks.

Signed-off-by: Russell Currey 
---
 arch/powerpc/lib/checksum_wrappers.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/arch/powerpc/lib/checksum_wrappers.c  
b/arch/powerpc/lib/checksum_wrappers.c

index a0cb63fb76a1..c67db0a6e18b 100644
--- a/arch/powerpc/lib/checksum_wrappers.c
+++ b/arch/powerpc/lib/checksum_wrappers.c
@@ -28,6 +28,7 @@ __wsum csum_and_copy_from_user(const void __user  
*src, void *dst,

 {
unsigned int csum;

+   unlock_user_access();
might_sleep();

*err_ptr = 0;
@@ -60,6 +61,7 @@ __wsum csum_and_copy_from_user(const void __user  
*src, void *dst,

}

 out:
+   lock_user_access();
return (__force __wsum)csum;
 }
 EXPORT_SYMBOL(csum_and_copy_from_user);
@@ -69,6 +71,7 @@ __wsum csum_and_copy_to_user(const void *src, void  
__user *dst, int len,

 {
unsigned int csum;

+   unlock_user_access();
might_sleep();

*err_ptr = 0;
@@ -97,6 +100,7 @@ __wsum csum_and_copy_to_user(const void *src,  
void __user *dst, int len,

}

 out:
+   lock_user_access();
return (__force __wsum)csum;
 }
 EXPORT_SYMBOL(csum_and_copy_to_user);
--
2.19.1

Re: [PATCH 5/5] powerpc/64s: Document that PPC supports nosmap

[LEROY Christophe]

Why not call our new functionnality SMAP instead of calling it GUAP ?

Christophe

Russell Currey  a écrit :


Signed-off-by: Russell Currey 
---
 Documentation/admin-guide/kernel-parameters.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt  
b/Documentation/admin-guide/kernel-parameters.txt

index a5ad67d5cb16..8f78e75965f0 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2764,7 +2764,7 @@
noexec=on: enable non-executable mappings (default)
noexec=off: disable non-executable mappings

-   nosmap  [X86]
+   nosmap  [X86,PPC]
Disable SMAP (Supervisor Mode Access Prevention)
even if it is supported by processor.

--
2.19.1

Re: [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention

[LEROY Christophe]

Russell Currey  a écrit :


Guarded Userspace Access Prevention (GUAP)  utilises a feature of
the Radix MMU which disallows read and write access to userspace
addresses.  By utilising this, the kernel is prevented from accessing
user data from outside of trusted paths that perform proper safety checks,
such as copy_{to/from}_user() and friends.

Userspace access is disabled from early boot and is only enabled when:

- exiting the kernel and entering userspace
- performing an operation like copy_{to/from}_user()
- context switching to a process that has access enabled

and similarly, access is disabled again when exiting userspace and entering
the kernel.

This feature has a slight performance impact which I roughly measured to be
3% slower in the worst case (performing 1GB of 1 byte read()/write()
syscalls), and is gated behind the CONFIG_PPC_RADIX_GUAP option for
performance-critical builds.

This feature can be tested by using the lkdtm driver (CONFIG_LKDTM=y) and
performing the following:

echo ACCESS_USERSPACE > [debugfs]/provoke-crash/DIRECT

if enabled, this should send SIGSEGV to the thread.

Signed-off-by: Russell Currey 


I think this patch should be split in at least two parts:
First part for implementing the generic part, including the changes to  
futex and csum, and a second part implementing the radix part.

---
Since the previous version of this patchset (named KHRAP) there have been
several changes, some of which include:

- macro naming, suggested by Nick
- builds should be fixed outside of 64s
- no longer unlock heading out to userspace
- removal of unnecessary isyncs
- more config option testing
- removal of save/restore
- use pr_crit() and reword message on fault

 arch/powerpc/include/asm/exception-64e.h |  3 ++
 arch/powerpc/include/asm/exception-64s.h | 19 +++-
 arch/powerpc/include/asm/mmu.h   |  7 +++
 arch/powerpc/include/asm/paca.h  |  3 ++
 arch/powerpc/include/asm/reg.h   |  1 +
 arch/powerpc/include/asm/uaccess.h   | 57 
 arch/powerpc/kernel/asm-offsets.c|  1 +
 arch/powerpc/kernel/dt_cpu_ftrs.c|  4 ++
 arch/powerpc/kernel/entry_64.S   | 17 ++-
 arch/powerpc/mm/fault.c  | 12 +
 arch/powerpc/mm/pgtable-radix.c  |  2 +
 arch/powerpc/mm/pkeys.c  |  7 ++-
 arch/powerpc/platforms/Kconfig.cputype   | 15 +++
 13 files changed, 135 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/include/asm/exception-64e.h  
b/arch/powerpc/include/asm/exception-64e.h

index 555e22d5e07f..bf25015834ee 100644
--- a/arch/powerpc/include/asm/exception-64e.h
+++ b/arch/powerpc/include/asm/exception-64e.h
@@ -215,5 +215,8 @@ exc_##label##_book3e:
 #define RFI_TO_USER\
rfi

+#define UNLOCK_USER_ACCESS(reg)
+#define LOCK_USER_ACCESS(reg)
+
 #endif /* _ASM_POWERPC_EXCEPTION_64E_H */

diff --git a/arch/powerpc/include/asm/exception-64s.h  
b/arch/powerpc/include/asm/exception-64s.h

index 3b4767ed3ec5..0cac5bd380ca 100644
--- a/arch/powerpc/include/asm/exception-64s.h
+++ b/arch/powerpc/include/asm/exception-64s.h
@@ -264,6 +264,19 @@ BEGIN_FTR_SECTION_NESTED(943)  
\
std ra,offset(r13); \
 END_FTR_SECTION_NESTED(ftr,ftr,943)

+#define LOCK_USER_ACCESS(reg)  
\
+BEGIN_MMU_FTR_SECTION_NESTED(944)  \
+   LOAD_REG_IMMEDIATE(reg,AMR_LOCKED); \
+   mtspr   SPRN_AMR,reg;   \
+END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_GUAP,MMU_FTR_RADIX_GUAP,944)
+
+#define UNLOCK_USER_ACCESS(reg)
\
+BEGIN_MMU_FTR_SECTION_NESTED(945)  \
+   li  reg,0;  \
+   mtspr   SPRN_AMR,reg;   \
+   isync   \
+END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_GUAP,MMU_FTR_RADIX_GUAP,945)
+
 #define EXCEPTION_PROLOG_0(area)   \
GET_PACA(r13);  \
std r9,area+EX_R9(r13); /* save r9 */   \
@@ -500,7 +513,11 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
beq 4f; /* if from kernel mode  */ \
ACCOUNT_CPU_USER_ENTRY(r13, r9, r10);  \
SAVE_PPR(area, r9);\
-4: EXCEPTION_PROLOG_COMMON_2(area)\
+4: lbz r9,PACA_USER_ACCESS_ALLOWED(r13);  \
+   cmpwi   cr1,r9,0;  

Re: NXP P50XX/e5500: SMP doesn't work anymore with the latest Git kernel

[LEROY Christophe]

Christian Zigotzky  a écrit :


Hello,

I figured out that the problem is in the OF source code of the  
commit: Merge tag devicetree-for-4.20. [1]


That's a merge commit. Can you bisect the branch and identify the  
faulting commit ?


Christophe



I reverted the following OF files and SMP works!

drivers/of/base.c
drivers/of/device.c
drivers/of/of_mdio.c
drivers/of/of_numa.c
drivers/of/of_private.h
drivers/of/overlay.c
drivers/of/platform.c
drivers/of/unittest-data/overlay_15.dts
drivers/of/unittest-data/tests-overlay.dtsi
drivers/of/unittest.c
include/linux/of.h

Cheers,
Christian

[1]  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b27186abb37b7bd19e0ca434f4f425c807dbd708



On 29 October 2018 at 10:56AM, Christian Zigotzky wrote:

Hello,

I have figured out that the commit 'devicetree-for-4.20' [1] is  
responsible for the SMP problem. I was able to revert this commit  
with 'git revert b27186abb37b7bd19e0ca434f4f425c807dbd708 -m 1'  
today.


[master ec81438] Revert "Merge tag 'devicetree-for-4.20' of  
git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux"

138 files changed, 931 insertions(+), 1538 deletions(-)
rename Documentation/devicetree/bindings/arm/{atmel-sysregs.txt =>  
atmel-at91.txt} (67%)
delete mode 100644  
Documentation/devicetree/bindings/arm/freescale/fsl,layerscape-dcfg.txt
delete mode 100644  
Documentation/devicetree/bindings/arm/freescale/fsl,layerscape-scfg.txt
rename Documentation/devicetree/bindings/arm/{zte,sysctrl.txt =>  
zte.txt} (62%)

delete mode 100644 Documentation/devicetree/bindings/misc/lwn-bk4.txt
create mode 100644 arch/c6x/boot/dts/linked_dtb.S
delete mode 100644 arch/nios2/boot/dts/Makefile
create mode 100644 arch/nios2/boot/linked_dtb.S
delete mode 100644 arch/powerpc/boot/dts/Makefile
delete mode 100644 arch/powerpc/boot/dts/fsl/Makefile
delete mode 100644 scripts/dtc/yamltree.c

It solves the SMP problem! SMP works again on my P5020 board and on  
virtual e5500 QEMU machines.


QEMU command: ./qemu-system-ppc64 -M ppce500 -cpu e5500 -m 2048  
-kernel /home/christian/Downloads/uImage-4.20-alpha5 -drive  
format=raw,file=/home/christian/Dokumente/ubuntu_MATE_16.04.3_LTS_PowerPC_QEMU/ubuntu_MATE_16.04_PowerPC.img,index=0,if=virtio -nic user,model=e1000 -append "rw root=/dev/vda3" -device virtio-vga -device virtio-mouse-pci -device virtio-keyboard-pci -soundhw es1370 -smp  
4


Screenshot:  
https://plus.google.com/u/0/photos/photo/115515624056477014971/6617705776207990082


Do we need a new dtb file or is it a bug?

Thanks,
Christian

[1]  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b27186abb37b7bd19e0ca434f4f425c807dbd708



On 28 October 2018 at 5:35PM, Christian Zigotzky wrote:

Hello,

SMP doesn't work anymore with the latest Git kernel (28/10/18  
11:12AM GMT) on my P5020 board and on virtual e5500 QEMU machines.


Board with P5020 dual core CPU:

[    0.00] -
[    0.00] phys_mem_size = 0x2
[    0.00] dcache_bsize  = 0x40
[    0.00] icache_bsize  = 0x40
[    0.00] cpu_features  = 0x0003008003b4
[    0.00]   possible    = 0x0003009003b4
[    0.00]   always  = 0x0003008003b4
[    0.00] cpu_user_features = 0xcc008000 0x0800
[    0.00] mmu_features  = 0x000a0010
[    0.00] firmware_features = 0x
[    0.00] -
[    0.00] CoreNet Generic board

    ...

[    0.002161] smp: Bringing up secondary CPUs ...
[    0.002339] No cpu-release-addr for cpu 1
[    0.002347] smp: failed starting cpu 1 (rc -2)
[    0.002401] smp: Brought up 1 node, 1 CPU

Virtual e5500 quad core QEMU machine:

[    0.026394] smp: Bringing up secondary CPUs ...
[    0.027831] No cpu-release-addr for cpu 1
[    0.027989] smp: failed starting cpu 1 (rc -2)
[    0.030143] No cpu-release-addr for cpu 2
[    0.030304] smp: failed starting cpu 2 (rc -2)
[    0.032400] No cpu-release-addr for cpu 3
[    0.032533] smp: failed starting cpu 3 (rc -2)
[    0.033117] smp: Brought up 1 node, 1 CPU

QEMU command: ./qemu-system-ppc64 -M ppce500 -cpu e5500 -m 2048  
-kernel  
/home/christian/Downloads/vmlinux-4.20-alpha4-AmigaOne_X1000_X5000/X5000_and_QEMU_e5500/uImage-4.20 -drive format=raw,file=/home/christian/Downloads/MATE_PowerPC_Remix_2017_0.9.img,index=0,if=virtio -nic user,model=e1000 -append "rw root=/dev/vda" -device virtio-vga -device virtio-mouse-pci -device virtio-keyboard-pci -usb -soundhw es1370 -smp  
4


.config:

...
CONFIG_SMP=y
CONFIG_NR_CPUS=4
...

Please test the latest Git kernel on your NXP P50XX boards.

Thanks,
Christian

Re: [PATCH 1/5] powerpc/64s: Guarded Userspace Access Prevention

[LEROY Christophe]

Russell Currey  a écrit :


On Sun, 2018-10-28 at 18:57 +0100, LEROY Christophe wrote:

Russell Currey  a écrit :

> Guarded Userspace Access Prevention (GUAP)  utilises a feature of
> the Radix MMU which disallows read and write access to userspace
> addresses.  By utilising this, the kernel is prevented from
> accessing
> user data from outside of trusted paths that perform proper safety
> checks,
> such as copy_{to/from}_user() and friends.
>
> Userspace access is disabled from early boot and is only enabled
> when:
>
>- exiting the kernel and entering userspace
>- performing an operation like copy_{to/from}_user()
>- context switching to a process that has access enabled
>
> and similarly, access is disabled again when exiting userspace and
> entering
> the kernel.
>
> This feature has a slight performance impact which I roughly
> measured to be
> 3% slower in the worst case (performing 1GB of 1 byte
> read()/write()
> syscalls), and is gated behind the CONFIG_PPC_RADIX_GUAP option for
> performance-critical builds.
>
> This feature can be tested by using the lkdtm driver
> (CONFIG_LKDTM=y) and
> performing the following:
>
>echo ACCESS_USERSPACE > [debugfs]/provoke-crash/DIRECT
>
> if enabled, this should send SIGSEGV to the thread.
>
> Signed-off-by: Russell Currey 

I think this patch should be split in at least two parts:
First part for implementing the generic part, including the changes
to
futex and csum, and a second part implementing the radix part.


I'll see how I go making generic handlers - I am concerned about the
implementation becoming more complex than it needs to be just to
accommodate potential future changes that could end up having different
requirements anyway, rather than something simple that works today.


I think we can do something quite simple. I'll draft something next  
week and send it to get your feedback.





> ---
> Since the previous version of this patchset (named KHRAP) there
> have been
> several changes, some of which include:
>
> - macro naming, suggested by Nick
> - builds should be fixed outside of 64s
> - no longer unlock heading out to userspace
> - removal of unnecessary isyncs
> - more config option testing
> - removal of save/restore
> - use pr_crit() and reword message on fault
>
>  arch/powerpc/include/asm/exception-64e.h |  3 ++
>  arch/powerpc/include/asm/exception-64s.h | 19 +++-
>  arch/powerpc/include/asm/mmu.h   |  7 +++
>  arch/powerpc/include/asm/paca.h  |  3 ++
>  arch/powerpc/include/asm/reg.h   |  1 +
>  arch/powerpc/include/asm/uaccess.h   | 57
> 
>  arch/powerpc/kernel/asm-offsets.c|  1 +
>  arch/powerpc/kernel/dt_cpu_ftrs.c|  4 ++
>  arch/powerpc/kernel/entry_64.S   | 17 ++-
>  arch/powerpc/mm/fault.c  | 12 +
>  arch/powerpc/mm/pgtable-radix.c  |  2 +
>  arch/powerpc/mm/pkeys.c  |  7 ++-
>  arch/powerpc/platforms/Kconfig.cputype   | 15 +++
>  13 files changed, 135 insertions(+), 13 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/exception-64e.h
> b/arch/powerpc/include/asm/exception-64e.h
> index 555e22d5e07f..bf25015834ee 100644
> --- a/arch/powerpc/include/asm/exception-64e.h
> +++ b/arch/powerpc/include/asm/exception-64e.h
> @@ -215,5 +215,8 @@ exc_##label##_book3e:
>  #define RFI_TO_USER
>\
>rfi
>
> +#define UNLOCK_USER_ACCESS(reg)
> +#define LOCK_USER_ACCESS(reg)
> +
>  #endif /* _ASM_POWERPC_EXCEPTION_64E_H */
>
> diff --git a/arch/powerpc/include/asm/exception-64s.h
> b/arch/powerpc/include/asm/exception-64s.h
> index 3b4767ed3ec5..0cac5bd380ca 100644
> --- a/arch/powerpc/include/asm/exception-64s.h
> +++ b/arch/powerpc/include/asm/exception-64s.h
> @@ -264,6 +264,19 @@ BEGIN_FTR_SECTION_NESTED(943)
>\
>std ra,offset(r13); \
>  END_FTR_SECTION_NESTED(ftr,ftr,943)
>
> +#define LOCK_USER_ACCESS(reg)
>\
> +BEGIN_MMU_FTR_SECTION_NESTED(944)
> \
> +  LOAD_REG_IMMEDIATE(reg,AMR_LOCKED); \
> +  mtspr   SPRN_AMR,reg;
> \
> +END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_GUAP,MMU_FTR_RADIX_GUAP,9
> 44)
> +
> +#define UNLOCK_USER_ACCESS(reg)
>\
> +BEGIN_MMU_FTR_SECTION_NESTED(945)
> \
> +  li  reg,0;  \
> +  mtspr   SPRN_AMR,reg;
> \
> +  isync
> \
> +END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_GUAP,MMU_FTR_RADIX_GUAP,9
> 45)
> +
>  #define EXCEPTION_PROLOG_0(area)
> \
>GET_PACA(r13);
> \
&g

Re: [PATCH 0/5] Guarded Userspace Access Prevention on Radix

[LEROY Christophe]

Russell Currey  a écrit :


On Fri, 2018-10-26 at 18:29 +0200, LEROY Christophe wrote:

Russell Currey  a écrit :

> Guarded Userspace Access Prevention is a security mechanism that
> prevents
> the kernel from being able to read and write userspace addresses
> outside of
> the allowed paths, most commonly copy_{to/from}_user().
>
> At present, the only CPU that supports this is POWER9, and only
> while using
> the Radix MMU.  Privileged reads and writes cannot access user data
> when
> key 0 of the AMR is set.  This is described in the "Radix Tree
> Translation
> Storage Protection" section of the POWER ISA as of version 3.0.

It is not right that only power9 can support that.


It's true that not only P9 can support it, but there are more
considerations under hash than radix, implementing this for radix is a
first step.


I don't know much about hash, but I was talking about the 8xx which is  
a nohash ppc32. I'll see next week if I can do something with it on  
top of your serie.






The 8xx has mmu access protection registers which can serve the
same
purpose. Today on the 8xx kernel space is group 0 and user space is
group 1. Group 0 is set to "page defined access permission" in
MD_AP
and MI_AP registers, and group 1 is set to "all accesses done with
supervisor rights". By setting group 1 to "user and supervisor
interpretation swapped" we can forbid kernel access to user space
while still allowing user access to it. Then by simply changing
group
1 mode at dedicated places we can lock/unlock kernel access to user
space.

Could you implement something as generic as possible having that in
mind for a future patch ?


I don't think anything in this series is particularly problematic in
relation to future work for hash.  I am interested in doing a hash
implementation in future too.


I think we have to look at that carrefuly to avoid uggly ifdefs

Christophe



- Russell



Christophe

> GUAP code sets key 0 of the AMR (thus disabling accesses of user
> data)
> early during boot, and only ever "unlocks" access prior to certain
> operations, like copy_{to/from}_user(), futex ops, etc.  Setting
> this does
> not prevent unprivileged access, so userspace can operate fine
> while access
> is locked.
>
> There is a performance impact, although I don't consider it
> heavy.  Running
> a worst-case benchmark of a 1GB copy 1 byte at a time (and thus
> constant
> read(1) write(1) syscalls), I found enabling GUAP to be 3.5% slower
> than
> when disabled.  In most cases, the difference is negligible.  The
> main
> performance impact is the mtspr instruction, which is quite slow.
>
> There are a few caveats with this series that could be improved
> upon in
> future.  Right now there is no saving and restoring of the AMR
> value -
> there is no userspace exploitation of the AMR on Radix in POWER9,
> but if
> this were to change in future, saving and restoring the value would
> be
> necessary.
>
> No attempt to optimise cases of repeated calls - for example, if
> some
> code was repeatedly calling copy_to_user() for small sizes very
> frequently,
> it would be slower than the equivalent of wrapping that code in an
> unlock
> and lock and only having to modify the AMR once.
>
> There are some interesting cases that I've attempted to handle,
> such as if
> the AMR is unlocked (i.e. because a copy_{to_from}_user is in
> progress)...
>
> - and an exception is taken, the kernel would then be running
> with the
> AMR unlocked and freely able to access userspace again.  I am
> working
> around this by storing a flag in the PACA to indicate if the
> AMR is
> unlocked (to save a costly SPR read), and if so, locking the
> AMR in
> the exception entry path and unlocking it on the way out.
>
> - and gets context switched out, goes into a path that locks
> the AMR,
> then context switches back, access will be disabled and will
> fault.
> As a result, I context switch the AMR between tasks as if it
> was used
> by userspace like hash (which already implements this).
>
> Another consideration is use of the isync instruction.  Without an
> isync
> following the mtspr instruction, there is no guarantee that the
> change
> takes effect.  The issue is that isync is very slow, and so I tried
> to
> avoid them wherever necessary.  In this series, the only place an
> isync
> gets used is after *unlocking* the AMR, because if an access takes
> place
> and access is still prevented, the kernel will fault.
>
> On the flipside, a slight delay in unlocking caused by skipping an
> isync
> potentially allows a small window of vulnerability.  It is my
> opinion
> t

Re: [PATCH 5/5] powerpc/64s: Document that PPC supports nosmap

[LEROY Christophe]

Russell Currey  a écrit :


On Fri, 2018-10-26 at 18:35 +0200, LEROY Christophe wrote:

Why not call our new functionnality SMAP instead of calling it GUAP ?


mpe wasn't a fan of using the same terminology as other architectures.


I don't like too much the word 'guarded' because it means something  
different for powerpc MMUs.


What about something like 'user space access protection' ?

Christophe


Having a separate term does avoid some assumptions about how things
work or are implemented, but sharing compatibility with an existing
parameter is nice.

Personally I don't really care too much about the name.

- Russell



Christophe

Russell Currey  a écrit :

> Signed-off-by: Russell Currey 
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index a5ad67d5cb16..8f78e75965f0 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2764,7 +2764,7 @@
>noexec=on: enable non-executable mappings
> (default)
>noexec=off: disable non-executable mappings
>
> -  nosmap  [X86]
> +  nosmap  [X86,PPC]
>Disable SMAP (Supervisor Mode Access
> Prevention)
>even if it is supported by processor.
>
> --
> 2.19.1

Re: [PATCH v8 01/25] scsi/atari_scsi: Don't select CONFIG_NVRAM

[LEROY Christophe]

Finn Thain  a écrit :


On powerpc, setting CONFIG_NVRAM=n builds a kernel with no NVRAM support.
Setting CONFIG_NVRAM=m enables the /dev/nvram misc device module without
enabling NVRAM support in drivers. Setting CONFIG_NVRAM=y enables the
misc device (built-in) and also enables NVRAM support in drivers.

m68k shares the valkyriefb driver with powerpc, and since that driver uses
NVRAM, it is affected by CONFIG_ATARI_SCSI, because of the use of
"select NVRAM".

Adopt the powerpc convention on m68k to avoid surprises.

Signed-off-by: Finn Thain 
Tested-by: Christian T. Steigies 
---
This patch temporarily disables CONFIG_NVRAM on Atari, to prevent build
failures when bisecting the rest of this patch series. It gets enabled
again with the introduction of CONFIG_HAVE_ARCH_NVRAM_OPS, once the
nvram_* global functions have been moved to an ops struct.
---
 drivers/char/Kconfig  | 5 +
 drivers/scsi/Kconfig  | 6 +++---
 drivers/scsi/atari_scsi.c | 7 ---
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 9d03b2ff5df6..5b54595dfe30 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -236,7 +236,7 @@ source "drivers/char/hw_random/Kconfig"

 config NVRAM
tristate "/dev/nvram support"
-   depends on ATARI || X86 || GENERIC_NVRAM
+   depends on X86 || GENERIC_NVRAM
---help---
  If you say Y here and create a character special file /dev/nvram
  with major number 10 and minor number 144 using mknod ("man mknod"),
@@ -254,9 +254,6 @@ config NVRAM
  should NEVER idly tamper with it. See Ralf Brown's interrupt list
  for a guide to the use of CMOS bytes by your BIOS.

- On Atari machines, /dev/nvram is always configured and does not need
- to be selected.
-
  To compile this driver as a module, choose M here: the
  module will be called nvram.

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 640cd1b31a18..924eb69e7fc4 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -1381,14 +1381,14 @@ config ATARI_SCSI
tristate "Atari native SCSI support"
depends on ATARI && SCSI
select SCSI_SPI_ATTRS
-   select NVRAM
---help---
  If you have an Atari with built-in NCR5380 SCSI controller (TT,
  Falcon, ...) say Y to get it supported. Of course also, if you have
  a compatible SCSI controller (e.g. for Medusa).

- To compile this driver as a module, choose M here: the
- module will be called atari_scsi.
+ To compile this driver as a module, choose M here: the module will
+ be called atari_scsi. If you also enable NVRAM support, the SCSI
+ host's ID is taken from the setting in TT RTC NVRAM.

  This driver supports both styles of NCR integration into the
  system: the TT style (separate DMA), and the Falcon style (via
diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 89f5154c40b6..99e5729d910d 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -755,9 +755,10 @@ static int __init atari_scsi_probe(struct  
platform_device *pdev)

if (ATARIHW_PRESENT(TT_SCSI) && setup_sg_tablesize >= 0)
atari_scsi_template.sg_tablesize = setup_sg_tablesize;

-   if (setup_hostid >= 0) {
+   if (setup_hostid >= 0)
atari_scsi_template.this_id = setup_hostid & 7;
-   } else {
+#ifdef CONFIG_NVRAM
+   else


Such ifdefs should be avoided in C files.
It would be better to use

} else if (IS_ENABLED(CONFIG_NVRAM)) {


/* Test if a host id is set in the NVRam */
if (ATARIHW_PRESENT(TT_CLK) && nvram_check_checksum()) {
unsigned char b = nvram_read_byte(16);
@@ -768,7 +769,7 @@ static int __init atari_scsi_probe(struct  
platform_device *pdev)

if (b & 0x80)
atari_scsi_template.this_id = b & 7;
}
-   }
+#endif

/* If running on a Falcon and if there's TT-Ram (i.e., more than one
 * memory block, since there's always ST-Ram in a Falcon), then
--
2.19.2

Re: [PATCH v8 02/25] m68k/atari: Move Atari-specific code out of drivers/char/nvram.c

[LEROY Christophe]

Finn Thain  a écrit :


Move the m68k-specific code out of the driver to make the driver generic.

I've used 'SPDX-License-Identifier: GPL-2.0+' for the new file because the
old file is covered by MODULE_LICENSE("GPL").

Signed-off-by: Finn Thain 
Tested-by: Christian T. Steigies 
Acked-by: Geert Uytterhoeven 
---
Changed since v7:
 - Added SPDX-License-Identifier.
---
 arch/m68k/atari/Makefile |   2 +
 arch/m68k/atari/nvram.c  | 243 +
 drivers/char/nvram.c | 280 +--
 3 files changed, 280 insertions(+), 245 deletions(-)
 create mode 100644 arch/m68k/atari/nvram.c

diff --git a/arch/m68k/atari/Makefile b/arch/m68k/atari/Makefile
index 0cac723306f9..0b86bb6cfa87 100644
--- a/arch/m68k/atari/Makefile
+++ b/arch/m68k/atari/Makefile
@@ -6,3 +6,5 @@ obj-y   := config.o time.o debug.o ataints.o stdma.o \
atasound.o stram.o

 obj-$(CONFIG_ATARI_KBD_CORE)   += atakeyb.o
+
+obj-$(CONFIG_NVRAM:m=y)+= nvram.o
diff --git a/arch/m68k/atari/nvram.c b/arch/m68k/atari/nvram.c
new file mode 100644
index ..3e620ee955ba
--- /dev/null
+++ b/arch/m68k/atari/nvram.c
@@ -0,0 +1,243 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * CMOS/NV-RAM driver for Atari. Adapted from drivers/char/nvram.c.
+ * Copyright (C) 1997 Roman Hodek 
+ * idea by and with help from Richard Jelinek 
+ * Portions copyright (c) 2001,2002 Sun Microsystems (thoc...@sun.com)
+ * Further contributions from Cesar Barros, Erik Gilling, Tim Hockin and
+ * Wim Van Sebroeck.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define NVRAM_BYTES50
+
+/* It is worth noting that these functions all access bytes of general
+ * purpose memory in the NVRAM - that is to say, they all add the
+ * NVRAM_FIRST_BYTE offset. Pass them offsets into NVRAM as if you did not
+ * know about the RTC cruft.
+ */
+
+/* Note that *all* calls to CMOS_READ and CMOS_WRITE must be done with
+ * rtc_lock held. Due to the index-port/data-port design of the RTC, we
+ * don't want two different things trying to get to it at once. (e.g. the
+ * periodic 11 min sync from kernel/time/ntp.c vs. this driver.)
+ */
+
+unsigned char __nvram_read_byte(int i)
+{
+   return CMOS_READ(NVRAM_FIRST_BYTE + i);
+}
+
+unsigned char nvram_read_byte(int i)
+{
+   unsigned long flags;
+   unsigned char c;
+
+   spin_lock_irqsave(&rtc_lock, flags);
+   c = __nvram_read_byte(i);
+   spin_unlock_irqrestore(&rtc_lock, flags);
+   return c;
+}
+EXPORT_SYMBOL(nvram_read_byte);
+
+/* This races nicely with trying to read with checksum checking */
+void __nvram_write_byte(unsigned char c, int i)
+{
+   CMOS_WRITE(c, NVRAM_FIRST_BYTE + i);
+}
+
+void nvram_write_byte(unsigned char c, int i)
+{
+   unsigned long flags;
+
+   spin_lock_irqsave(&rtc_lock, flags);
+   __nvram_write_byte(c, i);
+   spin_unlock_irqrestore(&rtc_lock, flags);
+}
+
+/* On Ataris, the checksum is over all bytes except the checksum bytes
+ * themselves; these are at the very end.
+ */
+#define ATARI_CKS_RANGE_START  0
+#define ATARI_CKS_RANGE_END47
+#define ATARI_CKS_LOC  48
+
+int __nvram_check_checksum(void)
+{
+   int i;
+   unsigned char sum = 0;
+
+   for (i = ATARI_CKS_RANGE_START; i <= ATARI_CKS_RANGE_END; ++i)
+   sum += __nvram_read_byte(i);
+   return (__nvram_read_byte(ATARI_CKS_LOC) == (~sum & 0xff)) &&
+  (__nvram_read_byte(ATARI_CKS_LOC + 1) == (sum & 0xff));
+}
+
+int nvram_check_checksum(void)
+{
+   unsigned long flags;
+   int rv;
+
+   spin_lock_irqsave(&rtc_lock, flags);
+   rv = __nvram_check_checksum();
+   spin_unlock_irqrestore(&rtc_lock, flags);
+   return rv;
+}
+EXPORT_SYMBOL(nvram_check_checksum);
+
+static void __nvram_set_checksum(void)
+{
+   int i;
+   unsigned char sum = 0;
+
+   for (i = ATARI_CKS_RANGE_START; i <= ATARI_CKS_RANGE_END; ++i)
+   sum += __nvram_read_byte(i);
+   __nvram_write_byte(~sum, ATARI_CKS_LOC);
+   __nvram_write_byte(sum, ATARI_CKS_LOC + 1);
+}
+
+#ifdef CONFIG_PROC_FS
+static struct {
+   unsigned char val;
+   const char *name;
+} boot_prefs[] = {
+   { 0x80, "TOS" },
+   { 0x40, "ASV" },
+   { 0x20, "NetBSD (?)" },
+   { 0x10, "Linux" },
+   { 0x00, "unspecified" },
+};
+
+static const char * const languages[] = {
+   "English (US)",
+   "German",
+   "French",
+   "English (UK)",
+   "Spanish",
+   "Italian",
+   "6 (undefined)",
+   "Swiss (French)",
+   "Swiss (German)",
+};
+
+static const char * const dateformat[] = {
+   "MM%cDD%cYY",
+   "DD%cMM%cYY",
+   "YY%cMM%cDD",
+   "YY%cDD%cMM",
+   "4 (undefined)",
+   "5 (undefined)",
+   "6 (undefined)",
+   "7 (undefined)",
+};
+
+static const char * const colors[] = {

Re: [PATCH v8 01/25] scsi/atari_scsi: Don't select CONFIG_NVRAM

[LEROY Christophe]

Michael Schmitz  a écrit :


Hi Finn,

Am 29.12.2018 um 14:06 schrieb Finn Thain:

On Fri, 28 Dec 2018, LEROY Christophe wrote:

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index 89f5154c40b6..99e5729d910d 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -755,9 +755,10 @@ static int __init atari_scsi_probe(struct
platform_device *pdev)
if (ATARIHW_PRESENT(TT_SCSI) && setup_sg_tablesize >= 0)
atari_scsi_template.sg_tablesize = setup_sg_tablesize;

-   if (setup_hostid >= 0) {
+   if (setup_hostid >= 0)
atari_scsi_template.this_id = setup_hostid & 7;
-   } else {
+#ifdef CONFIG_NVRAM
+   else


Such ifdefs should be avoided in C files.
It would be better to use

} else if (IS_ENABLED(CONFIG_NVRAM)) {



I don't like #ifdefs either. However, as the maintainer of this file, I am
okay with this one.

The old #ifdef CONFIG_NVRAM conditional compilation convention that gets
used here and under drivers/video/fbdev could probably be improved upon
but I consider this to be out-of-scope for this series, which is
complicated enough.

And as explained in the commit log, CONFIG_NVRAM=y and CONFIG_NVRAM=m are
treaded differently by drivers. Therefore, IS_ENABLED would be incorrect.


IS_BUILTIN(CONFIG_NVRAM) is probably what Christophe really meant to suggest.


Doesn't #ifdef means either y or m ? So the same as IS_ENABLED() ?

Christophe



Or (really going out on a limb here):

IS_BUILTIN(CONFIG_NVRAM) ||
( IS_MODULE(CONFIG_ATARI_SCSI) && IS_ENABLED(CONFIG_NVRAM) )

Not that I'd advocate that, for this series.

Cheers,

Michael

Re: [PATCH v8 20/25] powerpc, fbdev: Use arch_nvram_ops methods instead of nvram_read_byte() and nvram_write_byte()

[LEROY Christophe]

Finn Thain  a écrit :


Make use of arch_nvram_ops in device drivers so that the nvram_* function
exports can be removed.

Since they are no longer global symbols, rename the PPC32 nvram_* functions
appropriately.

Signed-off-by: Finn Thain 
---
 arch/powerpc/kernel/setup_32.c | 8 
 drivers/char/generic_nvram.c   | 4 ++--
 drivers/video/fbdev/controlfb.c| 4 ++--
 drivers/video/fbdev/imsttfb.c  | 4 ++--
 drivers/video/fbdev/matrox/matroxfb_base.c | 2 +-
 drivers/video/fbdev/platinumfb.c   | 4 ++--
 drivers/video/fbdev/valkyriefb.c   | 4 ++--
 7 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/kernel/setup_32.c b/arch/powerpc/kernel/setup_32.c
index e0d045677472..bdbe6acbef11 100644
--- a/arch/powerpc/kernel/setup_32.c
+++ b/arch/powerpc/kernel/setup_32.c
@@ -152,20 +152,18 @@ __setup("l3cr=", ppc_setup_l3cr);

 #ifdef CONFIG_GENERIC_NVRAM

-unsigned char nvram_read_byte(int addr)
+static unsigned char ppc_nvram_read_byte(int addr)
 {
if (ppc_md.nvram_read_val)
return ppc_md.nvram_read_val(addr);
return 0xff;
 }
-EXPORT_SYMBOL(nvram_read_byte);

-void nvram_write_byte(unsigned char val, int addr)
+static void ppc_nvram_write_byte(unsigned char val, int addr)
 {
if (ppc_md.nvram_write_val)
ppc_md.nvram_write_val(addr, val);
 }
-EXPORT_SYMBOL(nvram_write_byte);

 static ssize_t ppc_nvram_get_size(void)
 {
@@ -182,6 +180,8 @@ static long ppc_nvram_sync(void)
 }

 const struct nvram_ops arch_nvram_ops = {
+   .read_byte  = ppc_nvram_read_byte,
+   .write_byte = ppc_nvram_write_byte,
.get_size   = ppc_nvram_get_size,
.sync   = ppc_nvram_sync,
 };
diff --git a/drivers/char/generic_nvram.c b/drivers/char/generic_nvram.c
index f32d5663de95..41b76bf9614e 100644
--- a/drivers/char/generic_nvram.c
+++ b/drivers/char/generic_nvram.c
@@ -48,7 +48,7 @@ static ssize_t read_nvram(struct file *file, char  
__user *buf,

if (*ppos >= nvram_len)
return 0;
for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count)
-   if (__put_user(nvram_read_byte(i), p))
+   if (__put_user(arch_nvram_ops.read_byte(i), p))


Instead of modifying all drivers (in this patch and previous ones  
related to other arches), wouldn't it be better to add helpers like  
the following in nvram.h:


Static inline unsigned char nvram_read_byte(int addr)
{
return arch_nvram_ops.read_byte(addr);
}

Christophe


return -EFAULT;
*ppos = i;
return p - buf;
@@ -68,7 +68,7 @@ static ssize_t write_nvram(struct file *file,  
const char __user *buf,

for (i = *ppos; count > 0 && i < nvram_len; ++i, ++p, --count) {
if (__get_user(c, p))
return -EFAULT;
-   nvram_write_byte(c, i);
+   arch_nvram_ops.write_byte(c, i);
}
*ppos = i;
return p - buf;
diff --git a/drivers/video/fbdev/controlfb.c  
b/drivers/video/fbdev/controlfb.c

index 9cb0ef7ac29e..27ff33ccafcb 100644
--- a/drivers/video/fbdev/controlfb.c
+++ b/drivers/video/fbdev/controlfb.c
@@ -413,7 +413,7 @@ static int __init init_control(struct fb_info_control *p)
/* Try to pick a video mode out of NVRAM if we have one. */
 #ifdef CONFIG_NVRAM
if (default_cmode == CMODE_NVRAM) {
-   cmode = nvram_read_byte(NV_CMODE);
+   cmode = arch_nvram_ops.read_byte(NV_CMODE);
if(cmode < CMODE_8 || cmode > CMODE_32)
cmode = CMODE_8;
} else
@@ -421,7 +421,7 @@ static int __init init_control(struct fb_info_control *p)
cmode=default_cmode;
 #ifdef CONFIG_NVRAM
if (default_vmode == VMODE_NVRAM) {
-   vmode = nvram_read_byte(NV_VMODE);
+   vmode = arch_nvram_ops.read_byte(NV_VMODE);
if (vmode < 1 || vmode > VMODE_MAX ||
control_mac_modes[vmode - 1].m[full] < cmode) {
sense = read_control_sense(p);
diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index 8d231591ff0e..e9f3b8914145 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -1393,12 +1393,12 @@ static void init_imstt(struct fb_info *info)
int vmode = init_vmode, cmode = init_cmode;

if (vmode == -1) {
-   vmode = nvram_read_byte(NV_VMODE);
+   vmode = arch_nvram_ops.read_byte(NV_VMODE);
if (vmode <= 0 || vmode > VMODE_MAX)
vmode = VMODE_640_480_67;
}
if (cmode == -1) {
-   cmode = nvram_read_byte(NV_CMODE);
+   cmode = arch_nvram_ops.read_byte(NV_CMODE);
if (cmode < CMODE_8 || cmode > CMODE_32)
 

Re: [PATCH v8 01/25] scsi/atari_scsi: Don't select CONFIG_NVRAM

[LEROY Christophe]

Arnd Bergmann  a écrit :


On Sat, Dec 29, 2018 at 3:51 AM Michael Schmitz  wrote:


Hi Finn,

Am 29.12.2018 um 15:34 schrieb Finn Thain:
> On Sat, 29 Dec 2018, Michael Schmitz wrote:
>
>>
>> IS_BUILTIN(CONFIG_NVRAM) is probably what Christophe really  
meant to suggest.

>>
>> Or (really going out on a limb here):
>>
>> IS_BUILTIN(CONFIG_NVRAM) ||
>> ( IS_MODULE(CONFIG_ATARI_SCSI) && IS_ENABLED(CONFIG_NVRAM) )
>>
>> Not that I'd advocate that, for this series.
>>
>
> Well, you are a maintainer for atari_scsi.c.
>
> Are you saying that you want IS_BUILTIN(CONFIG_NVRAM) used here instead of
> ifdef?

No, just pointing out that there would be a way to avoid the ifdef
without messing up driver behaviour. I'm fine with the ifdef - not least
because it clearly eliminates code that would be unreachable.

(On second thought - I don't want to speculate whether there's weird
compiler options that could result in the nvram_check_checksum and
nvram_read_bytes symbols to still be referenced in the final link, even
though IS_BUILTIN(CONFIG_NVRAM) always evaluates to false. Best leave
this as-is.)


As far as I know, it's totally reliable with the supported compilers  
(gcc-4.6+).

In the older compilers (e.g. 4.1), there was a corner case, where it could
have failed to eliminate a function that was only referenced through  
a pointer

from a discarded variable, but a plain IS_ENABLED() check like the one here
was still ok, and lots of code relies on that.

Other than that, I agree either way is totally fine here, so no objections
to using the #ifdef.


As far as I know, kernel codying style promotes the use of  
IS_ENABLED() etc. instead of #ifdefs when possible.


Christophe



  Arnd

Re: [PATCH v8 13/25] m68k: Dispatch nvram_ops calls to Atari or Mac functions

[LEROY Christophe]

Finn Thain  a écrit :


On Sat, 29 Dec 2018, Arnd Bergmann wrote:

On Wed, Dec 26, 2018 at 1:43 AM Finn Thain  
 wrote:


> +
> +static ssize_t m68k_nvram_get_size(void)
> +{
> +   if (MACH_IS_ATARI)
> +   return atari_nvram_get_size();
> +   else if (MACH_IS_MAC)
> +   return mac_pram_get_size();
> +   return -ENODEV;
> +}
> +
> +/* Atari device drivers call .read (to get checksum validation) whereas
> + * Mac and PowerMac device drivers just use .read_byte.
> + */
> +const struct nvram_ops arch_nvram_ops = {
> +#ifdef CONFIG_MAC
> +   .read_byte  = m68k_nvram_read_byte,
> +   .write_byte = m68k_nvram_write_byte,
> +#endif
> +#ifdef CONFIG_ATARI
> +   .read   = m68k_nvram_read,
> +   .write  = m68k_nvram_write,
> +   .set_checksum   = m68k_nvram_set_checksum,
> +   .initialize = m68k_nvram_initialize,
> +#endif
> +   .get_size   = m68k_nvram_get_size,
> +};
> +EXPORT_SYMBOL(arch_nvram_ops);

Since the operations are almost entirely distinct, why not have two
separate 'nvram_ops' instances here that each refer to just
the set they actually need?



The reason for that is that I am alergic to code duplication. But I'll
change it if you think it matters. BTW, this patch has already been acked
by Geert.


I agree it would be cleaner, as it would also avoid this  
m68k_nvram_get_size() wouldn't it ?


I don't see potential code duplication here, do you ?

Christophe




I was actually expecting one more patch here that would make the
arch_nvram_ops a pointer to one of multiple structures, which would
be easier to do with multiple copies, but I suppose there is no need
for that here (there might be on ppc, I have to look again).



Yes, I considered that too. I picked the variation that makes everything
const.

--


   Arnd

Re: [PATCH 1/2] powerpc/4xx/ocm: Fix phys_addr_t printf warnings

[LEROY Christophe]

Michael Ellerman  a écrit :


Currently the code produces several warnings, eg:

  arch/powerpc/platforms/4xx/ocm.c:240:38: error: format '%llx'
  expects argument of type 'long long unsigned int', but argument 3
  has type 'phys_addr_t {aka unsigned int}'
 seq_printf(m, "PhysAddr : 0x%llx\n", ocm->phys);
   ~~~^ ~

Fix it by using the special %pa[p] format for printing phys_addr_t.
Note we need to pass the value by reference for the special specifier
to work.


When I fixed the same problem in prom.c, you suggested to simply cast  
it to unsigned long long. Is this a better solution ?


Christophe



Signed-off-by: Michael Ellerman 
---
 arch/powerpc/platforms/4xx/ocm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/4xx/ocm.c  
b/arch/powerpc/platforms/4xx/ocm.c

index c22b099c42f1..a1aaa1569d7c 100644
--- a/arch/powerpc/platforms/4xx/ocm.c
+++ b/arch/powerpc/platforms/4xx/ocm.c
@@ -237,12 +237,12 @@ static int ocm_debugfs_show(struct seq_file  
*m, void *v)

continue;

seq_printf(m, "PPC4XX OCM   : %d\n", ocm->index);
-   seq_printf(m, "PhysAddr : 0x%llx\n", ocm->phys);
+   seq_printf(m, "PhysAddr : %pa[p]\n", &(ocm->phys));
seq_printf(m, "MemTotal : %d Bytes\n", ocm->memtotal);
seq_printf(m, "MemTotal(NC) : %d Bytes\n", ocm->nc.memtotal);
seq_printf(m, "MemTotal(C)  : %d Bytes\n\n", ocm->c.memtotal);

-   seq_printf(m, "NC.PhysAddr  : 0x%llx\n", ocm->nc.phys);
+   seq_printf(m, "NC.PhysAddr  : %pa[p]\n", &(ocm->nc.phys));
seq_printf(m, "NC.VirtAddr  : 0x%p\n", ocm->nc.virt);
seq_printf(m, "NC.MemTotal  : %d Bytes\n", ocm->nc.memtotal);
seq_printf(m, "NC.MemFree   : %d Bytes\n", ocm->nc.memfree);
@@ -252,7 +252,7 @@ static int ocm_debugfs_show(struct seq_file *m, void *v)
blk->size, blk->owner);
}

-   seq_printf(m, "\nC.PhysAddr   : 0x%llx\n", ocm->c.phys);
+   seq_printf(m, "\nC.PhysAddr   : %pa[p]\n", &(ocm->c.phys));
seq_printf(m, "C.VirtAddr   : 0x%p\n", ocm->c.virt);
seq_printf(m, "C.MemTotal   : %d Bytes\n", ocm->c.memtotal);
seq_printf(m, "C.MemFree: %d Bytes\n", ocm->c.memfree);
--
2.17.2

Re: [PATCH v5] soc/fsl/qe: fix err handling of ucc_of_parse_tdm

[LEROY Christophe]

Peng Hao  a écrit :


From: Wen Yang 

Currently there are some issues with the ucc_of_parse_tdm function:
1, a possible null pointer dereference in ucc_of_parse_tdm,
detected by the semantic patch deref_null.cocci,
with the following warning:
drivers/soc/fsl/qe/qe_tdm.c:177:21-24: ERROR: pdev is NULL but dereferenced.
2, dev gets modified, so in any case that devm_iounmap() will fail
even when the new pdev is valid, because the iomap was done with a
 different pdev.
3, there is no driver bind with the "fsl,t1040-qe-si" or
"fsl,t1040-qe-siram" device. So allocating resources using devm_*()
with these devices won't provide a cleanup path for these resources
when the caller fails.

This patch fixes them.

Suggested-by: Li Yang 
Suggested-by: Christophe LEROY 
Signed-off-by: Wen Yang 
Reviewed-by: Peng Hao 
CC: Julia Lawall 
CC: Zhao Qiang 
CC: David S. Miller 
CC: net...@vger.kernel.org
CC: linuxppc-dev@lists.ozlabs.org
CC: linux-ker...@vger.kernel.org
---


In order to ease review, could add the list of changes between each  
version of the patch ? Usually we do it at this place so that it is  
available for reviewers but not part of the commit text.


Christophe

 drivers/net/wan/fsl_ucc_hdlc.c | 62  
+-

 drivers/soc/fsl/qe/qe_tdm.c| 55 -
 2 files changed, 61 insertions(+), 56 deletions(-)

diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 839fa77..f30a040 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -1057,6 +1057,54 @@ static const struct net_device_ops uhdlc_ops = {
.ndo_tx_timeout = uhdlc_tx_timeout,
 };

+static int hdlc_map_iomem(char *name, int init_flag, void __iomem **ptr)
+{
+   struct device_node *np;
+   struct platform_device *pdev;
+   struct resource *res;
+   static int siram_init_flag;
+   int ret = 0;
+
+   np = of_find_compatible_node(NULL, NULL, name);
+   if (!np)
+   return -EINVAL;
+
+   pdev = of_find_device_by_node(np);
+   if (!pdev) {
+   pr_err("%pOFn: failed to lookup pdev\n", np);
+   of_node_put(np);
+   return -EINVAL;
+   }
+
+   of_node_put(np);
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   if (!res) {
+   ret = -EINVAL;
+   goto error_put_device;
+   }
+   *ptr = ioremap(res->start, resource_size(res));
+   if (!*ptr) {
+   ret = -ENOMEM;
+   goto error_put_device;
+   }
+
+   /* We've remapped the addresses, and we don't need the device any
+* more, so we should release it.
+*/
+   put_device(&pdev->dev);
+
+   if (init_flag && siram_init_flag == 0) {
+   memset_io(*ptr, 0, resource_size(res));
+   siram_init_flag = 1;
+   }
+   return  0;
+
+error_put_device:
+   put_device(&pdev->dev);
+
+   return ret;
+}
+
 static int ucc_hdlc_probe(struct platform_device *pdev)
 {
struct device_node *np = pdev->dev.of_node;
@@ -1151,6 +1199,15 @@ static int ucc_hdlc_probe(struct  
platform_device *pdev)

ret = ucc_of_parse_tdm(np, utdm, ut_info);
if (ret)
goto free_utdm;
+
+   ret = hdlc_map_iomem("fsl,t1040-qe-si", 0,
+(void __iomem **)&utdm->si_regs);
+   if (ret)
+   goto free_utdm;
+   ret = hdlc_map_iomem("fsl,t1040-qe-siram", 1,
+(void __iomem **)&utdm->siram);
+   if (ret)
+   goto unmap_si_regs;
}

if (of_property_read_u16(np, "fsl,hmask", &uhdlc_priv->hmask))
@@ -1159,7 +1216,7 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
ret = uhdlc_init(uhdlc_priv);
if (ret) {
dev_err(&pdev->dev, "Failed to init uhdlc\n");
-   goto free_utdm;
+   goto undo_uhdlc_init;
}

dev = alloc_hdlcdev(uhdlc_priv);
@@ -1188,6 +1245,9 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
 free_dev:
free_netdev(dev);
 undo_uhdlc_init:
+   iounmap(utdm->siram);
+unmap_si_regs:
+   iounmap(utdm->si_regs);
 free_utdm:
if (uhdlc_priv->tsa)
kfree(utdm);
diff --git a/drivers/soc/fsl/qe/qe_tdm.c b/drivers/soc/fsl/qe/qe_tdm.c
index f78c346..76480df 100644
--- a/drivers/soc/fsl/qe/qe_tdm.c
+++ b/drivers/soc/fsl/qe/qe_tdm.c
@@ -44,10 +44,6 @@ int ucc_of_parse_tdm(struct device_node *np,  
struct ucc_tdm *utdm,

const char *sprop;
int ret = 0;
u32 val;
-   struct resource *res;
-   struct device_node *np2;
-   static int siram_init_flag;
-   struct platform_device *pdev;

sprop = of_get_property(np, "fsl,rx-sync-clock", NULL);
if (sprop) {
@@ -124,57 +120,6 @@ int ucc_of_parse_tdm(struct devic

Re: [PATCH v3 1/2] selftests/powerpc: Add MSR bits

[LEROY Christophe]

Breno Leitao  a écrit :


This patch simply adds definitions for the MSR bits and some macros to
test for MSR TM bits.

This was copied from arch/powerpc/include/asm/reg.h generic MSR part.


Can't we find a way to avoid duplicating such defines ?

Christophe



Signed-off-by: Breno Leitao 
---
 tools/testing/selftests/powerpc/include/reg.h | 45 +++
 1 file changed, 45 insertions(+)

diff --git a/tools/testing/selftests/powerpc/include/reg.h  
b/tools/testing/selftests/powerpc/include/reg.h

index 52b4710469d2..b67a85404255 100644
--- a/tools/testing/selftests/powerpc/include/reg.h
+++ b/tools/testing/selftests/powerpc/include/reg.h
@@ -77,6 +77,51 @@
 #define TEXASR_TE  0x0400
 #define TEXASR_ROT 0x0200

+/* MSR register bits */
+#define MSR_SF_LG   63  /* Enable 64 bit mode */
+#define MSR_ISF_LG  61  /* Interrupt 64b mode valid  
on 630 */

+#define MSR_HV_LG   60  /* Hypervisor state */
+#define MSR_TS_T_LG 34  /* Trans Mem state: Transactional */
+#define MSR_TS_S_LG 33  /* Trans Mem state: Suspended */
+#define MSR_TS_LG   33  /* Trans Mem state (2 bits) */
+#define MSR_TM_LG   32  /* Trans Mem Available */
+#define MSR_VEC_LG  25  /* Enable AltiVec */
+#define MSR_VSX_LG  23  /* Enable VSX */
+#define MSR_POW_LG  18  /* Enable Power Management */
+#define MSR_WE_LG   18  /* Wait State Enable */
+#define MSR_TGPR_LG 17  /* TLB Update registers in use */
+#define MSR_CE_LG   17  /* Critical Interrupt Enable */
+#define MSR_ILE_LG  16  /* Interrupt Little Endian */
+#define MSR_EE_LG   15  /* External Interrupt Enable */
+#define MSR_PR_LG   14  /* Problem State /  
Privilege Level */

+#define MSR_FP_LG   13  /* Floating Point enable */
+#define MSR_ME_LG   12  /* Machine Check Enable */
+#define MSR_FE0_LG  11  /* Floating Exception mode 0 */
+#define MSR_SE_LG   10  /* Single Step */
+#define MSR_BE_LG   9   /* Branch Trace */
+#define MSR_DE_LG   9   /* Debug Exception Enable */
+#define MSR_FE1_LG  8   /* Floating Exception mode 1 */
+#define MSR_IP_LG   6   /* Exception prefix 0x000/0xFFF */
+#define MSR_IR_LG   5   /* Instruction Relocate */
+#define MSR_DR_LG   4   /* Data Relocate */
+#define MSR_PE_LG   3   /* Protection Enable */
+#define MSR_PX_LG   2   /* Protection Exclusive Mode */
+#define MSR_PMM_LG  2   /* Performance monitor */
+#define MSR_RI_LG   1   /* Recoverable Exception */
+#define MSR_LE_LG   0   /* Little Endian */
+
+#ifdef __ASSEMBLY__
+#define __MASK(X)   (1<<(X))
+#else
+#define __MASK(X)   (1UL<<(X))
+#endif
+
+/* macros to check TM MSR bits */
+#define MSR_TM  __MASK(MSR_TM_LG) /* Transactional Mem  
Available */

+#define MSR_TS_S__MASK(MSR_TS_S_LG)   /* Transaction Suspended */
+#define MSR_TS_T__MASK(MSR_TS_T_LG)   /* Transaction  
Transactional */

+#define MSR_TS_MASK (MSR_TS_T | MSR_TS_S) /* Transaction State bits */
+
 /* Vector Instructions */
 #define VSX_XX1(xs, ra, rb)(((xs) & 0x1f) << 21 | ((ra) << 16) |  \
 ((rb) << 11) | (((xs) >> 5)))
--
2.19.0

Re: [PATCH] Remove 'type' argument from access_ok() function

[LEROY Christophe]

Mathieu Malaterre  a écrit :


In commit 05a4ab823983 ("powerpc/uaccess: fix warning/error with
access_ok()") an attempt was made to remove a warning by referencing the
variable `type`, however in commit 96d4f267e40f ("Remove 'type' argument
from access_ok() function") the variable `type` has been removed.

Revert commit 05a4ab823983 ("powerpc/uaccess: fix warning/error with
access_ok()") to fix the following compilation error:

  arch/powerpc/include/asm/uaccess.h:66:32: error: ‘type’ undeclared  
(first use in this function)


Signed-off-by: Mathieu Malaterre 


Should you add a Fixes:  96d4f267e40f ?

Note that sparc arch will have the same issue.

Christophe


---
 arch/powerpc/include/asm/uaccess.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/uaccess.h  
b/arch/powerpc/include/asm/uaccess.h

index b31bf45eebd4..5f0c98e511a0 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -63,7 +63,7 @@ static inline int __access_ok(unsigned long addr,  
unsigned long size,

 #endif

 #define access_ok(addr, size)  \
-   (__chk_user_ptr(addr), (void)(type),\
+   (__chk_user_ptr(addr),  \
 __access_ok((__force unsigned long)(addr), (size), get_fs()))

 /*
--
2.19.2

Re: [PATCH v13 00/10] powerpc: Switch to CONFIG_THREAD_INFO_IN_TASK

[LEROY Christophe]

Michael Ellerman  a écrit :


Christophe Leroy  writes:


The purpose of this serie is to activate CONFIG_THREAD_INFO_IN_TASK which
moves the thread_info into task_struct.

Moving thread_info into task_struct has the following advantages:
- It protects thread_info from corruption in the case of stack
overflows.
- Its address is harder to determine if stack addresses are
leaked, making a number of attacks more difficult.

Changes since v12:
 - Patch 1: Taken comment from Mike (re-introduced the 'panic' in  
case memblock allocation fails in setup_64.c
 - Patch 1: Added alloc_stack() function in setup_32.c to also  
panic in case of allocation failure.


Hi Christophe,

I can't get this series to boot on qemu mac99. I'm getting eg:


Problem new with version 13 or it is the first time you test ?



[0.981514] NFS: Registering the id_resolver key type
[0.981752] Key type id_resolver registered
[0.981868] Key type id_legacy registered
[0.995711] Unrecoverable exception 0 at 0 (msr=0)
[0.996091] Oops: Unrecoverable exception, sig: 6 [#1]
[0.996314] BE PAGE_SIZE=4K MMU=Hash PowerMac
[0.996617] Modules linked in:
[0.996869] CPU: 0 PID: 416 Comm: modprobe Not tainted  
5.0.0-rc2-gcc-7.3.0-00043-g53f2de798792 #342


Comm:modprobe  ==> Something wrong with modules ? I never tested with  
CONFIG_MODULES.


Christophe


[0.997138] NIP:   LR:  CTR: 
[0.997309] REGS: ef237f50 TRAP:    Not tainted   
(5.0.0-rc2-gcc-7.3.0-00043-g53f2de798792)

[0.997508] MSR:   <>  CR:   XER: 
[0.997712]
[0.997712] GPR00:  ef238000     
  
[0.997712] GPR08:       
 c006477c ef13d8c0
[0.997712] GPR16:       
  
[0.997712] GPR24:       
  

[0.998671] NIP []   (null)
[0.998774] LR []   (null)
[0.998895] Call Trace:
[0.999030] Instruction dump:
[0.999320]        
 
[0.999546]     6000   
 

[1.23] ---[ end trace 925ea3419844fe68 ]---

I haven't had time to dig any further.

cheers

Re: [PATCH v4 0/6] powerpc/fsl: Speculation barrier for NXP PowerPC Book3E

[LEROY Christophe]

Diana Craciun  a écrit :


Implement barrier_nospec for NXP PowerPC Book3E processors.

Diana Craciun (6):
  Disable the speculation barrier from the command line
  Document nospectre_v1 kernel parameter.
  Make stf barrier PPC_BOOK3S_64 specific.
  Enable cpu vulnerabilities reporting for NXP PPC BOOK3E
  Add barrier_nospec implementation for NXP PowerPC Book3E
  powerpc/fsl: Sanitize the syscall table for NXP PowerPC 32 bit
platforms


This list doesn't corresponds to the names of following 6 patches

Christophe



 Documentation/admin-guide/kernel-parameters.txt |  4 +++
 arch/powerpc/Kconfig|  7 -
 arch/powerpc/include/asm/barrier.h  | 12 ++---
 arch/powerpc/include/asm/setup.h|  6 -
 arch/powerpc/kernel/Makefile|  3 ++-
 arch/powerpc/kernel/entry_32.S  | 10 +++
 arch/powerpc/kernel/module.c|  4 ++-
 arch/powerpc/kernel/security.c  | 17 +++-
 arch/powerpc/kernel/setup-common.c  |  2 ++
 arch/powerpc/kernel/vmlinux.lds.S   |  4 ++-
 arch/powerpc/lib/feature-fixups.c   | 35  
-

 arch/powerpc/platforms/powernv/setup.c  |  1 -
 arch/powerpc/platforms/pseries/setup.c  |  1 -
 13 files changed, 94 insertions(+), 12 deletions(-)

--
History:
v3 --> v4
- fixed compilation issues

v2 --> v3
- addressed review comments
- included the 32bit sanitization in the same patch series

v1 --> v2
- added implementation for cpu_show_spectre_x functions
- the mitigation is no longer enabled through device tree options

2.5.5

Re: [PATCH v4 4/6] powerpc/fsl: Enable cpu vulnerabilities reporting for NXP PPC BOOK3E

[LEROY Christophe]

Diana Craciun  a écrit :


The NXP PPC Book3E platforms are not vulnerable to meltdown and
Spectre v4, so make them PPC_BOOK3S_64 specific.

Signed-off-by: Diana Craciun 
---
History:

v2-->v3
- used the existing functions for spectre v1/v2

 arch/powerpc/Kconfig   | 7 ++-
 arch/powerpc/kernel/security.c | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 9f2b75f..116c953 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -165,7 +165,7 @@ config PPC
select GENERIC_CLOCKEVENTS_BROADCASTif SMP
select GENERIC_CMOS_UPDATE
select GENERIC_CPU_AUTOPROBE
-   select GENERIC_CPU_VULNERABILITIES  if PPC_BOOK3S_64
+   select GENERIC_CPU_VULNERABILITIES  if PPC_NOSPEC


I don't understand.  You say this patch is to make something specific  
to book3s64 specific, and you are creating a new config param that  
make things less specific


Christophe


select GENERIC_IRQ_SHOW
select GENERIC_IRQ_SHOW_LEVEL
select GENERIC_SMP_IDLE_THREAD
@@ -240,6 +240,11 @@ config PPC
# Please keep this list sorted alphabetically.
#

+config PPC_NOSPEC
+bool
+default y
+depends on PPC_BOOK3S_64 || PPC_FSL_BOOK3E
+
 config GENERIC_CSUM
def_bool n

diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index 3a4e5c3..539c744 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -92,6 +92,7 @@ static __init int barrier_nospec_debugfs_init(void)
 device_initcall(barrier_nospec_debugfs_init);
 #endif /* CONFIG_DEBUG_FS */

+#ifdef CONFIG_PPC_BOOK3S_64
 ssize_t cpu_show_meltdown(struct device *dev, struct  
device_attribute *attr, char *buf)

 {
bool thread_priv;
@@ -124,6 +125,7 @@ ssize_t cpu_show_meltdown(struct device *dev,  
struct device_attribute *attr, cha


return sprintf(buf, "Vulnerable\n");
 }
+#endif

 ssize_t cpu_show_spectre_v1(struct device *dev, struct  
device_attribute *attr, char *buf)

 {
--
2.5.5

Re: [PATCH v4 4/6] powerpc/fsl: Enable cpu vulnerabilities reporting for NXP PPC BOOK3E

[LEROY Christophe]

Diana Madalina Craciun  a écrit :


On 7/17/2018 7:47 PM, LEROY Christophe wrote:

Diana Craciun  a écrit :


The NXP PPC Book3E platforms are not vulnerable to meltdown and
Spectre v4, so make them PPC_BOOK3S_64 specific.

Signed-off-by: Diana Craciun 
---
History:

v2-->v3
- used the existing functions for spectre v1/v2

 arch/powerpc/Kconfig   | 7 ++-
 arch/powerpc/kernel/security.c | 2 ++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 9f2b75f..116c953 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -165,7 +165,7 @@ config PPC
select GENERIC_CLOCKEVENTS_BROADCASTif SMP
select GENERIC_CMOS_UPDATE
select GENERIC_CPU_AUTOPROBE
-   select GENERIC_CPU_VULNERABILITIES  if PPC_BOOK3S_64
+   select GENERIC_CPU_VULNERABILITIES  if PPC_NOSPEC

I don't understand.  You say this patch is to make something specific
to book3s64 specific, and you are creating a new config param that
make things less specific

Christophe


In order to enable the vulnerabilities reporting on NXP socs I need to
enable them for PPC_FSL_BOOK3E. So they will be enabled for both
PPC_FSL_BOOK3E and PPC_BOOK3S_64. This is the reason for adding the
Kconfig. However this will enable: spectre v1/v2 and meltdown. NXP socs
are not vulnerable to meltdown, so I made the meltdown reporting
PPC_BOOK3S_64 specific. I guess I can have the PPC_NOSPEC definition in
a separate patch to be more clear.


Yes you can. Or keep it as a single patch and add the details you gave  
me in the patch description.


Christophe



Diana




select GENERIC_IRQ_SHOW
select GENERIC_IRQ_SHOW_LEVEL
select GENERIC_SMP_IDLE_THREAD
@@ -240,6 +240,11 @@ config PPC
# Please keep this list sorted alphabetically.
#

+config PPC_NOSPEC
+bool
+default y
+depends on PPC_BOOK3S_64 || PPC_FSL_BOOK3E
+
 config GENERIC_CSUM
def_bool n

diff --git a/arch/powerpc/kernel/security.c  
b/arch/powerpc/kernel/security.c

index 3a4e5c3..539c744 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -92,6 +92,7 @@ static __init int barrier_nospec_debugfs_init(void)
 device_initcall(barrier_nospec_debugfs_init);
 #endif /* CONFIG_DEBUG_FS */

+#ifdef CONFIG_PPC_BOOK3S_64
 ssize_t cpu_show_meltdown(struct device *dev, struct
device_attribute *attr, char *buf)
 {
bool thread_priv;
@@ -124,6 +125,7 @@ ssize_t cpu_show_meltdown(struct device *dev,
struct device_attribute *attr, cha

return sprintf(buf, "Vulnerable\n");
 }
+#endif

 ssize_t cpu_show_spectre_v1(struct device *dev, struct
device_attribute *attr, char *buf)
 {
--
2.5.5

Re: [PATCH 2/7] powerpc/traps: Return early in show_signal_msg()

[LEROY Christophe]

Murilo Opsfelder Araujo  a écrit :


Modify logic of show_signal_msg() to return early, if possible.  Replace
printk_ratelimited() by printk() and a default rate limit burst to limit
displaying unhandled signals messages.


Can you explain more the benefits of this change ?

Christophe



Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index cbd3dc365193..4faab4705774 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -301,6 +301,13 @@ void user_single_step_siginfo(struct task_struct *tsk,
info->si_addr = (void __user *)regs->nip;
 }

+static bool show_unhandled_signals_ratelimited(void)
+{
+   static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
+ DEFAULT_RATELIMIT_BURST);
+   return show_unhandled_signals && __ratelimit(&rs);
+}
+
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
unsigned long addr)
 {
@@ -309,11 +316,12 @@ static void show_signal_msg(int signr, struct  
pt_regs *regs, int code,

const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
"at %016lx nip %016lx lr %016lx code %x\n";

-   if (show_unhandled_signals && unhandled_signal(current, signr)) {
-   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
-   }
+   if (!unhandled_signal(current, signr))
+   return;
+
+   printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
+  current->comm, current->pid, signr,
+  addr, regs->nip, regs->link, code);
 }

 void _exception_pkey(int signr, struct pt_regs *regs, int code,
@@ -326,7 +334,8 @@ void _exception_pkey(int signr, struct pt_regs  
*regs, int code,

return;
}

-   show_signal_msg(signr, regs, code, addr);
+   if (show_unhandled_signals_ratelimited())
+   show_signal_msg(signr, regs, code, addr);

if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
local_irq_enable();
--
2.17.1

Re: [PATCH 6/7] powerpc/traps: Print signal name for unhandled signals

[LEROY Christophe]

Murilo Opsfelder Araujo  a écrit :


This adds a human-readable name in the unhandled signal message.

Before this patch, a page fault looked like:

Jul 11 16:04:11 localhost kernel: pandafault[6303]: unhandled  
signal 11 at 17d0 nip 161c lr  
7fff93c55100 code 2 in pandafault[1000+1]


After this patch, a page fault looks like:

Jul 11 18:14:48 localhost kernel: pandafault[6352]: segfault  
(11) at 00013a2a09f8 nip 00013a2a086c lr 7fffb63e5100  
code 2 in pandafault[13a2a+1]


Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 43 +
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index e6c43ef9fb50..e55ee639d010 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler);
 #define TM_DEBUG(x...) do { } while(0)
 #endif

+static const char *signames[SIGRTMIN + 1] = {
+   "UNKNOWN",
+   "SIGHUP", // 1
+   "SIGINT", // 2
+   "SIGQUIT",// 3
+   "SIGILL", // 4
+   "unhandled trap", // 5 = SIGTRAP
+   "SIGABRT",// 6 = SIGIOT
+   "bus error",  // 7 = SIGBUS
+   "floating point exception",   // 8 = SIGFPE
+   "illegal instruction",// 9 = SIGILL
+   "SIGUSR1",// 10
+   "segfault",   // 11 = SIGSEGV
+   "SIGUSR2",// 12
+   "SIGPIPE",// 13
+   "SIGALRM",// 14
+   "SIGTERM",// 15
+   "SIGSTKFLT",  // 16
+   "SIGCHLD",// 17
+   "SIGCONT",// 18
+   "SIGSTOP",// 19
+   "SIGTSTP",// 20
+   "SIGTTIN",// 21
+   "SIGTTOU",// 22
+   "SIGURG", // 23
+   "SIGXCPU",// 24
+   "SIGXFSZ",// 25
+   "SIGVTALRM",  // 26
+   "SIGPROF",// 27
+   "SIGWINCH",   // 28
+   "SIGIO",  // 29 = SIGPOLL = SIGLOST
+   "SIGPWR", // 30
+   "SIGSYS", // 31 = SIGUNUSED
+};
+
 /*
  * Trap & Exception support
  */
@@ -314,10 +349,10 @@ static void show_signal_msg(int signr, struct  
pt_regs *regs, int code,

if (!unhandled_signal(current, signr))
return;

-   pr_info("%s[%d]: unhandled signal %d at "REG_FMT \
-   " nip "REG_FMT" lr "REG_FMT" code %x",
-   current->comm, current->pid, signr, addr,
-   regs->nip, regs->link, code);
+   pr_info("%s[%d]: %s (%d) at "REG_FMT" nip "REG_FMT \
+   " lr "REG_FMT" code %x",
+   current->comm, current->pid, signames[signr],
+   signr, addr, regs->nip, regs->link, code);


Are we sure that signr is always within the limits of the table ?

Christophe


print_vma_addr(KERN_CONT " in ", regs->nip);

--
2.17.1

Re: [PATCH 7/7] powerpc/traps: Show instructions on exceptions

[LEROY Christophe]

Murilo Opsfelder Araujo  a écrit :


Move show_instructions() declaration to arch/powerpc/include/asm/stacktrace.h
and include asm/stracktrace.h in arch/powerpc/kernel/process.c,  
which contains

the implementation.

Modify show_instructions() not to call __kernel_text_address(), allowing
userspace instruction dump.  probe_kernel_address(), which returns -EFAULT if
something goes wrong, is still being called.

Call show_instructions() in arch/powerpc/kernel/traps.c to dump  
instructions at

faulty location, useful to debugging.


Shouldn't this part be in a second patch ?

Wouldn't it be better to also see regs in addition if we want to use  
this to understand what happened ?

So you could call show_regs() instead of show_instructions() ?

Christophe



Before this patch, an unhandled signal message looked like:

Jul 24 09:57:00 localhost kernel: pandafault[10524]: segfault  
(11) at 17d0 nip 161c lr 7fffbd295100  
code 2 in pandafault[1000+1]


After this patch, it looks like:

Jul 24 09:57:00 localhost kernel: pandafault[10524]: segfault  
(11) at 17d0 nip 161c lr 7fffbd295100  
code 2 in pandafault[1000+1]

Jul 24 09:57:00 localhost kernel: Instruction dump:
Jul 24 09:57:00 localhost kernel: 4bfffeec 4bfffee8 3c401002  
38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
Jul 24 09:57:00 localhost kernel: 392988d0 f93f0020 e93f0020  
39400048 <9949> 3920 7d234b78 383f0040


Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/include/asm/stacktrace.h | 7 +++
 arch/powerpc/kernel/process.c | 6 +++---
 arch/powerpc/kernel/traps.c   | 3 +++
 3 files changed, 13 insertions(+), 3 deletions(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

diff --git a/arch/powerpc/include/asm/stacktrace.h  
b/arch/powerpc/include/asm/stacktrace.h

new file mode 100644
index ..46e5ef451578
--- /dev/null
+++ b/arch/powerpc/include/asm/stacktrace.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_POWERPC_STACKTRACE_H
+#define _ASM_POWERPC_STACKTRACE_H
+
+void show_instructions(struct pt_regs *regs);
+
+#endif /* _ASM_POWERPC_STACKTRACE_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index b1af3390249c..ee1d63e03c52 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -52,6 +52,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1261,7 +1262,7 @@ struct task_struct *__switch_to(struct  
task_struct *prev,


 static int instructions_to_print = 16;

-static void show_instructions(struct pt_regs *regs)
+void show_instructions(struct pt_regs *regs)
 {
int i;
unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
@@ -1283,8 +1284,7 @@ static void show_instructions(struct pt_regs *regs)
pc = (unsigned long)phys_to_virt(pc);
 #endif

-   if (!__kernel_text_address(pc) ||
-probe_kernel_address((unsigned int __user *)pc, instr)) {
+   if (probe_kernel_address((unsigned int __user *)pc, instr)) {
pr_cont(" ");
} else {
if (regs->nip == pc)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index e55ee639d010..3beca17ac1b1 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -70,6 +70,7 @@
 #include 
 #include 
 #include 
+#include 

 #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC_CORE)
 int (*__debugger)(struct pt_regs *regs) __read_mostly;
@@ -357,6 +358,8 @@ static void show_signal_msg(int signr, struct  
pt_regs *regs, int code,

print_vma_addr(KERN_CONT " in ", regs->nip);

pr_cont("\n");
+
+   show_instructions(regs);
 }

 void _exception_pkey(int signr, struct pt_regs *regs, int code,
--
2.17.1

Re: [PATCH v4 00/11] hugetlb: Factorize hugetlb architecture primitives

[LEROY Christophe]

Alex Ghiti  a écrit :


Hi everyone,

This is the result of the build for all arches tackled in this  
series rebased on 4.18-rc6:


arm:
    versatile_defconfig: without huge page OK
    keystone_defconfig: with huge page OK
arm64:
    defconfig: with huge page OK
ia64:
    generic_defconfig: with huge page OK
mips:
    Paul Burton tested cavium octeon with huge page OK
parisc:
    generic-64bit_defconfig: with huge page does not link
    generic-64bit_defconfig: without huge page does not link
    BUT not because of this series, any feedback welcome.
powerpc:
    ppc64_defconfig: without huge page OK
    ppc64_defconfig: with huge page OK


Can you also test ppc32 both with and without hugepage (mpc885_ads_defconfig)

Thanks
Christophe


sh:
    dreamcast_defconfig: with huge page OK
sparc:
    sparc32_defconfig: without huge page OK
sparc64:
    sparc64_defconfig: with huge page OK
x86:
    with huge page OK

Alex

On 07/23/2018 02:00 PM, Michael Ellerman wrote:

Alex Ghiti  writes:


Does anyone have any suggestion about those patches ?

Cross compiling it for some non-x86 arches would be a good start :)

There are cross compilers available here:

  https://mirrors.edge.kernel.org/pub/tools/crosstool/


cheers


On 07/09/2018 02:16 PM, Michal Hocko wrote:
[CC hugetlb guys -  
http://lkml.kernel.org/r/20180705110716.3919-1-a...@ghiti.fr]


On Thu 05-07-18 11:07:05, Alexandre Ghiti wrote:

In order to reduce copy/paste of functions across architectures and then
make riscv hugetlb port (and future ports) simpler and smaller, this
patchset intends to factorize the numerous hugetlb primitives that are
defined across all the architectures.

Except for prepare_hugepage_range, this patchset moves the versions that
are just pass-through to standard pte primitives into
asm-generic/hugetlb.h by using the same #ifdef semantic that can be
found in asm-generic/pgtable.h, i.e. __HAVE_ARCH_***.

s390 architecture has not been tackled in this serie since it does not
use asm-generic/hugetlb.h at all.
powerpc could be factorized a bit more (cf huge_ptep_set_wrprotect).

This patchset has been compiled on x86 only.

Changelog:

v4:
   Fix powerpc build error due to misplacing of #include
outside of #ifdef CONFIG_HUGETLB_PAGE, as
   pointed by Christophe Leroy.

v1, v2, v3:
   Same version, just problems with email provider and misuse of
   --batch-size option of git send-email

Alexandre Ghiti (11):
   hugetlb: Harmonize hugetlb.h arch specific defines with pgtable.h
   hugetlb: Introduce generic version of hugetlb_free_pgd_range
   hugetlb: Introduce generic version of set_huge_pte_at
   hugetlb: Introduce generic version of huge_ptep_get_and_clear
   hugetlb: Introduce generic version of huge_ptep_clear_flush
   hugetlb: Introduce generic version of huge_pte_none
   hugetlb: Introduce generic version of huge_pte_wrprotect
   hugetlb: Introduce generic version of prepare_hugepage_range
   hugetlb: Introduce generic version of huge_ptep_set_wrprotect
   hugetlb: Introduce generic version of huge_ptep_set_access_flags
   hugetlb: Introduce generic version of huge_ptep_get

  arch/arm/include/asm/hugetlb-3level.h| 32 +-
  arch/arm/include/asm/hugetlb.h   | 33 +--
  arch/arm64/include/asm/hugetlb.h | 39 +++-
  arch/ia64/include/asm/hugetlb.h  | 47 ++-
  arch/mips/include/asm/hugetlb.h  | 40 +++--
  arch/parisc/include/asm/hugetlb.h| 33 +++
  arch/powerpc/include/asm/book3s/32/pgtable.h |  2 +
  arch/powerpc/include/asm/book3s/64/pgtable.h |  1 +
  arch/powerpc/include/asm/hugetlb.h   | 43 ++
  arch/powerpc/include/asm/nohash/32/pgtable.h |  2 +
  arch/powerpc/include/asm/nohash/64/pgtable.h |  1 +
  arch/sh/include/asm/hugetlb.h| 54 ++---
  arch/sparc/include/asm/hugetlb.h | 40 +++--
  arch/x86/include/asm/hugetlb.h   | 72  
+--
  include/asm-generic/hugetlb.h| 88  
+++-

  15 files changed, 143 insertions(+), 384 deletions(-)

--
2.16.2