* Re: [PATCH 4/6] powerpc/mm/64s/hash: Factor out change_memory_range()
From: Michael Ellerman @ 2021-03-16 6:30 UTC (permalink / raw)
To: Daniel Axtens, linuxppc-dev; +Cc: aneesh.kumar
In-Reply-To: <87k0r4q060.fsf@dja-thinkpad.axtens.net>
Daniel Axtens <dja@axtens.net> writes:
> Michael Ellerman <mpe@ellerman.id.au> writes:
>
>> Pull the loop calling hpte_updateboltedpp() out of
>> hash__change_memory_range() into a helper function. We need it to be a
>> separate function for the next patch.
>>
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> ---
>> arch/powerpc/mm/book3s64/hash_pgtable.c | 23 +++++++++++++++--------
>> 1 file changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/book3s64/hash_pgtable.c b/arch/powerpc/mm/book3s64/hash_pgtable.c
>> index 03819c259f0a..3663d3cdffac 100644
>> --- a/arch/powerpc/mm/book3s64/hash_pgtable.c
>> +++ b/arch/powerpc/mm/book3s64/hash_pgtable.c
>> @@ -400,10 +400,23 @@ EXPORT_SYMBOL_GPL(hash__has_transparent_hugepage);
>> #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>>
>> #ifdef CONFIG_STRICT_KERNEL_RWX
>> +static void change_memory_range(unsigned long start, unsigned long end,
>> + unsigned int step, unsigned long newpp)
>
> Looking at the call paths, this gets called only in bare metal, not
> virtualised: should the name reflect that?
It's also called on bare metal:
static bool hash__change_memory_range(unsigned long start, unsigned long end,
unsigned long newpp)
{
...
if (firmware_has_feature(FW_FEATURE_LPAR)) {
...
stop_machine_cpuslocked(change_memory_range_fn, &chmem_parms,
cpu_online_mask);
...
} else
change_memory_range(start, end, step, newpp);
^^^^^^^^^^^^^^^^^^^
>> +{
>> + unsigned long idx;
>> +
>> + pr_debug("Changing page protection on range 0x%lx-0x%lx, to 0x%lx, step 0x%x\n",
>> + start, end, newpp, step);
>> +
>> + for (idx = start; idx < end; idx += step)
>> + /* Not sure if we can do much with the return value */
>
> Hmm, I realise this comment isn't changed, but it did make me wonder
> what the return value!
>
> It turns out that the function doesn't actually return anything.
>
> Tracking back the history of hpte_updateboltedpp, it looks like it has
> not had a return value since the start of git history:
>
> ^1da177e4c3f4 include/asm-ppc64/machdep.h void (*hpte_updateboltedpp)(unsigned long newpp,
> 3c726f8dee6f5 include/asm-powerpc/machdep.h unsigned long ea,
> 1189be6508d45 include/asm-powerpc/machdep.h int psize, int ssize);
>
> The comment comes from commit cd65d6971334 ("powerpc/mm/hash: Implement
> mark_rodata_ro() for hash") where Balbir added the comment, but again I
> can't figure out what sort of return value there would be to ignore.
I suspect he just assumed there was a return value, and the comment is
saying we aren't really allowed to fail here, so what could we do?
In general these routines changing the kernel map permissions aren't
allowed to fail, because the callers don't cope with a failure, and at
least in some cases eg. RW -> RX the permission change is not optional.
> Should we drop the comment? (or return something from hpte_updateboltedpp)
I'll leave the comment for now, but we could probably drop it.
It would be good if hpte_updateboltedpp() could fail and return an
error. Currently pSeries_lpar_hpte_updateboltedpp() BUGs if the hcall
fails, because the only error cases are due to bad input on our part.
And similarly native_hpte_updateboltedpp() panics if we give it bad
input.
We may still need to panic() at a higher level, ie. adding execute to a
mapping is not optional. But possibly for some changes, like RW->RO we
could WARN and continue.
And I guess for modules we could eventually plumb the error all the way
out and fail the module load.
>> + mmu_hash_ops.hpte_updateboltedpp(newpp, idx, mmu_linear_psize,
>> + mmu_kernel_ssize);
>> +}
>> +
>> static bool hash__change_memory_range(unsigned long start, unsigned long end,
>> unsigned long newpp)
>> {
>> - unsigned long idx;
>> unsigned int step, shift;
>>
>> shift = mmu_psize_defs[mmu_linear_psize].shift;
>> @@ -415,13 +428,7 @@ static bool hash__change_memory_range(unsigned long start, unsigned long end,
>> if (start >= end)
>> return false;
>>
>> - pr_debug("Changing page protection on range 0x%lx-0x%lx, to 0x%lx, step 0x%x\n",
>> - start, end, newpp, step);
>> -
>> - for (idx = start; idx < end; idx += step)
>> - /* Not sure if we can do much with the return value */
>> - mmu_hash_ops.hpte_updateboltedpp(newpp, idx, mmu_linear_psize,
>> - mmu_kernel_ssize);
>> + change_memory_range(start, end, step, newpp);
>
> Looking at how change_memory_range is called, step is derived by:
>
> shift = mmu_psize_defs[mmu_linear_psize].shift;
> step = 1 << shift;
>
> We probably therefore don't really need to pass step in to
> change_memory_range. Having said that, I'm not sure it would really be that
> much tidier to compute step in change_memory_range, especially since we
> also need step for the other branch in hash__change_memory_range.
Hmm yeah, swings and roundabouts. I think I'll leave it as is, so that
we're only calculating step in one place.
> Beyond that it all looks reasonable to me!
>
> I also checked that the loop operations made sense, I think they do - we
> cover from start inclusive to end exclusive and the alignment is done
> before we call into change_memory_range.
Thanks.
cheers
^ permalink raw reply
* Re: [PATCH v9 2/8] powerpc/lib/code-patching: Set up Strict RWX patching earlier
From: Christophe Leroy @ 2021-03-16 6:32 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev
Cc: christophe.leroy, ajd, npiggin, naveen.n.rao, dja
In-Reply-To: <20210316031741.1004850-2-jniethe5@gmail.com>
Le 16/03/2021 à 04:17, Jordan Niethe a écrit :
> setup_text_poke_area() is a late init call so it runs before
> mark_rodata_ro() and after the init calls. This lets all the init code
> patching simply write to their locations. In the future, kprobes is
> going to allocate its instruction pages RO which means they will need
> setup_text__poke_area() to have been already called for their code
> patching. However, init_kprobes() (which allocates and patches some
> instruction pages) is an early init call so it happens before
> setup_text__poke_area().
>
> start_kernel() calls poking_init() before any of the init calls. On
> powerpc, poking_init() is currently a nop. setup_text_poke_area() relies
> on kernel virtual memory, cpu hotplug and per_cpu_areas being setup.
> setup_per_cpu_areas(), boot_cpu_hotplug_init() and mm_init() are called
> before poking_init().
>
> Turn setup_text_poke_area() into poking_init().
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v9: New to series
> ---
> arch/powerpc/lib/code-patching.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 2333625b5e31..b28afa1133db 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -65,14 +65,11 @@ static int text_area_cpu_down(unsigned int cpu)
> }
>
> /*
> - * Run as a late init call. This allows all the boot time patching to be done
> - * simply by patching the code, and then we're called here prior to
> - * mark_rodata_ro(), which happens after all init calls are run. Although
> - * BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge
> - * it as being preferable to a kernel that will crash later when someone tries
> - * to use patch_instruction().
> + * Although BUG_ON() is rude, in this case it should only happen if ENOMEM, and
> + * we judge it as being preferable to a kernel that will crash later when
> + * someone tries to use patch_instruction().
Please use WARN_ON(), see why at https://www.kernel.org/doc/html/latest/process/deprecated.html
> */
> -static int __init setup_text_poke_area(void)
> +int __init poking_init(void)
> {
> BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> "powerpc/text_poke:online", text_area_cpu_up,
> @@ -80,7 +77,6 @@ static int __init setup_text_poke_area(void)
>
> return 0;
> }
> -late_initcall(setup_text_poke_area);
>
> /*
> * This can be called for kernel text or a module.
>
^ permalink raw reply
* Re: [PATCH 5/6] powerpc/mm/64s/hash: Add real-mode change_memory_range() for hash LPAR
From: Michael Ellerman @ 2021-03-16 6:40 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: aneesh.kumar
In-Reply-To: <1613086376.ygjdbhz8p5.astroid@bobo.none>
Nicholas Piggin <npiggin@gmail.com> writes:
> Excerpts from Michael Ellerman's message of February 11, 2021 11:51 pm:
>> When we enabled STRICT_KERNEL_RWX we received some reports of boot
>> failures when using the Hash MMU and running under phyp. The crashes
>> are intermittent, and often exhibit as a completely unresponsive
>> system, or possibly an oops.
...
>>
>> diff --git a/arch/powerpc/mm/book3s64/hash_pgtable.c b/arch/powerpc/mm/book3s64/hash_pgtable.c
>> index 3663d3cdffac..01de985df2c4 100644
>> --- a/arch/powerpc/mm/book3s64/hash_pgtable.c
>> +++ b/arch/powerpc/mm/book3s64/hash_pgtable.c
>> @@ -414,6 +428,73 @@ static void change_memory_range(unsigned long start, unsigned long end,
>> mmu_kernel_ssize);
>> }
>>
>> +static int notrace chmem_secondary_loop(struct change_memory_parms *parms)
>> +{
>> + unsigned long msr, tmp, flags;
>> + int *p;
>> +
>> + p = &parms->cpu_counter.counter;
>> +
>> + local_irq_save(flags);
>> + __hard_EE_RI_disable();
>> +
>> + asm volatile (
>> + // Switch to real mode and leave interrupts off
>> + "mfmsr %[msr] ;"
>> + "li %[tmp], %[MSR_IR_DR] ;"
>> + "andc %[tmp], %[msr], %[tmp] ;"
>> + "mtmsrd %[tmp] ;"
>> +
>> + // Tell the master we are in real mode
>> + "1: "
>> + "lwarx %[tmp], 0, %[p] ;"
>> + "addic %[tmp], %[tmp], -1 ;"
>> + "stwcx. %[tmp], 0, %[p] ;"
>> + "bne- 1b ;"
>> +
>> + // Spin until the counter goes to zero
>> + "2: ;"
>> + "lwz %[tmp], 0(%[p]) ;"
>> + "cmpwi %[tmp], 0 ;"
>> + "bne- 2b ;"
>> +
>> + // Switch back to virtual mode
>> + "mtmsrd %[msr] ;"
>
> Pity we don't have something that can switch to emergency stack and
> so we can write this stuff in C.
>
> How's something like this suit you?
It looks like it would be really good for writing exploits :)
I think at the very least we would want the asm part to load the SP
from the paca itself, rather than taking it as a parameter.
But I'm not sure writing these type of things in C is a big win, because
you have to be so careful about what you call anyway. It's almost better
in asm because it's so restrictive.
Obviously having said that, my first attempt got the IRQ save/restore
wrong, so maybe we should at least have some macros to help with it.
Did you have another user for this in mind? The only one that I can
think of at the moment is the subcore stuff.
cheers
> diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
> index 070465825c21..5e911d0b0b16 100644
> --- a/arch/powerpc/kernel/misc_64.S
> +++ b/arch/powerpc/kernel/misc_64.S
> @@ -27,6 +27,28 @@
>
> .text
>
> +#ifdef CONFIG_PPC_BOOK3S_64
> +_GLOBAL(__call_realmode)
> + mflr r0
> + std r0,16(r1)
> + stdu r1,THREAD_SIZE-STACK_FRAME_OVERHEAD(r5)
> + mr r1,r5
> + mtctr r3
> + mr r3,r4
> + mfmsr r4
> + xori r4,r4,(MSR_IR|MSR_DR)
> + mtmsrd r4
> + bctrl
> + mfmsr r4
> + xori r4,r4,(MSR_IR|MSR_DR)
> + mtmsrd r4
> + ld r1,0(r1)
> + ld r0,16(r1)
> + mtlr r0
> + blr
> +
> +#endif
> +
> _GLOBAL(call_do_softirq)
> mflr r0
> std r0,16(r1)
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index a66f435dabbf..260d60f665a3 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -2197,6 +2197,43 @@ void show_stack(struct task_struct *tsk, unsigned long *stack,
> put_task_stack(tsk);
> }
>
> +#ifdef CONFIG_PPC_BOOK3S_64
> +int __call_realmode(int (*fn)(void *arg), void *arg, void *sp);
> +
> +/* XXX: find a better place for this
> + * Executing C code in real-mode in general Book3S-64 code can only be done
> + * via this function that switches the stack to one inside the real-mode-area,
> + * which may cover only a small first part of real memory on hash guest LPARs.
> + * fn must be NOKPROBES, must not access vmalloc or anything outside the RMA,
> + * probably shouldn't enable the MMU or interrupts, etc, and be very careful
> + * about calling other generic kernel or powerpc functions.
> + */
> +int call_realmode(int (*fn)(void *arg), void *arg)
> +{
> + unsigned long flags;
> + void *cursp, *emsp;
> + int ret;
> +
> + /* Stack switch is only really required for HPT LPAR, but do it for all to help test coverage of tricky code */
> + cursp = (void *)(current_stack_pointer & ~(THREAD_SIZE - 1));
> + emsp = (void *)(local_paca->emergency_sp - THREAD_SIZE);
> +
> + /* XXX check_stack_overflow(); */
> +
> + if (WARN_ON_ONCE(cursp == emsp))
> + return -EBUSY;
> +
> + local_irq_save(flags);
> + hard_irq_disable();
> +
> + ret = __call_realmode(fn, arg, emsp);
> +
> + local_irq_restore(flags);
> +
> + return ret;
> +}
> +#endif
> +
> #ifdef CONFIG_PPC64
> /* Called with hard IRQs off */
> void notrace __ppc64_runlatch_on(void)
> --
> 2.23.0
^ permalink raw reply
* Re: [PATCH v9 3/8] powerpc/kprobes: Mark newly allocated probes as RO
From: Christophe Leroy @ 2021-03-16 6:44 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev
Cc: christophe.leroy, ajd, npiggin, naveen.n.rao, dja
In-Reply-To: <20210316031741.1004850-3-jniethe5@gmail.com>
Le 16/03/2021 à 04:17, Jordan Niethe a écrit :
> From: Russell Currey <ruscur@russell.cc>
>
> With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be one
> W+X page at boot by default. This can be tested with
> CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the
> kernel log during boot.
>
This text is confusing. I don't understand what is the status before the patch, and what is the
status after.
"there will be one ...", does it mean after the patch ?
> Add an arch specific insn page allocator which returns RO pages if
> STRICT_KERNEL_RWX is enabled. This page is only written to with
> patch_instruction() which is able to write RO pages.
"an" or "the" arch specific insn page allocator ?
>
> Reviewed-by: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> [jpn: Reword commit message, switch from vmalloc_exec(), add
> free_insn_page()]
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v9: - vmalloc_exec() no longer exists
> - Set the page to RW before freeing it
> ---
> arch/powerpc/kernel/kprobes.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
> index 01ab2163659e..bb7e4d321988 100644
> --- a/arch/powerpc/kernel/kprobes.c
> +++ b/arch/powerpc/kernel/kprobes.c
> @@ -25,6 +25,8 @@
> #include <asm/sections.h>
> #include <asm/inst.h>
> #include <linux/uaccess.h>
> +#include <linux/set_memory.h>
> +#include <linux/vmalloc.h>
>
> DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
> DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
> @@ -103,6 +105,26 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
> return addr;
> }
>
> +void *alloc_insn_page(void)
> +{
> + void *page = vmalloc(PAGE_SIZE);
Can't do that on book3s/32, see https://github.com/linuxppc/linux/commit/6ca05532 and
https://github.com/linuxppc/linux/commit/7fbc22ce
Should do:
return __vmalloc_node_range(size, 1, MODULES_VADDR, MODULES_END, GFP_KERNEL,
PAGE_KERNEL_ROX, VM_FLUSH_RESET_PERMS, NUMA_NO_NODE,
__builtin_return_address(0));
To keep it simple, you'll probably need to define MODULES_VADDR and MODULES_END as resp
VMALLOC_START and VMALLOC_END when they are not defined, maybe in asm/pgtable.h
> +
> + if (!page)
> + return NULL;
> +
> + set_memory_ro((unsigned long)page, 1);
> + set_memory_x((unsigned long)page, 1);
> +
> + return page;
> +}
> +
> +void free_insn_page(void *page)
> +{
> + set_memory_nx((unsigned long)page, 1);
> + set_memory_rw((unsigned long)page, 1);
> + vfree(page);
> +}
> +
> int arch_prepare_kprobe(struct kprobe *p)
> {
> int ret = 0;
>
^ permalink raw reply
* Re: [PATCH v9 4/8] powerpc/mm/ptdump: debugfs handler for W+X checks at runtime
From: Christophe Leroy @ 2021-03-16 6:47 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev
Cc: christophe.leroy, ajd, Kees Cook, npiggin, naveen.n.rao, dja
In-Reply-To: <20210316031741.1004850-4-jniethe5@gmail.com>
Le 16/03/2021 à 04:17, Jordan Niethe a écrit :
> From: Russell Currey <ruscur@russell.cc>
>
> Very rudimentary, just
>
> echo 1 > [debugfs]/check_wx_pages
>
> and check the kernel log. Useful for testing strict module RWX.
>
> Updated the Kconfig entry to reflect this.
>
> Also fixed a typo.
Why not just perform the test everytime someone dumps kernel_page_tables ?
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> arch/powerpc/Kconfig.debug | 6 ++++--
> arch/powerpc/mm/ptdump/ptdump.c | 21 ++++++++++++++++++++-
> 2 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> index ae084357994e..56e99e9a30d9 100644
> --- a/arch/powerpc/Kconfig.debug
> +++ b/arch/powerpc/Kconfig.debug
> @@ -371,7 +371,7 @@ config PPC_PTDUMP
> If you are unsure, say N.
>
> config PPC_DEBUG_WX
> - bool "Warn on W+X mappings at boot"
> + bool "Warn on W+X mappings at boot & enable manual checks at runtime"
> depends on PPC_PTDUMP && STRICT_KERNEL_RWX
> help
> Generate a warning if any W+X mappings are found at boot.
> @@ -385,7 +385,9 @@ config PPC_DEBUG_WX
> of other unfixed kernel bugs easier.
>
> There is no runtime or memory usage effect of this option
> - once the kernel has booted up - it's a one time check.
> + once the kernel has booted up, it only automatically checks once.
> +
> + Enables the "check_wx_pages" debugfs entry for checking at runtime.
>
> If in doubt, say "Y".
>
> diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
> index aca354fb670b..78497d57b66b 100644
> --- a/arch/powerpc/mm/ptdump/ptdump.c
> +++ b/arch/powerpc/mm/ptdump/ptdump.c
> @@ -4,7 +4,7 @@
> *
> * This traverses the kernel pagetables and dumps the
> * information about the used sections of memory to
> - * /sys/kernel/debug/kernel_pagetables.
> + * /sys/kernel/debug/kernel_page_tables.
> *
> * Derived from the arm64 implementation:
> * Copyright (c) 2014, The Linux Foundation, Laura Abbott.
> @@ -459,6 +459,25 @@ void ptdump_check_wx(void)
> else
> pr_info("Checked W+X mappings: passed, no W+X pages found\n");
> }
> +
> +static int check_wx_debugfs_set(void *data, u64 val)
> +{
> + if (val != 1ULL)
> + return -EINVAL;
> +
> + ptdump_check_wx();
> +
> + return 0;
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(check_wx_fops, NULL, check_wx_debugfs_set, "%llu\n");
> +
> +static int ptdump_check_wx_init(void)
> +{
> + return debugfs_create_file("check_wx_pages", 0200, NULL,
> + NULL, &check_wx_fops) ? 0 : -ENOMEM;
> +}
> +device_initcall(ptdump_check_wx_init);
> #endif
>
> static int ptdump_init(void)
>
^ permalink raw reply
* Re: [PATCH v9 5/8] powerpc: Set ARCH_HAS_STRICT_MODULE_RWX
From: Christophe Leroy @ 2021-03-16 6:51 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev
Cc: christophe.leroy, ajd, npiggin, naveen.n.rao, dja
In-Reply-To: <20210316031741.1004850-5-jniethe5@gmail.com>
Le 16/03/2021 à 04:17, Jordan Niethe a écrit :
> From: Russell Currey <ruscur@russell.cc>
>
> To enable strict module RWX on powerpc, set:
>
> CONFIG_STRICT_MODULE_RWX=y
>
> You should also have CONFIG_STRICT_KERNEL_RWX=y set to have any real
> security benefit.
>
> ARCH_HAS_STRICT_MODULE_RWX is set to require ARCH_HAS_STRICT_KERNEL_RWX.
> This is due to a quirk in arch/Kconfig and arch/powerpc/Kconfig that
> makes STRICT_MODULE_RWX *on by default* in configurations where
> STRICT_KERNEL_RWX is *unavailable*.
Not that easy on book3s/32. On it, you can't protect memory against execution on a page basis, you
can only do it on a segment basis. So in order to do that, when would need to allocate to areas of
memory: one in module space for text and one in vmalloc space for data.
See https://github.com/linuxppc/linux/commit/6ca05532 and
https://github.com/linuxppc/linux/commit/7fbc22ce
>
> Since this doesn't make much sense, and module RWX without kernel RWX
> doesn't make much sense, having the same dependencies as kernel RWX
> works around this problem.
>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> arch/powerpc/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 4498a27ac9db..d9cadc4212d0 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -137,6 +137,7 @@ config PPC
> select ARCH_HAS_SCALED_CPUTIME if VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
> select ARCH_HAS_SET_MEMORY
> select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 || PPC32) && !HIBERNATION)
> + select ARCH_HAS_STRICT_MODULE_RWX if ARCH_HAS_STRICT_KERNEL_RWX
> select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> select ARCH_HAS_UACCESS_FLUSHCACHE
> select ARCH_HAS_COPY_MC if PPC64
>
^ permalink raw reply
* Re: [PATCH] powerpc: arch/powerpc/kernel/setup_64.c - cleanup warnings
From: Christophe Leroy @ 2021-03-16 6:57 UTC (permalink / raw)
To: He Ying, mpe, benh, paulus, npiggin, dja, akpm, aneesh.kumar,
rppt, ardb, clg
Cc: johnny.chenyi, linuxppc-dev, linux-kernel
In-Reply-To: <20210316041148.29694-1-heying24@huawei.com>
Le 16/03/2021 à 05:11, He Ying a écrit :
> warning: symbol 'rfi_flush' was not declared.
> warning: symbol 'entry_flush' was not declared.
> warning: symbol 'uaccess_flush' was not declared.
> We found warnings above in arch/powerpc/kernel/setup_64.c by using
> sparse tool.
>
> Define 'entry_flush' and 'uaccess_flush' as static because they are not
> referenced outside the file. Include asm/security_features.h in which
> 'rfi_flush' is declared.
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: He Ying <heying24@huawei.com>
Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/setup_64.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index 560ed8b975e7..f92d72a7e7ce 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -68,6 +68,7 @@
> #include <asm/early_ioremap.h>
> #include <asm/pgalloc.h>
> #include <asm/asm-prototypes.h>
> +#include <asm/security_features.h>
>
> #include "setup.h"
>
> @@ -949,8 +950,8 @@ static bool no_rfi_flush;
> static bool no_entry_flush;
> static bool no_uaccess_flush;
> bool rfi_flush;
> -bool entry_flush;
> -bool uaccess_flush;
> +static bool entry_flush;
> +static bool uaccess_flush;
> DEFINE_STATIC_KEY_FALSE(uaccess_flush_key);
> EXPORT_SYMBOL(uaccess_flush_key);
>
>
^ permalink raw reply
* Re: [PATCH 03/10] powerpc/64e/interrupt: use new interrupt return
From: Nicholas Piggin @ 2021-03-16 7:03 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <186d3513-d7ab-a658-cdb2-6fe5146c1fc4@csgroup.eu>
Excerpts from Christophe Leroy's message of March 15, 2021 5:50 pm:
>
>
> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>> Update the new C and asm interrupt return code to account for 64e
>> specifics, switch over to use it.
>>
>> The now-unused old ret_from_except code, that was moved to 64e after the
>> 64s conversion, is removed.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/asm-prototypes.h | 2 -
>> arch/powerpc/kernel/entry_64.S | 9 +-
>> arch/powerpc/kernel/exceptions-64e.S | 321 ++--------------------
>> arch/powerpc/kernel/interrupt.c | 27 +-
>> arch/powerpc/kernel/irq.c | 76 -----
>> 5 files changed, 56 insertions(+), 379 deletions(-)
>>
...
>> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
>> index da78eb6ab92f..1bb4e9b37748 100644
>> --- a/arch/powerpc/kernel/exceptions-64e.S
>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>> @@ -139,7 +139,8 @@ ret_from_level_except:
>> ld r3,_MSR(r1)
>> andi. r3,r3,MSR_PR
>> beq 1f
>> - b ret_from_except
>> + REST_NVGPRS(r1)
>
> Could this be in a separate preceding patch (only the adding of REST_NVGPRS(), the call to
> ret_from_except can remain as is by removing the REST_NVGPRS() which is there to make
> ret_from_except and ret_from_except_lite identical).
>
> Or maybe you can also do the name change to interrupt_return in that preceeding patch, so than the
> "use new interrupt return" patch only contains the interesting parts.
I don't like that so much, maybe the better split is to first change the
common code to add the 64e bits, and then convert 64e from
ret_from_except to interrupt_return and remove the old code.
...
>> @@ -1016,284 +1021,8 @@ alignment_more:
>
> ...
>
>> -fast_exception_return:
>> - wrteei 0
>> -1: mr r0,r13
>> - ld r10,_MSR(r1)
>> - REST_4GPRS(2, r1)
>> - andi. r6,r10,MSR_PR
>> - REST_2GPRS(6, r1)
>> - beq 1f
>> - ACCOUNT_CPU_USER_EXIT(r13, r10, r11)
>
> Then ACCOUNT_CPU_USER_EXIT can be removed from asm/ppc_asm.h
Will do.
>> @@ -387,7 +396,11 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>> while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
>> local_irq_enable(); /* returning to user: may enable */
>> if (ti_flags & _TIF_NEED_RESCHED) {
>> +#ifdef CONFIG_PPC_BOOK3E_64
>> + schedule_user();
>> +#else
>> schedule();
>> +#endif
>> } else {
>> if (ti_flags & _TIF_SIGPENDING)
>> ret |= _TIF_RESTOREALL;
>> @@ -435,7 +448,10 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>> /*
>> * We do this at the end so that we do context switch with KERNEL AMR
>> */
>> +#ifndef CONFIG_PPC_BOOK3E_64
>> kuap_user_restore(regs);
>
> Why do you need to ifdef this out ?
> Only PPC_8xx, PPC_BOOK3S_32 and PPC_RADIX_MMU select PPC_HAVE_KUAP.
> When PPC_KUAP is not selected, kuap_user_restore() is a static inline {} defined in asm/kup.h
It came in from an old patch rebase. I'll get rid of them.
...
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 03/10] powerpc/64e/interrupt: use new interrupt return
From: Nicholas Piggin @ 2021-03-16 7:04 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <a2c192a2-ebdb-d18a-6e21-b27d8890fe06@csgroup.eu>
Excerpts from Christophe Leroy's message of March 15, 2021 11:30 pm:
>
>
> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>> Update the new C and asm interrupt return code to account for 64e
>> specifics, switch over to use it.
>>
>> The now-unused old ret_from_except code, that was moved to 64e after the
>> 64s conversion, is removed.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/asm-prototypes.h | 2 -
>> arch/powerpc/kernel/entry_64.S | 9 +-
>> arch/powerpc/kernel/exceptions-64e.S | 321 ++--------------------
>> arch/powerpc/kernel/interrupt.c | 27 +-
>> arch/powerpc/kernel/irq.c | 76 -----
>> 5 files changed, 56 insertions(+), 379 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
>> index fbabb49888d3..ae7b058b2970 100644
>> --- a/arch/powerpc/kernel/interrupt.c
>> +++ b/arch/powerpc/kernel/interrupt.c
>> @@ -235,6 +235,10 @@ static notrace void booke_load_dbcr0(void)
>> #endif
>> }
>>
>> +/* temporary hack for context tracking, removed in later patch */
>> +#include <linux/sched/debug.h>
>> +asmlinkage __visible void __sched schedule_user(void);
>> +
>> /*
>> * This should be called after a syscall returns, with r3 the return value
>> * from the syscall. If this function returns non-zero, the system call
>> @@ -292,7 +296,11 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>> while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
>> local_irq_enable();
>> if (ti_flags & _TIF_NEED_RESCHED) {
>> +#ifdef CONFIG_PPC_BOOK3E_64
>> + schedule_user();
>> +#else
>> schedule();
>> +#endif
>> } else {
>> /*
>> * SIGPENDING must restore signal handler function
>> @@ -360,7 +368,6 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>> return ret;
>> }
>>
>> -#ifndef CONFIG_PPC_BOOK3E_64 /* BOOK3E not yet using this */
>> notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
>> {
>> unsigned long ti_flags;
>> @@ -372,7 +379,9 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>> BUG_ON(!(regs->msr & MSR_PR));
>> BUG_ON(!FULL_REGS(regs));
>> BUG_ON(arch_irq_disabled_regs(regs));
>> +#ifdef CONFIG_PPC_BOOK3S_64
>
> Shouldn't this go away in patch 6 as well ?
> Or is that needed at all ? In syscall_exit_prepare() it is not ifdefed .
Hmm, not sure. I'll take a look. It probably shouldn't be ifdefed at all
but definitely by the end it should run without warning.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 07/10] powerpc/64e/interrupt: handle bad_page_fault in C
From: Nicholas Piggin @ 2021-03-16 7:06 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <6e0873c6-7102-ff8b-1552-8ce158613fd7@csgroup.eu>
Excerpts from Christophe Leroy's message of March 16, 2021 12:07 am:
>
>
> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>> With non-volatile registers saved on interrupt, bad_page_fault
>> can now be called by do_page_fault.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/kernel/exceptions-64e.S | 6 ------
>> arch/powerpc/mm/fault.c | 5 +----
>> 2 files changed, 1 insertion(+), 10 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
>> index a059ab3542c2..b08c84e0fa56 100644
>> --- a/arch/powerpc/kernel/exceptions-64e.S
>> +++ b/arch/powerpc/kernel/exceptions-64e.S
>> @@ -937,12 +937,6 @@ storage_fault_common:
>> ld r14,PACA_EXGEN+EX_R14(r13)
>> ld r15,PACA_EXGEN+EX_R15(r13)
>> bl do_page_fault
>> - cmpdi r3,0
>> - bne- 1f
>> - b interrupt_return
>> - mr r4,r3
>> - addi r3,r1,STACK_FRAME_OVERHEAD
>> - bl __bad_page_fault
>
> Then __bad_page_fault() can be static now.
Good point, I'll change it.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 10/10] powerpc: move norestart trap flag to bit 0
From: Nicholas Piggin @ 2021-03-16 7:11 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <99f15df0-dc86-4601-066f-a6c067ece8bf@csgroup.eu>
Excerpts from Christophe Leroy's message of March 15, 2021 6:14 pm:
>
>
> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>> Compact the trap flags down to use the low 4 bits of regs.trap.
>>
>> A few 64e interrupt trap numbers set bit 4. Although they tended to be
>> trivial so it wasn't a real problem[1], it is not the right thing to do,
>> and confusing.
>>
>> [*] E.g., 0x310 hypercall goes to unknown_exception, which prints
>> regs->trap directly so 0x310 will appear fine, and only the syscall
>> interrupt will test norestart, so it won't be confused by 0x310.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/ptrace.h | 14 ++++++++++----
>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
>> index 91194fdd5d01..6a04abfe5eb6 100644
>> --- a/arch/powerpc/include/asm/ptrace.h
>> +++ b/arch/powerpc/include/asm/ptrace.h
>> @@ -185,15 +185,21 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
>> #define current_pt_regs() \
>> ((struct pt_regs *)((unsigned long)task_stack_page(current) + THREAD_SIZE) - 1)
>>
>> +/*
>> + * The 4 low bits (0xf) are available as flags to overload the trap word,
>> + * because interrupt vectors have minimum alignment of 0x10. TRAP_FLAGS_MASK
>> + * must cover the bits used as flags, including bit 0 which is used as the
>> + * "norestart" bit.
>> + */
>> #ifdef __powerpc64__
>> -#define TRAP_FLAGS_MASK 0x10
>> +#define TRAP_FLAGS_MASK 0x1
>> #define TRAP(regs) ((regs)->trap & ~TRAP_FLAGS_MASK)
>> #else
>> /*
>> * On 4xx we use bit 1 in the trap word to indicate whether the exception
>> * is a critical exception (1 means it is).
>> */
>> -#define TRAP_FLAGS_MASK 0x1E
>> +#define TRAP_FLAGS_MASK 0xf
>
> Could we set 0xf for all and remove the ifdef __powerpc64__ ?
I like that it documents the bit number allocation so I prefer to leave
it, but TRAP() does not have to be defined twice at least.
>
>> #define TRAP(regs) ((regs)->trap & ~TRAP_FLAGS_MASK)
>> #define IS_CRITICAL_EXC(regs) (((regs)->trap & 2) != 0)
>> #define IS_MCHECK_EXC(regs) (((regs)->trap & 4) != 0)
>> @@ -222,12 +228,12 @@ static inline bool trap_is_syscall(struct pt_regs *regs)
>>
>> static inline bool trap_norestart(struct pt_regs *regs)
>> {
>> - return regs->trap & 0x10;
>> + return regs->trap & 0x1;
>> }
>>
>> static inline void set_trap_norestart(struct pt_regs *regs)
>> {
>> - regs->trap |= 0x10;
>> + regs->trap |= 0x1;
>> }
>>
>> #define arch_has_single_step() (1)
>>
>
> While we are playing with ->trap, in mm/book3s64/hash_utils.c there is an if (regs->trap == 0x400).
> Should be TRAP(regs) == 0x400 ?
Yes I would say so, if you want to do a patch you can add
Acked-by: Nicholas Piggin <npiggin@gmail.com>
Otherwise I can do it.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 10/10] powerpc: move norestart trap flag to bit 0
From: Christophe Leroy @ 2021-03-16 7:13 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <1615878424.0gp943h7l3.astroid@bobo.none>
Le 16/03/2021 à 08:11, Nicholas Piggin a écrit :
> Excerpts from Christophe Leroy's message of March 15, 2021 6:14 pm:
>>
>>
>> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>>> Compact the trap flags down to use the low 4 bits of regs.trap.
>>>
>>> A few 64e interrupt trap numbers set bit 4. Although they tended to be
>>> trivial so it wasn't a real problem[1], it is not the right thing to do,
>>> and confusing.
>>>
>>> [*] E.g., 0x310 hypercall goes to unknown_exception, which prints
>>> regs->trap directly so 0x310 will appear fine, and only the syscall
>>> interrupt will test norestart, so it won't be confused by 0x310.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> arch/powerpc/include/asm/ptrace.h | 14 ++++++++++----
>>> 1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/asm/ptrace.h
>>> index 91194fdd5d01..6a04abfe5eb6 100644
>>> --- a/arch/powerpc/include/asm/ptrace.h
>>> +++ b/arch/powerpc/include/asm/ptrace.h
>>> @@ -185,15 +185,21 @@ static inline void regs_set_return_value(struct pt_regs *regs, unsigned long rc)
>>> #define current_pt_regs() \
>>> ((struct pt_regs *)((unsigned long)task_stack_page(current) + THREAD_SIZE) - 1)
>>>
>>> +/*
>>> + * The 4 low bits (0xf) are available as flags to overload the trap word,
>>> + * because interrupt vectors have minimum alignment of 0x10. TRAP_FLAGS_MASK
>>> + * must cover the bits used as flags, including bit 0 which is used as the
>>> + * "norestart" bit.
>>> + */
>>> #ifdef __powerpc64__
>>> -#define TRAP_FLAGS_MASK 0x10
>>> +#define TRAP_FLAGS_MASK 0x1
>>> #define TRAP(regs) ((regs)->trap & ~TRAP_FLAGS_MASK)
>>> #else
>>> /*
>>> * On 4xx we use bit 1 in the trap word to indicate whether the exception
>>> * is a critical exception (1 means it is).
>>> */
>>> -#define TRAP_FLAGS_MASK 0x1E
>>> +#define TRAP_FLAGS_MASK 0xf
>>
>> Could we set 0xf for all and remove the ifdef __powerpc64__ ?
>
> I like that it documents the bit number allocation so I prefer to leave
> it, but TRAP() does not have to be defined twice at least.
>
>>
>>> #define TRAP(regs) ((regs)->trap & ~TRAP_FLAGS_MASK)
>>> #define IS_CRITICAL_EXC(regs) (((regs)->trap & 2) != 0)
>>> #define IS_MCHECK_EXC(regs) (((regs)->trap & 4) != 0)
>>> @@ -222,12 +228,12 @@ static inline bool trap_is_syscall(struct pt_regs *regs)
>>>
>>> static inline bool trap_norestart(struct pt_regs *regs)
>>> {
>>> - return regs->trap & 0x10;
>>> + return regs->trap & 0x1;
>>> }
>>>
>>> static inline void set_trap_norestart(struct pt_regs *regs)
>>> {
>>> - regs->trap |= 0x10;
>>> + regs->trap |= 0x1;
>>> }
>>>
>>> #define arch_has_single_step() (1)
>>>
>>
>> While we are playing with ->trap, in mm/book3s64/hash_utils.c there is an if (regs->trap == 0x400).
>> Should be TRAP(regs) == 0x400 ?
>
> Yes I would say so, if you want to do a patch you can add
> Acked-by: Nicholas Piggin <npiggin@gmail.com>
>
> Otherwise I can do it.
Yes please do.
Thanks
Christophe
^ permalink raw reply
* Re: [PATCH] powerpc/64s: power4 nap fixup in C
From: Christophe Leroy @ 2021-03-16 7:16 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20210312012044.3660743-1-npiggin@gmail.com>
Le 12/03/2021 à 02:20, Nicholas Piggin a écrit :
> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> Hopefully this works on a real G5 now, but I couldn't reproduce the
> problem with QEMU.
>
> Thanks,
> Nick
>
> arch/powerpc/include/asm/interrupt.h | 19 +++++++++++
> arch/powerpc/include/asm/processor.h | 1 +
> arch/powerpc/include/asm/thread_info.h | 6 ++++
> arch/powerpc/kernel/exceptions-64s.S | 45 --------------------------
> arch/powerpc/kernel/idle_book3s.S | 4 +++
> 5 files changed, 30 insertions(+), 45 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index aedfba29e43a..ef015d3b5e39 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -9,6 +9,17 @@
> #include <asm/kprobes.h>
> #include <asm/runlatch.h>
>
> +static inline void nap_adjust_return(struct pt_regs *regs)
> +{
> +#ifdef CONFIG_PPC_970_NAP
> + if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
> + /* Can avoid a test-and-clear because NMIs do not call this */
> + clear_thread_local_flags(_TLF_NAPPING);
> + regs->nip = (unsigned long)power4_idle_nap_return;
Why don't you do regs->nip = regs->link like PPC32 instead of going via an intermediate symbol that
does nothing else than branching to LR ?
> + }
> +#endif
> +}
> +
> struct interrupt_state {
> #ifdef CONFIG_PPC_BOOK3E_64
> enum ctx_state ctx_state;
> @@ -111,6 +122,9 @@ static inline void interrupt_async_exit_prepare(struct pt_regs *regs, struct int
> {
> irq_exit();
> interrupt_exit_prepare(regs, state);
> +
> + /* Adjust at exit so the main handler sees the true NIA */
> + nap_adjust_return(regs);
> }
>
> struct interrupt_nmi_state {
> @@ -164,6 +178,11 @@ static inline void interrupt_nmi_exit_prepare(struct pt_regs *regs, struct inter
> radix_enabled() || (mfmsr() & MSR_DR))
> nmi_exit();
>
> + /*
> + * nmi does not call nap_adjust_return because nmi should not create
> + * new work to do (must use irq_work for that).
> + */
> +
> #ifdef CONFIG_PPC64
> if (TRAP(regs) != 0x900 && TRAP(regs) != 0xf00 && TRAP(regs) != 0x260)
> this_cpu_set_ftrace_enabled(state->ftrace_enabled);
...
> diff --git a/arch/powerpc/kernel/idle_book3s.S b/arch/powerpc/kernel/idle_book3s.S
> index f9e6d83e6720..abb719b21cae 100644
> --- a/arch/powerpc/kernel/idle_book3s.S
> +++ b/arch/powerpc/kernel/idle_book3s.S
> @@ -209,4 +209,8 @@ _GLOBAL(power4_idle_nap)
> mtmsrd r7
> isync
> b 1b
> +
> + .globl power4_idle_nap_return
> +power4_idle_nap_return:
> + blr
> #endif
>
^ permalink raw reply
* Re: [PATCH 12/14] powerpc/64s: system call avoid setting MSR[RI] until we set MSR[EE]
From: Christophe Leroy @ 2021-03-16 7:21 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev
In-Reply-To: <20210315220402.260594-13-npiggin@gmail.com>
Le 15/03/2021 à 23:04, Nicholas Piggin a écrit :
> This extends the MSR[RI]=0 window a little further into the system
> call in order to pair RI and EE enabling with a single mtmsrd.
Time ago, I proposed to delay that on PPC32 and Michael objected, see
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/9f9dd859d571e324c7412ed9db9da8cfba678257.1548956511.git.christophe.leroy@c-s.fr/
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kernel/exceptions-64s.S | 2 --
> arch/powerpc/kernel/interrupt_64.S | 6 +++---
> 2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index bd0c82ac9de5..2f14ac3c377c 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -1999,8 +1999,6 @@ END_FTR_SECTION_IFSET(CPU_FTR_REAL_LE)
> mtctr r10
> bctr
> .else
> - li r10,MSR_RI
> - mtmsrd r10,1 /* Set RI (EE=0) */
> #ifdef CONFIG_RELOCATABLE
> __LOAD_HANDLER(r10, system_call_common)
> mtctr r10
> diff --git a/arch/powerpc/kernel/interrupt_64.S b/arch/powerpc/kernel/interrupt_64.S
> index f28f41a1a85a..eef61800f734 100644
> --- a/arch/powerpc/kernel/interrupt_64.S
> +++ b/arch/powerpc/kernel/interrupt_64.S
> @@ -311,10 +311,10 @@ END_BTB_FLUSH_SECTION
> * nothing pending. system_call_exception() will call
> * trace_hardirqs_off().
> */
> - li r11,IRQS_ALL_DISABLED
> - li r12,PACA_IRQ_HARD_DIS
> + li r11,IRQS_DISABLED
> + li r12,-1 /* Set MSR_EE and MSR_RI */
> stb r11,PACAIRQSOFTMASK(r13)
> - stb r12,PACAIRQHAPPENED(r13)
> + mtmsrd r12,1
>
> ENTER_KERNEL_SECURITY_FALLBACK
>
>
^ permalink raw reply
* Re: [PATCH] scsi: ibmvfc: Switch to using the new API kobj_to_dev()
From: Martin K. Petersen @ 2021-03-16 3:15 UTC (permalink / raw)
To: Yang Li
Cc: tyreld, Martin K . Petersen, linux-scsi, jejb, linux-kernel,
paulus, linuxppc-dev
In-Reply-To: <1613976429-89853-1-git-send-email-yang.lee@linux.alibaba.com>
On Mon, 22 Feb 2021 14:47:09 +0800, Yang Li wrote:
> fixed the following coccicheck:
> ./drivers/scsi/ibmvscsi/ibmvfc.c:3161:60-61: WARNING opportunity for
> kobj_to_dev()
Applied to 5.13/scsi-queue, thanks!
[1/1] scsi: ibmvfc: Switch to using the new API kobj_to_dev()
https://git.kernel.org/mkp/scsi/c/18c2a59a4190
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply
* Re: [PATCH v9 7/8] powerpc/mm: implement set_memory_attr()
From: Christophe Leroy @ 2021-03-16 7:25 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev
Cc: christophe.leroy, ajd, npiggin, kbuild test robot, naveen.n.rao,
dja
In-Reply-To: <20210316031741.1004850-7-jniethe5@gmail.com>
Le 16/03/2021 à 04:17, Jordan Niethe a écrit :
> From: Christophe Leroy <christophe.leroy@c-s.fr>
Can you please update the whole series with my new email address: christophe.leroy@csgroup.eu
>
> In addition to the set_memory_xx() functions which allows to change
> the memory attributes of not (yet) used memory regions, implement a
> set_memory_attr() function to:
> - set the final memory protection after init on currently used
> kernel regions.
> - enable/disable kernel memory regions in the scope of DEBUG_PAGEALLOC.
>
> Unlike the set_memory_xx() which can act in three step as the regions
> are unused, this function must modify 'on the fly' as the kernel is
> executing from them. At the moment only PPC32 will use it and changing
> page attributes on the fly is not an issue.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> Reported-by: kbuild test robot <lkp@intel.com>
> [ruscur: cast "data" to unsigned long instead of int]
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> arch/powerpc/include/asm/set_memory.h | 2 ++
> arch/powerpc/mm/pageattr.c | 33 +++++++++++++++++++++++++++
> 2 files changed, 35 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/set_memory.h b/arch/powerpc/include/asm/set_memory.h
> index 64011ea444b4..b040094f7920 100644
> --- a/arch/powerpc/include/asm/set_memory.h
> +++ b/arch/powerpc/include/asm/set_memory.h
> @@ -29,4 +29,6 @@ static inline int set_memory_x(unsigned long addr, int numpages)
> return change_memory_attr(addr, numpages, SET_MEMORY_X);
> }
>
> +int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot);
> +
> #endif
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> index 2da3fbab6ff7..2fde1b195c85 100644
> --- a/arch/powerpc/mm/pageattr.c
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -79,3 +79,36 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
> return apply_to_existing_page_range(&init_mm, start, sz,
> change_page_attr, (void *)action);
> }
> +
> +/*
> + * Set the attributes of a page:
> + *
> + * This function is used by PPC32 at the end of init to set final kernel memory
> + * protection. It includes changing the maping of the page it is executing from
> + * and data pages it is using.
> + */
> +static int set_page_attr(pte_t *ptep, unsigned long addr, void *data)
> +{
> + pgprot_t prot = __pgprot((unsigned long)data);
> +
> + spin_lock(&init_mm.page_table_lock);
> +
> + set_pte_at(&init_mm, addr, ptep, pte_modify(*ptep, prot));
> + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> +
> + spin_unlock(&init_mm.page_table_lock);
> +
> + return 0;
> +}
> +
> +int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot)
> +{
> + unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
> + unsigned long sz = numpages * PAGE_SIZE;
> +
> + if (numpages <= 0)
> + return 0;
> +
> + return apply_to_existing_page_range(&init_mm, start, sz, set_page_attr,
> + (void *)pgprot_val(prot));
> +}
>
^ permalink raw reply
* Re: [PATCH 03/10] powerpc/64e/interrupt: use new interrupt return
From: Nicholas Piggin @ 2021-03-16 7:25 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <1615878222.rk6eq5hjpl.astroid@bobo.none>
Excerpts from Nicholas Piggin's message of March 16, 2021 5:04 pm:
> Excerpts from Christophe Leroy's message of March 15, 2021 11:30 pm:
>>
>>
>> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>>> Update the new C and asm interrupt return code to account for 64e
>>> specifics, switch over to use it.
>>>
>>> The now-unused old ret_from_except code, that was moved to 64e after the
>>> 64s conversion, is removed.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> arch/powerpc/include/asm/asm-prototypes.h | 2 -
>>> arch/powerpc/kernel/entry_64.S | 9 +-
>>> arch/powerpc/kernel/exceptions-64e.S | 321 ++--------------------
>>> arch/powerpc/kernel/interrupt.c | 27 +-
>>> arch/powerpc/kernel/irq.c | 76 -----
>>> 5 files changed, 56 insertions(+), 379 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
>>> index fbabb49888d3..ae7b058b2970 100644
>>> --- a/arch/powerpc/kernel/interrupt.c
>>> +++ b/arch/powerpc/kernel/interrupt.c
>>> @@ -235,6 +235,10 @@ static notrace void booke_load_dbcr0(void)
>>> #endif
>>> }
>>>
>>> +/* temporary hack for context tracking, removed in later patch */
>>> +#include <linux/sched/debug.h>
>>> +asmlinkage __visible void __sched schedule_user(void);
>>> +
>>> /*
>>> * This should be called after a syscall returns, with r3 the return value
>>> * from the syscall. If this function returns non-zero, the system call
>>> @@ -292,7 +296,11 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>> while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
>>> local_irq_enable();
>>> if (ti_flags & _TIF_NEED_RESCHED) {
>>> +#ifdef CONFIG_PPC_BOOK3E_64
>>> + schedule_user();
>>> +#else
>>> schedule();
>>> +#endif
>>> } else {
>>> /*
>>> * SIGPENDING must restore signal handler function
>>> @@ -360,7 +368,6 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>> return ret;
>>> }
>>>
>>> -#ifndef CONFIG_PPC_BOOK3E_64 /* BOOK3E not yet using this */
>>> notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
>>> {
>>> unsigned long ti_flags;
>>> @@ -372,7 +379,9 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>>> BUG_ON(!(regs->msr & MSR_PR));
>>> BUG_ON(!FULL_REGS(regs));
>>> BUG_ON(arch_irq_disabled_regs(regs));
>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>
>> Shouldn't this go away in patch 6 as well ?
>> Or is that needed at all ? In syscall_exit_prepare() it is not ifdefed .
>
> Hmm, not sure. I'll take a look. It probably shouldn't be ifdefed at all
> but definitely by the end it should run without warning.
Oh I got confused and thought that was the syscall exit. Interrupt exit
has to keep this until patch 6 because 64e context tracking does
everything in interrupt wrappers, so by the time we get here it will
already be set to CONTEXT_USER.
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH 03/10] powerpc/64e/interrupt: use new interrupt return
From: Christophe Leroy @ 2021-03-16 7:29 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Scott Wood
In-Reply-To: <1615879421.ckimzb9u3c.astroid@bobo.none>
Le 16/03/2021 à 08:25, Nicholas Piggin a écrit :
> Excerpts from Nicholas Piggin's message of March 16, 2021 5:04 pm:
>> Excerpts from Christophe Leroy's message of March 15, 2021 11:30 pm:
>>>
>>>
>>> Le 15/03/2021 à 04:17, Nicholas Piggin a écrit :
>>>> Update the new C and asm interrupt return code to account for 64e
>>>> specifics, switch over to use it.
>>>>
>>>> The now-unused old ret_from_except code, that was moved to 64e after the
>>>> 64s conversion, is removed.
>>>>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>> ---
>>>> arch/powerpc/include/asm/asm-prototypes.h | 2 -
>>>> arch/powerpc/kernel/entry_64.S | 9 +-
>>>> arch/powerpc/kernel/exceptions-64e.S | 321 ++--------------------
>>>> arch/powerpc/kernel/interrupt.c | 27 +-
>>>> arch/powerpc/kernel/irq.c | 76 -----
>>>> 5 files changed, 56 insertions(+), 379 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
>>>> index fbabb49888d3..ae7b058b2970 100644
>>>> --- a/arch/powerpc/kernel/interrupt.c
>>>> +++ b/arch/powerpc/kernel/interrupt.c
>>>> @@ -235,6 +235,10 @@ static notrace void booke_load_dbcr0(void)
>>>> #endif
>>>> }
>>>>
>>>> +/* temporary hack for context tracking, removed in later patch */
>>>> +#include <linux/sched/debug.h>
>>>> +asmlinkage __visible void __sched schedule_user(void);
>>>> +
>>>> /*
>>>> * This should be called after a syscall returns, with r3 the return value
>>>> * from the syscall. If this function returns non-zero, the system call
>>>> @@ -292,7 +296,11 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>>> while (unlikely(ti_flags & (_TIF_USER_WORK_MASK & ~_TIF_RESTORE_TM))) {
>>>> local_irq_enable();
>>>> if (ti_flags & _TIF_NEED_RESCHED) {
>>>> +#ifdef CONFIG_PPC_BOOK3E_64
>>>> + schedule_user();
>>>> +#else
>>>> schedule();
>>>> +#endif
>>>> } else {
>>>> /*
>>>> * SIGPENDING must restore signal handler function
>>>> @@ -360,7 +368,6 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
>>>> return ret;
>>>> }
>>>>
>>>> -#ifndef CONFIG_PPC_BOOK3E_64 /* BOOK3E not yet using this */
>>>> notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
>>>> {
>>>> unsigned long ti_flags;
>>>> @@ -372,7 +379,9 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
>>>> BUG_ON(!(regs->msr & MSR_PR));
>>>> BUG_ON(!FULL_REGS(regs));
>>>> BUG_ON(arch_irq_disabled_regs(regs));
>>>> +#ifdef CONFIG_PPC_BOOK3S_64
>>>
>>> Shouldn't this go away in patch 6 as well ?
>>> Or is that needed at all ? In syscall_exit_prepare() it is not ifdefed .
>>
>> Hmm, not sure. I'll take a look. It probably shouldn't be ifdefed at all
>> but definitely by the end it should run without warning.
>
> Oh I got confused and thought that was the syscall exit. Interrupt exit
> has to keep this until patch 6 because 64e context tracking does
> everything in interrupt wrappers, so by the time we get here it will
> already be set to CONTEXT_USER.
>
ok, but that it has to go in patch 6 ? At the time being the #ifdef is still there at the end of the
series
^ permalink raw reply
* [PATCH] selftests: remove duplicate include
From: menglong8.dong @ 2021-03-16 7:33 UTC (permalink / raw)
To: pbonzini
Cc: kvm, linuxppc-dev, linux-kernel, zhang.yunkai, paulus,
linux-kselftest, akpm, ricardo.canuelo, shuah
From: Zhang Yunkai <zhang.yunkai@zte.com.cn>
'assert.h' included in 'sparsebit.c' is duplicated.
It is also included in the 161th line.
'string.h' included in 'mincore_selftest.c' is duplicated.
It is also included in the 15th line.
'sched.h' included in 'tlbie_test.c' is duplicated.
It is also included in the 33th line.
Signed-off-by: Zhang Yunkai <zhang.yunkai@zte.com.cn>
---
tools/testing/selftests/kvm/lib/sparsebit.c | 1 -
tools/testing/selftests/mincore/mincore_selftest.c | 1 -
tools/testing/selftests/powerpc/mm/tlbie_test.c | 1 -
3 files changed, 3 deletions(-)
diff --git a/tools/testing/selftests/kvm/lib/sparsebit.c b/tools/testing/selftests/kvm/lib/sparsebit.c
index 031ba3c932ed..a0d0c83d83de 100644
--- a/tools/testing/selftests/kvm/lib/sparsebit.c
+++ b/tools/testing/selftests/kvm/lib/sparsebit.c
@@ -1890,7 +1890,6 @@ void sparsebit_validate_internal(struct sparsebit *s)
*/
#include <stdlib.h>
-#include <assert.h>
struct range {
sparsebit_idx_t first, last;
diff --git a/tools/testing/selftests/mincore/mincore_selftest.c b/tools/testing/selftests/mincore/mincore_selftest.c
index 5a1e85ff5d32..e54106643337 100644
--- a/tools/testing/selftests/mincore/mincore_selftest.c
+++ b/tools/testing/selftests/mincore/mincore_selftest.c
@@ -14,7 +14,6 @@
#include <sys/mman.h>
#include <string.h>
#include <fcntl.h>
-#include <string.h>
#include "../kselftest.h"
#include "../kselftest_harness.h"
diff --git a/tools/testing/selftests/powerpc/mm/tlbie_test.c b/tools/testing/selftests/powerpc/mm/tlbie_test.c
index f85a0938ab25..48344a74b212 100644
--- a/tools/testing/selftests/powerpc/mm/tlbie_test.c
+++ b/tools/testing/selftests/powerpc/mm/tlbie_test.c
@@ -33,7 +33,6 @@
#include <sched.h>
#include <time.h>
#include <stdarg.h>
-#include <sched.h>
#include <pthread.h>
#include <signal.h>
#include <sys/prctl.h>
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v3 28/32] powerpc/64s: interrupt implement exit logic in C
From: Nicholas Piggin @ 2021-03-16 7:36 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: Michal Suchanek
In-Reply-To: <2d68d9ad-c3a2-2372-a5b2-1a1e3fdb41e4@csgroup.eu>
Excerpts from Christophe Leroy's message of March 15, 2021 11:41 pm:
>
>
> Le 25/02/2020 à 18:35, Nicholas Piggin a écrit :
>> Implement the bulk of interrupt return logic in C. The asm return code
>> must handle a few cases: restoring full GPRs, and emulating stack store.
>>
>> The stack store emulation is significantly simplfied, rather than creating
>> a new return frame and switching to that before performing the store, it
>> uses the PACA to keep a scratch register around to perform thestore.
>>
>> The asm return code is moved into 64e for now. The new logic has made
>> allowance for 64e, but I don't have a full environment that works well
>> to test it, and even booting in emulated qemu is not great for stress
>> testing. 64e shouldn't be too far off working with this, given a bit
>> more testing and auditing of the logic.
>>
>> This is slightly faster on a POWER9 (page fault speed increases about
>> 1.1%), probably due to reduced mtmsrd.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>> ---
>
> ...
>
>> +notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned long msr)
>> +{
>
> ...
>
>> +
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> + local_paca->tm_scratch = regs->msr;
>> +#endif
>
> Could we define a helper for that in asm/tm.h, that voids when CONFIG_PPC_TRANSACTIONAL_MEM is not
> selected ?
Yeah I wanted to do something about that. I don't know what it's used
for here. I guess it saves the return MSR so if that causes a crash then
the next oops would see it, but I wonder if we can just get that from
SRR1 + program check error codes, or if there is something we can't
reconstruct from there. Have to check with someone who knows TM better.
Thanks,
Nick
^ permalink raw reply
* [PATCH v1 1/4] powerpc: Activate HAVE_RELIABLE_STACKTRACE for all
From: Christophe Leroy @ 2021-03-16 7:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
CONFIG_HAVE_RELIABLE_STACKTRACE is applicable to all, no
reason to limit it to book3s/64le
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/Kconfig | 2 +-
arch/powerpc/kernel/stacktrace.c | 2 --
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 67c47b60cc84..bb7ca6fee885 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -235,7 +235,7 @@ config PPC
select MMU_GATHER_RCU_TABLE_FREE
select MMU_GATHER_PAGE_SIZE
select HAVE_REGS_AND_STACK_ACCESS_API
- select HAVE_RELIABLE_STACKTRACE if PPC_BOOK3S_64 && CPU_LITTLE_ENDIAN
+ select HAVE_RELIABLE_STACKTRACE
select HAVE_SOFTIRQ_ON_OWN_STACK
select HAVE_SYSCALL_TRACEPOINTS
select HAVE_VIRT_CPU_ACCOUNTING
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index b6440657ef92..a2a050551109 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -88,7 +88,6 @@ save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
}
EXPORT_SYMBOL_GPL(save_stack_trace_regs);
-#ifdef CONFIG_HAVE_RELIABLE_STACKTRACE
/*
* This function returns an error if it detects any unreliable features of the
* stack. Otherwise it guarantees that the stack trace is reliable.
@@ -220,7 +219,6 @@ int save_stack_trace_tsk_reliable(struct task_struct *tsk,
return ret;
}
-#endif /* CONFIG_HAVE_RELIABLE_STACKTRACE */
#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_NMI_IPI)
static void handle_backtrace_ipi(struct pt_regs *regs)
--
2.25.0
^ permalink raw reply related
* [PATCH v1 3/4] powerpc: Convert stacktrace to generic ARCH_STACKWALK
From: Christophe Leroy @ 2021-03-16 7:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <955248c6423cb068c5965923121ba31d4dd2fdde.1615881400.git.christophe.leroy@csgroup.eu>
This patch converts powerpc stacktrace to the generic ARCH_STACKWALK
implemented by commit 214d8ca6ee85 ("stacktrace: Provide common
infrastructure")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/kernel/stacktrace.c | 91 ++++++--------------------------
2 files changed, 17 insertions(+), 75 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index bb7ca6fee885..60827904a816 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -145,6 +145,7 @@ config PPC
select ARCH_MIGHT_HAVE_PC_PARPORT
select ARCH_MIGHT_HAVE_PC_SERIO
select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
+ select ARCH_STACKWALK
select ARCH_SUPPORTS_ATOMIC_RMW
select ARCH_SUPPORTS_DEBUG_PAGEALLOC if PPC32 || PPC_BOOK3S_64
select ARCH_USE_BUILTIN_BSWAP
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index 5b93650bc16c..80f92f5b5393 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -23,12 +23,18 @@
#include <asm/paca.h>
-/*
- * Save stack-backtrace addresses into a stack_trace buffer.
- */
-static void save_context_stack(struct stack_trace *trace, unsigned long sp,
- struct task_struct *task, int savesched)
+void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
+ struct task_struct *task, struct pt_regs *regs)
{
+ unsigned long sp;
+
+ if (regs)
+ sp = regs->gpr[1];
+ else if (task == current)
+ sp = current_stack_frame();
+ else
+ sp = task->thread.ksp;
+
for (;;) {
unsigned long *stack = (unsigned long *) sp;
unsigned long newsp, ip;
@@ -39,63 +45,21 @@ static void save_context_stack(struct stack_trace *trace, unsigned long sp,
newsp = stack[0];
ip = stack[STACK_FRAME_LR_SAVE];
- if (savesched || !in_sched_functions(ip)) {
- if (!trace->skip)
- trace->entries[trace->nr_entries++] = ip;
- else
- trace->skip--;
- }
-
- if (trace->nr_entries >= trace->max_entries)
+ if (!consume_entry(cookie, ip))
return;
sp = newsp;
}
}
-void save_stack_trace(struct stack_trace *trace)
-{
- unsigned long sp;
-
- sp = current_stack_frame();
-
- save_context_stack(trace, sp, current, 1);
-}
-EXPORT_SYMBOL_GPL(save_stack_trace);
-
-void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
-{
- unsigned long sp;
-
- if (!try_get_task_stack(tsk))
- return;
-
- if (tsk == current)
- sp = current_stack_frame();
- else
- sp = tsk->thread.ksp;
-
- save_context_stack(trace, sp, tsk, 0);
-
- put_task_stack(tsk);
-}
-EXPORT_SYMBOL_GPL(save_stack_trace_tsk);
-
-void
-save_stack_trace_regs(struct pt_regs *regs, struct stack_trace *trace)
-{
- save_context_stack(trace, regs->gpr[1], current, 0);
-}
-EXPORT_SYMBOL_GPL(save_stack_trace_regs);
-
/*
* This function returns an error if it detects any unreliable features of the
* stack. Otherwise it guarantees that the stack trace is reliable.
*
* If the task is not 'current', the caller *must* ensure the task is inactive.
*/
-static int __save_stack_trace_tsk_reliable(struct task_struct *task,
- struct stack_trace *trace)
+int arch_stack_walk_reliable(stack_trace_consume_fn consume_entry,
+ void *cookie, struct task_struct *task)
{
unsigned long sp;
unsigned long newsp;
@@ -191,35 +155,12 @@ static int __save_stack_trace_tsk_reliable(struct task_struct *task,
return -EINVAL;
#endif
- if (trace->nr_entries >= trace->max_entries)
- return -E2BIG;
- if (!trace->skip)
- trace->entries[trace->nr_entries++] = ip;
- else
- trace->skip--;
+ if (!consume_entry(cookie, ip))
+ return -EINVAL;
}
return 0;
}
-int save_stack_trace_tsk_reliable(struct task_struct *tsk,
- struct stack_trace *trace)
-{
- int ret;
-
- /*
- * If the task doesn't have a stack (e.g., a zombie), the stack is
- * "reliably" empty.
- */
- if (!try_get_task_stack(tsk))
- return 0;
-
- ret = __save_stack_trace_tsk_reliable(tsk, trace);
-
- put_task_stack(tsk);
-
- return ret;
-}
-
#if defined(CONFIG_PPC_BOOK3S_64) && defined(CONFIG_NMI_IPI)
static void handle_backtrace_ipi(struct pt_regs *regs)
{
--
2.25.0
^ permalink raw reply related
* [PATCH v1 2/4] powerpc: Rename 'tsk' parameter into 'task'
From: Christophe Leroy @ 2021-03-16 7:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <955248c6423cb068c5965923121ba31d4dd2fdde.1615881400.git.christophe.leroy@csgroup.eu>
To better match generic code, rename 'tsk' to 'task' in
some stacktrace functions in preparation of following
patch which converts powerpc to generic ARCH_STACKWALK.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/stacktrace.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index a2a050551109..5b93650bc16c 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -27,13 +27,13 @@
* Save stack-backtrace addresses into a stack_trace buffer.
*/
static void save_context_stack(struct stack_trace *trace, unsigned long sp,
- struct task_struct *tsk, int savesched)
+ struct task_struct *task, int savesched)
{
for (;;) {
unsigned long *stack = (unsigned long *) sp;
unsigned long newsp, ip;
- if (!validate_sp(sp, tsk, STACK_FRAME_OVERHEAD))
+ if (!validate_sp(sp, task, STACK_FRAME_OVERHEAD))
return;
newsp = stack[0];
@@ -94,18 +94,18 @@ EXPORT_SYMBOL_GPL(save_stack_trace_regs);
*
* If the task is not 'current', the caller *must* ensure the task is inactive.
*/
-static int __save_stack_trace_tsk_reliable(struct task_struct *tsk,
+static int __save_stack_trace_tsk_reliable(struct task_struct *task,
struct stack_trace *trace)
{
unsigned long sp;
unsigned long newsp;
- unsigned long stack_page = (unsigned long)task_stack_page(tsk);
+ unsigned long stack_page = (unsigned long)task_stack_page(task);
unsigned long stack_end;
int graph_idx = 0;
bool firstframe;
stack_end = stack_page + THREAD_SIZE;
- if (!is_idle_task(tsk)) {
+ if (!is_idle_task(task)) {
/*
* For user tasks, this is the SP value loaded on
* kernel entry, see "PACAKSAVE(r13)" in _switch() and
@@ -129,10 +129,10 @@ static int __save_stack_trace_tsk_reliable(struct task_struct *tsk,
stack_end -= STACK_FRAME_OVERHEAD;
}
- if (tsk == current)
+ if (task == current)
sp = current_stack_frame();
else
- sp = tsk->thread.ksp;
+ sp = task->thread.ksp;
if (sp < stack_page + sizeof(struct thread_struct) ||
sp > stack_end - STACK_FRAME_MIN_SIZE) {
@@ -181,7 +181,7 @@ static int __save_stack_trace_tsk_reliable(struct task_struct *tsk,
* FIXME: IMHO these tests do not belong in
* arch-dependent code, they are generic.
*/
- ip = ftrace_graph_ret_addr(tsk, &graph_idx, ip, stack);
+ ip = ftrace_graph_ret_addr(task, &graph_idx, ip, stack);
#ifdef CONFIG_KPROBES
/*
* Mark stacktraces with kretprobed functions on them
--
2.25.0
^ permalink raw reply related
* [PATCH v1 4/4] powerpc: Fix arch_stack_walk() to have running function as first entry
From: Christophe Leroy @ 2021-03-16 7:57 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <955248c6423cb068c5965923121ba31d4dd2fdde.1615881400.git.christophe.leroy@csgroup.eu>
It seems like other architectures, namely x86 and arm64 and riscv
at least, include the running function as top entry when saving
stack trace with save_stack_trace_regs().
Functionnalities like KFENCE expect it.
Do the same on powerpc, it allows KFENCE and other users to
properly identify the faulting function as depicted below.
Before the patch KFENCE was identifying finish_task_switch.isra
as the faulting function.
[ 14.937370] ==================================================================
[ 14.948692] BUG: KFENCE: invalid read in test_invalid_access+0x54/0x108
[ 14.948692]
[ 14.956814] Invalid read at 0xdf98800a:
[ 14.960664] test_invalid_access+0x54/0x108
[ 14.964876] finish_task_switch.isra.0+0x54/0x23c
[ 14.969606] kunit_try_run_case+0x5c/0xd0
[ 14.973658] kunit_generic_run_threadfn_adapter+0x24/0x30
[ 14.979079] kthread+0x15c/0x174
[ 14.982342] ret_from_kernel_thread+0x14/0x1c
[ 14.986731]
[ 14.988236] CPU: 0 PID: 111 Comm: kunit_try_catch Tainted: G B 5.12.0-rc1-01537-g95f6e2088d7e-dirty #4682
[ 14.999795] NIP: c016ec2c LR: c02f517c CTR: c016ebd8
[ 15.004851] REGS: e2449d90 TRAP: 0301 Tainted: G B (5.12.0-rc1-01537-g95f6e2088d7e-dirty)
[ 15.015274] MSR: 00009032 <EE,ME,IR,DR,RI> CR: 22000004 XER: 00000000
[ 15.022043] DAR: df98800a DSISR: 20000000
[ 15.022043] GPR00: c02f517c e2449e50 c1142080 e100dd24 c084b13c 00000008 c084b32b c016ebd8
[ 15.022043] GPR08: c0850000 df988000 c0d10000 e2449eb0 22000288
[ 15.040581] NIP [c016ec2c] test_invalid_access+0x54/0x108
[ 15.046010] LR [c02f517c] kunit_try_run_case+0x5c/0xd0
[ 15.051181] Call Trace:
[ 15.053637] [e2449e50] [c005a68c] finish_task_switch.isra.0+0x54/0x23c (unreliable)
[ 15.061338] [e2449eb0] [c02f517c] kunit_try_run_case+0x5c/0xd0
[ 15.067215] [e2449ed0] [c02f648c] kunit_generic_run_threadfn_adapter+0x24/0x30
[ 15.074472] [e2449ef0] [c004e7b0] kthread+0x15c/0x174
[ 15.079571] [e2449f30] [c001317c] ret_from_kernel_thread+0x14/0x1c
[ 15.085798] Instruction dump:
[ 15.088784] 8129d608 38e7ebd8 81020280 911f004c 39000000 995f0024 907f0028 90ff001c
[ 15.096613] 3949000a 915f0020 3d40c0d1 3d00c085 <8929000a> 3908adb0 812a4b98 3d40c02f
[ 15.104612] ==================================================================
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Fixes: 35de3b1aa168 ("powerpc: Implement save_stack_trace_regs() to enable kprobe stack tracing")
Acked-by: Marco Elver <elver@google.com>
---
arch/powerpc/kernel/stacktrace.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/kernel/stacktrace.c b/arch/powerpc/kernel/stacktrace.c
index 80f92f5b5393..1deb1bf331dd 100644
--- a/arch/powerpc/kernel/stacktrace.c
+++ b/arch/powerpc/kernel/stacktrace.c
@@ -28,6 +28,9 @@ void arch_stack_walk(stack_trace_consume_fn consume_entry, void *cookie,
{
unsigned long sp;
+ if (regs && !consume_entry(cookie, regs->nip))
+ return;
+
if (regs)
sp = regs->gpr[1];
else if (task == current)
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] powerpc/64s: power4 nap fixup in C
From: Nicholas Piggin @ 2021-03-16 8:11 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev
In-Reply-To: <6ab2073b-dd10-ef2c-375d-1300f071ae1a@csgroup.eu>
Excerpts from Christophe Leroy's message of March 16, 2021 5:16 pm:
>
>
> Le 12/03/2021 à 02:20, Nicholas Piggin a écrit :
>> There is no need for this to be in asm, use the new intrrupt entry wrapper.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> Hopefully this works on a real G5 now, but I couldn't reproduce the
>> problem with QEMU.
>>
>> Thanks,
>> Nick
>>
>> arch/powerpc/include/asm/interrupt.h | 19 +++++++++++
>> arch/powerpc/include/asm/processor.h | 1 +
>> arch/powerpc/include/asm/thread_info.h | 6 ++++
>> arch/powerpc/kernel/exceptions-64s.S | 45 --------------------------
>> arch/powerpc/kernel/idle_book3s.S | 4 +++
>> 5 files changed, 30 insertions(+), 45 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
>> index aedfba29e43a..ef015d3b5e39 100644
>> --- a/arch/powerpc/include/asm/interrupt.h
>> +++ b/arch/powerpc/include/asm/interrupt.h
>> @@ -9,6 +9,17 @@
>> #include <asm/kprobes.h>
>> #include <asm/runlatch.h>
>>
>> +static inline void nap_adjust_return(struct pt_regs *regs)
>> +{
>> +#ifdef CONFIG_PPC_970_NAP
>> + if (unlikely(test_thread_local_flags(_TLF_NAPPING))) {
>> + /* Can avoid a test-and-clear because NMIs do not call this */
>> + clear_thread_local_flags(_TLF_NAPPING);
>> + regs->nip = (unsigned long)power4_idle_nap_return;
>
> Why don't you do regs->nip = regs->link like PPC32 instead of going via an intermediate symbol that
> does nothing else than branching to LR ?
It is supposed to keep the return branch predictor balanced.
I don't know if these CPUs have one, if it gets lost during nap, or if
nap latency is so high it really doesn't matter. But I think it is good
practice to make a habit of keeping things balanced.
Thanks,
Nick
^ permalink raw reply
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