* [PATCH v3, 1/5] powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr
From: Wang Wenhu @ 2020-04-24 2:45 UTC (permalink / raw)
To: gregkh, arnd, linux-kernel, linuxppc-dev
Cc: robh, oss, kernel, paulus, Wang Wenhu
In-Reply-To: <20200424024554.30709-1-wenhu.wang@vivo.com>
Include "linux/of_address.h" to fix the compile error for
mpc85xx_l2ctlr_of_probe() when compiling fsl_85xx_cache_sram.c.
CC arch/powerpc/sysdev/fsl_85xx_l2ctlr.o
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c: In function ‘mpc85xx_l2ctlr_of_probe’:
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:11: error: implicit declaration of function ‘of_iomap’; did you mean ‘pci_iomap’? [-Werror=implicit-function-declaration]
l2ctlr = of_iomap(dev->dev.of_node, 0);
^~~~~~~~
pci_iomap
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:9: error: assignment makes pointer from integer without a cast [-Werror=int-conversion]
l2ctlr = of_iomap(dev->dev.of_node, 0);
^
cc1: all warnings being treated as errors
scripts/Makefile.build:267: recipe for target 'arch/powerpc/sysdev/fsl_85xx_l2ctlr.o' failed
make[2]: *** [arch/powerpc/sysdev/fsl_85xx_l2ctlr.o] Error 1
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
No changes
---
arch/powerpc/sysdev/fsl_85xx_l2ctlr.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
index 2d0af0c517bb..7533572492f0 100644
--- a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
+++ b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
@@ -10,6 +10,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of_platform.h>
+#include <linux/of_address.h>
#include <asm/io.h>
#include "fsl_85xx_cache_ctlr.h"
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 3/3] powerpc/module_64: Use special stub for _mcount() with -mprofile-kernel
From: Qian Cai @ 2020-04-24 1:19 UTC (permalink / raw)
To: Naveen N. Rao; +Cc: linuxppc-dev, Steven Rostedt
In-Reply-To: <8affd4298d22099bbd82544fab8185700a6222b1.1587488954.git.naveen.n.rao@linux.vnet.ibm.com>
> On Apr 21, 2020, at 1:35 PM, Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> wrote:
>
> Since commit c55d7b5e64265f ("powerpc: Remove STRICT_KERNEL_RWX
> incompatibility with RELOCATABLE"), powerpc kernels with
> -mprofile-kernel can crash in certain scenarios with a trace like below:
>
> BUG: Unable to handle kernel instruction fetch (NULL pointer?)
> Faulting instruction address: 0x00000000
> Oops: Kernel access of bad area, sig: 11 [#1]
> LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=256 DEBUG_PAGEALLOC NUMA PowerNV
> <snip>
> NIP [0000000000000000] 0x0
> LR [c0080000102c0048] ext4_iomap_end+0x8/0x30 [ext4]
> Call Trace:
> iomap_apply+0x20c/0x920 (unreliable)
> iomap_bmap+0xfc/0x160
> ext4_bmap+0xa4/0x180 [ext4]
> bmap+0x4c/0x80
> jbd2_journal_init_inode+0x44/0x1a0 [jbd2]
> ext4_load_journal+0x440/0x860 [ext4]
> ext4_fill_super+0x342c/0x3ab0 [ext4]
> mount_bdev+0x25c/0x290
> ext4_mount+0x28/0x50 [ext4]
> legacy_get_tree+0x4c/0xb0
> vfs_get_tree+0x4c/0x130
> do_mount+0xa18/0xc50
> sys_mount+0x158/0x180
> system_call+0x5c/0x68
>
> The NIP points to NULL, or a random location (data even), while the LR
> always points to the LEP of a function (with an offset of 8), indicating
> that something went wrong with ftrace. However, ftrace is not
> necessarily active when such crashes occur.
>
> The kernel OOPS sometimes follows a warning from ftrace indicating that
> some module functions could not be patched with a nop. Other times, if a
> module is loaded early during boot, instruction patching can fail due to
> a separate bug, but the error is not reported due to missing error
> reporting.
>
> In all the above cases when instruction patching fails, ftrace will be
> disabled but certain kernel module functions will be left with default
> calls to _mcount(). This is not a problem with ELFv1. However, with
> -mprofile-kernel, the default stub is problematic since it depends on a
> valid module TOC in r2. If the kernel (or a different module) calls into
> a function that does not use the TOC, the function won't have a prologue
> to setup the module TOC. When that function calls into _mcount(), we
> will end up in the relocation stub that will use the previous TOC, and
> end up trying to jump into a random location. From the above trace:
>
> iomap_apply+0x20c/0x920 [kernel TOC]
> |
> V
> ext4_iomap_end+0x8/0x30 [no GEP == kernel TOC]
> |
> V
> _mcount() stub
> [uses kernel TOC -> random entry]
>
> To address this, let's change over to using the special stub that is
> used for ftrace_[regs_]caller() for _mcount(). This ensures that we are
> not dependent on a valid module TOC in r2 for default _mcount()
> handling.
>
> Reported-by: Qian Cai <cai@lca.pw>
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Feel free to add,
Tested-by: Qian Cai <cai@lca.pw>
^ permalink raw reply
* Re: [PATCH 17/21] mm: free_area_init: allow defining max_zone_pfn in descending order
From: Baoquan He @ 2020-04-24 0:33 UTC (permalink / raw)
To: Mike Rapoport
Cc: Rich Felker, linux-ia64, linux-doc, Catalin Marinas,
Heiko Carstens, Michal Hocko, James E.J. Bottomley, Max Filippov,
Guo Ren, linux-csky, linux-parisc, sparclinux, linux-hexagon,
linux-riscv, Greg Ungerer, linux-arch, linux-s390, linux-snps-arc,
linux-c6x-dev, Brian Cain, Jonathan Corbet, linux-sh,
Helge Deller, x86, Russell King, Ley Foon Tan, Mike Rapoport,
Geert Uytterhoeven, linux-arm-kernel, Mark Salter, Matt Turner,
linux-mips, uclinux-h8-devel, linux-xtensa, linux-alpha, linux-um,
linux-m68k, Tony Luck, Greentime Hu, Paul Walmsley,
Stafford Horne, Guan Xuetao, Hoan Tran, Michal Simek,
Thomas Bogendoerfer, Yoshinori Sato, Nick Hu, linux-mm,
Vineet Gupta, linux-kernel, openrisc, Richard Weinberger,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20200423055559.GF14260@kernel.org>
On 04/23/20 at 08:55am, Mike Rapoport wrote:
> On Thu, Apr 23, 2020 at 10:57:20AM +0800, Baoquan He wrote:
> > On 04/23/20 at 10:53am, Baoquan He wrote:
> > > On 04/12/20 at 10:48pm, Mike Rapoport wrote:
> > > > From: Mike Rapoport <rppt@linux.ibm.com>
> > > >
> > > > Some architectures (e.g. ARC) have the ZONE_HIGHMEM zone below the
> > > > ZONE_NORMAL. Allowing free_area_init() parse max_zone_pfn array even it is
> > > > sorted in descending order allows using free_area_init() on such
> > > > architectures.
> > > >
> > > > Add top -> down traversal of max_zone_pfn array in free_area_init() and use
> > > > the latter in ARC node/zone initialization.
> > >
> > > Or maybe leave ARC as is. The change in this patchset doesn't impact
> > > ARC's handling about zone initialization, leaving it as is can reduce
> > > the complication in implementation of free_area_init(), which is a
> > > common function. So I personally don't see a strong motivation to have
> > > this patch.
> >
> > OK, seems this patch is prepared to simplify free_area_init_node(), so
> > take back what I said at above.
> >
> > Then this looks necessary, even though it introduces special case into
> > common function free_area_init().
>
> The idea is to have a single free_area_init() for all architectures
> without keeping two completely different ways of calculating the zone
> extents.
> Another thing, is that with this we could eventually switch ARC from
> DISCONTIGMEM.
Yeah, I think uniting them into a single free_area_init() is a great
idea. Even though I had been through this patchset, when looked into
each of them, still may forget the detail in later patch :)
^ permalink raw reply
* [Bug 206695] kmemleak reports leaks in drivers/macintosh/windfarm
From: bugzilla-daemon @ 2020-04-24 0:30 UTC (permalink / raw)
To: linuxppc-dev
In-Reply-To: <bug-206695-206035@https.bugzilla.kernel.org/>
https://bugzilla.kernel.org/show_bug.cgi?id=206695
Dennis Clarke (dclarke@blastwave.org) changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |dclarke@blastwave.org
--- Comment #8 from Dennis Clarke (dclarke@blastwave.org) ---
123456789+123456789+123456789+123456789+123456789+123456789+123456789+
I will apply the patch and try with Linux 5.7-rc2 and post any results
seen.
Also this does close off : https://bugzilla.kernel.org/show_bug.cgi?id=199471
I see Wolfram Sang has commented there. OKay ... good stuff.
Dennis Clarke
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* [powerpc:fixes-test] BUILD SUCCESS feb8e960d780e170e992a70491eec9dd68f4dbf2
From: kbuild test robot @ 2020-04-23 23:04 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git fixes-test
branch HEAD: feb8e960d780e170e992a70491eec9dd68f4dbf2 powerpc/mm: Fix CONFIG_PPC_KUAP_DEBUG on PPC32
elapsed time: 1303m
configs tested: 221
configs skipped: 167
The following configs have been built successfully.
More configs may be tested in the coming days.
arm efm32_defconfig
arm at91_dt_defconfig
arm shmobile_defconfig
arm64 defconfig
arm exynos_defconfig
arm multi_v5_defconfig
arm sunxi_defconfig
arm multi_v7_defconfig
arm64 allyesconfig
arm allyesconfig
arm64 allmodconfig
arm allmodconfig
arm64 allnoconfig
arm allnoconfig
sparc allyesconfig
mips ar7_defconfig
um i386_defconfig
ia64 tiger_defconfig
i386 defconfig
riscv allmodconfig
i386 allnoconfig
i386 allyesconfig
i386 alldefconfig
i386 debian-10.3
ia64 allmodconfig
ia64 defconfig
ia64 allnoconfig
ia64 generic_defconfig
ia64 bigsur_defconfig
ia64 allyesconfig
ia64 alldefconfig
nios2 3c120_defconfig
nios2 10m50_defconfig
c6x evmc6678_defconfig
xtensa iss_defconfig
c6x allyesconfig
xtensa common_defconfig
openrisc simple_smp_defconfig
openrisc or1ksim_defconfig
h8300 h8s-sim_defconfig
h8300 edosk2674_defconfig
m68k m5475evb_defconfig
m68k allmodconfig
h8300 h8300h-sim_defconfig
m68k sun3_defconfig
m68k multi_defconfig
powerpc defconfig
powerpc ppc64_defconfig
powerpc rhel-kconfig
powerpc allnoconfig
arc defconfig
arc allyesconfig
microblaze mmu_defconfig
microblaze nommu_defconfig
mips fuloong2e_defconfig
mips malta_kvm_defconfig
mips allyesconfig
mips 64r6el_defconfig
mips allnoconfig
mips 32r2_defconfig
mips allmodconfig
mips malta_kvm_guest_defconfig
mips tb0287_defconfig
mips capcella_defconfig
mips ip32_defconfig
mips decstation_64_defconfig
mips loongson3_defconfig
mips ath79_defconfig
mips bcm63xx_defconfig
parisc allnoconfig
parisc generic-64bit_defconfig
parisc generic-32bit_defconfig
parisc allyesconfig
parisc allmodconfig
parisc randconfig-a001-20200423
alpha randconfig-a001-20200423
mips randconfig-a001-20200423
m68k randconfig-a001-20200423
riscv randconfig-a001-20200423
nds32 randconfig-a001-20200423
parisc randconfig-a001-20200422
mips randconfig-a001-20200422
alpha randconfig-a001-20200422
m68k randconfig-a001-20200422
riscv randconfig-a001-20200422
nds32 randconfig-a001-20200422
nios2 randconfig-a001-20200423
h8300 randconfig-a001-20200423
c6x randconfig-a001-20200423
sparc64 randconfig-a001-20200423
microblaze randconfig-a001-20200423
nios2 randconfig-a001-20200422
h8300 randconfig-a001-20200422
c6x randconfig-a001-20200422
sparc64 randconfig-a001-20200422
microblaze randconfig-a001-20200422
sh randconfig-a001-20200423
csky randconfig-a001-20200423
xtensa randconfig-a001-20200423
openrisc randconfig-a001-20200423
sh randconfig-a001-20200422
csky randconfig-a001-20200422
s390 randconfig-a001-20200422
xtensa randconfig-a001-20200422
openrisc randconfig-a001-20200422
i386 randconfig-b002-20200422
i386 randconfig-b001-20200422
x86_64 randconfig-b001-20200422
i386 randconfig-b003-20200422
x86_64 randconfig-b003-20200422
x86_64 randconfig-a001-20200422
i386 randconfig-a003-20200422
x86_64 randconfig-a003-20200422
i386 randconfig-a002-20200422
i386 randconfig-a001-20200422
x86_64 randconfig-a002-20200422
i386 randconfig-c002-20200423
i386 randconfig-c001-20200423
x86_64 randconfig-c002-20200423
x86_64 randconfig-c001-20200423
i386 randconfig-c003-20200423
x86_64 randconfig-c003-20200423
x86_64 randconfig-d001-20200423
x86_64 randconfig-d002-20200423
i386 randconfig-d002-20200423
i386 randconfig-d001-20200423
x86_64 randconfig-d003-20200423
i386 randconfig-d003-20200423
x86_64 randconfig-d002-20200422
i386 randconfig-d002-20200422
i386 randconfig-d001-20200422
i386 randconfig-d003-20200422
i386 randconfig-e003-20200422
x86_64 randconfig-e003-20200422
i386 randconfig-e002-20200422
i386 randconfig-e001-20200422
x86_64 randconfig-e001-20200422
x86_64 randconfig-e002-20200423
i386 randconfig-e003-20200423
x86_64 randconfig-e003-20200423
i386 randconfig-e002-20200423
i386 randconfig-e001-20200423
x86_64 randconfig-e001-20200423
i386 randconfig-f002-20200421
i386 randconfig-f003-20200421
x86_64 randconfig-f003-20200421
i386 randconfig-f001-20200421
x86_64 randconfig-f001-20200421
x86_64 randconfig-f002-20200422
i386 randconfig-f002-20200422
x86_64 randconfig-f003-20200422
i386 randconfig-f003-20200422
i386 randconfig-f001-20200422
x86_64 randconfig-f001-20200422
i386 randconfig-g003-20200422
x86_64 randconfig-g001-20200422
i386 randconfig-g001-20200422
x86_64 randconfig-g002-20200422
i386 randconfig-g002-20200422
x86_64 randconfig-g003-20200422
i386 randconfig-g003-20200424
i386 randconfig-g001-20200424
x86_64 randconfig-g001-20200424
x86_64 randconfig-g002-20200424
i386 randconfig-g002-20200424
x86_64 randconfig-g003-20200424
i386 randconfig-g003-20200423
x86_64 randconfig-g001-20200423
i386 randconfig-g001-20200423
i386 randconfig-g002-20200423
x86_64 randconfig-g003-20200423
i386 randconfig-h003-20200424
x86_64 randconfig-h001-20200424
x86_64 randconfig-h003-20200424
x86_64 randconfig-h002-20200424
i386 randconfig-h001-20200424
i386 randconfig-h002-20200424
i386 randconfig-h003-20200422
x86_64 randconfig-h001-20200422
x86_64 randconfig-h003-20200422
x86_64 randconfig-h002-20200422
i386 randconfig-h001-20200422
i386 randconfig-h002-20200422
i386 randconfig-h003-20200423
x86_64 randconfig-h002-20200423
i386 randconfig-h002-20200423
i386 randconfig-h001-20200423
sparc randconfig-a001-20200423
ia64 randconfig-a001-20200423
arm randconfig-a001-20200423
arm64 randconfig-a001-20200423
arc randconfig-a001-20200423
sparc randconfig-a001-20200422
ia64 randconfig-a001-20200422
powerpc randconfig-a001-20200422
arm randconfig-a001-20200422
arm64 randconfig-a001-20200422
arc randconfig-a001-20200422
riscv allyesconfig
riscv nommu_virt_defconfig
riscv allnoconfig
riscv defconfig
riscv rv32_defconfig
s390 zfcpdump_defconfig
s390 debug_defconfig
s390 allyesconfig
s390 allnoconfig
s390 allmodconfig
s390 alldefconfig
s390 defconfig
sh rsk7269_defconfig
sh allmodconfig
sh titan_defconfig
sh sh7785lcr_32bit_defconfig
sh allnoconfig
sparc defconfig
sparc64 defconfig
sparc64 allnoconfig
sparc64 allyesconfig
sparc64 allmodconfig
x86_64 rhel
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply
* Re: [PATCH v8 1/1] powerpc/powernv: Introduce support and parsing for self-save API
From: Gautham R Shenoy @ 2020-04-23 18:15 UTC (permalink / raw)
To: Pratik Rajesh Sampat
Cc: ego, pratik.r.sampat, linuxram, linux-kernel, linuxppc-dev,
oohall, skiboot
In-Reply-To: <20200423105557.29108-2-psampat@linux.ibm.com>
On Thu, Apr 23, 2020 at 04:25:57PM +0530, Pratik Rajesh Sampat wrote:
> This commit introduces and leverages the Self save API. The difference
> between self-save and self-restore is that the value to be saved for the
> SPR does not need to be passed to the call.
>
> Add the new Self Save OPAL API call in the list of OPAL calls.
>
> The device tree is parsed looking for the property "ibm,opal-self-save"
> If self-save is supported then for all SPRs self-save is invoked for all
> P9 supported registers. In the case self-save fails corresponding
> self-restore call is invoked as a fallback.
>
> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
A suggestion from the bisectability point of view though.
Since in this patch you are also invoking self_save API for a new SPR,
namely PTCR which was previously not present, I would suggest that you
move the PTCR changes to a different patch.
Otherwise, the patchset looks good to me
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/opal-api.h | 3 +-
> arch/powerpc/include/asm/opal.h | 1 +
> arch/powerpc/platforms/powernv/idle.c | 73 ++++++++++++++++++----
> arch/powerpc/platforms/powernv/opal-call.c | 1 +
> 4 files changed, 64 insertions(+), 14 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h
> index 1dffa3cb16ba..7ba698369083 100644
> --- a/arch/powerpc/include/asm/opal-api.h
> +++ b/arch/powerpc/include/asm/opal-api.h
> @@ -214,7 +214,8 @@
> #define OPAL_SECVAR_GET 176
> #define OPAL_SECVAR_GET_NEXT 177
> #define OPAL_SECVAR_ENQUEUE_UPDATE 178
> -#define OPAL_LAST 178
> +#define OPAL_SLW_SELF_SAVE_REG 181
> +#define OPAL_LAST 181
>
> #define QUIESCE_HOLD 1 /* Spin all calls at entry */
> #define QUIESCE_REJECT 2 /* Fail all calls with OPAL_BUSY */
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index 9986ac34b8e2..a370b0e8d899 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -204,6 +204,7 @@ int64_t opal_handle_hmi2(__be64 *out_flags);
> int64_t opal_register_dump_region(uint32_t id, uint64_t start, uint64_t end);
> int64_t opal_unregister_dump_region(uint32_t id);
> int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val);
> +int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn);
> int64_t opal_config_cpu_idle_state(uint64_t state, uint64_t flag);
> int64_t opal_pci_set_phb_cxl_mode(uint64_t phb_id, uint64_t mode, uint64_t pe_number);
> int64_t opal_pci_get_pbcq_tunnel_bar(uint64_t phb_id, uint64_t *addr);
> diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c
> index 78599bca66c2..ada7ece24521 100644
> --- a/arch/powerpc/platforms/powernv/idle.c
> +++ b/arch/powerpc/platforms/powernv/idle.c
> @@ -32,6 +32,11 @@
> #define P9_STOP_SPR_MSR 2000
> #define P9_STOP_SPR_PSSCR 855
>
> +/* Caching the self-save functionality, lpcr, ptcr support */
> +DEFINE_STATIC_KEY_FALSE(self_save_available);
> +DEFINE_STATIC_KEY_FALSE(is_lpcr_self_save);
> +DEFINE_STATIC_KEY_FALSE(is_ptcr_self_save);
> +
> static u32 supported_cpuidle_states;
> struct pnv_idle_states_t *pnv_idle_states;
> int nr_pnv_idle_states;
> @@ -61,6 +66,35 @@ static bool deepest_stop_found;
>
> static unsigned long power7_offline_type;
>
> +/*
> + * Cache support for SPRs that support self-save as well as kernel save restore
> + * so that kernel does not duplicate efforts in saving and restoring SPRs
> + */
> +static void cache_spr_self_save_support(u64 sprn)
> +{
> + switch (sprn) {
> + case SPRN_LPCR:
> + static_branch_enable(&is_lpcr_self_save);
> + break;
> + case SPRN_PTCR:
> + static_branch_enable(&is_ptcr_self_save);
> + break;
> + }
> +}
> +
> +static int pnv_save_one_spr(u64 pir, u64 sprn, u64 val)
> +{
> + if (static_branch_likely(&self_save_available)) {
> + int rc = opal_slw_self_save_reg(pir, sprn);
> +
> + if (!rc) {
> + cache_spr_self_save_support(sprn);
> + return rc;
> + }
> + }
> + return opal_slw_set_reg(pir, sprn, val);
> +}
> +
> static int pnv_save_sprs_for_deep_states(void)
> {
> int cpu;
> @@ -72,6 +106,7 @@ static int pnv_save_sprs_for_deep_states(void)
> * same across all cpus.
> */
> uint64_t lpcr_val = mfspr(SPRN_LPCR);
> + uint64_t ptcr_val = mfspr(SPRN_PTCR);
> uint64_t hid0_val = mfspr(SPRN_HID0);
> uint64_t hid1_val = mfspr(SPRN_HID1);
> uint64_t hid4_val = mfspr(SPRN_HID4);
> @@ -84,30 +119,34 @@ static int pnv_save_sprs_for_deep_states(void)
> uint64_t pir = get_hard_smp_processor_id(cpu);
> uint64_t hsprg0_val = (uint64_t)paca_ptrs[cpu];
>
> - rc = opal_slw_set_reg(pir, SPRN_HSPRG0, hsprg0_val);
> + rc = pnv_save_one_spr(pir, SPRN_HSPRG0, hsprg0_val);
> if (rc != 0)
> return rc;
>
> - rc = opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
> + rc = pnv_save_one_spr(pir, SPRN_LPCR, lpcr_val);
> if (rc != 0)
> return rc;
>
> + /*
> + * No need to check for failure, if firmware fails to save then
> + * kernel handles save-restore for PTCR
> + */
> + pnv_save_one_spr(pir, SPRN_PTCR, ptcr_val);
> +
> if (cpu_has_feature(CPU_FTR_ARCH_300)) {
> - rc = opal_slw_set_reg(pir, P9_STOP_SPR_MSR, msr_val);
> + rc = pnv_save_one_spr(pir, P9_STOP_SPR_MSR, msr_val);
> if (rc)
> return rc;
>
> - rc = opal_slw_set_reg(pir,
> + rc = pnv_save_one_spr(pir,
> P9_STOP_SPR_PSSCR, psscr_val);
> -
> if (rc)
> return rc;
> }
>
> /* HIDs are per core registers */
> if (cpu_thread_in_core(cpu) == 0) {
> -
> - rc = opal_slw_set_reg(pir, SPRN_HMEER, hmeer_val);
> + rc = pnv_save_one_spr(pir, SPRN_HMEER, hmeer_val);
> if (rc != 0)
> return rc;
>
> @@ -658,7 +697,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
> mmcr0 = mfspr(SPRN_MMCR0);
> }
> if ((psscr & PSSCR_RL_MASK) >= pnv_first_spr_loss_level) {
> - sprs.lpcr = mfspr(SPRN_LPCR);
> + if (!static_branch_unlikely(&is_lpcr_self_save))
> + sprs.lpcr = mfspr(SPRN_LPCR);
> sprs.hfscr = mfspr(SPRN_HFSCR);
> sprs.fscr = mfspr(SPRN_FSCR);
> sprs.pid = mfspr(SPRN_PID);
> @@ -672,7 +712,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
> sprs.mmcr1 = mfspr(SPRN_MMCR1);
> sprs.mmcr2 = mfspr(SPRN_MMCR2);
>
> - sprs.ptcr = mfspr(SPRN_PTCR);
> + if (!static_branch_unlikely(&is_ptcr_self_save))
> + sprs.ptcr = mfspr(SPRN_PTCR);
> sprs.rpr = mfspr(SPRN_RPR);
> sprs.tscr = mfspr(SPRN_TSCR);
> if (!firmware_has_feature(FW_FEATURE_ULTRAVISOR))
> @@ -756,7 +797,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
> goto core_woken;
>
> /* Per-core SPRs */
> - mtspr(SPRN_PTCR, sprs.ptcr);
> + if (!static_branch_unlikely(&is_ptcr_self_save))
> + mtspr(SPRN_PTCR, sprs.ptcr);
> mtspr(SPRN_RPR, sprs.rpr);
> mtspr(SPRN_TSCR, sprs.tscr);
>
> @@ -777,7 +819,8 @@ static unsigned long power9_idle_stop(unsigned long psscr, bool mmu_on)
> atomic_unlock_and_stop_thread_idle();
>
> /* Per-thread SPRs */
> - mtspr(SPRN_LPCR, sprs.lpcr);
> + if (!static_branch_unlikely(&is_lpcr_self_save))
> + mtspr(SPRN_LPCR, sprs.lpcr);
> mtspr(SPRN_HFSCR, sprs.hfscr);
> mtspr(SPRN_FSCR, sprs.fscr);
> mtspr(SPRN_PID, sprs.pid);
> @@ -956,8 +999,10 @@ void pnv_program_cpu_hotplug_lpcr(unsigned int cpu, u64 lpcr_val)
> * Program the LPCR via stop-api only if the deepest stop state
> * can lose hypervisor context.
> */
> - if (supported_cpuidle_states & OPAL_PM_LOSE_FULL_CONTEXT)
> - opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
> + if (supported_cpuidle_states & OPAL_PM_LOSE_FULL_CONTEXT) {
> + if (!static_branch_unlikely(&is_lpcr_self_save))
> + opal_slw_set_reg(pir, SPRN_LPCR, lpcr_val);
> + }
> }
>
> /*
> @@ -1298,6 +1343,8 @@ static int pnv_parse_cpuidle_dt(void)
> }
> for (i = 0; i < nr_idle_states; i++)
> pnv_idle_states[i].psscr_mask = temp_u64[i];
> + if (of_property_read_bool(np, "ibm,opal-self-save"))
> + static_branch_enable(&self_save_available);
> }
>
> /*
> diff --git a/arch/powerpc/platforms/powernv/opal-call.c b/arch/powerpc/platforms/powernv/opal-call.c
> index 5cd0f52d258f..11e0ceb90de0 100644
> --- a/arch/powerpc/platforms/powernv/opal-call.c
> +++ b/arch/powerpc/platforms/powernv/opal-call.c
> @@ -223,6 +223,7 @@ OPAL_CALL(opal_handle_hmi, OPAL_HANDLE_HMI);
> OPAL_CALL(opal_handle_hmi2, OPAL_HANDLE_HMI2);
> OPAL_CALL(opal_config_cpu_idle_state, OPAL_CONFIG_CPU_IDLE_STATE);
> OPAL_CALL(opal_slw_set_reg, OPAL_SLW_SET_REG);
> +OPAL_CALL(opal_slw_self_save_reg, OPAL_SLW_SELF_SAVE_REG);
> OPAL_CALL(opal_register_dump_region, OPAL_REGISTER_DUMP_REGION);
> OPAL_CALL(opal_unregister_dump_region, OPAL_UNREGISTER_DUMP_REGION);
> OPAL_CALL(opal_pci_set_phb_cxl_mode, OPAL_PCI_SET_PHB_CAPI_MODE);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v8 3/3] Self save API integration
From: Gautham R Shenoy @ 2020-04-23 18:10 UTC (permalink / raw)
To: Pratik Rajesh Sampat
Cc: ego, pratik.r.sampat, linuxram, linux-kernel, linuxppc-dev,
oohall, skiboot
In-Reply-To: <20200423105438.29034-4-psampat@linux.ibm.com>
On Thu, Apr 23, 2020 at 04:24:38PM +0530, Pratik Rajesh Sampat wrote:
> The commit makes the self save API available outside the firmware by defining
> an OPAL wrapper.
> This wrapper has a similar interface to that of self restore and expects the
> cpu pir, SPR number, minus the value of that SPR to be passed in its
> paramters and returns OPAL_SUCCESS on success. It adds a device-tree
> node signifying support for self-save after verifying the stop API
> version compatibility.
>
> The commit also documents both the self-save and the self-restore API
> calls along with their working and usage.
>
> Signed-off-by: Pratik Rajesh Sampat <psampat@linux.ibm.com>
LGTM.
Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
> doc/opal-api/opal-slw-self-save-reg-181.rst | 51 ++++++++++
> doc/opal-api/opal-slw-set-reg-100.rst | 5 +
> doc/power-management.rst | 48 +++++++++
> hw/slw.c | 106 ++++++++++++++++++++
> include/opal-api.h | 3 +-
> include/p9_stop_api.H | 18 ++++
> include/skiboot.h | 3 +
> 7 files changed, 233 insertions(+), 1 deletion(-)
> create mode 100644 doc/opal-api/opal-slw-self-save-reg-181.rst
>
> diff --git a/doc/opal-api/opal-slw-self-save-reg-181.rst b/doc/opal-api/opal-slw-self-save-reg-181.rst
> new file mode 100644
> index 00000000..f20e9b81
> --- /dev/null
> +++ b/doc/opal-api/opal-slw-self-save-reg-181.rst
> @@ -0,0 +1,51 @@
> +.. OPAL_SLW_SELF_SAVE_REG:
> +
> +OPAL_SLW_SELF_SAVE_REG
> +======================
> +
> +.. code-block:: c
> +
> + #define OPAL_SLW_SELF_SAVE_REG 181
> +
> + int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn);
> +
> +:ref:`OPAL_SLW_SELF_SAVE_REG` is used to inform low-level firmware to save
> +the current contents of the SPR before entering a state of loss and
> +also restore the content back on waking up from a deep stop state.
> +
> +An OPAL call `OPAL_SLW_SET_REG` exists which is similar in function as
> +saving and restoring the SPR, with one difference being that the value of the
> +SPR must also be supplied in the parameters.
> +Complete reference: doc/opal-api/opal-slw-set-reg-100.rst
> +
> +Parameters
> +----------
> +
> +``uint64_t cpu_pir``
> + This parameter specifies the pir of the cpu for which the call is being made.
> +``uint64_t sprn``
> + This parameter specifies the spr number as mentioned in p9_stop_api.H
> + The list of SPRs supported is as follows.
> + P9_STOP_SPR_DAWR,
> + P9_STOP_SPR_HSPRG0,
> + P9_STOP_SPR_LDBAR,
> + P9_STOP_SPR_LPCR,
> + P9_STOP_SPR_PSSCR,
> + P9_STOP_SPR_MSR,
> + P9_STOP_SPR_HRMOR,
> + P9_STOP_SPR_HMEER,
> + P9_STOP_SPR_PMCR,
> + P9_STOP_SPR_PTCR
> +
> + The property "ibm,opal-self-save" is supplied to the device tree to advterise
> + support.
> +
> +Returns
> +-------
> +
> +:ref:`OPAL_UNSUPPORTED`
> + If spr restore is not supported by pore engine.
> +:ref:`OPAL_PARAMETER`
> + Invalid handle for the pir/chip
> +:ref:`OPAL_SUCCESS`
> + On success
> diff --git a/doc/opal-api/opal-slw-set-reg-100.rst b/doc/opal-api/opal-slw-set-reg-100.rst
> index 2e8f1bd6..ee3e68ce 100644
> --- a/doc/opal-api/opal-slw-set-reg-100.rst
> +++ b/doc/opal-api/opal-slw-set-reg-100.rst
> @@ -21,6 +21,11 @@ In Power 9, it uses p9_stop_save_cpureg(), api provided by self restore code,
> to inform the spr with their corresponding values with which they
> must be restored.
>
> +An OPAL call `OPAL_SLW_SELF_SAVE_REG` exists which is similar in function
> +saving and restoring the SPR, with one difference being that the value of the
> +SPR doesn't need to be passed in the parameters, only with the SPR number
> +the firmware can identify, save and restore the values for the same.
> +Complete reference: doc/opal-api/opal-slw-self-save-reg-181.rst
>
> Parameters
> ----------
> diff --git a/doc/power-management.rst b/doc/power-management.rst
> index 76491a71..d6bd5358 100644
> --- a/doc/power-management.rst
> +++ b/doc/power-management.rst
> @@ -15,3 +15,51 @@ On boot, specific stop states can be disabled via setting a mask. For example,
> to disable all but stop 0,1,2, use ~0xE0000000. ::
>
> nvram -p ibm,skiboot --update-config opal-stop-state-disable-mask=0x1FFFFFFF
> +
> +Saving and restoring Special Purpose Registers(SPRs)
> +----------------------------------------------------
> +
> +When a CPU wakes up from a deep stop state which can result in
> +hypervisor state loss, all the SPRs are lost. The Linux Kernel expects
> +a small set of SPRs to contain an expected value when the CPU wakes up
> +from such a deep stop state. The microcode firmware provides the
> +following two APIs, collectively known as the stop-APIs, to allow the
> +kernel/OPAL to specify this set of SPRs and the value that they need
> +to be restored with on waking up from a deep stop state.
> +
> +Self-restore:
> +int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val);
> +The SPR number and the value of the that SPR must be restored with on
> +wakeup from the deep-stop state must be specified. When this call is
> +made, the microcode inserts instruction into the HOMER region to
> +restore the content of the SPR to the specified value on wakeup from a
> +deep-stop state. These instructions are executed by the CPU as soon as
> +it wakes up from a deep stop state. The call is to be made once per
> +SPR.
> +
> +Self-Save:
> +int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn);
> +Only the SPR number needs to be specified. When this call is made, the
> +microcode inserts instructions into the HOMER region to save the
> +current value of the SPR before the CPU goes to a deep stop state, and
> +restores the value back when the CPU wakes up from a deep stop state.
> +These instructions are correspondingly executed just before and after
> +the CPU goes/comes out of a deep stop state. This call can be made
> +once per SPR.
> +
> +The key difference between self-save and self-restore is the
> +use-case. If the Kernel expects the SPR to contain a particular value
> +on waking up from a deep-stop state, that wasn't the value of that SPR
> +before entering deep stop-state, then self-restore is preferable.
> +Also in a case where SPR does not change across the lifetime
> +self-restore is more efficient as when the value is same the memeory location
> +is not updated.
> +
> +When deep stop states are to be supported in an Ultravisor
> +environment, since HOMER is in a secure region, the stop-api cannot
> +update the HOMER if invoked from a context when the OPAL/Kernel is
> +executing without the ultravisor privilege. In this scenario, at the
> +time of early OPAL boot, while OPAL has ultravisor privileges, it can
> +make the self-save stop-api call for all the supported SPRs, so that
> +the microcode in the HOMER will always save and restore all the
> +supported SPRs during entry/exit from a deep stop state.
> diff --git a/hw/slw.c b/hw/slw.c
> index beb129a8..8423eeaf 100644
> --- a/hw/slw.c
> +++ b/hw/slw.c
> @@ -35,6 +35,43 @@ static bool slw_current_le = false;
> enum wakeup_engine_states wakeup_engine_state = WAKEUP_ENGINE_NOT_PRESENT;
> bool has_deep_states = false;
>
> +/**
> + * The struct and SPR list is partially consistent with libpore/p9_stop_api.c
> + */
> +/**
> + * @brief summarizes attributes associated with a SPR register.
> + */
> +typedef struct
> +{
> + uint32_t iv_sprId;
> + bool iv_isThreadScope;
> + uint32_t iv_saveMaskPos;
> +
> +} StopSprReg_t;
> +
> +/**
> + * @brief a true in the table below means register is of scope thread
> + * whereas a false meanse register is of scope core.
> + * The number is the bit position on a uint32_t mask
> + */
> +
> +static const StopSprReg_t g_sprRegister[] =
> +{
> + { P9_STOP_SPR_DAWR, true, 1 },
> + { P9_STOP_SPR_HSPRG0, true, 3 },
> + { P9_STOP_SPR_LDBAR, true, 4, },
> + { P9_STOP_SPR_LPCR, true, 5 },
> + { P9_STOP_SPR_PSSCR, true, 6 },
> + { P9_STOP_SPR_MSR, true, 7 },
> + { P9_STOP_SPR_HRMOR, false, 255 },
> + { P9_STOP_SPR_HID, false, 21 },
> + { P9_STOP_SPR_HMEER, false, 22 },
> + { P9_STOP_SPR_PMCR, false, 23 },
> + { P9_STOP_SPR_PTCR, false, 24 },
> +};
> +
> +static const uint32_t MAX_SPR_SUPPORTED = ARRAY_SIZE(g_sprRegister);
> +
> DEFINE_LOG_ENTRY(OPAL_RC_SLW_INIT, OPAL_PLATFORM_ERR_EVT, OPAL_SLW,
> OPAL_PLATFORM_FIRMWARE, OPAL_PREDICTIVE_ERR_GENERAL,
> OPAL_NA);
> @@ -720,11 +757,14 @@ void add_cpu_idle_state_properties(void)
> struct cpu_idle_states *states;
> struct proc_chip *chip;
> int nr_states;
> + int rc;
>
> bool can_sleep = true;
> bool has_stop_inst = false;
> + bool has_self_save = true;
> u8 i;
>
> + u64 compVector = -1;
> fdt64_t *pm_ctrl_reg_val_buf;
> fdt64_t *pm_ctrl_reg_mask_buf;
> u32 supported_states_mask;
> @@ -766,6 +806,20 @@ void add_cpu_idle_state_properties(void)
> */
> chip = next_chip(NULL);
> assert(chip);
> + rc = proc_stop_api_discover_capability((void *) chip->homer_base,
> + &compVector);
> + if (rc == STOP_SAVE_ARG_INVALID_IMG) {
> + prlog(PR_DEBUG, "HOMER BASE INVALID\n");
> + return;
> + } else if (rc == STOP_SAVE_API_IMG_INCOMPATIBLE) {
> + prlog(PR_DEBUG, "STOP API running incompatible versions\n");
> + if ((compVector & SELF_RESTORE_VER_MISMATCH) == 0) {
> + prlog(PR_DEBUG, "Self-save API unsupported\n");
> + has_self_save = false;
> + }
> + }
> + if (has_self_save)
> + dt_add_property(power_mgt, "ibm,opal-self-save", NULL, 0);
> if (chip->type == PROC_CHIP_P9_NIMBUS ||
> chip->type == PROC_CHIP_P9_CUMULUS ||
> chip->type == PROC_CHIP_P9P) {
> @@ -1446,6 +1500,58 @@ int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val)
>
> opal_call(OPAL_SLW_SET_REG, opal_slw_set_reg, 3);
>
> +int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn)
> +{
> + struct cpu_thread * c = find_cpu_by_pir(cpu_pir);
> + uint32_t save_reg_vector = 0;
> + struct proc_chip * chip;
> + int rc;
> + int index;
> +
> + if (!c) {
> + prlog(PR_DEBUG, "SLW: Unknown thread with pir %x\n",
> + (u32) cpu_pir);
> + return OPAL_PARAMETER;
> + }
> +
> + chip = get_chip(c->chip_id);
> + if (!chip) {
> + prlog(PR_DEBUG, "SLW: Unknown chip for thread with pir %x\n",
> + (u32) cpu_pir);
> + return OPAL_PARAMETER;
> + }
> + if (proc_gen != proc_gen_p9 || !has_deep_states) {
> + prlog(PR_DEBUG, "SLW: Self-save feature unsupported\n");
> + return OPAL_UNSUPPORTED;
> + }
> + if (wakeup_engine_state != WAKEUP_ENGINE_PRESENT) {
> + log_simple_error(&e_info(OPAL_RC_SLW_REG),
> + "SLW: wakeup_engine in bad state=%d chip=%x\n",
> + wakeup_engine_state, chip->id);
> + return OPAL_INTERNAL_ERROR;
> + }
> + for (index = 0; index < MAX_SPR_SUPPORTED; ++index) {
> + if (sprn == (CpuReg_t) g_sprRegister[index].iv_sprId) {
> + save_reg_vector = PPC_BIT32(
> + g_sprRegister[index].iv_saveMaskPos);
> + break;
> + }
> + }
> + if (save_reg_vector == 0)
> + return OPAL_INTERNAL_ERROR;
> + rc = p9_stop_save_cpureg_control((void *) chip->homer_base,
> + cpu_pir, save_reg_vector);
> +
> + if (rc) {
> + log_simple_error(&e_info(OPAL_RC_SLW_REG),
> + "SLW: Failed to save vector %x for CPU %x\n",
> + save_reg_vector, c->pir);
> + return OPAL_INTERNAL_ERROR;
> + }
> + return OPAL_SUCCESS;
> +}
> +opal_call(OPAL_SLW_SELF_SAVE_REG, opal_slw_self_save_reg, 2);
> +
> void slw_init(void)
> {
> struct proc_chip *chip;
> diff --git a/include/opal-api.h b/include/opal-api.h
> index e90cab1e..1607a89b 100644
> --- a/include/opal-api.h
> +++ b/include/opal-api.h
> @@ -227,7 +227,8 @@
> #define OPAL_SECVAR_ENQUEUE_UPDATE 178
> #define OPAL_PHB_SET_OPTION 179
> #define OPAL_PHB_GET_OPTION 180
> -#define OPAL_LAST 180
> +#define OPAL_SLW_SELF_SAVE_REG 181
> +#define OPAL_LAST 181
>
> #define QUIESCE_HOLD 1 /* Spin all calls at entry */
> #define QUIESCE_REJECT 2 /* Fail all calls with OPAL_BUSY */
> diff --git a/include/p9_stop_api.H b/include/p9_stop_api.H
> index cb5ffd6f..09ce3dc1 100644
> --- a/include/p9_stop_api.H
> +++ b/include/p9_stop_api.H
> @@ -34,6 +34,8 @@
> ///
> /// @file p9_stop_api.H
> /// @brief describes STOP API which create/manipulate STOP image.
> +/// This header need not be consistent, however is a subset of the
> +/// libpore/p9_stop_api.H counterpart
> ///
> // *HWP HW Owner : Greg Still <stillgs@us.ibm.com>
> // *HWP FW Owner : Prem Shanker Jha <premjha2@in.ibm.com>
> @@ -58,6 +60,7 @@ typedef enum
> P9_STOP_SPR_HRMOR = 313, // core register
> P9_STOP_SPR_LPCR = 318, // thread register
> P9_STOP_SPR_HMEER = 337, // core register
> + P9_STOP_SPR_PTCR = 464, // core register
> P9_STOP_SPR_LDBAR = 850, // thread register
> P9_STOP_SPR_PSSCR = 855, // thread register
> P9_STOP_SPR_PMCR = 884, // core register
> @@ -247,6 +250,21 @@ StopReturnCode_t p9_stop_save_scom( void* const i_pImage,
> const ScomOperation_t i_operation,
> const ScomSection_t i_section );
>
> +/**
> + * @brief Facilitates self save and restore of a list of SPRs of a thread.
> + * @param[in] i_pImage points to the start of HOMER image of P9 chip.
> + * @param[in] i_pir PIR associated with thread
> + * @param[in] i_saveRegVector bit vector representing SPRs that needs to be restored.
> + * @return STOP_SAVE_SUCCESS if API succeeds, error code otherwise.
> + * @note SPR save vector is a bit vector. For each SPR supported,
> + * there is an associated bit position in the bit vector.Refer
> + * to definition of SprBitPositionList_t to determine bit position
> + * associated with a particular SPR.
> + */
> +StopReturnCode_t
> +p9_stop_save_cpureg_control( void* i_pImage, const uint64_t i_pir,
> + const uint32_t i_saveRegVector );
> +
> /**
> * @brief verifies if API is compatible of current HOMER image.
> * @param[in] i_pImage points to the start of HOMER image of P9 chip.
> diff --git a/include/skiboot.h b/include/skiboot.h
> index 30ff500c..9ced240e 100644
> --- a/include/skiboot.h
> +++ b/include/skiboot.h
> @@ -306,6 +306,9 @@ extern void nx_p9_rng_late_init(void);
> /* SLW reinit function for switching core settings */
> extern int64_t slw_reinit(uint64_t flags);
>
> +/* Self save SPR before entering the stop state */
> +extern int64_t opal_slw_self_save_reg(uint64_t cpu_pir, uint64_t sprn);
> +
> /* Patch SPR in SLW image */
> extern int64_t opal_slw_set_reg(uint64_t cpu_pir, uint64_t sprn, uint64_t val);
>
> --
> 2.25.1
>
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-23 17:42 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: libc-dev, libc-alpha, linuxppc-dev, Nicholas Piggin, musl
In-Reply-To: <64d82a23-1f6e-2e6a-b7a9-0eeab8a53435@linaro.org>
On Thu, Apr 23, 2020 at 02:15:58PM -0300, Adhemerval Zanella wrote:
>
>
> On 23/04/2020 13:43, Rich Felker wrote:
> > On Thu, Apr 23, 2020 at 01:35:01PM -0300, Adhemerval Zanella wrote:
> >>
> >>
> >> On 23/04/2020 13:18, Rich Felker wrote:
> >>> On Thu, Apr 23, 2020 at 09:13:57AM -0300, Adhemerval Zanella wrote:
> >>>>
> >>>>
> >>>> On 22/04/2020 23:36, Rich Felker wrote:
> >>>>> On Wed, Apr 22, 2020 at 04:18:36PM +1000, Nicholas Piggin wrote:
> >>>>>> Yeah I had a bit of a play around with musl (which is very nice code I
> >>>>>> must say). The powerpc64 syscall asm is missing ctr clobber by the way.
> >>>>>> Fortunately adding it doesn't change code generation for me, but it
> >>>>>> should be fixed. glibc had the same bug at one point I think (probably
> >>>>>> due to syscall ABI documentation not existing -- something now lives in
> >>>>>> linux/Documentation/powerpc/syscall64-abi.rst).
> >>>>>
> >>>>> Do you know anywhere I can read about the ctr issue, possibly the
> >>>>> relevant glibc bug report? I'm not particularly familiar with ppc
> >>>>> register file (at least I have to refamiliarize myself every time I
> >>>>> work on this stuff) so it'd be nice to understand what's
> >>>>> potentially-wrong now.
> >>>>
> >>>> My understanding is the ctr issue only happens for vDSO calls where it
> >>>> fallback to a syscall in case an error (invalid argument, etc. and
> >>>> assuming if vDSO does not fallback to a syscall it always succeed).
> >>>> This makes the vDSO call on powerpc to have same same ABI constraint
> >>>> as a syscall, where it clobbers CR0.
> >>>
> >>> I think you mean "vsyscall", the old thing glibc used where there are
> >>> in-userspace implementations of some syscalls with call interfaces
> >>> roughly equivalent to a syscall. musl has never used this. It only
> >>> uses the actual exported functions from the vdso which have normal
> >>> external function call ABI.
> >>
> >> I wasn't thinking in vsyscall in fact, which afaik it is a x86 thing.
> >> The issue is indeed when calling the powerpc provided functions in
> >> vDSO, which musl might want to do eventually.
> >
> > AIUI (at least this is true for all other archs) the functions have
> > normal external function call ABI and calling them has nothing to do
> > with syscall mechanisms.
>
> My point is powerpc specifically does not follow it, since it issues a
> syscall in fallback and its semantic follow kernel syscalls (error
> signalled in cr0, r3 being always a positive value):
Oh, then I think we'll just ignore these unless the kernel can make
ones with a reasonable ABI. It's not worth having ppc-specific code
for this... It would be really nice if ones that actually behave like
functions could be added though.
> --
> V_FUNCTION_BEGIN(__kernel_clock_gettime)
> .cfi_startproc
> [...]
> /*
> * syscall fallback
> */
> 99:
> li r0,__NR_clock_gettime
> .cfi_restore lr
> sc
> blr
> .cfi_endproc
> V_FUNCTION_END(__kernel_clock_gettime)
>
>
> >
> > It looks like we're not using them right now and I'm not sure why. It
> > could be that there are ABI mismatch issues (are 32-bit ones
> > compatible with secure-plt? are 64-bit ones compatible with ELFv2?) or
> > just that nobody proposed adding them. Also as of 5.4 32-bit ppc
> > lacked time64 versions of them; not sure if this is fixed yet.
>
> For 64-bit it also have an issue where vDSO does not provide an OPD
> for ELFv1, which has bitten glibc while trying to implement an ifunc
> optimization. I don't recall any issue for ELFv2.
>
> For 32-bit I am not sure secure-plt will change anything, at least not
> on powerpc where we use the same strategy for 64-bit and use a
> mtctr/bctr directly.
Indeed, I don't think there's a secure-plt distinction unless you're
making outgoing calls to possibly-cross-DSO functions.
Rich
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Adhemerval Zanella @ 2020-04-23 17:15 UTC (permalink / raw)
To: Rich Felker; +Cc: libc-dev, libc-alpha, linuxppc-dev, Nicholas Piggin, musl
In-Reply-To: <20200423164314.GX11469@brightrain.aerifal.cx>
On 23/04/2020 13:43, Rich Felker wrote:
> On Thu, Apr 23, 2020 at 01:35:01PM -0300, Adhemerval Zanella wrote:
>>
>>
>> On 23/04/2020 13:18, Rich Felker wrote:
>>> On Thu, Apr 23, 2020 at 09:13:57AM -0300, Adhemerval Zanella wrote:
>>>>
>>>>
>>>> On 22/04/2020 23:36, Rich Felker wrote:
>>>>> On Wed, Apr 22, 2020 at 04:18:36PM +1000, Nicholas Piggin wrote:
>>>>>> Yeah I had a bit of a play around with musl (which is very nice code I
>>>>>> must say). The powerpc64 syscall asm is missing ctr clobber by the way.
>>>>>> Fortunately adding it doesn't change code generation for me, but it
>>>>>> should be fixed. glibc had the same bug at one point I think (probably
>>>>>> due to syscall ABI documentation not existing -- something now lives in
>>>>>> linux/Documentation/powerpc/syscall64-abi.rst).
>>>>>
>>>>> Do you know anywhere I can read about the ctr issue, possibly the
>>>>> relevant glibc bug report? I'm not particularly familiar with ppc
>>>>> register file (at least I have to refamiliarize myself every time I
>>>>> work on this stuff) so it'd be nice to understand what's
>>>>> potentially-wrong now.
>>>>
>>>> My understanding is the ctr issue only happens for vDSO calls where it
>>>> fallback to a syscall in case an error (invalid argument, etc. and
>>>> assuming if vDSO does not fallback to a syscall it always succeed).
>>>> This makes the vDSO call on powerpc to have same same ABI constraint
>>>> as a syscall, where it clobbers CR0.
>>>
>>> I think you mean "vsyscall", the old thing glibc used where there are
>>> in-userspace implementations of some syscalls with call interfaces
>>> roughly equivalent to a syscall. musl has never used this. It only
>>> uses the actual exported functions from the vdso which have normal
>>> external function call ABI.
>>
>> I wasn't thinking in vsyscall in fact, which afaik it is a x86 thing.
>> The issue is indeed when calling the powerpc provided functions in
>> vDSO, which musl might want to do eventually.
>
> AIUI (at least this is true for all other archs) the functions have
> normal external function call ABI and calling them has nothing to do
> with syscall mechanisms.
My point is powerpc specifically does not follow it, since it issues a
syscall in fallback and its semantic follow kernel syscalls (error
signalled in cr0, r3 being always a positive value):
--
V_FUNCTION_BEGIN(__kernel_clock_gettime)
.cfi_startproc
[...]
/*
* syscall fallback
*/
99:
li r0,__NR_clock_gettime
.cfi_restore lr
sc
blr
.cfi_endproc
V_FUNCTION_END(__kernel_clock_gettime)
>
> It looks like we're not using them right now and I'm not sure why. It
> could be that there are ABI mismatch issues (are 32-bit ones
> compatible with secure-plt? are 64-bit ones compatible with ELFv2?) or
> just that nobody proposed adding them. Also as of 5.4 32-bit ppc
> lacked time64 versions of them; not sure if this is fixed yet.
For 64-bit it also have an issue where vDSO does not provide an OPD
for ELFv1, which has bitten glibc while trying to implement an ifunc
optimization. I don't recall any issue for ELFv2.
For 32-bit I am not sure secure-plt will change anything, at least not
on powerpc where we use the same strategy for 64-bit and use a
mtctr/bctr directly.
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-23 16:43 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: libc-dev, libc-alpha, linuxppc-dev, Nicholas Piggin, musl
In-Reply-To: <3fe73604-7c92-e073-cbe7-abb4a8ae7c1a@linaro.org>
On Thu, Apr 23, 2020 at 01:35:01PM -0300, Adhemerval Zanella wrote:
>
>
> On 23/04/2020 13:18, Rich Felker wrote:
> > On Thu, Apr 23, 2020 at 09:13:57AM -0300, Adhemerval Zanella wrote:
> >>
> >>
> >> On 22/04/2020 23:36, Rich Felker wrote:
> >>> On Wed, Apr 22, 2020 at 04:18:36PM +1000, Nicholas Piggin wrote:
> >>>> Yeah I had a bit of a play around with musl (which is very nice code I
> >>>> must say). The powerpc64 syscall asm is missing ctr clobber by the way.
> >>>> Fortunately adding it doesn't change code generation for me, but it
> >>>> should be fixed. glibc had the same bug at one point I think (probably
> >>>> due to syscall ABI documentation not existing -- something now lives in
> >>>> linux/Documentation/powerpc/syscall64-abi.rst).
> >>>
> >>> Do you know anywhere I can read about the ctr issue, possibly the
> >>> relevant glibc bug report? I'm not particularly familiar with ppc
> >>> register file (at least I have to refamiliarize myself every time I
> >>> work on this stuff) so it'd be nice to understand what's
> >>> potentially-wrong now.
> >>
> >> My understanding is the ctr issue only happens for vDSO calls where it
> >> fallback to a syscall in case an error (invalid argument, etc. and
> >> assuming if vDSO does not fallback to a syscall it always succeed).
> >> This makes the vDSO call on powerpc to have same same ABI constraint
> >> as a syscall, where it clobbers CR0.
> >
> > I think you mean "vsyscall", the old thing glibc used where there are
> > in-userspace implementations of some syscalls with call interfaces
> > roughly equivalent to a syscall. musl has never used this. It only
> > uses the actual exported functions from the vdso which have normal
> > external function call ABI.
>
> I wasn't thinking in vsyscall in fact, which afaik it is a x86 thing.
> The issue is indeed when calling the powerpc provided functions in
> vDSO, which musl might want to do eventually.
AIUI (at least this is true for all other archs) the functions have
normal external function call ABI and calling them has nothing to do
with syscall mechanisms.
It looks like we're not using them right now and I'm not sure why. It
could be that there are ABI mismatch issues (are 32-bit ones
compatible with secure-plt? are 64-bit ones compatible with ELFv2?) or
just that nobody proposed adding them. Also as of 5.4 32-bit ppc
lacked time64 versions of them; not sure if this is fixed yet.
Rich
^ permalink raw reply
* Re: [PATCH] lib/mpi: Fix building for powerpc with clang
From: Nathan Chancellor @ 2020-04-23 16:36 UTC (permalink / raw)
To: Herbert Xu
Cc: kbuild test robot, linux-kernel, clang-built-linux,
Paul Mackerras, linux-crypto, linuxppc-dev
In-Reply-To: <20200414135731.GA8766@gondor.apana.org.au>
On Tue, Apr 14, 2020 at 11:57:31PM +1000, Herbert Xu wrote:
> On Mon, Apr 13, 2020 at 12:50:42PM -0700, Nathan Chancellor wrote:
> > 0day reports over and over on an powerpc randconfig with clang:
> >
> > lib/mpi/generic_mpih-mul1.c:37:13: error: invalid use of a cast in a
> > inline asm context requiring an l-value: remove the cast or build with
> > -fheinous-gnu-extensions
> >
> > Remove the superfluous casts, which have been done previously for x86
> > and arm32 in commit dea632cadd12 ("lib/mpi: fix build with clang") and
> > commit 7b7c1df2883d ("lib/mpi/longlong.h: fix building with 32-bit
> > x86").
> >
> > Reported-by: kbuild test robot <lkp@intel.com>
> > Link: https://github.com/ClangBuiltLinux/linux/issues/991
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Might be better for you to take this instead. 0day just tripped over
this again.
Cheers,
Nathan
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Adhemerval Zanella @ 2020-04-23 16:35 UTC (permalink / raw)
To: Rich Felker; +Cc: libc-dev, libc-alpha, linuxppc-dev, Nicholas Piggin, musl
In-Reply-To: <20200423161841.GU11469@brightrain.aerifal.cx>
On 23/04/2020 13:18, Rich Felker wrote:
> On Thu, Apr 23, 2020 at 09:13:57AM -0300, Adhemerval Zanella wrote:
>>
>>
>> On 22/04/2020 23:36, Rich Felker wrote:
>>> On Wed, Apr 22, 2020 at 04:18:36PM +1000, Nicholas Piggin wrote:
>>>> Yeah I had a bit of a play around with musl (which is very nice code I
>>>> must say). The powerpc64 syscall asm is missing ctr clobber by the way.
>>>> Fortunately adding it doesn't change code generation for me, but it
>>>> should be fixed. glibc had the same bug at one point I think (probably
>>>> due to syscall ABI documentation not existing -- something now lives in
>>>> linux/Documentation/powerpc/syscall64-abi.rst).
>>>
>>> Do you know anywhere I can read about the ctr issue, possibly the
>>> relevant glibc bug report? I'm not particularly familiar with ppc
>>> register file (at least I have to refamiliarize myself every time I
>>> work on this stuff) so it'd be nice to understand what's
>>> potentially-wrong now.
>>
>> My understanding is the ctr issue only happens for vDSO calls where it
>> fallback to a syscall in case an error (invalid argument, etc. and
>> assuming if vDSO does not fallback to a syscall it always succeed).
>> This makes the vDSO call on powerpc to have same same ABI constraint
>> as a syscall, where it clobbers CR0.
>
> I think you mean "vsyscall", the old thing glibc used where there are
> in-userspace implementations of some syscalls with call interfaces
> roughly equivalent to a syscall. musl has never used this. It only
> uses the actual exported functions from the vdso which have normal
> external function call ABI.
I wasn't thinking in vsyscall in fact, which afaik it is a x86 thing.
The issue is indeed when calling the powerpc provided functions in
vDSO, which musl might want to do eventually.
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-23 16:18 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: libc-dev, libc-alpha, linuxppc-dev, Nicholas Piggin, musl
In-Reply-To: <a972a0e4-25bd-9cf5-75ec-2eb993ef1b6a@linaro.org>
On Thu, Apr 23, 2020 at 09:13:57AM -0300, Adhemerval Zanella wrote:
>
>
> On 22/04/2020 23:36, Rich Felker wrote:
> > On Wed, Apr 22, 2020 at 04:18:36PM +1000, Nicholas Piggin wrote:
> >> Yeah I had a bit of a play around with musl (which is very nice code I
> >> must say). The powerpc64 syscall asm is missing ctr clobber by the way.
> >> Fortunately adding it doesn't change code generation for me, but it
> >> should be fixed. glibc had the same bug at one point I think (probably
> >> due to syscall ABI documentation not existing -- something now lives in
> >> linux/Documentation/powerpc/syscall64-abi.rst).
> >
> > Do you know anywhere I can read about the ctr issue, possibly the
> > relevant glibc bug report? I'm not particularly familiar with ppc
> > register file (at least I have to refamiliarize myself every time I
> > work on this stuff) so it'd be nice to understand what's
> > potentially-wrong now.
>
> My understanding is the ctr issue only happens for vDSO calls where it
> fallback to a syscall in case an error (invalid argument, etc. and
> assuming if vDSO does not fallback to a syscall it always succeed).
> This makes the vDSO call on powerpc to have same same ABI constraint
> as a syscall, where it clobbers CR0.
I think you mean "vsyscall", the old thing glibc used where there are
in-userspace implementations of some syscalls with call interfaces
roughly equivalent to a syscall. musl has never used this. It only
uses the actual exported functions from the vdso which have normal
external function call ABI.
Rich
^ permalink raw reply
* Re: [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()
From: Christophe Leroy @ 2020-04-23 16:21 UTC (permalink / raw)
To: Naveen N. Rao, linuxppc-dev; +Cc: Steven Rostedt
In-Reply-To: <b1dbbb34a389a6f59eb6c99102d94c0070ddaf98.1587654213.git.naveen.n.rao@linux.vnet.ibm.com>
Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> With STRICT_KERNEL_RWX, we are currently ignoring return value from
> __patch_instruction() in do_patch_instruction(), resulting in the error
> not being propagated back. Fix the same.
Good patch.
Be aware that there is ongoing work which tend to wanting to replace
error reporting by BUG_ON() . See
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=166003
>
> Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> arch/powerpc/lib/code-patching.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 3345f039a876..5c713a6c0bd8 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -138,7 +138,7 @@ static inline int unmap_patch_area(unsigned long addr)
>
> static int do_patch_instruction(unsigned int *addr, unsigned int instr)
> {
> - int err;
> + int err, rc = 0;
> unsigned int *patch_addr = NULL;
> unsigned long flags;
> unsigned long text_poke_addr;
> @@ -163,7 +163,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
> patch_addr = (unsigned int *)(text_poke_addr) +
> ((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
>
> - __patch_instruction(addr, instr, patch_addr);
> + rc = __patch_instruction(addr, instr, patch_addr);
>
> err = unmap_patch_area(text_poke_addr);
> if (err)
> @@ -172,7 +172,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
> out:
> local_irq_restore(flags);
>
> - return err;
> + return rc ? rc : err;
That's not really consistent. __patch_instruction() and
unmap_patch_area() return a valid minus errno, while in case of
map_patch_area() failure, err has value -1
> }
> #else /* !CONFIG_STRICT_KERNEL_RWX */
>
>
Christophe
^ permalink raw reply
* Re: [PATCH 2/3] powerpc/ftrace: Simplify error checking when patching instructions
From: Christophe Leroy @ 2020-04-23 15:44 UTC (permalink / raw)
To: Naveen N. Rao, linuxppc-dev; +Cc: Steven Rostedt
In-Reply-To: <872c5c3d9cf6db8e52b2abcdd16d7ab61fce8070.1587654213.git.naveen.n.rao@linux.vnet.ibm.com>
Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> Introduce a macro PATCH_INSN() to simplify instruction patching, and to
> make the error messages more uniform and useful:
> - print an error message that includes the original return value
> - print the function name and line numbers, so that the offending
> location is clear
> - always return -EPERM, which ftrace_bug() expects for proper error
> handling
>
> Also eliminate use of patch_branch() since most such uses already call
> create_branch() for error checking before patching. Instead, use the
> return value from create_branch() with PATCH_INSN().
I have the same comment here as for patch 3, this kind of macro hides
the return action and can be dangerous.
What about implementing a macro that takes an explicit label as third
argument and jump to that label in case of error ? On the same model as
unsafe_put_user() ?
Christophe
^ permalink raw reply
* Re: [PATCH 3/3] powerpc/kprobes: Check return value of patch_instruction()
From: Christophe Leroy @ 2020-04-23 15:41 UTC (permalink / raw)
To: Naveen N. Rao, linuxppc-dev; +Cc: Steven Rostedt
In-Reply-To: <3a132ac385340244b8d74179ac7bbbda7bf1f503.1587654213.git.naveen.n.rao@linux.vnet.ibm.com>
Le 23/04/2020 à 17:09, Naveen N. Rao a écrit :
> patch_instruction() can fail in some scenarios. Add appropriate error
> checking so that such failures are caught and logged, and suitable error
> code is returned.
>
> Fixes: d07df82c43be8 ("powerpc/kprobes: Move kprobes over to patch_instruction()")
> Fixes: f3eca95638931 ("powerpc/kprobes/optprobes: Use patch_instruction()")
> Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> arch/powerpc/kernel/kprobes.c | 10 +++-
> arch/powerpc/kernel/optprobes.c | 99 ++++++++++++++++++++++++++-------
> 2 files changed, 87 insertions(+), 22 deletions(-)
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 81efb605113e..4a297ae2bd87 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -138,13 +138,19 @@ NOKPROBE_SYMBOL(arch_prepare_kprobe);
>
> void arch_arm_kprobe(struct kprobe *p)
> {
> - patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
> + int rc = patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
> +
> + if (rc)
> + WARN("Failed to patch trap at 0x%pK: %d\n", (void *)p->addr, rc);
> }
> NOKPROBE_SYMBOL(arch_arm_kprobe);
>
> void arch_disarm_kprobe(struct kprobe *p)
> {
> - patch_instruction(p->addr, p->opcode);
> + int rc = patch_instruction(p->addr, p->opcode);
> +
> + if (rc)
> + WARN("Failed to remove trap at 0x%pK: %d\n", (void *)p->addr, rc);
> }
> NOKPROBE_SYMBOL(arch_disarm_kprobe);
>
> diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
> index 024f7aad1952..046485bb0a52 100644
> --- a/arch/powerpc/kernel/optprobes.c
> +++ b/arch/powerpc/kernel/optprobes.c
> @@ -139,52 +139,67 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
> }
> }
>
> +#define PATCH_INSN(addr, instr) \
> +do { \
> + int rc = patch_instruction((unsigned int *)(addr), instr); \
> + if (rc) { \
> + pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n", \
> + __func__, __LINE__, \
> + (void *)(addr), (void *)(addr), rc); \
> + return rc; \
> + } \
> +} while (0)
> +
I hate this kind of macro which hides the "return".
What about keeping the return action in the caller ?
Otherwise, what about implementing something based on the use of goto,
on the same model as unsafe_put_user() for instance ?
Christophe
^ permalink raw reply
* [PATCH 3/3] powerpc/hw_bkpt: Update printk format specifiers for kernel addresses
From: Naveen N. Rao @ 2020-04-23 15:17 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Ravi Bangoria, Balamuruhan S
In-Reply-To: <cover.1587652966.git.naveen.n.rao@linux.vnet.ibm.com>
Change prinkt format specifier from %lx to %pK to indicate kernel
pointer, and to hide the addresses from unprivileged users.
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/hw_breakpoint.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 72f461bd70fb..93a303cf0c67 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -257,7 +257,8 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
if (!ret && (type == LARX || type == STCX)) {
printk_ratelimited("Breakpoint hit on instruction that can't be emulated."
- " Breakpoint at 0x%lx will be disabled.\n", addr);
+ " Breakpoint at 0x%pK will be disabled.\n",
+ (void *)addr);
goto disable;
}
@@ -286,7 +287,7 @@ static bool stepping_handler(struct pt_regs *regs, struct perf_event *bp,
* it and throw a warning message to let the user know about it.
*/
WARN(1, "Unable to handle hardware breakpoint. Breakpoint at "
- "0x%lx will be disabled.", addr);
+ "0x%pK will be disabled.", (void *)addr);
disable:
perf_event_disable_inatomic(bp);
--
2.25.1
^ permalink raw reply related
* [PATCH 1/3] powerpc/kprobes: Use appropriate format specifier for printing kernel address
From: Naveen N. Rao @ 2020-04-23 15:17 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Ravi Bangoria, Balamuruhan S
In-Reply-To: <cover.1587652966.git.naveen.n.rao@linux.vnet.ibm.com>
From: Balamuruhan S <bala24@linux.ibm.com>
Change use of %p to %pK when printing address of the instruction slot so
that the actual kernel address is visible for privileged users.
Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/optprobes.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index ef0924b0809d..d5f8c25b7cac 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -247,7 +247,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
/* Setup template */
/* We can optimize this via patch_instruction_window later */
size = (TMPL_END_IDX * sizeof(kprobe_opcode_t)) / sizeof(int);
- pr_devel("Copying template to %p, size %lu\n", buff, size);
+ pr_devel("Copying template to %pK, size %lu\n", (void *)buff, size);
for (i = 0; i < size; i++) {
rc = patch_instruction(buff + i, *(optprobe_template_entry + i));
if (rc) {
--
2.25.1
^ permalink raw reply related
* [PATCH 2/3] powerpc/ftrace: Use appropriate format specifier for printing kernel addresses
From: Naveen N. Rao @ 2020-04-23 15:17 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Ravi Bangoria, Balamuruhan S
In-Reply-To: <cover.1587652966.git.naveen.n.rao@linux.vnet.ibm.com>
Update use of printk format specifiers in ftrace code, so that addresses
are made visible for privileged users, or always for pr_devel() code:
- change %lx to use %px or %pK
- change %p to %px or %pK
- add %pS in certain places to show the symbol as well
Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/trace/ftrace.c | 74 +++++++++++++++++-------------
1 file changed, 41 insertions(+), 33 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 679d5249b002..29b77204f46d 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -83,8 +83,8 @@ ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new)
/* Make sure it is what we expect it to be */
if (replaced != old) {
- pr_err("%p: replaced (%#x) != old (%#x)",
- (void *)ip, replaced, old);
+ pr_err("%pK (%pS): replaced (%#x) != old (%#x)",
+ (void *)ip, (void *)ip, replaced, old);
return -EINVAL;
}
@@ -152,19 +152,20 @@ __ftrace_make_nop(struct module *mod,
/* lets find where the pointer goes */
tramp = find_bl_target(ip, op);
- pr_devel("ip:%lx jumps to %lx", ip, tramp);
+ pr_devel("ip:0x%px jumps to 0x%px", (void *)ip, (void *)tramp);
if (module_trampoline_target(mod, tramp, &ptr)) {
pr_err("Failed to get trampoline target\n");
return -EFAULT;
}
- pr_devel("trampoline target %lx", ptr);
+ pr_devel("trampoline target 0x%px", (void *)ptr);
entry = ppc_global_function_entry((void *)addr);
/* This should match what was called */
if (ptr != entry) {
- pr_err("addr %lx does not match expected %lx\n", ptr, entry);
+ pr_err("addr 0x%pK does not match expected 0x%pK\n",
+ (void *)ptr, (void *)entry);
return -EINVAL;
}
@@ -173,7 +174,8 @@ __ftrace_make_nop(struct module *mod,
pop = PPC_INST_NOP;
if (probe_kernel_read(&op, (void *)(ip - 4), 4)) {
- pr_err("Fetching instruction at %lx failed.\n", ip - 4);
+ pr_err("Fetching instruction at 0x%pK (%pS) failed.\n",
+ (void *)(ip - 4), (void *)(ip - 4));
return -EFAULT;
}
@@ -249,11 +251,11 @@ __ftrace_make_nop(struct module *mod,
* 0x4e, 0x80, 0x04, 0x20 bctr
*/
- pr_devel("ip:%lx jumps to %lx", ip, tramp);
+ pr_devel("ip:0x%px jumps to 0x%px", (void *)ip, (void *)tramp);
/* Find where the trampoline jumps to */
if (probe_kernel_read(jmp, (void *)tramp, sizeof(jmp))) {
- pr_err("Failed to read %lx\n", tramp);
+ pr_err("Failed to read 0x%pK\n", (void *)tramp);
return -EFAULT;
}
@@ -273,11 +275,11 @@ __ftrace_make_nop(struct module *mod,
if (tramp & 0x8000)
tramp -= 0x10000;
- pr_devel(" %lx ", tramp);
+ pr_devel(" 0x%px ", (void *)tramp);
if (tramp != addr) {
- pr_err("Trampoline location %08lx does not match addr\n",
- tramp);
+ pr_err("Trampoline location 0x%pK does not match addr\n",
+ (void *)tramp);
return -EINVAL;
}
@@ -362,7 +364,8 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
ptr = find_bl_target(tramp, op);
if (ptr != ppc_global_function_entry((void *)_mcount)) {
- pr_debug("Trampoline target %p is not _mcount\n", (void *)ptr);
+ pr_debug("Trampoline target 0x%px (%pS) is not _mcount\n",
+ (void *)ptr, (void *)ptr);
return -1;
}
@@ -374,8 +377,8 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
#endif
op = create_branch((void *)tramp, ptr, 0);
if (!op) {
- pr_debug("%ps is not reachable from existing mcount tramp\n",
- (void *)ptr);
+ pr_debug("0x%px (%ps) is not reachable from existing mcount tramp\n",
+ (void *)ptr, (void *)ptr);
return -1;
}
@@ -409,13 +412,13 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr)
/* Let's find where the pointer goes */
tramp = find_bl_target(ip, op);
- pr_devel("ip:%lx jumps to %lx", ip, tramp);
+ pr_devel("ip:0x%px jumps to 0x%px", (void *)ip, (void *)tramp);
if (setup_mcount_compiler_tramp(tramp)) {
/* Are other trampolines reachable? */
if (!find_ftrace_tramp(ip)) {
- pr_err("No ftrace trampolines reachable from %ps\n",
- (void *)ip);
+ pr_err("No ftrace trampolines reachable from 0x%pK (%pS)\n",
+ (void *)ip, (void *)ip);
return -EINVAL;
}
}
@@ -452,13 +455,13 @@ int ftrace_make_nop(struct module *mod,
*/
if (!rec->arch.mod) {
if (!mod) {
- pr_err("No module loaded addr=%lx\n", addr);
+ pr_err("No module loaded addr=0x%pK\n", (void *)addr);
return -EFAULT;
}
rec->arch.mod = mod;
} else if (mod) {
if (mod != rec->arch.mod) {
- pr_err("Record mod %p not equal to passed in mod %p\n",
+ pr_err("Record mod %pK not equal to passed in mod %pK\n",
rec->arch.mod, mod);
return -EINVAL;
}
@@ -521,8 +524,8 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
return -EFAULT;
if (!expected_nop_sequence(ip, op[0], op[1])) {
- pr_err("Unexpected call sequence at %p: %x %x\n",
- ip, op[0], op[1]);
+ pr_err("Unexpected call sequence at %pK: %x %x\n",
+ ip, op[0], op[1]);
return -EINVAL;
}
@@ -548,12 +551,13 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
return -EFAULT;
}
- pr_devel("trampoline target %lx", ptr);
+ pr_devel("trampoline target 0x%px", (void *)ptr);
entry = ppc_global_function_entry((void *)addr);
/* This should match what was called */
if (ptr != entry) {
- pr_err("addr %lx does not match expected %lx\n", ptr, entry);
+ pr_err("addr 0x%pK does not match expected 0x%pK\n",
+ (void *)ptr, (void *)entry);
return -EINVAL;
}
@@ -600,7 +604,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
return -EINVAL;
}
- pr_devel("write to %lx\n", rec->ip);
+ pr_devel("write to 0x%px\n", (void *)rec->ip);
PATCH_INSN(ip, op);
@@ -624,7 +628,8 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr)
entry = ppc_global_function_entry((void *)ftrace_regs_caller);
if (ptr != entry) {
#endif
- pr_err("Unknown ftrace addr to patch: %ps\n", (void *)ptr);
+ pr_err("Unknown ftrace addr to patch: 0x%pK (%pS)\n",
+ (void *)ptr, (void *)ptr);
return -EINVAL;
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
}
@@ -633,18 +638,19 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr)
/* Make sure we have a nop */
if (probe_kernel_read(&op, ip, sizeof(op))) {
- pr_err("Unable to read ftrace location %p\n", ip);
+ pr_err("Unable to read ftrace location %pK\n", ip);
return -EFAULT;
}
if (op != PPC_INST_NOP) {
- pr_err("Unexpected call sequence at %p: %x\n", ip, op);
+ pr_err("Unexpected call sequence at %pK: %x\n", ip, op);
return -EINVAL;
}
tramp = find_ftrace_tramp((unsigned long)ip);
if (!tramp) {
- pr_err("No ftrace trampolines reachable from %ps\n", ip);
+ pr_err("No ftrace trampolines reachable from 0x%pK (%pS)\n",
+ ip, ip);
return -EINVAL;
}
@@ -728,7 +734,7 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
tramp = find_bl_target(ip, op);
entry = ppc_global_function_entry((void *)old_addr);
- pr_devel("ip:%lx jumps to %lx", ip, tramp);
+ pr_devel("ip:0x%px jumps to 0x%px", (void *)ip, (void *)tramp);
if (tramp != entry) {
/* old_addr is not within range, so we must have used a trampoline */
@@ -737,11 +743,12 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
return -EFAULT;
}
- pr_devel("trampoline target %lx", ptr);
+ pr_devel("trampoline target 0x%px", (void *)ptr);
/* This should match what was called */
if (ptr != entry) {
- pr_err("addr %lx does not match expected %lx\n", ptr, entry);
+ pr_err("addr 0x%pK does not match expected 0x%pK\n",
+ (void *)ptr, (void *)entry);
return -EINVAL;
}
}
@@ -765,12 +772,13 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
return -EFAULT;
}
- pr_devel("trampoline target %lx", ptr);
+ pr_devel("trampoline target 0x%px", (void *)ptr);
entry = ppc_global_function_entry((void *)addr);
/* This should match what was called */
if (ptr != entry) {
- pr_err("addr %lx does not match expected %lx\n", ptr, entry);
+ pr_err("addr 0x%pK does not match expected 0x%pK\n",
+ (void *)ptr, (void *)entry);
return -EINVAL;
}
--
2.25.1
^ permalink raw reply related
* [PATCH 0/3] powerpc: Use proper printk format specifiers
From: Naveen N. Rao @ 2020-04-23 15:17 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Ravi Bangoria, Balamuruhan S
This series changes printk format specifiers from bare %p to %px/%pK in
ftrace, kprobes and hw bkpts code. In addition, use of %lx is also
changed over to conform to the recommended practice.
This series applies on top of the below patch series:
https://lore.kernel.org/r/cover.1587654213.git.naveen.n.rao@linux.vnet.ibm.com
- Naveen
Balamuruhan S (1):
powerpc/kprobes: Use appropriate format specifier for printing kernel
address
Naveen N. Rao (2):
powerpc/ftrace: Use appropriate format specifier for printing kernel
addresses
powerpc/hw_bkpt: Update printk format specifiers for kernel addresses
arch/powerpc/kernel/hw_breakpoint.c | 5 +-
arch/powerpc/kernel/optprobes.c | 2 +-
arch/powerpc/kernel/trace/ftrace.c | 74 ++++++++++++++++-------------
3 files changed, 45 insertions(+), 36 deletions(-)
base-commit: 8299da600ad05b8aa0f15ec0f5f03bd40e37d6f0
-- 2.25.1
^ permalink raw reply
* [PATCH 1/3] powerpc: Properly return error code from do_patch_instruction()
From: Naveen N. Rao @ 2020-04-23 15:09 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Steven Rostedt
In-Reply-To: <cover.1587654213.git.naveen.n.rao@linux.vnet.ibm.com>
With STRICT_KERNEL_RWX, we are currently ignoring return value from
__patch_instruction() in do_patch_instruction(), resulting in the error
not being propagated back. Fix the same.
Fixes: 37bc3e5fd764f ("powerpc/lib/code-patching: Use alternate map for patch_instruction()")
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/lib/code-patching.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 3345f039a876..5c713a6c0bd8 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -138,7 +138,7 @@ static inline int unmap_patch_area(unsigned long addr)
static int do_patch_instruction(unsigned int *addr, unsigned int instr)
{
- int err;
+ int err, rc = 0;
unsigned int *patch_addr = NULL;
unsigned long flags;
unsigned long text_poke_addr;
@@ -163,7 +163,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
patch_addr = (unsigned int *)(text_poke_addr) +
((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
- __patch_instruction(addr, instr, patch_addr);
+ rc = __patch_instruction(addr, instr, patch_addr);
err = unmap_patch_area(text_poke_addr);
if (err)
@@ -172,7 +172,7 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
out:
local_irq_restore(flags);
- return err;
+ return rc ? rc : err;
}
#else /* !CONFIG_STRICT_KERNEL_RWX */
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v2 2/2] PCI/DPC: Allow Native DPC Host Bridges to use DPC
From: Derrick, Jonathan @ 2020-04-23 15:11 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy@linux.intel.com, helgaas@kernel.org
Cc: bhelgaas@google.com, Patel, Mayurkumar, fred@fredlawl.com,
sbobroff@linux.ibm.com, linuxppc-dev@lists.ozlabs.org,
Wysocki, Rafael J, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, andriy.shevchenko@linux.intel.com,
olof@lixom.net, alex.williamson@redhat.com, oohall@gmail.com,
kbusch@kernel.org, rajatja@google.com,
mika.westerberg@linux.intel.com
In-Reply-To: <0058b993-0663-7fed-ed31-cb0adf845a39@linux.intel.com>
Hi Sathyanarayanan,
On Wed, 2020-04-22 at 15:50 -0700, Kuppuswamy, Sathyanarayanan wrote:
>
> On 4/20/20 2:37 PM, Jon Derrick wrote:
> > The existing portdrv model prevents DPC services without either OS
> > control (_OSC) granted to AER services, a Host Bridge requesting Native
> > AER, or using one of the 'pcie_ports=' parameters of 'native' or
> > 'dpc-native'.
> >
> > The DPC port service driver itself will also fail to probe if the kernel
> > assumes the port is using Firmware-First AER. It's a reasonable
> > expectation that a port using Firmware-First AER will also be using
> > Firmware-First DPC, however if a Host Bridge requests Native DPC, the
> > DPC driver should allow it and not fail to bind due to AER capability
> > settings.
> >
> > Host Bridges which request Native DPC port services will also likely
> > request Native AER, however it shouldn't be a requirement. This patch
> > allows ports on those Host Bridges to have DPC port services.
> >
> > This will avoid the unlikely situation where the port is Firmware-First
> > AER and Native DPC, and a BIOS or switch firmware preconfiguration of
> > the DPC trigger could result in unhandled DPC events.
> >
> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> > drivers/pci/pcie/dpc.c | 3 ++-
> > drivers/pci/pcie/portdrv_core.c | 3 ++-
> > 2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index 7621704..3f3106f 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -284,7 +284,8 @@ static int dpc_probe(struct pcie_device *dev)
> > int status;
> > u16 ctl, cap;
> >
> > - if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
> > + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native &&
> > + !pci_find_host_bridge(pdev->bus)->native_dpc)
> Why do it in probe as well ? if host->native_dpc is not set then the
> device DPC probe it self won't happen right ?
Portdrv only enables the interrupt and allows the probe to occur.
The probe itself will still fail if there's a mixed-mode _OSC
negotiated AER & DPC, due to pcie_aer_get_firmware_first returning 1
for AER and no check for DPC.
I don't know if such a platform will exist, but the kernel is already
wired for 'dpc-native' so it makes sense to extend it for this..
This transform might be more readable:
if (pcie_aer_get_firmware_first(pdev) &&
!(pcie_ports_dpc_native || hb->native_dpc))
> > return -ENOTSUPP;
> >
> > status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
> > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> > index 50a9522..f2139a1 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -256,7 +256,8 @@ static int get_port_device_capability(struct pci_dev *dev)
> > */
> > if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC) &&
> > pci_aer_available() &&
> > - (pcie_ports_dpc_native || (services & PCIE_PORT_SERVICE_AER)))
> > + (pcie_ports_dpc_native || host->native_dpc ||
> > + (services & PCIE_PORT_SERVICE_AER)))
> > services |= PCIE_PORT_SERVICE_DPC;
> >
> > if (pci_pcie_type(dev) == PCI_EXP_TYPE_DOWNSTREAM ||
> >
^ permalink raw reply
* Re: [PATCH v2 1/2] PCI/AER: Allow Native AER Host Bridges to use AER
From: Derrick, Jonathan @ 2020-04-23 15:11 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy@linux.intel.com, helgaas@kernel.org
Cc: bhelgaas@google.com, Patel, Mayurkumar, fred@fredlawl.com,
sbobroff@linux.ibm.com, linuxppc-dev@lists.ozlabs.org,
Wysocki, Rafael J, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org, andriy.shevchenko@linux.intel.com,
olof@lixom.net, alex.williamson@redhat.com, oohall@gmail.com,
kbusch@kernel.org, rajatja@google.com,
mika.westerberg@linux.intel.com
In-Reply-To: <9f8c2a62-e67d-2869-db11-4644b69815f4@linux.intel.com>
Hi Sathyanarayanan,
On Wed, 2020-04-22 at 15:48 -0700, Kuppuswamy, Sathyanarayanan wrote:
>
> On 4/20/20 2:37 PM, Jon Derrick wrote:
> > Some platforms have a mix of ports whose capabilities can be negotiated
> > by _OSC, and some ports which are not described by ACPI and instead
> > managed by Native drivers. The existing Firmware-First HEST model can
> > incorrectly tag these Native, Non-ACPI ports as Firmware-First managed
> > ports by advertising the HEST Global Flag and matching the type and
> > class of the port (aer_hest_parse).
> Is there a real use case for mixed mode (one host bridge in FF mode and
> another in native)?
Intel's VMD exposes PCIe segments containing Root Ports and Bridges and
other DPC consumers. These extra PCIe domains aren't described by ACPI.
There have been a few versions where DPC won't bind due to platform's
HEST configuration.
> > If the port requests Native AER through the Host Bridge's capability
> > settings, the AER driver should honor those settings and allow the port
> > to bind. This patch changes the definition of Firmware-First to exclude
> > ports whose Host Bridges request Native AER.
> >
> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> > drivers/pci/pcie/aer.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index f4274d3..30fbd1f 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -314,6 +314,9 @@ int pcie_aer_get_firmware_first(struct pci_dev *dev)
> > if (pcie_ports_native)
> > return 0;
> >
> > + if (pci_find_host_bridge(dev->bus)->native_aer)
> > + return 0;
> > +
> > if (!dev->__aer_firmware_first_valid)
> > aer_set_firmware_first(dev);
> > return dev->__aer_firmware_first;
> >
^ permalink raw reply
* [PATCH 2/3] powerpc/ftrace: Simplify error checking when patching instructions
From: Naveen N. Rao @ 2020-04-23 15:09 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Steven Rostedt
In-Reply-To: <cover.1587654213.git.naveen.n.rao@linux.vnet.ibm.com>
Introduce a macro PATCH_INSN() to simplify instruction patching, and to
make the error messages more uniform and useful:
- print an error message that includes the original return value
- print the function name and line numbers, so that the offending
location is clear
- always return -EPERM, which ftrace_bug() expects for proper error
handling
Also eliminate use of patch_branch() since most such uses already call
create_branch() for error checking before patching. Instead, use the
return value from create_branch() with PATCH_INSN().
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/trace/ftrace.c | 69 ++++++++++++++----------------
1 file changed, 33 insertions(+), 36 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 7ea0ca044b65..5cf84c0c64cb 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -31,6 +31,17 @@
#ifdef CONFIG_DYNAMIC_FTRACE
+#define PATCH_INSN(addr, instr) \
+do { \
+ int rc = patch_instruction((unsigned int *)(addr), instr); \
+ if (rc) { \
+ pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n", \
+ __func__, __LINE__, \
+ (void *)(addr), (void *)(addr), rc); \
+ return -EPERM; \
+ } \
+} while (0)
+
/*
* We generally only have a single long_branch tramp and at most 2 or 3 plt
* tramps generated. But, we don't use the plt tramps currently. We also allot
@@ -78,8 +89,7 @@ ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new)
}
/* replace the text with the new text */
- if (patch_instruction((unsigned int *)ip, new))
- return -EPERM;
+ PATCH_INSN(ip, new);
return 0;
}
@@ -204,10 +214,7 @@ __ftrace_make_nop(struct module *mod,
}
#endif /* CONFIG_MPROFILE_KERNEL */
- if (patch_instruction((unsigned int *)ip, pop)) {
- pr_err("Patching NOP failed.\n");
- return -EPERM;
- }
+ PATCH_INSN(ip, pop);
return 0;
}
@@ -276,8 +283,7 @@ __ftrace_make_nop(struct module *mod,
op = PPC_INST_NOP;
- if (patch_instruction((unsigned int *)ip, op))
- return -EPERM;
+ PATCH_INSN(ip, op);
return 0;
}
@@ -322,7 +328,7 @@ static int add_ftrace_tramp(unsigned long tramp)
*/
static int setup_mcount_compiler_tramp(unsigned long tramp)
{
- int i, op;
+ unsigned int i, op;
unsigned long ptr;
static unsigned long ftrace_plt_tramps[NUM_FTRACE_TRAMPS];
@@ -366,16 +372,14 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
#else
ptr = ppc_global_function_entry((void *)ftrace_caller);
#endif
- if (!create_branch((void *)tramp, ptr, 0)) {
+ op = create_branch((void *)tramp, ptr, 0);
+ if (!op) {
pr_debug("%ps is not reachable from existing mcount tramp\n",
(void *)ptr);
return -1;
}
- if (patch_branch((unsigned int *)tramp, ptr, 0)) {
- pr_debug("REL24 out of range!\n");
- return -1;
- }
+ PATCH_INSN(tramp, op);
if (add_ftrace_tramp(tramp)) {
pr_debug("No tramp locations left\n");
@@ -416,10 +420,7 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace *rec, unsigned long addr)
}
}
- if (patch_instruction((unsigned int *)ip, PPC_INST_NOP)) {
- pr_err("Patching NOP failed.\n");
- return -EPERM;
- }
+ PATCH_INSN(ip, PPC_INST_NOP);
return 0;
}
@@ -557,15 +558,13 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
}
/* Ensure branch is within 24 bits */
- if (!create_branch(ip, tramp, BRANCH_SET_LINK)) {
+ op[0] = create_branch(ip, tramp, BRANCH_SET_LINK);
+ if (!op[0]) {
pr_err("Branch out of range\n");
return -EINVAL;
}
- if (patch_branch(ip, tramp, BRANCH_SET_LINK)) {
- pr_err("REL24 out of range!\n");
- return -EINVAL;
- }
+ PATCH_INSN(ip, op[0]);
return 0;
}
@@ -603,8 +602,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
pr_devel("write to %lx\n", rec->ip);
- if (patch_instruction((unsigned int *)ip, op))
- return -EPERM;
+ PATCH_INSN(ip, op);
return 0;
}
@@ -650,11 +648,14 @@ static int __ftrace_make_call_kernel(struct dyn_ftrace *rec, unsigned long addr)
return -EINVAL;
}
- if (patch_branch(ip, tramp, BRANCH_SET_LINK)) {
- pr_err("Error patching branch to ftrace tramp!\n");
+ op = create_branch(ip, tramp, BRANCH_SET_LINK);
+ if (!op) {
+ pr_err("Branch out of range\n");
return -EINVAL;
}
+ PATCH_INSN(ip, op);
+
return 0;
}
@@ -748,10 +749,8 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
/* The new target may be within range */
if (test_24bit_addr(ip, addr)) {
/* within range */
- if (patch_branch((unsigned int *)ip, addr, BRANCH_SET_LINK)) {
- pr_err("REL24 out of range!\n");
- return -EINVAL;
- }
+ op = create_branch((unsigned int *)ip, addr, BRANCH_SET_LINK);
+ PATCH_INSN(ip, op);
return 0;
}
@@ -776,15 +775,13 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
}
/* Ensure branch is within 24 bits */
- if (!create_branch((unsigned int *)ip, tramp, BRANCH_SET_LINK)) {
+ op = create_branch((unsigned int *)ip, tramp, BRANCH_SET_LINK);
+ if (!op) {
pr_err("Branch out of range\n");
return -EINVAL;
}
- if (patch_branch((unsigned int *)ip, tramp, BRANCH_SET_LINK)) {
- pr_err("REL24 out of range!\n");
- return -EINVAL;
- }
+ PATCH_INSN(ip, op);
return 0;
}
--
2.25.1
^ permalink raw reply related
* [PATCH 3/3] powerpc/kprobes: Check return value of patch_instruction()
From: Naveen N. Rao @ 2020-04-23 15:09 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Steven Rostedt
In-Reply-To: <cover.1587654213.git.naveen.n.rao@linux.vnet.ibm.com>
patch_instruction() can fail in some scenarios. Add appropriate error
checking so that such failures are caught and logged, and suitable error
code is returned.
Fixes: d07df82c43be8 ("powerpc/kprobes: Move kprobes over to patch_instruction()")
Fixes: f3eca95638931 ("powerpc/kprobes/optprobes: Use patch_instruction()")
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/kernel/kprobes.c | 10 +++-
arch/powerpc/kernel/optprobes.c | 99 ++++++++++++++++++++++++++-------
2 files changed, 87 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 81efb605113e..4a297ae2bd87 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -138,13 +138,19 @@ NOKPROBE_SYMBOL(arch_prepare_kprobe);
void arch_arm_kprobe(struct kprobe *p)
{
- patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
+ int rc = patch_instruction(p->addr, BREAKPOINT_INSTRUCTION);
+
+ if (rc)
+ WARN("Failed to patch trap at 0x%pK: %d\n", (void *)p->addr, rc);
}
NOKPROBE_SYMBOL(arch_arm_kprobe);
void arch_disarm_kprobe(struct kprobe *p)
{
- patch_instruction(p->addr, p->opcode);
+ int rc = patch_instruction(p->addr, p->opcode);
+
+ if (rc)
+ WARN("Failed to remove trap at 0x%pK: %d\n", (void *)p->addr, rc);
}
NOKPROBE_SYMBOL(arch_disarm_kprobe);
diff --git a/arch/powerpc/kernel/optprobes.c b/arch/powerpc/kernel/optprobes.c
index 024f7aad1952..046485bb0a52 100644
--- a/arch/powerpc/kernel/optprobes.c
+++ b/arch/powerpc/kernel/optprobes.c
@@ -139,52 +139,67 @@ void arch_remove_optimized_kprobe(struct optimized_kprobe *op)
}
}
+#define PATCH_INSN(addr, instr) \
+do { \
+ int rc = patch_instruction((unsigned int *)(addr), instr); \
+ if (rc) { \
+ pr_err("%s:%d Error patching instruction at 0x%pK (%pS): %d\n", \
+ __func__, __LINE__, \
+ (void *)(addr), (void *)(addr), rc); \
+ return rc; \
+ } \
+} while (0)
+
/*
* emulate_step() requires insn to be emulated as
* second parameter. Load register 'r4' with the
* instruction.
*/
-void patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr)
+static int patch_imm32_load_insns(unsigned int val, kprobe_opcode_t *addr)
{
/* addis r4,0,(insn)@h */
- patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(4) |
+ PATCH_INSN(addr, PPC_INST_ADDIS | ___PPC_RT(4) |
((val >> 16) & 0xffff));
addr++;
/* ori r4,r4,(insn)@l */
- patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(4) |
+ PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(4) |
___PPC_RS(4) | (val & 0xffff));
+
+ return 0;
}
/*
* Generate instructions to load provided immediate 64-bit value
* to register 'r3' and patch these instructions at 'addr'.
*/
-void patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
+static int patch_imm64_load_insns(unsigned long val, kprobe_opcode_t *addr)
{
/* lis r3,(op)@highest */
- patch_instruction(addr, PPC_INST_ADDIS | ___PPC_RT(3) |
+ PATCH_INSN(addr, PPC_INST_ADDIS | ___PPC_RT(3) |
((val >> 48) & 0xffff));
addr++;
/* ori r3,r3,(op)@higher */
- patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(3) |
+ PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(3) |
___PPC_RS(3) | ((val >> 32) & 0xffff));
addr++;
/* rldicr r3,r3,32,31 */
- patch_instruction(addr, PPC_INST_RLDICR | ___PPC_RA(3) |
+ PATCH_INSN(addr, PPC_INST_RLDICR | ___PPC_RA(3) |
___PPC_RS(3) | __PPC_SH64(32) | __PPC_ME64(31));
addr++;
/* oris r3,r3,(op)@h */
- patch_instruction(addr, PPC_INST_ORIS | ___PPC_RA(3) |
+ PATCH_INSN(addr, PPC_INST_ORIS | ___PPC_RA(3) |
___PPC_RS(3) | ((val >> 16) & 0xffff));
addr++;
/* ori r3,r3,(op)@l */
- patch_instruction(addr, PPC_INST_ORI | ___PPC_RA(3) |
+ PATCH_INSN(addr, PPC_INST_ORI | ___PPC_RA(3) |
___PPC_RS(3) | (val & 0xffff));
+
+ return 0;
}
int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
@@ -216,14 +231,18 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
* be within 32MB on either side of the current instruction.
*/
b_offset = (unsigned long)buff - (unsigned long)p->addr;
- if (!is_offset_in_branch_range(b_offset))
+ if (!is_offset_in_branch_range(b_offset)) {
+ rc = -ERANGE;
goto error;
+ }
/* Check if the return address is also within 32MB range */
b_offset = (unsigned long)(buff + TMPL_RET_IDX) -
(unsigned long)nip;
- if (!is_offset_in_branch_range(b_offset))
+ if (!is_offset_in_branch_range(b_offset)) {
+ rc = -ERANGE;
goto error;
+ }
/* Setup template */
/* We can optimize this via patch_instruction_window later */
@@ -231,15 +250,22 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
pr_devel("Copying template to %p, size %lu\n", buff, size);
for (i = 0; i < size; i++) {
rc = patch_instruction(buff + i, *(optprobe_template_entry + i));
- if (rc < 0)
+ if (rc) {
+ pr_err("%s: Error copying optprobe template to 0x%pK: %d\n",
+ __func__, (void *)(buff + i), rc);
+ rc = -EFAULT;
goto error;
+ }
}
/*
* Fixup the template with instructions to:
* 1. load the address of the actual probepoint
*/
- patch_imm64_load_insns((unsigned long)op, buff + TMPL_OP_IDX);
+ if (patch_imm64_load_insns((unsigned long)op, buff + TMPL_OP_IDX)) {
+ rc = -EFAULT;
+ goto error;
+ }
/*
* 2. branch to optimized_callback() and emulate_step()
@@ -248,6 +274,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
emulate_step_addr = (kprobe_opcode_t *)ppc_kallsyms_lookup_name("emulate_step");
if (!op_callback_addr || !emulate_step_addr) {
WARN(1, "Unable to lookup optimized_callback()/emulate_step()\n");
+ rc = -ERANGE;
goto error;
}
@@ -259,21 +286,48 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
(unsigned long)emulate_step_addr,
BRANCH_SET_LINK);
- if (!branch_op_callback || !branch_emulate_step)
+ if (!branch_op_callback || !branch_emulate_step) {
+ rc = -ERANGE;
goto error;
+ }
- patch_instruction(buff + TMPL_CALL_HDLR_IDX, branch_op_callback);
- patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step);
+ rc = patch_instruction(buff + TMPL_CALL_HDLR_IDX, branch_op_callback);
+ if (rc) {
+ pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
+ __func__, __LINE__,
+ (void *)(buff + TMPL_CALL_HDLR_IDX), rc);
+ rc = -EFAULT;
+ goto error;
+ }
+
+ rc = patch_instruction(buff + TMPL_EMULATE_IDX, branch_emulate_step);
+ if (rc) {
+ pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
+ __func__, __LINE__,
+ (void *)(buff + TMPL_EMULATE_IDX), rc);
+ rc = -EFAULT;
+ goto error;
+ }
/*
* 3. load instruction to be emulated into relevant register, and
*/
- patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX);
+ if (patch_imm32_load_insns(*p->ainsn.insn, buff + TMPL_INSN_IDX)) {
+ rc = -EFAULT;
+ goto error;
+ }
/*
* 4. branch back from trampoline
*/
- patch_branch(buff + TMPL_RET_IDX, (unsigned long)nip, 0);
+ rc = patch_branch(buff + TMPL_RET_IDX, (unsigned long)nip, 0);
+ if (rc) {
+ pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
+ __func__, __LINE__,
+ (void *)(buff + TMPL_RET_IDX), rc);
+ rc = -EFAULT;
+ goto error;
+ }
flush_icache_range((unsigned long)buff,
(unsigned long)(&buff[TMPL_END_IDX]));
@@ -284,7 +338,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op, struct kprobe *p)
error:
free_ppc_optinsn_slot(buff, 0);
- return -ERANGE;
+ return rc;
}
@@ -307,6 +361,7 @@ void arch_optimize_kprobes(struct list_head *oplist)
{
struct optimized_kprobe *op;
struct optimized_kprobe *tmp;
+ int rc;
list_for_each_entry_safe(op, tmp, oplist, list) {
/*
@@ -315,9 +370,13 @@ void arch_optimize_kprobes(struct list_head *oplist)
*/
memcpy(op->optinsn.copied_insn, op->kp.addr,
RELATIVEJUMP_SIZE);
- patch_instruction(op->kp.addr,
+ rc = patch_instruction(op->kp.addr,
create_branch((unsigned int *)op->kp.addr,
(unsigned long)op->optinsn.insn, 0));
+ if (rc)
+ pr_err("%s:%d: Error patching instruction at 0x%pK: %d\n",
+ __func__, __LINE__,
+ (void *)(op->kp.addr), rc);
list_del_init(&op->list);
}
}
--
2.25.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox