Re: [PATCH] powerpc/xics: Set the IRQ chip data for the ICS native backend

[Gustavo Romero]

Hi,

I confirm that if this fix is *not* applied to current Linus
tree (c605c39677b9) and kernel boots on Microwatt (18eb029) the
kernel will crash with the following exception:



[1.846437] BUG: Kernel NULL pointer dereference on read at 0x0048
[1.853121] Faulting instruction address: 0xc003146c
Vector: 300 (Data Access) at [c10332c0]
pc: c003146c: ics_native_unmask_irq+0x10/0x60
lr: c00314d4: ics_native_startup+0x18/0x2c
sp: c1033560
   msr: 90009033
   dar: 48
 dsisr: 4000
  current = 0xc105
  paca= 0xc068c000   irqmask: 0x03   irq_happened: 0x01
pid   = 1, comm = swapper
Linux version 5.14.0-11101-gc605c39677b9 (gromero@amd) (powerpc64-linux-gnu-gcc 
(Ubuntu 10.3.0-1ubuntu1) 10.3.0, GNU ld (GNU Binutils for Ubuntu) 2.36.1) #5 
Sat Sep 11 22:01
enter ? for help
[link register   ] c00314d4 ics_native_startup+0x18/0x2c
[c1033560] 3000 (unreliable)
[c1033580] c0074c00 irq_startup+0x8c/0xd4
[c10335c0] c0072434 __setup_irq+0x534/0x6c4
[c1033660] c00728ac request_threaded_irq+0x130/0x154
[c10336d0] c0217478 univ8250_setup_irq+0x1b0/0x20c
[c1033720] c021af2c serial8250_do_startup+0x428/0x654
[c10337b0] c021475c uart_startup+0xd0/0x1a0
[c1033800] c021487c uart_port_activate+0x50/0x74
[c1033830] c020fc98 tty_port_open+0xa4/0x110
[c1033880] c0212810 uart_open+0x24/0x4c
[c10338a0] c0208b04 tty_open+0x2d4/0x394
[c1033920] c00e38a8 chrdev_open+0xd4/0x15c
[c1033980] c00dc080 do_dentry_open+0x24c/0x2d0
[c10339d0] c00edd60 path_openat+0x8fc/0xa2c
[c1033ab0] c00edee8 do_filp_open+0x58/0xbc
[c1033be0] c00dd530 file_open_name+0x54/0x7c
[c1033c50] c00dd5a0 filp_open+0x48/0x68
[c1033c90] c048e1d4 console_on_rootfs+0x2c/0x88
[c1033d00] c048e424 kernel_init_freeable+0x1f4/0x238
[c1033db0] c000e51c kernel_init+0x28/0x138
[c1033e10] c000b114 ret_from_kernel_thread+0x5c/0x64
mon>


Thanks for fixing it Cédric.

Tested-by: Gustavo Romero 


Cheers,
Gustavo

Re: [PATCH] powerpc/xics: Set the IRQ chip data for the ICS native backend

[Gustavo Romero]

Hi,

I confirm that if this fix is *not* applied to current Linus
tree (c605c39677b9) and kernel boots on Microwatt (18eb029) the
kernel will crash with the following exception:



[1.846437] BUG: Kernel NULL pointer dereference on read at 0x0048
[1.853121] Faulting instruction address: 0xc003146c
Vector: 300 (Data Access) at [c10332c0]
pc: c003146c: ics_native_unmask_irq+0x10/0x60
lr: c00314d4: ics_native_startup+0x18/0x2c
sp: c1033560
   msr: 90009033
   dar: 48
 dsisr: 4000
  current = 0xc105
  paca= 0xc068c000   irqmask: 0x03   irq_happened: 0x01
pid   = 1, comm = swapper
Linux version 5.14.0-11101-gc605c39677b9 (gromero@amd) (powerpc64-linux-gnu-gcc 
(Ubuntu 10.3.0-1ubuntu1) 10.3.0, GNU ld (GNU Binutils for Ubuntu) 2.36.1) #5 
Sat Sep 11 22:01
enter ? for help
[link register   ] c00314d4 ics_native_startup+0x18/0x2c
[c1033560] 3000 (unreliable)
[c1033580] c0074c00 irq_startup+0x8c/0xd4
[c10335c0] c0072434 __setup_irq+0x534/0x6c4
[c1033660] c00728ac request_threaded_irq+0x130/0x154
[c10336d0] c0217478 univ8250_setup_irq+0x1b0/0x20c
[c1033720] c021af2c serial8250_do_startup+0x428/0x654
[c10337b0] c021475c uart_startup+0xd0/0x1a0
[c1033800] c021487c uart_port_activate+0x50/0x74
[c1033830] c020fc98 tty_port_open+0xa4/0x110
[c1033880] c0212810 uart_open+0x24/0x4c
[c10338a0] c0208b04 tty_open+0x2d4/0x394
[c1033920] c00e38a8 chrdev_open+0xd4/0x15c
[c1033980] c00dc080 do_dentry_open+0x24c/0x2d0
[c10339d0] c00edd60 path_openat+0x8fc/0xa2c
[c1033ab0] c00edee8 do_filp_open+0x58/0xbc
[c1033be0] c00dd530 file_open_name+0x54/0x7c
[c1033c50] c00dd5a0 filp_open+0x48/0x68
[c1033c90] c048e1d4 console_on_rootfs+0x2c/0x88
[c1033d00] c048e424 kernel_init_freeable+0x1f4/0x238
[c1033db0] c000e51c kernel_init+0x28/0x138
[c1033e10] c000b114 ret_from_kernel_thread+0x5c/0x64
mon>


Thanks for fixing it Cédric.

Tested-by: Gustavo Romero 


Cheers,
Gustavo

[PATCH] selftests/powerpc: Add test to check if TM is disabled when it must be

[Gustavo Romero]

Add a TM test to check that when TM is not advertised by the OS (is disabled) a
transaction can not really be started and generates a SIGILL, which is the right
behavior in that case.

Signed-off-by: Gustavo Romero 
---
 tools/testing/selftests/powerpc/tm/.gitignore |  1 +
 tools/testing/selftests/powerpc/tm/Makefile   |  2 +-
 tools/testing/selftests/powerpc/tm/tm-no-tm.c | 48 +++
 3 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/powerpc/tm/tm-no-tm.c

diff --git a/tools/testing/selftests/powerpc/tm/.gitignore 
b/tools/testing/selftests/powerpc/tm/.gitignore
index d8900a0..1d23309 100644
--- a/tools/testing/selftests/powerpc/tm/.gitignore
+++ b/tools/testing/selftests/powerpc/tm/.gitignore
@@ -20,3 +20,4 @@ tm-unavailable
 tm-trap
 tm-sigreturn
 tm-poison
+tm-no-tm
diff --git a/tools/testing/selftests/powerpc/tm/Makefile 
b/tools/testing/selftests/powerpc/tm/Makefile
index 5881e97..756a03f 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -5,7 +5,7 @@ SIGNAL_CONTEXT_CHK_TESTS := tm-signal-context-chk-gpr 
tm-signal-context-chk-fpu
 TEST_GEN_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv 
tm-signal-stack \
tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail tm-unavailable 
tm-trap \
$(SIGNAL_CONTEXT_CHK_TESTS) tm-sigreturn tm-signal-sigreturn-nt \
-   tm-signal-context-force-tm tm-poison tm-signal-pagefault
+   tm-signal-context-force-tm tm-poison tm-signal-pagefault tm-no-tm
 
 TEST_FILES := settings
 
diff --git a/tools/testing/selftests/powerpc/tm/tm-no-tm.c 
b/tools/testing/selftests/powerpc/tm/tm-no-tm.c
new file mode 100644
index 000..3b83e20
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-no-tm.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2020, Gustavo Romero, IBM Corp.
+ *
+ * This test checks if when TM is not supported by the OS indeed it's not
+ * possible to start a TM transaction. Moreover, when trying to start a new
+ * transaction the user gets an illegal instruction, which is the correct
+ * behavior in that case, instead of any other signal, like SIGSEGV etc.
+ *
+ * Since firmware can change the TM instruction behavior in many ways, it's 
good
+ * to have a test to check if TM is properly disabled when the OS advertises
+ * that TM is not available in userspace.
+ *
+ */
+
+#include 
+#include 
+#include 
+
+#include "utils.h"
+#include "tm.h"
+
+void illegal_signal_handler(int signo_notused, siginfo_t *si_notused, void 
*uc_notused)
+{
+   exit(EXIT_SUCCESS);
+}
+
+int tm_no_tm_test(void)
+{
+   struct sigaction illegal_sa;
+
+   SKIP_IF(have_htm());
+
+   illegal_sa.sa_flags = SA_SIGINFO;
+   illegal_sa.sa_sigaction = illegal_signal_handler;
+
+   sigaction(SIGILL, _sa, NULL);
+
+   /* It must cause a SIGILL since TM is not supported by the OS */
+   asm("tbegin.;");
+
+   return EXIT_FAILURE;
+}
+
+int main(int argc, char **argv)
+{
+   return test_harness(tm_no_tm_test, "tm_no_tm_test");
+}
-- 
2.7.4

[PATCH v2] powerpc/tm: Save and restore AMR on treclaim and trechkpt

[Gustavo Romero]

Althought AMR is stashed in the checkpoint area, currently we don't save
it to the per thread checkpoint struct after a treclaim and so we don't
restore it either from that struct when we trechkpt. As a consequence when
the transaction is later rolled back the kernel space AMR value when the
trechkpt was done appears in userspace.

That commit saves and restores AMR accordingly on treclaim and trechkpt.
Since AMR value is also used in kernel space in other functions, it also
takes care of stashing kernel live AMR into the stack before treclaim and
before trechkpt, restoring it later, just before returning from tm_reclaim
and __tm_recheckpoint.

Is also fixes two nonrelated comments about CR and MSR.

Signed-off-by: Gustavo Romero 
---
 arch/powerpc/include/asm/processor.h |  1 +
 arch/powerpc/kernel/asm-offsets.c|  1 +
 arch/powerpc/kernel/tm.S | 35 
 3 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/include/asm/processor.h 
b/arch/powerpc/include/asm/processor.h
index ed0d633ab5aa..9f4f6cc033ac 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -220,6 +220,7 @@ struct thread_struct {
unsigned long   tm_tar;
unsigned long   tm_ppr;
unsigned long   tm_dscr;
+   unsigned long   tm_amr;
 
/*
 * Checkpointed FP and VSX 0-31 register set.
diff --git a/arch/powerpc/kernel/asm-offsets.c 
b/arch/powerpc/kernel/asm-offsets.c
index 8711c2164b45..c2722ff36e98 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -176,6 +176,7 @@ int main(void)
OFFSET(THREAD_TM_TAR, thread_struct, tm_tar);
OFFSET(THREAD_TM_PPR, thread_struct, tm_ppr);
OFFSET(THREAD_TM_DSCR, thread_struct, tm_dscr);
+   OFFSET(THREAD_TM_AMR, thread_struct, tm_amr);
OFFSET(PT_CKPT_REGS, thread_struct, ckpt_regs);
OFFSET(THREAD_CKVRSTATE, thread_struct, ckvr_state.vr);
OFFSET(THREAD_CKVRSAVE, thread_struct, ckvrsave);
diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
index 6ba0fdd1e7f8..2b91f233b05d 100644
--- a/arch/powerpc/kernel/tm.S
+++ b/arch/powerpc/kernel/tm.S
@@ -122,6 +122,13 @@ _GLOBAL(tm_reclaim)
std r3, STK_PARAM(R3)(r1)
SAVE_NVGPRS(r1)
 
+   /*
+* Save kernel live AMR since it will be clobbered by treclaim
+* but can be used elsewhere later in kernel space.
+*/
+   mfspr   r3, SPRN_AMR
+   std r3, TM_FRAME_L1(r1)
+
/* We need to setup MSR for VSX register save instructions. */
mfmsr   r14
mr  r15, r14
@@ -245,7 +252,7 @@ _GLOBAL(tm_reclaim)
 * but is used in signal return to 'wind back' to the abort handler.
 */
 
-   /*  CR,LR,CCR,MSR ** */
+   /* * CTR, LR, CR, XER ** */
mfctr   r3
mflrr4
mfcrr5
@@ -256,7 +263,6 @@ _GLOBAL(tm_reclaim)
std r5, _CCR(r7)
std r6, _XER(r7)
 
-
/*  TAR, DSCR ** */
mfspr   r3, SPRN_TAR
mfspr   r4, SPRN_DSCR
@@ -264,6 +270,10 @@ _GLOBAL(tm_reclaim)
std r3, THREAD_TM_TAR(r12)
std r4, THREAD_TM_DSCR(r12)
 
+/*  AMR  */
+mfspr  r3, SPRN_AMR
+stdr3, THREAD_TM_AMR(r12)
+
/*
 * MSR and flags: We don't change CRs, and we don't need to alter MSR.
 */
@@ -308,7 +318,9 @@ _GLOBAL(tm_reclaim)
std r3, THREAD_TM_TFHAR(r12)
std r4, THREAD_TM_TFIAR(r12)
 
-   /* AMR is checkpointed too, but is unsupported by Linux. */
+   /* Restore kernel live AMR */
+   ld  r8, TM_FRAME_L1(r1)
+   mtspr   SPRN_AMR, r8
 
/* Restore original MSR/IRQ state & clear TM mode */
ld  r14, TM_FRAME_L0(r1)/* Orig MSR */
@@ -355,6 +367,13 @@ _GLOBAL(__tm_recheckpoint)
 */
SAVE_NVGPRS(r1)
 
+   /*
+* Save kernel live AMR since it will be clobbered for trechkpt
+* but can be used elsewhere later in kernel space.
+*/
+   mfspr   r8, SPRN_AMR
+   std r8, TM_FRAME_L0(r1)
+
/* Load complete register state from ts_ckpt* registers */
 
addir7, r3, PT_CKPT_REGS/* Thread's ckpt_regs */
@@ -404,7 +423,7 @@ _GLOBAL(__tm_recheckpoint)
 
 restore_gprs:
 
-   /*  CR,LR,CCR,MSR ** */
+   /* ** CTR, LR, XER * */
ld  r4, _CTR(r7)
ld  r5, _LINK(r7)
ld  r8, _XER(r7)
@@ -417,6 +436,10 @@ restore_gprs:
ld  r4, THREAD_TM_TAR(r3)
mtspr   SPRN_TAR,   r4
 
+   /*  AMR  */
+   ld  r4, THREAD_TM_AMR(r3)
+   mtspr   SPRN_AMR, r4
+
/* Load up the PPR and DSCR in GPRs only at this stage */
  

[PATCH] powerpc/tm: Save and restore AMR on treclaim and trechkpt

[Gustavo Romero]

Althought AMR is stashed on the checkpoint area, currently we don't save
it to the per thread checkpoint struct after a treclaim and so we don't
restore it either from that struct when we trechkpt. As a consequence when
the transaction is later rolled back kernel space AMR value when the
trechkpt was done appears in userspace.

That commit saves and restores AMR accordingly on treclaim and trechkpt.
Since AMR value is also used in kernel space in other functions, it also
takes care of stashing kernel live AMR into PACA before treclaim and before
trechkpt, restoring it later, just before returning from tm_reclaim and
__tm_recheckpoint.

Is also fixes two nonrelated comments about CR and MSR.

Signed-off-by: Gustavo Romero 
---
 arch/powerpc/include/asm/paca.h  |  1 +
 arch/powerpc/include/asm/processor.h |  1 +
 arch/powerpc/kernel/asm-offsets.c|  2 ++
 arch/powerpc/kernel/tm.S | 31 +++-
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index 9454d29ff4b4..44c605181529 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -179,6 +179,7 @@ struct paca_struct {
u64 sprg_vdso;  /* Saved user-visible sprg */
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
u64 tm_scratch; /* TM scratch area for reclaim */
+   u64 tm_amr; /* Saved Kernel AMR for 
treclaim/trechkpt */
 #endif
 
 #ifdef CONFIG_PPC_POWERNV
diff --git a/arch/powerpc/include/asm/processor.h 
b/arch/powerpc/include/asm/processor.h
index ed0d633ab5aa..9f4f6cc033ac 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -220,6 +220,7 @@ struct thread_struct {
unsigned long   tm_tar;
unsigned long   tm_ppr;
unsigned long   tm_dscr;
+   unsigned long   tm_amr;
 
/*
 * Checkpointed FP and VSX 0-31 register set.
diff --git a/arch/powerpc/kernel/asm-offsets.c 
b/arch/powerpc/kernel/asm-offsets.c
index 8711c2164b45..cf1a6d68a91f 100644
--- a/arch/powerpc/kernel/asm-offsets.c
+++ b/arch/powerpc/kernel/asm-offsets.c
@@ -170,12 +170,14 @@ int main(void)
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
OFFSET(PACATMSCRATCH, paca_struct, tm_scratch);
+   OFFSET(PACATMAMR, paca_struct, tm_amr);
OFFSET(THREAD_TM_TFHAR, thread_struct, tm_tfhar);
OFFSET(THREAD_TM_TEXASR, thread_struct, tm_texasr);
OFFSET(THREAD_TM_TFIAR, thread_struct, tm_tfiar);
OFFSET(THREAD_TM_TAR, thread_struct, tm_tar);
OFFSET(THREAD_TM_PPR, thread_struct, tm_ppr);
OFFSET(THREAD_TM_DSCR, thread_struct, tm_dscr);
+   OFFSET(THREAD_TM_AMR, thread_struct, tm_amr);
OFFSET(PT_CKPT_REGS, thread_struct, ckpt_regs);
OFFSET(THREAD_CKVRSTATE, thread_struct, ckvr_state.vr);
OFFSET(THREAD_CKVRSAVE, thread_struct, ckvrsave);
diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
index 6ba0fdd1e7f8..e178ddb43619 100644
--- a/arch/powerpc/kernel/tm.S
+++ b/arch/powerpc/kernel/tm.S
@@ -152,6 +152,10 @@ _GLOBAL(tm_reclaim)
li  r5, 0
mtmsrd  r5, 1
 
+/* Save AMR since it's used elsewhere in kernel space */
+   mfspr   r8, SPRN_AMR
+   std r8, PACATMAMR(r13)
+
/*
 * BE CAREFUL HERE:
 * At this point we can't take an SLB miss since we have MSR_RI
@@ -245,7 +249,7 @@ _GLOBAL(tm_reclaim)
 * but is used in signal return to 'wind back' to the abort handler.
 */
 
-   /*  CR,LR,CCR,MSR ** */
+   /* * CTR, LR, CR, XER ** */
mfctr   r3
mflrr4
mfcrr5
@@ -256,7 +260,6 @@ _GLOBAL(tm_reclaim)
std r5, _CCR(r7)
std r6, _XER(r7)
 
-
/*  TAR, DSCR ** */
mfspr   r3, SPRN_TAR
mfspr   r4, SPRN_DSCR
@@ -264,6 +267,10 @@ _GLOBAL(tm_reclaim)
std r3, THREAD_TM_TAR(r12)
std r4, THREAD_TM_DSCR(r12)
 
+/*  AMR  */
+mfspr  r3, SPRN_AMR
+stdr3, THREAD_TM_AMR(r12)
+
/*
 * MSR and flags: We don't change CRs, and we don't need to alter MSR.
 */
@@ -308,8 +315,6 @@ _GLOBAL(tm_reclaim)
std r3, THREAD_TM_TFHAR(r12)
std r4, THREAD_TM_TFIAR(r12)
 
-   /* AMR is checkpointed too, but is unsupported by Linux. */
-
/* Restore original MSR/IRQ state & clear TM mode */
ld  r14, TM_FRAME_L0(r1)/* Orig MSR */
 
@@ -330,6 +335,10 @@ _GLOBAL(tm_reclaim)
ld  r0, PACA_DSCR_DEFAULT(r13)
mtspr   SPRN_DSCR, r0
 
+/* Restore kernel saved AMR */
+   ld  r4, PACATMAMR(r13)
+   mtspr   SPRN_AMR, r4
+
blr
 
 
@@ -355,6 +364,10 @@ _GLOBAL(__tm_recheckpoint)
 */
SAVE_NVGPRS(r1)
 
+   /* Save kernel AMR s

Re: [PATCH] powerpc/fadump: Fix build error with CONFIG_PRESERVE_FA_DUMP=y

[Gustavo Romero]

On 7/27/20 4:03 AM, Michael Ellerman wrote:

skiroot_defconfig fails:

arch/powerpc/kernel/fadump.c:48:17: error: ‘cpus_in_fadump’ defined but not used
48 | static atomic_t cpus_in_fadump;

Fix it by moving the definition into the #ifdef where it's used.

Fixes: ba608c4fa12c ("powerpc/fadump: fix race between pstore write and fadump crash 
trigger")
Signed-off-by: Michael Ellerman 
---
  arch/powerpc/kernel/fadump.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c
index 1858896d6809..10ebb4bf71ad 100644
--- a/arch/powerpc/kernel/fadump.c
+++ b/arch/powerpc/kernel/fadump.c
@@ -45,10 +45,12 @@ static struct fw_dump fw_dump;
  static void __init fadump_reserve_crash_area(u64 base);

  struct kobject *fadump_kobj;
-static atomic_t cpus_in_fadump;

  #ifndef CONFIG_PRESERVE_FA_DUMP
+
+static atomic_t cpus_in_fadump;
  static DEFINE_MUTEX(fadump_mutex);
+
  struct fadump_mrange_info crash_mrange_info = { "crash", NULL, 0, 0, 0, false 
};

  #define RESERVED_RNGS_SZ  16384 /* 16K - 128 entries */



Tested-by: Gustavo Romero 


Thanks,
Gustavo

Re: [PATCH] powerpc/powernv/pci: Fix build of pci-ioda.o

[Gustavo Romero]

Hi Oliver,

On 7/28/20 7:50 PM, Oliver O'Halloran wrote:

On Wed, Jul 29, 2020 at 8:35 AM Gustavo Romero  wrote:


Currently pnv_ioda_setup_bus_dma() is outside of a CONFIG_IOMMU_API guard
and if CONFIG_IOMMU_API=n the build can fail if the compiler sets
-Werror=unused-function, because pnv_ioda_setup_bus_dma() is only used in
functions guarded by a CONFIG_IOMMU_API guard.

That issue can be easily reproduced using the skiroot_defconfig. For other
configs, like powernv_defconfig, that issue is hidden by the fact that
if CONFIG_IOMMU_SUPPORT is enabled plus other common IOMMU options, like
CONFIG_OF_IOMMU, by default CONFIG_IOMMU_API is enabled as well. Hence, for
powernv_defconfig, it's necessary to set CONFIG_IOMMU_SUPPORT=n to make the
build fail, because CONFIG_PCI=y and pci-ioda.c is included in the build,
but since CONFIG_IOMMU_SUPPORT=n the CONFIG_IOMMU_API is disabled, breaking
the build.

This commit fixes that build issue by moving the pnv_ioda_setup_bus_dma()
inside a CONFIG_IOMMU_API guard, so when CONFIG_IOMMU_API is disabled that
function is not defined.


I think a fix for this is already in -next.


Indeed.

For the records, it's fixed in -next by:

commit e3417faec526cbf97773dca691dcd743f5bfeb64
Author: Oliver O'Halloran 
Date:   Sun Jul 5 23:35:57 2020 +1000

powerpc/powernv: Move pnv_ioda_setup_bus_dma under CONFIG_IOMMU_API

pnv_ioda_setup_bus_dma() is only used when a passed through PE is

returned to the host. If the kernel is built without IOMMU support
this is dead code. Move it under the #ifdef with the rest of the
IOMMU API support.

Reported-by: kernel test robot 

Signed-off-by: Oliver O'Halloran 
Reviewed-by: Alexey Kardashevskiy 
Signed-off-by: Michael Ellerman 
Link: https://lore.kernel.org/r/20200705133557.443607-2-ooh...@gmail.com


Thanks.


Cheers,
Gustavo

[PATCH] powerpc/powernv/pci: Fix build of pci-ioda.o

[Gustavo Romero]

Currently pnv_ioda_setup_bus_dma() is outside of a CONFIG_IOMMU_API guard
and if CONFIG_IOMMU_API=n the build can fail if the compiler sets
-Werror=unused-function, because pnv_ioda_setup_bus_dma() is only used in
functions guarded by a CONFIG_IOMMU_API guard.

That issue can be easily reproduced using the skiroot_defconfig. For other
configs, like powernv_defconfig, that issue is hidden by the fact that
if CONFIG_IOMMU_SUPPORT is enabled plus other common IOMMU options, like
CONFIG_OF_IOMMU, by default CONFIG_IOMMU_API is enabled as well. Hence, for
powernv_defconfig, it's necessary to set CONFIG_IOMMU_SUPPORT=n to make the
build fail, because CONFIG_PCI=y and pci-ioda.c is included in the build,
but since CONFIG_IOMMU_SUPPORT=n the CONFIG_IOMMU_API is disabled, breaking
the build.

This commit fixes that build issue by moving the pnv_ioda_setup_bus_dma()
inside a CONFIG_IOMMU_API guard, so when CONFIG_IOMMU_API is disabled that
function is not defined.

Signed-off-by: Gustavo Romero 
---
 arch/powerpc/platforms/powernv/pci-ioda.c | 26 +++
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/powerpc/platforms/powernv/pci-ioda.c 
b/arch/powerpc/platforms/powernv/pci-ioda.c
index 73a63efcf855..743d840712da 100644
--- a/arch/powerpc/platforms/powernv/pci-ioda.c
+++ b/arch/powerpc/platforms/powernv/pci-ioda.c
@@ -1885,19 +1885,6 @@ static bool pnv_pci_ioda_iommu_bypass_supported(struct 
pci_dev *pdev,
return false;
 }
 
-static void pnv_ioda_setup_bus_dma(struct pnv_ioda_pe *pe, struct pci_bus *bus)
-{
-   struct pci_dev *dev;
-
-   list_for_each_entry(dev, >devices, bus_list) {
-   set_iommu_table_base(>dev, pe->table_group.tables[0]);
-   dev->dev.archdata.dma_offset = pe->tce_bypass_base;
-
-   if ((pe->flags & PNV_IODA_PE_BUS_ALL) && dev->subordinate)
-   pnv_ioda_setup_bus_dma(pe, dev->subordinate);
-   }
-}
-
 static inline __be64 __iomem *pnv_ioda_get_inval_reg(struct pnv_phb *phb,
 bool real_mode)
 {
@@ -2501,6 +2488,19 @@ static long pnv_pci_ioda2_unset_window(struct 
iommu_table_group *table_group,
 #endif
 
 #ifdef CONFIG_IOMMU_API
+static void pnv_ioda_setup_bus_dma(struct pnv_ioda_pe *pe, struct pci_bus *bus)
+{
+   struct pci_dev *dev;
+
+   list_for_each_entry(dev, >devices, bus_list) {
+   set_iommu_table_base(>dev, pe->table_group.tables[0]);
+   dev->dev.archdata.dma_offset = pe->tce_bypass_base;
+
+   if ((pe->flags & PNV_IODA_PE_BUS_ALL) && dev->subordinate)
+   pnv_ioda_setup_bus_dma(pe, dev->subordinate);
+   }
+}
+
 unsigned long pnv_pci_ioda2_get_table_size(__u32 page_shift,
__u64 window_size, __u32 levels)
 {
-- 
2.17.1

Re: powerpc/pci: [PATCH 1/1 V2] PCIE PHB reset

[Gustavo Romero]

Hi Xiong,

On 5/15/20 5:04 PM, wenxi...@linux.vnet.ibm.com wrote:

From: Wen Xiong 

Several device drivers hit EEH(Extended Error handling) when triggering
kdump on Pseries PowerVM. This patch implemented a reset of the PHBs
in pci general code. PHB reset stop all PCI transactions from previous
kernel. We have tested the patch in several enviroments:


What do you mean exactly by "previous kernel" in here? Is there a way to
enhance that comment a bit further?


Thanks,
Gustavo

Re: [PATCH] selftests/powerpc: Always build the tm-poison test 64-bit

[Gustavo Romero]

On 04/06/2020 10:06 AM, Michael Ellerman wrote:

On Fri, 2020-04-03 at 09:56:56 UTC, Michael Ellerman wrote:

The tm-poison test includes inline asm which is 64-bit only, so the
test must be built 64-bit in order to work.

Otherwise it fails, eg:
   # file tm-poison
   tm-poison: ELF 32-bit MSB executable, PowerPC or cisco 4500, version 1 
(SYSV) ...
   # ./tm-poison
   test: tm_poison_test
   Unknown value 0x1fff71150 leaked into f31!
   Unknown value 0x1fff710c0 leaked into vr31!
   failure: tm_poison_test

Fixes: a003365cab64 ("powerpc/tm: Add tm-poison test")
Signed-off-by: Michael Ellerman 


Applied to powerpc next.

https://git.kernel.org/powerpc/c/6ba4a2d3591039aea1cb45c7c42262d26351a2fa

cheers


Ack.

Thank you, Michael.

Cheers,
Gustavo

Re: [PATCH] KVM: PPC: Book3S HV: Fix typos in comments

[Gustavo Romero]

Hi Gabriel,

On 03/06/2020 01:06 PM, Gabriel Paubert wrote:

On Fri, Mar 06, 2020 at 11:26:36AM +1100, Gustavo Romero wrote:

Fix typos found in comments about the parameter passed
through r5 to kvmppc_{save,restore}_tm_hv functions.


Actually "iff" is a common shorthand in some fields and not necessarily
a spelling error:

https://en.wikipedia.org/wiki/If_and_only_if


I see. Thank you.


Best regards,
Gustavo

[PATCH] KVM: PPC: Book3S HV: Fix typos in comments

[Gustavo Romero]

Fix typos found in comments about the parameter passed
through r5 to kvmppc_{save,restore}_tm_hv functions.

Signed-off-by: Gustavo Romero 
---
 arch/powerpc/kvm/book3s_hv_rmhandlers.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv_rmhandlers.S 
b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
index dbc2fec..a55dbe8 100644
--- a/arch/powerpc/kvm/book3s_hv_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_hv_rmhandlers.S
@@ -3121,7 +3121,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_ALTIVEC)
  * Save transactional state and TM-related registers.
  * Called with r3 pointing to the vcpu struct and r4 containing
  * the guest MSR value.
- * r5 is non-zero iff non-volatile register state needs to be maintained.
+ * r5 is non-zero if non-volatile register state needs to be maintained.
  * If r5 == 0, this can modify all checkpointed registers, but
  * restores r1 and r2 before exit.
  */
@@ -3194,7 +3194,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_P9_TM_XER_SO_BUG)
  * Restore transactional state and TM-related registers.
  * Called with r3 pointing to the vcpu struct
  * and r4 containing the guest MSR value.
- * r5 is non-zero iff non-volatile register state needs to be maintained.
+ * r5 is non-zero if non-volatile register state needs to be maintained.
  * This potentially modifies all checkpointed registers.
  * It restores r1 and r2 from the PACA.
  */
-- 
1.8.3.1

[PATCH v3] KVM: PPC: Book3S HV: Treat TM-related invalid form instructions on P9 like the valid ones

[Gustavo Romero]

owing a trace and treating it as a 'nop'.

Signed-off-by: Gustavo Romero 
Reviewed-by: Segher Boessenkool 
Acked-By: Michael Neuling 
Reviewed-by: Leonardo Bras 
---
 arch/powerpc/include/asm/kvm_asm.h  |  3 +++
 arch/powerpc/kvm/book3s_hv_tm.c | 28 -
 arch/powerpc/kvm/book3s_hv_tm_builtin.c | 16 --
 3 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_asm.h 
b/arch/powerpc/include/asm/kvm_asm.h
index 635fb154b33f..a3633560493b 100644
--- a/arch/powerpc/include/asm/kvm_asm.h
+++ b/arch/powerpc/include/asm/kvm_asm.h
@@ -150,4 +150,7 @@
 
 #define KVM_INST_FETCH_FAILED  -1
 
+/* Extract PO and XOP opcode fields */
+#define PO_XOP_OPCODE_MASK 0xfc0007fe
+
 #endif /* __POWERPC_KVM_ASM_H__ */
diff --git a/arch/powerpc/kvm/book3s_hv_tm.c b/arch/powerpc/kvm/book3s_hv_tm.c
index 0db937497169..cc90b8b82329 100644
--- a/arch/powerpc/kvm/book3s_hv_tm.c
+++ b/arch/powerpc/kvm/book3s_hv_tm.c
@@ -3,6 +3,8 @@
  * Copyright 2017 Paul Mackerras, IBM Corp. 
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include 
 
 #include 
@@ -44,7 +46,18 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
u64 newmsr, bescr;
int ra, rs;
 
-   switch (instr & 0xfc0007ff) {
+   /*
+* rfid, rfebb, and mtmsrd encode bit 31 = 0 since it's a reserved bit
+* in these instructions, so masking bit 31 out doesn't change these
+* instructions. For treclaim., tsr., and trechkpt. instructions if bit
+* 31 = 0 then they are per ISA invalid forms, however P9 UM, in section
+* 4.6.10 Book II Invalid Forms, informs specifically that ignoring bit
+* 31 is an acceptable way to handle these invalid forms that have
+* bit 31 = 0. Moreover, for emulation purposes both forms (w/ and wo/
+* bit 31 set) can generate a softpatch interrupt. Hence both forms
+* are handled below for these instructions so they behave the same way.
+*/
+   switch (instr & PO_XOP_OPCODE_MASK) {
case PPC_INST_RFID:
/* XXX do we need to check for PR=0 here? */
newmsr = vcpu->arch.shregs.srr1;
@@ -105,7 +118,8 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
vcpu->arch.shregs.msr = newmsr;
return RESUME_GUEST;
 
-   case PPC_INST_TSR:
+   /* ignore bit 31, see comment above */
+   case (PPC_INST_TSR & PO_XOP_OPCODE_MASK):
/* check for PR=1 and arch 2.06 bit set in PCR */
if ((msr & MSR_PR) && (vcpu->arch.vcore->pcr & PCR_ARCH_206)) {
/* generate an illegal instruction interrupt */
@@ -140,7 +154,8 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
vcpu->arch.shregs.msr = msr;
return RESUME_GUEST;
 
-   case PPC_INST_TRECLAIM:
+   /* ignore bit 31, see comment above */
+   case (PPC_INST_TRECLAIM & PO_XOP_OPCODE_MASK):
/* check for TM disabled in the HFSCR or MSR */
if (!(vcpu->arch.hfscr & HFSCR_TM)) {
/* generate an illegal instruction interrupt */
@@ -176,7 +191,8 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
vcpu->arch.shregs.msr &= ~MSR_TS_MASK;
return RESUME_GUEST;
 
-   case PPC_INST_TRECHKPT:
+   /* ignore bit 31, see comment above */
+   case (PPC_INST_TRECHKPT & PO_XOP_OPCODE_MASK):
/* XXX do we need to check for PR=0 here? */
/* check for TM disabled in the HFSCR or MSR */
if (!(vcpu->arch.hfscr & HFSCR_TM)) {
@@ -208,6 +224,8 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
}
 
/* What should we do here? We didn't recognize the instruction */
-   WARN_ON_ONCE(1);
+   kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
+   pr_warn_ratelimited("Unrecognized TM-related instruction %#x for 
emulation", instr);
+
return RESUME_GUEST;
 }
diff --git a/arch/powerpc/kvm/book3s_hv_tm_builtin.c 
b/arch/powerpc/kvm/book3s_hv_tm_builtin.c
index 217246279dfa..fad931f224ef 100644
--- a/arch/powerpc/kvm/book3s_hv_tm_builtin.c
+++ b/arch/powerpc/kvm/book3s_hv_tm_builtin.c
@@ -23,7 +23,18 @@ int kvmhv_p9_tm_emulation_early(struct kvm_vcpu *vcpu)
u64 newmsr, msr, bescr;
int rs;
 
-   switch (instr & 0xfc0007ff) {
+   /*
+* rfid, rfebb, and mtmsrd encode bit 31 = 0 since it's a reserved bit
+* in these instructions, so masking bit 31 out doesn't change these
+* instructions. For the tsr. instruction if bit 31 = 0 then it is per
+* ISA an invalid form, however P9 UM, in section 4.6.10 Book II Invalid
+* Forms, informs specifically that ignoring bit 31 is an acceptable way
+* to handle TM-related invalid forms that have bit 31 = 0. Moreover,
+* for emulati

Re: [PATCH] KVM: PPC: Book3S HV: Treat TM-related invalid form instructions on P9 like the valid ones

[Gustavo Romero]

Hi Leonardo,

Thanks a lot for the review.

On 02/20/2020 02:51 PM, Leonardo Bras wrote:

+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+


Could not see where is this used.


This is used by pr_warn_ratelimited() below so the module name is printed before
the message, for instance:

[531454.670909] kvm_hv: Unrecognized TM-related instruction 0x7c00075c for 
emulation



  #include 

  #include 
@@ -44,7 +46,18 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
u64 newmsr, bescr;
int ra, rs;

-   switch (instr & 0xfc0007ff) {
+   /*
+* rfid, rfebb, and mtmsrd encode bit 31 = 0 since it's a reserved bit
+* in these instructions, so masking bit 31 out doesn't change these
+* instructions. For treclaim., tsr., and trechkpt. instructions if bit
+* 31 = 0 then they are per ISA invalid forms, however P9 UM, in section
+* 4.6.10 Book II Invalid Forms, informs specifically that ignoring bit
+* 31 is an acceptable way to handle these invalid forms that have
+* bit 31 = 0. Moreover, for emulation purposes both forms (w/ and wo/
+* bit 31 set) can generate a softpatch interrupt. Hence both forms
+* are handled below for these instructions so they behave the same way.
+*/
+   switch (instr & PO_XOP_OPCODE_MASK) {




-   case PPC_INST_TRECHKPT:
+   /* ignore bit 31, see comment above */
+   case (PPC_INST_TRECHKPT & PO_XOP_OPCODE_MASK):
/* XXX do we need to check for PR=0 here? */
/* check for TM disabled in the HFSCR or MSR */
if (!(vcpu->arch.hfscr & HFSCR_TM)) {
@@ -208,6 +224,8 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
}



Seems good, using the same flag to mask out bit 31 of these macros.
They are used only in a few places, and I think removing the macro bit
would be ok, but I think your way is better to keep it documented.

I just noticed that there is a similar function that uses PPC_INST_TSR:
kvmhv_p9_tm_emulation_early @ arch/powerpc/kvm/book3s_hv_tm_builtin.c.
Wouldn't it need to be changed as well?


oh! you're right, I forgot that one. I'll send a v3.



/* What should we do here? We didn't recognize the instruction */
-   WARN_ON_ONCE(1);
+   kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
+   pr_warn_ratelimited("Unrecognized TM-related instruction %#x for 
emulation", instr);
+
return RESUME_GUEST;
  }


I suppose this is the right thing to do, but I think it would be better
to give this change it's own patch.

What do you think?


I think it's sufficiently self-contained and trivial to be in the same file and
to be in a single commit.


Best regards,
Gustavo

Re: [PATCH] KVM: PPC: Book3S HV: Treat unrecognized TM instructions as illegal

[Gustavo Romero]

Hi,

On 02/17/2020 04:37 AM, Segher Boessenkool wrote:

On Mon, Feb 17, 2020 at 05:23:07PM +1100, Michael Neuling wrote:

Hence, we should NOP this, not generate an illegal.


It is not a reserved bit.

The IMC entry for it matches op1=01 op2=101110 presumably, which
catches all TM instructions and nothing else (bits 0..5 and bits 21..30).
That does not look at bit 31, the softpatch handler has to deal with this.

Some TM insns have bit 31 as 1 and some have it as /.  All instructions
with a "." in the mnemonic have bit 31 is 1, all other have it reserved.
The tables in appendices D, E, F show tend. and tsr. as having it
reserved, which contradicts the individual instruction description (and
does not make much sense).  (Only tcheck has /, everything else has 1;
everything else has a mnemonic with a dot, and does write CR0 always).


Wow, interesting.

P8 seems to be treating 31 as a reserved bit (with the table definition rather
than the individual instruction description). I'm inclined to match P8 even
though it's inconsistent with the dot mnemonic as you say.


"The POWER8 core ignores the state of reserved bits in the instructions
(denoted by “///” in the instruction definition) and executes the
instruction normally. Software should set these bits to ‘0’ per the
Power ISA." (p8 UM, 3.1.1.3; same in the p9 UM).


For the records, I've sent a v2 addressing Mikey's comments:

https://lists.ozlabs.org/pipermail/linuxppc-dev/2020-February/204502.html

or

https://marc.info/?l=kvm-ppc=158206045520038=2

Thanks for the review.


Best regards,
Gustavo

[PATCH] KVM: PPC: Book3S HV: Treat TM-related invalid form instructions on P9 like the valid ones

[Gustavo Romero]

owing a trace and treating it as a 'nop'.

Signed-off-by: Gustavo Romero 
---
 arch/powerpc/include/asm/kvm_asm.h |  3 +++
 arch/powerpc/kvm/book3s_hv_tm.c| 28 +++-
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_asm.h 
b/arch/powerpc/include/asm/kvm_asm.h
index 635fb154b33f..a3633560493b 100644
--- a/arch/powerpc/include/asm/kvm_asm.h
+++ b/arch/powerpc/include/asm/kvm_asm.h
@@ -150,4 +150,7 @@
 
 #define KVM_INST_FETCH_FAILED  -1
 
+/* Extract PO and XOP opcode fields */
+#define PO_XOP_OPCODE_MASK 0xfc0007fe
+
 #endif /* __POWERPC_KVM_ASM_H__ */
diff --git a/arch/powerpc/kvm/book3s_hv_tm.c b/arch/powerpc/kvm/book3s_hv_tm.c
index 0db937497169..cc90b8b82329 100644
--- a/arch/powerpc/kvm/book3s_hv_tm.c
+++ b/arch/powerpc/kvm/book3s_hv_tm.c
@@ -3,6 +3,8 @@
  * Copyright 2017 Paul Mackerras, IBM Corp. 
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include 
 
 #include 
@@ -44,7 +46,18 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
u64 newmsr, bescr;
int ra, rs;
 
-   switch (instr & 0xfc0007ff) {
+   /*
+* rfid, rfebb, and mtmsrd encode bit 31 = 0 since it's a reserved bit
+* in these instructions, so masking bit 31 out doesn't change these
+* instructions. For treclaim., tsr., and trechkpt. instructions if bit
+* 31 = 0 then they are per ISA invalid forms, however P9 UM, in section
+* 4.6.10 Book II Invalid Forms, informs specifically that ignoring bit
+* 31 is an acceptable way to handle these invalid forms that have
+* bit 31 = 0. Moreover, for emulation purposes both forms (w/ and wo/
+* bit 31 set) can generate a softpatch interrupt. Hence both forms
+* are handled below for these instructions so they behave the same way.
+*/
+   switch (instr & PO_XOP_OPCODE_MASK) {
case PPC_INST_RFID:
/* XXX do we need to check for PR=0 here? */
newmsr = vcpu->arch.shregs.srr1;
@@ -105,7 +118,8 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
vcpu->arch.shregs.msr = newmsr;
return RESUME_GUEST;
 
-   case PPC_INST_TSR:
+   /* ignore bit 31, see comment above */
+   case (PPC_INST_TSR & PO_XOP_OPCODE_MASK):
/* check for PR=1 and arch 2.06 bit set in PCR */
if ((msr & MSR_PR) && (vcpu->arch.vcore->pcr & PCR_ARCH_206)) {
/* generate an illegal instruction interrupt */
@@ -140,7 +154,8 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
vcpu->arch.shregs.msr = msr;
return RESUME_GUEST;
 
-   case PPC_INST_TRECLAIM:
+   /* ignore bit 31, see comment above */
+   case (PPC_INST_TRECLAIM & PO_XOP_OPCODE_MASK):
/* check for TM disabled in the HFSCR or MSR */
if (!(vcpu->arch.hfscr & HFSCR_TM)) {
/* generate an illegal instruction interrupt */
@@ -176,7 +191,8 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
vcpu->arch.shregs.msr &= ~MSR_TS_MASK;
return RESUME_GUEST;
 
-   case PPC_INST_TRECHKPT:
+   /* ignore bit 31, see comment above */
+   case (PPC_INST_TRECHKPT & PO_XOP_OPCODE_MASK):
/* XXX do we need to check for PR=0 here? */
/* check for TM disabled in the HFSCR or MSR */
if (!(vcpu->arch.hfscr & HFSCR_TM)) {
@@ -208,6 +224,8 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
}
 
/* What should we do here? We didn't recognize the instruction */
-   WARN_ON_ONCE(1);
+   kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
+   pr_warn_ratelimited("Unrecognized TM-related instruction %#x for 
emulation", instr);
+
return RESUME_GUEST;
 }
-- 
2.17.1

Re: [PATCH] KVM: PPC: Book3S HV: Treat unrecognized TM instructions as illegal

[Gustavo Romero]

Hi Segher,

Thanks a lot for reviewing it.

On 02/13/2020 08:31 PM, Segher Boessenkool wrote:




---
  arch/powerpc/kvm/book3s_hv_tm.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kvm/book3s_hv_tm.c b/arch/powerpc/kvm/book3s_hv_tm.c
index 0db937497169..d342a9e11298 100644
--- a/arch/powerpc/kvm/book3s_hv_tm.c
+++ b/arch/powerpc/kvm/book3s_hv_tm.c
@@ -3,6 +3,8 @@
   * Copyright 2017 Paul Mackerras, IBM Corp. 
   */
  
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

+
  #include 
  
  #include 

@@ -208,6 +210,8 @@ int kvmhv_p9_tm_emulation(struct kvm_vcpu *vcpu)
}
  
  	/* What should we do here? We didn't recognize the instruction */

-   WARN_ON_ONCE(1);
+   kvmppc_core_queue_program(vcpu, SRR1_PROGILL);
+   pr_warn_ratelimited("Unrecognized TM-related instruction %#x for 
emulation", instr);
+
return RESUME_GUEST;
  }


Do we actually know it is TM-related here?  Otherwise, looks good to me :-)


Correct, I understand it's only TM-related here, so I don't expect anything 
else to hit 0x1500.


Best regards,
Gustavo

[PATCH] KVM: PPC: Book3S HV: Treat unrecognized TM instructions as illegal

[Gustavo Romero]

On P9 DD2.2 due to a CPU defect some TM instructions need to be emulated by
KVM. This is handled at first by the hardware raising a softpatch interrupt
when certain TM instructions that need KVM assistance are executed in the
guest. Some TM instructions, although not defined in the Power ISA, might
raise a softpatch interrupt. For instance, 'tresume.' instruction as
defined in the ISA must have bit 31 set (1), but an instruction that
matches 'tresume.' OP and XO opcodes but has bit 31 not set (0), like
0x7cfe9ddc, also raises a softpatch interrupt, for example, if a code
like the following is executed in the guest it will raise a softpatch
interrupt just like a 'tresume.' when the TM facility is enabled:

int main() { asm("tabort. 0; .long 0x7cfe9ddc;"); }

Currently in such a case KVM throws a complete trace like the following:

[345523.705984] WARNING: CPU: 24 PID: 64413 at 
arch/powerpc/kvm/book3s_hv_tm.c:211 kvmhv_p9_tm_emulation+0x68/0x620 [kvm_hv]
[345523.705985] Modules linked in: kvm_hv(E) xt_conntrack ipt_REJECT 
nf_reject_ipv4 xt_tcpudp ip6table_mangle ip6table_nat
iptable_mangle iptable_nat nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 
ebtable_filter ebtables ip6table_filter
ip6_tables iptable_filter bridge stp llc sch_fq_codel ipmi_powernv at24 
vmx_crypto ipmi_devintf ipmi_msghandler
ibmpowernv uio_pdrv_genirq kvm opal_prd uio leds_powernv ib_iser rdma_cm iw_cm 
ib_cm ib_core iscsi_tcp libiscsi_tcp
libiscsi scsi_transport_iscsi ip_tables x_tables autofs4 btrfs blake2b_generic 
zstd_compress raid10 raid456
async_raid6_recov async_memcpy async_pq async_xor async_tx libcrc32c xor 
raid6_pq raid1 raid0 multipath linear tg3
crct10dif_vpmsum crc32c_vpmsum ipr [last unloaded: kvm_hv]
[345523.706030] CPU: 24 PID: 64413 Comm: CPU 0/KVM Tainted: GW   E 
5.5.0+ #1
[345523.706031] NIP:  c008072cb9c0 LR: c008072b5e80 CTR: 
c008085c7850
[345523.706034] REGS: c00399467680 TRAP: 0700   Tainted: GW   E 
 (5.5.0+)
[345523.706034] MSR:  90010282b033 
  CR: 24022428  XER: 
[345523.706042] CFAR: c008072b5e7c IRQMASK: 0
GPR00: c008072b5e80 c00399467910 c008072db500 
c00375ccc720
GPR04: c00375ccc720 0003fbec a10395dda5a6 

GPR08: 7cfe9ddc 7cfe9ddc05dc 7cfe9ddc7c0005dc 
c008072cd530
GPR12: c008085c7850 c003fffeb800 0001 
7dfb737f
GPR16: c0002001edcca558   
0001
GPR20: c1b21258 c0002001edcca558 0018 

GPR24: 0100  0001 
1500
GPR28: c0002001edcc4278 c0037dd8 80050280f033 
c00375ccc720
[345523.706062] NIP [c008072cb9c0] kvmhv_p9_tm_emulation+0x68/0x620 [kvm_hv]
[345523.706065] LR [c008072b5e80] kvmppc_handle_exit_hv.isra.53+0x3e8/0x798 
[kvm_hv]
[345523.706066] Call Trace:
[345523.706069] [c00399467910] [c00399467940] 0xc00399467940 
(unreliable)
[345523.706071] [c00399467950] [c00399467980] 0xc00399467980
[345523.706075] [c003994679f0] [c008072bd1c4] 
kvmhv_run_single_vcpu+0xa1c/0xb80 [kvm_hv]
[345523.706079] [c00399467ac0] [c008072bd8e0] 
kvmppc_vcpu_run_hv+0x5b8/0xb00 [kvm_hv]
[345523.706087] [c00399467b90] [c008085c93cc] kvmppc_vcpu_run+0x34/0x48 
[kvm]
[345523.706095] [c00399467bb0] [c008085c582c] 
kvm_arch_vcpu_ioctl_run+0x244/0x420 [kvm]
[345523.706101] [c00399467c40] [c008085b7498] 
kvm_vcpu_ioctl+0x3d0/0x7b0 [kvm]
[345523.706105] [c00399467db0] [c04adf9c] ksys_ioctl+0x13c/0x170
[345523.706107] [c00399467e00] [c04adff8] sys_ioctl+0x28/0x80
[345523.706111] [c00399467e20] [c000b278] system_call+0x5c/0x68
[345523.706112] Instruction dump:
[345523.706114] 419e0390 7f8a4840 409d0048 6d497c00 2f89075d 419e021c 6d497c00 
2f8907dd
[345523.706119] 419e01c0 6d497c00 2f8905dd 419e00a4 <0fe0> 38210040 
3860 ebc1fff0

and then treats the executed instruction as 'nop' whilst it should actually
be treated as an illegal instruction since it's not defined by the ISA.

This commit changes the handling of the case above by treating the
unrecognized TM instructions that can raise a softpatch but are not
defined in the ISA as illegal ones instead of as 'nop' and by gently
reporting it to the host instead of throwing a trace.

Signed-off-by: Gustavo Romero 
---
 arch/powerpc/kvm/book3s_hv_tm.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kvm/book3s_hv_tm.c b/arch/powerpc/kvm/book3s_hv_tm.c
index 0db937497169..d342a9e11298 100644
--- a/arch/powerpc/kvm/book3s_hv_tm.c
+++ b/arch/powerpc/kvm/book3s_hv_tm.c
@@ -3,6 +3,8 @@
  * Copyright 2017 Paul Mackerras, IBM Corp. 
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include 
 
 

Re: [PATCH 1/3] powerpc/tm: Clear the current thread's MSR[TS] after treclaim

[Gustavo Romero]

nal_32.c: In function 'handle_rt_signal32':
/linux/arch/powerpc/kernel/signal_32.c:908:16: error: unused variable 'msr' 
[-Werror=unused-variable]
  908 |  unsigned long msr = regs->msr;
  |^~~
/linux/arch/powerpc/kernel/signal_32.c: In function 'handle_signal32':
/linux/arch/powerpc/kernel/signal_32.c:1367:16: error: unused variable 'msr' 
[-Werror=unused-variable]
 1367 |  unsigned long msr = regs->msr;
  |


Feel free to send a v2 only after Mikey's review.

Otherwise, LGTM.

Reviewed-by: Gustavo Romero 

Best regards,
Gustavo

Re: [PATCH 3/3] selftests/powerpc: Don't rely on segfault to rerun the test

[Gustavo Romero]

On 01/16/2020 07:05 PM, Gustavo Luiz Duarte wrote:

The test case tm-signal-context-force-tm expects a segfault to happen on
returning from signal handler, and then does a setcontext() to run the test
again. However, the test doesn't always segfault, causing the test to run a
single time.

This patch fixes the test by putting it within a loop and jumping, via
setcontext, just prior to the loop in case it segfaults. This way we get the
desired behavior (run the test COUNT_MAX times) regardless if it segfaults or
not. This also reduces the use of setcontext for control flow logic, keeping it
only in the segfault handler.

Also, since 'count' is changed within the signal handler, it is declared as
volatile to prevent any compiler optimization getting confused with
asynchronous changes.

Signed-off-by: Gustavo Luiz Duarte 
---
  .../powerpc/tm/tm-signal-context-force-tm.c   | 79 +--
  1 file changed, 37 insertions(+), 42 deletions(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c 
b/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c
index 31717625f318..9ff7bdb6d47a 100644
--- a/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c
+++ b/tools/testing/selftests/powerpc/tm/tm-signal-context-force-tm.c
@@ -42,9 +42,10 @@
  #endif
  
  /* Setting contexts because the test will crash and we want to recover */

-ucontext_t init_context, main_context;
+ucontext_t init_context;
  
-static int count, first_time;

+/* count is changed in the signal handler, so it must be volatile */
+static volatile int count;
  
  void usr_signal_handler(int signo, siginfo_t *si, void *uc)

  {
@@ -98,11 +99,6 @@ void usr_signal_handler(int signo, siginfo_t *si, void *uc)
  
  void seg_signal_handler(int signo, siginfo_t *si, void *uc)

  {
-   if (count == COUNT_MAX) {
-   /* Return to tm_signal_force_msr() and exit */
-   setcontext(_context);
-   }
-
count++;
  
  	/* Reexecute the test */

@@ -126,37 +122,40 @@ void tm_trap_test(void)
 */
getcontext(_context);
  
-	/* Allocated an alternative signal stack area */

-   ss.ss_sp = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
-   MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
-   ss.ss_size = SIGSTKSZ;
-   ss.ss_flags = 0;
-
-   if (ss.ss_sp == (void *)-1) {
-   perror("mmap error\n");
-   exit(-1);
-   }
-
-   /* Force the allocation through a page fault */
-   if (madvise(ss.ss_sp, SIGSTKSZ, MADV_DONTNEED)) {
-   perror("madvise\n");
-   exit(-1);
-   }
-
-   /* Setting an alternative stack to generate a page fault when
-* the signal is raised.
-*/
-   if (sigaltstack(, NULL)) {
-   perror("sigaltstack\n");
-   exit(-1);
+   while (count < COUNT_MAX) {
+   /* Allocated an alternative signal stack area */
+   ss.ss_sp = mmap(NULL, SIGSTKSZ, PROT_READ | PROT_WRITE,
+   MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
+   ss.ss_size = SIGSTKSZ;
+   ss.ss_flags = 0;
+
+   if (ss.ss_sp == (void *)-1) {
+   perror("mmap error\n");
+   exit(-1);
+   }
+
+   /* Force the allocation through a page fault */
+   if (madvise(ss.ss_sp, SIGSTKSZ, MADV_DONTNEED)) {
+   perror("madvise\n");
+   exit(-1);
+   }
+
+   /* Setting an alternative stack to generate a page fault when
+* the signal is raised.
+*/
+   if (sigaltstack(, NULL)) {
+   perror("sigaltstack\n");
+   exit(-1);
+   }
+
+   /* The signal handler will enable MSR_TS */
+   sigaction(SIGUSR1, _sa, NULL);
+   /* If it does not crash, it might segfault, avoid it to retest 
*/
+   sigaction(SIGSEGV, _sa, NULL);
+
+   raise(SIGUSR1);
+   count++;
}
-
-   /* The signal handler will enable MSR_TS */
-   sigaction(SIGUSR1, _sa, NULL);
-   /* If it does not crash, it will segfault, avoid it to retest */
-   sigaction(SIGSEGV, _sa, NULL);
-
-   raise(SIGUSR1);
  }
  
  int tm_signal_context_force_tm(void)

@@ -169,11 +168,7 @@ int tm_signal_context_force_tm(void)
 */
SKIP_IF(!is_ppc64le());
  
-	/* Will get back here after COUNT_MAX interactions */

-   getcontext(_context);
-
-   if (!first_time++)
-   tm_trap_test();
+   tm_trap_test();
  
  	return EXIT_SUCCESS;

  }



Reviewed-by: Gustavo Romero 


Best regards,
Gustavo

Re: [PATCH 2/3] selftests/powerpc: Add tm-signal-pagefault test

[Gustavo Romero]

suspended state, i.e. uc->uc_link != NULL.
+ */
+void signal_handler(int signo, siginfo_t *si, void *uc)
+{
+   ucontext_t *ucp = uc;
+
+   /* Skip 'trap' after returning, otherwise we get a SIGTRAP again */
+   ucp->uc_link->uc_mcontext.regs->nip += 4;
+
+   ucp->uc_mcontext.v_regs =
+   get_uf_mem(sizeof(elf_vrreg_t), ucp->uc_mcontext.v_regs);
+
+   ucp->uc_link->uc_mcontext.v_regs =
+   get_uf_mem(sizeof(elf_vrreg_t), 
ucp->uc_link->uc_mcontext.v_regs);
+
+   ucp->uc_link = get_uf_mem(sizeof(ucontext_t), ucp->uc_link);
+}
+
+int tm_signal_pagefault(void)
+{
+   struct sigaction sa;
+   stack_t ss;
+
+   SKIP_IF(!have_htm());
+
+   setup_uf_mem();
+
+   /*
+* Set an alternative stack that will generate a page fault when the
+* signal is raised. The page fault will be treated via userfaultfd,
+* i.e. via fault_handler_thread.
+*/
+   ss.ss_sp = get_uf_mem(SIGSTKSZ, NULL);
+   ss.ss_size = SIGSTKSZ;
+   ss.ss_flags = 0;
+   if (sigaltstack(, NULL) == -1) {
+   perror("sigaltstack() failed");
+   exit(EXIT_FAILURE);
+   }
+
+   sa.sa_flags = SA_SIGINFO | SA_ONSTACK;
+   sa.sa_sigaction = signal_handler;
+   if (sigaction(SIGTRAP, , NULL) == -1) {
+   perror("sigaction() failed");
+   exit(EXIT_FAILURE);
+   }
+
+   /* Trigger a SIGTRAP in transactional state */
+   asm __volatile__(
+   "tbegin.;"
+   "beq1f;"
+   "trap;"
+   "1: ;"
+   : : : "memory");
+
+   /* Trigger a SIGTRAP in suspended state */
+   asm __volatile__(
+   "tbegin.;"
+   "beq1f;"
+   "tsuspend.;"
+   "trap;"
+   "tresume.;"
+   "1: ;"
+   : : : "memory");
+
+   return EXIT_SUCCESS;
+}
+
+int main(int argc, char **argv)
+{
+   /*
+* Depending on kernel config, the TM Bad Thing might not result in a
+* crash, instead the kernel never returns control back to userspace, so
+* set a tight timeout. If the test passes it completes almost
+* immediately.
+*/
+   test_harness_set_timeout(2);
+   return test_harness(tm_signal_pagefault, "tm_signal_pagefault");
+}



Reviewed-by: Gustavo Romero 


Best regards,
Gustavo

Re: [PATCH 3/3] powerpc/tm: Add tm-poison test

[Gustavo Romero]

Hi Michael,

On 09/03/2019 08:46 AM, Michael Ellerman wrote:

Michael Neuling  writes:

From: Gustavo Romero 

Add TM selftest to check if FP or VEC register values from one process
can leak into another process when both run on the same CPU.

This tests for CVE-2019-15030 and CVE-2019-15031.

Signed-off-by: Gustavo Romero 
Signed-off-by: Michael Neuling 
---
  tools/testing/selftests/powerpc/tm/.gitignore |   1 +
  tools/testing/selftests/powerpc/tm/Makefile   |   2 +-
  .../testing/selftests/powerpc/tm/tm-poison.c  | 180 ++
  3 files changed, 182 insertions(+), 1 deletion(-)
  create mode 100644 tools/testing/selftests/powerpc/tm/tm-poison.c


This doesn't build on 64-bit big endian:

tm-poison.c: In function 'tm_poison_test':
tm-poison.c:118:10: error: format '%lx' expects argument of type 'long unsigned 
int', but argument 2 has type 'uint64_t {aka long long unsigned int}' 
[-Werror=format=]
printf("Unknown value %#lx leaked into f31!\n", unknown);
   ^
tm-poison.c:166:10: error: format '%lx' expects argument of type 'long unsigned 
int', but argument 2 has type 'uint64_t {aka long long unsigned int}' 
[-Werror=format=]
printf("Unknown value %#lx leaked into vr31!\n", unknown);
   ^


Sorry. I sent a v2 addressing it. Now I tested the fix against Travis CI.

Thank you.

Cheers,
Gustavo

[PATCH] selftests/powerpc: Retry on host facility unavailable

[Gustavo Romero]

TM test tm-unavailable must take into account aborts due to host aborting
a transactin because of a facility unavailable exception, just like it
already does for aborts on reschedules (TM_CAUSE_KVM_RESCHED).

Reported-by: Desnes A. Nunes do Rosario 
Tested-by: Desnes A. Nunes do Rosario 
Signed-off-by: Gustavo Romero 
---
 tools/testing/selftests/powerpc/tm/tm.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm.h 
b/tools/testing/selftests/powerpc/tm/tm.h
index 97f9f49..c402464 100644
--- a/tools/testing/selftests/powerpc/tm/tm.h
+++ b/tools/testing/selftests/powerpc/tm/tm.h
@@ -55,7 +55,8 @@ static inline bool failure_is_unavailable(void)
 static inline bool failure_is_reschedule(void)
 {
if ((failure_code() & TM_CAUSE_RESCHED) == TM_CAUSE_RESCHED ||
-   (failure_code() & TM_CAUSE_KVM_RESCHED) == TM_CAUSE_KVM_RESCHED)
+   (failure_code() & TM_CAUSE_KVM_RESCHED) == TM_CAUSE_KVM_RESCHED ||
+   (failure_code() & TM_CAUSE_KVM_FAC_UNAV) == TM_CAUSE_KVM_FAC_UNAV)
return true;
 
return false;
-- 
2.7.4

[PATCH 2/2] selftests/powerpc: Ignore generated files

[Gustavo Romero]

Currently some binary files which are generated when tests are compiled
are not ignored by git, so 'git status' catch them.

For copyloops test, fix wrong binary names already in .gitignore. For
ptrace, security, and stringloops tests add missing binary names to the
.gitignore file.

Signed-off-by: Gustavo Romero 
---
 tools/testing/selftests/powerpc/copyloops/.gitignore   | 8 
 tools/testing/selftests/powerpc/ptrace/.gitignore  | 3 +++
 tools/testing/selftests/powerpc/security/.gitignore| 1 +
 tools/testing/selftests/powerpc/stringloops/.gitignore | 5 -
 4 files changed, 12 insertions(+), 5 deletions(-)
 create mode 100644 tools/testing/selftests/powerpc/security/.gitignore

diff --git a/tools/testing/selftests/powerpc/copyloops/.gitignore 
b/tools/testing/selftests/powerpc/copyloops/.gitignore
index ce12cd0e2967..de158104912a 100644
--- a/tools/testing/selftests/powerpc/copyloops/.gitignore
+++ b/tools/testing/selftests/powerpc/copyloops/.gitignore
@@ -1,13 +1,13 @@
 copyuser_64_t0
 copyuser_64_t1
 copyuser_64_t2
-copyuser_power7_t0
-copyuser_power7_t1
+copyuser_p7_t0
+copyuser_p7_t1
 memcpy_64_t0
 memcpy_64_t1
 memcpy_64_t2
-memcpy_power7_t0
-memcpy_power7_t1
+memcpy_p7_t0
+memcpy_p7_t1
 copyuser_64_exc_t0
 copyuser_64_exc_t1
 copyuser_64_exc_t2
diff --git a/tools/testing/selftests/powerpc/ptrace/.gitignore 
b/tools/testing/selftests/powerpc/ptrace/.gitignore
index 07ec449a2767..dce19f221c46 100644
--- a/tools/testing/selftests/powerpc/ptrace/.gitignore
+++ b/tools/testing/selftests/powerpc/ptrace/.gitignore
@@ -10,3 +10,6 @@ ptrace-tm-spd-vsx
 ptrace-tm-spr
 ptrace-hwbreak
 perf-hwbreak
+core-pkey
+ptrace-pkey
+ptrace-syscall
diff --git a/tools/testing/selftests/powerpc/security/.gitignore 
b/tools/testing/selftests/powerpc/security/.gitignore
new file mode 100644
index ..0b969fba3beb
--- /dev/null
+++ b/tools/testing/selftests/powerpc/security/.gitignore
@@ -0,0 +1 @@
+rfi_flush
diff --git a/tools/testing/selftests/powerpc/stringloops/.gitignore 
b/tools/testing/selftests/powerpc/stringloops/.gitignore
index 0b43da74ee46..31a17e0ba884 100644
--- a/tools/testing/selftests/powerpc/stringloops/.gitignore
+++ b/tools/testing/selftests/powerpc/stringloops/.gitignore
@@ -1 +1,4 @@
-memcmp
+memcmp_64
+memcmp_32
+strlen
+strlen_32
-- 
2.17.1

[PATCH 1/2] powerpc: Document xmon options

[Gustavo Romero]

Document all options currently supported by xmon debugger.

Signed-off-by: Gustavo Romero 
---
 .../admin-guide/kernel-parameters.txt | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 7ccd158b3894..6d495aab4d0b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5305,3 +5305,22 @@
A hex value specifying bitmask with supplemental xhci
host controller quirks. Meaning of each bit can be
consulted in header drivers/usb/host/xhci.h.
+
+   xmon[PPC]
+   Format: { early | on | rw | ro | off }
+   Controls if xmon debugger is enabled. Default is off.
+   Passing only "xmon" is equivalent to "xmon=early".
+   early   Call xmon as early as possible on boot; xmon
+   debugger is called from setup_arch().
+   on  xmon debugger hooks will be installed so xmon
+   is only called on a kernel crash. Default mode,
+   i.e. either "ro" or "rw" mode, is controlled
+   with CONFIG_XMON_DEFAULT_RO_MODE.
+   rw  xmon debugger hooks will be installed so xmon
+   is called only on a kernel crash, mode is write,
+   meaning SPR registers, memory and, other data
+   can be written using xmon commands.
+   ro  same as "rw" option above but SPR registers,
+   memory, and other data can't be written using
+   xmon commands.
+   off xmon is disabled.
-- 
2.17.1

[PATCH] powerpc/selftests: Fix and enhance TM signal context tests

[Gustavo Romero]

Currently TM signal context tests for GPR, FPR, VMX, and VSX registers
print wrong register numbers (wrongly starting from register 0 instead of
the first register in the non-volatile subset). Besides it the output when
a mismatch happens is poor giving not much information about which context
and which register mismatches, because it prints both contexts at the same
time and not a comparison between the value that mismatches and the value
expected and, moreover, it stops printing on the first mismatch, but it's
important to know if there are other mismatches happening beyond the first
one.

For instance, this is the current output when a mismatch happens:

test: tm_signal_context_chk_gpr
tags: git_version:v5.2-8249-g02e970fae465-dirty
Failed on 0 GPR 1 or 18446744073709551615
failure: tm_signal_context_chk_gpr

test: tm_signal_context_chk_fpu
tags: git_version:v5.2-8248-g09c289e3ef80
Failed on 0 FP -1 or -1
failure: tm_signal_context_chk_fpu

test: tm_signal_context_chk_vmx
tags: git_version:v5.2-8248-g09c289e3ef80
Failed on 0 vmx 0xfffefffdfffc vs 
0xfffefffdfffc
failure: tm_signal_context_chk_vmx

test: tm_signal_context_chk_vsx
tags: git_version:v5.2-8248-g09c289e3ef80
Failed on 0 vsx 0xfefffdfffcff vs 
0xfefffdfffcff
failure: tm_signal_context_chk_vsx

This commit fixes the register numbers printed and enhances the error
output by providing a full list of mismatching registers separated by the
context (non-speculative or speculative context), for example:

test: tm_signal_context_chk_gpr
tags: git_version:v5.2-8249-g02e970fae465-dirty
GPR14 (1st context) == 1 instead of -1 (expected)
GPR15 (1st context) == 2 instead of -2 (expected)
GPR14 (2nd context) == 0 instead of 18446744073709551615 (expected)
GPR15 (2nd context) == 0 instead of 18446744073709551614 (expected)
failure: tm_signal_context_chk_gpr

test: tm_signal_context_chk_fpu
tags: git_version:v5.2-8249-g02e970fae465-dirty
FPR14 (1st context) == -1 instead of 1 (expected)
FPR15 (1st context) == -2 instead of 2 (expected)
failure: tm_signal_context_chk_fpu

test: tm_signal_context_chk_vmx
tags: git_version:v5.2-8249-g02e970fae465-dirty
VMX20 (1st context) == 0xfffefffdfffc instead of 
0x0001000200030004 (expected)
VMX21 (1st context) == 0xfffbfffafff9fff8 instead of 
0x0005000600070008 (expected)
failure: tm_signal_context_chk_vmx

test: tm_signal_context_chk_vsx
tags: git_version:v5.2-8249-g02e970fae465-dirty
VSX20 (1st context) == 0xfefffdfffcff instead of 
0x0001000200030004 (expected)
VSX21 (1st context) == 0xfbfffafff9fff8ff instead of 
0x0005000600070008 (expected)
failure: tm_signal_context_chk_vsx

Finally, this commit adds comments to the tests in the hope that it will
help people not so familiar with TM understand the tests.

Signed-off-by: Gustavo Romero 
---
 .../powerpc/tm/tm-signal-context-chk-fpu.c|  49 +--
 .../powerpc/tm/tm-signal-context-chk-gpr.c|  59 +---
 .../powerpc/tm/tm-signal-context-chk-vmx.c|  74 +++---
 .../powerpc/tm/tm-signal-context-chk-vsx.c| 130 +-
 4 files changed, 228 insertions(+), 84 deletions(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm-signal-context-chk-fpu.c 
b/tools/testing/selftests/powerpc/tm/tm-signal-context-chk-fpu.c
index d57c2d2ab6ec..254f912ad611 100644
--- a/tools/testing/selftests/powerpc/tm/tm-signal-context-chk-fpu.c
+++ b/tools/testing/selftests/powerpc/tm/tm-signal-context-chk-fpu.c
@@ -5,10 +5,11 @@
  * Test the kernel's signal frame code.
  *
  * The kernel sets up two sets of ucontexts if the signal was to be
- * delivered while the thread was in a transaction.
+ * delivered while the thread was in a transaction (referred too as
+ * first and second contexts).
  * Expected behaviour is that the checkpointed state is in the user
- * context passed to the signal handler. The speculated state can be
- * accessed with the uc_link pointer.
+ * context passed to the signal handler (first context). The speculated
+ * state can be accessed with the uc_link pointer (second context).
  *
  * The rationale for this is that if TM unaware code (which linked
  * against TM libs) installs a signal handler it will not know of the
@@ -28,17 +29,20 @@
 
 #define MAX_ATTEMPT 50
 
-#define NV_FPU_REGS 18
+#define NV_FPU_REGS 18 /* Number of non-volatile FP registers */
+#define FPR14 14 /* First non-volatile FP register to check in f14-31 subset */
 
 long tm_signal_self_context_load(pid_t pid, long *gprs, double *fps, vector 
int *vms, vector int *vss);
 
-/* Be sure there are 2x as many as there are NV FPU regs (2x18) */
+/* Test only non-volatile registers, i.e. 18 fpr registers from f14 to f31 */
 static double fps[] = {
+   /* First context will be set with these values, i.e. non-speculative */
 1, 2, 3, 4, 5, 6, 7, 8, 9

[PATCH] selftests/powerpc: Fix earlyclobber in tm-vmxcopy

[Gustavo Romero]

In some cases, compiler can allocate the same register for operand 'res'
and 'vecoutptr', resulting in segfault at 'stxvd2x 40,0,%[vecoutptr]'
because base register will contain 1, yielding a false-positive.

This is because output 'res' must be marked as an earlyclobber operand so
it may not overlap an input operand ('vecoutptr').

Signed-off-by: Gustavo Romero 
---
 tools/testing/selftests/powerpc/tm/tm-vmxcopy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c 
b/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c
index 147c6dc..c1e788a 100644
--- a/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c
+++ b/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c
@@ -79,7 +79,7 @@ int test_vmxcopy()
 
"5:;"
"stxvd2x 40,0,%[vecoutptr];"
-   : [res]"=r"(aborted)
+   : [res]"="(aborted)
: [vecinptr]"r"(),
  [vecoutptr]"r"(),
  [map]"r"(a)
-- 
2.7.4

Re: It looks like that wild_bctr on powerpc/fixes is still not compiling

[Gustavo Romero]

Hi Michael,

On 11/13/2018 10:58 PM, Michael Ellerman wrote:

It looks like binutils 2.27 doesn't accept ULL but binutils 2.28 does.

Ah yep, here:

   
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=86b80085c889cd388fa677a5ae9053fd4be3776c


The following trivial workaround can solve it by forcing a type promotion on
the compiler side whilst leaving the macro taken into the asm code without
the UL string:

diff --git a/tools/testing/selftests/powerpc/mm/wild_bctr.c 
b/tools/testing/selftests/powerpc/mm/wild_bctr.c
index 90469a9..d2772f4 100644
--- a/tools/testing/selftests/powerpc/mm/wild_bctr.c
+++ b/tools/testing/selftests/powerpc/mm/wild_bctr.c
@@ -47,8 +47,9 @@ static int ok(void)
  return 0;
   }
   
-#define REG_POISON 0x5a5aUL

-#define POISONED_REG(n)((REG_POISON << 48) | ((n) << 32) | (REG_POISON 
<< 16) | (n))
+#define REG_POISON 0x5a5a
+#define POISONED_REG(n)(((REG_POISON+0UL) << 48) | ((n) << 32) | 
((REG_POISON+0UL) << 16) | (n))
   
   static inline void poison_regs(void)

   {


Should I contribute such a fix?


Yes thanks.


Segher kindly suggested to use explicitly "unsigned long" (thanks!), so I sent
a v2 to:

https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-November/181434.html


Best regards,
Gustavo

[PATCH v2] selftests/powerpc: Adjust wild_bctr to build with old gcc

[Gustavo Romero]

Currently the selftest wild_bctr can fail to build when an old gcc is used,
notably on gcc using a binutils version <= 2.27, because the assembler does
not support the integer suffix UL.

This patch adjusts the wild_bctr test so the REG_POISON value is still
treated as an unsigned long for the shifts on compilation but the UL
suffix is absent on the stringification, so the inline asm code generated
has no UL suffixes.

Signed-off-by: Gustavo Romero 
---
 tools/testing/selftests/powerpc/mm/wild_bctr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/powerpc/mm/wild_bctr.c 
b/tools/testing/selftests/powerpc/mm/wild_bctr.c
index 90469a9..aadac172 100644
--- a/tools/testing/selftests/powerpc/mm/wild_bctr.c
+++ b/tools/testing/selftests/powerpc/mm/wild_bctr.c
@@ -47,8 +47,8 @@ static int ok(void)
return 0;
 }
 
-#define REG_POISON 0x5a5aUL
-#define POISONED_REG(n)((REG_POISON << 48) | ((n) << 32) | (REG_POISON 
<< 16) | (n))
+#define REG_POISON 0x5a5a
+#define POISONED_REG(n)unsigned long) REG_POISON) << 48) | ((n) << 
32) | (((unsigned long) REG_POISON) << 16) | (n))
 
 static inline void poison_regs(void)
 {
-- 
2.7.4

[PATCH] selftests/powerpc: Adjust wild_bctr to build with old gcc

[Gustavo Romero]

Currently the selftest wild_bctr can fail to build when an old gcc is used,
notably on gcc using a binutils version <= 2.27, because the assembler does
not support the integer suffix UL.

That patch adjusts the wild_bctr test so the type promotion to UL for the
shifts on compilation still happens but the UL suffix is absent on the
stringification, so the inline asm code generated has no UL suffixes.

Signed-off-by: Gustavo Romero 
---
 tools/testing/selftests/powerpc/mm/wild_bctr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/powerpc/mm/wild_bctr.c 
b/tools/testing/selftests/powerpc/mm/wild_bctr.c
index 90469a9..7e56aa4 100644
--- a/tools/testing/selftests/powerpc/mm/wild_bctr.c
+++ b/tools/testing/selftests/powerpc/mm/wild_bctr.c
@@ -47,8 +47,8 @@ static int ok(void)
return 0;
 }
 
-#define REG_POISON 0x5a5aUL
-#define POISONED_REG(n)((REG_POISON << 48) | ((n) << 32) | (REG_POISON 
<< 16) | (n))
+#define REG_POISON 0x5a5a
+#define POISONED_REG(n)(((REG_POISON+0UL) << 48) | ((n) << 32) | 
((REG_POISON+0UL) << 16) | (n))
 
 static inline void poison_regs(void)
 {
-- 
2.7.4

It looks like that wild_bctr on powerpc/fixes is still not compiling

[Gustavo Romero]

Hi mpe,

Even after the latest fix for the wild_bctr selftest I'm still getting the
following compilation (actually, an assembling error) because UL is not
understood by the assembler:

BUILD_TARGET=/home/gromero/git/linux/tools/testing/selftests/powerpc/mm; mkdir 
-p $BUILD_TARGET; make OUTPUT=$BUILD_TARGET -k -C mm all
make[1]: Entering directory 
'/home/gromero/git/linux/tools/testing/selftests/powerpc/mm'
gcc -std=gnu99 -O2 -Wall -Werror -DGIT_VERSION='"v4.20-rc1-8-g2c7645b"' 
-I/home/gromero/git/linux/tools/testing/selftests/powerpc/include  -m64wild_bctr.c 
../harness.c  -o /home/gromero/git/linux/tools/testing/selftests/powerpc/mm/wild_bctr
/tmp/cctUajlx.s: Assembler messages:
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
/tmp/cctUajlx.s:270: Error: syntax error; found `U', expected `,'
/tmp/cctUajlx.s:270: Error: junk at end of line: `UL'
../../lib.mk:152: recipe for target 
'/home/gromero/git/linux/tools/testing/selftests/powerpc/mm/wild_bctr' failed
make[1]: *** 
[/home/gromero/git/linux/tools/testing/selftests/powerpc/mm/wild_bctr] Error 1
make[1]: Target 'all' not remade because of errors.
make[1]: Leaving directory 
'/home/gromero/git/linux/tools/testing/selftests/powerpc/mm'
Makefile:39: recipe for target 'mm' failed
make: *** [mm] Error 2

For:
git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git (powerpc/fixes)
$ git describe
v4.20-rc1-8-g2c7645b

This is gcc:
$ gcc --version
gcc (Ubuntu/IBM 5.4.0-6ubuntu1~16.04.10) 5.4.0 20160609

but it should not make a difference, so  I'm wondering if anybody else is

Re: Looking for architecture papers

[Gustavo Romero]

Hi Raz,

On 10/04/2018 04:41 AM, Raz wrote:

Frankly, the more I read the more perplexed I get. For example,
according to BOOK III-S, chapter 3,
the MSR bits are differ from the ones described in
arch/powerpc/include/asm/reg.h.
Bit zero, is LE, but in the book it is 64-bit mode.

Would someone be kind to explain what I do not understand?


Yes, I know that can be confusing at the first sight when one is used to, for
instance, x86.

x86 documents use LSB 0 notation, which means (as others already pointed out)
that the least significant bit of a value is marked as being bit 0.

On the other hand Power documents use MSB 0 notation, which means that the most
significant bit of a value is marked as being bit 0 and as a consequence the
least significant bit in that notation in a 64-bit platform is bit 63, not bit
0. MSB 0 notation is also known as IBM bit notation/bit numbering.

Historically LSB 0 notation tend to be used on docs about little-endian
architectures (for instance, x86), whilst MSB 0 notation tend to be used on docs
about big-endian architectures (for instance, Power - Power is actually a little
different because it's now bi-endian actually).

However LSB 0 and MSB 0 are only different notations, so LSB 0 can be employed
on a big-endian architecture documentation, and vice versa.

It happens that kernel code is written in C and for shifts, etc, it's convenient
the LSB 0 notation, not the MSB 0 notation, so it's convenient to use LSB 0
notation when creating a mask, like in arch/powerpc/include/asm/reg.h), i.e.
it's convenient to employ bit positions as '63 - '.

So, as another example, in the following gcc macro '_TEXASR_EXTRACT_BITS' takes
a bit position 'BITNUM' as found in the PowerISA documentation but then for the
shift right it uses '63 - BITNUM':

https://github.com/gcc-mirror/gcc/blob/master/gcc/config/rs6000/htmintrin.h#L44-L45

I think it's also important to mention that on PowerISA the elements also follow
the MSB 0 notation. So byte, word, and dword elements in a register found in the
instruction descriptions when referred to 0 are the element "at the left tip",
i.e. "the most significant elements", so to speak. For instance, take
instruction "vperm": doc says 'index' takes bits 3:7 of a byte from [byte]
element 'i'. So for a byte element i=0 it means the most significant byte
("on the left tip") of vector register operand 'VRC'. Moreover, specified bits
in that byte element, i.e. bits 3:7,  also follow the MSB 0, so for the
little-endian addicted thought they are bits 4:0 (LSB 0 notation).

Now, if bits 4:0 = 0b00011 (decimal 3), we grab byte element 3 from 'src'
(256-bit). However byte element 3 is also in MSB 0 notation, so it means third
byte of 'src' but starting counting bytes from 0 from the left to the right
(which in IMO looks indeed more natural since we count, for instance, Natural
Numbers on the 'x' axis similarly).

Hence, it's like to say that 'vperm' instruction in a certain sense has a
"big-endian semantics" for the byte indices. The 'vpermr' instruction introduced
by PowerISA v3.0 is meant to cope with that, so 'vpermr' byte indices have a
"little-endian semantics", so for bits 3:7 MSB 0 (or bits 4:0 in LSB 0 
notation) =
0b00011 (decimal 3), on the 'vpermr' instruction it really means we must count
bytes starting from right to left as in the LSB 0 notation and grab the third 
byte
element from right to left.

So, for instance:

vr0uint128 = 0x
vr1uint128 = 0x00102030405060708090a0b0c0d0e0f0
vr2uint128 = 0x0111223344556677aabbccddeeff
vr3uint128 = 0x0300

we have 'src' as:

MSB 0: v--- byte 0, 1, 2, 3, ...
LSB 0:  
...  3, 2, 1, byte 0 ---v
src = vr1 || vr2 = 00 10 20 30 40 50 60 70 80 90 A0 B0 C0 D0 E0 F0 01 11 22 33 
44 55 66 77 99 99 AA BB CC DD EE FF

vperm   vr0, vr1, vr2, vr3 result is:
vr0uint128 = 0x3000
byte 3 in MSB 0  = 0x30 ---^ and 0x00 (byte 0 in MSB 0) copied to the remaining 
bytes

whilst with vpermr (PowerISA v3.0 / POWER9):
vpermr   vr0, vr1, vr2, vr3 result is:
vr0uint128 = 0xccff
byte 3 in LSB 0  = 0xCC^ and 0xFF (byte 0 in LSB 0) copied to the remaining 
bytes


Anyway, vperm/vpermr was just an example about notation not being restricted to
bits on Power ISA. So read the docs carefully :) GDB is always useful for 
checking
if one's understanding about a given Power instruction is correct.

HTH.


Regards,
Gustavo

Re: [PATCH] selftests/powerpc: Do not fail on TM_CAUSE_RESCHED

[Gustavo Romero]

Hi Breno,

On 08/21/2018 03:56 PM, Breno Leitao wrote:

There are cases where the test is not expecting to have the transaction
aborted, but, the test process might have been rescheduled, either in the OS
level or by KVM (if it is running on a KVM guest machine). The process
reschedule will cause a treclaim/recheckpoint which will cause the transaction
to doom, failing as soon as the process is rescheduled back to the CPU. This
might cause the test to fail, but this is not a failure in essence.

If that is the case, TEXASR[FC] is indicated with either
TM_CAUSE_RESCHEDULE or TM_CAUSE_KVM_RESCHEDULE for KVM interruptions.

In this scenario, ignore these two failures and avoid the whole test to return
failure.

Signed-off-by: Breno Leitao 


Thanks for improving the code.

I understand that filtering out the aborts caused by the re-schedules is 
correct.

Only a nit:


@@ -244,9 +245,12 @@ void *tm_una_ping(void *input)

/*
 * Check if TM failed due to the cause we were expecting. 0xda is a
-* TM_CAUSE_FAC_UNAV cause, otherwise it's an unexpected cause.
+* TM_CAUSE_FAC_UNAV cause, otherwise it's an unexpected cause, unless
+* it was caused by a reschedule.
 */
-   if (is_failure(cr_) && !failure_is_unavailable()) {
+
+   if (is_failure(cr_) && !failure_is_unavailable()
+   & !failure_is_reschedule()) {


^---

It should read a short-circuit operator here instead of a bitwise operator.

Otherwise it LGTM.

Reviewed-by: Gustavo Romero 


Best regards,
Gustavo

Re: [PATCH 2/3] selftests/powerpc: Only run some tests on ppc64le

[Gustavo Romero]

Hi Michael,

On 07/27/2018 02:51 AM, Michael Ellerman wrote:

Gustavo Romero  writes:


Hi Michael,

On 07/26/2018 09:24 AM, Michael Ellerman wrote:

These tests are currently failing on (some) big endian systems. Until
we can fix that, skip them unless we're on ppc64le.


I can take a look at this.


Thanks!


Is that the same issue related to the gcc version we discussed a month ago?


Maybe, I've forgotten :)


If not, could you provide the crash logs as a starting point?


There's not much:

   test: tm_tar
   tags: git_version:9794259
   failure: tm_tar
   run-parts: ./tm-tar exited with return code 1
   
   test: tm_vmxcopy

   tags: git_version:9794259
   !! child died by signal 11
   failure: tm_vmxcopy
   run-parts: ./tm-vmxcopy exited with return code 1

And now I can't get the sigreturn one to fail :/


OK. I _think_ there is a chance it's the same issue we've discussed.

Hence, just for the record (in case somebody else wants to take a look at it as
well), on the occasion we had the following log about tm-sigreturn, that
although is not listed above (only tm-tar and tm-vmxcopy are ), it's marked to
run only on LE as per this patch, just like tm-tar and tm-vmxcopy:

[  403.458953] Unexpected TM Bad Thing exception at c006da4c (msr 
0x201032)
13:59:41 console: [  403.459865] Oops: Unrecoverable exception, sig: 6 [#1]
13:59:41 console: [  403.460286] BE SMP NR_CPUS=32 NUMA pSeries
13:59:41 console: [  403.460611] Modules linked in:
13:59:41 console: [  403.460929] CPU: 2 PID: 25233 Comm: tm-sigreturn Not 
tainted 4.16.0-gcc6x-g709b973 #1
13:59:41 console: [  403.461530] NIP:  c006da4c LR: c001d6a4 
CTR: 
13:59:41 console: [  403.461782] REGS: c0003ffcbd80 TRAP: 0700   Not 
tainted  (4.16.0-gcc6x-g709b973)
13:59:41 console: [  403.461943] MSR:  800300201032  
 CR: 48004288  XER: 2000
13:59:41 console: [  403.462112] CFAR: c001d6a0 SOFTE: 3
13:59:41 console: [  403.462112] PACATMSCRATCH: 00034280f032
13:59:41 console: [  403.462112] GPR00: d4018c01 c000f09bfc20 
c165bd00 c000fbd5c900
13:59:41 console: [  403.462112] GPR04: 80039032  
 
13:59:41 console: [  403.462112] GPR08:  0001 
0001 
13:59:41 console: [  403.462112] GPR12:  c0003fffdf00 
000f 1003a338
13:59:41 console: [  403.462112] GPR16: 10002e5c 10002e44 
10002d08 10002e24
13:59:41 console: [  403.462112] GPR20: 10002eac  
c000fbd5cdc8 c0df6578
13:59:41 console: [  403.462112] GPR24:  bfff 
fffef394 fffeee10
13:59:41 console: [  403.462112] GPR28: c000f09bfea0 0280d032 
 c000fbd5c900
13:59:41 console: [  403.463642] NIP [c006da4c] 
.tm_restore_sprs+0xc/0x1c
13:59:41 console: [  403.463782] LR [c001d6a4] 
.tm_recheckpoint.part.7+0x54/0x90
13:59:41 console: [  403.463912] Call Trace:
13:59:41 console: [  403.464003] [c000f09bfc20] [c0312ae0] 
.__might_fault+0x70/0x90 (unreliable)
13:59:41 console: [  403.464165] [c000f09bfca0] [c001b2b8] 
.restore_tm_user_regs.part.0+0x418/0x6c0
13:59:41 console: [  403.464326] [c000f09bfd70] [c001c55c] 
.compat_sys_sigreturn+0x14c/0x490
13:59:41 console: [  403.464495] [c000f09bfe30] [c000c070] 
system_call+0x58/0x6c
13:59:41 console: [  403.464624] Instruction dump:
13:59:41 console: [  403.464705] 7c800164 4e800020 7c0022a6 f80304b0 7c0222a6 
f80304b8 7c0122a6 f80304c0
13:59:41 console: [  403.464868] 4e800020 e80304b0 7c0023a6 e80304b8 <7c0223a6> 
e80304c0 7c0123a6 4e800020
13:59:41 console: [  403.465030] ---[ end trace 0045efc572a679cf ]---

and some initial debugging by mpe using xmon:

Unexpected TM Bad Thing exception at c0069e6c (msr 0x201032)
cpu 0x6: Vector: 700 (Program Check) at [c0003ff9bd80]
pc: c0069e6c: .tm_restore_sprs+0xc/0x1c
lr: c001e2a4: .tm_recheckpoint.part.11+0x64/0xf0
sp: c000f2a2bc30
   msr: 800300201032
  current = 0xc000fa68b000
  paca= 0xc0003fff8f00 softe: 3 irq_happened: 0x01
pid   = 3879, comm = tm-sigreturn
Linux version 4.17.0-rc1-gcc-7.0.1-5-g56376c5864f8 
(mich...@ka3.ozlabs.ibm.com) (gcc version 7.0.1 20170213 (experimental) (Custom 
8e8a14c238db56c7)) #172 SMP Thu Apr 19 21:48:31 AEST 2018
enter ? for help
[link register   ] c001e2a4 .tm_recheckpoint.part.11+0x64/0xf0
[c000f2a2bc30] c000f2a2bcb0 (unreliable)
[c000f2a2bcb0] c001c30c .restore_tm_user_regs.part.0+0x55c/0x610
[c000f2a2bd70] c001d400 .compat_sys_sigreturn+0x130/0x410
[c000f2a2be30] c000c268 system_call+0x58/0x6c
--- Exception: 300 (Data Access) at 19c8
SP (ffd75e90) is in userspace
6:mon> di %pc
c0069e6c  7c0223a6

Re: [PATCH 2/3] selftests/powerpc: Only run some tests on ppc64le

[Gustavo Romero]

Hi Michael,

On 07/26/2018 09:24 AM, Michael Ellerman wrote:

These tests are currently failing on (some) big endian systems. Until
we can fix that, skip them unless we're on ppc64le.


I can take a look at this.

Is that the same issue related to the gcc version we discussed a month ago?
If not, could you provide the crash logs as a starting point?
 


Thanks,
Gustavo


Signed-off-by: Michael Ellerman 
---
  tools/testing/selftests/powerpc/tm/tm-sigreturn.c | 1 +
  tools/testing/selftests/powerpc/tm/tm-tar.c   | 1 +
  tools/testing/selftests/powerpc/tm/tm-vmxcopy.c   | 1 +
  3 files changed, 3 insertions(+)

diff --git a/tools/testing/selftests/powerpc/tm/tm-sigreturn.c 
b/tools/testing/selftests/powerpc/tm/tm-sigreturn.c
index 85d63449243b..9a6017a1d769 100644
--- a/tools/testing/selftests/powerpc/tm/tm-sigreturn.c
+++ b/tools/testing/selftests/powerpc/tm/tm-sigreturn.c
@@ -55,6 +55,7 @@ int tm_sigreturn(void)
uint64_t ret = 0;

SKIP_IF(!have_htm());
+   SKIP_IF(!is_ppc64le());

memset(, 0, sizeof(sa));
sa.sa_handler = handler;
diff --git a/tools/testing/selftests/powerpc/tm/tm-tar.c 
b/tools/testing/selftests/powerpc/tm/tm-tar.c
index 2d2fcc2b7a60..f31fe5a28ddb 100644
--- a/tools/testing/selftests/powerpc/tm/tm-tar.c
+++ b/tools/testing/selftests/powerpc/tm/tm-tar.c
@@ -26,6 +26,7 @@ int test_tar(void)
int i;

SKIP_IF(!have_htm());
+   SKIP_IF(!is_ppc64le());

for (i = 0; i < num_loops; i++)
{
diff --git a/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c 
b/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c
index 0274de7b11f3..fe52811584ae 100644
--- a/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c
+++ b/tools/testing/selftests/powerpc/tm/tm-vmxcopy.c
@@ -46,6 +46,7 @@ int test_vmxcopy()
uint64_t aborted = 0;

SKIP_IF(!have_htm());
+   SKIP_IF(!is_ppc64le());

fd = mkstemp(tmpfile);
assert(fd >= 0);

Re: [PATCH 6/7] powerpc/traps: Print signal name for unhandled signals

[Gustavo Romero]

Hi Murilo,

LGTM.

Just a comment:

On 07/24/2018 04:27 PM, Murilo Opsfelder Araujo wrote:

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]


I _really_ don't want to bikeshed here, but I vouch for keeping the
"unhandled" word before the signal name, like:

[...] pandafault[6352]: unhandled segfault (11) at 00013a2a09f8 nip [...]

because the issue reported here is really that we got a segfault _and_
there was no handler to catch it.

But feel free to wait for additional comments to decide it.


Cheers,
Gustavo

Re: [PATCH] powerpc/64s: Fix NULL AT_BASE_PLATFORM when using DT CPU features

[Gustavo Romero]

Hi Yi,

On 03/14/2018 05:34 AM, Li Yi (Adam) wrote:

So set it in the DT CPU features code also.

This results in eg:
   $ LD_SHOW_AUXV=1 /bin/true | grep "AT_.*PLATFORM"
   AT_PLATFORM: power9
   AT_BASE_PLATFORM:power9



Is this issue related with DD2.2 CPU?
I tested on a Boston system with DD2.1 CPU, with kernel 4.14.23,
AT_BASE_PLATFORM value is correct:

[root@boston-sh-04 ~]# LD_SHOW_AUXV=1 /bin/true | grep "AT_.*PLATFORM"
AT_PLATFORM: power9
AT_BASE_PLATFORM:power9
[root@boston-sh-04 ~]# uname -a
Linux boston-sh-04 4.14.23 #1 SMP Sat Mar 3 12:14:56 CST 2018 ppc64le
ppc64le ppc64le GNU/Linux


It's not related to the processor version, rather it's related to if you
are under a VM or running on baremetal. The issue happens on baremetal
when you are using DT to get the CPU features. So I guess the test you
performed above is on a VM? On DD2.1 baremetal AT_BASE_PLATFORM is
missing as on a DD2.2 baremetal (if mpe's patch is not applied):

root@r222l:~# LD_SHOW_AUXV=1 /bin/true | grep "AT_.*PLATFORM"
AT_PLATFORM: power9
root@r222l:~# lscpu | fgrep Model
Model:   2.1 (pvr 004e 1201)
Model name:  POWER9, altivec supported
root@r222l:~# uname -a
Linux r222l 4.13.0-32-generic #35-Ubuntu SMP Thu Jan 25 09:05:20 UTC 2018 
ppc64le ppc64le ppc64le GNU/Linux
  


Regards,
Gustavo



Thanks,
-Yi


Signed-off-by: Michael Ellerman 
---
  arch/powerpc/kernel/dt_cpu_ftrs.c | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c 
b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 945e2c29ad2d..0bcfb0f256e1 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -720,6 +720,9 @@ static void __init cpufeatures_setup_finished(void)
 cur_cpu_spec->cpu_features |= CPU_FTR_HVMODE;
 }

+   /* Make sure powerpc_base_platform is non-NULL */
+   powerpc_base_platform = cur_cpu_spec->platform;
+
 system_registers.lpcr = mfspr(SPRN_LPCR);
 system_registers.hfscr = mfspr(SPRN_HFSCR);
 system_registers.fscr = mfspr(SPRN_FSCR);
--
2.14.1

Re: [PATCH] selftests/powerpc: Skip tm-unavailable if TM is not enabled

[Gustavo Romero]

Hi Cyril,

On 03/05/2018 08:49 PM, Cyril Bur wrote:
> On Mon, 2018-03-05 at 15:48 -0500, Gustavo Romero wrote:
>> Some processor revisions do not support transactional memory, and
>> additionally kernel support can be disabled. In either case the
>> tm-unavailable test should be skipped, otherwise it will fail with
>> a SIGILL.
>>
>> That commit also sets this selftest to be called through the test
>> harness as it's done for other TM selftests.
>>
>> Finally, it avoids using "ping" as a thread name since it's
>> ambiguous and can be confusing when shown, for instance,
>> in a kernel backtrace log.
>>
> 
> I spent more time than I care to admit looking at backtraces wondering
> how "ping" got in the mix ;).

heh sorry about that... :)


>> Fixes: 77fad8bfb1d2 ("selftests/powerpc: Check FP/VEC on exception in TM")
>> Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
> 
> Reviewed-by: Cyril Bur <cyril...@gmail.com>

Thanks for reviewing it.


Cheers,
Gustavo

>> ---
>>  .../testing/selftests/powerpc/tm/tm-unavailable.c  | 24 
>> ++
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c 
>> b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
>> index e6a0fad..156c8e7 100644
>> --- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
>> +++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
>> @@ -80,7 +80,7 @@ bool is_failure(uint64_t condition_reg)
>>  return ((condition_reg >> 28) & 0xa) == 0xa;
>>  }
>>  
>> -void *ping(void *input)
>> +void *tm_una_ping(void *input)
>>  {
>>  
>>  /*
>> @@ -280,7 +280,7 @@ void *ping(void *input)
>>  }
>>  
>>  /* Thread to force context switch */
>> -void *pong(void *not_used)
>> +void *tm_una_pong(void *not_used)
>>  {
>>  /* Wait thread get its name "pong". */
>>  if (DEBUG)
>> @@ -311,11 +311,11 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
>>  do {
>>  int rc;
>>  
>> -/* Bind 'ping' to CPU 0, as specified in 'attr'. */
>> -rc = pthread_create(, attr, ping, (void *) );
>> +/* Bind to CPU 0, as specified in 'attr'. */
>> +rc = pthread_create(, attr, tm_una_ping, (void *) );
>>  if (rc)
>>  pr_err(rc, "pthread_create()");
>> -rc = pthread_setname_np(t0, "ping");
>> +rc = pthread_setname_np(t0, "tm_una_ping");
>>  if (rc)
>>  pr_warn(rc, "pthread_setname_np");
>>  rc = pthread_join(t0, _value);
>> @@ -333,13 +333,15 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
>>  }
>>  }
>>  
>> -int main(int argc, char **argv)
>> +int tm_unavailable_test(void)
>>  {
>>  int rc, exception; /* FP = 0, VEC = 1, VSX = 2 */
>>  pthread_t t1;
>>  pthread_attr_t attr;
>>  cpu_set_t cpuset;
>>  
>> +SKIP_IF(!have_htm());
>> +
>>  /* Set only CPU 0 in the mask. Both threads will be bound to CPU 0. */
>>  CPU_ZERO();
>>  CPU_SET(0, );
>> @@ -354,12 +356,12 @@ int main(int argc, char **argv)
>>  if (rc)
>>  pr_err(rc, "pthread_attr_setaffinity_np()");
>>  
>> -rc = pthread_create(,  /* Bind 'pong' to CPU 0 */, pong, NULL);
>> +rc = pthread_create(,  /* Bind to CPU 0 */, tm_una_pong, NULL);
>>  if (rc)
>>  pr_err(rc, "pthread_create()");
>>  
>>  /* Name it for systemtap convenience */
>> -rc = pthread_setname_np(t1, "pong");
>> +rc = pthread_setname_np(t1, "tm_una_pong");
>>  if (rc)
>>  pr_warn(rc, "pthread_create()");
>>  
>> @@ -394,3 +396,9 @@ int main(int argc, char **argv)
>>  exit(0);
>>  }
>>  }
>> +
>> +int main(int argc, char **argv)
>> +{
>> +test_harness_set_timeout(220);
>> +return test_harness(tm_unavailable_test, "tm_unavailable_test");
>> +}
> 

[PATCH] selftests/powerpc: Skip tm-unavailable if TM is not enabled

[Gustavo Romero]

Some processor revisions do not support transactional memory, and
additionally kernel support can be disabled. In either case the
tm-unavailable test should be skipped, otherwise it will fail with
a SIGILL.

That commit also sets this selftest to be called through the test
harness as it's done for other TM selftests.

Finally, it avoids using "ping" as a thread name since it's
ambiguous and can be confusing when shown, for instance,
in a kernel backtrace log.

Fixes: 77fad8bfb1d2 ("selftests/powerpc: Check FP/VEC on exception in TM")
Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
---
 .../testing/selftests/powerpc/tm/tm-unavailable.c  | 24 ++
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c 
b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
index e6a0fad..156c8e7 100644
--- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
+++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
@@ -80,7 +80,7 @@ bool is_failure(uint64_t condition_reg)
return ((condition_reg >> 28) & 0xa) == 0xa;
 }
 
-void *ping(void *input)
+void *tm_una_ping(void *input)
 {
 
/*
@@ -280,7 +280,7 @@ void *ping(void *input)
 }
 
 /* Thread to force context switch */
-void *pong(void *not_used)
+void *tm_una_pong(void *not_used)
 {
/* Wait thread get its name "pong". */
if (DEBUG)
@@ -311,11 +311,11 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
do {
int rc;
 
-   /* Bind 'ping' to CPU 0, as specified in 'attr'. */
-   rc = pthread_create(, attr, ping, (void *) );
+   /* Bind to CPU 0, as specified in 'attr'. */
+   rc = pthread_create(, attr, tm_una_ping, (void *) );
if (rc)
pr_err(rc, "pthread_create()");
-   rc = pthread_setname_np(t0, "ping");
+   rc = pthread_setname_np(t0, "tm_una_ping");
if (rc)
pr_warn(rc, "pthread_setname_np");
rc = pthread_join(t0, _value);
@@ -333,13 +333,15 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
}
 }
 
-int main(int argc, char **argv)
+int tm_unavailable_test(void)
 {
int rc, exception; /* FP = 0, VEC = 1, VSX = 2 */
pthread_t t1;
pthread_attr_t attr;
cpu_set_t cpuset;
 
+   SKIP_IF(!have_htm());
+
/* Set only CPU 0 in the mask. Both threads will be bound to CPU 0. */
CPU_ZERO();
CPU_SET(0, );
@@ -354,12 +356,12 @@ int main(int argc, char **argv)
if (rc)
pr_err(rc, "pthread_attr_setaffinity_np()");
 
-   rc = pthread_create(,  /* Bind 'pong' to CPU 0 */, pong, NULL);
+   rc = pthread_create(,  /* Bind to CPU 0 */, tm_una_pong, NULL);
if (rc)
pr_err(rc, "pthread_create()");
 
/* Name it for systemtap convenience */
-   rc = pthread_setname_np(t1, "pong");
+   rc = pthread_setname_np(t1, "tm_una_pong");
if (rc)
pr_warn(rc, "pthread_create()");
 
@@ -394,3 +396,9 @@ int main(int argc, char **argv)
exit(0);
}
 }
+
+int main(int argc, char **argv)
+{
+   test_harness_set_timeout(220);
+   return test_harness(tm_unavailable_test, "tm_unavailable_test");
+}
-- 
2.7.4

[PATCH] Fixes for selftest tm-unavailable

[Gustavo Romero]

The recent fix by mpe for tm-trap [1] (thanks for fixing it! really
sorry for the late reply, I got dragged away by the darn thingy)
caught my attention to the fact that tm-unavailable needs a similar
fix. Also before that on Feb. Cyril Bur proposed another small fix for
the same selftest [2]. Since Cyril's change is not merged yet I
decided to take Cyril's fix into account as well. Finally, I also
noted that tm-unavailable is not using the test harness, thus I
added it to tm-unavailable.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=192b2e742c06af399e8eecb4a17265
[2] https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-February/169111.html

Gustavo Romero (1):
  selftests/powerpc: Skip tm-unavailable if TM is not enabled

 .../testing/selftests/powerpc/tm/tm-unavailable.c  | 24 ++
 1 file changed, 16 insertions(+), 8 deletions(-)

-- 
2.7.4

Re: [PATCH 00/26] KVM: PPC: Book3S PR: Transaction memory support on PR KVM

[Gustavo Romero]

Hi Simon,

On 01/11/2018 08:11 AM, wei.guo.si...@gmail.com wrote:
> From: Simon Guo 
> 
> In current days, many OS distributions have utilized transaction
> memory functionality. In PowerPC, HV KVM supports TM. But PR KVM
> does not.
> 
> The drive for the transaction memory support of PR KVM is the
> openstack Continuous Integration testing - They runs a HV(hypervisor)
> KVM(as level 1) and then run PR KVM(as level 2) on top of that.
> 
> This patch set add transaction memory support on PR KVM.

Is this correct to assume that this emulation mode will just kick in on P9
with kernel TM workarounds and HV KVM will continue to be used on POWER8
since HV KVM is supported on POWER8 hosts?


Regards,
Gustavo

> Test cases performed:
> linux/tools/testing/selftests/powerpc/tm/tm-syscall
> linux/tools/testing/selftests/powerpc/tm/tm-fork
> linux/tools/testing/selftests/powerpc/tm/tm-vmx-unavail
> linux/tools/testing/selftests/powerpc/tm/tm-tmspr
> linux/tools/testing/selftests/powerpc/tm/tm-signal-msr-resv
> linux/tools/testing/selftests/powerpc/math/vsx_preempt
> linux/tools/testing/selftests/powerpc/math/fpu_signal
> linux/tools/testing/selftests/powerpc/math/vmx_preempt
> linux/tools/testing/selftests/powerpc/math/fpu_syscall
> linux/tools/testing/selftests/powerpc/math/vmx_syscall
> linux/tools/testing/selftests/powerpc/math/fpu_preempt
> linux/tools/testing/selftests/powerpc/math/vmx_signal
> linux/tools/testing/selftests/powerpc/ptrace/ptrace-tm-gpr
> linux/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-gpr
> linux/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spd-vsx
> linux/tools/testing/selftests/powerpc/ptrace/ptrace-tm-spr
> linux/tools/testing/selftests/powerpc/ptrace/ptrace-tm-vsx
> https://github.com/justdoitqd/publicFiles/blob/master/test_tbegin_pr.c
> https://github.com/justdoitqd/publicFiles/blob/master/test_tabort.c
> https://github.com/justdoitqd/publicFiles/blob/master/test_kvm_htm_cap.c
> 
> Simon Guo (25):
>   KVM: PPC: Book3S PR: Move kvmppc_save_tm/kvmppc_restore_tm to separate
> file
>   KVM: PPC: Book3S PR: add new parameter (guest MSR) for
> kvmppc_save_tm()/kvmppc_restore_tm()
>   KVM: PPC: Book3S PR: turn on FP/VSX/VMX MSR bits in kvmppc_save_tm()
>   KVM: PPC: Book3S PR: add C function wrapper for
> _kvmppc_save/restore_tm()
>   KVM: PPC: Book3S PR: In PR KVM suspends Transactional state when
> inject an interrupt.
>   KVM: PPC: Book3S PR: PR KVM pass through MSR TM/TS bits to shadow_msr.
>   KVM: PPC: Book3S PR: add TEXASR related macros
>   KVM: PPC: Book3S PR: Sync TM bits to shadow msr for problem state
> guest
>   KVM: PPC: Book3S PR: implement RFID TM behavior to suppress change
> from S0 to N0
>   KVM: PPC: Book3S PR: set MSR HV bit accordingly for PPC970 and others.
>   KVM: PPC: Book3S PR: prevent TS bits change in kvmppc_interrupt_pr()
>   powerpc: export symbol msr_check_and_set().
>   KVM: PPC: Book3S PR: adds new
> kvmppc_copyto_vcpu_tm/kvmppc_copyfrom_vcpu_tm API for PR KVM.
>   KVM: PPC: Book3S PR: export tm_enable()/tm_disable/tm_abort() APIs
>   KVM: PPC: Book3S PR: add kvmppc_save/restore_tm_sprs() APIs
>   KVM: PPC: Book3S PR: add transaction memory save/restore skeleton for
> PR KVM
>   KVM: PPC: Book3S PR: add math support for PR KVM HTM
>   KVM: PPC: Book3S PR: make mtspr/mfspr emulation behavior based on
> active TM SPRs
>   KVM: PPC: Book3S PR: always fail transaction in guest privilege state
>   KVM: PPC: Book3S PR: enable NV reg restore for reading TM SPR at guest
> privilege state
>   KVM: PPC: Book3S PR: adds emulation for treclaim.
>   KVM: PPC: Book3S PR: add emulation for trechkpt in PR KVM.
>   KVM: PPC: Book3S PR: add emulation for tabort. for privilege guest
>   KVM: PPC: Book3S PR: add guard code to prevent returning to guest with
> PR=0 and Transactional state
>   KVM: PPC: Book3S PR: enable HTM for PR KVM for KVM_CHECK_EXTENSION
> ioctl
> 
>  arch/powerpc/include/asm/asm-prototypes.h   |  10 +
>  arch/powerpc/include/asm/kvm_book3s.h   |   8 +
>  arch/powerpc/include/asm/kvm_host.h |   3 +
>  arch/powerpc/include/asm/reg.h  |  25 +-
>  arch/powerpc/include/asm/tm.h   |   2 -
>  arch/powerpc/include/uapi/asm/tm.h  |   2 +-
>  arch/powerpc/kernel/process.c   |   1 +
>  arch/powerpc/kernel/tm.S|  12 +
>  arch/powerpc/kvm/Makefile   |   3 +
>  arch/powerpc/kvm/book3s.h   |   1 +
>  arch/powerpc/kvm/book3s_64_mmu.c|  11 +-
>  arch/powerpc/kvm/book3s_emulate.c   | 279 +++-
>  arch/powerpc/kvm/book3s_hv_rmhandlers.S | 259 ++-
>  arch/powerpc/kvm/book3s_pr.c| 256 +--
>  arch/powerpc/kvm/book3s_segment.S   |  13 +
>  arch/powerpc/kvm/powerpc.c  |   3 +-
>  arch/powerpc/kvm/tm.S   | 379 
> 
>  

[PATCH 2/2] powerpc/selftests: Check endianness on trap in TM

[Gustavo Romero]

Add a selftest to check if endianness is flipped inadvertently to BE
(MSR.LE set to zero) on BE and LE machines when a trap is caught in
transactional mode and load_fp and load_vec are zero, i.e. when MSR.FP
and MSR.VEC are zeroed (disabled).

Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
---
 tools/testing/selftests/powerpc/tm/.gitignore |   1 +
 tools/testing/selftests/powerpc/tm/Makefile   |   3 +-
 tools/testing/selftests/powerpc/tm/tm-trap.c  | 329 ++
 3 files changed, 332 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/powerpc/tm/tm-trap.c

diff --git a/tools/testing/selftests/powerpc/tm/.gitignore 
b/tools/testing/selftests/powerpc/tm/.gitignore
index 241a4a4..bb90d4b 100644
--- a/tools/testing/selftests/powerpc/tm/.gitignore
+++ b/tools/testing/selftests/powerpc/tm/.gitignore
@@ -13,3 +13,4 @@ tm-signal-context-chk-vmx
 tm-signal-context-chk-vsx
 tm-vmx-unavail
 tm-unavailable
+tm-trap
diff --git a/tools/testing/selftests/powerpc/tm/Makefile 
b/tools/testing/selftests/powerpc/tm/Makefile
index 8ed6f8c..a234539 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -3,7 +3,7 @@ SIGNAL_CONTEXT_CHK_TESTS := tm-signal-context-chk-gpr 
tm-signal-context-chk-fpu
tm-signal-context-chk-vmx tm-signal-context-chk-vsx
 
 TEST_GEN_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv 
tm-signal-stack \
-   tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail tm-unavailable \
+   tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail tm-unavailable 
tm-trap \
$(SIGNAL_CONTEXT_CHK_TESTS)
 
 include ../../lib.mk
@@ -18,6 +18,7 @@ $(OUTPUT)/tm-tmspr: CFLAGS += -pthread
 $(OUTPUT)/tm-vmx-unavail: CFLAGS += -pthread -m64
 $(OUTPUT)/tm-resched-dscr: ../pmu/lib.o
 $(OUTPUT)/tm-unavailable: CFLAGS += -O0 -pthread -m64 -Wno-error=uninitialized 
-mvsx
+$(OUTPUT)/tm-trap: CFLAGS += -O0 -pthread -m64
 
 SIGNAL_CONTEXT_CHK_TESTS := $(patsubst 
%,$(OUTPUT)/%,$(SIGNAL_CONTEXT_CHK_TESTS))
 $(SIGNAL_CONTEXT_CHK_TESTS): tm-signal.S
diff --git a/tools/testing/selftests/powerpc/tm/tm-trap.c 
b/tools/testing/selftests/powerpc/tm/tm-trap.c
new file mode 100644
index 000..5d92c23
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-trap.c
@@ -0,0 +1,329 @@
+/*
+ * Copyright 2017, Gustavo Romero, IBM Corp.
+ * Licensed under GPLv2.
+ *
+ * Check if thread endianness is flipped inadvertently to BE on trap
+ * caught in TM whilst MSR.FP and MSR.VEC are zero (i.e. just after
+ * load_fp and load_vec overflowed).
+ *
+ * The issue can be checked on LE machines simply by zeroing load_fp
+ * and load_vec and then causing a trap in TM. Since the endianness
+ * changes to BE on return from the signal handler, 'nop' is
+ * thread as an illegal instruction in following sequence:
+ * tbegin.
+ * beq 1f
+ * trap
+ * tend.
+ * 1:  nop
+ *
+ * However, although the issue is also present on BE machines, it's a
+ * bit trickier to check it on BE machines because MSR.LE bit is set
+ * to zero which determines a BE endianness that is the native
+ * endianness on BE machines, so nothing notably critical happens,
+ * i.e. no illegal instruction is observed immediately after returning
+ * from the signal handler (as it happens on LE machines). Thus to test
+ * it on BE machines LE endianness is forced after a first trap and then
+ * the endianness is verified on subsequent traps to determine if the
+ * endianness "flipped back" to the native endianness (BE).
+ */
+
+#define _GNU_SOURCE
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "tm.h"
+#include "utils.h"
+
+#define pr_error(error_code, format, ...) \
+   error_at_line(1, error_code, __FILE__, __LINE__, format, ##__VA_ARGS__)
+
+#define MSR_LE 1UL
+#define LE 1UL
+
+pthread_t t0_ping;
+pthread_t t1_pong;
+
+int exit_from_pong;
+
+int trap_event;
+int le;
+
+bool success;
+
+void trap_signal_handler(int signo, siginfo_t *si, void *uc)
+{
+   ucontext_t *ucp = uc;
+   uint64_t thread_endianness;
+
+   /* Get thread endianness: extract bit LE from MSR */
+   thread_endianness = MSR_LE & ucp->uc_mcontext.gp_regs[PT_MSR];
+
+   /***
+* Little-Endian Machine
+*/
+
+   if (le) {
+   /* First trap event */
+   if (trap_event == 0) {
+   /* Do nothing. Since it is returning from this trap
+* event that endianness is flipped by the bug, so just
+* let the process return from the signal handler and
+* check on the second trap event if endianness is
+* flipped or not.
+*/
+   }
+   /* Second trap event */
+   else if (trap_event == 1) {
+   /*
+* Sinc

[PATCH 1/2] powerpc/tm: Fix endianness flip on trap

[Gustavo Romero]

Currently it's possible that a thread on PPC64 LE has its endianness
flipped inadvertently to Big-Endian resulting in a crash once the process
is back from the signal handler.

If giveup_all() is called when regs->msr has the bits MSR.FP and MSR.VEC
disabled (and hence MSR.VSX disabled too) it returns without calling
check_if_tm_restore_required() which copies regs->msr to ckpt_regs->msr if
the process caught a signal whilst in transactional mode. Then once in
setup_tm_sigcontexts() MSR from ckpt_regs.msr is used, but since
check_if_tm_restore_required() was not called previuosly, gp_regs[PT_MSR]
gets a copy of invalid MSR bits as MSR in ckpt_regs was not updated from
regs->msr and so is zeroed. Later when leaving the signal handler once in
sys_rt_sigreturn() the TS bits of gp_regs[PT_MSR] are checked to determine
if restore_tm_sigcontexts() must be called to pull in the correct MSR state
into the user context. Because TS bits are zeroed
restore_tm_sigcontexts() is never called and MSR restored from the user
context on returning from the signal handler has the MSR.LE (the endianness
bit) forced to zero (Big-Endian). That leads, for instance, to 'nop' being
treated as an illegal instruction in the following sequence:

tbegin.
beq 1f
trap
tend.
1:  nop

on PPC64 LE machines and the process dies just after returning from the
signal handler.

PPC64 BE is also affected but in a subtle way since forcing Big-Endian on
a BE machine does not change the endianness.

This commit fixes the issue described above by ensuring that once in
setup_tm_sigcontexts() the MSR used is from regs->msr instead of from
ckpt_regs->msr and by ensuring that we pull in only the MSR.FP, MSR.VEC,
and MSR.VSX bits from ckpt_regs->msr.

The fix was tested both on LE and BE machines and no regression regarding
the powerpc/tm selftests was observed.

Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/signal_64.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 4b9ca35..b1b9962 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -207,7 +207,7 @@ static long setup_tm_sigcontexts(struct sigcontext __user 
*sc,
elf_vrreg_t __user *tm_v_regs = sigcontext_vmx_regs(tm_sc);
 #endif
struct pt_regs *regs = tsk->thread.regs;
-   unsigned long msr = tsk->thread.ckpt_regs.msr;
+   unsigned long msr = tsk->thread.regs->msr;
long err = 0;
 
BUG_ON(tsk != current);
@@ -216,6 +216,12 @@ static long setup_tm_sigcontexts(struct sigcontext __user 
*sc,
 
WARN_ON(tm_suspend_disabled);
 
+   /* Restore checkpointed FP, VEC, and VSX bits from ckpt_regs as
+* it contains the correct FP, VEC, VSX state after we treclaimed
+* the transaction and giveup_all() was called on reclaiming.
+*/
+   msr |= tsk->thread.ckpt_regs.msr & (MSR_FP | MSR_VEC | MSR_VSX);
+
/* Remove TM bits from thread's MSR.  The MSR in the sigcontext
 * just indicates to userland that we were doing a transaction, but we
 * don't want to return in transactional state.  This also ensures
-- 
2.7.4

Re: [PATCH 2/2] selftests/powerpc: Calculate spin time in tm-unavailable

[Gustavo Romero]

Hi Cyril,

On 21-11-2017 05:17, Cyril Bur wrote:
> Currently the tm-unavailable test spins for a fixed amount of time in
> an attempt to ensure the FPU/VMX/VSX facilities are off. This value was
> experimentally tested to be long enough.
> 
> Problems may arise if kernel heuristics were to change. This patch
> should future proof this test.

I've tried it on a VM running on '4.14.0-rc7' and apparently it gets stuck /
pretty slow on calibration, since it ran ~7m without finding the correct value
(before it would take about 3m), like:

$ time ./tm-unavailable
Testing required spin time required for facility unavailable...
Trying 0x1800...
Trying 0x1900...
Trying 0x1a00...
...
Trying 0xfd00... ^C

real7m15.304s
user7m15.291s
sys 0m0.004s

Trying it on a BM running on '4.13.0-rc2' it indeed found an initial value for
the timeout but for some reason the value was not sufficient for the subsequent
tests and the value raised more and more  (I understand that it's an expected
behavior tho). Even tho it runs about half the time (~3m, good!) but I think the
output could be little bit less "overloaded":

$ ./tm-unavailable
Testing required spin time required for facility unavailable...
Trying 0x1800...
Trying 0x1900...
Trying 0x1a00...
Trying 0x1b00...
Trying 0x1c00...
Trying 0x1d00...
Trying 0x1e00...
Trying 0x1f00... 1, 2, 3
Spin time required for a reliable facility unavailable TM failure: 0x1f00
Checking if FP/VEC registers are sane after a FP unavailable exception...
If MSR.FP=0 MSR.VEC=0:
Expecting the transaction to fail, but it didn't
FP ok VEC ok
Adjusting the facility unavailable spin time...
Trying 0x2100... 1, 2, 3
Now using 0x2100
If MSR.FP=0 MSR.VEC=0:
Expecting the transaction to fail, but it didn't
FP ok VEC ok
Adjusting the facility unavailable spin time...
Trying 0x2300... 1, 2, 3
Now using 0x2300
If MSR.FP=1 MSR.VEC=0: FP ok VEC ok
If MSR.FP=0 MSR.VEC=1:
Expecting the transaction to fail, but it didn't
FP ok VEC ok
Now using 0x4700
...

So, putting output question aside, are you getting a different result on VM,
i.e. did you notice if it got stuck/pretty slow?


Regards,
Gustavo

> Signed-off-by: Cyril Bur 
> ---
> Because the test no longer needs to use such a conservative time for
> the busy wait, it actually runs much faster.
> 
> 
>  .../testing/selftests/powerpc/tm/tm-unavailable.c  | 92 
> --
>  1 file changed, 84 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c 
> b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> index e6a0fad2bfd0..54aeb7a7fbb1 100644
> --- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> +++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> @@ -33,6 +33,11 @@
>  #define VEC_UNA_EXCEPTION1
>  #define VSX_UNA_EXCEPTION2
> 
> +#define ERR_RETRY 1
> +#define ERR_ADJUST 2
> +
> +#define COUNTER_INCREMENT (0x100)
> +
>  #define NUM_EXCEPTIONS   3
>  #define err_at_line(status, errnum, format, ...) \
>   error_at_line(status, errnum,  __FILE__, __LINE__, format ##__VA_ARGS__)
> @@ -45,6 +50,7 @@ struct Flags {
>   int touch_vec;
>   int result;
>   int exception;
> + uint64_t counter;
>  } flags;
> 
>  bool expecting_failure(void)
> @@ -87,14 +93,12 @@ void *ping(void *input)
>* Expected values for vs0 and vs32 after a TM failure. They must never
>* change, otherwise they got corrupted.
>*/
> + long rc = 0;
>   uint64_t high_vs0 = 0x;
>   uint64_t low_vs0 = 0x;
>   uint64_t high_vs32 = 0x;
>   uint64_t low_vs32 = 0x;
> 
> - /* Counter for busy wait */
> - uint64_t counter = 0x1ff00;
> -
>   /*
>* Variable to keep a copy of CR register content taken just after we
>* leave the transactional state.
> @@ -217,7 +221,7 @@ void *ping(void *input)
> [ex_fp] "i"  (FP_UNA_EXCEPTION),
> [ex_vec]"i"  (VEC_UNA_EXCEPTION),
> [ex_vsx]"i"  (VSX_UNA_EXCEPTION),
> -   [counter]   "r"  (counter)
> +   [counter]   "r"  (flags.counter)
> 
>   : "cr0", "ctr", "v10", "vs0", "vs10", "vs3", "vs32", "vs33",
> "vs34", "fr10"
> @@ -232,14 +236,14 @@ void *ping(void *input)
>   if (expecting_failure() && !is_failure(cr_)) {
>   printf("\n\tExpecting the transaction to fail, %s",
>   "but it didn't\n\t");
> - flags.result++;
> + rc = ERR_ADJUST;
>   }
> 
>   /* Check if we were not expecting a failure and a it occurred. */
>   if (!expecting_failure() && is_failure(cr_)) {
>   

Re: [PATCH 1/2] selftests/powerpc: Check for pthread errors in tm-unavailable

[Gustavo Romero]

Hi Cyril,

Thanks for adding the checks!

On 21-11-2017 05:17, Cyril Bur wrote:
> Signed-off-by: Cyril Bur <cyril...@gmail.com>

Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>

Cheers,
Gustavo

> ---
>  .../testing/selftests/powerpc/tm/tm-unavailable.c  | 43 
> +-
>  1 file changed, 34 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c 
> b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> index 96c37f84ce54..e6a0fad2bfd0 100644
> --- a/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> +++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
> @@ -15,6 +15,7 @@
>   */
> 
>  #define _GNU_SOURCE
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -33,6 +34,11 @@
>  #define VSX_UNA_EXCEPTION2
> 
>  #define NUM_EXCEPTIONS   3
> +#define err_at_line(status, errnum, format, ...) \
> + error_at_line(status, errnum,  __FILE__, __LINE__, format ##__VA_ARGS__)
> +
> +#define pr_warn(code, format, ...) err_at_line(0, code, format, 
> ##__VA_ARGS__)
> +#define pr_err(code, format, ...) err_at_line(1, code, format, ##__VA_ARGS__)
> 
>  struct Flags {
>   int touch_fp;
> @@ -303,10 +309,19 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
>* checking if the failure cause is the one we expect.
>*/
>   do {
> + int rc;
> +
>   /* Bind 'ping' to CPU 0, as specified in 'attr'. */
> - pthread_create(, attr, ping, (void *) );
> - pthread_setname_np(t0, "ping");
> - pthread_join(t0, _value);
> + rc = pthread_create(, attr, ping, (void *) );
> + if (rc)
> + pr_err(rc, "pthread_create()");
> + rc = pthread_setname_np(t0, "ping");
> + if (rc)
> + pr_warn(rc, "pthread_setname_np");
> + rc = pthread_join(t0, _value);
> + if (rc)
> + pr_err(rc, "pthread_join");
> +
>   retries--;
>   } while (ret_value != NULL && retries);
> 
> @@ -320,7 +335,7 @@ void test_fp_vec(int fp, int vec, pthread_attr_t *attr)
> 
>  int main(int argc, char **argv)
>  {
> - int exception; /* FP = 0, VEC = 1, VSX = 2 */
> + int rc, exception; /* FP = 0, VEC = 1, VSX = 2 */
>   pthread_t t1;
>   pthread_attr_t attr;
>   cpu_set_t cpuset;
> @@ -330,13 +345,23 @@ int main(int argc, char **argv)
>   CPU_SET(0, );
> 
>   /* Init pthread attribute. */
> - pthread_attr_init();
> + rc = pthread_attr_init();
> + if (rc)
> + pr_err(rc, "pthread_attr_init()");
> 
>   /* Set CPU 0 mask into the pthread attribute. */
> - pthread_attr_setaffinity_np(, sizeof(cpu_set_t), );
> -
> - pthread_create(,  /* Bind 'pong' to CPU 0 */, pong, NULL);
> - pthread_setname_np(t1, "pong"); /* Name it for systemtap convenience */
> + rc = pthread_attr_setaffinity_np(, sizeof(cpu_set_t), );
> + if (rc)
> + pr_err(rc, "pthread_attr_setaffinity_np()");
> +
> + rc = pthread_create(,  /* Bind 'pong' to CPU 0 */, pong, NULL);
> + if (rc)
> + pr_err(rc, "pthread_create()");
> +
> + /* Name it for systemtap convenience */
> + rc = pthread_setname_np(t1, "pong");
> + if (rc)
> + pr_warn(rc, "pthread_create()");
> 
>   flags.result = 0;
> 

Re: [PATCH] selftests/powerpc: Check FP/VEC on exception in TM

[Gustavo Romero]

Hi Cyril!

On 01-11-2017 20:10, Cyril Bur wrote:
> Thanks Gustavo,
> 
> I do have one more thought on an improvement for this test which is
> that:
> + /* Counter for busy wait *
> + uint64_t counter = 0x1ff00;
> is a bit fragile, what we should do is have the test work out long it
> should spin until it reliably gets a TM_CAUSE_FAC_UNAV failure and then
> use that for these tests.
> 
> This will only become a problem if we were to change kernel heuristics
> which is fine for now. I'll try to get that added soon but for now this
> test has proven too useful to delay adding as is.

I see. Yup, 'counter' value was indeed determined experimentally under many
different scenarios (VM and BM, different CPU loads, etc). At least if the
heuristics changes hurting the test it will catch that pointing out that
the expected failure did not happen, like:

Checking if FP/VEC registers are sane after a FP unavailable exception...
If MSR.FP=0 MSR.VEC=0:
Expecting the transaction to fail, but it didn't
FP ok VEC ok
...

So it won't let the hurting change pass fine silently :-)


>> Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
>> Signed-off-by: Breno Leitao <lei...@debian.org>
>> Signed-off-by: Cyril Bur <cyril...@gmail.com>

Thanks a lot for reviewing it.

Cheers,
Gustavo

[PATCH] selftests/powerpc: Check FP/VEC on exception in TM

[Gustavo Romero]

Add a self test to check if FP/VEC/VSX registers are sane (restored
correctly) after a FP/VEC/VSX unavailable exception is caught during a
transaction.

This test checks all possibilities in a thread regarding the combination
of MSR.[FP|VEC] states in a thread and for each scenario raises a
FP/VEC/VSX unavailable exception in transactional state, verifying if
vs0 and vs32 registers, which are representatives of FP/VEC/VSX reg
sets, are not corrupted.

Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
Signed-off-by: Breno Leitao <lei...@debian.org>
Signed-off-by: Cyril Bur <cyril...@gmail.com>
---
 tools/testing/selftests/powerpc/tm/Makefile|   3 +-
 .../testing/selftests/powerpc/tm/tm-unavailable.c  | 368 +
 tools/testing/selftests/powerpc/tm/tm.h|   5 +
 3 files changed, 375 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/powerpc/tm/tm-unavailable.c

diff --git a/tools/testing/selftests/powerpc/tm/Makefile 
b/tools/testing/selftests/powerpc/tm/Makefile
index 7bfcd45..24855c0 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -2,7 +2,7 @@ SIGNAL_CONTEXT_CHK_TESTS := tm-signal-context-chk-gpr 
tm-signal-context-chk-fpu
tm-signal-context-chk-vmx tm-signal-context-chk-vsx
 
 TEST_GEN_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv 
tm-signal-stack \
-   tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail \
+   tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail tm-unavailable \
$(SIGNAL_CONTEXT_CHK_TESTS)
 
 include ../../lib.mk
@@ -16,6 +16,7 @@ $(OUTPUT)/tm-syscall: CFLAGS += -I../../../../../usr/include
 $(OUTPUT)/tm-tmspr: CFLAGS += -pthread
 $(OUTPUT)/tm-vmx-unavail: CFLAGS += -pthread -m64
 $(OUTPUT)/tm-resched-dscr: ../pmu/lib.o
+$(OUTPUT)/tm-unavailable: CFLAGS += -O0 -pthread -m64 -Wno-error=uninitialized 
-mvsx
 
 SIGNAL_CONTEXT_CHK_TESTS := $(patsubst 
%,$(OUTPUT)/%,$(SIGNAL_CONTEXT_CHK_TESTS))
 $(SIGNAL_CONTEXT_CHK_TESTS): tm-signal.S
diff --git a/tools/testing/selftests/powerpc/tm/tm-unavailable.c 
b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
new file mode 100644
index 000..69a4e8c
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-unavailable.c
@@ -0,0 +1,368 @@
+/*
+ * Copyright 2017, Gustavo Romero, Breno Leitao, Cyril Bur, IBM Corp.
+ * Licensed under GPLv2.
+ *
+ * Force FP, VEC and VSX unavailable exception during transaction in all
+ * possible scenarios regarding the MSR.FP and MSR.VEC state, e.g. when FP
+ * is enable and VEC is disable, when FP is disable and VEC is enable, and
+ * so on. Then we check if the restored state is correctly set for the
+ * FP and VEC registers to the previous state we set just before we entered
+ * in TM, i.e. we check if it corrupts somehow the recheckpointed FP and
+ * VEC/Altivec registers on abortion due to an unavailable exception in TM.
+ * N.B. In this test we do not test all the FP/Altivec/VSX registers for
+ * corruption, but only for registers vs0 and vs32, which are respectively
+ * representatives of FP and VEC/Altivec reg sets.
+ */
+
+#define _GNU_SOURCE
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "tm.h"
+
+#define DEBUG 0
+
+/* Unavailable exceptions to test in HTM */
+#define FP_UNA_EXCEPTION   0
+#define VEC_UNA_EXCEPTION  1
+#define VSX_UNA_EXCEPTION  2
+
+#define NUM_EXCEPTIONS 3
+
+struct Flags {
+   int touch_fp;
+   int touch_vec;
+   int result;
+   int exception;
+} flags;
+
+bool expecting_failure(void)
+{
+   if (flags.touch_fp && flags.exception == FP_UNA_EXCEPTION)
+   return false;
+
+   if (flags.touch_vec && flags.exception == VEC_UNA_EXCEPTION)
+   return false;
+
+   /* If both FP and VEC are touched it does not mean that touching
+* VSX won't raise an exception. However since FP and VEC state
+* are already correctly loaded, the transaction is not aborted
+* (i.e. treclaimed/trecheckpointed) and MSR.VSX is just set as 1,
+* so a TM failure is not expected also in this case.
+*/
+   if ((flags.touch_fp && flags.touch_vec) &&
+flags.exception == VSX_UNA_EXCEPTION)
+   return false;
+
+   return true;
+}
+
+/* Check if failure occurred whilst in transaction. */
+bool is_failure(uint64_t condition_reg)
+{
+   /* When failure handling occurs, CR0 is set to 0b1010 (0xa). Otherwise
+* transaction completes without failure and hence reaches out 'tend.'
+* that sets CR0 to 0b0100 (0x4).
+*/
+   return ((condition_reg >> 28) & 0xa) == 0xa;
+}
+
+void *ping(void *input)
+{
+
+   /* Expected values for vs0 and vs32 after a TM failure. They must
+* never change, otherwise they got corrupted.
+*/
+   uint64_t high_vs0 = 0x;
+  

Re: [PATCH 2/3] powerpc/tm: P9 disabled suspend mode workaround

[Gustavo Romero]

Hi Cyril,

On 06-10-2017 04:46, Cyril Bur wrote:
> [added by Cyril Bur]
> As the no-suspend firmware change is novel and untested using it should
> be opt in by users. Furthumore, currently the kernel has no method to

I forgot to mention on my last reply, but should s/Furthumore/Furthermore/ ?

Regards,
Gustavo

Re: [PATCH 3/3] powerpc/tm: P9 disable transactionally suspended sigcontexts

[Gustavo Romero]

Hi Cyril,

On 06-10-2017 04:46, Cyril Bur wrote:
> From: Michael Neuling 
> 
> Unfortunately userspace can construct a sigcontext which enables
> suspend. Thus userspace can force Linux into a path where trechkpt is
> executed.
> 
> This patch blocks this from happening on POWER9 but sanity checking
> sigcontexts passed in.

I think "but" should say "by" as pointed out by Joel and acked by Mikey
previously.

Regards,
Gustavo

[PATCH v2] powerpc/tm: Flush TM only if CPU has TM feature

[Gustavo Romero]

Commit cd63f3c ("powerpc/tm: Fix saving of TM SPRs in core dump")
added code to access TM SPRs in flush_tmregs_to_thread(). However
flush_tmregs_to_thread() does not check if TM feature is available on
CPU before trying to access TM SPRs in order to copy live state to
thread structures. flush_tmregs_to_thread() is indeed guarded by
CONFIG_PPC_TRANSACTIONAL_MEM but it might be the case that kernel
was compiled with CONFIG_PPC_TRANSACTIONAL_MEM enabled and ran on
a CPU without TM feature available, thus rendering the execution
of TM instructions that are treated by the CPU as illegal instructions.

The fix is just to add proper checking in flush_tmregs_to_thread()
if CPU has the TM feature before accessing any TM-specific resource,
returning immediately if TM is no available on the CPU. Adding
that checking in flush_tmregs_to_thread() instead of in places
where it is called, like in vsr_get() and vsr_set(), is better because
avoids the same problem cropping up elsewhere.

Cc: sta...@vger.kernel.org # v4.13+
Fixes: cd63f3c ("powerpc/tm: Fix saving of TM SPRs in core dump")
Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 07cd22e..f52ad5b 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -131,7 +131,7 @@ static void flush_tmregs_to_thread(struct task_struct *tsk)
 * in the appropriate thread structures from live.
 */
 
-   if (tsk != current)
+   if ((!cpu_has_feature(CPU_FTR_TM)) || (tsk != current))
return;
 
if (MSR_TM_SUSPENDED(mfmsr())) {
-- 
2.7.4

[PATCH] powerpc/tm: Flush TM only if CPU has TM feature

[Gustavo Romero]

Commit cd63f3cf1d59 ("powerpc/tm: Fix saving of TM SPRs in core dump")
added code to access TM SPRs in flush_tmregs_to_thread(). However
flush_tmregs_to_thread() does not check if TM feature is available on
CPU before trying to access TM SPRs in order to copy live state to
thread structures. flush_tmregs_to_thread() is indeed guarded by
CONFIG_PPC_TRANSACTIONAL_MEM but it might be the case that kernel
was compiled with CONFIG_PPC_TRANSACTIONAL_MEM enabled and ran on
a CPU without TM feature available, thus rendering the execution
of TM instructions that are treated by the CPU as illegal instructions.

The fix is just to add proper checking in flush_tmregs_to_thread()
if CPU has the TM feature before accessing any TM-specific resource,
returning immediately if TM is no available on the CPU. Adding
that checking in flush_tmregs_to_thread() instead of in places
where it is called, like in vsr_get() and vsr_set(), is better because
avoids the same problem cropping up elsewhere.

Cc: sta...@vger.kernel.org # v4.13+
Fixes: cd63f3c ("powerpc/tm: Fix saving of TM SPRs in core dump")
Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 07cd22e..18f547f 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -124,6 +124,8 @@ static const struct pt_regs_offset regoffset_table[] = {
 static void flush_tmregs_to_thread(struct task_struct *tsk)
 {
/*
+* If CPU has no TM support, return.
+*
 * If task is not current, it will have been flushed already to
 * it's thread_struct during __switch_to().
 *
@@ -131,7 +133,7 @@ static void flush_tmregs_to_thread(struct task_struct *tsk)
 * in the appropriate thread structures from live.
 */
 
-   if (tsk != current)
+   if (!cpu_has_feature(CPU_FTR_TM) || tsk != current)
return;
 
if (MSR_TM_SUSPENDED(mfmsr())) {
-- 
2.7.4

[PATCH] powerpc/tm: fix TM SPRs in code dump file

[Gustavo Romero]

Currently flush_tmregs_to_thread() does not update accordingly the thread
structures from live state before a core dump rendering wrong values of
THFAR, TFIAR, and TEXASR in core dump files.

That commit fixes it by copying from live state to the appropriate thread
structures when it's necessary.

Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/ptrace.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/ptrace.c b/arch/powerpc/kernel/ptrace.c
index 925a4ef..660ed39 100644
--- a/arch/powerpc/kernel/ptrace.c
+++ b/arch/powerpc/kernel/ptrace.c
@@ -127,12 +127,19 @@ static void flush_tmregs_to_thread(struct task_struct 
*tsk)
 * If task is not current, it will have been flushed already to
 * it's thread_struct during __switch_to().
 *
-* A reclaim flushes ALL the state.
+* A reclaim flushes ALL the state or if not in TM save TM SPRs
+* in the appropriate thread structures from live.
 */
 
-   if (tsk == current && MSR_TM_SUSPENDED(mfmsr()))
-   tm_reclaim_current(TM_CAUSE_SIGNAL);
+   if (tsk != current)
+   return;
 
+   if (MSR_TM_SUSPENDED(mfmsr())) {
+   tm_reclaim_current(TM_CAUSE_SIGNAL);
+   } else {
+   tm_enable();
+   tm_save_sprs(&(tsk->thread));
+   }
 }
 #else
 static inline void flush_tmregs_to_thread(struct task_struct *tsk) { }
-- 
2.7.4

Re: Kernel build issues with upstream binutils

[Gustavo Romero]

No sure if in reply-to took correct effect, so I'm posting the thread ref 
explicit:
https://lists.ozlabs.org/pipermail/linuxppc-dev/2016-November/151672.html

On 17-07-2017 09:57, Gustavo Romero wrote:
> Hi,
> 
> On bissecting kernel on Debian (gcc version 6.3.0 20170516) I've got the same
> error on linking kernels <= v4.9 as reported previously in this thread:
> 
> ld: arch/powerpc/boot/zImage.pseries: Not enough room for program headers, 
> try linking with -N
> ld: final link failed: Bad value
> arch/powerpc/boot/Makefile:364: recipe for target 
> 'arch/powerpc/boot/zImage.pseries' failed
> make[1]: *** [arch/powerpc/boot/zImage.pseries] Error 1
> arch/powerpc/Makefile:278: recipe for target 'zImage' failed
> make: *** [zImage] Error 2
> 
> Applying the fix proposed by Nicholas resolved the issue.
> 
> However that patch is not present - afaics - in > v4.9 (where it builds fine).
> 
> Any clue on why that is happening on <= v4.9 but not on kernels above that?
> 
> Thank you.
> 
> 
> Regards,
> Gustavo
> 

Re: Kernel build issues with upstream binutils

[Gustavo Romero]

Hi,

On bissecting kernel on Debian (gcc version 6.3.0 20170516) I've got the same
error on linking kernels <= v4.9 as reported previously in this thread:

ld: arch/powerpc/boot/zImage.pseries: Not enough room for program headers, try 
linking with -N
ld: final link failed: Bad value
arch/powerpc/boot/Makefile:364: recipe for target 
'arch/powerpc/boot/zImage.pseries' failed
make[1]: *** [arch/powerpc/boot/zImage.pseries] Error 1
arch/powerpc/Makefile:278: recipe for target 'zImage' failed
make: *** [zImage] Error 2

Applying the fix proposed by Nicholas resolved the issue.

However that patch is not present - afaics - in > v4.9 (where it builds fine).

Any clue on why that is happening on <= v4.9 but not on kernels above that?

Thank you.


Regards,
Gustavo

Re: [bug] KVM: Unrecoverable TM Unavailable Exception f60

[Gustavo Romero]

Hi Jan

On 13-07-2017 09:07, Jan Stancek wrote:
>> [  181.328511] Unrecoverable TM Unavailable Exception f60 at d0001e7d9980
>> [  181.328605] Oops: Unrecoverable TM Unavailable Exception, sig: 6 [#1]
>> [  181.328613] SMP NR_CPUS=2048
>> [  181.328613] NUMA
>> [  181.328618] PowerNV
>> [  181.328646] Modules linked in: vhost_net vhost tap nfs_layout_nfsv41_files
>> rpcsec_gss_krb5 nfsv4 dns_resolver nfs fscache xt_CHECKSUM iptable_mangle
>> ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat
>> nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT
>> nf_reject_ipv4 tun ebtable_filter ebtables ip6table_filter ip6_tables
>> iptable_filter bridge stp llc kvm_hv kvm nfsd ses enclosure
>> scsi_transport_sas ghash_generic auth_rpcgss gf128mul xts sg ctr nfs_acl
>> lockd vmx_crypto shpchp ipmi_powernv i2c_opal grace ipmi_devintf i2c_core
>> powernv_rng sunrpc ipmi_msghandler ibmpowernv uio_pdrv_genirq uio
>> leds_powernv powernv_op_panel ip_tables xfs sd_mod lpfc ipr bnx2x libata
>> mdio ptp pps_core scsi_transport_fc libcrc32c dm_mirror dm_region_hash
>> dm_log dm_mod
>> [  181.329278] CPU: 40 PID: 9926 Comm: CPU 0/KVM Not tainted 4.12.0+ #1
>> [  181.329337] task: c03fc698 task.stack: c03fe4d8
>> [  181.329396] NIP: d0001e7d9980 LR: d0001e77381c CTR:
>> d0001e7d98f0
>> [  181.329465] REGS: c03fe4d837e0 TRAP: 0f60   Not tainted  (4.12.0+)
>> [  181.329523] MSR: 90009033 
>> [  181.329527]   CR: 24022448  XER: 
>> [  181.329608] CFAR: d0001e773818 SOFTE: 1
>> [  181.329608] GPR00: d0001e77381c c03fe4d83a60 d0001e7ef410
>> c03fdcfe
>> [  181.329608] GPR04: c03fe4f0  
>> c03fd7954800
>> [  181.329608] GPR08: 0001 c03fc698 
>> d0001e7e2880
>> [  181.329608] GPR12: d0001e7d98f0 c7b19000 0001295220e0
>> 7fffc0ce2090
>> [  181.329608] GPR16: 010011886608 7fff8c89f260 0001
>> 7fff8c080028
>> [  181.329608] GPR20:  0100118500a6 01001185
>> 01001185
>> [  181.329608] GPR24: 7fffc0ce1b48 01001185 d673b901
>> 
>> [  181.329608] GPR28:  c03fdcfe c03fdcfe
>> c03fe4f0
>> [  181.330199] NIP [d0001e7d9980] kvmppc_vcpu_run_hv+0x90/0x6b0 [kvm_hv]
>> [  181.330264] LR [d0001e77381c] kvmppc_vcpu_run+0x2c/0x40 [kvm]
>> [  181.330322] Call Trace:
>> [  181.330351] [c03fe4d83a60] [d0001e773478]
>> kvmppc_set_one_reg+0x48/0x340 [kvm] (unreliable)
>> [  181.330437] [c03fe4d83b30] [d0001e77381c]
>> kvmppc_vcpu_run+0x2c/0x40 [kvm]
>> [  181.330513] [c03fe4d83b50] [d0001e7700b4]
>> kvm_arch_vcpu_ioctl_run+0x114/0x2a0 [kvm]
>> [  181.330586] [c03fe4d83bd0] [d0001e7642f8]
>> kvm_vcpu_ioctl+0x598/0x7a0 [kvm]
>> [  181.330658] [c03fe4d83d40] [c03451b8] do_vfs_ioctl+0xc8/0x8b0
>> [  181.330717] [c03fe4d83de0] [c0345a64] SyS_ioctl+0xc4/0x120
>> [  181.330776] [c03fe4d83e30] [c000b004] system_call+0x58/0x6c
>> [  181.330833] Instruction dump:
>> [  181.330869] e92d0260 e9290b50 e9290108 792807e3 41820058 e92d0260 e9290b50
>> e9290108
>> [  181.330941] 792ae8a4 794a1f87 408204f4 e92d0260 <7d4022a6> f9490ff0
>> e92d0260 7d4122a6
>> [  181.331013] ---[ end trace 6f6ddeb4bfe92a92 ]---
>> [  181.334574]
>> [  183.334758] Kernel panic - not syncing: Fatal exception
>> [  183.338352] Rebooting in 10 seconds..


Looks like that TM unavailable exception will only able to recover
properly if it comes from problem state and since the trigger comes
from kernel space (kvm module) it does not match
"if (user_mode(regs))" in tm_unavailable().

I'm able to avoid this problem using the following patch:

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index d4e545d27ef9..1091dc4f4274 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1433,13 +1433,11 @@ void vsx_unavailable_exception(struct pt_regs *regs)
 static void tm_unavailable(struct pt_regs *regs)
 {
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-   if (user_mode(regs)) {
-   current->thread.load_tm++;
-   regs->msr |= MSR_TM;
-   tm_enable();
-   tm_restore_sprs(>thread);
-   return;
-   }
+   current->thread.load_tm++;
+   regs->msr |= MSR_TM;
+   tm_enable();
+   tm_restore_sprs(>thread);
+   return;
 #endif
pr_emerg("Unrecoverable TM Unavailable Exception "
"%lx at %lx\n", regs->trap, regs->nip);

Regards,
Gustavo

[PATCH] powerpc/tm: fix live state of vs0/32 in tm_reclaim

[Gustavo Romero]

Currently tm_reclaim() can return with a corrupted vs0 (fp0) or vs32 (v0)
due to the fact vs0 is used to save FPSCR and vs32 is used to save VSCR.

Later, we recheckpoint trusting that the live state of FP and VEC are ok
depending on the MSR.FP and MSR.VEC bits, i.e. if MSR.FP is enabled that
means the FP registers checkpointed when we entered in TM are correct and
after a treclaim. we can trust the FP live state. Similarly to VEC regs.
However if tm_reclaim() does not return a sane state then tm_recheckpoint()
will recheckpoint a corrupted state from live state back to the checkpoint
area.

That commit fixes the corruption by restoring vs0 and vs32 from the
ckfp_state and ckvr_state after they are used to save FPSCR and VSCR,
respectively.

The effect of the issue described above is observed, for instance, once a
VSX unavailable exception is caught in the middle of a transaction with
MSR.FP = 1 or MSR.VEC = 1. If MSR.FP = 1, then after getting back to user
space FP state is corrupted. If MSR.VEC = 1, then VEC state is corrupted.

The issue does not occur if MSR.FP = 0 and MSR.VEC = 0 because ckfp_state
and ckvr_state are both copied from fp_state and vr_state, respectively,
and on recheckpointing both states will be restored from these thread
structures and not from the live state.

The issue does not occur also if MSR.FP = 1 and MSR.VEC = 1 because it
implies MSR.VSX = 1 and in that case the VSX unavailable exception does not
happen in the middle of the transactional block.

Finally, that commit also fixes the MSR used to check if FP and VEC bits
are enabled once we are in tm_reclaim_thread(). ckpt_regs.msr is valid only
if giveup_all() is called *before* using ckpt_regs.msr for checks because
check_if_tm_restore_required() in giveup_all() will copy regs->msr to
ckpt_regs.msr and so ckpt_regs.msr reflects exactly the MSR that the thread
had when it came off the processor.

No regression was observed on powerpc/tm selftests after this fix.

Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
Signed-off-by: Breno Leitao <lei...@debian.org>
---
 arch/powerpc/kernel/process.c |  9 +++--
 arch/powerpc/kernel/tm.S  | 14 ++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 2ad725e..ac1fc51 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -864,6 +864,13 @@ static void tm_reclaim_thread(struct thread_struct *thr,
if (!MSR_TM_SUSPENDED(mfmsr()))
return;
 
+   /* Copy regs->msr to ckpt_regs.msr making the last valid for
+* the checks below. check_if_tm_restore_required() in
+* giveup_all() will take care of it. Also update fp_state
+* and vr_state from live state if the live state is valid.
+*/
+   giveup_all(container_of(thr, struct task_struct, thread));
+
/*
 * If we are in a transaction and FP is off then we can't have
 * used FP inside that transaction. Hence the checkpointed
@@ -883,8 +890,6 @@ static void tm_reclaim_thread(struct thread_struct *thr,
memcpy(>ckvr_state, >vr_state,
   sizeof(struct thread_vr_state));
 
-   giveup_all(container_of(thr, struct task_struct, thread));
-
tm_reclaim(thr, thr->ckpt_regs.msr, cause);
 }
 
diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
index 3a2d041..5dfbddb 100644
--- a/arch/powerpc/kernel/tm.S
+++ b/arch/powerpc/kernel/tm.S
@@ -259,9 +259,17 @@ _GLOBAL(tm_reclaim)
 
addir7, r3, THREAD_CKVRSTATE
SAVE_32VRS(0, r6, r7)   /* r6 scratch, r7 transact vr state */
+
+   /* Corrupting v0 (vs32). Should restore it later. */
mfvscr  v0
li  r6, VRSTATE_VSCR
stvxv0, r7, r6
+
+   /* Restore v0 (vs32) from ckvr_state.vr[0], otherwise we might
+* recheckpoint the wrong live value.
+*/
+   LXVD2X_ROT(32, R0, R7)
+
 dont_backup_vec:
mfspr   r0, SPRN_VRSAVE
std r0, THREAD_CKVRSAVE(r3)
@@ -272,9 +280,15 @@ dont_backup_vec:
addir7, r3, THREAD_CKFPSTATE
SAVE_32FPRS_VSRS(0, R6, R7) /* r6 scratch, r7 transact fp state */
 
+   /* Corrupting fr0 (vs0). Should restore it later. */
mffsfr0
stfdfr0,FPSTATE_FPSCR(r7)
 
+   /* Restore fr0 (vs0) from ckfp_state.fpr[0], otherwise we might
+* recheckpoint the wrong live value.
+*/
+   LXVD2X_ROT(0, R0, R7)
+
 dont_backup_fp:
 
/* TM regs, incl TEXASR -- these live in thread_struct.  Note they've
-- 
2.7.4

[PATCH 2/2] powerpc/tm: test for regs sanity in VSX exception

[Gustavo Romero]

Add a test to check if FP/VSX registers are sane (restored correctly) after
a VSX unavailable exception is caught in the middle of a transaction.

Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
Signed-off-by: Breno Leitao <lei...@debian.org>
---
 tools/testing/selftests/powerpc/tm/Makefile|   3 +-
 .../testing/selftests/powerpc/tm/tm-vsx-unavail.c  | 144 +
 2 files changed, 146 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/powerpc/tm/tm-vsx-unavail.c

diff --git a/tools/testing/selftests/powerpc/tm/Makefile 
b/tools/testing/selftests/powerpc/tm/Makefile
index 958c11c..d0ffbb8 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -2,7 +2,7 @@ SIGNAL_CONTEXT_CHK_TESTS := tm-signal-context-chk-gpr 
tm-signal-context-chk-fpu
tm-signal-context-chk-vmx tm-signal-context-chk-vsx
 
 TEST_GEN_PROGS := tm-resched-dscr tm-syscall tm-signal-msr-resv 
tm-signal-stack \
-   tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail \
+   tm-vmxcopy tm-fork tm-tar tm-tmspr tm-vmx-unavail tm-vsx-unavail \
$(SIGNAL_CONTEXT_CHK_TESTS)
 
 include ../../lib.mk
@@ -15,6 +15,7 @@ $(OUTPUT)/tm-syscall: tm-syscall-asm.S
 $(OUTPUT)/tm-syscall: CFLAGS += -I../../../../../usr/include
 $(OUTPUT)/tm-tmspr: CFLAGS += -pthread
 $(OUTPUT)/tm-vmx-unavail: CFLAGS += -pthread -m64
+$(OUTPUT)/tm-vsx-unavail: CFLAGS += -pthread -m64
 
 SIGNAL_CONTEXT_CHK_TESTS := $(patsubst 
%,$(OUTPUT)/%,$(SIGNAL_CONTEXT_CHK_TESTS))
 $(SIGNAL_CONTEXT_CHK_TESTS): tm-signal.S
diff --git a/tools/testing/selftests/powerpc/tm/tm-vsx-unavail.c 
b/tools/testing/selftests/powerpc/tm/tm-vsx-unavail.c
new file mode 100644
index 000..7ff933a
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-vsx-unavail.c
@@ -0,0 +1,144 @@
+/*
+ * Copyright 2017, Gustavo Romero and Breno Leitao, IBM Corp.
+ * Licensed under GPLv2.
+ *
+ * Force VSX unavailable exception during a transaction and see
+ * if it corrupts the checkpointed FP registers state after the abort.
+ */
+
+#define _GNU_SOURCE
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "tm.h"
+#include "utils.h"
+
+int passed;
+
+void *ping(void *not_used)
+{
+   asm goto(
+   // r3 = 0x
+   "lis  3, 0x;"
+   "ori  3, 3, 0x ;"
+   "sldi 3, 3, 32 ;"
+   "oris 3, 3, 0x ;"
+   "ori  3, 3, 0x ;"
+
+   //r4 = 0x
+   "lis  4, 0x;"
+   "ori  4, 4, 0x ;"
+   "sldi 4, 4, 32 ;"
+   "oris 4, 4, 0x ;"
+   "ori  4, 4, 0x ;"
+
+   // vs33 and vs34 will just be used to construct vs0 from r3 and
+   // r4. Both won't be used in any other place after that.
+   "mtvsrd 33, 3  ;"
+   "mtvsrd 34, 4  ;"
+
+   // vs0 = (r3 || r4) = 0x
+   "xxmrghd 0, 33, 34 ;"
+
+
+   // Wait ~8s so we have a sufficient amount of context
+   // switches so load_fp and load_vec overflow and MSR.FP, MSR.VEC
+   // and MSR.VSX are disabled.
+   "   lis  7, 0x1   ;"
+   "   ori  7, 7, 0xBFFE ;"
+   "   sldi 7, 7, 15 ;"
+   "1: addi 7, 7, -1 ;"
+   "   cmpdi7, 0 ;"
+   "   bne  1b   ;"
+
+   // Any floating-point instruction in here.
+   // N.B. 'fmr' is *not touching* any previously set register,
+   // i.e. it's not touching vs0.
+   "fmr10, 10 ;"
+
+   // vs0 is *still* 0x, right?
+   // Get in a transaction and cause a VSX unavailable exception.
+   "2: tbegin.   ;" // Begin HTM
+   "   beq  3f   ;" // Failure handler
+   "   xxmrghd  10, 10, 10   ;" // VSX unavailable in TM
+   "   tend. ;" // End HTM
+   "3: nop   ;" // Fall through to code below
+
+   // Immediately after a transaction failure we save vs0 to two
+   // general purpose registers to check its value. We need to have
+   // the same value as before we entered in transactional state.
+
+   // vs0 should be *still* 0x
+
+   // Save high half - MSB (64bit).
+   "mfvsrd  5, 0   

[PATCH 1/2] powerpc/tm: fix live state of vs0/32 in tm_reclaim

[Gustavo Romero]

Currently tm_reclaim() can return with a corrupted vs0 (fp0) or vs32 (v0)
due to the fact vs0 is used to save FPSCR and vs32 is used to save VSCR.

Later, we recheckpoint trusting that the live state of FP and VEC are ok
depending on the MSR.FP and MSR.VEC bits, i.e. if MSR.FP is enabled that
means the FP registers checkpointed before entering are correct and so
after a treclaim. we can trust the FP live state, and similarly on VEC regs.
However if tm_reclaim() does not return a sane state then tm_recheckpoint()
will recheckpoint a corrupted instate back to the checkpoint area.

That commit fixes the corruption by restoring vs0 and vs32 from the
ckfp_state and ckvr_state after they are used to save FPSCR and VSCR,
respectively.

The effect of the issue described above is observed, for instance, once a
VSX unavailable exception is caught in the middle of a transaction with
MSR.FP = 1 or MSR.VEC = 1. If MSR.FP = 1, then after getting back to user
space FP state is corrupted. If MSR.VEC = 1, then VEC state is corrupted.

The issue does occur if MSR.FP = 0 and MSR.VEC = 0 because ckfp_state and
ckvr_state are both copied from fp_state and vr_state, respectively, and
on recheckpointing both states will be restored from these thread
structures and not from the live state.

The issue does no occur also if MSR.FP = 1 and MSR.VEC = 1 because it
implies MSR.VSX = 1 and in that case the VSX unavailable exception does not
happen in the middle of the transactional block.

Finally, that commit also fixes the MSR used to check if FP or VEC bit are
enabled once we are in tm_reclaim_thread(). Checking ckpt_regs.msr is not
correct because giveup_all(), which copies regs->msr to ckpt_regs.msr, was
not called before and so the ckpt_regs.msr at that point is not valid, i.e.
it does not reflect the MSR state in userspace.

No regression was observed on powerpc/tm selftests after this fix.

Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
Signed-off-by: Breno Leitao <lei...@debian.org>
---
 arch/powerpc/kernel/process.c | 15 ---
 arch/powerpc/kernel/tm.S  | 16 
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 2ad725e..df8e348 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -870,21 +870,22 @@ static void tm_reclaim_thread(struct thread_struct *thr,
 * state is the same as the live state. We need to copy the
 * live state to the checkpointed state so that when the
 * transaction is restored, the checkpointed state is correct
-* and the aborted transaction sees the correct state. We use
-* ckpt_regs.msr here as that's what tm_reclaim will use to
-* determine if it's going to write the checkpointed state or
-* not. So either this will write the checkpointed registers,
-* or reclaim will. Similarly for VMX.
+* and the aborted transaction sees the correct state. So
+* either this will write the checkpointed registers, or
+* reclaim will. Similarly for VMX.
 */
-   if ((thr->ckpt_regs.msr & MSR_FP) == 0)
+   if ((thr->regs->msr & MSR_FP) == 0)
memcpy(>ckfp_state, >fp_state,
   sizeof(struct thread_fp_state));
-   if ((thr->ckpt_regs.msr & MSR_VEC) == 0)
+   if ((thr->regs->msr & MSR_VEC) == 0)
memcpy(>ckvr_state, >vr_state,
   sizeof(struct thread_vr_state));
 
giveup_all(container_of(thr, struct task_struct, thread));
 
+   /* After giveup_all(), ckpt_regs.msr contains the same value
+* of regs->msr when we entered in kernel space.
+*/
tm_reclaim(thr, thr->ckpt_regs.msr, cause);
 }
 
diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
index 3a2d041..67b56af 100644
--- a/arch/powerpc/kernel/tm.S
+++ b/arch/powerpc/kernel/tm.S
@@ -259,9 +259,18 @@ _GLOBAL(tm_reclaim)
 
addir7, r3, THREAD_CKVRSTATE
SAVE_32VRS(0, r6, r7)   /* r6 scratch, r7 transact vr state */
+
+   /* Corrupting v0 (vs32). Should restore it later. */
mfvscr  v0
li  r6, VRSTATE_VSCR
stvxv0, r7, r6
+
+   /* Restore v0 (vs32) from ckvr_state.vr[0], otherwise we might
+* recheckpoint the wrong live value.
+*/
+   lxvxvs32, 0, r7
+   xxswapd vs32, vs32
+
 dont_backup_vec:
mfspr   r0, SPRN_VRSAVE
std r0, THREAD_CKVRSAVE(r3)
@@ -272,9 +281,16 @@ dont_backup_vec:
addir7, r3, THREAD_CKFPSTATE
SAVE_32FPRS_VSRS(0, R6, R7) /* r6 scratch, r7 transact fp state */
 
+   /* Corrupting fr0 (vs0). Should restore it later. */
mffsfr0
stfdfr0,FPSTATE_FPSCR(r7)
 
+   /* Restore fr0 (vs32) from ckfp_state.fpr[0], otherwise we might
+* recheckpoint the wrong live val

[RFC] Fix si->si_code for guard page access on PowerPC

[Gustavo Romero]

Fix si->si_code for guard page access on PowerPC

Currently, the mm code on PowerPC/POWER returns a si->si_code = 2
(SEGV_ACCERR) when the stack tries to grow beyond the stack guard
(stack ulimit). On other architectures, notably x86, the si->si_code
returned when a guard page access occurs is 1 (SEGV_MAPERR).

Although si->si_code is not historically reliable and hence no
program should trust it for any semantic behavior, the right
si->si_code for a guard page access is 1 (SEGV_MAPERR) and,
besides that, some tests still trust it in specific cases.

On PowerPC/POWER, if the mm tries to expand the stack and
hits a page mapped by the program (say, an anonymous page
with permission ---p) it generates a SIG_SEGV and a si->si_code = 2
(SEGV_ACCERR), the same way it happens on x86. But then, when this
guard page is removed (un-mapped) and the stack grows again reaching
the stack guard (stack ulimit), the mm generates a SIG_SEGV and a
si->si_code = 2 (SEGV_ACCERR) again, contrary to, for example,
what happens on x86 (si->si_code = 1 (SIG_MAPERR)). It means that
on PowerPC/POWER there is no semantic difference between a stack
growth hitting a mapped area the stack has no permission to rd/wr
and reaching the stack limit (stack ulimit), although indeed there
is a difference.

Signed-off-by: Gustavo Romero <grom...@linux.vnet.ibm.com>
---
 arch/powerpc/mm/fault.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index a67c6d7..6954971 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -431,8 +431,10 @@ good_area:
   */
  fault = handle_mm_fault(mm, vma, address, flags);
  if (unlikely(fault & (VM_FAULT_RETRY|VM_FAULT_ERROR))) {
-  if (fault & VM_FAULT_SIGSEGV)
+  if (fault & VM_FAULT_SIGSEGV) {
+   code = SEGV_MAPERR;
goto bad_area;
+  }
   rc = mm_fault_error(regs, address, fault);
   if (rc >= MM_FAULT_RETURN)
goto bail;

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