* 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 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] 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 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] 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 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 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 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 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 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] 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 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 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 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 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 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 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 v3 00/41] KVM: PPC: Book3S: C-ify the P9 entry/exit code
From: Nicholas Piggin @ 2021-03-16 6:06 UTC (permalink / raw)
To: kvm-ppc; +Cc: Fabiano Rosas, linuxppc-dev, Daniel Axtens
In-Reply-To: <20210305150638.2675513-1-npiggin@gmail.com>
Excerpts from Nicholas Piggin's message of March 6, 2021 1:05 am:
> This tidies up things, and fixes a few corner cases and unfinished
> "XXX:" bits. The major one being "bad host interrupt" detection and
> handling (taking a MCE/SRESET while switching into guest regs) which
> was missing previously from the new P9 path.
>
> Adds a few new patches and makes some changes for issues noticed
> by reviewers (thank you).
I've put a tree up here with latest changes.
https://github.com/npiggin/linux/tree/kvm-in-c
Should include all the review comments from this thread so far, no
other major changes.
I'll hold off posting out another series probably at least until
powerpc next solidifies a bit more (some of the interrupt handler
changes pending there might clash).
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH] powerpc/numa: Fix topology_physical_package_id() on pSeries
From: Srikar Dronamraju @ 2021-03-16 5:23 UTC (permalink / raw)
To: G
Cc: Nathan Lynch, Daniel Henrique Barboza, Greg Kurz, Vasant Hegde,
linuxppc-dev, David Gibson
In-Reply-To: <20210312143154.3181109-1-clg@kaod.org>
* C?dric Le Goater <clg@kaod.org> [2021-03-12 15:31:54]:
> Initial commit 15863ff3b8da ("powerpc: Make chip-id information
> available to userspace") introduce a cpu_to_chip_id() routine for the
> PowerNV platform using the "ibm,chip-id" property to query the chip id
> of a CPU. But PAPR does not specify such a property and the node id
> query is broken.
>
> Use cpu_to_node() instead which guarantees to have a correct value on
> all platforms, PowerNV an pSeries.
>
While this looks good to me, @mpe had reservations on using nid as chip-id.
https://lore.kernel.org/linuxppc-dev/87lfwhypv0.fsf@concordia.ellerman.id.au/t/#u
He may be okay with using nid as a "virtual" package id in a pseries
environment.
Reviewed-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> Cc: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> arch/powerpc/include/asm/topology.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/topology.h b/arch/powerpc/include/asm/topology.h
> index 3beeb030cd78..887c42a4e43d 100644
> --- a/arch/powerpc/include/asm/topology.h
> +++ b/arch/powerpc/include/asm/topology.h
> @@ -123,7 +123,7 @@ static inline int cpu_to_coregroup_id(int cpu)
> #ifdef CONFIG_PPC64
> #include <asm/smp.h>
>
> -#define topology_physical_package_id(cpu) (cpu_to_chip_id(cpu))
> +#define topology_physical_package_id(cpu) (cpu_to_node(cpu))
>
> #define topology_sibling_cpumask(cpu) (per_cpu(cpu_sibling_map, cpu))
> #define topology_core_cpumask(cpu) (cpu_cpu_mask(cpu))
> --
> 2.26.2
>
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH] scsi: ibmvfc: Switch to using the new API kobj_to_dev()
From: Martin K. Petersen @ 2021-03-16 3:14 UTC (permalink / raw)
To: Jiapeng Chong, tyreld
Cc: Martin K . Petersen, linux-scsi, jejb, linux-kernel, paulus,
linuxppc-dev
In-Reply-To: <1614850124-54111-1-git-send-email-jiapeng.chong@linux.alibaba.com>
On Thu, 4 Mar 2021 17:28:44 +0800, Jiapeng Chong wrote:
> Fix the following coccicheck warnings:
>
> ./drivers/scsi/ibmvscsi/ibmvfc.c:3483: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 3/3] powerpc/qspinlock: Use generic smp_cond_load_relaxed
From: Nicholas Piggin @ 2021-03-16 4:59 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: will, Davidlohr Bueso, peterz, linux-kernel, mingo, paulus,
longman, linuxppc-dev
In-Reply-To: <20210309015950.27688-4-dave@stgolabs.net>
Excerpts from Davidlohr Bueso's message of March 9, 2021 11:59 am:
> 49a7d46a06c3 (powerpc: Implement smp_cond_load_relaxed()) added
> busy-waiting pausing with a preferred SMT priority pattern, lowering
> the priority (reducing decode cycles) during the whole loop slowpath.
>
> However, data shows that while this pattern works well with simple
> spinlocks, queued spinlocks benefit more being kept in medium priority,
> with a cpu_relax() instead, being a low+medium combo on powerpc.
Thanks for tracking this down and the comprehensive results, great
work.
It's only a relatively recent patch, so I think the revert is a
good idea (i.e., don't keep it around for possibly other code to
hit problems with).
One request, could you add a comment in place that references
smp_cond_load_relaxed() so this commit can be found again if
someone looks at it? Something like this
/*
* smp_cond_load_relaxed was found to have performance problems if
* implemented with spin_begin()/spin_end().
*/
I wonder if it should have a Fixes: tag to the original commit as
well.
Otherwise,
Acked-by: Nicholas Piggin <npiggin@gmail.com>
Thanks,
Nick
>
> Data is from three benchmarks on a Power9: 9008-22L 64 CPUs with
> 2 sockets and 8 threads per core.
>
> 1. locktorture.
>
> This is data for the lowest and most artificial/pathological level,
> with increasing thread counts pounding on the lock. Metrics are total
> ops/minute. Despite some small hits in the 4-8 range, scenarios are
> either neutral or favorable to this patch.
>
> +=========+==========+==========+=======+
> | # tasks | vanilla | dirty | %diff |
> +=========+==========+==========+=======+
> | 2 | 46718565 | 48751350 | 4.35 |
> +---------+----------+----------+-------+
> | 4 | 51740198 | 50369082 | -2.65 |
> +---------+----------+----------+-------+
> | 8 | 63756510 | 62568821 | -1.86 |
> +---------+----------+----------+-------+
> | 16 | 67824531 | 70966546 | 4.63 |
> +---------+----------+----------+-------+
> | 32 | 53843519 | 61155508 | 13.58 |
> +---------+----------+----------+-------+
> | 64 | 53005778 | 53104412 | 0.18 |
> +---------+----------+----------+-------+
> | 128 | 53331980 | 54606910 | 2.39 |
> +=========+==========+==========+=======+
>
> 2. sockperf (tcp throughput)
>
> Here a client will do one-way throughput tests to a localhost server, with
> increasing message sizes, dealing with the sk_lock. This patch shows to put
> the performance of the qspinlock back to par with that of the simple lock:
>
> simple-spinlock vanilla dirty
> Hmean 14 73.50 ( 0.00%) 54.44 * -25.93%* 73.45 * -0.07%*
> Hmean 100 654.47 ( 0.00%) 385.61 * -41.08%* 771.43 * 17.87%*
> Hmean 300 2719.39 ( 0.00%) 2181.67 * -19.77%* 2666.50 * -1.94%*
> Hmean 500 4400.59 ( 0.00%) 3390.77 * -22.95%* 4322.14 * -1.78%*
> Hmean 850 6726.21 ( 0.00%) 5264.03 * -21.74%* 6863.12 * 2.04%*
>
> 3. dbench (tmpfs)
>
> Configured to run with up to ncpusx8 clients, it shows both latency and
> throughput metrics. For the latency, with the exception of the 64 case,
> there is really nothing to go by:
> vanilla dirty
> Amean latency-1 1.67 ( 0.00%) 1.67 * 0.09%*
> Amean latency-2 2.15 ( 0.00%) 2.08 * 3.36%*
> Amean latency-4 2.50 ( 0.00%) 2.56 * -2.27%*
> Amean latency-8 2.49 ( 0.00%) 2.48 * 0.31%*
> Amean latency-16 2.69 ( 0.00%) 2.72 * -1.37%*
> Amean latency-32 2.96 ( 0.00%) 3.04 * -2.60%*
> Amean latency-64 7.78 ( 0.00%) 8.17 * -5.07%*
> Amean latency-512 186.91 ( 0.00%) 186.41 * 0.27%*
>
> For the dbench4 Throughput (misleading but traditional) there's a small
> but rather constant improvement:
>
> vanilla dirty
> Hmean 1 849.13 ( 0.00%) 851.51 * 0.28%*
> Hmean 2 1664.03 ( 0.00%) 1663.94 * -0.01%*
> Hmean 4 3073.70 ( 0.00%) 3104.29 * 1.00%*
> Hmean 8 5624.02 ( 0.00%) 5694.16 * 1.25%*
> Hmean 16 9169.49 ( 0.00%) 9324.43 * 1.69%*
> Hmean 32 11969.37 ( 0.00%) 12127.09 * 1.32%*
> Hmean 64 15021.12 ( 0.00%) 15243.14 * 1.48%*
> Hmean 512 14891.27 ( 0.00%) 15162.11 * 1.82%*
>
> Measuring the dbench4 Per-VFS Operation latency, shows some very minor
> differences within the noise level, around the 0-1% ranges.
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
> arch/powerpc/include/asm/barrier.h | 16 ----------------
> 1 file changed, 16 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
> index aecfde829d5d..7ae29cfb06c0 100644
> --- a/arch/powerpc/include/asm/barrier.h
> +++ b/arch/powerpc/include/asm/barrier.h
> @@ -80,22 +80,6 @@ do { \
> ___p1; \
> })
>
> -#ifdef CONFIG_PPC64
> -#define smp_cond_load_relaxed(ptr, cond_expr) ({ \
> - typeof(ptr) __PTR = (ptr); \
> - __unqual_scalar_typeof(*ptr) VAL; \
> - VAL = READ_ONCE(*__PTR); \
> - if (unlikely(!(cond_expr))) { \
> - spin_begin(); \
> - do { \
> - VAL = READ_ONCE(*__PTR); \
> - } while (!(cond_expr)); \
> - spin_end(); \
> - } \
> - (typeof(*ptr))VAL; \
> -})
> -#endif
> -
> #ifdef CONFIG_PPC_BOOK3S_64
> #define NOSPEC_BARRIER_SLOT nop
> #elif defined(CONFIG_PPC_FSL_BOOK3E)
> --
> 2.26.2
>
>
^ permalink raw reply
* [PATCH] powerpc: arch/powerpc/kernel/setup_64.c - cleanup warnings
From: He Ying @ 2021-03-16 4:11 UTC (permalink / raw)
To: mpe, benh, paulus, npiggin, dja, akpm, aneesh.kumar, rppt, ardb,
clg, christophe.leroy
Cc: johnny.chenyi, linuxppc-dev, linux-kernel
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>
---
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);
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v9 2/8] powerpc/lib/code-patching: Set up Strict RWX patching earlier
From: Russell Currey @ 2021-03-16 3:36 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev
Cc: christophe.leroy, naveen.n.rao, ajd, npiggin, dja
In-Reply-To: <20210316031741.1004850-2-jniethe5@gmail.com>
On Tue, 2021-03-16 at 14:17 +1100, Jordan Niethe wrote:
> 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>
Good job finding & fixing this bug!
Reviewed-by: Russell Currey <ruscur@russell.cc>
> ---
> 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().
> */
> -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 v3 12/41] KVM: PPC: Book3S 64: Move hcall early register setup to KVM
From: Nicholas Piggin @ 2021-03-16 3:43 UTC (permalink / raw)
To: Daniel Axtens, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <87czw57wn8.fsf@linkitivity.dja.id.au>
Excerpts from Daniel Axtens's message of March 12, 2021 3:45 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
>> System calls / hcalls have a different calling convention than
>> other interrupts, so there is code in the KVMTEST to massage these
>> into the same form as other interrupt handlers.
>>
>> Move this work into the KVM hcall handler. This means teaching KVM
>> a little more about the low level interrupt handler setup, PACA save
>> areas, etc., although that's not obviously worse than the current
>> approach of coming up with an entirely different interrupt register
>> / save convention.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/exception-64s.h | 13 ++++++++
>> arch/powerpc/kernel/exceptions-64s.S | 42 +-----------------------
>> arch/powerpc/kvm/book3s_64_entry.S | 17 ++++++++++
>> 3 files changed, 31 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
>> index c1a8aac01cf9..bb6f78fcf981 100644
>> --- a/arch/powerpc/include/asm/exception-64s.h
>> +++ b/arch/powerpc/include/asm/exception-64s.h
>> @@ -35,6 +35,19 @@
>> /* PACA save area size in u64 units (exgen, exmc, etc) */
>> #define EX_SIZE 10
>>
>> +/* PACA save area offsets */
>> +#define EX_R9 0
>> +#define EX_R10 8
>> +#define EX_R11 16
>> +#define EX_R12 24
>> +#define EX_R13 32
>> +#define EX_DAR 40
>> +#define EX_DSISR 48
>> +#define EX_CCR 52
>> +#define EX_CFAR 56
>> +#define EX_PPR 64
>> +#define EX_CTR 72
>> +
>> /*
>> * maximum recursive depth of MCE exceptions
>> */
>> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
>> index 292435bd80f0..b7092ba87da8 100644
>> --- a/arch/powerpc/kernel/exceptions-64s.S
>> +++ b/arch/powerpc/kernel/exceptions-64s.S
>> @@ -21,22 +21,6 @@
>> #include <asm/feature-fixups.h>
>> #include <asm/kup.h>
>>
>> -/* PACA save area offsets (exgen, exmc, etc) */
>> -#define EX_R9 0
>> -#define EX_R10 8
>> -#define EX_R11 16
>> -#define EX_R12 24
>> -#define EX_R13 32
>> -#define EX_DAR 40
>> -#define EX_DSISR 48
>> -#define EX_CCR 52
>> -#define EX_CFAR 56
>> -#define EX_PPR 64
>> -#define EX_CTR 72
>> -.if EX_SIZE != 10
>> - .error "EX_SIZE is wrong"
>> -.endif
>> -
>> /*
>> * Following are fixed section helper macros.
>> *
>> @@ -1964,29 +1948,8 @@ EXC_VIRT_END(system_call, 0x4c00, 0x100)
>>
>> #ifdef CONFIG_KVM_BOOK3S_64_HANDLER
>> TRAMP_REAL_BEGIN(system_call_kvm)
>> - /*
>> - * This is a hcall, so register convention is as above, with these
>> - * differences:
>> - * r13 = PACA
>> - * ctr = orig r13
>> - * orig r10 saved in PACA
>> - */
>> - /*
>> - * Save the PPR (on systems that support it) before changing to
>> - * HMT_MEDIUM. That allows the KVM code to save that value into the
>> - * guest state (it is the guest's PPR value).
>> - */
>> -BEGIN_FTR_SECTION
>> - mfspr r10,SPRN_PPR
>> - std r10,HSTATE_PPR(r13)
>> -END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> - HMT_MEDIUM
>> mfctr r10
>> - SET_SCRATCH0(r10)
>> - mfcr r10
>> - std r12,HSTATE_SCRATCH0(r13)
>> - sldi r12,r10,32
>> - ori r12,r12,0xc00
>> + SET_SCRATCH0(r10) /* Save r13 in SCRATCH0 */
>
> If I've understood correctly, you've saved the _original_/guest r13 in
> SCRATCH0. That makes sense - it just took me a while to follow the
> logic, especially because the parameter to SET_SCRATCH0 is r10, not r13.
>
> I would probably expand the comment to say the original or guest r13 (as
> you do in the comment at the start of kvmppc_hcall), but if there's a
> convention here that I've missed that might not be necessary.
There is a convention which is that all kvm interrupts including system
call come in with r13 saved in SCRATCH0, although that's all in a state
of flux throughput this series of course.
I added the comment because I moved the bigger comment here, I think
that's okay because you're always referring to interrupted context
(i.e., guest in this case) when talking about saved registers.
>
>> #ifdef CONFIG_RELOCATABLE
>> /*
>> * Requires __LOAD_FAR_HANDLER beause kvmppc_hcall lives
>> @@ -1994,15 +1957,12 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>> */
>> __LOAD_FAR_HANDLER(r10, kvmppc_hcall)
>> mtctr r10
>> - ld r10,PACA_EXGEN+EX_R10(r13)
>> bctr
>> #else
>> - ld r10,PACA_EXGEN+EX_R10(r13)
>> b kvmppc_hcall
>> #endif
>> #endif
>>
>> -
>> /**
>> * Interrupt 0xd00 - Trace Interrupt.
>> * This is a synchronous interrupt in response to instruction step or
>> diff --git a/arch/powerpc/kvm/book3s_64_entry.S b/arch/powerpc/kvm/book3s_64_entry.S
>> index 8cf5e24a81eb..a7b6edd18bc8 100644
>> --- a/arch/powerpc/kvm/book3s_64_entry.S
>> +++ b/arch/powerpc/kvm/book3s_64_entry.S
>> @@ -14,6 +14,23 @@
>> .global kvmppc_hcall
>> .balign IFETCH_ALIGN_BYTES
>> kvmppc_hcall:
>> + /*
>> + * This is a hcall, so register convention is as
>> + * Documentation/powerpc/papr_hcalls.rst, with these additions:
>> + * R13 = PACA
>> + * guest R13 saved in SPRN_SCRATCH0
>> + * R10 = free
>> + */
>> +BEGIN_FTR_SECTION
>> + mfspr r10,SPRN_PPR
>> + std r10,HSTATE_PPR(r13)
>> +END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
>
> Do we want to preserve the comment about why we save the PPR?
Wouldn't hurt. I think the reason the comment is there is because it's a
difference with system calls. Hcalls preserve the PPR, system calls do not.
Should probably leave the "orig r10 saved in the PACA" comment too.
>
>> + HMT_MEDIUM
>> + mfcr r10
>> + std r12,HSTATE_SCRATCH0(r13)
>> + sldi r12,r10,32
>> + ori r12,r12,0xc00
>
> I see that this is a direct copy from the earlier code, but it confuses
> me a bit. Looking at exceptions-64s.S, there's the following comment:
>
> * In HPT, sc 1 always goes to 0xc00 real mode. In RADIX, sc 1 can go to
> * 0x4c00 virtual mode.
>
> However, this code uncondionally sets the low bits to be c00, even if
> the exception came in via 4c00. Is this right? Do we need to pass
> that through somehow?
We don't need to. The "trap" numbers are always the real-mode vectors
(except scv which is a bit weird) by convention.
>
>> + ld r10,PACA_EXGEN+EX_R10(r13)
>>
>
> Otherwise, this looks good to me so far.
Thanks,
Nick
^ permalink raw reply
* [PATCH v9 8/8] powerpc/32: use set_memory_attr()
From: Jordan Niethe @ 2021-03-16 3:17 UTC (permalink / raw)
To: linuxppc-dev
Cc: christophe.leroy, ajd, npiggin, naveen.n.rao, Jordan Niethe, dja
In-Reply-To: <20210316031741.1004850-1-jniethe5@gmail.com>
From: Christophe Leroy <christophe.leroy@c-s.fr>
Use set_memory_attr() instead of the PPC32 specific change_page_attr()
change_page_attr() was checking that the address was not mapped by
blocks and was handling highmem, but that's unneeded because the
affected pages can't be in highmem and block mapping verification
is already done by the callers.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
[ruscur: rebase on powerpc/merge with Christophe's new patches]
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
---
arch/powerpc/mm/pgtable_32.c | 60 ++++++------------------------------
1 file changed, 10 insertions(+), 50 deletions(-)
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index e0ec67a16887..dcf5ecca19d9 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -23,6 +23,7 @@
#include <linux/highmem.h>
#include <linux/memblock.h>
#include <linux/slab.h>
+#include <linux/set_memory.h>
#include <asm/pgalloc.h>
#include <asm/fixmap.h>
@@ -132,64 +133,20 @@ void __init mapin_ram(void)
}
}
-static int __change_page_attr_noflush(struct page *page, pgprot_t prot)
-{
- pte_t *kpte;
- unsigned long address;
-
- BUG_ON(PageHighMem(page));
- address = (unsigned long)page_address(page);
-
- if (v_block_mapped(address))
- return 0;
- kpte = virt_to_kpte(address);
- if (!kpte)
- return -EINVAL;
- __set_pte_at(&init_mm, address, kpte, mk_pte(page, prot), 0);
-
- return 0;
-}
-
-/*
- * Change the page attributes of an page in the linear mapping.
- *
- * THIS DOES NOTHING WITH BAT MAPPINGS, DEBUG USE ONLY
- */
-static int change_page_attr(struct page *page, int numpages, pgprot_t prot)
-{
- int i, err = 0;
- unsigned long flags;
- struct page *start = page;
-
- local_irq_save(flags);
- for (i = 0; i < numpages; i++, page++) {
- err = __change_page_attr_noflush(page, prot);
- if (err)
- break;
- }
- wmb();
- local_irq_restore(flags);
- flush_tlb_kernel_range((unsigned long)page_address(start),
- (unsigned long)page_address(page));
- return err;
-}
-
void mark_initmem_nx(void)
{
- struct page *page = virt_to_page(_sinittext);
unsigned long numpages = PFN_UP((unsigned long)_einittext) -
PFN_DOWN((unsigned long)_sinittext);
if (v_block_mapped((unsigned long)_sinittext))
mmu_mark_initmem_nx();
else
- change_page_attr(page, numpages, PAGE_KERNEL);
+ set_memory_attr((unsigned long)_sinittext, numpages, PAGE_KERNEL);
}
#ifdef CONFIG_STRICT_KERNEL_RWX
void mark_rodata_ro(void)
{
- struct page *page;
unsigned long numpages;
if (v_block_mapped((unsigned long)_stext + 1)) {
@@ -198,20 +155,18 @@ void mark_rodata_ro(void)
return;
}
- page = virt_to_page(_stext);
numpages = PFN_UP((unsigned long)_etext) -
PFN_DOWN((unsigned long)_stext);
- change_page_attr(page, numpages, PAGE_KERNEL_ROX);
+ set_memory_attr((unsigned long)_stext, numpages, PAGE_KERNEL_ROX);
/*
* mark .rodata as read only. Use __init_begin rather than __end_rodata
* to cover NOTES and EXCEPTION_TABLE.
*/
- page = virt_to_page(__start_rodata);
numpages = PFN_UP((unsigned long)__init_begin) -
PFN_DOWN((unsigned long)__start_rodata);
- change_page_attr(page, numpages, PAGE_KERNEL_RO);
+ set_memory_attr((unsigned long)__start_rodata, numpages, PAGE_KERNEL_RO);
// mark_initmem_nx() should have already run by now
ptdump_check_wx();
@@ -221,9 +176,14 @@ void mark_rodata_ro(void)
#ifdef CONFIG_DEBUG_PAGEALLOC
void __kernel_map_pages(struct page *page, int numpages, int enable)
{
+ unsigned long addr = (unsigned long)page_address(page);
+
if (PageHighMem(page))
return;
- change_page_attr(page, numpages, enable ? PAGE_KERNEL : __pgprot(0));
+ if (enable)
+ set_memory_attr(addr, numpages, PAGE_KERNEL);
+ else
+ set_memory_attr(addr, numpages, __pgprot(0));
}
#endif /* CONFIG_DEBUG_PAGEALLOC */
--
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