[PATCH 2/2] KVM: PPC: Book3s HV: Remove unused function kvmppc_bad_interrupt

[Murilo Opsfelder Araujo]

The commit fae5c9f3664b ("KVM: PPC: Book3S HV: remove ISA v3.0 and v3.1
support from P7/8 path") removed the last reference to the function.

Fixes: fae5c9f3664b ("KVM: PPC: Book3S HV: remove ISA v3.0 and v3.1 support 
from P7/8 path")
Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/include/asm/kvm_book3s.h |  1 -
 arch/powerpc/kvm/book3s_hv_builtin.c  | 18 --
 2 files changed, 19 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_book3s.h 
b/arch/powerpc/include/asm/kvm_book3s.h
index ff1336ab4c47..bbf5e2c5fe09 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -280,7 +280,6 @@ extern void kvmppc_copy_to_svcpu(struct kvm_vcpu *vcpu);
 extern void kvmppc_copy_from_svcpu(struct kvm_vcpu *vcpu);
 
 long kvmppc_read_intr(void);
-void kvmppc_bad_interrupt(struct pt_regs *regs);
 void kvmppc_set_msr_hv(struct kvm_vcpu *vcpu, u64 msr);
 void kvmppc_inject_interrupt_hv(struct kvm_vcpu *vcpu, int vec, u64 
srr1_flags);
 
diff --git a/arch/powerpc/kvm/book3s_hv_builtin.c 
b/arch/powerpc/kvm/book3s_hv_builtin.c
index 88a8f6473c4e..c15c6faedce5 100644
--- a/arch/powerpc/kvm/book3s_hv_builtin.c
+++ b/arch/powerpc/kvm/book3s_hv_builtin.c
@@ -489,24 +489,6 @@ static long kvmppc_read_one_intr(bool *again)
return kvmppc_check_passthru(xisr, xirr, again);
 }
 
-void kvmppc_bad_interrupt(struct pt_regs *regs)
-{
-   /*
-* 100 could happen at any time, 200 can happen due to invalid real
-* address access for example (or any time due to a hardware problem).
-*/
-   if (TRAP(regs) == 0x100) {
-   get_paca()->in_nmi++;
-   system_reset_exception(regs);
-   get_paca()->in_nmi--;
-   } else if (TRAP(regs) == 0x200) {
-   machine_check_exception(regs);
-   } else {
-   die("Bad interrupt in KVM entry/exit code", regs, SIGABRT);
-   }
-   panic("Bad KVM trap");
-}
-
 static void kvmppc_end_cede(struct kvm_vcpu *vcpu)
 {
vcpu->arch.ceded = 0;
-- 
2.36.1

[PATCH 1/2] KVM: PPC: Book3S HV: Remove kvmhv_p9_[set,restore]_lpcr declarations

[Murilo Opsfelder Araujo]

The commit b1b1697ae0cc ("KVM: PPC: Book3S HV: Remove support for
running HPT guest on RPT host without mixed mode support") removed the
last references to these functions.

Fixes: b1b1697ae0cc ("KVM: PPC: Book3S HV: Remove support for running HPT guest 
on RPT host without mixed mode support")
Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/include/asm/kvm_book3s.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_book3s.h 
b/arch/powerpc/include/asm/kvm_book3s.h
index 91c9f937edcd..ff1336ab4c47 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -281,8 +281,6 @@ extern void kvmppc_copy_from_svcpu(struct kvm_vcpu *vcpu);
 
 long kvmppc_read_intr(void);
 void kvmppc_bad_interrupt(struct pt_regs *regs);
-void kvmhv_p9_set_lpcr(struct kvm_split_mode *sip);
-void kvmhv_p9_restore_lpcr(struct kvm_split_mode *sip);
 void kvmppc_set_msr_hv(struct kvm_vcpu *vcpu, u64 msr);
 void kvmppc_inject_interrupt_hv(struct kvm_vcpu *vcpu, int vec, u64 
srr1_flags);
 
-- 
2.36.1

[PATCH 0/2] KVM: PPC: Book3s HV: Cleanup unused function and declarations

[Murilo Opsfelder Araujo]

Minor cleanup to remove unused function and declarations.

Murilo Opsfelder Araujo (2):
  KVM: PPC: Book3S HV: Remove kvmhv_p9_[set,restore]_lpcr declarations
  KVM: PPC: Book3s HV: Remove unused function kvmppc_bad_interrupt

 arch/powerpc/include/asm/kvm_book3s.h |  3 ---
 arch/powerpc/kvm/book3s_hv_builtin.c  | 18 --
 2 files changed, 21 deletions(-)

-- 
2.36.1

[PATCH v2] powerpc/64s: Fix build failure when CONFIG_PPC_64S_HASH_MMU is not set

[Murilo Opsfelder Araujo]

The following build failure occurs when CONFIG_PPC_64S_HASH_MMU is not
set:

arch/powerpc/kernel/setup_64.c: In function ‘setup_per_cpu_areas’:
arch/powerpc/kernel/setup_64.c:811:21: error: ‘mmu_linear_psize’ undeclared 
(first use in this function); did you mean ‘mmu_virtual_psize’?
  811 | if (mmu_linear_psize == MMU_PAGE_4K)
  | ^~~~
  | mmu_virtual_psize
arch/powerpc/kernel/setup_64.c:811:21: note: each undeclared identifier is 
reported only once for each function it appears in

Move the declaration of mmu_linear_psize outside of
CONFIG_PPC_64S_HASH_MMU ifdef.

After the above is fixed, it fails later with the following error:

ld: arch/powerpc/kexec/file_load_64.o: in function 
`.arch_kexec_kernel_image_probe':
file_load_64.c:(.text+0x1c1c): undefined reference to `.add_htab_mem_range'

Fix that, too, by conditioning add_htab_mem_range() symbol to
CONFIG_PPC_64S_HASH_MMU.

Fixes: 387e220a2e5e (powerpc/64s: Move hash MMU support code under 
CONFIG_PPC_64S_HASH_MMU)
Reported-by: Erhard F. 
Signed-off-by: Murilo Opsfelder Araujo 
Cc: Michael Ellerman 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Nicholas Piggin 
Cc: Fabiano Rosas 
Cc: Nick Child 
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215567
Signed-off-by: Murilo Opsfelder Araujo 
---
v1: 
https://lore.kernel.org/linuxppc-dev/20220301164843.29170-1-muri...@linux.ibm.com/

 arch/powerpc/include/asm/book3s/64/mmu.h | 2 +-
 arch/powerpc/include/asm/kexec_ranges.h  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h 
b/arch/powerpc/include/asm/book3s/64/mmu.h
index ba5b1becf518..006cbec70ffe 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu.h
@@ -202,7 +202,6 @@ static inline struct subpage_prot_table 
*mm_ctx_subpage_prot(mm_context_t *ctx)
 /*
  * The current system page and segment sizes
  */
-extern int mmu_linear_psize;
 extern int mmu_virtual_psize;
 extern int mmu_vmalloc_psize;
 extern int mmu_io_psize;
@@ -213,6 +212,7 @@ extern int mmu_io_psize;
 #define mmu_virtual_psize MMU_PAGE_4K
 #endif
 #endif
+extern int mmu_linear_psize;
 extern int mmu_vmemmap_psize;
 
 /* MMU initialization */
diff --git a/arch/powerpc/include/asm/kexec_ranges.h 
b/arch/powerpc/include/asm/kexec_ranges.h
index 7a9f8d15..f83866a19e87 100644
--- a/arch/powerpc/include/asm/kexec_ranges.h
+++ b/arch/powerpc/include/asm/kexec_ranges.h
@@ -9,7 +9,7 @@ struct crash_mem *realloc_mem_ranges(struct crash_mem 
**mem_ranges);
 int add_mem_range(struct crash_mem **mem_ranges, u64 base, u64 size);
 int add_tce_mem_ranges(struct crash_mem **mem_ranges);
 int add_initrd_mem_range(struct crash_mem **mem_ranges);
-#ifdef CONFIG_PPC_BOOK3S_64
+#ifdef CONFIG_PPC_64S_HASH_MMU
 int add_htab_mem_range(struct crash_mem **mem_ranges);
 #else
 static inline int add_htab_mem_range(struct crash_mem **mem_ranges)
-- 
2.35.1

[PATCH] powerpc/64s: Fix build failure when CONFIG_PPC_64S_HASH_MMU is not set

[Murilo Opsfelder Araujo]

The following build failure occurs when CONFIG_PPC_64S_HASH_MMU is not set:

arch/powerpc/kernel/setup_64.c: In function ‘setup_per_cpu_areas’:
arch/powerpc/kernel/setup_64.c:811:21: error: ‘mmu_linear_psize’ undeclared 
(first use in this function); did you mean ‘mmu_virtual_psize’?
  811 | if (mmu_linear_psize == MMU_PAGE_4K)
  | ^~~~
  | mmu_virtual_psize
arch/powerpc/kernel/setup_64.c:811:21: note: each undeclared identifier is 
reported only once for each function it appears in

Move the declaration of mmu_linear_psize outside of CONFIG_PPC_64S_HASH_MMU
ifdef.

Fixes: 387e220a2e5e (powerpc/64s: Move hash MMU support code under 
CONFIG_PPC_64S_HASH_MMU)
Reported-by: Erhard F. 
Signed-off-by: Murilo Opsfelder Araujo 
Cc: Michael Ellerman 
Cc: Benjamin Herrenschmidt 
Cc: Paul Mackerras 
Cc: Nicholas Piggin 
Cc: Fabiano Rosas 
Cc: Nick Child 
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215567
---
 arch/powerpc/include/asm/book3s/64/mmu.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/book3s/64/mmu.h 
b/arch/powerpc/include/asm/book3s/64/mmu.h
index ba5b1becf518..006cbec70ffe 100644
--- a/arch/powerpc/include/asm/book3s/64/mmu.h
+++ b/arch/powerpc/include/asm/book3s/64/mmu.h
@@ -202,7 +202,6 @@ static inline struct subpage_prot_table 
*mm_ctx_subpage_prot(mm_context_t *ctx)
 /*
  * The current system page and segment sizes
  */
-extern int mmu_linear_psize;
 extern int mmu_virtual_psize;
 extern int mmu_vmalloc_psize;
 extern int mmu_io_psize;
@@ -213,6 +212,7 @@ extern int mmu_io_psize;
 #define mmu_virtual_psize MMU_PAGE_4K
 #endif
 #endif
+extern int mmu_linear_psize;
 extern int mmu_vmemmap_psize;
 
 /* MMU initialization */
-- 
2.35.1

[PATCH 3/3] powerpc/dt_cpu_ftrs: Make use of macro ISA_V3_1

[Murilo Opsfelder Araujo]

Macro ISA_V3_1 was defined but never used.  Use it instead of literal.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/dt_cpu_ftrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c 
b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 9d6e833da2bd..a0edeb391e3e 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -678,7 +678,7 @@ static void __init cpufeatures_setup_start(u32 isa)
cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_ARCH_3_00;
}
 
-   if (isa >= 3100) {
+   if (isa >= ISA_V3_1) {
cur_cpu_spec->cpu_features |= CPU_FTR_ARCH_31;
cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_ARCH_3_1;
}
-- 
2.25.4

[PATCH 2/3] powerpc/dt_cpu_ftrs: Make use of macro ISA_V3_0B

[Murilo Opsfelder Araujo]

Macro ISA_V3_0B was defined but never used.  Use it instead of
literal.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/dt_cpu_ftrs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c 
b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 1b7da8b5ce0d..9d6e833da2bd 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -673,7 +673,7 @@ static void __init cpufeatures_setup_start(u32 isa)
 {
pr_info("setup for ISA %d\n", isa);
 
-   if (isa >= 3000) {
+   if (isa >= ISA_V3_0B) {
cur_cpu_spec->cpu_features |= CPU_FTR_ARCH_300;
cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_ARCH_3_00;
}
-- 
2.25.4

[PATCH 0/3] powerpc/dt_cpu_ftrs: Make use of ISA_V3_* macros

[Murilo Opsfelder Araujo]

The first patch removes unused macro ISA_V2_07B.  The second and third
patches make use of macros ISA_V3_0B and ISA_V3_1, respectively,
instead their corresponding literals.

Murilo Opsfelder Araujo (3):
  powerpc/dt_cpu_ftrs: Remove unused macro ISA_V2_07B
  powerpc/dt_cpu_ftrs: Make use of macro ISA_V3_0B
  powerpc/dt_cpu_ftrs: Make use of macro ISA_V3_1

 arch/powerpc/kernel/dt_cpu_ftrs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

--
2.25.4

[PATCH 1/3] powerpc/dt_cpu_ftrs: Remove unused macro ISA_V2_07B

[Murilo Opsfelder Araujo]

Macro ISA_V2_07B is defined but not used anywhere else in the code.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/dt_cpu_ftrs.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c 
b/arch/powerpc/kernel/dt_cpu_ftrs.c
index 3a409517c031..1b7da8b5ce0d 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
@@ -24,7 +24,6 @@
 
 
 /* Device-tree visible constants follow */
-#define ISA_V2_07B  2070
 #define ISA_V3_0B   3000
 #define ISA_V3_13100
 
-- 
2.25.4

Re: [PATCH v3 1/2] mm: add probe_user_read()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Wed, Jan 16, 2019 at 04:59:27PM +, Christophe Leroy wrote:
> In powerpc code, there are several places implementing safe
> access to user data. This is sometimes implemented using
> probe_kernel_address() with additional access_ok() verification,
> sometimes with get_user() enclosed in a pagefault_disable()/enable()
> pair, etc. :
> show_user_instructions()
> bad_stack_expansion()
> p9_hmi_special_emu()
> fsl_pci_mcheck_exception()
> read_user_stack_64()
> read_user_stack_32() on PPC64
> read_user_stack_32() on PPC32
> power_pmu_bhrb_to()
>
> In the same spirit as probe_kernel_read(), this patch adds
> probe_user_read().
>
> probe_user_read() does the same as probe_kernel_read() but
> first checks that it is really a user address.
>
> The patch defines this function as a static inline so the "size"
> variable can be examined for const-ness by the check_object_size()
> in __copy_from_user_inatomic()
>
> Signed-off-by: Christophe Leroy 
> ---
>  v3: Moved 'Returns:" comment after description.
>  Explained in the commit log why the function is defined static inline
>
>  v2: Added "Returns:" comment and removed probe_user_address()
>
>  include/linux/uaccess.h | 34 ++
>  1 file changed, 34 insertions(+)
>
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 37b226e8df13..ef99edd63da3 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -263,6 +263,40 @@ extern long strncpy_from_unsafe(char *dst, const void 
> *unsafe_addr, long count);
>  #define probe_kernel_address(addr, retval)   \
>   probe_kernel_read(, addr, sizeof(retval))
>
> +/**
> + * probe_user_read(): safely attempt to read from a user location
> + * @dst: pointer to the buffer that shall take the data
> + * @src: address to read from
> + * @size: size of the data chunk
> + *
> + * Safely read from address @src to the buffer at @dst.  If a kernel fault
> + * happens, handle that and return -EFAULT.
> + *
> + * We ensure that the copy_from_user is executed in atomic context so that
> + * do_page_fault() doesn't attempt to take mmap_sem.  This makes
> + * probe_user_read() suitable for use within regions where the caller
> + * already holds mmap_sem, or other locks which nest inside mmap_sem.
> + *
> + * Returns: 0 on success, -EFAULT on error.
> + */
> +
> +#ifndef probe_user_read
> +static __always_inline long probe_user_read(void *dst, const void __user 
> *src,
> + size_t size)
> +{
> + long ret;
> +
> + if (!access_ok(src, size))
> + return -EFAULT;

Hopefully, there is still time for a minor comment.

Do we need to differentiate the returned error here, e.g.: return
-EACCES?

I wonder if there will be situations where callers need to know why
probe_user_read() failed.

Besides that:

Acked-by: Murilo Opsfelder Araujo 

> +
> + pagefault_disable();
> + ret = __copy_from_user_inatomic(dst, src, size);
> + pagefault_enable();
> +
> + return ret ? -EFAULT : 0;
> +}
> +#endif
> +
>  #ifndef user_access_begin
>  #define user_access_begin(ptr,len) access_ok(ptr, len)
>  #define user_access_end() do { } while (0)
> --
> 2.13.3
>

--
Murilo

Re: [PATCH kernel v7 20/20] vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver

[Murilo Opsfelder Araujo]

On Thu, Dec 20, 2018 at 07:23:50PM +1100, Alexey Kardashevskiy wrote:
> POWER9 Witherspoon machines come with 4 or 6 V100 GPUs which are not
> pluggable PCIe devices but still have PCIe links which are used
> for config space and MMIO. In addition to that the GPUs have 6 NVLinks
> which are connected to other GPUs and the POWER9 CPU. POWER9 chips
> have a special unit on a die called an NPU which is an NVLink2 host bus
> adapter with p2p connections to 2 to 3 GPUs, 3 or 2 NVLinks to each.
> These systems also support ATS (address translation services) which is
> a part of the NVLink2 protocol. Such GPUs also share on-board RAM
> (16GB or 32GB) to the system via the same NVLink2 so a CPU has
> cache-coherent access to a GPU RAM.
> 
> This exports GPU RAM to the userspace as a new VFIO device region. This
> preregisters the new memory as device memory as it might be used for DMA.
> This inserts pfns from the fault handler as the GPU memory is not onlined
> until the vendor driver is loaded and trained the NVLinks so doing this
> earlier causes low level errors which we fence in the firmware so
> it does not hurt the host system but still better be avoided; for the same
> reason this does not map GPU RAM into the host kernel (usual thing for
> emulated access otherwise).
> 
> This exports an ATSD (Address Translation Shootdown) register of NPU which
> allows TLB invalidations inside GPU for an operating system. The register
> conveniently occupies a single 64k page. It is also presented to
> the userspace as a new VFIO device region. One NPU has 8 ATSD registers,
> each of them can be used for TLB invalidation in a GPU linked to this NPU.
> This allocates one ATSD register per an NVLink bridge allowing passing
> up to 6 registers. Due to the host firmware bug (just recently fixed),
> only 1 ATSD register per NPU was actually advertised to the host system
> so this passes that alone register via the first NVLink bridge device in
> the group which is still enough as QEMU collects them all back and
> presents to the guest via vPHB to mimic the emulated NPU PHB on the host.
> 
> In order to provide the userspace with the information about GPU-to-NVLink
> connections, this exports an additional capability called "tgt"
> (which is an abbreviated host system bus address). The "tgt" property
> tells the GPU its own system address and allows the guest driver to
> conglomerate the routing information so each GPU knows how to get directly
> to the other GPUs.
> 
> For ATS to work, the nest MMU (an NVIDIA block in a P9 CPU) needs to
> know LPID (a logical partition ID or a KVM guest hardware ID in other
> words) and PID (a memory context ID of a userspace process, not to be
> confused with a linux pid). This assigns a GPU to LPID in the NPU and
> this is why this adds a listener for KVM on an IOMMU group. A PID comes
> via NVLink from a GPU and NPU uses a PID wildcard to pass it through.
> 
> This requires coherent memory and ATSD to be available on the host as
> the GPU vendor only supports configurations with both features enabled
> and other configurations are known not to work. Because of this and
> because of the ways the features are advertised to the host system
> (which is a device tree with very platform specific properties),
> this requires enabled POWERNV platform.
> 
> The V100 GPUs do not advertise any of these capabilities via the config
> space and there are more than just one device ID so this relies on
> the platform to tell whether these GPUs have special abilities such as
> NVLinks.
> 
> Signed-off-by: Alexey Kardashevskiy 
> ---
> Changes:
> v6.1:
> * fixed outdated comment about VFIO_REGION_INFO_CAP_NVLINK2_LNKSPD
> 
> v6:
> * reworked capabilities - tgt for nvlink and gpu and link-speed
> for nvlink only
> 
> v5:
> * do not memremap GPU RAM for emulation, map it only when it is needed
> * allocate 1 ATSD register per NVLink bridge, if none left, then expose
> the region with a zero size
> * separate caps per device type
> * addressed AW review comments
> 
> v4:
> * added nvlink-speed to the NPU bridge capability as this turned out to
> be not a constant value
> * instead of looking at the exact device ID (which also changes from system
> to system), now this (indirectly) looks at the device tree to know
> if GPU and NPU support NVLink
> 
> v3:
> * reworded the commit log about tgt
> * added tracepoints (do we want them enabled for entire vfio-pci?)
> * added code comments
> * added write|mmap flags to the new regions
> * auto enabled VFIO_PCI_NVLINK2 config option
> * added 'tgt' capability to a GPU so QEMU can recreate ibm,npu and ibm,gpu
> references; there are required by the NVIDIA driver
> * keep notifier registered only for short time
> ---
>  drivers/vfio/pci/Makefile   |   1 +
>  drivers/vfio/pci/trace.h| 102 ++
>  drivers/vfio/pci/vfio_pci_private.h |  14 +
>  include/uapi/linux/vfio.h   |  37 +++
>  drivers/vfio/pci/vfio_pci.c |  27 +-
>  

Re: [PATCH] powerpc: Don't print kernel instructions in show_user_instructions()

[Murilo Opsfelder Araujo]

On Fri, Oct 05, 2018 at 11:21:23PM +1000, Michael Ellerman wrote:
> Recently we implemented show_user_instructions() which dumps the code
> around the NIP when a user space process dies with an unhandled
> signal. This was modelled on the x86 code, and we even went so far as
> to implement the exact same bug, namely that if the user process
> crashed with its NIP pointing into the kernel we will dump kernel text
> to dmesg. eg:
>
>   bad-bctr[2996]: segfault (11) at c001 nip c001 lr 
> 12d0b0894 code 1
>   bad-bctr[2996]: code: fbe10068 7cbe2b78 7c7f1b78 fb610048 38a10028 38810020 
> fb810050 7f8802a6
>   bad-bctr[2996]: code: 3860001c f8010080 48242371 6000 <7c7b1b79> 
> 4082002c e8010080 eb610048
>
> This was discovered on x86 by Jann Horn and fixed in commit
> 342db04ae712 ("x86/dumpstack: Don't dump kernel memory based on usermode 
> RIP").
>
> Fix it by checking the adjusted NIP value (pc) and number of
> instructions against USER_DS, and bail if we fail the check, eg:
>
>   bad-bctr[2969]: segfault (11) at c001 nip c001 lr 
> 107930894 code 1
>   bad-bctr[2969]: Bad NIP, not dumping instructions.
>
> Fixes: 88b0fe175735 ("powerpc: Add show_user_instructions()")
> Signed-off-by: Michael Ellerman 

Reviewed-by: Murilo Opsfelder Araujo 

Thank you all!

> ---
>  arch/powerpc/kernel/process.c | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 913c5725cdb2..bb6ac471a784 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1306,6 +1306,16 @@ void show_user_instructions(struct pt_regs *regs)
>
>   pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>
> + /*
> +  * Make sure the NIP points at userspace, not kernel text/data or
> +  * elsewhere.
> +  */
> + if (!__access_ok(pc, instructions_to_print * sizeof(int), USER_DS)) {
> + pr_info("%s[%d]: Bad NIP, not dumping instructions.\n",
> + current->comm, current->pid);
> + return;
> + }
> +
>   pr_info("%s[%d]: code: ", current->comm, current->pid);
>
>   for (i = 0; i < instructions_to_print; i++) {
> --
> 2.17.1
>

--
Murilo

Re: [PATCH v3 2/3] powerpc/process: fix interleaved output in show_user_instructions()

[Murilo Opsfelder Araujo]

On Fri, Sep 07, 2018 at 01:47:31PM +, Christophe Leroy wrote:
> When two processes crash at the same time, we sometimes encounter
> interleaving in the middle of a line:
> 
> [4.365317] init[1]: segfault (11) at 0 nip 0 lr 0 code 1
> [4.370452] init[1]: code:    
> [4.372042] init[74]: segfault (11) at 10a74 nip 1000c198 lr 100078c8 code 
> 1 in sh[1000+14000]
> [4.386829]    
> [4.391542] init[1]: code:      
>   
> [4.400863] init[74]: code: 90010024 bf61000c 91490a7c 3fa01002 3be0 
> 7d3e4b78 3bbd0c20 3b60
> [4.409867] init[74]: code: 3b9d0040 7c7fe02e 2f83 419e0028 <8923> 
> 2f89 41be001c 4b7f6e79
> 
> This patch fixes it by preparing complete lines in a buffer and
> printing it at once.
> 
> Fixes: 88b0fe1757359 ("powerpc: Add show_user_instructions()")
> Cc: Murilo Opsfelder Araujo 
> Signed-off-by: Christophe Leroy 

Reviewed-by: Murilo Opsfelder Araujo 

> ---
>  v3: no change
>  v2: Using seq_buf and reworked the loop to avoid redundant prints.
> 
>  arch/powerpc/kernel/process.c | 37 +++--
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index e108e1ef2b85..2a39f7aca846 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -43,6 +43,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -1303,33 +1304,33 @@ static void show_instructions(struct pt_regs *regs)
>  void show_user_instructions(struct pt_regs *regs)
>  {
>   unsigned long pc;
> - int i;
> + int n = instructions_to_print;
> + struct seq_buf s;
> + char buf[96]; /* enough for 8 times 9 + 2 chars */
>  
>   pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>  
> - pr_info("%s[%d]: code: ", current->comm, current->pid);
> + seq_buf_init(, buf, sizeof(buf));
>  
> - for (i = 0; i < instructions_to_print; i++) {
> - int instr;
> + while (n) {
> + int i;
>  
> - if (!(i % 8) && (i > 0)) {
> - pr_cont("\n");
> - pr_info("%s[%d]: code: ", current->comm, current->pid);
> - }
> + seq_buf_clear();
>  
> - if (probe_kernel_address((const void *)pc, instr)) {
> - pr_cont(" ");
> - } else {
> - if (regs->nip == pc)
> - pr_cont("<%08x> ", instr);
> - else
> - pr_cont("%08x ", instr);
> + for (i = 0; i < 8 && n; i++, n--, pc += sizeof(int)) {
> + int instr;
> +
> + if (probe_kernel_address((const void *)pc, instr)) {
> + seq_buf_puts(, " ");
> + continue;
> + }
> + seq_buf_printf(, regs->nip == pc ? "<%08x> " : "%08x 
> ", instr);
>   }
>  
> - pc += sizeof(int);
> + if (!seq_buf_has_overflowed())
> + pr_info("%s[%d]: code: %s\n", current->comm,
> + current->pid, s.buffer);
>   }
> -
> - pr_cont("\n");
>  }
>  
>  struct regbit {
> -- 
> 2.13.3
> 

-- 
Murilo

Re: [PATCH v3 3/3] powerpc/process: Constify the number of insns printed by show instructions functions.

[Murilo Opsfelder Araujo]

On Fri, Sep 07, 2018 at 01:47:33PM +, Christophe Leroy wrote:
> instructions_to_print var is assigned value 16 and there is no
> way to change it.
> 
> This patch replaces it by a constant.
> 
> Signed-off-by: Christophe Leroy 

Reviewed-by: Murilo Opsfelder Araujo 

> ---
>  v3: no change
>  v2: no change
> 
>  arch/powerpc/kernel/process.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 2a39f7aca846..7d86b4f7949e 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1261,17 +1261,16 @@ struct task_struct *__switch_to(struct task_struct 
> *prev,
>   return last;
>  }
>  
> -static int instructions_to_print = 16;
> +#define NR_INSN_TO_PRINT 16
>  
>  static void show_instructions(struct pt_regs *regs)
>  {
>   int i;
> - unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
> - sizeof(int));
> + unsigned long pc = regs->nip - (NR_INSN_TO_PRINT * 3 / 4 * sizeof(int));
>  
>   printk("Instruction dump:");
>  
> - for (i = 0; i < instructions_to_print; i++) {
> + for (i = 0; i < NR_INSN_TO_PRINT; i++) {
>   int instr;
>  
>   if (!(i % 8))
> @@ -1304,11 +1303,11 @@ static void show_instructions(struct pt_regs *regs)
>  void show_user_instructions(struct pt_regs *regs)
>  {
>   unsigned long pc;
> - int n = instructions_to_print;
> + int n = NR_INSN_TO_PRINT;
>   struct seq_buf s;
>   char buf[96]; /* enough for 8 times 9 + 2 chars */
>  
> - pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
> + pc = regs->nip - (NR_INSN_TO_PRINT * 3 / 4 * sizeof(int));
>  
>   seq_buf_init(, buf, sizeof(buf));
>  
> -- 
> 2.13.3
> 

-- 
Murilo

Re: [PATCH v3 1/3] powerpc/process: fix casting and missing header

[Murilo Opsfelder Araujo]

On Fri, Sep 07, 2018 at 01:47:29PM +, Christophe Leroy wrote:
> This patch fixes the following warnings. The first ones are leftovers
> from when __get_user() was replaced by probe_kernel_address().
> 
> The last one is from when show_user_instructions() was added.
> 
> arch/powerpc/kernel/process.c:1287:22: warning: incorrect type in argument 2 
> (different address spaces)
> arch/powerpc/kernel/process.c:1287:22:expected void const *src
> arch/powerpc/kernel/process.c:1287:22:got unsigned int [noderef] 
> *
> arch/powerpc/kernel/process.c:1319:21: warning: incorrect type in argument 2 
> (different address spaces)
> arch/powerpc/kernel/process.c:1319:21:expected void const *src
> arch/powerpc/kernel/process.c:1319:21:got unsigned int [noderef] 
> *
> arch/powerpc/kernel/process.c:1302:6: warning: symbol 
> 'show_user_instructions' was not declared. Should it be static?
> 
> Fixes: 7b051f665c32d ("powerpc: Use probe_kernel_address in 
> show_instructions")
> Fixes: 88b0fe1757359 ("powerpc: Add show_user_instructions()")
> Signed-off-by: Christophe Leroy 

Smoke test passed.  Thank you.

Reviewed-by: Murilo Opsfelder Araujo 

> ---
>  v3: new in v3 to fix sparse warnings reported by snowpatch on the serie
> 
>  arch/powerpc/kernel/process.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 913c5725cdb2..e108e1ef2b85 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -65,6 +65,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -1284,7 +1285,7 @@ static void show_instructions(struct pt_regs *regs)
>  #endif
>  
>   if (!__kernel_text_address(pc) ||
> -  probe_kernel_address((unsigned int __user *)pc, instr)) {
> + probe_kernel_address((const void *)pc, instr)) {
>   pr_cont(" ");
>   } else {
>   if (regs->nip == pc)
> @@ -1316,7 +1317,7 @@ void show_user_instructions(struct pt_regs *regs)
>   pr_info("%s[%d]: code: ", current->comm, current->pid);
>   }
>  
> - if (probe_kernel_address((unsigned int __user *)pc, instr)) {
> + if (probe_kernel_address((const void *)pc, instr)) {
>   pr_cont(" ");
>   } else {
>   if (regs->nip == pc)
> -- 
> 2.13.3
> 

-- 
Murilo

Re: [PATCH] powerpc/traps: Avoid rate limit messages from show unhandled signals

[Murilo Opsfelder Araujo]

Hi, Michael.

On Fri, Aug 17, 2018 at 04:55:00PM +1000, Michael Ellerman wrote:
> In the recent commit to add an explicit ratelimit state when showing
> unhandled signals, commit 35a52a10c3ac ("powerpc/traps: Use an
> explicit ratelimit state for show_signal_msg()"), I put the check of
> show_unhandled_signals and the ratelimit state before the call to
> unhandled_signal() so as to avoid unnecessarily calling the latter
> when show_unhandled_signals is false.
> 
> However that causes us to check the ratelimit state on every call, so
> if we take a lot of *handled* signals that has the effect of making
> the ratelimit code print warnings that callbacks have been suppressed
> when they haven't.
> 
> So rearrange the code so that we check show_unhandled_signals first,
> then call unhandled_signal() and finally check the ratelimit state.
> 
> Signed-off-by: Michael Ellerman 

Nice catch.  Thanks for patching it.

Reviewed-by: Murilo Opsfelder Araujo 

> ---
>  arch/powerpc/kernel/traps.c | 13 ++---
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 070e96f1773a..c85adb858271 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -315,22 +315,21 @@ void user_single_step_siginfo(struct task_struct *tsk,
>   info->si_addr = (void __user *)regs->nip;
>  }
>  
> -static bool show_unhandled_signals_ratelimited(void)
> +static void show_signal_msg(int signr, struct pt_regs *regs, int code,
> + unsigned long addr)
>  {
>   static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
> DEFAULT_RATELIMIT_BURST);
> - return show_unhandled_signals && __ratelimit();
> -}
>  
> -static void show_signal_msg(int signr, struct pt_regs *regs, int code,
> - unsigned long addr)
> -{
> - if (!show_unhandled_signals_ratelimited())
> + if (!show_unhandled_signals)
>   return;
>  
>   if (!unhandled_signal(current, signr))
>   return;
>  
> + if (!__ratelimit())
> + return;
> +
>   pr_info("%s[%d]: %s (%d) at %lx nip %lx lr %lx code %x",
>   current->comm, current->pid, signame(signr), signr,
>   addr, regs->nip, regs->link, code);
> -- 
> 2.17.1
> 

-- 
Murilo

Re: [PATCH 2/2] powerpc/process: Constify the number of insns printed by show instructions functions.

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Tue, Aug 14, 2018 at 08:59:20AM +, Christophe Leroy wrote:
> instructions_to_print var is assigned value 16 and there is no
> way to change it.
> 
> This patch replaces it by a constant.
> 
> Signed-off-by: Christophe Leroy 

Reviewed-by: Murilo Opsfelder Araujo 

> ---
>  arch/powerpc/kernel/process.c | 11 +--
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index c722ce4ca1c0..6317f2ed04ab 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1259,17 +1259,16 @@ struct task_struct *__switch_to(struct task_struct 
> *prev,
>   return last;
>  }
>  
> -static int instructions_to_print = 16;
> +#define NR_INSN_TO_PRINT 16
>  
>  static void show_instructions(struct pt_regs *regs)
>  {
>   int i;
> - unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
> - sizeof(int));
> + unsigned long pc = regs->nip - (NR_INSN_TO_PRINT * 3 / 4 * sizeof(int));
>  
>   printk("Instruction dump:");
>  
> - for (i = 0; i < instructions_to_print; i++) {
> + for (i = 0; i < NR_INSN_TO_PRINT; i++) {
>   int instr;
>  
>   if (!(i % 8))
> @@ -1306,9 +1305,9 @@ void show_user_instructions(struct pt_regs *regs)
>   char buf[96]; /* enough for 8 times 9 + 2 chars */
>   int l = 0;
>  
> - pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
> + pc = regs->nip - (NR_INSN_TO_PRINT * 3 / 4 * sizeof(int));
>  
> - for (i = 0; i < instructions_to_print; i++) {
> + for (i = 0; i < NR_INSN_TO_PRINT; i++) {
>   int instr;
>  
>   if (!(i % 8) && (i > 0)) {
> -- 
> 2.13.3
> 

-- 
Murilo

Re: [PATCH 1/2] powerpc/process: fix nested output in show_user_instructions()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Tue, Aug 14, 2018 at 08:59:18AM +, Christophe Leroy wrote:
> When two processes crash at the same time, we sometimes encounter
> nesting in the middle of a line:
> 
> [4.365317] init[1]: segfault (11) at 0 nip 0 lr 0 code 1
> [4.370452] init[1]: code:    
> [4.372042] init[74]: segfault (11) at 10a74 nip 1000c198 lr 100078c8 code 
> 1 in sh[1000+14000]
> [4.386829]    
> [4.391542] init[1]: code:      
>   
> [4.400863] init[74]: code: 90010024 bf61000c 91490a7c 3fa01002 3be0 
> 7d3e4b78 3bbd0c20 3b60
> [4.409867] init[74]: code: 3b9d0040 7c7fe02e 2f83 419e0028 <8923> 
> 2f89 41be001c 4b7f6e79

My smoke test passed with the two patches.

Perhaps adding an output sample of how messages would look like after your patch
could be an enhancement to the commit message.

Otherwise:

Reviewed-by: Murilo Opsfelder Araujo 

> This patch fixes it by preparing complete lines in a buffer and
> printing it at once.
> 
> Fixes: 88b0fe1757359 ("powerpc: Add show_user_instructions()")
> Cc: Murilo Opsfelder Araujo 
> Signed-off-by: Christophe Leroy 
> ---
>  arch/powerpc/kernel/process.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 913c5725cdb2..c722ce4ca1c0 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1303,32 +1303,33 @@ void show_user_instructions(struct pt_regs *regs)
>  {
>   unsigned long pc;
>   int i;
> + char buf[96]; /* enough for 8 times 9 + 2 chars */
> + int l = 0;
>  
>   pc = regs->nip - (instructions_to_print * 3 / 4 * sizeof(int));
>  
> - pr_info("%s[%d]: code: ", current->comm, current->pid);
> -
>   for (i = 0; i < instructions_to_print; i++) {
>   int instr;
>  
>   if (!(i % 8) && (i > 0)) {
> - pr_cont("\n");
> - pr_info("%s[%d]: code: ", current->comm, current->pid);
> + pr_info("%s[%d]: code: %s\n", current->comm, 
> current->pid, buf);
> + l = 0;
>   }
>  
>   if (probe_kernel_address((unsigned int __user *)pc, instr)) {
> - pr_cont(" ");
> + l += sprintf(buf + l, " ");
>   } else {
>   if (regs->nip == pc)
> - pr_cont("<%08x> ", instr);
> + l += sprintf(buf + l, "<%08x> ", instr);
>   else
> - pr_cont("%08x ", instr);
> + l += sprintf(buf + l, "%08x ", instr);
>   }
>  
>   pc += sizeof(int);
>   }
>  
> - pr_cont("\n");
> + if (l)
> + pr_info("%s[%d]: code: %s\n", current->comm, current->pid, buf);
>  }
>  
>  struct regbit {
> -- 
> 2.13.3
> 

-- 
Murilo

Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Fri, Aug 10, 2018 at 11:29:43AM +0200, Christophe LEROY wrote:
>
>
> Le 03/08/2018 à 13:31, Murilo Opsfelder Araujo a écrit :
> > Hi, everyone.
> >
> > I'd like to thank you all that contributed to refining and making this
> > series better.  I did appreciate.
> >
> > Thank you!
>
> You are welcome.
>
> It seems that nested segfaults don't print very well. The code line is split
> in two, le second half being alone on one line. Any way to avoid that ?

What do you mean by "nested segfaults"?  I'd guess it's related to nested
virtualization, e.g.: host (kvm hv) -> guest1 (kvm pr) -> guest2?  So the
segfault would have been originated in guest2?

Can you provide us with more details about how to reproduce that?

Anyway, is "" so important that it needs to be printed at all?  Can't we
just discard it and not print it?

Murilo

Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Murilo Opsfelder Araujo]

Hi, everyone.

I'd like to thank you all that contributed to refining and making this
series better.  I did appreciate.

Thank you!

Cheers
Murilo

Re: [PATCH v4 5/6] powerpc: Add show_user_instructions()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Thu, Aug 02, 2018 at 07:26:20AM +0200, Christophe LEROY wrote:
>
>
> Le 01/08/2018 à 23:33, Murilo Opsfelder Araujo a écrit :
> > show_user_instructions() is a slightly modified version of
> > show_instructions() that allows userspace instruction dump.
> >
> > This will be useful within show_signal_msg() to dump userspace
> > instructions of the faulty location.
> >
> > Here is a sample of what show_user_instructions() outputs:
> >
> >pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 
> > f821ffc1 7c3f0b78 3d22fffe
> >pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <9949> 
> > 3920 7d234b78 383f0040
> >
> > The current->comm and current->pid printed can serve as a glue that
> > links the instructions dump to its originator, allowing messages to be
> > interleaved in the logs.
> >
> > Signed-off-by: Murilo Opsfelder Araujo 
> > ---
> >   arch/powerpc/include/asm/stacktrace.h | 13 +
> >   arch/powerpc/kernel/process.c | 40 +++
> >   2 files changed, 53 insertions(+)
> >   create mode 100644 arch/powerpc/include/asm/stacktrace.h
> >
> > diff --git a/arch/powerpc/include/asm/stacktrace.h 
> > b/arch/powerpc/include/asm/stacktrace.h
> > new file mode 100644
> > index ..6149b53b3bc8
> > --- /dev/null
> > +++ b/arch/powerpc/include/asm/stacktrace.h
> > @@ -0,0 +1,13 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Stack trace functions.
> > + *
> > + * Copyright 2018, Murilo Opsfelder Araujo, IBM Corporation.
> > + */
> > +
> > +#ifndef _ASM_POWERPC_STACKTRACE_H
> > +#define _ASM_POWERPC_STACKTRACE_H
> > +
> > +void show_user_instructions(struct pt_regs *regs);
> > +
> > +#endif /* _ASM_POWERPC_STACKTRACE_H */
> > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> > index e9533b4d2f08..364645ac732c 100644
> > --- a/arch/powerpc/kernel/process.c
> > +++ b/arch/powerpc/kernel/process.c
> > @@ -1299,6 +1299,46 @@ static void show_instructions(struct pt_regs *regs)
> > pr_cont("\n");
> >   }
> > +void show_user_instructions(struct pt_regs *regs)
> > +{
> > +   int i;
> > +   const char *prefix = KERN_INFO "%s[%d]: code: ";
> > +   unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
> > +   sizeof(int));
> > +
> > +   printk(prefix, current->comm, current->pid);
>
> Why not use pr_info() and remove KERN_INFO from *prefix ?

Because it doesn't compile:

  arch/powerpc/kernel/process.c:1317:10: error: expected ‘)’ before ‘prefix’
pr_info(prefix, current->comm, current->pid);
^
  ./include/linux/printk.h:288:21: note: in definition of macro ‘pr_fmt’
   #define pr_fmt(fmt) fmt
 ^

`pr_info(prefix, ...)` expands to `printk("\001" "6" prefix, ...)`,
which is an invalid string concatenation.

`pr_info("%s", ...)` expands to `printk("\001" "6" "%s", ...)`, which is
valid.

> > +
> > +   for (i = 0; i < instructions_to_print; i++) {
> > +   int instr;
> > +
> > +   if (!(i % 8) && (i > 0)) {
> > +   pr_cont("\n");
> > +   printk(prefix, current->comm, current->pid);
> > +   }
> > +
> > +#if !defined(CONFIG_BOOKE)
> > +   /* If executing with the IMMU off, adjust pc rather
> > +* than print .
> > +*/
> > +   if (!(regs->msr & MSR_IR))
> > +   pc = (unsigned long)phys_to_virt(pc);
>
> Shouldn't this be done outside of the loop, only once ?

I don't think so.

pc gets incremented at the bottom of the loop:

  pc += sizeof(int);

Adjusting pc is necessary at each iteration.  Leaving this block inside
the loop seems correct.

Cheers
Murilo

[PATCH v4 4/6] powerpc/traps: Print VMA for unhandled signals

[Murilo Opsfelder Araujo]

This adds VMA address in the message printed for unhandled signals,
similarly to what other architectures, like x86, print.

Before this patch, a page fault looked like:

  pandafault[61470]: unhandled signal 11 at 17d0 nip 161c lr 
7fff8d185100 code 2

After this patch, a page fault looks like:

  pandafault[6303]: segfault 11 at 17d0 nip 161c lr 7fff93c55100 code 2 
in pandafault[1000+1]

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 4503e22f6ba5..bcefbb1ee771 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -96,6 +96,19 @@ EXPORT_SYMBOL(__debugger_fault_handler);
 #define TM_DEBUG(x...) do { } while(0)
 #endif
 
+static const char *signame(int signr)
+{
+   switch (signr) {
+   case SIGBUS:return "bus error";
+   case SIGFPE:return "floating point exception";
+   case SIGILL:return "illegal instruction";
+   case SIGSEGV:   return "segfault";
+   case SIGTRAP:   return "unhandled trap";
+   }
+
+   return "unknown signal";
+}
+
 /*
  * Trap & Exception support
  */
@@ -317,9 +330,13 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
if (!show_unhandled_signals_ratelimited())
return;
 
-   pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x\n",
-   current->comm, current->pid, signr,
+   pr_info("%s[%d]: %s (%d) at %lx nip %lx lr %lx code %x",
+   current->comm, current->pid, signame(signr), signr,
addr, regs->nip, regs->link, code);
+
+   print_vma_addr(KERN_CONT " in ", regs->nip);
+
+   pr_cont("\n");
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v4 5/6] powerpc: Add show_user_instructions()

[Murilo Opsfelder Araujo]

show_user_instructions() is a slightly modified version of
show_instructions() that allows userspace instruction dump.

This will be useful within show_signal_msg() to dump userspace
instructions of the faulty location.

Here is a sample of what show_user_instructions() outputs:

  pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 
f821ffc1 7c3f0b78 3d22fffe
  pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <9949> 
3920 7d234b78 383f0040

The current->comm and current->pid printed can serve as a glue that
links the instructions dump to its originator, allowing messages to be
interleaved in the logs.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/include/asm/stacktrace.h | 13 +
 arch/powerpc/kernel/process.c | 40 +++
 2 files changed, 53 insertions(+)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

diff --git a/arch/powerpc/include/asm/stacktrace.h 
b/arch/powerpc/include/asm/stacktrace.h
new file mode 100644
index ..6149b53b3bc8
--- /dev/null
+++ b/arch/powerpc/include/asm/stacktrace.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Stack trace functions.
+ *
+ * Copyright 2018, Murilo Opsfelder Araujo, IBM Corporation.
+ */
+
+#ifndef _ASM_POWERPC_STACKTRACE_H
+#define _ASM_POWERPC_STACKTRACE_H
+
+void show_user_instructions(struct pt_regs *regs);
+
+#endif /* _ASM_POWERPC_STACKTRACE_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index e9533b4d2f08..364645ac732c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1299,6 +1299,46 @@ static void show_instructions(struct pt_regs *regs)
pr_cont("\n");
 }
 
+void show_user_instructions(struct pt_regs *regs)
+{
+   int i;
+   const char *prefix = KERN_INFO "%s[%d]: code: ";
+   unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
+   sizeof(int));
+
+   printk(prefix, current->comm, current->pid);
+
+   for (i = 0; i < instructions_to_print; i++) {
+   int instr;
+
+   if (!(i % 8) && (i > 0)) {
+   pr_cont("\n");
+   printk(prefix, current->comm, current->pid);
+   }
+
+#if !defined(CONFIG_BOOKE)
+   /* If executing with the IMMU off, adjust pc rather
+* than print .
+*/
+   if (!(regs->msr & MSR_IR))
+   pc = (unsigned long)phys_to_virt(pc);
+#endif
+
+   if (probe_kernel_address((unsigned int __user *)pc, instr)) {
+   pr_cont(" ");
+   } else {
+   if (regs->nip == pc)
+   pr_cont("<%08x> ", instr);
+   else
+   pr_cont("%08x ", instr);
+   }
+
+   pc += sizeof(int);
+   }
+
+   pr_cont("\n");
+}
+
 struct regbit {
unsigned long bit;
const char *name;
-- 
2.17.1

[PATCH v4 6/6] powerpc/traps: Show instructions on exceptions

[Murilo Opsfelder Araujo]

Call show_user_instructions() in arch/powerpc/kernel/traps.c to dump
instructions at faulty location, useful to debugging.

Before this patch, an unhandled signal message looked like:

  pandafault[10524]: segfault (11) at 17d0 nip 161c lr 7fffbd295100 
code 2 in pandafault[1000+1]

After this patch, it looks like:

  pandafault[10524]: segfault (11) at 17d0 nip 161c lr 7fffbd295100 
code 2 in pandafault[1000+1]
  pandafault[10524]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 
f821ffc1 7c3f0b78 3d22fffe
  pandafault[10524]: code: 392988d0 f93f0020 e93f0020 39400048 <9949> 
3920 7d234b78 383f0040

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index bcefbb1ee771..8494b0ff4904 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -70,6 +70,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC_CORE)
 int (*__debugger)(struct pt_regs *regs) __read_mostly;
@@ -337,6 +338,8 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
print_vma_addr(KERN_CONT " in ", regs->nip);
 
pr_cont("\n");
+
+   show_user_instructions(regs);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v4 3/6] powerpc/traps: Use %lx format in show_signal_msg()

[Murilo Opsfelder Araujo]

Use %lx format to print registers.  This avoids having two different
formats and avoids checking for MSR_64BIT, improving readability of the
function.

Even though we could have used %px, which is functionally equivalent to %lx
as per Documentation/core-api/printk-formats.rst, it is not semantically
correct because the data printed are not pointers.  And using %px requires
casting data to (void *).

Besides that, %lx matches the format used in show_regs().

Before this patch:

  pandafault[4808]: unhandled signal 11 at 1718 nip 
1574 lr 7fff935e7a6c code 2

After this patch:

  pandafault[4732]: unhandled signal 11 at 1718 nip 1574 lr 
7fff86697a6c code 2

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 367534b41617..4503e22f6ba5 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -311,20 +311,15 @@ static bool show_unhandled_signals_ratelimited(void)
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
unsigned long addr)
 {
-   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %08lx nip %08lx lr %08lx code %x\n";
-   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %016lx nip %016lx lr %016lx code %x\n";
-
if (!unhandled_signal(current, signr))
return;
 
if (!show_unhandled_signals_ratelimited())
return;
 
-   printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
+   pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x\n",
+   current->comm, current->pid, signr,
+   addr, regs->nip, regs->link, code);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v4 2/6] powerpc/traps: Use an explicit ratelimit state for show_signal_msg()

[Murilo Opsfelder Araujo]

Replace printk_ratelimited() by printk() and a default rate limit
burst to limit displaying unhandled signals messages.

This will allow us to call print_vma_addr() in a future patch, which
does not work with printk_ratelimited().

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index cbd3dc365193..367534b41617 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -301,6 +301,13 @@ void user_single_step_siginfo(struct task_struct *tsk,
info->si_addr = (void __user *)regs->nip;
 }
 
+static bool show_unhandled_signals_ratelimited(void)
+{
+   static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
+ DEFAULT_RATELIMIT_BURST);
+   return show_unhandled_signals && __ratelimit();
+}
+
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
unsigned long addr)
 {
@@ -309,11 +316,15 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
"at %016lx nip %016lx lr %016lx code %x\n";
 
-   if (show_unhandled_signals && unhandled_signal(current, signr)) {
-   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
-   }
+   if (!unhandled_signal(current, signr))
+   return;
+
+   if (!show_unhandled_signals_ratelimited())
+   return;
+
+   printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
+  current->comm, current->pid, signr,
+  addr, regs->nip, regs->link, code);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v4 1/6] powerpc/traps: Print unhandled signals in a separate function

[Murilo Opsfelder Araujo]

Isolate the logic of printing unhandled signals out of _exception_pkey().
No functional change, only code rearrangement.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 0e17dcb48720..cbd3dc365193 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -301,26 +301,32 @@ void user_single_step_siginfo(struct task_struct *tsk,
info->si_addr = (void __user *)regs->nip;
 }
 
+static void show_signal_msg(int signr, struct pt_regs *regs, int code,
+   unsigned long addr)
+{
+   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
+   "at %08lx nip %08lx lr %08lx code %x\n";
+   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
+   "at %016lx nip %016lx lr %016lx code %x\n";
+
+   if (show_unhandled_signals && unhandled_signal(current, signr)) {
+   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
+  current->comm, current->pid, signr,
+  addr, regs->nip, regs->link, code);
+   }
+}
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-   unsigned long addr, int key)
+unsigned long addr, int key)
 {
siginfo_t info;
-   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %08lx nip %08lx lr %08lx code %x\n";
-   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %016lx nip %016lx lr %016lx code %x\n";
 
if (!user_mode(regs)) {
die("Exception in kernel mode", regs, signr);
return;
}
 
-   if (show_unhandled_signals && unhandled_signal(current, signr)) {
-   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
-   }
+   show_signal_msg(signr, regs, code, addr);
 
if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
local_irq_enable();
-- 
2.17.1

[PATCH v4 0/6] powerpc: Modernize unhandled signals message

[Murilo Opsfelder Araujo]

Hi, everyone.

This series was inspired by the need to modernize and display more
informative messages about unhandled signals.

The "unhandled signal NN" is not very informative.  We thought it would
be helpful adding a human-readable message describing what the signal
number means, printing the VMA address, and dumping the instructions.

Before this series:

  pandafault32[4724]: unhandled signal 11 at 15e4 nip 1444 lr 0fe31ef4 
code 2

  pandafault64[4725]: unhandled signal 11 at 1718 nip 
1574 lr 7fff7faa7a6c code 2

After this series:

  pandafault32[4753]: segfault (11) at 15e4 nip 1444 lr fe31ef4 code 2 
in pandafault32[1000+1]
  pandafault32[4753]: code: 4b3c 6000 6042 4b30 9421ffd0 
93e1002c 7c3f0b78 3d201000
  pandafault32[4753]: code: 392905e4 913f0008 813f0008 39400048 <9949> 
3920 7d234b78 397f0030

  pandafault64[4754]: segfault (11) at 1718 nip 1574 lr 7fffb0007a6c 
code 2 in pandafault64[1000+1]
  pandafault64[4754]: code: e8010010 7c0803a6 4bfffef4 4bfffef0 fbe1fff8 
f821ffb1 7c3f0b78 3d22fffe
  pandafault64[4754]: code: 39298818 f93f0030 e93f0030 39400048 <9949> 
3920 7d234b78 383f0050

Link to v3:

  https://lore.kernel.org/lkml/20180731145020.14009-1-muri...@linux.ibm.com/

v3..v4:

  - Added new show_user_instructions() based on the existing
show_instructions()
  - Updated commit messages
  - Replaced signals names table with a tiny function that returns a
literal string for each signal number

Cheers!

Murilo Opsfelder Araujo (6):
  powerpc/traps: Print unhandled signals in a separate function
  powerpc/traps: Use an explicit ratelimit state for show_signal_msg()
  powerpc/traps: Use %lx format in show_signal_msg()
  powerpc/traps: Print VMA for unhandled signals
  powerpc: Add show_user_instructions()
  powerpc/traps: Show instructions on exceptions

 arch/powerpc/include/asm/stacktrace.h | 13 +++
 arch/powerpc/kernel/process.c | 40 +
 arch/powerpc/kernel/traps.c   | 52 +--
 3 files changed, 95 insertions(+), 10 deletions(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

-- 
2.17.1

Re: [PATCH v3 9/9] powerpc/traps: Add line prefix in show_instructions()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Wed, Aug 01, 2018 at 08:41:15AM +0200, Christophe LEROY wrote:
>
>
> Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
> > Remove "Instruction dump:" line by adding a prefix to display current->comm
> > and current->pid, along with the instructions dump.
> >
> > The prefix can serve as a glue that links the instructions dump to its
> > originator, allowing messages to be interleaved in the logs.
> >
> > Before this patch, a page fault looked like:
> >
> >pandafault[10524]: segfault (11) at 17d0 nip 161c lr 
> > 7fffbd295100 code 2 in pandafault[1000+1]
> >Instruction dump:
> >4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
> >392988d0 f93f0020 e93f0020 39400048 <9949> 3920 7d234b78 383f0040
> >
> > After this patch, it looks like:
> >
> >pandafault[10850]: segfault (11) at 17d0 nip 161c lr 
> > 7fff9f3e5100 code 2 in pandafault[1000+1]
> >pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 
> > f821ffc1 7c3f0b78 3d22fffe
> >pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <9949> 
> > 3920 7d234b78 383f0040
> >
> > Signed-off-by: Murilo Opsfelder Araujo 
>
> Does the script scripts/decode_stacktrace.sh also works with this format
> change ?

I've got more feedback from Michael about this.  A better approach would be
having a new show_user_instructions(), a slightly modified version of
show_instructions(), that can be called from within show_signal_msg().

Since _exception_pkey() dies if the exception is in kernel mode, we'll be
safe to call the new show_user_instructions(), without interfering in
scripts/decode_stacktrace.sh.

Cheers
Murilo

Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals

[Murilo Opsfelder Araujo]

Hi, Segher.

On Wed, Aug 01, 2018 at 02:49:03AM -0500, Segher Boessenkool wrote:
> On Wed, Aug 01, 2018 at 12:03:50AM -0700, Joe Perches wrote:
> > On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote:
> > > Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
> > > I would suggest to instead use a function like this:
> > >
> > > static const char *signame(int signr)
> > > {
> > >   if (signr == SIGBUS)
> > >   return "bus error";
> > >   if (signr == SIGFPE)
> > >   return "floating point exception";
> > >   if (signr == SIGILL)
> > >   return "illegal instruction";
> > >   if (signr == SIGILL)
> > >   return "segfault";
> > >   if (signr == SIGTRAP)
> > >   return "unhandled trap";
> > >   return "unknown signal";
> > > }
> >
> > trivia:
> >
> > Unless the if tests are ordered most to least likely,
> > perhaps it would be better to use a switch/case and
> > let the compiler decide.
>
> That would also show there are two entries for SIGILL (here and in the
> original patch), one of them very wrong.

Good catch.  I'll take care of that in my next respin.

> Check the table with psignal or something?

Nice suggestion.  Thanks!

Cheers
Murilo

Re: [PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals

[Murilo Opsfelder Araujo]

On Wed, Aug 01, 2018 at 12:03:50AM -0700, Joe Perches wrote:
> On Wed, 2018-08-01 at 08:37 +0200, Christophe LEROY wrote:
> > Le 31/07/2018 à 16:50, Murilo Opsfelder Araujo a écrit :
> > > This adds a human-readable name in the unhandled signal message.
> > > Before this patch, a page fault looked like:
> > >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:
> > >pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 
> > > 7fffb63e5100 code 2 in pandafault[13a2a+1]
> ]]
> > > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> []
> > > @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler);
> > >   #define TM_DEBUG(x...) do { } while(0)
> > >   #endif
> > >
> > > +static const char *signames[SIGRTMIN + 1] = {
> > > + "UNKNOWN",
> > > + "SIGHUP",   // 1
> > > + "SIGINT",   // 2
> []
> > I don't think is is worth having that full table when we only use a few
> > of them. (As discussed in v1 https://patchwork.ozlabs.org/patch/948802/)
> >
> > I would suggest to instead use a function like this:
> >
> > static const char *signame(int signr)
> > {
> > if (signr == SIGBUS)
> > return "bus error";
> > if (signr == SIGFPE)
> > return "floating point exception";
> > if (signr == SIGILL)
> > return "illegal instruction";
> > if (signr == SIGILL)
> > return "segfault";
> > if (signr == SIGTRAP)
> > return "unhandled trap";
> > return "unknown signal";
> > }
>
> trivia:
>
> Unless the if tests are ordered most to least likely,
> perhaps it would be better to use a switch/case and
> let the compiler decide.
>
>   switch (signr) {
>   case SIGBUS:return "bus error";
>   case SIGFPE:return "floating point exception";
>   case SIGILL:return "illegal instruction";
>   case SIGSEGV:   return "segfault";
>   case SIGTRAP:   return "unhandled trap";
>   }
>   return "unknown signal";
> }
>

Hi, Joe, Christophe.

That's a nice enhancement.  I'll do that in my next respin.

Cheers
Murilo

[PATCH v3 5/9] powerpc/traps: Print signal name for unhandled signals

[Murilo Opsfelder Araujo]

This adds a human-readable name in the unhandled signal message.

Before this patch, a page fault looked like:

  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:

  pandafault[6352]: segfault (11) at 13a2a09f8 nip 13a2a086c lr 7fffb63e5100 
code 2 in pandafault[13a2a+1]

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 39 +++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 1c4f06fca370..e71f12bca146 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler);
 #define TM_DEBUG(x...) do { } while(0)
 #endif
 
+static const char *signames[SIGRTMIN + 1] = {
+   "UNKNOWN",
+   "SIGHUP",   // 1
+   "SIGINT",   // 2
+   "SIGQUIT",  // 3
+   "SIGILL",   // 4
+   "unhandled trap",   // 5 = SIGTRAP
+   "SIGABRT",  // 6 = SIGIOT
+   "bus error",// 7 = SIGBUS
+   "floating point exception", // 8 = SIGFPE
+   "illegal instruction",  // 9 = SIGILL
+   "SIGUSR1",  // 10
+   "segfault", // 11 = SIGSEGV
+   "SIGUSR2",  // 12
+   "SIGPIPE",  // 13
+   "SIGALRM",  // 14
+   "SIGTERM",  // 15
+   "SIGSTKFLT",// 16
+   "SIGCHLD",  // 17
+   "SIGCONT",  // 18
+   "SIGSTOP",  // 19
+   "SIGTSTP",  // 20
+   "SIGTTIN",  // 21
+   "SIGTTOU",  // 22
+   "SIGURG",   // 23
+   "SIGXCPU",  // 24
+   "SIGXFSZ",  // 25
+   "SIGVTALRM",// 26
+   "SIGPROF",  // 27
+   "SIGWINCH", // 28
+   "SIGIO",// 29 = SIGPOLL = SIGLOST
+   "SIGPWR",   // 30
+   "SIGSYS",   // 31 = SIGUNUSED
+};
+
 /*
  * Trap & Exception support
  */
@@ -314,8 +349,8 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
if (!unhandled_signal(current, signr))
return;
 
-   pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x",
-   current->comm, current->pid, signr,
+   pr_info("%s[%d]: %s (%d) at %lx nip %lx lr %lx code %x",
+   current->comm, current->pid, signames[signr], signr,
addr, regs->nip, regs->link, code);
 
print_vma_addr(KERN_CONT " in ", regs->nip);
-- 
2.17.1

[PATCH v3 9/9] powerpc/traps: Add line prefix in show_instructions()

[Murilo Opsfelder Araujo]

Remove "Instruction dump:" line by adding a prefix to display current->comm
and current->pid, along with the instructions dump.

The prefix can serve as a glue that links the instructions dump to its
originator, allowing messages to be interleaved in the logs.

Before this patch, a page fault looked like:

  pandafault[10524]: segfault (11) at 17d0 nip 161c lr 7fffbd295100 
code 2 in pandafault[1000+1]
  Instruction dump:
  4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
  392988d0 f93f0020 e93f0020 39400048 <9949> 3920 7d234b78 383f0040

After this patch, it looks like:

  pandafault[10850]: segfault (11) at 17d0 nip 161c lr 7fff9f3e5100 
code 2 in pandafault[1000+1]
  pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 
f821ffc1 7c3f0b78 3d22fffe
  pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <9949> 
3920 7d234b78 383f0040

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/process.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index e78799a8855a..d12143e7d8f9 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1265,16 +1265,19 @@ static int instructions_to_print = 16;
 void show_instructions(struct pt_regs *regs)
 {
int i;
+   const char *prefix = KERN_INFO "%s[%d]: code: ";
unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
sizeof(int));
 
-   printk("Instruction dump:");
+   printk(prefix, current->comm, current->pid);
 
for (i = 0; i < instructions_to_print; i++) {
int instr;
 
-   if (!(i % 8))
+   if (!(i % 8) && (i > 0)) {
pr_cont("\n");
+   printk(prefix, current->comm, current->pid);
+   }
 
 #if !defined(CONFIG_BOOKE)
/* If executing with the IMMU off, adjust pc rather
-- 
2.17.1

[PATCH v3 8/9] powerpc/traps: Show instructions on exceptions

[Murilo Opsfelder Araujo]

Call show_instructions() in arch/powerpc/kernel/traps.c to dump
instructions at faulty location, useful to debugging.

Before this patch, an unhandled signal message looked like:

  pandafault[10524]: segfault (11) at 17d0 nip 161c lr 7fffbd295100 
code 2 in pandafault[1000+1]

After this patch, it looks like:

  pandafault[10524]: segfault (11) at 17d0 nip 161c lr 7fffbd295100 
code 2 in pandafault[1000+1]
  Instruction dump:
  4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
  392988d0 f93f0020 e93f0020 39400048 <9949> 3920 7d234b78 383f0040

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index e71f12bca146..b27f3f71d745 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -70,6 +70,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC_CORE)
 int (*__debugger)(struct pt_regs *regs) __read_mostly;
@@ -356,6 +357,8 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
print_vma_addr(KERN_CONT " in ", regs->nip);
 
pr_cont("\n");
+
+   show_instructions(regs);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v3 7/9] powerpc: Add stacktrace.h header

[Murilo Opsfelder Araujo]

Move show_instructions() declaration to
arch/powerpc/include/asm/stacktrace.h and include asm/stracktrace.h in
arch/powerpc/kernel/process.c, which contains the implementation.

This allows show_instructions() to be called on, for example,
show_signal_msg().

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/include/asm/stacktrace.h | 13 +
 arch/powerpc/kernel/process.c |  3 ++-
 2 files changed, 15 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

diff --git a/arch/powerpc/include/asm/stacktrace.h 
b/arch/powerpc/include/asm/stacktrace.h
new file mode 100644
index ..217ebc52ff97
--- /dev/null
+++ b/arch/powerpc/include/asm/stacktrace.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Stack trace functions.
+ *
+ * Copyright 2018, Murilo Opsfelder Araujo, IBM Corporation.
+ */
+
+#ifndef _ASM_POWERPC_STACKTRACE_H
+#define _ASM_POWERPC_STACKTRACE_H
+
+void show_instructions(struct pt_regs *regs);
+
+#endif /* _ASM_POWERPC_STACKTRACE_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 50094c44bf79..e78799a8855a 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -52,6 +52,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1261,7 +1262,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
 
 static int instructions_to_print = 16;
 
-static void show_instructions(struct pt_regs *regs)
+void show_instructions(struct pt_regs *regs)
 {
int i;
unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
-- 
2.17.1

[PATCH v3 6/9] powerpc: Do not call __kernel_text_address() in show_instructions()

[Murilo Opsfelder Araujo]

Modify show_instructions() not to call __kernel_text_address(), allowing
userspace instruction dump.  probe_kernel_address(), which returns -EFAULT
if something goes wrong, is still being called.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index e9533b4d2f08..50094c44bf79 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1283,8 +1283,7 @@ static void show_instructions(struct pt_regs *regs)
pc = (unsigned long)phys_to_virt(pc);
 #endif
 
-   if (!__kernel_text_address(pc) ||
-probe_kernel_address((unsigned int __user *)pc, instr)) {
+   if (probe_kernel_address((unsigned int __user *)pc, instr)) {
pr_cont(" ");
} else {
if (regs->nip == pc)
-- 
2.17.1

[PATCH v3 4/9] powerpc/traps: Print VMA for unhandled signals

[Murilo Opsfelder Araujo]

This adds VMA address in the message printed for unhandled signals,
similarly to what other architectures, like x86, print.

Before this patch, a page fault looked like:

  pandafault[61470]: unhandled signal 11 at 17d0 nip 161c lr 
7fff8d185100 code 2

After this patch, a page fault looks like:

  pandafault[6303]: unhandled signal 11 at 17d0 nip 161c lr 
7fff93c55100 code 2 in pandafault[1000+1]

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index fd4e0648a2d2..1c4f06fca370 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -314,9 +314,13 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
if (!unhandled_signal(current, signr))
return;
 
-   pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x\n",
+   pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x",
current->comm, current->pid, signr,
addr, regs->nip, regs->link, code);
+
+   print_vma_addr(KERN_CONT " in ", regs->nip);
+
+   pr_cont("\n");
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v3 3/9] powerpc/traps: Use %lx format in show_signal_msg()

[Murilo Opsfelder Araujo]

Use %lx format to print registers.  This avoids having two different
formats and avoids checking for MSR_64BIT, improving readability of the
function.

Even though we could have used %px, which is functionally equivalent to %lx
as per Documentation/core-api/printk-formats.rst, it is not semantically
correct because the data printed are not pointers.  And using %px requires
casting data to (void *).

Besides that, %lx matches the format used in show_regs().

Before this patch:

  pandafault[4808]: unhandled signal 11 at 1718 nip 
1574 lr 7fff935e7a6c code 2

After this patch:

  pandafault[4732]: unhandled signal 11 at 1718 nip 1574 lr 
7fff86697a6c code 2

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 11 +++
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 4faab4705774..fd4e0648a2d2 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -311,17 +311,12 @@ static bool show_unhandled_signals_ratelimited(void)
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
unsigned long addr)
 {
-   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %08lx nip %08lx lr %08lx code %x\n";
-   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %016lx nip %016lx lr %016lx code %x\n";
-
if (!unhandled_signal(current, signr))
return;
 
-   printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
+   pr_info("%s[%d]: unhandled signal %d at %lx nip %lx lr %lx code %x\n",
+   current->comm, current->pid, signr,
+   addr, regs->nip, regs->link, code);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v3 2/9] powerpc/traps: Return early in show_signal_msg()

[Murilo Opsfelder Araujo]

Modify the logic of show_signal_msg() to return early, if possible.
Replace printk_ratelimited() by printk() and a default rate limit burst to
limit displaying unhandled signals messages.

Mainly reason of this change is to improve readability of the function.
The conditions to display the message were coupled together in one single
`if` statement.

Splitting out the rate limit check outside show_signal_msg() makes it
easier to the caller decide if it wants to respect a printk rate limit or
not.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index cbd3dc365193..4faab4705774 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -301,6 +301,13 @@ void user_single_step_siginfo(struct task_struct *tsk,
info->si_addr = (void __user *)regs->nip;
 }
 
+static bool show_unhandled_signals_ratelimited(void)
+{
+   static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
+ DEFAULT_RATELIMIT_BURST);
+   return show_unhandled_signals && __ratelimit();
+}
+
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
unsigned long addr)
 {
@@ -309,11 +316,12 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
"at %016lx nip %016lx lr %016lx code %x\n";
 
-   if (show_unhandled_signals && unhandled_signal(current, signr)) {
-   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
-   }
+   if (!unhandled_signal(current, signr))
+   return;
+
+   printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
+  current->comm, current->pid, signr,
+  addr, regs->nip, regs->link, code);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
@@ -326,7 +334,8 @@ void _exception_pkey(int signr, struct pt_regs *regs, int 
code,
return;
}
 
-   show_signal_msg(signr, regs, code, addr);
+   if (show_unhandled_signals_ratelimited())
+   show_signal_msg(signr, regs, code, addr);
 
if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
local_irq_enable();
-- 
2.17.1

[PATCH v3 1/9] powerpc/traps: Print unhandled signals in a separate function

[Murilo Opsfelder Araujo]

Isolate the logic of printing unhandled signals out of _exception_pkey().
No functional change, only code rearrangement.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 0e17dcb48720..cbd3dc365193 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -301,26 +301,32 @@ void user_single_step_siginfo(struct task_struct *tsk,
info->si_addr = (void __user *)regs->nip;
 }
 
+static void show_signal_msg(int signr, struct pt_regs *regs, int code,
+   unsigned long addr)
+{
+   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
+   "at %08lx nip %08lx lr %08lx code %x\n";
+   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
+   "at %016lx nip %016lx lr %016lx code %x\n";
+
+   if (show_unhandled_signals && unhandled_signal(current, signr)) {
+   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
+  current->comm, current->pid, signr,
+  addr, regs->nip, regs->link, code);
+   }
+}
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-   unsigned long addr, int key)
+unsigned long addr, int key)
 {
siginfo_t info;
-   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %08lx nip %08lx lr %08lx code %x\n";
-   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %016lx nip %016lx lr %016lx code %x\n";
 
if (!user_mode(regs)) {
die("Exception in kernel mode", regs, signr);
return;
}
 
-   if (show_unhandled_signals && unhandled_signal(current, signr)) {
-   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
-   }
+   show_signal_msg(signr, regs, code, addr);
 
if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
local_irq_enable();
-- 
2.17.1

[PATCH v3 0/9] powerpc: Modernize unhandled signals message

[Murilo Opsfelder Araujo]

Hi, everyone.

This series was inspired by the need to modernize and display more
informative messages about unhandled signals.

The "unhandled signal NN" is not very informative.  We thought it would be
helpful adding a human-readable message describing what the signal number
means, printing the VMA address, and dumping the instructions.

Before this series:

  pandafault32[4724]: unhandled signal 11 at 15e4 nip 1444 lr 0fe31ef4 
code 2

  pandafault64[4725]: unhandled signal 11 at 1718 nip 
1574 lr 7fff7faa7a6c code 2

After this series:

  pandafault32[4753]: segfault (11) at 15e4 nip 1444 lr fe31ef4 code 2 
in pandafault32[1000+1]
  pandafault32[4753]: code: 4b3c 6000 6042 4b30 9421ffd0 
93e1002c 7c3f0b78 3d201000
  pandafault32[4753]: code: 392905e4 913f0008 813f0008 39400048 <9949> 
3920 7d234b78 397f0030

  pandafault64[4754]: segfault (11) at 1718 nip 1574 lr 7fffb0007a6c 
code 2 in pandafault64[1000+1]
  pandafault64[4754]: code: e8010010 7c0803a6 4bfffef4 4bfffef0 fbe1fff8 
f821ffb1 7c3f0b78 3d22fffe
  pandafault64[4754]: code: 39298818 f93f0030 e93f0030 39400048 <9949> 
3920 7d234b78 383f0050

Link to v2:

  https://lore.kernel.org/lkml/20180727145811.12334-1-muri...@linux.ibm.com/

v2..v3:

  - Dropped patch 3
  - Updated patch 4 to use %lx

Cheers!

Murilo Opsfelder Araujo (9):
  powerpc/traps: Print unhandled signals in a separate function
  powerpc/traps: Return early in show_signal_msg()
  powerpc/traps: Use %lx format in show_signal_msg()
  powerpc/traps: Print VMA for unhandled signals
  powerpc/traps: Print signal name for unhandled signals
  powerpc: Do not call __kernel_text_address() in show_instructions()
  powerpc: Add stacktrace.h header
  powerpc/traps: Show instructions on exceptions
  powerpc/traps: Add line prefix in show_instructions()

 arch/powerpc/include/asm/stacktrace.h | 13 +
 arch/powerpc/kernel/process.c | 13 +++--
 arch/powerpc/kernel/traps.c   | 72 +++
 3 files changed, 83 insertions(+), 15 deletions(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

-- 
2.17.1

Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Mon, Jul 30, 2018 at 06:30:47PM +0200, LEROY Christophe wrote:
> Murilo Opsfelder Araujo  a écrit :
>
> > Hi, Christophe.
> >
> > On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote:
> > > Murilo Opsfelder Araujo  a écrit :
> > >
> > > > Simplify the message format by using REG_FMT as the register format.  
> > > > This
> > > > avoids having two different formats and avoids checking for MSR_64BIT.
> > >
> > > Are you sure it is what we want ?
> >
> > Yes.
> >
> > > Won't it change the behaviour for a 32 bits app running on a 64bits 
> > > kernel ?
> >
> > In fact, this changes how many zeroes are prefixed when displaying the
> > registers
> > (%016lx vs. %08lx format).  For example, 32-bits userspace, 64-bits kernel:
>
> Indeed that's what I suspected. What is the real benefit of this change ?
> Why not keep the current format for 32bits userspace ? All those leading
> zeroes are pointless to me.

One of the benefits is simplifying the code by removing some checks.  Another is
deduplicating almost identical format strings in favor of a unified one.

After reading Joe's comment [1], %px seems to be the format we're looking for.
An extract from Documentation/core-api/printk-formats.rst:

  "%px is functionally equivalent to %lx (or %lu). %px is preferred because it
  is more uniquely grep'able."

So I guess we don't need to worry about the format (%016lx vs. %08lx), let's
just use %px, as per the guideline.

[1] 
https://lore.kernel.org/lkml/26f07092cdde378ebb42c1034badde1b56521c36.ca...@perches.com/

Cheers
Murilo

Re: [PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Fri, Jul 27, 2018 at 06:40:23PM +0200, LEROY Christophe wrote:
> Murilo Opsfelder Araujo  a écrit :
>
> > Simplify the message format by using REG_FMT as the register format.  This
> > avoids having two different formats and avoids checking for MSR_64BIT.
>
> Are you sure it is what we want ?

Yes.

> Won't it change the behaviour for a 32 bits app running on a 64bits kernel ?

In fact, this changes how many zeroes are prefixed when displaying the registers
(%016lx vs. %08lx format).  For example, 32-bits userspace, 64-bits kernel:

before this series:
  [66475.002900] segv[4599]: unhandled signal 11 at  nip 1420 lr 
0fe61854 code 1

after this series:
  [   73.414535] segv[3759]: segfault (11) at  nip 
1420 lr 0fe61854 code 1 in segv[1000+1]
  [   73.414641] segv[3759]: code: 4e800421 80010014 38210010 7c0803a6 4b30 
9421ffd0 93e1002c 7c3f0b78
  [   73.414665] segv[3759]: code: 3920 913f001c 813f001c 3941 
<9149> 3920 7d234b78 397f0030

Have you spotted any other behaviour change?

Cheers
Murilo

[PATCH v2 10/10] powerpc/traps: Add line prefix in show_instructions()

[Murilo Opsfelder Araujo]

Remove "Instruction dump:" line by adding a prefix to display current->comm
and current->pid, along with the instructions dump.

The prefix can serve as a glue that links the instructions dump to its
originator, allowing messages to be interleaved in the logs.

Before this patch, a page fault looked like:

pandafault[10524]: segfault (11) at 17d0 nip 161c 
lr 7fffbd295100 code 2 in pandafault[1000+1]
Instruction dump:
4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
392988d0 f93f0020 e93f0020 39400048 <9949> 3920 7d234b78 383f0040

After this patch, it looks like:

pandafault[10850]: segfault (11) at 17d0 nip 161c 
lr 7fff9f3e5100 code 2 in pandafault[1000+1]
pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 
f821ffc1 7c3f0b78 3d22fffe
pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <9949> 
3920 7d234b78 383f0040

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/process.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 709bfb524b84..25b6dfc8dd81 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1265,16 +1265,19 @@ static int instructions_to_print = 16;
 void show_instructions(struct pt_regs *regs)
 {
int i;
+   const char *prefix = KERN_INFO "%s[%d]: code: ";
unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
sizeof(int));
 
-   printk("Instruction dump:");
+   printk(prefix, current->comm, current->pid);
 
for (i = 0; i < instructions_to_print; i++) {
int instr;
 
-   if (!(i % 8))
+   if (!(i % 8) && (i > 0)) {
pr_cont("\n");
+   printk(prefix, current->comm, current->pid);
+   }
 
 #if !defined(CONFIG_BOOKE)
/* If executing with the IMMU off, adjust pc rather
-- 
2.17.1

[PATCH v2 09/10] powerpc/traps: Show instructions on exceptions

[Murilo Opsfelder Araujo]

Call show_instructions() in arch/powerpc/kernel/traps.c to dump
instructions at faulty location, useful to debugging.

Before this patch, an unhandled signal message looked like:

pandafault[10524]: segfault (11) at 17d0 nip 161c 
lr 7fffbd295100 code 2 in pandafault[1000+1]

After this patch, it looks like:

pandafault[10524]: segfault (11) at 17d0 nip 161c 
lr 7fffbd295100 code 2 in pandafault[1000+1]
Instruction dump:
4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
392988d0 f93f0020 e93f0020 39400048 <9949> 3920 7d234b78 383f0040

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index e55ee639d010..3beca17ac1b1 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -70,6 +70,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC_CORE)
 int (*__debugger)(struct pt_regs *regs) __read_mostly;
@@ -357,6 +358,8 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
print_vma_addr(KERN_CONT " in ", regs->nip);
 
pr_cont("\n");
+
+   show_instructions(regs);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v2 08/10] powerpc: Add stacktrace.h header

[Murilo Opsfelder Araujo]

Move show_instructions() declaration to
arch/powerpc/include/asm/stacktrace.h and include asm/stracktrace.h in
arch/powerpc/kernel/process.c, which contains the implementation.

This allows show_instructions() to be called on, for example,
show_signal_msg().

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/include/asm/stacktrace.h | 13 +
 arch/powerpc/kernel/process.c |  3 ++-
 2 files changed, 15 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

diff --git a/arch/powerpc/include/asm/stacktrace.h 
b/arch/powerpc/include/asm/stacktrace.h
new file mode 100644
index ..217ebc52ff97
--- /dev/null
+++ b/arch/powerpc/include/asm/stacktrace.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Stack trace functions.
+ *
+ * Copyright 2018, Murilo Opsfelder Araujo, IBM Corporation.
+ */
+
+#ifndef _ASM_POWERPC_STACKTRACE_H
+#define _ASM_POWERPC_STACKTRACE_H
+
+void show_instructions(struct pt_regs *regs);
+
+#endif /* _ASM_POWERPC_STACKTRACE_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 04960796fcce..709bfb524b84 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -52,6 +52,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1261,7 +1262,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
 
 static int instructions_to_print = 16;
 
-static void show_instructions(struct pt_regs *regs)
+void show_instructions(struct pt_regs *regs)
 {
int i;
unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
-- 
2.17.1

[PATCH v2 07/10] powerpc: Do not call __kernel_text_address() in show_instructions()

[Murilo Opsfelder Araujo]

Modify show_instructions() not to call __kernel_text_address(), allowing
userspace instruction dump.  probe_kernel_address(), which returns -EFAULT
if something goes wrong, is still being called.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/process.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 25b562c21b7b..04960796fcce 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1283,8 +1283,7 @@ static void show_instructions(struct pt_regs *regs)
pc = (unsigned long)phys_to_virt(pc);
 #endif
 
-   if (!__kernel_text_address(pc) ||
-probe_kernel_address((unsigned int __user *)pc, instr)) {
+   if (probe_kernel_address((unsigned int __user *)pc, instr)) {
pr_cont(" ");
} else {
if (regs->nip == pc)
-- 
2.17.1

[PATCH v2 04/10] powerpc/traps: Use REG_FMT in show_signal_msg()

[Murilo Opsfelder Araujo]

Simplify the message format by using REG_FMT as the register format.  This
avoids having two different formats and avoids checking for MSR_64BIT.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 4faab4705774..047d980ac776 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -311,17 +311,13 @@ static bool show_unhandled_signals_ratelimited(void)
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
unsigned long addr)
 {
-   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %08lx nip %08lx lr %08lx code %x\n";
-   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %016lx nip %016lx lr %016lx code %x\n";
-
if (!unhandled_signal(current, signr))
return;
 
-   printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
+   pr_info("%s[%d]: unhandled signal %d at "REG_FMT \
+   " nip "REG_FMT" lr "REG_FMT" code %x\n",
+   current->comm, current->pid, signr, addr,
+   regs->nip, regs->link, code);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v2 06/10] powerpc/traps: Print signal name for unhandled signals

[Murilo Opsfelder Araujo]

This adds a human-readable name in the unhandled signal message.

Before this patch, a page fault looked like:

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:

pandafault[6352]: segfault (11) at 00013a2a09f8 nip 00013a2a086c lr 
7fffb63e5100 code 2 in pandafault[13a2a+1]

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 43 +
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index e6c43ef9fb50..e55ee639d010 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler);
 #define TM_DEBUG(x...) do { } while(0)
 #endif
 
+static const char *signames[SIGRTMIN + 1] = {
+   "UNKNOWN",
+   "SIGHUP",   // 1
+   "SIGINT",   // 2
+   "SIGQUIT",  // 3
+   "SIGILL",   // 4
+   "unhandled trap",   // 5 = SIGTRAP
+   "SIGABRT",  // 6 = SIGIOT
+   "bus error",// 7 = SIGBUS
+   "floating point exception", // 8 = SIGFPE
+   "illegal instruction",  // 9 = SIGILL
+   "SIGUSR1",  // 10
+   "segfault", // 11 = SIGSEGV
+   "SIGUSR2",  // 12
+   "SIGPIPE",  // 13
+   "SIGALRM",  // 14
+   "SIGTERM",  // 15
+   "SIGSTKFLT",// 16
+   "SIGCHLD",  // 17
+   "SIGCONT",  // 18
+   "SIGSTOP",  // 19
+   "SIGTSTP",  // 20
+   "SIGTTIN",  // 21
+   "SIGTTOU",  // 22
+   "SIGURG",   // 23
+   "SIGXCPU",  // 24
+   "SIGXFSZ",  // 25
+   "SIGVTALRM",// 26
+   "SIGPROF",  // 27
+   "SIGWINCH", // 28
+   "SIGIO",// 29 = SIGPOLL = SIGLOST
+   "SIGPWR",   // 30
+   "SIGSYS",   // 31 = SIGUNUSED
+};
+
 /*
  * Trap & Exception support
  */
@@ -314,10 +349,10 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
if (!unhandled_signal(current, signr))
return;
 
-   pr_info("%s[%d]: unhandled signal %d at "REG_FMT \
-   " nip "REG_FMT" lr "REG_FMT" code %x",
-   current->comm, current->pid, signr, addr,
-   regs->nip, regs->link, code);
+   pr_info("%s[%d]: %s (%d) at "REG_FMT" nip "REG_FMT \
+   " lr "REG_FMT" code %x",
+   current->comm, current->pid, signames[signr],
+   signr, addr, regs->nip, regs->link, code);
 
print_vma_addr(KERN_CONT " in ", regs->nip);
 
-- 
2.17.1

[PATCH v2 05/10] powerpc/traps: Print VMA for unhandled signals

[Murilo Opsfelder Araujo]

This adds VMA address in the message printed for unhandled signals,
similarly to what other architectures, like x86, print.

Before this patch, a page fault looked like:

pandafault[61470]: unhandled signal 11 at 17d0 nip 
161c lr 7fff8d185100 code 2

After this patch, a page fault looks like:

pandafault[6303]: unhandled signal 11 at 17d0 nip 
161c lr 7fff93c55100 code 2 in pandafault[1000+1]

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 047d980ac776..e6c43ef9fb50 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -315,9 +315,13 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
return;
 
pr_info("%s[%d]: unhandled signal %d at "REG_FMT \
-   " nip "REG_FMT" lr "REG_FMT" code %x\n",
+   " nip "REG_FMT" lr "REG_FMT" code %x",
current->comm, current->pid, signr, addr,
regs->nip, regs->link, code);
+
+   print_vma_addr(KERN_CONT " in ", regs->nip);
+
+   pr_cont("\n");
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH v2 03/10] powerpc/reg: Add REG_FMT definition

[Murilo Opsfelder Araujo]

Make REG definition, in arch/powerpc/kernel/process.c, generic enough by
renaming it to REG_FMT and placing it in arch/powerpc/include/asm/reg.h to
be used elsewhere.

Replace occurrences of REG by REG_FMT in arch/powerpc/kernel/process.c.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/include/asm/reg.h |  6 ++
 arch/powerpc/kernel/process.c  | 22 ++
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 858aa7984ab0..d6c5c77383de 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1319,6 +1319,12 @@
 #define PVR_ARCH_207   0x0f04
 #define PVR_ARCH_300   0x0f05
 
+#ifdef CONFIG_PPC64
+#define REG_FMT"%016lx"
+#else
+#define REG_FMT"%08lx"
+#endif /* CONFIG_PPC64 */
+
 /* Macros for setting and retrieving special purpose registers */
 #ifndef __ASSEMBLY__
 #define mfmsr()({unsigned long rval; \
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index e9533b4d2f08..25b562c21b7b 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1381,11 +1381,9 @@ static void print_msr_bits(unsigned long val)
 }
 
 #ifdef CONFIG_PPC64
-#define REG"%016lx"
 #define REGS_PER_LINE  4
 #define LAST_VOLATILE  13
 #else
-#define REG"%08lx"
 #define REGS_PER_LINE  8
 #define LAST_VOLATILE  12
 #endif
@@ -1396,21 +1394,21 @@ void show_regs(struct pt_regs * regs)
 
show_regs_print_info(KERN_DEFAULT);
 
-   printk("NIP:  "REG" LR: "REG" CTR: "REG"\n",
+   printk("NIP:  "REG_FMT" LR: "REG_FMT" CTR: "REG_FMT"\n",
   regs->nip, regs->link, regs->ctr);
printk("REGS: %px TRAP: %04lx   %s  (%s)\n",
   regs, regs->trap, print_tainted(), init_utsname()->release);
-   printk("MSR:  "REG" ", regs->msr);
+   printk("MSR:  "REG_FMT" ", regs->msr);
print_msr_bits(regs->msr);
-   pr_cont("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
+   pr_cont("  CR: "REG_FMT"  XER: "REG_FMT"\n", regs->ccr, regs->xer);
trap = TRAP(regs);
if ((TRAP(regs) != 0xc00) && cpu_has_feature(CPU_FTR_CFAR))
-   pr_cont("CFAR: "REG" ", regs->orig_gpr3);
+   pr_cont("CFAR: "REG_FMT" ", regs->orig_gpr3);
if (trap == 0x200 || trap == 0x300 || trap == 0x600)
 #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
-   pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
+   pr_cont("DEAR: "REG_FMT" ESR: "REG_FMT" ", regs->dar, 
regs->dsisr);
 #else
-   pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
+   pr_cont("DAR: "REG_FMT" DSISR: "REG_FMT" ", regs->dar, 
regs->dsisr);
 #endif
 #ifdef CONFIG_PPC64
pr_cont("IRQMASK: %lx ", regs->softe);
@@ -1423,7 +1421,7 @@ void show_regs(struct pt_regs * regs)
for (i = 0;  i < 32;  i++) {
if ((i % REGS_PER_LINE) == 0)
pr_cont("\nGPR%02d: ", i);
-   pr_cont(REG " ", regs->gpr[i]);
+   pr_cont(REG_FMT " ", regs->gpr[i]);
if (i == LAST_VOLATILE && !FULL_REGS(regs))
break;
}
@@ -1433,8 +1431,8 @@ void show_regs(struct pt_regs * regs)
 * Lookup NIP late so we have the best change of getting the
 * above info out without failing
 */
-   printk("NIP ["REG"] %pS\n", regs->nip, (void *)regs->nip);
-   printk("LR ["REG"] %pS\n", regs->link, (void *)regs->link);
+   printk("NIP ["REG_FMT"] %pS\n", regs->nip, (void *)regs->nip);
+   printk("LR ["REG_FMT"] %pS\n", regs->link, (void *)regs->link);
 #endif
show_stack(current, (unsigned long *) regs->gpr[1]);
if (!user_mode(regs))
@@ -2038,7 +2036,7 @@ void show_stack(struct task_struct *tsk, unsigned long 
*stack)
newsp = stack[0];
ip = stack[STACK_FRAME_LR_SAVE];
if (!firstframe || ip != lr) {
-   printk("["REG"] ["REG"] %pS", sp, ip, (void *)ip);
+   printk("["REG_FMT"] ["REG_FMT"] %pS", sp, ip, (void 
*)ip);
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
if ((ip == rth) && curr_frame >= 0) {
pr_cont(" (%pS)",
-- 
2.17.1

[PATCH v2 02/10] powerpc/traps: Return early in show_signal_msg()

[Murilo Opsfelder Araujo]

Modify the logic of show_signal_msg() to return early, if possible.
Replace printk_ratelimited() by printk() and a default rate limit burst to
limit displaying unhandled signals messages.

Mainly reason of this change is to improve readability of the function.
The conditions to display the message were coupled together in one single
`if` statement.

Splitting out the rate limit check outside show_signal_msg() makes it
easier to the caller decide if it wants to respect a printk rate limit or
not.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index cbd3dc365193..4faab4705774 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -301,6 +301,13 @@ void user_single_step_siginfo(struct task_struct *tsk,
info->si_addr = (void __user *)regs->nip;
 }
 
+static bool show_unhandled_signals_ratelimited(void)
+{
+   static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
+ DEFAULT_RATELIMIT_BURST);
+   return show_unhandled_signals && __ratelimit();
+}
+
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
unsigned long addr)
 {
@@ -309,11 +316,12 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
"at %016lx nip %016lx lr %016lx code %x\n";
 
-   if (show_unhandled_signals && unhandled_signal(current, signr)) {
-   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
-   }
+   if (!unhandled_signal(current, signr))
+   return;
+
+   printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
+  current->comm, current->pid, signr,
+  addr, regs->nip, regs->link, code);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
@@ -326,7 +334,8 @@ void _exception_pkey(int signr, struct pt_regs *regs, int 
code,
return;
}
 
-   show_signal_msg(signr, regs, code, addr);
+   if (show_unhandled_signals_ratelimited())
+   show_signal_msg(signr, regs, code, addr);
 
if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
local_irq_enable();
-- 
2.17.1

[PATCH v2 01/10] powerpc/traps: Print unhandled signals in a separate function

[Murilo Opsfelder Araujo]

Isolate the logic of printing unhandled signals out of _exception_pkey().
No functional change, only code rearrangement.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 0e17dcb48720..cbd3dc365193 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -301,26 +301,32 @@ void user_single_step_siginfo(struct task_struct *tsk,
info->si_addr = (void __user *)regs->nip;
 }
 
+static void show_signal_msg(int signr, struct pt_regs *regs, int code,
+   unsigned long addr)
+{
+   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
+   "at %08lx nip %08lx lr %08lx code %x\n";
+   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
+   "at %016lx nip %016lx lr %016lx code %x\n";
+
+   if (show_unhandled_signals && unhandled_signal(current, signr)) {
+   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
+  current->comm, current->pid, signr,
+  addr, regs->nip, regs->link, code);
+   }
+}
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-   unsigned long addr, int key)
+unsigned long addr, int key)
 {
siginfo_t info;
-   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %08lx nip %08lx lr %08lx code %x\n";
-   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %016lx nip %016lx lr %016lx code %x\n";
 
if (!user_mode(regs)) {
die("Exception in kernel mode", regs, signr);
return;
}
 
-   if (show_unhandled_signals && unhandled_signal(current, signr)) {
-   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
-   }
+   show_signal_msg(signr, regs, code, addr);
 
if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
local_irq_enable();
-- 
2.17.1

[PATCH v2 00/10] powerpc: Modernize unhandled signals message

[Murilo Opsfelder Araujo]

Hi, everyone.

This series was inspired by the need to modernize and display more
informative messages about unhandled signals.

The "unhandled signal NN" is not very informative.  We thought it would be
helpful adding a human-readable message describing what the signal number
means, printing the VMA address, and dumping the instructions.

We can add more informative messages, like informing what each code of a
SIGSEGV signal means.  We are open to suggestions.

Before this series:

pandafault[5815]: unhandled signal 11 at 17d0 nip 
161c lr 3fff87ff5100 code 2

After this series:

pandafault[10850]: segfault (11) at 17d0 nip 161c 
lr 7fff9f3e5100 code 2 in pandafault[1000+1]
pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 
f821ffc1 7c3f0b78 3d22fffe
pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <9949> 
3920 7d234b78 383f0040

Link to v1:

https://lore.kernel.org/lkml/20180724192720.32417-1-muri...@linux.ibm.com/

v1..v2:

- Broke patch 7 down into patches 7-9
- Added proper copyright in arch/powerpc/include/asm/stacktrace.h
- show_instructions(): prefixed lines with current->comm and current->pid

Cheers!

Murilo Opsfelder Araujo (10):
  powerpc/traps: Print unhandled signals in a separate function
  powerpc/traps: Return early in show_signal_msg()
  powerpc/reg: Add REG_FMT definition
  powerpc/traps: Use REG_FMT in show_signal_msg()
  powerpc/traps: Print VMA for unhandled signals
  powerpc/traps: Print signal name for unhandled signals
  powerpc: Do not call __kernel_text_address() in show_instructions()
  powerpc: Add stacktrace.h header
  powerpc/traps: Show instructions on exceptions
  powerpc/traps: Add line prefix in show_instructions()

 arch/powerpc/include/asm/reg.h|  6 +++
 arch/powerpc/include/asm/stacktrace.h | 13 +
 arch/powerpc/kernel/process.c | 35 ++---
 arch/powerpc/kernel/traps.c   | 73 +++
 4 files changed, 100 insertions(+), 27 deletions(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

--
2.17.1

Re: [PATCH 7/7] powerpc/traps: Show instructions on exceptions

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Wed, Jul 25, 2018 at 06:01:34PM +0200, LEROY Christophe wrote:
> Murilo Opsfelder Araujo  a écrit :
> 
> > Move show_instructions() declaration to 
> > arch/powerpc/include/asm/stacktrace.h
> > and include asm/stracktrace.h in arch/powerpc/kernel/process.c, which
> > contains
> > the implementation.
> > 
> > Modify show_instructions() not to call __kernel_text_address(), allowing
> > userspace instruction dump.  probe_kernel_address(), which returns -EFAULT 
> > if
> > something goes wrong, is still being called.
> > 
> > Call show_instructions() in arch/powerpc/kernel/traps.c to dump
> > instructions at
> > faulty location, useful to debugging.
> 
> Shouldn't this part be in a second patch ?

Makes sense.  Perhaps I should split this patch in two: one to remove
__kernel_text_address() check in show_instructions(), and another to
call show_instructions() in show_signal_msg().

> Wouldn't it be better to also see regs in addition if we want to use this to
> understand what happened ?
> So you could call show_regs() instead of show_instructions() ?

I see that show_regs() prints more information and calls
show_instructions() at the end if in privileged state.

I'm not sure about which situations we might want to call show_regs() -
and display a bunch of information - or just dump instructions for some
signals.

Isn't calling show_regs() in this case considered overkill?

Cheers
Murilo

Re: [PATCH 2/7] powerpc/traps: Return early in show_signal_msg()

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Wed, Jul 25, 2018 at 05:42:28PM +0200, LEROY Christophe wrote:
> Murilo Opsfelder Araujo  a écrit :
> 
> > Modify logic of show_signal_msg() to return early, if possible.  Replace
> > printk_ratelimited() by printk() and a default rate limit burst to limit
> > displaying unhandled signals messages.
> 
> Can you explain more the benefits of this change ?

Mainly is to improve readability of the function.

The conditions to display the message were coupled together in one
single `if` statement.

Besides that, patch 5/7 adds a call to print_vma_addr(), which is not
aware of any rate limit - it simply calls printk().

So splitting out the rate limit check outside show_signal_msg() makes it
easier to the caller decide if it wants to respect a printk rate limit
or not.

Cheers
Murilo

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

[Murilo Opsfelder Araujo]

Hi, Christophe.

On Wed, Jul 25, 2018 at 05:49:27PM +0200, LEROY Christophe wrote:
> Murilo Opsfelder Araujo  a écrit :
>
> > This adds a human-readable name in the unhandled signal message.
> >
> > Before this patch, a page fault looked like:
> >
> > Jul 11 16:04:11 localhost kernel: pandafault[6303]: unhandled signal
> > 11 at 17d0 nip 161c lr 7fff93c55100 code 2
> > in pandafault[1000+1]
> >
> > After this patch, a page fault looks like:
> >
> > Jul 11 18:14:48 localhost kernel: pandafault[6352]: segfault (11) at
> > 00013a2a09f8 nip 00013a2a086c lr 7fffb63e5100 code 2 in
> > pandafault[13a2a+1]
> >
> > Signed-off-by: Murilo Opsfelder Araujo 
> > ---
> >  arch/powerpc/kernel/traps.c | 43 +
> >  1 file changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> > index e6c43ef9fb50..e55ee639d010 100644
> > --- a/arch/powerpc/kernel/traps.c
> > +++ b/arch/powerpc/kernel/traps.c
> > @@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler);
> >  #define TM_DEBUG(x...) do { } while(0)
> >  #endif
> >
> > +static const char *signames[SIGRTMIN + 1] = {
> > +   "UNKNOWN",
> > +   "SIGHUP",   // 1
> > +   "SIGINT",   // 2
> > +   "SIGQUIT",  // 3
> > +   "SIGILL",   // 4
> > +   "unhandled trap",   // 5 = SIGTRAP
> > +   "SIGABRT",  // 6 = SIGIOT
> > +   "bus error",// 7 = SIGBUS
> > +   "floating point exception", // 8 = SIGFPE
> > +   "illegal instruction",  // 9 = SIGILL
> > +   "SIGUSR1",  // 10
> > +   "segfault", // 11 = SIGSEGV
> > +   "SIGUSR2",  // 12
> > +   "SIGPIPE",  // 13
> > +   "SIGALRM",  // 14
> > +   "SIGTERM",  // 15
> > +   "SIGSTKFLT",// 16
> > +   "SIGCHLD",  // 17
> > +   "SIGCONT",  // 18
> > +   "SIGSTOP",  // 19
> > +   "SIGTSTP",  // 20
> > +   "SIGTTIN",  // 21
> > +   "SIGTTOU",  // 22
> > +   "SIGURG",   // 23
> > +   "SIGXCPU",  // 24
> > +   "SIGXFSZ",  // 25
> > +   "SIGVTALRM",// 26
> > +   "SIGPROF",  // 27
> > +   "SIGWINCH", // 28
> > +   "SIGIO",// 29 = SIGPOLL = SIGLOST
> > +   "SIGPWR",   // 30
> > +   "SIGSYS",   // 31 = SIGUNUSED
> > +};
> > +
> >  /*
> >   * Trap & Exception support
> >   */
> > @@ -314,10 +349,10 @@ static void show_signal_msg(int signr, struct
> > pt_regs *regs, int code,
> > if (!unhandled_signal(current, signr))
> > return;
> >
> > -   pr_info("%s[%d]: unhandled signal %d at "REG_FMT \
> > -   " nip "REG_FMT" lr "REG_FMT" code %x",
> > -   current->comm, current->pid, signr, addr,
> > -   regs->nip, regs->link, code);
> > +   pr_info("%s[%d]: %s (%d) at "REG_FMT" nip "REG_FMT \
> > +   " lr "REG_FMT" code %x",
> > +   current->comm, current->pid, signames[signr],
> > +   signr, addr, regs->nip, regs->link, code);
>
> Are we sure that signr is always within the limits of the table ?

Looking at the code, we only pass the following signals:

SIGBUS
SIGFPE
SIGILL
SIGSEGV
SIGTRAP

All of them are within the limits of the table.  We've added other
signals for completeness.

Cheers
Murilo

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

[Murilo Opsfelder Araujo]

Hi, Gustavo.

On Wed, Jul 25, 2018 at 12:19:00PM -0300, Gustavo Romero wrote:
> 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.

Either way works for me.

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

Sure.  I intend to wait a couple of weeks to respin the series based on
community feedback.

Cheers
Murilo

Re: [PATCH 0/7] powerpc: Modernize unhandled signals message

[Murilo Opsfelder Araujo]

Hi, Mikey.

On Wed, Jul 25, 2018 at 05:00:21PM +1000, Michael Neuling wrote:
>  On Tue, 2018-07-24 at 16:27 -0300, Murilo Opsfelder Araujo wrote:
> > Hi, everyone.
> > 
> > This series was inspired by the need to modernize and display more
> > informative messages about unhandled signals.
> > 
> > The "unhandled signal NN" is not very informative.  We thought it would
> > be helpful adding a human-readable message describing what the signal
> > number means, printing the VMA address, and dumping the instructions.
> > 
> > We can add more informative messages, like informing what each code of a
> > SIGSEGV signal means.  We are open to suggestions.
> > 
> > I have collected some early feedback from Michael Ellerman about this
> > series and would love to hear more feedback from you all.
> 
> Nice.. the instruction dump would have been very handy when debugging the PCR
> init issue I had a month or so back.
> 
> > Before this series:
> > 
> > Jul 24 13:01:07 localhost kernel: pandafault[5989]: unhandled signal 11 
> > at 17d0 nip 161c lr 3fff85a75100 code 2
> > 
> > After this series:
> > 
> > Jul 24 13:08:01 localhost kernel: pandafault[10758]: segfault (11) at 
> > 17d0 nip 161c lr 7fffabc85100 code 2 in 
> > pandafault[1000+1]
> > Jul 24 13:08:01 localhost kernel: Instruction dump:
> > Jul 24 13:08:01 localhost kernel: 4bfffeec 4bfffee8 3c401002 38427f00 
> > fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
> > Jul 24 13:08:01 localhost kernel: 392988d0 f93f0020 e93f0020 39400048 
> > <9949> 3920 7d234b78 383f0040
> 
> What happens if we get a sudden flood of these from different processes that
> overlap their output? Are we going to be able to match up the process with
> instruction dump?

As to the flood of messages, ___ratelimit() makes me think that we'll
likely see some warn messages informing how many show_signal_msg()
callbacks were suppressed, instead of interleaved messages and
instruction dumps.

As to matching process with instruction dump, I believe we'd need more
information to glue them together.

What if we modify show_instructions() to accept a string prefix to be
printed along with each line?

> Should we prefix every line with the PID to avoid this?

That's possible.  An alternative would be prefixing each line with the
process name and its PID, as in the first line.  For example:

pandafault[10758]: segfault (11) at 17d0 nip 161c 
lr 7fffabc85100 code 2 in pandafault[1000+1]
pandafault[10758]: Instruction dump:
pandafault[10758]: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 
7c3f0b78 3d22fffe
pandafault[10758]: 392988d0 f93f0020 e93f0020 39400048 <9949> 3920 
7d234b78 383f0040

The above can be interleaved with other messages and we'll still be able
to match process and its corresponding instruction dump.

Cheers
Murilo

[PATCH 7/7] powerpc/traps: Show instructions on exceptions

[Murilo Opsfelder Araujo]

Move show_instructions() declaration to arch/powerpc/include/asm/stacktrace.h
and include asm/stracktrace.h in arch/powerpc/kernel/process.c, which contains
the implementation.

Modify show_instructions() not to call __kernel_text_address(), allowing
userspace instruction dump.  probe_kernel_address(), which returns -EFAULT if
something goes wrong, is still being called.

Call show_instructions() in arch/powerpc/kernel/traps.c to dump instructions at
faulty location, useful to debugging.

Before this patch, an unhandled signal message looked like:

Jul 24 09:57:00 localhost kernel: pandafault[10524]: segfault (11) at 
17d0 nip 161c lr 7fffbd295100 code 2 in 
pandafault[1000+1]

After this patch, it looks like:

Jul 24 09:57:00 localhost kernel: pandafault[10524]: segfault (11) at 
17d0 nip 161c lr 7fffbd295100 code 2 in 
pandafault[1000+1]
Jul 24 09:57:00 localhost kernel: Instruction dump:
Jul 24 09:57:00 localhost kernel: 4bfffeec 4bfffee8 3c401002 38427f00 
fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
Jul 24 09:57:00 localhost kernel: 392988d0 f93f0020 e93f0020 39400048 
<9949> 3920 7d234b78 383f0040

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/include/asm/stacktrace.h | 7 +++
 arch/powerpc/kernel/process.c | 6 +++---
 arch/powerpc/kernel/traps.c   | 3 +++
 3 files changed, 13 insertions(+), 3 deletions(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

diff --git a/arch/powerpc/include/asm/stacktrace.h 
b/arch/powerpc/include/asm/stacktrace.h
new file mode 100644
index ..46e5ef451578
--- /dev/null
+++ b/arch/powerpc/include/asm/stacktrace.h
@@ -0,0 +1,7 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_POWERPC_STACKTRACE_H
+#define _ASM_POWERPC_STACKTRACE_H
+
+void show_instructions(struct pt_regs *regs);
+
+#endif /* _ASM_POWERPC_STACKTRACE_H */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index b1af3390249c..ee1d63e03c52 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -52,6 +52,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -1261,7 +1262,7 @@ struct task_struct *__switch_to(struct task_struct *prev,
 
 static int instructions_to_print = 16;
 
-static void show_instructions(struct pt_regs *regs)
+void show_instructions(struct pt_regs *regs)
 {
int i;
unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 *
@@ -1283,8 +1284,7 @@ static void show_instructions(struct pt_regs *regs)
pc = (unsigned long)phys_to_virt(pc);
 #endif
 
-   if (!__kernel_text_address(pc) ||
-probe_kernel_address((unsigned int __user *)pc, instr)) {
+   if (probe_kernel_address((unsigned int __user *)pc, instr)) {
pr_cont(" ");
} else {
if (regs->nip == pc)
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index e55ee639d010..3beca17ac1b1 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -70,6 +70,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #if defined(CONFIG_DEBUGGER) || defined(CONFIG_KEXEC_CORE)
 int (*__debugger)(struct pt_regs *regs) __read_mostly;
@@ -357,6 +358,8 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
print_vma_addr(KERN_CONT " in ", regs->nip);
 
pr_cont("\n");
+
+   show_instructions(regs);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

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

[Murilo Opsfelder Araujo]

This adds a human-readable name in the unhandled signal message.

Before this patch, a page fault looked like:

Jul 11 16:04:11 localhost kernel: pandafault[6303]: unhandled signal 11 at 
17d0 nip 161c lr 7fff93c55100 code 2 in 
pandafault[1000+1]

After this patch, a page fault looks like:

Jul 11 18:14:48 localhost kernel: pandafault[6352]: segfault (11) at 
00013a2a09f8 nip 00013a2a086c lr 7fffb63e5100 code 2 in 
pandafault[13a2a+1]

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 43 +
 1 file changed, 39 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index e6c43ef9fb50..e55ee639d010 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -96,6 +96,41 @@ EXPORT_SYMBOL(__debugger_fault_handler);
 #define TM_DEBUG(x...) do { } while(0)
 #endif
 
+static const char *signames[SIGRTMIN + 1] = {
+   "UNKNOWN",
+   "SIGHUP",   // 1
+   "SIGINT",   // 2
+   "SIGQUIT",  // 3
+   "SIGILL",   // 4
+   "unhandled trap",   // 5 = SIGTRAP
+   "SIGABRT",  // 6 = SIGIOT
+   "bus error",// 7 = SIGBUS
+   "floating point exception", // 8 = SIGFPE
+   "illegal instruction",  // 9 = SIGILL
+   "SIGUSR1",  // 10
+   "segfault", // 11 = SIGSEGV
+   "SIGUSR2",  // 12
+   "SIGPIPE",  // 13
+   "SIGALRM",  // 14
+   "SIGTERM",  // 15
+   "SIGSTKFLT",// 16
+   "SIGCHLD",  // 17
+   "SIGCONT",  // 18
+   "SIGSTOP",  // 19
+   "SIGTSTP",  // 20
+   "SIGTTIN",  // 21
+   "SIGTTOU",  // 22
+   "SIGURG",   // 23
+   "SIGXCPU",  // 24
+   "SIGXFSZ",  // 25
+   "SIGVTALRM",// 26
+   "SIGPROF",  // 27
+   "SIGWINCH", // 28
+   "SIGIO",// 29 = SIGPOLL = SIGLOST
+   "SIGPWR",   // 30
+   "SIGSYS",   // 31 = SIGUNUSED
+};
+
 /*
  * Trap & Exception support
  */
@@ -314,10 +349,10 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
if (!unhandled_signal(current, signr))
return;
 
-   pr_info("%s[%d]: unhandled signal %d at "REG_FMT \
-   " nip "REG_FMT" lr "REG_FMT" code %x",
-   current->comm, current->pid, signr, addr,
-   regs->nip, regs->link, code);
+   pr_info("%s[%d]: %s (%d) at "REG_FMT" nip "REG_FMT \
+   " lr "REG_FMT" code %x",
+   current->comm, current->pid, signames[signr],
+   signr, addr, regs->nip, regs->link, code);
 
print_vma_addr(KERN_CONT " in ", regs->nip);
 
-- 
2.17.1

[PATCH 5/7] powerpc/traps: Print VMA for unhandled signals

[Murilo Opsfelder Araujo]

This adds VMA address in the message printed for unhandled signals, similarly to
what other architectures, like x86, print.

Before this patch, a page fault looked like:

Jul 11 15:56:25 localhost kernel: pandafault[61470]: unhandled signal 11 at 
17d0 nip 161c lr 7fff8d185100 code 2

After this patch, a page fault looks 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]

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 047d980ac776..e6c43ef9fb50 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -315,9 +315,13 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
return;
 
pr_info("%s[%d]: unhandled signal %d at "REG_FMT \
-   " nip "REG_FMT" lr "REG_FMT" code %x\n",
+   " nip "REG_FMT" lr "REG_FMT" code %x",
current->comm, current->pid, signr, addr,
regs->nip, regs->link, code);
+
+   print_vma_addr(KERN_CONT " in ", regs->nip);
+
+   pr_cont("\n");
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH 4/7] powerpc/traps: Use REG_FMT in show_signal_msg()

[Murilo Opsfelder Araujo]

Simplify the message format by using REG_FMT as the register format.  This
avoids having two different formats and avoids checking for MSR_64BIT.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 4faab4705774..047d980ac776 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -311,17 +311,13 @@ static bool show_unhandled_signals_ratelimited(void)
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
unsigned long addr)
 {
-   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %08lx nip %08lx lr %08lx code %x\n";
-   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %016lx nip %016lx lr %016lx code %x\n";
-
if (!unhandled_signal(current, signr))
return;
 
-   printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
+   pr_info("%s[%d]: unhandled signal %d at "REG_FMT \
+   " nip "REG_FMT" lr "REG_FMT" code %x\n",
+   current->comm, current->pid, signr, addr,
+   regs->nip, regs->link, code);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-- 
2.17.1

[PATCH 3/7] powerpc/reg: Add REG_FMT definition

[Murilo Opsfelder Araujo]

Make REG definition, in arch/powerpc/kernel/process.c, generic enough by
renaming it to REG_FMT and placing it in arch/powerpc/include/asm/reg.h to be
used elsewhere.

Replace occurrences of REG by REG_FMT in arch/powerpc/kernel/process.c.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/include/asm/reg.h |  6 ++
 arch/powerpc/kernel/process.c  | 22 ++
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 858aa7984ab0..d6c5c77383de 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h
@@ -1319,6 +1319,12 @@
 #define PVR_ARCH_207   0x0f04
 #define PVR_ARCH_300   0x0f05
 
+#ifdef CONFIG_PPC64
+#define REG_FMT"%016lx"
+#else
+#define REG_FMT"%08lx"
+#endif /* CONFIG_PPC64 */
+
 /* Macros for setting and retrieving special purpose registers */
 #ifndef __ASSEMBLY__
 #define mfmsr()({unsigned long rval; \
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 27f0caee55ea..b1af3390249c 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1381,11 +1381,9 @@ static void print_msr_bits(unsigned long val)
 }
 
 #ifdef CONFIG_PPC64
-#define REG"%016lx"
 #define REGS_PER_LINE  4
 #define LAST_VOLATILE  13
 #else
-#define REG"%08lx"
 #define REGS_PER_LINE  8
 #define LAST_VOLATILE  12
 #endif
@@ -1396,21 +1394,21 @@ void show_regs(struct pt_regs * regs)
 
show_regs_print_info(KERN_DEFAULT);
 
-   printk("NIP:  "REG" LR: "REG" CTR: "REG"\n",
+   printk("NIP:  "REG_FMT" LR: "REG_FMT" CTR: "REG_FMT"\n",
   regs->nip, regs->link, regs->ctr);
printk("REGS: %px TRAP: %04lx   %s  (%s)\n",
   regs, regs->trap, print_tainted(), init_utsname()->release);
-   printk("MSR:  "REG" ", regs->msr);
+   printk("MSR:  "REG_FMT" ", regs->msr);
print_msr_bits(regs->msr);
-   pr_cont("  CR: %08lx  XER: %08lx\n", regs->ccr, regs->xer);
+   pr_cont("  CR: "REG_FMT"  XER: "REG_FMT"\n", regs->ccr, regs->xer);
trap = TRAP(regs);
if ((TRAP(regs) != 0xc00) && cpu_has_feature(CPU_FTR_CFAR))
-   pr_cont("CFAR: "REG" ", regs->orig_gpr3);
+   pr_cont("CFAR: "REG_FMT" ", regs->orig_gpr3);
if (trap == 0x200 || trap == 0x300 || trap == 0x600)
 #if defined(CONFIG_4xx) || defined(CONFIG_BOOKE)
-   pr_cont("DEAR: "REG" ESR: "REG" ", regs->dar, regs->dsisr);
+   pr_cont("DEAR: "REG_FMT" ESR: "REG_FMT" ", regs->dar, 
regs->dsisr);
 #else
-   pr_cont("DAR: "REG" DSISR: %08lx ", regs->dar, regs->dsisr);
+   pr_cont("DAR: "REG_FMT" DSISR: "REG_FMT" ", regs->dar, 
regs->dsisr);
 #endif
 #ifdef CONFIG_PPC64
pr_cont("IRQMASK: %lx ", regs->softe);
@@ -1423,7 +1421,7 @@ void show_regs(struct pt_regs * regs)
for (i = 0;  i < 32;  i++) {
if ((i % REGS_PER_LINE) == 0)
pr_cont("\nGPR%02d: ", i);
-   pr_cont(REG " ", regs->gpr[i]);
+   pr_cont(REG_FMT " ", regs->gpr[i]);
if (i == LAST_VOLATILE && !FULL_REGS(regs))
break;
}
@@ -1433,8 +1431,8 @@ void show_regs(struct pt_regs * regs)
 * Lookup NIP late so we have the best change of getting the
 * above info out without failing
 */
-   printk("NIP ["REG"] %pS\n", regs->nip, (void *)regs->nip);
-   printk("LR ["REG"] %pS\n", regs->link, (void *)regs->link);
+   printk("NIP ["REG_FMT"] %pS\n", regs->nip, (void *)regs->nip);
+   printk("LR ["REG_FMT"] %pS\n", regs->link, (void *)regs->link);
 #endif
show_stack(current, (unsigned long *) regs->gpr[1]);
if (!user_mode(regs))
@@ -2038,7 +2036,7 @@ void show_stack(struct task_struct *tsk, unsigned long 
*stack)
newsp = stack[0];
ip = stack[STACK_FRAME_LR_SAVE];
if (!firstframe || ip != lr) {
-   printk("["REG"] ["REG"] %pS", sp, ip, (void *)ip);
+   printk("["REG_FMT"] ["REG_FMT"] %pS", sp, ip, (void 
*)ip);
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
if ((ip == rth) && curr_frame >= 0) {
pr_cont(" (%pS)",
-- 
2.17.1

[PATCH 2/7] powerpc/traps: Return early in show_signal_msg()

[Murilo Opsfelder Araujo]

Modify logic of show_signal_msg() to return early, if possible.  Replace
printk_ratelimited() by printk() and a default rate limit burst to limit
displaying unhandled signals messages.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index cbd3dc365193..4faab4705774 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -301,6 +301,13 @@ void user_single_step_siginfo(struct task_struct *tsk,
info->si_addr = (void __user *)regs->nip;
 }
 
+static bool show_unhandled_signals_ratelimited(void)
+{
+   static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL,
+ DEFAULT_RATELIMIT_BURST);
+   return show_unhandled_signals && __ratelimit();
+}
+
 static void show_signal_msg(int signr, struct pt_regs *regs, int code,
unsigned long addr)
 {
@@ -309,11 +316,12 @@ static void show_signal_msg(int signr, struct pt_regs 
*regs, int code,
const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
"at %016lx nip %016lx lr %016lx code %x\n";
 
-   if (show_unhandled_signals && unhandled_signal(current, signr)) {
-   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
-   }
+   if (!unhandled_signal(current, signr))
+   return;
+
+   printk(regs->msr & MSR_64BIT ? fmt64 : fmt32,
+  current->comm, current->pid, signr,
+  addr, regs->nip, regs->link, code);
 }
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
@@ -326,7 +334,8 @@ void _exception_pkey(int signr, struct pt_regs *regs, int 
code,
return;
}
 
-   show_signal_msg(signr, regs, code, addr);
+   if (show_unhandled_signals_ratelimited())
+   show_signal_msg(signr, regs, code, addr);
 
if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
local_irq_enable();
-- 
2.17.1

[PATCH 1/7] powerpc/traps: Print unhandled signals in a separate function

[Murilo Opsfelder Araujo]

Isolate the logic of printing unhandled signals out of _exception_pkey().  No
functional change, only code rearrangement.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/traps.c | 26 --
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 0e17dcb48720..cbd3dc365193 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -301,26 +301,32 @@ void user_single_step_siginfo(struct task_struct *tsk,
info->si_addr = (void __user *)regs->nip;
 }
 
+static void show_signal_msg(int signr, struct pt_regs *regs, int code,
+   unsigned long addr)
+{
+   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
+   "at %08lx nip %08lx lr %08lx code %x\n";
+   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
+   "at %016lx nip %016lx lr %016lx code %x\n";
+
+   if (show_unhandled_signals && unhandled_signal(current, signr)) {
+   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
+  current->comm, current->pid, signr,
+  addr, regs->nip, regs->link, code);
+   }
+}
 
 void _exception_pkey(int signr, struct pt_regs *regs, int code,
-   unsigned long addr, int key)
+unsigned long addr, int key)
 {
siginfo_t info;
-   const char fmt32[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %08lx nip %08lx lr %08lx code %x\n";
-   const char fmt64[] = KERN_INFO "%s[%d]: unhandled signal %d " \
-   "at %016lx nip %016lx lr %016lx code %x\n";
 
if (!user_mode(regs)) {
die("Exception in kernel mode", regs, signr);
return;
}
 
-   if (show_unhandled_signals && unhandled_signal(current, signr)) {
-   printk_ratelimited(regs->msr & MSR_64BIT ? fmt64 : fmt32,
-  current->comm, current->pid, signr,
-  addr, regs->nip, regs->link, code);
-   }
+   show_signal_msg(signr, regs, code, addr);
 
if (arch_irqs_disabled() && !arch_irq_disabled_regs(regs))
local_irq_enable();
-- 
2.17.1

[PATCH 0/7] powerpc: Modernize unhandled signals message

[Murilo Opsfelder Araujo]

Hi, everyone.

This series was inspired by the need to modernize and display more
informative messages about unhandled signals.

The "unhandled signal NN" is not very informative.  We thought it would
be helpful adding a human-readable message describing what the signal
number means, printing the VMA address, and dumping the instructions.

We can add more informative messages, like informing what each code of a
SIGSEGV signal means.  We are open to suggestions.

I have collected some early feedback from Michael Ellerman about this
series and would love to hear more feedback from you all.

Before this series:

Jul 24 13:01:07 localhost kernel: pandafault[5989]: unhandled signal 11 at 
17d0 nip 161c lr 3fff85a75100 code 2

After this series:

Jul 24 13:08:01 localhost kernel: pandafault[10758]: segfault (11) at 
17d0 nip 161c lr 7fffabc85100 code 2 in 
pandafault[1000+1]
Jul 24 13:08:01 localhost kernel: Instruction dump:
Jul 24 13:08:01 localhost kernel: 4bfffeec 4bfffee8 3c401002 38427f00 
fbe1fff8 f821ffc1 7c3f0b78 3d22fffe
Jul 24 13:08:01 localhost kernel: 392988d0 f93f0020 e93f0020 39400048 
<9949> 3920 7d234b78 383f0040

Cheers
Murilo

Murilo Opsfelder Araujo (7):
  powerpc/traps: Print unhandled signals in a separate function
  powerpc/traps: Return early in show_signal_msg()
  powerpc/reg: Add REG_FMT definition
  powerpc/traps: Use REG_FMT in show_signal_msg()
  powerpc/traps: Print VMA for unhandled signals
  powerpc/traps: Print signal name for unhandled signals
  powerpc/traps: Show instructions on exceptions

 arch/powerpc/include/asm/reg.h|  6 +++
 arch/powerpc/include/asm/stacktrace.h |  7 +++
 arch/powerpc/kernel/process.c | 28 +-
 arch/powerpc/kernel/traps.c   | 73 +++
 4 files changed, 89 insertions(+), 25 deletions(-)
 create mode 100644 arch/powerpc/include/asm/stacktrace.h

-- 
2.17.1

Re: [PATCH] powerpc/prom_init: remove linux,stdout-package property

[Murilo Opsfelder Araujo]

On Wed, Jul 18, 2018 at 07:37:37PM +1000, Michael Ellerman wrote:
> Hi Murilo,
> 
> Thanks for the patch.
> 
> Murilo Opsfelder Araujo  writes:
> > This property was added in 2004 by
> >
> > 
> > https://github.com/mpe/linux-fullhistory/commit/689fe5072fe9a0dec914bfa4fa60aed1e54563e6
> >
> > and the only use of it, which was already inside `#if 0`, was removed a 
> > month
> > later by
> >
> > 
> > https://github.com/mpe/linux-fullhistory/commit/1fbe5a6d90f6cd4ea610737ef488719d1a875de7
> >
> > Fixes: https://github.com/linuxppc/linux/issues/125
> 
> That is going to confuse some scripts that are expecting that to be a
> "Fixes: " tag :)
> 
> The proper tag to use there would be "Link:".
> 
> But, I'd prefer we didn't add github issue links to the history, as I'm
> not sure they won't bit-rot eventually. Not because I'm a anti-Microsoft
> conspiracy person but just because it's a repo I set up and manage and
> there's no long term plan for it necessarily.
> 
> > ---
> >  arch/powerpc/kernel/prom_init.c | 2 --
> >  1 file changed, 2 deletions(-)
> 
> Including the link here would be ideal, as it means it doesn't end up in
> the commit history, but it does end up in the mail archive. So if we
> ever really need to find it, it should be there.
> 
> cheers

Hi, Michael.

Thanks for reviewing it.  I've sent v2 with your suggestions:

https://lkml.kernel.org/r/20180718161544.12134-1-muri...@linux.ibm.com

Cheers

--
Murilo

[PATCH v2] powerpc/prom_init: remove linux,stdout-package property

[Murilo Opsfelder Araujo]

This property was added in 2004 and the only use of it, which was already inside
`#if 0`, was removed a month later.

Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/prom_init.c | 2 --
 1 file changed, 2 deletions(-)

v1..v2:
- Move github.com links to this section so they can still end up in the mail
  archive.
- Remove Fixes: tag from commit message.
- Add Link: tag here.
- Link to v1: 
https://lkml.kernel.org/r/20180712171904.18971-1-muri...@linux.ibm.com

This property was added in 2004 by


https://github.com/mpe/linux-fullhistory/commit/689fe5072fe9a0dec914bfa4fa60aed1e54563e6

and the only use of it, which was already inside `#if 0`, was removed a month
later by


https://github.com/mpe/linux-fullhistory/commit/1fbe5a6d90f6cd4ea610737ef488719d1a875de7

Link: https://github.com/linuxppc/linux/issues/125

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 5425dd3d6a9f..c45fb463c9e5 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -2102,8 +2102,6 @@ static void __init prom_init_stdout(void)
stdout_node = call_prom("instance-to-package", 1, 1, prom.stdout);
if (stdout_node != PROM_ERROR) {
val = cpu_to_be32(stdout_node);
-   prom_setprop(prom.chosen, "/chosen", "linux,stdout-package",
-, sizeof(val));
 
/* If it's a display, note it */
memset(type, 0, sizeof(type));
-- 
2.17.1

[PATCH] powerpc/prom_init: remove linux,stdout-package property

[Murilo Opsfelder Araujo]

This property was added in 2004 by


https://github.com/mpe/linux-fullhistory/commit/689fe5072fe9a0dec914bfa4fa60aed1e54563e6

and the only use of it, which was already inside `#if 0`, was removed a month
later by


https://github.com/mpe/linux-fullhistory/commit/1fbe5a6d90f6cd4ea610737ef488719d1a875de7

Fixes: https://github.com/linuxppc/linux/issues/125
Signed-off-by: Murilo Opsfelder Araujo 
---
 arch/powerpc/kernel/prom_init.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 5425dd3d6a9f..c45fb463c9e5 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -2102,8 +2102,6 @@ static void __init prom_init_stdout(void)
stdout_node = call_prom("instance-to-package", 1, 1, prom.stdout);
if (stdout_node != PROM_ERROR) {
val = cpu_to_be32(stdout_node);
-   prom_setprop(prom.chosen, "/chosen", "linux,stdout-package",
-, sizeof(val));
 
/* If it's a display, note it */
memset(type, 0, sizeof(type));
-- 
2.17.1

Re: [PATCH v3 1/2] powerpc: Detect the presence of big-cores via "ibm, thread-groups"

[Murilo Opsfelder Araujo]

On Fri, Jul 06, 2018 at 02:35:48PM +0530, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" 
> 
> On IBM POWER9, the device tree exposes a property array identifed by
> "ibm,thread-groups" which will indicate which groups of threads share a
> particular set of resources.
> 
> As of today we only have one form of grouping identifying the group of
> threads in the core that share the L1 cache, translation cache and
> instruction data flow.
> 
> This patch defines the helper function to parse the contents of
> "ibm,thread-groups" and a new structure to contain the parsed output.
> 
> The patch also creates the sysfs file named "small_core_siblings" that
> returns the physical ids of the threads in the core that share the L1
> cache, translation cache and instruction data flow.
> 
> Signed-off-by: Gautham R. Shenoy 
> ---
>  Documentation/ABI/testing/sysfs-devices-system-cpu |   8 ++
>  arch/powerpc/include/asm/cputhreads.h  |  22 +++
>  arch/powerpc/kernel/setup-common.c | 154 
> +
>  arch/powerpc/kernel/sysfs.c|  35 +
>  4 files changed, 219 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu 
> b/Documentation/ABI/testing/sysfs-devices-system-cpu
> index 9c5e7732..62f24de 100644
> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
> @@ -487,3 +487,11 @@ Description: Information about CPU vulnerabilities
>   "Not affected"CPU is not affected by the vulnerability
>   "Vulnerable"  CPU is affected and no mitigation in effect
>   "Mitigation: $M"  CPU is affected and mitigation $M is in effect
> +
> +What:/sys/devices/system/cpu/cpu[0-9]+/small_core_siblings
> +Date:05-Jul-2018
> +KernelVersion:   v4.18.0
> +Contact: Gautham R. Shenoy 
> +Description: List of Physical ids of CPUs which share the the L1 cache,
> + translation cache and instruction data-flow with this CPU.

What about this?

Description: List of physical CPU IDs that share a common L1 cache,
 translation cache and instruction data flow with this CPU.

Or perhaps just remove the extra "the".

> +Values:  Comma separated list of decimal integers.
> diff --git a/arch/powerpc/include/asm/cputhreads.h 
> b/arch/powerpc/include/asm/cputhreads.h
> index d71a909..33226d7 100644
> --- a/arch/powerpc/include/asm/cputhreads.h
> +++ b/arch/powerpc/include/asm/cputhreads.h
> @@ -23,11 +23,13 @@
>  extern int threads_per_core;
>  extern int threads_per_subcore;
>  extern int threads_shift;
> +extern bool has_big_cores;
>  extern cpumask_t threads_core_mask;
>  #else
>  #define threads_per_core 1
>  #define threads_per_subcore  1
>  #define threads_shift0
> +#define has_big_cores0
>  #define threads_core_mask(*get_cpu_mask(0))
>  #endif
>  
> @@ -69,12 +71,32 @@ static inline cpumask_t cpu_online_cores_map(void)
>   return cpu_thread_mask_to_cores(cpu_online_mask);
>  }
>  
> +#define MAX_THREAD_LIST_SIZE 8
> +struct thread_groups {
> + unsigned int property;
> + unsigned int nr_groups;
> + unsigned int threads_per_group;
> + unsigned int thread_list[MAX_THREAD_LIST_SIZE];
> +};
> +
>  #ifdef CONFIG_SMP
>  int cpu_core_index_of_thread(int cpu);
>  int cpu_first_thread_of_core(int core);
> +int parse_thread_groups(struct device_node *dn, struct thread_groups *tg);
> +int get_cpu_thread_group_start(int cpu, struct thread_groups *tg);
>  #else
>  static inline int cpu_core_index_of_thread(int cpu) { return cpu; }
>  static inline int cpu_first_thread_of_core(int core) { return core; }
> +static inline int parse_thread_groups(struct device_node *dn,
> +   struct thread_groups *tg)
> +{
> + return -ENODATA;
> +}
> +
> +static inline int get_cpu_thread_group_start(int cpu, struct thread_groups 
> *tg)
> +{
> + return -1;
> +}
>  #endif
>  
>  static inline int cpu_thread_in_core(int cpu)
> diff --git a/arch/powerpc/kernel/setup-common.c 
> b/arch/powerpc/kernel/setup-common.c
> index 40b44bb..989edc1 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -402,10 +402,12 @@ void __init check_for_initrd(void)
>  #ifdef CONFIG_SMP
>  
>  int threads_per_core, threads_per_subcore, threads_shift;
> +bool has_big_cores;
>  cpumask_t threads_core_mask;
>  EXPORT_SYMBOL_GPL(threads_per_core);
>  EXPORT_SYMBOL_GPL(threads_per_subcore);
>  EXPORT_SYMBOL_GPL(threads_shift);
> +EXPORT_SYMBOL_GPL(has_big_cores);
>  EXPORT_SYMBOL_GPL(threads_core_mask);
>  
>  static void __init cpu_init_thread_core_maps(int tpc)
> @@ -433,6 +435,152 @@ static void __init cpu_init_thread_core_maps(int tpc)
>  
>  u32 *cpu_to_phys_id = NULL;
>  
> +/*
> + * parse_thread_groups: Parses the "ibm,thread-groups" device tree
> + *  property 

Re: [v2 PATCH 2/2] powerpc: Enable CPU_FTR_ASYM_SMT for interleaved big-cores

[Murilo Opsfelder Araujo]

On Wed, Jul 04, 2018 at 01:45:05PM +0530, Gautham R Shenoy wrote:
> Hi Murilo,
> 
> Thanks for the review.
> 
> On Tue, Jul 03, 2018 at 02:53:46PM -0300, Murilo Opsfelder Araujo wrote:
> [..snip..]
> 
> > > -/* Initialize CPU <=> thread mapping/
> > > + if (has_interleaved_big_core) {
> > > + int key = __builtin_ctzl(CPU_FTR_ASYM_SMT);
> > > +
> > > + cur_cpu_spec->cpu_features |= CPU_FTR_ASYM_SMT;
> > > + static_branch_enable(_feature_keys[key]);
> > > + pr_info("Detected interleaved big-cores\n");
> > > + }
> > 
> > Shouldn't we use cpu_has_feature(CPU_FTR_ASYM_SMT) before setting
> > > it?
> 
> 
> Are you suggesting that we do the following?
> 
> if (has_interleaved_big_core &&
> !cpu_has_feature(CPU_FTR_ASYM_SMT)) {
>   ...
> }
> 
> Currently CPU_FTR_ASYM_SMT is set at compile time for only POWER7
> where running the tasks on lower numbered threads give us the benefit
> of SMT thread folding. Interleaved big core is a feature introduced
> only on POWER9. Thus, we know that CPU_FTR_ASYM_SMT is not set in
> cpu_features at this point.

Since we're setting CPU_FTR_ASYM_SMT, it doesn't make sense to use
cpu_has_feature(CPU_FTR_ASYM_SMT).  I thought cpu_has_feature() held all
available features (not necessarily enabled) that we could check before
setting or enabling such feature.  I think I misread it.  Sorry.

> 
> > 
> > > +
> > > + /* Initialize CPU <=> thread mapping/
> > >*
> > >* WARNING: We assume that the number of threads is the same for
> > >* every CPU in the system. If that is not the case, then some code
> > > -- 
> > > 1.9.4
> > > 
> > 
> > -- 
> > Murilo
> 
> --
> Thanks and Regards
> gautham.
> 

-- 
Murilo

Re: [v2 PATCH 2/2] powerpc: Enable CPU_FTR_ASYM_SMT for interleaved big-cores

[Murilo Opsfelder Araujo]

On Tue, Jul 03, 2018 at 04:33:51PM +0530, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" 
> 
> A pair of IBM POWER9 SMT4 cores can be fused together to form a big-core
> with 8 SMT threads. This can be discovered via the "ibm,thread-groups"
> CPU property in the device tree which will indicate which group of
> threads that share the L1 cache, translation cache and instruction data
> flow. If there are multiple such group of threads, then the core is a
> big-core.
> 
> Furthermore, if the thread-ids of the threads of the big-core can be
> obtained by interleaving the thread-ids of the thread-groups
> (component small core), then such a big-core is called an interleaved
> big-core.
> 
> Eg: Threads in the pair of component SMT4 cores of an interleaved
> big-core are numbered {0,2,4,6} and {1,3,5,7} respectively.
> 
> The SMT4 cores forming a big-core are more or less independent
> units. Thus when multiple tasks are scheduled to run on the fused
> core, we get the best performance when the tasks are spread across the
> pair of SMT4 cores.
> 
> This patch enables CPU_FTR_ASYM_SMT bit in the cpu-features on
> detecting the presence of interleaved big-cores at boot up. This will
> will bias the load-balancing of tasks on smaller numbered threads,
> which will automatically result in spreading the tasks uniformly
> across the associated pair of SMT4 cores.
> 
> Signed-off-by: Gautham R. Shenoy 
> ---
>  arch/powerpc/kernel/setup-common.c | 67 
> +-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/setup-common.c 
> b/arch/powerpc/kernel/setup-common.c
> index a78ec66..f63d797 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -537,6 +537,56 @@ int get_cpu_thread_group_start(int cpu, struct 
> thread_groups *tg)
>   return -1;
>  }
>  
> +/*
> + * check_interleaved_big_core - Checks if the thread group tg
> + * corresponds to a big-core whose threads are interleavings of the
> + * threads of the component small cores.
> + *
> + * @tg: A thread-group struct for the core.
> + *
> + * Returns true if the core is a interleaved big-core.
> + * Returns false otherwise.
> + */
> +static inline bool check_interleaved_big_core(struct thread_groups *tg)
> +{
> + int nr_groups;
> + int threads_per_group;
> + int cur_cpu, next_cpu, i, j;
> +
> + nr_groups = tg->nr_groups;
> + threads_per_group = tg->threads_per_group;
> +
> + if (tg->property != 1)
> + return false;
> +
> + if (nr_groups < 2 || threads_per_group < 2)
> + return false;
> +
> + /*
> +  * In case of an interleaved big-core, the thread-ids of the
> +  * big-core can be obtained by interleaving the the thread-ids
> +  * of the component small
> +  *
> +  * Eg: On a 8-thread big-core with two SMT4 small cores, the
> +  * threads of the two component small cores will be
> +  * {0, 2, 4, 6} and {1, 3, 5, 7}.
> +  */
> + for (i = 0; i < nr_groups; i++) {
> + int group_start = i * threads_per_group;
> +
> + for (j = 0; j < threads_per_group - 1; j++) {
> + int cur_idx = group_start + j;
> +
> + cur_cpu = tg->thread_list[cur_idx];
> + next_cpu = tg->thread_list[cur_idx + 1];
> + if (next_cpu != cur_cpu + nr_groups)
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
>  /**
>   * setup_cpu_maps - initialize the following cpu maps:
>   *  cpu_possible_mask
> @@ -560,6 +610,7 @@ void __init smp_setup_cpu_maps(void)
>   struct device_node *dn;
>   int cpu = 0;
>   int nthreads = 1;
> + bool has_interleaved_big_core = true;
>  
>   DBG("smp_setup_cpu_maps()\n");
>  
> @@ -613,6 +664,12 @@ void __init smp_setup_cpu_maps(void)
>   if (parse_thread_groups(dn, ) ||
>   tg.nr_groups < 1 || tg.property != 1) {
>   has_big_cores = false;
> + has_interleaved_big_core = false;
> + }
> +
> + if (has_interleaved_big_core) {
> + has_interleaved_big_core =
> + check_interleaved_big_core();
>   }
>  
>   if (cpu >= nr_cpu_ids) {
> @@ -669,7 +726,15 @@ void __init smp_setup_cpu_maps(void)
>   vdso_data->processorCount = num_present_cpus();
>  #endif /* CONFIG_PPC64 */
>  
> -/* Initialize CPU <=> thread mapping/
> + if (has_interleaved_big_core) {
> + int key = __builtin_ctzl(CPU_FTR_ASYM_SMT);
> +
> + cur_cpu_spec->cpu_features |= CPU_FTR_ASYM_SMT;
> + static_branch_enable(_feature_keys[key]);
> + pr_info("Detected interleaved big-cores\n");
> + }

Shouldn't we use cpu_has_feature(CPU_FTR_ASYM_SMT) before setting it?

> +
> + /* Initialize CPU 

Re: [v2 PATCH 1/2] powerpc: Detect the presence of big-cores via "ibm, thread-groups"

[Murilo Opsfelder Araujo]

On Tue, Jul 03, 2018 at 04:33:50PM +0530, Gautham R. Shenoy wrote:
> From: "Gautham R. Shenoy" 
> 
> On IBM POWER9, the device tree exposes a property array identifed by
> "ibm,thread-groups" which will indicate which groups of threads share a
> particular set of resources.
> 
> As of today we only have one form of grouping identifying the group of
> threads in the core that share the L1 cache, translation cache and
> instruction data flow.
> 
> This patch defines the helper function to parse the contents of
> "ibm,thread-groups" and a new structure to contain the parsed output.
> 
> The patch also creates the sysfs file named "small_core_siblings" that
> returns the physical ids of the threads in the core that share the L1
> cache, translation cache and instruction data flow.
> 
> Signed-off-by: Gautham R. Shenoy 
> ---
>  Documentation/ABI/testing/sysfs-devices-system-cpu |   8 ++
>  arch/powerpc/include/asm/cputhreads.h  |  22 +
>  arch/powerpc/kernel/setup-common.c | 110 
> +
>  arch/powerpc/kernel/sysfs.c|  35 +++
>  4 files changed, 175 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu 
> b/Documentation/ABI/testing/sysfs-devices-system-cpu
> index 9c5e7732..53a823a 100644
> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
> @@ -487,3 +487,11 @@ Description: Information about CPU vulnerabilities
>   "Not affected"CPU is not affected by the vulnerability
>   "Vulnerable"  CPU is affected and no mitigation in effect
>   "Mitigation: $M"  CPU is affected and mitigation $M is in effect
> +
> +What:/sys/devices/system/cpu/cpu[0-9]+/small_core_sibings

s/small_core_sibings/small_core_siblings

By the way, big_core_siblings was mentioned in the introductory email.

> +Date:03-Jul-2018
> +KernelVersion:   v4.18.0
> +Contact: Gautham R. Shenoy 
> +Description: List of Physical ids of CPUs which share the the L1 cache,
> + translation cache and instruction data-flow with this CPU.
> +Values:  Comma separated list of decimal integers.
> diff --git a/arch/powerpc/include/asm/cputhreads.h 
> b/arch/powerpc/include/asm/cputhreads.h
> index d71a909..33226d7 100644
> --- a/arch/powerpc/include/asm/cputhreads.h
> +++ b/arch/powerpc/include/asm/cputhreads.h
> @@ -23,11 +23,13 @@
>  extern int threads_per_core;
>  extern int threads_per_subcore;
>  extern int threads_shift;
> +extern bool has_big_cores;
>  extern cpumask_t threads_core_mask;
>  #else
>  #define threads_per_core 1
>  #define threads_per_subcore  1
>  #define threads_shift0
> +#define has_big_cores0
>  #define threads_core_mask(*get_cpu_mask(0))
>  #endif
>  
> @@ -69,12 +71,32 @@ static inline cpumask_t cpu_online_cores_map(void)
>   return cpu_thread_mask_to_cores(cpu_online_mask);
>  }
>  
> +#define MAX_THREAD_LIST_SIZE 8
> +struct thread_groups {
> + unsigned int property;
> + unsigned int nr_groups;
> + unsigned int threads_per_group;
> + unsigned int thread_list[MAX_THREAD_LIST_SIZE];
> +};
> +
>  #ifdef CONFIG_SMP
>  int cpu_core_index_of_thread(int cpu);
>  int cpu_first_thread_of_core(int core);
> +int parse_thread_groups(struct device_node *dn, struct thread_groups *tg);
> +int get_cpu_thread_group_start(int cpu, struct thread_groups *tg);
>  #else
>  static inline int cpu_core_index_of_thread(int cpu) { return cpu; }
>  static inline int cpu_first_thread_of_core(int core) { return core; }
> +static inline int parse_thread_groups(struct device_node *dn,
> +   struct thread_groups *tg)
> +{
> + return -ENODATA;
> +}
> +
> +static inline int get_cpu_thread_group_start(int cpu, struct thread_groups 
> *tg)
> +{
> + return -1;
> +}
>  #endif
>  
>  static inline int cpu_thread_in_core(int cpu)
> diff --git a/arch/powerpc/kernel/setup-common.c 
> b/arch/powerpc/kernel/setup-common.c
> index 40b44bb..a78ec66 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -402,10 +402,12 @@ void __init check_for_initrd(void)
>  #ifdef CONFIG_SMP
>  
>  int threads_per_core, threads_per_subcore, threads_shift;
> +bool has_big_cores = true;
>  cpumask_t threads_core_mask;
>  EXPORT_SYMBOL_GPL(threads_per_core);
>  EXPORT_SYMBOL_GPL(threads_per_subcore);
>  EXPORT_SYMBOL_GPL(threads_shift);
> +EXPORT_SYMBOL_GPL(has_big_cores);
>  EXPORT_SYMBOL_GPL(threads_core_mask);
>  
>  static void __init cpu_init_thread_core_maps(int tpc)
> @@ -433,6 +435,108 @@ static void __init cpu_init_thread_core_maps(int tpc)
>  
>  u32 *cpu_to_phys_id = NULL;
>  
> +/*
> + * parse_thread_groups: Parses the "ibm,thread-groups" device tree
> + *  property for the CPU device node dn and stores
> + *  the parsed output 

Re: [PATCH v3 1/5] rfi-flush: Move the logic to avoid a redo into the debugfs code

[Murilo Opsfelder Araujo]

On 03/14/2018 07:40 PM, Mauricio Faria de Oliveira wrote:
> From: Michael Ellerman 
> 
> rfi_flush_enable() includes a check to see if we're already
> enabled (or disabled), and in that case does nothing.
> 
> But that means calling setup_rfi_flush() a 2nd time doesn't actually
> work, which is a bit confusing.
> 
> Move that check into the debugfs code, where it really belongs.
> 
> Signed-off-by: Michael Ellerman 
> Signed-off-by: Mauricio Faria de Oliveira 
> ---
>  arch/powerpc/kernel/setup_64.c | 13 -
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index c388cc3..3efc01a 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -846,9 +846,6 @@ static void do_nothing(void *unused)
> 
>  void rfi_flush_enable(bool enable)
>  {
> - if (rfi_flush == enable)
> - return;
> -
>   if (enable) {
>   do_rfi_flush_fixups(enabled_flush_types);
>   on_each_cpu(do_nothing, NULL, 1);
> @@ -902,13 +899,19 @@ void __init setup_rfi_flush(enum l1d_flush_type types, 
> bool enable)
>  #ifdef CONFIG_DEBUG_FS
>  static int rfi_flush_set(void *data, u64 val)
>  {
> + bool enable;
> +
>   if (val == 1)
> - rfi_flush_enable(true);
> + enable = true;
>   else if (val == 0)
> - rfi_flush_enable(false);
> + enable = false;
>   else
>   return -EINVAL;
> 
> + /* Only do anything if we're changing state */
> + if (enable != rfi_flush)

Hi, Mauricio.

Do we need to take into account if no_rfi_flush == true?

if ((enable != rfi_flush) && !no_rfi_flush)

> + rfi_flush_enable(enable);
> +
>   return 0;
>  }
> 

Cheers
Murilo

Re: [PATCH V2 4/4] powerpc/mm/hash64: Increase the VA range

[Murilo Opsfelder Araujo]

On 02/26/2018 11:08 AM, Aneesh Kumar K.V wrote:
> ---
>  arch/powerpc/include/asm/book3s/64/hash-64k.h | 2 +-
>  arch/powerpc/include/asm/processor.h  | 9 -
>  arch/powerpc/mm/init_64.c | 6 --
>  arch/powerpc/mm/pgtable_64.c  | 5 -
>  4 files changed, 9 insertions(+), 13 deletions(-)

Hi, Aneesh.

This patch is missing Signed-off-by: line. You're encouraged to run
checkpatch.pl to inspect your patches.

You may also want to add a brief paragraph in the commit message
explaining the "why" of this change.

Cheers
Murilo

[PATCH v2] include/linux/vfio.h: Guard powerpc-specific functions with CONFIG_VFIO_SPAPR_EEH

[Murilo Opsfelder Araujo]

When CONFIG_EEH=y and CONFIG_VFIO_SPAPR_EEH=n, build fails with the
following:

drivers/vfio/pci/vfio_pci.o: In function `.vfio_pci_release':
vfio_pci.c:(.text+0xa98): undefined reference to 
`.vfio_spapr_pci_eeh_release'
drivers/vfio/pci/vfio_pci.o: In function `.vfio_pci_open':
vfio_pci.c:(.text+0x1420): undefined reference to `.vfio_spapr_pci_eeh_open'

In this case, vfio_pci.c should use the empty definitions of
vfio_spapr_pci_eeh_open and vfio_spapr_pci_eeh_release functions.

This patch fixes it by guarding these function definitions with
CONFIG_VFIO_SPAPR_EEH, the symbol that controls whether vfio_spapr_eeh.c is
built, which is where the non-empty versions of these functions are. We need to
make use of IS_ENABLED() macro because CONFIG_VFIO_SPAPR_EEH is a tristate
option.

This issue was found during a randconfig build. Logs are here:

http://kisskb.ellerman.id.au/kisskb/buildresult/12982362/

Signed-off-by: Murilo Opsfelder Araujo <mopsfel...@gmail.com>
---

Changes from v1:
- Rebased on top of next-20170718.

 include/linux/vfio.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 586809a..a47b985 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -152,7 +152,7 @@ extern int vfio_set_irqs_validate_and_prepare(struct 
vfio_irq_set *hdr,
  size_t *data_size);

 struct pci_dev;
-#ifdef CONFIG_EEH
+#if IS_ENABLED(CONFIG_VFIO_SPAPR_EEH)
 extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
 extern void vfio_spapr_pci_eeh_release(struct pci_dev *pdev);
 extern long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
@@ -173,7 +173,7 @@ static inline long vfio_spapr_iommu_eeh_ioctl(struct 
iommu_group *group,
 {
return -ENOTTY;
 }
-#endif /* CONFIG_EEH */
+#endif /* CONFIG_VFIO_SPAPR_EEH */

 /*
  * IRQfd - generic
--
2.9.4

[PATCH v2] include/linux/vfio.h: Guard powerpc-specific functions with CONFIG_VFIO_SPAPR_EEH

[Murilo Opsfelder Araujo]

When CONFIG_EEH=y and CONFIG_VFIO_SPAPR_EEH=n, build fails with the
following:

drivers/vfio/pci/vfio_pci.o: In function `.vfio_pci_release':
vfio_pci.c:(.text+0xa98): undefined reference to 
`.vfio_spapr_pci_eeh_release'
drivers/vfio/pci/vfio_pci.o: In function `.vfio_pci_open':
vfio_pci.c:(.text+0x1420): undefined reference to `.vfio_spapr_pci_eeh_open'

In this case, vfio_pci.c should use the empty definitions of
vfio_spapr_pci_eeh_open and vfio_spapr_pci_eeh_release functions.

This patch fixes it by guarding these function definitions with
CONFIG_VFIO_SPAPR_EEH, the symbol that controls whether vfio_spapr_eeh.c is
built, which is where the non-empty versions of these functions are. We need to
make use of IS_ENABLED() macro because CONFIG_VFIO_SPAPR_EEH is a tristate
option.

This issue was found during a randconfig build. Logs are here:

http://kisskb.ellerman.id.au/kisskb/buildresult/12982362/

Signed-off-by: Murilo Opsfelder Araujo <mopsfel...@gmail.com>
---
v1..v2:
- Make use of IS_ENABLED() macro because CONFIG_VFIO_SPAPR_EEH is a tristate
  option (fix http://www.spinics.net/lists/kvm/msg151032.html).

 include/linux/vfio.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index edf9b2c..92232f73 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -150,7 +150,7 @@ extern int vfio_set_irqs_validate_and_prepare(struct 
vfio_irq_set *hdr,
  size_t *data_size);
 
 struct pci_dev;
-#ifdef CONFIG_EEH
+#if IS_ENABLED(CONFIG_VFIO_SPAPR_EEH)
 extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
 extern void vfio_spapr_pci_eeh_release(struct pci_dev *pdev);
 extern long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
@@ -171,7 +171,7 @@ static inline long vfio_spapr_iommu_eeh_ioctl(struct 
iommu_group *group,
 {
return -ENOTTY;
 }
-#endif /* CONFIG_EEH */
+#endif /* CONFIG_VFIO_SPAPR_EEH */
 
 /*
  * IRQfd - generic
-- 
2.9.4

[PATCH] include/linux/vfio.h: Guard powerpc-specific functions with CONFIG_VFIO_SPAPR_EEH

[Murilo Opsfelder Araujo]

When CONFIG_EEH=y and CONFIG_VFIO_SPAPR_EEH=n, build fails with the
following:

drivers/vfio/pci/vfio_pci.o: In function `.vfio_pci_release':
vfio_pci.c:(.text+0xa98): undefined reference to 
`.vfio_spapr_pci_eeh_release'
drivers/vfio/pci/vfio_pci.o: In function `.vfio_pci_open':
vfio_pci.c:(.text+0x1420): undefined reference to `.vfio_spapr_pci_eeh_open'

In this case, vfio_pci.c should use the empty definitions of
vfio_spapr_pci_eeh_open and vfio_spapr_pci_eeh_release functions.

This patch fixes it by guarding these function definitions with
CONFIG_VFIO_SPAPR_EEH, the symbol that controls whether vfio_spapr_eeh.c is
built, which is where the non-empty versions of these functions are.

This issue was found during a randconfig build. Logs are here:

http://kisskb.ellerman.id.au/kisskb/buildresult/12982362/

Signed-off-by: Murilo Opsfelder Araujo <mopsfel...@gmail.com>
---
 include/linux/vfio.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index edf9b2c..0a05d57 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -150,7 +150,7 @@ extern int vfio_set_irqs_validate_and_prepare(struct 
vfio_irq_set *hdr,
  size_t *data_size);
 
 struct pci_dev;
-#ifdef CONFIG_EEH
+#ifdef CONFIG_VFIO_SPAPR_EEH
 extern void vfio_spapr_pci_eeh_open(struct pci_dev *pdev);
 extern void vfio_spapr_pci_eeh_release(struct pci_dev *pdev);
 extern long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group,
@@ -171,7 +171,7 @@ static inline long vfio_spapr_iommu_eeh_ioctl(struct 
iommu_group *group,
 {
return -ENOTTY;
 }
-#endif /* CONFIG_EEH */
+#endif /* CONFIG_VFIO_SPAPR_EEH */
 
 /*
  * IRQfd - generic
-- 
2.9.4

[PATCH] Update MAINTAINERS

[Murilo Opsfelder Araujo]

drivers/watchdog/wdrtas.c is of insterest of linuxppc maintainers.

Signed-off-by: Murilo Opsfelder Araujo <mopsfel...@gmail.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9609ca6..3f05927 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7670,6 +7670,7 @@ F:drivers/pci/hotplug/rpa*
 F: drivers/rtc/rtc-opal.c
 F: drivers/scsi/ibmvscsi/
 F: drivers/tty/hvc/hvc_opal.c
+F: drivers/watchdog/wdrtas.c
 F: tools/testing/selftests/powerpc
 N: /pmac
 N: powermac
-- 
2.9.4

[PATCH v2] drivers/watchdog/Kconfig: Update CONFIG_WATCHDOG_RTAS dependencies

[Murilo Opsfelder Araujo]

drivers/watchdog/wdrtas.c uses symbols defined in arch/powerpc/kernel/rtas.c,
which are exported iff CONFIG_PPC_RTAS is selected. Building wdrtas.c without
setting CONFIG_PPC_RTAS throws the following errors:

ERROR: ".rtas_token" [drivers/watchdog/wdrtas.ko] undefined!
ERROR: "rtas_data_buf" [drivers/watchdog/wdrtas.ko] undefined!
ERROR: "rtas_data_buf_lock" [drivers/watchdog/wdrtas.ko] undefined!
ERROR: ".rtas_get_sensor" [drivers/watchdog/wdrtas.ko] undefined!
ERROR: ".rtas_call" [drivers/watchdog/wdrtas.ko] undefined!

This was identified during a randconfig build where CONFIG_WATCHDOG_RTAS=m and
CONFIG_PPC_RTAS was not set. Logs are here:

http://kisskb.ellerman.id.au/kisskb/buildresult/12982152/

This patch fixes the issue by updating CONFIG_WATCHDOG_RTAS to depend on just
CONFIG_PPC_RTAS, removing COMPILE_TEST entirely.

Signed-off-by: Murilo Opsfelder Araujo <mopsfel...@gmail.com>
---
 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 8b9049d..e6e31a1 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1688,7 +1688,7 @@ config MEN_A21_WDT
 
 config WATCHDOG_RTAS
tristate "RTAS watchdog"
-   depends on PPC_RTAS || (PPC64 && COMPILE_TEST)
+   depends on PPC_RTAS
help
  This driver adds watchdog support for the RTAS watchdog.
 
-- 
2.9.4