* Re: [PATCH v2] powerpc/traps: Enhance readability for trap types
From: Michael Ellerman @ 2021-03-31 9:58 UTC (permalink / raw)
To: Xiongwei Song, benh, paulus, oleg, npiggin, christophe.leroy,
msuchanek, aneesh.kumar, ravi.bangoria, mikey, haren, alistair,
jniethe5, peterz, leobras.c, akpm, rppt, peterx, atrajeev, maddy,
kjain, kan.liang, aik, pmladek, john.ogness
Cc: Xiongwei Song, linuxppc-dev, linux-kernel, kvm-ppc
In-Reply-To: <20210330150425.10145-1-sxwjean@me.com>
Xiongwei Song <sxwjean@me.com> writes:
> From: Xiongwei Song <sxwjean@gmail.com>
>
> Create a new header named traps.h, define macros to list ppc exception
> types in traps.h, replace the reference of the real trap values with
> these macros.
Personally I find the hex values easier to recognise, but I realise
that's probably not true of other people :)
...
> diff --git a/arch/powerpc/include/asm/traps.h b/arch/powerpc/include/asm/traps.h
> new file mode 100644
> index 000000000000..a31b6122de23
> --- /dev/null
> +++ b/arch/powerpc/include/asm/traps.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_PPC_TRAPS_H
> +#define _ASM_PPC_TRAPS_H
> +
> +#define TRAP_RESET 0x100 /* System reset */
> +#define TRAP_MCE 0x200 /* Machine check */
> +#define TRAP_DSI 0x300 /* Data storage */
> +#define TRAP_DSEGI 0x380 /* Data segment */
> +#define TRAP_ISI 0x400 /* Instruction storage */
> +#define TRAP_ISEGI 0x480 /* Instruction segment */
> +#define TRAP_ALIGN 0x600 /* Alignment */
> +#define TRAP_PROG 0x700 /* Program */
> +#define TRAP_DEC 0x900 /* Decrementer */
> +#define TRAP_SYSCALL 0xc00 /* System call */
> +#define TRAP_TRACEI 0xd00 /* Trace */
> +#define TRAP_FPA 0xe00 /* Floating-point Assist */
> +#define TRAP_PMI 0xf00 /* Performance monitor */
I know the macro is called TRAP and the field in pt_regs is called trap,
but the terminology in the architecture is "exception", and we already
have many uses of that. In particular we have a lot of uses of "exc" as
an abbreviation for "exception". So I think I'd rather we use that than
"TRAP".
I think we should probably use the names from the ISA, unless they are
really over long.
Which are:
0x100 System Reset
0x200 Machine Check
0x300 Data Storage
0x380 Data Segment
0x400 Instruction Storage
0x480 Instruction Segment
0x500 External
0x600 Alignment
0x700 Program
0x800 Floating-Point Unavailable
0x900 Decrementer
0x980 Hypervisor Decrementer
0xA00 Directed Privileged Doorbell
0xC00 System Call
0xD00 Trace
0xE00 Hypervisor Data Storage
0xE20 Hypervisor Instruction Storage
0xE40 Hypervisor Emulation Assistance
0xE60 Hypervisor Maintenance
0xE80 Directed Hypervisor Doorbell
0xEA0 Hypervisor Virtualization
0xF00 Performance Monitor
0xF20 Vector Unavailable
0xF40 VSX Unavailable
0xF60 Facility Unavailable
0xF80 Hypervisor Facility Unavailable
0xFA0 Directed Ultravisor Doorbell
So perhaps:
EXC_SYSTEM_RESET
EXC_MACHINE_CHECK
EXC_DATA_STORAGE
EXC_DATA_SEGMENT
EXC_INST_STORAGE
EXC_INST_SEGMENT
EXC_EXTERNAL_INTERRUPT
EXC_ALIGNMENT
EXC_PROGRAM_CHECK
EXC_FP_UNAVAILABLE
EXC_DECREMENTER
EXC_HV_DECREMENTER
EXC_SYSTEM_CALL
EXC_HV_DATA_STORAGE
EXC_PERF_MONITOR
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/vdso: Separate vvar vma from vdso
From: Michael Ellerman @ 2021-03-31 9:59 UTC (permalink / raw)
To: Christophe Leroy, Dmitry Safonov, linux-kernel
Cc: Dmitry Safonov, stable, Andrei Vagin, Paul Mackerras,
Andy Lutomirski, Laurent Dufour, linuxppc-dev
In-Reply-To: <09e8d68d-54fe-e327-b44f-8f68543edba1@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 26/03/2021 à 20:17, Dmitry Safonov a écrit :
>> Since commit 511157ab641e ("powerpc/vdso: Move vdso datapage up front")
>> VVAR page is in front of the VDSO area. In result it breaks CRIU
>> (Checkpoint Restore In Userspace) [1], where CRIU expects that "[vdso]"
>> from /proc/../maps points at ELF/vdso image, rather than at VVAR data page.
>> Laurent made a patch to keep CRIU working (by reading aux vector).
>> But I think it still makes sence to separate two mappings into different
>> VMAs. It will also make ppc64 less "special" for userspace and as
>> a side-bonus will make VVAR page un-writable by debugger (which previously
>> would COW page and can be unexpected).
>>
>> I opportunistically Cc stable on it: I understand that usually such
>> stuff isn't a stable material, but that will allow us in CRIU have
>> one workaround less that is needed just for one release (v5.11) on
>> one platform (ppc64), which we otherwise have to maintain.
>> I wouldn't go as far as to say that the commit 511157ab641e is ABI
>> regression as no other userspace got broken, but I'd really appreciate
>> if it gets backported to v5.11 after v5.12 is released, so as not
>> to complicate already non-simple CRIU-vdso code. Thanks!
>>
>> Cc: Andrei Vagin <avagin@gmail.com>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Cc: Laurent Dufour <ldufour@linux.ibm.com>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: stable@vger.kernel.org # v5.11
>> [1]: https://github.com/checkpoint-restore/criu/issues/1417
>> Signed-off-by: Dmitry Safonov <dima@arista.com>
>> Tested-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> arch/powerpc/include/asm/mmu_context.h | 2 +-
>> arch/powerpc/kernel/vdso.c | 54 +++++++++++++++++++-------
>> 2 files changed, 40 insertions(+), 16 deletions(-)
>>
>
>> @@ -133,7 +135,13 @@ static int __arch_setup_additional_pages(struct linux_binprm *bprm, int uses_int
>> * install_special_mapping or the perf counter mmap tracking code
>> * will fail to recognise it as a vDSO.
>> */
>> - mm->context.vdso = (void __user *)vdso_base + PAGE_SIZE;
>> + mm->context.vdso = (void __user *)vdso_base + vvar_size;
>> +
>> + vma = _install_special_mapping(mm, vdso_base, vvar_size,
>> + VM_READ | VM_MAYREAD | VM_IO |
>> + VM_DONTDUMP | VM_PFNMAP, &vvar_spec);
>> + if (IS_ERR(vma))
>> + return PTR_ERR(vma);
>>
>> /*
>> * our vma flags don't have VM_WRITE so by default, the process isn't
>
>
> IIUC, VM_PFNMAP is for when we have a vvar_fault handler.
Some of the other flags seem odd too.
eg. VM_IO ? VM_DONTDUMP ?
cheers
^ permalink raw reply
* Re: [PATCH v3] powerpc/papr_scm: Implement support for H_SCM_FLUSH hcall
From: Michael Ellerman @ 2021-03-31 10:20 UTC (permalink / raw)
To: Aneesh Kumar K.V, Shivaprasad G Bhat, sbhat, linuxppc-dev,
kvm-ppc, linux-nvdimm
Cc: vaibhav, linux-doc
In-Reply-To: <87mtul6xzj.fsf@linux.ibm.com>
"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> Shivaprasad G Bhat <sbhat@linux.ibm.com> writes:
>
>> Add support for ND_REGION_ASYNC capability if the device tree
>> indicates 'ibm,hcall-flush-required' property in the NVDIMM node.
>> Flush is done by issuing H_SCM_FLUSH hcall to the hypervisor.
>>
>> If the flush request failed, the hypervisor is expected to
>> to reflect the problem in the subsequent nvdimm H_SCM_HEALTH call.
>>
>> This patch prevents mmap of namespaces with MAP_SYNC flag if the
>> nvdimm requires an explicit flush[1].
>>
>> References:
>> [1] https://github.com/avocado-framework-tests/avocado-misc-tests/blob/master/memory/ndctl.py.data/map_sync.c
>
>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Do we need an ack from nvdimm folks on this?
Or is it entirely powerpc internal (seems like it from the diffstat)?
cheers
^ permalink raw reply
* Re: [PATCH v10 05/10] powerpc/bpf: Write protect JIT code
From: Michael Ellerman @ 2021-03-31 10:37 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev
Cc: ajd, Jordan Niethe, cmr, npiggin, naveen.n.rao, dja
In-Reply-To: <20210330045132.722243-6-jniethe5@gmail.com>
Jordan Niethe <jniethe5@gmail.com> writes:
> Once CONFIG_STRICT_MODULE_RWX is enabled there will be no need to
> override bpf_jit_free() because it is now possible to set images
> read-only. So use the default implementation.
>
> Also add the necessary call to bpf_jit_binary_lock_ro() which will
> remove write protection and add exec protection to the JIT image after
> it has finished being written.
>
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v10: New to series
> ---
> arch/powerpc/net/bpf_jit_comp.c | 5 ++++-
> arch/powerpc/net/bpf_jit_comp64.c | 4 ++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index e809cb5a1631..8015e4a7d2d4 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -659,12 +659,15 @@ void bpf_jit_compile(struct bpf_prog *fp)
> bpf_jit_dump(flen, proglen, pass, code_base);
>
> bpf_flush_icache(code_base, code_base + (proglen/4));
> -
> #ifdef CONFIG_PPC64
> /* Function descriptor nastiness: Address + TOC */
> ((u64 *)image)[0] = (u64)code_base;
> ((u64 *)image)[1] = local_paca->kernel_toc;
> #endif
> + if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX)) {
> + set_memory_ro((unsigned long)image, alloclen >> PAGE_SHIFT);
> + set_memory_x((unsigned long)image, alloclen >> PAGE_SHIFT);
> + }
You don't need to check the ifdef in a caller, there are stubs that
compile to nothing when CONFIG_ARCH_HAS_SET_MEMORY=n.
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> index aaf1a887f653..1484ad588685 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -1240,6 +1240,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
> fp->jited_len = alloclen;
>
> bpf_flush_icache(bpf_hdr, (u8 *)bpf_hdr + (bpf_hdr->pages * PAGE_SIZE));
> + if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
> + bpf_jit_binary_lock_ro(bpf_hdr);
Do we need the ifdef here either? Looks like it should be safe to call
due to the stubs.
> @@ -1262,6 +1264,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
> }
>
> /* Overriding bpf_jit_free() as we don't set images read-only. */
> +#ifndef CONFIG_STRICT_MODULE_RWX
Did you test without this and notice something broken?
Looking at the generic version I can't tell why we need to override
this. Maybe we don't (anymore?) ?
cheers
> void bpf_jit_free(struct bpf_prog *fp)
> {
> unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
> @@ -1272,3 +1275,4 @@ void bpf_jit_free(struct bpf_prog *fp)
>
> bpf_prog_unlock_free(fp);
> }
> +#endif
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH v10 05/10] powerpc/bpf: Write protect JIT code
From: Christophe Leroy @ 2021-03-31 10:39 UTC (permalink / raw)
To: Michael Ellerman, Jordan Niethe, linuxppc-dev
Cc: naveen.n.rao, cmr, ajd, npiggin, dja
In-Reply-To: <87wntnwqw9.fsf@mpe.ellerman.id.au>
Le 31/03/2021 à 12:37, Michael Ellerman a écrit :
> Jordan Niethe <jniethe5@gmail.com> writes:
>
>> Once CONFIG_STRICT_MODULE_RWX is enabled there will be no need to
>> override bpf_jit_free() because it is now possible to set images
>> read-only. So use the default implementation.
>>
>> Also add the necessary call to bpf_jit_binary_lock_ro() which will
>> remove write protection and add exec protection to the JIT image after
>> it has finished being written.
>>
>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>> ---
>> v10: New to series
>> ---
>> arch/powerpc/net/bpf_jit_comp.c | 5 ++++-
>> arch/powerpc/net/bpf_jit_comp64.c | 4 ++++
>> 2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
>> index e809cb5a1631..8015e4a7d2d4 100644
>> --- a/arch/powerpc/net/bpf_jit_comp.c
>> +++ b/arch/powerpc/net/bpf_jit_comp.c
>> @@ -659,12 +659,15 @@ void bpf_jit_compile(struct bpf_prog *fp)
>> bpf_jit_dump(flen, proglen, pass, code_base);
>>
>> bpf_flush_icache(code_base, code_base + (proglen/4));
>> -
>> #ifdef CONFIG_PPC64
>> /* Function descriptor nastiness: Address + TOC */
>> ((u64 *)image)[0] = (u64)code_base;
>> ((u64 *)image)[1] = local_paca->kernel_toc;
>> #endif
>> + if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX)) {
>> + set_memory_ro((unsigned long)image, alloclen >> PAGE_SHIFT);
>> + set_memory_x((unsigned long)image, alloclen >> PAGE_SHIFT);
>> + }
>
> You don't need to check the ifdef in a caller, there are stubs that
> compile to nothing when CONFIG_ARCH_HAS_SET_MEMORY=n.
I was about to do the same comment, but ....
CONFIG_STRICT_MODULE_RWX is not CONFIG_ARCH_HAS_SET_MEMORY
>
>> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
>> index aaf1a887f653..1484ad588685 100644
>> --- a/arch/powerpc/net/bpf_jit_comp64.c
>> +++ b/arch/powerpc/net/bpf_jit_comp64.c
>> @@ -1240,6 +1240,8 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>> fp->jited_len = alloclen;
>>
>> bpf_flush_icache(bpf_hdr, (u8 *)bpf_hdr + (bpf_hdr->pages * PAGE_SIZE));
>> + if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
>> + bpf_jit_binary_lock_ro(bpf_hdr);
>
> Do we need the ifdef here either? Looks like it should be safe to call
> due to the stubs.
Same
>
>> @@ -1262,6 +1264,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
>> }
>>
>> /* Overriding bpf_jit_free() as we don't set images read-only. */
>> +#ifndef CONFIG_STRICT_MODULE_RWX
>
> Did you test without this and notice something broken?
>
> Looking at the generic version I can't tell why we need to override
> this. Maybe we don't (anymore?) ?
>
> cheers
>
>> void bpf_jit_free(struct bpf_prog *fp)
>> {
>> unsigned long addr = (unsigned long)fp->bpf_func & PAGE_MASK;
>> @@ -1272,3 +1275,4 @@ void bpf_jit_free(struct bpf_prog *fp)
>>
>> bpf_prog_unlock_free(fp);
>> }
>> +#endif
>> --
>> 2.25.1
^ permalink raw reply
* Re: [PATCH v10 01/10] powerpc/mm: Implement set_memory() routines
From: Michael Ellerman @ 2021-03-31 11:16 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev
Cc: ajd, Jordan Niethe, cmr, npiggin, naveen.n.rao, dja
In-Reply-To: <20210330045132.722243-2-jniethe5@gmail.com>
Hi Jordan,
A few nits below ...
Jordan Niethe <jniethe5@gmail.com> writes:
> From: Russell Currey <ruscur@russell.cc>
>
> The set_memory_{ro/rw/nx/x}() functions are required for STRICT_MODULE_RWX,
> and are generally useful primitives to have. This implementation is
> designed to be completely generic across powerpc's many MMUs.
>
> It's possible that this could be optimised to be faster for specific
> MMUs, but the focus is on having a generic and safe implementation for
> now.
>
> This implementation does not handle cases where the caller is attempting
> to change the mapping of the page it is executing from, or if another
> CPU is concurrently using the page being altered. These cases likely
> shouldn't happen, but a more complex implementation with MMU-specific code
> could safely handle them, so that is left as a TODO for now.
>
> On hash the linear mapping is not kept in the linux pagetable, so this
> will not change the protection if used on that range. Currently these
> functions are not used on the linear map so just WARN for now.
>
> These functions do nothing if STRICT_KERNEL_RWX is not enabled.
>
> Reviewed-by: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> [jpn: -rebase on next plus "powerpc/mm/64s: Allow STRICT_KERNEL_RWX again"
> - WARN on hash linear map]
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v10: WARN if trying to change the hash linear map
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/set_memory.h | 32 ++++++++++
> arch/powerpc/mm/Makefile | 2 +-
> arch/powerpc/mm/pageattr.c | 88 +++++++++++++++++++++++++++
> 4 files changed, 122 insertions(+), 1 deletion(-)
> create mode 100644 arch/powerpc/include/asm/set_memory.h
> create mode 100644 arch/powerpc/mm/pageattr.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index fc7f5c5933e6..4498a27ac9db 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -135,6 +135,7 @@ config PPC
> select ARCH_HAS_MEMBARRIER_CALLBACKS
> select ARCH_HAS_MEMBARRIER_SYNC_CORE
> select ARCH_HAS_SCALED_CPUTIME if VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
> + select ARCH_HAS_SET_MEMORY
Below you do:
if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
return 0;
Which suggests we should instead just only select ARCH_HAS_SET_MEMORY if
STRICT_KERNEL_RWX ?
> diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
> index 3b4e9e4e25ea..d8a08abde1ae 100644
> --- a/arch/powerpc/mm/Makefile
> +++ b/arch/powerpc/mm/Makefile
> @@ -5,7 +5,7 @@
>
> ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC)
>
> -obj-y := fault.o mem.o pgtable.o mmap.o maccess.o \
> +obj-y := fault.o mem.o pgtable.o mmap.o maccess.o pageattr.o \
.. and then the file should only be built if ARCH_HAS_SET_MEMORY = y.
> init_$(BITS).o pgtable_$(BITS).o \
> pgtable-frag.o ioremap.o ioremap_$(BITS).o \
> init-common.o mmu_context.o drmem.o
> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
> new file mode 100644
> index 000000000000..9efcb01088da
> --- /dev/null
> +++ b/arch/powerpc/mm/pageattr.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * MMU-generic set_memory implementation for powerpc
> + *
> + * Copyright 2019, IBM Corporation.
Should be 2019-2021.
> + */
> +
> +#include <linux/mm.h>
> +#include <linux/set_memory.h>
> +
> +#include <asm/mmu.h>
> +#include <asm/page.h>
> +#include <asm/pgtable.h>
> +
> +
> +/*
> + * Updates the attributes of a page in three steps:
> + *
> + * 1. invalidate the page table entry
> + * 2. flush the TLB
> + * 3. install the new entry with the updated attributes
> + *
> + * This is unsafe if the caller is attempting to change the mapping of the
> + * page it is executing from, or if another CPU is concurrently using the
> + * page being altered.
Is the 2nd part of that statement true?
Or, I guess maybe it is true depending on what "unsafe" means.
AIUI it's unsafe to use this on the page you're executing from, and by
unsafe we mean the kernel will potentially crash because it will lose
the mapping for the currently executing text.
Using this on a page that another CPU is accessing could be safe, if eg.
the other CPU is reading from the page and we are just changing it from
RW->RO.
So I'm not sure they're the same type of "unsafe".
> + * TODO make the implementation resistant to this.
> + *
> + * NOTE: can be dangerous to call without STRICT_KERNEL_RWX
I don't think we need that anymore?
> + */
> +static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
> +{
> + long action = (long)data;
> + pte_t pte;
> +
> + spin_lock(&init_mm.page_table_lock);
> +
> + /* invalidate the PTE so it's safe to modify */
> + pte = ptep_get_and_clear(&init_mm, addr, ptep);
> + flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> +
> + /* modify the PTE bits as desired, then apply */
> + switch (action) {
> + case SET_MEMORY_RO:
> + pte = pte_wrprotect(pte);
> + break;
So set_memory_ro() removes write, but doesn't remove execute.
That doesn't match my mental model of what "set to ro" means, but I
guess I'm wrong because the other implementations seem to do something
similar.
> + case SET_MEMORY_RW:
> + pte = pte_mkwrite(pte);
I think we want to add pte_mkdirty() here also to avoid a fault when the
mapping is written to.
eg. pmd_mkwrite(pmd_mkdirty(pte));
> + break;
> + case SET_MEMORY_NX:
> + pte = pte_exprotect(pte);
> + break;
> + case SET_MEMORY_X:
> + pte = pte_mkexec(pte);
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + break;
> + }
> +
> + set_pte_at(&init_mm, addr, ptep, pte);
> + spin_unlock(&init_mm.page_table_lock);
> +
> + return 0;
> +}
> +
> +int change_memory_attr(unsigned long addr, int numpages, long action)
> +{
> + unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
> + unsigned long sz = numpages * PAGE_SIZE;
> +
> + if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
> + return 0;
> +
> + if (numpages <= 0)
> + return 0;
> +
This ↓ should have a comment explaining what it's doing:
> +#ifdef CONFIG_PPC_BOOK3S_64
> + if (WARN_ON_ONCE(!radix_enabled() &&
> + get_region_id(addr) == LINEAR_MAP_REGION_ID)) {
> + return -1;
> + }
> +#endif
Maybe:
if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) &&
WARN_ON_ONCE(!radix_enabled() && get_region_id(addr) == LINEAR_MAP_REGION_ID)) {
return -1;
}
But then Aneesh pointed out that we should also block VMEMMAP_REGION_ID.
It might be better to just check for the permitted regions.
if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled()) {
int region = get_region_id(addr);
if (WARN_ON_ONCE(region != VMALLOC_REGION_ID && region != IO_REGION_ID))
return -1;
}
> +
> + return apply_to_existing_page_range(&init_mm, start, sz,
> + change_page_attr, (void *)action);
> +}
cheers
^ permalink raw reply
* Re: [PATCH v10 06/10] powerpc/mm/ptdump: debugfs handler for W+X checks at runtime
From: Michael Ellerman @ 2021-03-31 11:24 UTC (permalink / raw)
To: Jordan Niethe, linuxppc-dev
Cc: ajd, Kees Cook, Jordan Niethe, cmr, npiggin, naveen.n.rao, dja
In-Reply-To: <20210330045132.722243-7-jniethe5@gmail.com>
Jordan Niethe <jniethe5@gmail.com> writes:
> From: Russell Currey <ruscur@russell.cc>
>
> Optionally run W+X checks when dumping pagetable information to
> debugfs' kernel_page_tables.
>
> To use:
> $ echo 1 > /sys/kernel/debug/check_wx_pages
> $ cat /sys/kernel/debug/kernel_page_tables
>
> and check the kernel log. Useful for testing strict module RWX.
>
> To disable W+X checks:
> $ echo 0 > /sys/kernel/debug/check_wx_pages
>
> Update the Kconfig entry to reflect this.
>
> Also fix a typo.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> [jpn: Change check_wx_pages to act as mode bit affecting
> kernel_page_tables instead of triggering action on its own]
> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
> ---
> v10: check_wx_pages now affects kernel_page_tables rather then triggers
> its own action.
Hmm. I liked the old version better :)
I think you changed it based on Christophe's comment:
Why not just perform the test everytime someone dumps kernel_page_tables ?
But I think he meant *always* do the check when someone dumps
kernel_page_tables, not have another file to enable checking and then
require someone to dump kernel_page_tables to do the actual check.
Still I like the previous version where you can do the checks
separately, without having to dump the page tables, because dumping can
sometimes take quite a while.
What would be even better is if ptdump_check_wx() returned an error when
wx pages were found, and that was plumbed out to the debugs file. That
way you can script around it.
cheers
^ permalink raw reply
* Re: [PATCH 16/18] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
From: Will Deacon @ 2021-03-31 11:49 UTC (permalink / raw)
To: Robin Murphy
Cc: freedreno, kvm, Joerg Roedel, linuxppc-dev, dri-devel, Li Yang,
iommu, netdev, linux-arm-msm, virtualization, David Woodhouse,
Christoph Hellwig, linux-arm-kernel, Lu Baolu
In-Reply-To: <578d6aa5-4239-f5d7-2e9f-686b18e52bba@arm.com>
On Tue, Mar 30, 2021 at 05:28:19PM +0100, Robin Murphy wrote:
> On 2021-03-30 14:58, Will Deacon wrote:
> > On Tue, Mar 30, 2021 at 02:19:38PM +0100, Robin Murphy wrote:
> > > On 2021-03-30 14:11, Will Deacon wrote:
> > > > On Tue, Mar 16, 2021 at 04:38:22PM +0100, Christoph Hellwig wrote:
> > > > > From: Robin Murphy <robin.murphy@arm.com>
> > > > >
> > > > > Instead make the global iommu_dma_strict paramete in iommu.c canonical by
> > > > > exporting helpers to get and set it and use those directly in the drivers.
> > > > >
> > > > > This make sure that the iommu.strict parameter also works for the AMD and
> > > > > Intel IOMMU drivers on x86. As those default to lazy flushing a new
> > > > > IOMMU_CMD_LINE_STRICT is used to turn the value into a tristate to
> > > > > represent the default if not overriden by an explicit parameter.
> > > > >
> > > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>.
> > > > > [ported on top of the other iommu_attr changes and added a few small
> > > > > missing bits]
> > > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > > ---
> > > > > drivers/iommu/amd/iommu.c | 23 +-------
> > > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 +---------------
> > > > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 -
> > > > > drivers/iommu/arm/arm-smmu/arm-smmu.c | 27 +--------
> > > > > drivers/iommu/dma-iommu.c | 9 +--
> > > > > drivers/iommu/intel/iommu.c | 64 ++++-----------------
> > > > > drivers/iommu/iommu.c | 27 ++++++---
> > > > > include/linux/iommu.h | 4 +-
> > > > > 8 files changed, 40 insertions(+), 165 deletions(-)
> > > >
> > > > I really like this cleanup, but I can't help wonder if it's going in the
> > > > wrong direction. With SoCs often having multiple IOMMU instances and a
> > > > distinction between "trusted" and "untrusted" devices, then having the
> > > > flush-queue enabled on a per-IOMMU or per-domain basis doesn't sound
> > > > unreasonable to me, but this change makes it a global property.
> > >
> > > The intent here was just to streamline the existing behaviour of stuffing a
> > > global property into a domain attribute then pulling it out again in the
> > > illusion that it was in any way per-domain. We're still checking
> > > dev_is_untrusted() before making an actual decision, and it's not like we
> > > can't add more factors at that point if we want to.
> >
> > Like I say, the cleanup is great. I'm just wondering whether there's a
> > better way to express the complicated logic to decide whether or not to use
> > the flush queue than what we end up with:
> >
> > if (!cookie->fq_domain && (!dev || !dev_is_untrusted(dev)) &&
> > domain->ops->flush_iotlb_all && !iommu_get_dma_strict())
> >
> > which is mixing up globals, device properties and domain properties. The
> > result is that the driver code ends up just using the global to determine
> > whether or not to pass IO_PGTABLE_QUIRK_NON_STRICT to the page-table code,
> > which is a departure from the current way of doing things.
>
> But previously, SMMU only ever saw the global policy piped through the
> domain attribute by iommu_group_alloc_default_domain(), so there's no
> functional change there.
For DMA domains sure, but I don't think that's the case for unmanaged
domains such as those used by VFIO.
> Obviously some of the above checks could be factored out into some kind of
> iommu_use_flush_queue() helper that IOMMU drivers can also call if they need
> to keep in sync. Or maybe we just allow iommu-dma to set
> IO_PGTABLE_QUIRK_NON_STRICT directly via iommu_set_pgtable_quirks() if we're
> treating that as a generic thing now.
I think a helper that takes a domain would be a good starting point.
Will
^ permalink raw reply
* Re: [PATCH v2 3/7] powerpc: convert config files to generic cmdline
From: Will Deacon @ 2021-03-31 11:52 UTC (permalink / raw)
To: Daniel Walker
Cc: Rob Herring, Daniel Gimpelevich, X86 ML, open list:MIPS,
linux-kernel@vger.kernel.org, Paul Mackerras, xe-linux-external,
Andrew Morton, linuxppc-dev
In-Reply-To: <20210330173521.GT109100@zorba>
On Tue, Mar 30, 2021 at 10:35:21AM -0700, Daniel Walker wrote:
> On Mon, Mar 29, 2021 at 11:07:51AM +0100, Will Deacon wrote:
> > On Thu, Mar 25, 2021 at 12:59:56PM -0700, Daniel Walker wrote:
> > > On Thu, Mar 25, 2021 at 01:03:55PM +0100, Christophe Leroy wrote:
> > > >
> > > > Ok, so you agree we don't need to provide two CMDLINE, one to be appended and one to be prepended.
> > > >
> > > > Let's only provide once CMDLINE as of today, and ask the user to select
> > > > whether he wants it appended or prepended or replacee. Then no need to
> > > > change all existing config to rename CONFIG_CMDLINE into either of the new
> > > > ones.
> > > >
> > > > That's the main difference between my series and Daniel's series. So I'll
> > > > finish taking Will's comment into account and we'll send out a v3 soon.
> > >
> > > It doesn't solve the needs of Cisco, I've stated many times your changes have
> > > little value. Please stop submitting them.
> >
> > FWIW, they're useful for arm64 and I will gladly review the updated series.
> >
> > I don't think asking people to stop submitting patches is ever the right
> > answer. Please don't do that.
>
> Why ? It's me nacking his series, is that not allowed anymore ?
If you're that way inclined then you can "nack" whatever you want, but
please allow the rest of us to continue reviewing the patches. You don't
have any basis on which to veto other people's contributions and so
demanding that somebody stops posting code is neither constructive nor
meaningful.
Will
^ permalink raw reply
* Re: [PATCH v10 01/10] powerpc/mm: Implement set_memory() routines
From: Christophe Leroy @ 2021-03-31 12:03 UTC (permalink / raw)
To: Michael Ellerman, Jordan Niethe, linuxppc-dev
Cc: ajd, npiggin, cmr, naveen.n.rao, dja
In-Reply-To: <87lfa37euw.fsf@mpe.ellerman.id.au>
Le 31/03/2021 à 13:16, Michael Ellerman a écrit :
> Hi Jordan,
>
> A few nits below ...
>
> Jordan Niethe <jniethe5@gmail.com> writes:
>> From: Russell Currey <ruscur@russell.cc>
>>
>> The set_memory_{ro/rw/nx/x}() functions are required for STRICT_MODULE_RWX,
>> and are generally useful primitives to have. This implementation is
>> designed to be completely generic across powerpc's many MMUs.
>>
>> It's possible that this could be optimised to be faster for specific
>> MMUs, but the focus is on having a generic and safe implementation for
>> now.
>>
>> This implementation does not handle cases where the caller is attempting
>> to change the mapping of the page it is executing from, or if another
>> CPU is concurrently using the page being altered. These cases likely
>> shouldn't happen, but a more complex implementation with MMU-specific code
>> could safely handle them, so that is left as a TODO for now.
>>
>> On hash the linear mapping is not kept in the linux pagetable, so this
>> will not change the protection if used on that range. Currently these
>> functions are not used on the linear map so just WARN for now.
>>
>> These functions do nothing if STRICT_KERNEL_RWX is not enabled.
>>
>> Reviewed-by: Daniel Axtens <dja@axtens.net>
>> Signed-off-by: Russell Currey <ruscur@russell.cc>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> [jpn: -rebase on next plus "powerpc/mm/64s: Allow STRICT_KERNEL_RWX again"
>> - WARN on hash linear map]
>> Signed-off-by: Jordan Niethe <jniethe5@gmail.com>
>> ---
>> v10: WARN if trying to change the hash linear map
>> ---
>
> This ↓ should have a comment explaining what it's doing:
>
>> +#ifdef CONFIG_PPC_BOOK3S_64
>> + if (WARN_ON_ONCE(!radix_enabled() &&
>> + get_region_id(addr) == LINEAR_MAP_REGION_ID)) {
>> + return -1;
>> + }
>> +#endif
>
> Maybe:
>
> if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) &&
> WARN_ON_ONCE(!radix_enabled() && get_region_id(addr) == LINEAR_MAP_REGION_ID)) {
> return -1;
> }
get_region_id() only exists for book3s/64 at the time being, and LINEAR_MAP_REGION_ID as well.
>
> But then Aneesh pointed out that we should also block VMEMMAP_REGION_ID.
>
> It might be better to just check for the permitted regions.
>
> if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled()) {
> int region = get_region_id(addr);
>
> if (WARN_ON_ONCE(region != VMALLOC_REGION_ID && region != IO_REGION_ID))
> return -1;
> }
>
>> +
>> + return apply_to_existing_page_range(&init_mm, start, sz,
>> + change_page_attr, (void *)action);
>> +}
>
>
> cheers
>
^ permalink raw reply
* [PATCH -next] powerpc/eeh: Add correct inline functions
From: YueHaibing @ 2021-03-31 12:53 UTC (permalink / raw)
To: ruscur, oohall, mpe, benh, paulus, sbobroff
Cc: YueHaibing, linuxppc-dev, linux-kernel
pseries_eeh_add_device_early()/pseries_eeh_add_device_tree_early() is
never used since adding, however pseries_eeh_init_edev() and
pseries_eeh_init_edev_recursive() need their inline versions.
Fixes: b6eebb093cad ("powerpc/eeh: Make early EEH init pseries specific")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
arch/powerpc/include/asm/eeh.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/include/asm/eeh.h b/arch/powerpc/include/asm/eeh.h
index b1a5bba2e0b9..0b6c2a6711d3 100644
--- a/arch/powerpc/include/asm/eeh.h
+++ b/arch/powerpc/include/asm/eeh.h
@@ -357,8 +357,8 @@ static inline int eeh_phb_pe_create(struct pci_controller *phb) { return 0; }
void pseries_eeh_init_edev(struct pci_dn *pdn);
void pseries_eeh_init_edev_recursive(struct pci_dn *pdn);
#else
-static inline void pseries_eeh_add_device_early(struct pci_dn *pdn) { }
-static inline void pseries_eeh_add_device_tree_early(struct pci_dn *pdn) { }
+static inline void pseries_eeh_init_edev(struct pci_dn *pdn) { }
+static inline void pseries_eeh_init_edev_recursive(struct pci_dn *pdn) { }
#endif
#ifdef CONFIG_PPC64
--
2.17.1
^ permalink raw reply related
* Re: [PATCH -next] powerpc/eeh: Remove unused inline function eeh_dev_phb_init_dynamic()
From: YueHaibing @ 2021-03-31 12:58 UTC (permalink / raw)
To: Daniel Axtens, ruscur, oohall, mpe, benh, paulus
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <874kgy8ptt.fsf@linkitivity.dja.id.au>
On 2021/3/26 13:08, Daniel Axtens wrote:
> Hi,
>
>> commit 475028efc708 ("powerpc/eeh: Remove eeh_dev_phb_init_dynamic()")
>> left behind this, so can remove it.
>
> I had a look: the inline that you are removing here is for the
> !CONFIG_EEH case, which explains why it was missed the first time.
>
> This looks like a good change. Out of interest, what tool are you using
> to find these unused inlines? If there are many more, it might make
> sense to combine future patches removing them into a single patch, but
> I'm not sure.
Just use some grep skill, will do that if any.
>
> checkpatch likes this patch, so that's also good :)
>
> Reviewed-by: Daniel Axtens <dja@axtens.net>
>
> Kind regards,
> Daniel
> .
>
^ permalink raw reply
* Re: [PATCH 16/18] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
From: Robin Murphy @ 2021-03-31 13:09 UTC (permalink / raw)
To: Will Deacon
Cc: kvm, linux-arm-msm, linuxppc-dev, dri-devel, Li Yang, iommu,
Christoph Hellwig, netdev, virtualization, freedreno,
David Woodhouse, linux-arm-kernel
In-Reply-To: <20210331114947.GA7626@willie-the-truck>
On 2021-03-31 12:49, Will Deacon wrote:
> On Tue, Mar 30, 2021 at 05:28:19PM +0100, Robin Murphy wrote:
>> On 2021-03-30 14:58, Will Deacon wrote:
>>> On Tue, Mar 30, 2021 at 02:19:38PM +0100, Robin Murphy wrote:
>>>> On 2021-03-30 14:11, Will Deacon wrote:
>>>>> On Tue, Mar 16, 2021 at 04:38:22PM +0100, Christoph Hellwig wrote:
>>>>>> From: Robin Murphy <robin.murphy@arm.com>
>>>>>>
>>>>>> Instead make the global iommu_dma_strict paramete in iommu.c canonical by
>>>>>> exporting helpers to get and set it and use those directly in the drivers.
>>>>>>
>>>>>> This make sure that the iommu.strict parameter also works for the AMD and
>>>>>> Intel IOMMU drivers on x86. As those default to lazy flushing a new
>>>>>> IOMMU_CMD_LINE_STRICT is used to turn the value into a tristate to
>>>>>> represent the default if not overriden by an explicit parameter.
>>>>>>
>>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>.
>>>>>> [ported on top of the other iommu_attr changes and added a few small
>>>>>> missing bits]
>>>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>>>>> ---
>>>>>> drivers/iommu/amd/iommu.c | 23 +-------
>>>>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 +---------------
>>>>>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h | 1 -
>>>>>> drivers/iommu/arm/arm-smmu/arm-smmu.c | 27 +--------
>>>>>> drivers/iommu/dma-iommu.c | 9 +--
>>>>>> drivers/iommu/intel/iommu.c | 64 ++++-----------------
>>>>>> drivers/iommu/iommu.c | 27 ++++++---
>>>>>> include/linux/iommu.h | 4 +-
>>>>>> 8 files changed, 40 insertions(+), 165 deletions(-)
>>>>>
>>>>> I really like this cleanup, but I can't help wonder if it's going in the
>>>>> wrong direction. With SoCs often having multiple IOMMU instances and a
>>>>> distinction between "trusted" and "untrusted" devices, then having the
>>>>> flush-queue enabled on a per-IOMMU or per-domain basis doesn't sound
>>>>> unreasonable to me, but this change makes it a global property.
>>>>
>>>> The intent here was just to streamline the existing behaviour of stuffing a
>>>> global property into a domain attribute then pulling it out again in the
>>>> illusion that it was in any way per-domain. We're still checking
>>>> dev_is_untrusted() before making an actual decision, and it's not like we
>>>> can't add more factors at that point if we want to.
>>>
>>> Like I say, the cleanup is great. I'm just wondering whether there's a
>>> better way to express the complicated logic to decide whether or not to use
>>> the flush queue than what we end up with:
>>>
>>> if (!cookie->fq_domain && (!dev || !dev_is_untrusted(dev)) &&
>>> domain->ops->flush_iotlb_all && !iommu_get_dma_strict())
>>>
>>> which is mixing up globals, device properties and domain properties. The
>>> result is that the driver code ends up just using the global to determine
>>> whether or not to pass IO_PGTABLE_QUIRK_NON_STRICT to the page-table code,
>>> which is a departure from the current way of doing things.
>>
>> But previously, SMMU only ever saw the global policy piped through the
>> domain attribute by iommu_group_alloc_default_domain(), so there's no
>> functional change there.
>
> For DMA domains sure, but I don't think that's the case for unmanaged
> domains such as those used by VFIO.
Eh? This is only relevant to DMA domains anyway. Flush queues are part
of the IOVA allocator that VFIO doesn't even use. It's always been the
case that unmanaged domains only use strict invalidation.
>> Obviously some of the above checks could be factored out into some kind of
>> iommu_use_flush_queue() helper that IOMMU drivers can also call if they need
>> to keep in sync. Or maybe we just allow iommu-dma to set
>> IO_PGTABLE_QUIRK_NON_STRICT directly via iommu_set_pgtable_quirks() if we're
>> treating that as a generic thing now.
>
> I think a helper that takes a domain would be a good starting point.
You mean device, right? The one condition we currently have is at the
device level, and there's really nothing inherent to the domain itself
that matters (since the type is implicitly IOMMU_DOMAIN_DMA to even care
about this).
Another idea that's just come to mind is now that IOMMU_DOMAIN_DMA has a
standard meaning, maybe we could split out a separate
IOMMU_DOMAIN_DMA_STRICT type such that it can all propagate from
iommu_get_def_domain_type()? That feels like it might be quite
promising, but I'd still do it as an improvement on top of this patch,
since it's beyond just cleaning up the abuse of domain attributes to
pass a command-line option around.
Robin.
^ permalink raw reply
* Re: [PATCH] powerpc/signal32: Fix Oops on sigreturn with unmapped VDSO
From: Michael Ellerman @ 2021-03-31 13:23 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <79ee6fe13b267b0c81d7626c471a99140f0c9e4a.1616939989.git.christophe.leroy@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> PPC32 encounters a KUAP fault when trying to handle a signal with
> VDSO unmapped.
>
> Kernel attempted to read user page (7fc07ec0) - exploit attempt? (uid: 0)
> BUG: Unable to handle kernel data access on read at 0x7fc07ec0
> Faulting instruction address: 0xc00111d4
> Oops: Kernel access of bad area, sig: 11 [#1]
> BE PAGE_SIZE=16K PREEMPT CMPC885
> CPU: 0 PID: 353 Comm: sigreturn_vdso Not tainted 5.12.0-rc4-s3k-dev-01553-gb30c310ea220 #4814
> NIP: c00111d4 LR: c0005a28 CTR: 00000000
> REGS: cadb3dd0 TRAP: 0300 Not tainted (5.12.0-rc4-s3k-dev-01553-gb30c310ea220)
> MSR: 00009032 <EE,ME,IR,DR,RI> CR: 48000884 XER: 20000000
> DAR: 7fc07ec0 DSISR: 88000000
> GPR00: c0007788 cadb3e90 c28d4a40 7fc07ec0 7fc07ed0 000004e0 7fc07ce0 00000000
> GPR08: 00000001 00000001 7fc07ec0 00000000 28000282 1001b828 100a0920 00000000
> GPR16: 100cac0c 100b0000 105c43a4 105c5685 100d0000 100d0000 100d0000 100b2e9e
> GPR24: ffffffff 105c43c8 00000000 7fc07ec8 cadb3f40 cadb3ec8 c28d4a40 00000000
> NIP [c00111d4] flush_icache_range+0x90/0xb4
> LR [c0005a28] handle_signal32+0x1bc/0x1c4
> Call Trace:
> [cadb3e90] [100d0000] 0x100d0000 (unreliable)
> [cadb3ec0] [c0007788] do_notify_resume+0x260/0x314
> [cadb3f20] [c000c764] syscall_exit_prepare+0x120/0x184
> [cadb3f30] [c00100b4] ret_from_syscall+0xc/0x28
> --- interrupt: c00 at 0xfe807f8
> NIP: 0fe807f8 LR: 10001060 CTR: c0139378
> REGS: cadb3f40 TRAP: 0c00 Not tainted (5.12.0-rc4-s3k-dev-01553-gb30c310ea220)
> MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 28000482 XER: 20000000
>
> GPR00: 00000025 7fc081c0 77bb1690 00000000 0000000a 28000482 00000001 0ff03a38
> GPR08: 0000d032 00006de5 c28d4a40 00000009 88000482 1001b828 100a0920 00000000
> GPR16: 100cac0c 100b0000 105c43a4 105c5685 100d0000 100d0000 100d0000 100b2e9e
> GPR24: ffffffff 105c43c8 00000000 77ba7628 10002398 10010000 10002124 00024000
> NIP [0fe807f8] 0xfe807f8
> LR [10001060] 0x10001060
> --- interrupt: c00
> Instruction dump:
> 38630010 7c001fac 38630010 4200fff0 7c0004ac 4c00012c 4e800020 7c001fac
> 2c0a0000 38630010 4082ffcc 4bffffe4 <7c00186c> 2c070000 39430010 4082ff8c
> ---[ end trace 3973fb72b049cb06 ]---
>
> This is because flush_icache_range() is called on user addresses.
>
> The same problem was detected some time ago on PPC64. It was fixed by
> enabling KUAP in commit 59bee45b9712 ("powerpc/mm: Fix missing KUAP
> disable in flush_coherent_icache()").
>
> PPC32 doesn't use flush_coherent_icache() and fallbacks on
> clean_dcache_range() and invalidate_icache_range().
But this code is also used for compat tasks on 64-bit.
> We could fix it similarly by enabling user access in those functions,
> but this is overkill for just flushing two instructions.
>
> The two instructions are 8 bytes aligned, so a single dcbst/icbi is
> enough to flush them. Do like __patch_instruction() and inline
> a dcbst followed by an icbi just after the write of the instructions,
> while user access is still allowed. The isync is not required because
> rfi will be used to return to user.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/signal_32.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 75ee918a120a..5b2ba2731957 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -809,6 +809,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
> unsafe_put_user(PPC_INST_ADDI + __NR_rt_sigreturn, &mctx->mc_pad[0],
> failed);
> unsafe_put_user(PPC_INST_SC, &mctx->mc_pad[1], failed);
> + asm("dcbst %y0; sync; icbi %y0; sync" :: "Z" (mctx->mc_pad[0]));
If I'm reading that right you're pointing the icbi at the user address.
That's going to cause a KUAP fault just like we fixed in commit
59bee45b9712 ("powerpc/mm: Fix missing KUAP disable in flush_coherent_icache()").
We have user write access enabled, but the icbi is treated as a load.
So I don't think that's going to work.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: Load modules closer to kernel text
From: Michael Ellerman @ 2021-03-31 13:39 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <db20abbecb129c49d3c663de67dac6a87501ac0b.1617001615.git.christophe.leroy@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> On the 8xx, TASK_SIZE is 0x80000000. The space between TASK_SIZE and
> PAGE_OFFSET is not used.
>
> Use it to load modules in order to minimise the distance between
> kernel text and modules and avoid trampolines in modules to access
> kernel functions or other module functions.
>
> Define a 16Mbytes area for modules, that's more than enough.
16MB seems kind of small.
At least on 64-bit we could potentially have hundreds of MBs of modules.
cheers
^ permalink raw reply
* [PATCH] selftests: timens: Fix gettime_perf to work on powerpc
From: Christophe Leroy @ 2021-03-31 13:59 UTC (permalink / raw)
To: Shuah Khan, Andrei Vagin, Dmitry Safonov
Cc: linuxppc-dev, linux-kernel, linux-kselftest
On powerpc:
- VDSO library is named linux-vdso32.so.1 or linux-vdso64.so.1
- clock_gettime is named __kernel_clock_gettime()
Ensure gettime_perf tries these names before giving up.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
tools/testing/selftests/timens/gettime_perf.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/tools/testing/selftests/timens/gettime_perf.c b/tools/testing/selftests/timens/gettime_perf.c
index 7bf841a3967b..6b13dc277724 100644
--- a/tools/testing/selftests/timens/gettime_perf.c
+++ b/tools/testing/selftests/timens/gettime_perf.c
@@ -25,12 +25,20 @@ static void fill_function_pointers(void)
if (!vdso)
vdso = dlopen("linux-gate.so.1",
RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD);
+ if (!vdso)
+ vdso = dlopen("linux-vdso32.so.1",
+ RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD);
+ if (!vdso)
+ vdso = dlopen("linux-vdso64.so.1",
+ RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD);
if (!vdso) {
pr_err("[WARN]\tfailed to find vDSO\n");
return;
}
vdso_clock_gettime = (vgettime_t)dlsym(vdso, "__vdso_clock_gettime");
+ if (!vdso_clock_gettime)
+ vdso_clock_gettime = (vgettime_t)dlsym(vdso, "__kernel_clock_gettime");
if (!vdso_clock_gettime)
pr_err("Warning: failed to find clock_gettime in vDSO\n");
--
2.25.0
^ permalink raw reply related
* [PATCH v2] powerpc/signal32: Fix Oops on sigreturn with unmapped VDSO
From: Christophe Leroy @ 2021-03-31 14:07 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
PPC32 encounters a KUAP fault when trying to handle a signal with
VDSO unmapped.
Kernel attempted to read user page (7fc07ec0) - exploit attempt? (uid: 0)
BUG: Unable to handle kernel data access on read at 0x7fc07ec0
Faulting instruction address: 0xc00111d4
Oops: Kernel access of bad area, sig: 11 [#1]
BE PAGE_SIZE=16K PREEMPT CMPC885
CPU: 0 PID: 353 Comm: sigreturn_vdso Not tainted 5.12.0-rc4-s3k-dev-01553-gb30c310ea220 #4814
NIP: c00111d4 LR: c0005a28 CTR: 00000000
REGS: cadb3dd0 TRAP: 0300 Not tainted (5.12.0-rc4-s3k-dev-01553-gb30c310ea220)
MSR: 00009032 <EE,ME,IR,DR,RI> CR: 48000884 XER: 20000000
DAR: 7fc07ec0 DSISR: 88000000
GPR00: c0007788 cadb3e90 c28d4a40 7fc07ec0 7fc07ed0 000004e0 7fc07ce0 00000000
GPR08: 00000001 00000001 7fc07ec0 00000000 28000282 1001b828 100a0920 00000000
GPR16: 100cac0c 100b0000 105c43a4 105c5685 100d0000 100d0000 100d0000 100b2e9e
GPR24: ffffffff 105c43c8 00000000 7fc07ec8 cadb3f40 cadb3ec8 c28d4a40 00000000
NIP [c00111d4] flush_icache_range+0x90/0xb4
LR [c0005a28] handle_signal32+0x1bc/0x1c4
Call Trace:
[cadb3e90] [100d0000] 0x100d0000 (unreliable)
[cadb3ec0] [c0007788] do_notify_resume+0x260/0x314
[cadb3f20] [c000c764] syscall_exit_prepare+0x120/0x184
[cadb3f30] [c00100b4] ret_from_syscall+0xc/0x28
--- interrupt: c00 at 0xfe807f8
NIP: 0fe807f8 LR: 10001060 CTR: c0139378
REGS: cadb3f40 TRAP: 0c00 Not tainted (5.12.0-rc4-s3k-dev-01553-gb30c310ea220)
MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 28000482 XER: 20000000
GPR00: 00000025 7fc081c0 77bb1690 00000000 0000000a 28000482 00000001 0ff03a38
GPR08: 0000d032 00006de5 c28d4a40 00000009 88000482 1001b828 100a0920 00000000
GPR16: 100cac0c 100b0000 105c43a4 105c5685 100d0000 100d0000 100d0000 100b2e9e
GPR24: ffffffff 105c43c8 00000000 77ba7628 10002398 10010000 10002124 00024000
NIP [0fe807f8] 0xfe807f8
LR [10001060] 0x10001060
--- interrupt: c00
Instruction dump:
38630010 7c001fac 38630010 4200fff0 7c0004ac 4c00012c 4e800020 7c001fac
2c0a0000 38630010 4082ffcc 4bffffe4 <7c00186c> 2c070000 39430010 4082ff8c
---[ end trace 3973fb72b049cb06 ]---
This is because flush_icache_range() is called on user addresses.
The same problem was detected some time ago on PPC64. It was fixed by
enabling KUAP in commit 59bee45b9712 ("powerpc/mm: Fix missing KUAP
disable in flush_coherent_icache()").
PPC32 doesn't use flush_coherent_icache() and fallbacks on
clean_dcache_range() and invalidate_icache_range().
We could fix it similarly by enabling user access in those functions,
but this is overkill for just flushing two instructions.
The two instructions are 8 bytes aligned, so a single dcbst/icbi is
enough to flush them. Do like __patch_instruction() and inline
a dcbst followed by an icbi just after the write of the instructions,
while user access is still allowed. The isync is not required because
rfi will be used to return to user.
icbi() is handled as a read so read-write user access is needed.
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: Do read-write user access.
---
arch/powerpc/kernel/signal_32.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index c505b444a613..09884af693aa 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -775,7 +775,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
else
prepare_save_user_regs(1);
- if (!user_write_access_begin(frame, sizeof(*frame)))
+ if (!user_access_begin(frame, sizeof(*frame)))
goto badframe;
/* Put the siginfo & fill in most of the ucontext */
@@ -809,17 +809,15 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
unsafe_put_user(PPC_INST_ADDI + __NR_rt_sigreturn, &mctx->mc_pad[0],
failed);
unsafe_put_user(PPC_INST_SC, &mctx->mc_pad[1], failed);
+ asm("dcbst %y0; sync; icbi %y0; sync" :: "Z" (mctx->mc_pad[0]));
}
unsafe_put_sigset_t(&frame->uc.uc_sigmask, oldset, failed);
- user_write_access_end();
+ user_access_end();
if (copy_siginfo_to_user(&frame->info, &ksig->info))
goto badframe;
- if (tramp == (unsigned long)mctx->mc_pad)
- flush_icache_range(tramp, tramp + 2 * sizeof(unsigned long));
-
regs->link = tramp;
#ifdef CONFIG_PPC_FPU_REGS
@@ -844,7 +842,7 @@ int handle_rt_signal32(struct ksignal *ksig, sigset_t *oldset,
return 0;
failed:
- user_write_access_end();
+ user_access_end();
badframe:
signal_fault(tsk, regs, "handle_rt_signal32", frame);
@@ -879,7 +877,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
else
prepare_save_user_regs(1);
- if (!user_write_access_begin(frame, sizeof(*frame)))
+ if (!user_access_begin(frame, sizeof(*frame)))
goto badframe;
sc = (struct sigcontext __user *) &frame->sctx;
@@ -908,11 +906,9 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
/* Set up the sigreturn trampoline: li r0,sigret; sc */
unsafe_put_user(PPC_INST_ADDI + __NR_sigreturn, &mctx->mc_pad[0], failed);
unsafe_put_user(PPC_INST_SC, &mctx->mc_pad[1], failed);
+ asm("dcbst %y0; sync; icbi %y0; sync" :: "Z" (mctx->mc_pad[0]));
}
- user_write_access_end();
-
- if (tramp == (unsigned long)mctx->mc_pad)
- flush_icache_range(tramp, tramp + 2 * sizeof(unsigned long));
+ user_access_end();
regs->link = tramp;
@@ -935,7 +931,7 @@ int handle_signal32(struct ksignal *ksig, sigset_t *oldset,
return 0;
failed:
- user_write_access_end();
+ user_access_end();
badframe:
signal_fault(tsk, regs, "handle_signal32", frame);
--
2.25.0
^ permalink raw reply related
* Re: [PATCH] powerpc/8xx: Load modules closer to kernel text
From: Christophe Leroy @ 2021-03-31 14:12 UTC (permalink / raw)
To: Michael Ellerman, Benjamin Herrenschmidt, Paul Mackerras
Cc: Jordan Niethe, linuxppc-dev, linux-kernel
In-Reply-To: <87czvf788t.fsf@mpe.ellerman.id.au>
Le 31/03/2021 à 15:39, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> On the 8xx, TASK_SIZE is 0x80000000. The space between TASK_SIZE and
>> PAGE_OFFSET is not used.
>>
>> Use it to load modules in order to minimise the distance between
>> kernel text and modules and avoid trampolines in modules to access
>> kernel functions or other module functions.
>>
>> Define a 16Mbytes area for modules, that's more than enough.
>
> 16MB seems kind of small.
>
> At least on 64-bit we could potentially have hundreds of MBs of modules.
>
Well, with a 16 MB kernel and 16 MB modules, my board is full :)
Even on the more recent board that has 128 MB, I don't expect more than a few MBs of modules in
addition to the kernel which is approx 8M.
But ok, I'll do something more generic, though it will conflict with Jordan's series.
Christophe
^ permalink raw reply
* [PATCH] powerpc/ptrace: Don't return error when getting/setting FP regs without CONFIG_PPC_FPU_REGS
From: Christophe Leroy @ 2021-03-31 14:14 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman
Cc: linuxppc-dev, linux-kernel
An #ifdef CONFIG_PPC_FPU_REGS is missing in arch_ptrace() leading
to the following Oops because [REGSET_FPR] entry is not initialised in
native_regsets[].
[ 41.917608] BUG: Unable to handle kernel instruction fetch
[ 41.922849] Faulting instruction address: 0xff8fd228
[ 41.927760] Oops: Kernel access of bad area, sig: 11 [#1]
[ 41.933089] BE PAGE_SIZE=4K PREEMPT CMPC885
[ 41.940753] Modules linked in:
[ 41.943768] CPU: 0 PID: 366 Comm: gdb Not tainted 5.12.0-rc5-s3k-dev-01666-g7aac86a0f057-dirty #4835
[ 41.952800] NIP: ff8fd228 LR: c004d9e0 CTR: ff8fd228
[ 41.957790] REGS: caae9df0 TRAP: 0400 Not tainted (5.12.0-rc5-s3k-dev-01666-g7aac86a0f057-dirty)
[ 41.966741] MSR: 40009032 <EE,ME,IR,DR,RI> CR: 82004248 XER: 20000000
[ 41.973540]
[ 41.973540] GPR00: c004d9b4 caae9eb0 c1b64f60 c1b64520 c0713cd4 caae9eb8 c1bacdfc 00000004
[ 41.973540] GPR08: 00000200 ff8fd228 c1bac700 00001032 28004242 1061aaf4 00000001 106d64a0
[ 41.973540] GPR16: 00000000 00000000 7fa0a774 10610000 7fa0aef9 00000000 10610000 7fa0a538
[ 41.973540] GPR24: 7fa0a580 7fa0a570 c1bacc00 c1b64520 c1bacc00 caae9ee8 00000108 c0713cd4
[ 42.009685] NIP [ff8fd228] 0xff8fd228
[ 42.013300] LR [c004d9e0] __regset_get+0x100/0x124
[ 42.018036] Call Trace:
[ 42.020443] [caae9eb0] [c004d9b4] __regset_get+0xd4/0x124 (unreliable)
[ 42.026899] [caae9ee0] [c004da94] copy_regset_to_user+0x5c/0xb0
[ 42.032751] [caae9f10] [c002f640] sys_ptrace+0xe4/0x588
[ 42.037915] [caae9f30] [c0011010] ret_from_syscall+0x0/0x28
[ 42.043422] --- interrupt: c00 at 0xfd1f8e4
[ 42.047553] NIP: 0fd1f8e4 LR: 1004a688 CTR: 00000000
[ 42.052544] REGS: caae9f40 TRAP: 0c00 Not tainted (5.12.0-rc5-s3k-dev-01666-g7aac86a0f057-dirty)
[ 42.061494] MSR: 0000d032 <EE,PR,ME,IR,DR,RI> CR: 48004442 XER: 00000000
[ 42.068551]
[ 42.068551] GPR00: 0000001a 7fa0a040 77dad7e0 0000000e 00000170 00000000 7fa0a078 00000004
[ 42.068551] GPR08: 00000000 108deb88 108dda40 106d6010 44004442 1061aaf4 00000001 106d64a0
[ 42.068551] GPR16: 00000000 00000000 7fa0a774 10610000 7fa0aef9 00000000 10610000 7fa0a538
[ 42.068551] GPR24: 7fa0a580 7fa0a570 1078fe00 1078fd70 1078fd70 00000170 0fdd3244 0000000d
[ 42.104696] NIP [0fd1f8e4] 0xfd1f8e4
[ 42.108225] LR [1004a688] 0x1004a688
[ 42.111753] --- interrupt: c00
[ 42.114768] Instruction dump:
[ 42.117698] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[ 42.125443] XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX
[ 42.133195] ---[ end trace d35616f22ab2100c ]---
Adding the missing #ifdef is not good because gdb doesn't like getting
an error when getting registers.
Instead, make ptrace return 0s when CONFIG_PPC_FPU_REGS is not set.
Fixes: b6254ced4da6 ("powerpc/signal: Don't manage floating point regs when no FPU")
Cc: stable@vger.kernel.org
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/kernel/ptrace/Makefile | 4 ++--
arch/powerpc/kernel/ptrace/ptrace-decl.h | 14 --------------
arch/powerpc/kernel/ptrace/ptrace-fpu.c | 10 ++++++++++
arch/powerpc/kernel/ptrace/ptrace-novsx.c | 8 ++++++++
arch/powerpc/kernel/ptrace/ptrace-view.c | 2 --
5 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/kernel/ptrace/Makefile b/arch/powerpc/kernel/ptrace/Makefile
index 8ebc11d1168d..77abd1a5a508 100644
--- a/arch/powerpc/kernel/ptrace/Makefile
+++ b/arch/powerpc/kernel/ptrace/Makefile
@@ -6,11 +6,11 @@
CFLAGS_ptrace-view.o += -DUTS_MACHINE='"$(UTS_MACHINE)"'
obj-y += ptrace.o ptrace-view.o
-obj-$(CONFIG_PPC_FPU_REGS) += ptrace-fpu.o
+obj-y += ptrace-fpu.o
obj-$(CONFIG_COMPAT) += ptrace32.o
obj-$(CONFIG_VSX) += ptrace-vsx.o
ifneq ($(CONFIG_VSX),y)
-obj-$(CONFIG_PPC_FPU_REGS) += ptrace-novsx.o
+obj-y += ptrace-novsx.o
endif
obj-$(CONFIG_ALTIVEC) += ptrace-altivec.o
obj-$(CONFIG_SPE) += ptrace-spe.o
diff --git a/arch/powerpc/kernel/ptrace/ptrace-decl.h b/arch/powerpc/kernel/ptrace/ptrace-decl.h
index 3487f2c9735c..eafe5f0f6289 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-decl.h
+++ b/arch/powerpc/kernel/ptrace/ptrace-decl.h
@@ -165,22 +165,8 @@ int ptrace_put_reg(struct task_struct *task, int regno, unsigned long data);
extern const struct user_regset_view user_ppc_native_view;
/* ptrace-fpu */
-#ifdef CONFIG_PPC_FPU_REGS
int ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data);
int ptrace_put_fpr(struct task_struct *child, int index, unsigned long data);
-#else
-static inline int
-ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data)
-{
- return -EIO;
-}
-
-static inline int
-ptrace_put_fpr(struct task_struct *child, int index, unsigned long data)
-{
- return -EIO;
-}
-#endif
/* ptrace-(no)adv */
void ppc_gethwdinfo(struct ppc_debug_info *dbginfo);
diff --git a/arch/powerpc/kernel/ptrace/ptrace-fpu.c b/arch/powerpc/kernel/ptrace/ptrace-fpu.c
index 8301cb52dd99..5dca19361316 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-fpu.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-fpu.c
@@ -8,32 +8,42 @@
int ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data)
{
+#ifdef CONFIG_PPC_FPU_REGS
unsigned int fpidx = index - PT_FPR0;
+#endif
if (index > PT_FPSCR)
return -EIO;
+#ifdef CONFIG_PPC_FPU_REGS
flush_fp_to_thread(child);
if (fpidx < (PT_FPSCR - PT_FPR0))
memcpy(data, &child->thread.TS_FPR(fpidx), sizeof(long));
else
*data = child->thread.fp_state.fpscr;
+#else
+ *data = 0;
+#endif
return 0;
}
int ptrace_put_fpr(struct task_struct *child, int index, unsigned long data)
{
+#ifdef CONFIG_PPC_FPU_REGS
unsigned int fpidx = index - PT_FPR0;
+#endif
if (index > PT_FPSCR)
return -EIO;
+#ifdef CONFIG_PPC_FPU_REGS
flush_fp_to_thread(child);
if (fpidx < (PT_FPSCR - PT_FPR0))
memcpy(&child->thread.TS_FPR(fpidx), &data, sizeof(long));
else
child->thread.fp_state.fpscr = data;
+#endif
return 0;
}
diff --git a/arch/powerpc/kernel/ptrace/ptrace-novsx.c b/arch/powerpc/kernel/ptrace/ptrace-novsx.c
index b3b36835658a..7433f3db979a 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-novsx.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-novsx.c
@@ -21,12 +21,16 @@
int fpr_get(struct task_struct *target, const struct user_regset *regset,
struct membuf to)
{
+#ifdef CONFIG_PPC_FPU_REGS
BUILD_BUG_ON(offsetof(struct thread_fp_state, fpscr) !=
offsetof(struct thread_fp_state, fpr[32]));
flush_fp_to_thread(target);
return membuf_write(&to, &target->thread.fp_state, 33 * sizeof(u64));
+#else
+ return membuf_write(&to, &empty_zero_page, 33 * sizeof(u64));
+#endif
}
/*
@@ -46,6 +50,7 @@ int fpr_set(struct task_struct *target, const struct user_regset *regset,
unsigned int pos, unsigned int count,
const void *kbuf, const void __user *ubuf)
{
+#ifdef CONFIG_PPC_FPU_REGS
BUILD_BUG_ON(offsetof(struct thread_fp_state, fpscr) !=
offsetof(struct thread_fp_state, fpr[32]));
@@ -53,4 +58,7 @@ int fpr_set(struct task_struct *target, const struct user_regset *regset,
return user_regset_copyin(&pos, &count, &kbuf, &ubuf,
&target->thread.fp_state, 0, -1);
+#else
+ return 0;
+#endif
}
diff --git a/arch/powerpc/kernel/ptrace/ptrace-view.c b/arch/powerpc/kernel/ptrace/ptrace-view.c
index 0923c94f684e..3579217ca9b0 100644
--- a/arch/powerpc/kernel/ptrace/ptrace-view.c
+++ b/arch/powerpc/kernel/ptrace/ptrace-view.c
@@ -522,13 +522,11 @@ static const struct user_regset native_regsets[] = {
.size = sizeof(long), .align = sizeof(long),
.regset_get = gpr_get, .set = gpr_set
},
-#ifdef CONFIG_PPC_FPU_REGS
[REGSET_FPR] = {
.core_note_type = NT_PRFPREG, .n = ELF_NFPREG,
.size = sizeof(double), .align = sizeof(double),
.regset_get = fpr_get, .set = fpr_set
},
-#endif
#ifdef CONFIG_ALTIVEC
[REGSET_VMX] = {
.core_note_type = NT_PPC_VMX, .n = 34,
--
2.25.0
^ permalink raw reply related
* [PATCH v6 0/9] riscv: Add qspinlock/qrwlock
From: guoren @ 2021-03-31 14:30 UTC (permalink / raw)
To: guoren
Cc: linux-arch, linux-xtensa, Guo Ren, linux-kernel, linux-csky,
openrisc, sparclinux, linux-riscv, linuxppc-dev
From: Guo Ren <guoren@linux.alibaba.com>
Current riscv is still using baby spinlock implementation. It'll cause
fairness and cache line bouncing problems. Many people are involved
and pay the efforts to improve it:
- The first version of patch was made in 2019.1:
https://lore.kernel.org/linux-riscv/20190211043829.30096-1-michaeljclark@mac.com/#r
- The second version was made in 2020.11:
https://lore.kernel.org/linux-riscv/1606225437-22948-2-git-send-email-guoren@kernel.org/
- A good discussion at Platform HSC.2021-03-08:
https://drive.google.com/drive/folders/1ooqdnIsYx7XKor5O1XTtM6D1CHp4hc0p
- A good discussion on V4 in mailling list:
https://lore.kernel.org/linux-riscv/1616868399-82848-1-git-send-email-guoren@kernel.org/T/#t
- Openrisc's maintainer want to implement arch_cmpxchg infrastructure.
https://lore.kernel.org/linux-riscv/1616868399-82848-1-git-send-email-guoren@kernel.org/T/#m11b712fb6a4fda043811b1f4c3d61446951ed65a
Hope your comments and Tested-by or Co-developed-by or Reviewed-by ...
Let's kick the qspinlock into riscv right now (Also for the
architecture which hasn't xchg16 atomic instruction.)
Change V6:
- Add ticket-lock for riscv, default is qspinlock
- Keep ticket-lock for csky, default is ticketlock
- Using smp_cond_load for riscv ticket-lock
- Optimize csky ticketlock with smp_cond_load, store_release
- Add PPC_LBARX_LWARX for powerpc
Change V5:
- Fixup #endif comment typo by Waiman
- Remove cmpxchg coding convention patches which will get into a
separate patchset later by Arnd's advice
- Try to involve more architectures in the discussion
Change V4:
- Remove custom sub-word xchg implementation
- Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32 in locking/qspinlock
Change V3:
- Coding convention by Peter Zijlstra's advices
Change V2:
- Coding convention in cmpxchg.h
- Re-implement short xchg
- Remove char & cmpxchg implementations
Guo Ren (8):
locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
riscv: locks: Introduce ticket-based spinlock implementation
csky: locks: Optimize coding convention
csky: Convert custom spinlock/rwlock to generic qspinlock/qrwlock
openrisc: qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
sparc: qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
xtensa: qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
powerpc/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
Michael Clark (1):
riscv: Convert custom spinlock/rwlock to generic qspinlock/qrwlock
arch/csky/Kconfig | 8 ++
arch/csky/include/asm/Kbuild | 2 +
arch/csky/include/asm/spinlock.h | 15 +--
arch/csky/include/asm/spinlock_types.h | 4 +
arch/openrisc/Kconfig | 1 +
arch/powerpc/Kconfig | 1 +
arch/riscv/Kconfig | 8 ++
arch/riscv/include/asm/Kbuild | 3 +
arch/riscv/include/asm/spinlock.h | 158 +++++++++---------------
arch/riscv/include/asm/spinlock_types.h | 26 ++--
arch/sparc/Kconfig | 1 +
arch/xtensa/Kconfig | 1 +
kernel/Kconfig.locks | 3 +
kernel/locking/qspinlock.c | 46 +++----
14 files changed, 142 insertions(+), 135 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH v6 1/9] locking/qspinlock: Add ARCH_USE_QUEUED_SPINLOCKS_XCHG32
From: guoren @ 2021-03-31 14:30 UTC (permalink / raw)
To: guoren
Cc: linux-arch, linux-xtensa, Guo Ren, Arnd Bergmann, Peter Zijlstra,
Will Deacon, linux-kernel, linux-csky, openrisc, Anup Patel,
sparclinux, Waiman Long, linux-riscv, linuxppc-dev, Ingo Molnar
In-Reply-To: <1617201040-83905-1-git-send-email-guoren@kernel.org>
From: Guo Ren <guoren@linux.alibaba.com>
Some architectures don't have sub-word swap atomic instruction,
they only have the full word's one.
The sub-word swap only improve the performance when:
NR_CPUS < 16K
* 0- 7: locked byte
* 8: pending
* 9-15: not used
* 16-17: tail index
* 18-31: tail cpu (+1)
The 9-15 bits are wasted to use xchg16 in xchg_tail.
Please let architecture select xchg16/xchg32 to implement
xchg_tail.
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Waiman Long <longman@redhat.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Anup Patel <anup@brainfault.org>
---
kernel/Kconfig.locks | 3 +++
kernel/locking/qspinlock.c | 46 +++++++++++++++++++++-----------------
2 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/kernel/Kconfig.locks b/kernel/Kconfig.locks
index 3de8fd11873b..d02f1261f73f 100644
--- a/kernel/Kconfig.locks
+++ b/kernel/Kconfig.locks
@@ -239,6 +239,9 @@ config LOCK_SPIN_ON_OWNER
config ARCH_USE_QUEUED_SPINLOCKS
bool
+config ARCH_USE_QUEUED_SPINLOCKS_XCHG32
+ bool
+
config QUEUED_SPINLOCKS
def_bool y if ARCH_USE_QUEUED_SPINLOCKS
depends on SMP
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index cbff6ba53d56..4bfaa969bd15 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -163,26 +163,6 @@ static __always_inline void clear_pending_set_locked(struct qspinlock *lock)
WRITE_ONCE(lock->locked_pending, _Q_LOCKED_VAL);
}
-/*
- * xchg_tail - Put in the new queue tail code word & retrieve previous one
- * @lock : Pointer to queued spinlock structure
- * @tail : The new queue tail code word
- * Return: The previous queue tail code word
- *
- * xchg(lock, tail), which heads an address dependency
- *
- * p,*,* -> n,*,* ; prev = xchg(lock, node)
- */
-static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
-{
- /*
- * We can use relaxed semantics since the caller ensures that the
- * MCS node is properly initialized before updating the tail.
- */
- return (u32)xchg_relaxed(&lock->tail,
- tail >> _Q_TAIL_OFFSET) << _Q_TAIL_OFFSET;
-}
-
#else /* _Q_PENDING_BITS == 8 */
/**
@@ -206,6 +186,30 @@ static __always_inline void clear_pending_set_locked(struct qspinlock *lock)
{
atomic_add(-_Q_PENDING_VAL + _Q_LOCKED_VAL, &lock->val);
}
+#endif /* _Q_PENDING_BITS == 8 */
+
+#if _Q_PENDING_BITS == 8 && !defined(CONFIG_ARCH_USE_QUEUED_SPINLOCKS_XCHG32)
+/*
+ * xchg_tail - Put in the new queue tail code word & retrieve previous one
+ * @lock : Pointer to queued spinlock structure
+ * @tail : The new queue tail code word
+ * Return: The previous queue tail code word
+ *
+ * xchg(lock, tail), which heads an address dependency
+ *
+ * p,*,* -> n,*,* ; prev = xchg(lock, node)
+ */
+static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
+{
+ /*
+ * We can use relaxed semantics since the caller ensures that the
+ * MCS node is properly initialized before updating the tail.
+ */
+ return (u32)xchg_relaxed(&lock->tail,
+ tail >> _Q_TAIL_OFFSET) << _Q_TAIL_OFFSET;
+}
+
+#else
/**
* xchg_tail - Put in the new queue tail code word & retrieve previous one
@@ -236,7 +240,7 @@ static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
}
return old;
}
-#endif /* _Q_PENDING_BITS == 8 */
+#endif
/**
* queued_fetch_set_pending_acquire - fetch the whole lock value and set pending
--
2.17.1
^ permalink raw reply related
* [PATCH v6 2/9] riscv: Convert custom spinlock/rwlock to generic qspinlock/qrwlock
From: guoren @ 2021-03-31 14:30 UTC (permalink / raw)
To: guoren
Cc: linux-arch, linux-xtensa, Guo Ren, Arnd Bergmann, Peter Zijlstra,
Anup Patel, Palmer Dabbelt, linux-kernel, linux-csky,
Michael Clark, openrisc, sparclinux, linux-riscv, linuxppc-dev
In-Reply-To: <1617201040-83905-1-git-send-email-guoren@kernel.org>
From: Michael Clark <michaeljclark@mac.com>
Update the RISC-V port to use the generic qspinlock and qrwlock.
This patch requires support for xchg_xtail for full-word which
are added by a previous patch:
Guo added select ARCH_USE_QUEUED_SPINLOCKS_XCHG32 in Kconfig
Guo fixed up compile error which made by below include sequence:
+#include <asm/qrwlock.h>
+#include <asm/qspinlock.h>
Signed-off-by: Michael Clark <michaeljclark@mac.com>
Co-developed-by: Guo Ren <guoren@linux.alibaba.com>
Tested-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Link: https://lore.kernel.org/linux-riscv/20190211043829.30096-3-michaeljclark@mac.com/
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Anup Patel <anup@brainfault.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Palmer Dabbelt <palmerdabbelt@google.com>
---
arch/riscv/Kconfig | 3 +
arch/riscv/include/asm/Kbuild | 3 +
arch/riscv/include/asm/spinlock.h | 126 +-----------------------
arch/riscv/include/asm/spinlock_types.h | 15 +--
4 files changed, 11 insertions(+), 136 deletions(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 87d7b52f278f..67cc65ba1ea1 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -33,6 +33,9 @@ config RISCV
select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
select ARCH_WANT_FRAME_POINTERS
select ARCH_WANT_HUGE_PMD_SHARE if 64BIT
+ select ARCH_USE_QUEUED_RWLOCKS
+ select ARCH_USE_QUEUED_SPINLOCKS
+ select ARCH_USE_QUEUED_SPINLOCKS_XCHG32
select CLONE_BACKWARDS
select CLINT_TIMER if !MMU
select COMMON_CLK
diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
index 445ccc97305a..750c1056b90f 100644
--- a/arch/riscv/include/asm/Kbuild
+++ b/arch/riscv/include/asm/Kbuild
@@ -3,5 +3,8 @@ generic-y += early_ioremap.h
generic-y += extable.h
generic-y += flat.h
generic-y += kvm_para.h
+generic-y += mcs_spinlock.h
+generic-y += qrwlock.h
+generic-y += qspinlock.h
generic-y += user.h
generic-y += vmlinux.lds.h
diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
index f4f7fa1b7ca8..a557de67a425 100644
--- a/arch/riscv/include/asm/spinlock.h
+++ b/arch/riscv/include/asm/spinlock.h
@@ -7,129 +7,7 @@
#ifndef _ASM_RISCV_SPINLOCK_H
#define _ASM_RISCV_SPINLOCK_H
-#include <linux/kernel.h>
-#include <asm/current.h>
-#include <asm/fence.h>
-
-/*
- * Simple spin lock operations. These provide no fairness guarantees.
- */
-
-/* FIXME: Replace this with a ticket lock, like MIPS. */
-
-#define arch_spin_is_locked(x) (READ_ONCE((x)->lock) != 0)
-
-static inline void arch_spin_unlock(arch_spinlock_t *lock)
-{
- smp_store_release(&lock->lock, 0);
-}
-
-static inline int arch_spin_trylock(arch_spinlock_t *lock)
-{
- int tmp = 1, busy;
-
- __asm__ __volatile__ (
- " amoswap.w %0, %2, %1\n"
- RISCV_ACQUIRE_BARRIER
- : "=r" (busy), "+A" (lock->lock)
- : "r" (tmp)
- : "memory");
-
- return !busy;
-}
-
-static inline void arch_spin_lock(arch_spinlock_t *lock)
-{
- while (1) {
- if (arch_spin_is_locked(lock))
- continue;
-
- if (arch_spin_trylock(lock))
- break;
- }
-}
-
-/***********************************************************/
-
-static inline void arch_read_lock(arch_rwlock_t *lock)
-{
- int tmp;
-
- __asm__ __volatile__(
- "1: lr.w %1, %0\n"
- " bltz %1, 1b\n"
- " addi %1, %1, 1\n"
- " sc.w %1, %1, %0\n"
- " bnez %1, 1b\n"
- RISCV_ACQUIRE_BARRIER
- : "+A" (lock->lock), "=&r" (tmp)
- :: "memory");
-}
-
-static inline void arch_write_lock(arch_rwlock_t *lock)
-{
- int tmp;
-
- __asm__ __volatile__(
- "1: lr.w %1, %0\n"
- " bnez %1, 1b\n"
- " li %1, -1\n"
- " sc.w %1, %1, %0\n"
- " bnez %1, 1b\n"
- RISCV_ACQUIRE_BARRIER
- : "+A" (lock->lock), "=&r" (tmp)
- :: "memory");
-}
-
-static inline int arch_read_trylock(arch_rwlock_t *lock)
-{
- int busy;
-
- __asm__ __volatile__(
- "1: lr.w %1, %0\n"
- " bltz %1, 1f\n"
- " addi %1, %1, 1\n"
- " sc.w %1, %1, %0\n"
- " bnez %1, 1b\n"
- RISCV_ACQUIRE_BARRIER
- "1:\n"
- : "+A" (lock->lock), "=&r" (busy)
- :: "memory");
-
- return !busy;
-}
-
-static inline int arch_write_trylock(arch_rwlock_t *lock)
-{
- int busy;
-
- __asm__ __volatile__(
- "1: lr.w %1, %0\n"
- " bnez %1, 1f\n"
- " li %1, -1\n"
- " sc.w %1, %1, %0\n"
- " bnez %1, 1b\n"
- RISCV_ACQUIRE_BARRIER
- "1:\n"
- : "+A" (lock->lock), "=&r" (busy)
- :: "memory");
-
- return !busy;
-}
-
-static inline void arch_read_unlock(arch_rwlock_t *lock)
-{
- __asm__ __volatile__(
- RISCV_RELEASE_BARRIER
- " amoadd.w x0, %1, %0\n"
- : "+A" (lock->lock)
- : "r" (-1)
- : "memory");
-}
-
-static inline void arch_write_unlock(arch_rwlock_t *lock)
-{
- smp_store_release(&lock->lock, 0);
-}
+#include <asm/qspinlock.h>
+#include <asm/qrwlock.h>
#endif /* _ASM_RISCV_SPINLOCK_H */
diff --git a/arch/riscv/include/asm/spinlock_types.h b/arch/riscv/include/asm/spinlock_types.h
index f398e7638dd6..d033a973f287 100644
--- a/arch/riscv/include/asm/spinlock_types.h
+++ b/arch/riscv/include/asm/spinlock_types.h
@@ -6,20 +6,11 @@
#ifndef _ASM_RISCV_SPINLOCK_TYPES_H
#define _ASM_RISCV_SPINLOCK_TYPES_H
-#ifndef __LINUX_SPINLOCK_TYPES_H
+#if !defined(__LINUX_SPINLOCK_TYPES_H) && !defined(_ASM_RISCV_SPINLOCK_H)
# error "please don't include this file directly"
#endif
-typedef struct {
- volatile unsigned int lock;
-} arch_spinlock_t;
-
-#define __ARCH_SPIN_LOCK_UNLOCKED { 0 }
-
-typedef struct {
- volatile unsigned int lock;
-} arch_rwlock_t;
-
-#define __ARCH_RW_LOCK_UNLOCKED { 0 }
+#include <asm-generic/qspinlock_types.h>
+#include <asm-generic/qrwlock_types.h>
#endif /* _ASM_RISCV_SPINLOCK_TYPES_H */
--
2.17.1
^ permalink raw reply related
* [PATCH v6 3/9] riscv: locks: Introduce ticket-based spinlock implementation
From: guoren @ 2021-03-31 14:30 UTC (permalink / raw)
To: guoren
Cc: linux-arch, linux-xtensa, Guo Ren, Arnd Bergmann, Peter Zijlstra,
Anup Patel, linux-kernel, linux-csky, openrisc, sparclinux,
linux-riscv, linuxppc-dev
In-Reply-To: <1617201040-83905-1-git-send-email-guoren@kernel.org>
From: Guo Ren <guoren@linux.alibaba.com>
This patch introduces a ticket lock implementation for riscv, along the
same lines as the implementation for arch/arm & arch/csky.
We still use qspinlock as default.
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Anup Patel <anup@brainfault.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
arch/riscv/Kconfig | 7 ++-
arch/riscv/include/asm/spinlock.h | 84 +++++++++++++++++++++++++
arch/riscv/include/asm/spinlock_types.h | 17 +++++
3 files changed, 107 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 67cc65ba1ea1..34d0276f01d5 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -34,7 +34,7 @@ config RISCV
select ARCH_WANT_FRAME_POINTERS
select ARCH_WANT_HUGE_PMD_SHARE if 64BIT
select ARCH_USE_QUEUED_RWLOCKS
- select ARCH_USE_QUEUED_SPINLOCKS
+ select ARCH_USE_QUEUED_SPINLOCKS if !RISCV_TICKET_LOCK
select ARCH_USE_QUEUED_SPINLOCKS_XCHG32
select CLONE_BACKWARDS
select CLINT_TIMER if !MMU
@@ -344,6 +344,11 @@ config NEED_PER_CPU_EMBED_FIRST_CHUNK
def_bool y
depends on NUMA
+config RISCV_TICKET_LOCK
+ bool "Ticket-based spin-locking"
+ help
+ Say Y here to use ticket-based spin-locking.
+
config RISCV_ISA_C
bool "Emit compressed instructions when building Linux"
default y
diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
index a557de67a425..90b7eaa950cf 100644
--- a/arch/riscv/include/asm/spinlock.h
+++ b/arch/riscv/include/asm/spinlock.h
@@ -7,7 +7,91 @@
#ifndef _ASM_RISCV_SPINLOCK_H
#define _ASM_RISCV_SPINLOCK_H
+#ifdef CONFIG_RISCV_TICKET_LOCK
+#ifdef CONFIG_32BIT
+#define __ASM_SLLIW "slli\t"
+#define __ASM_SRLIW "srli\t"
+#else
+#define __ASM_SLLIW "slliw\t"
+#define __ASM_SRLIW "srliw\t"
+#endif
+
+/*
+ * Ticket-based spin-locking.
+ */
+static inline void arch_spin_lock(arch_spinlock_t *lock)
+{
+ arch_spinlock_t lockval;
+ u32 tmp;
+
+ asm volatile (
+ "1: lr.w %0, %2 \n"
+ " mv %1, %0 \n"
+ " addw %0, %0, %3 \n"
+ " sc.w %0, %0, %2 \n"
+ " bnez %0, 1b \n"
+ : "=&r" (tmp), "=&r" (lockval), "+A" (lock->lock)
+ : "r" (1 << TICKET_NEXT)
+ : "memory");
+
+ smp_cond_load_acquire(&lock->tickets.owner,
+ VAL == lockval.tickets.next);
+}
+
+static inline int arch_spin_trylock(arch_spinlock_t *lock)
+{
+ u32 tmp, contended, res;
+
+ do {
+ asm volatile (
+ " lr.w %0, %3 \n"
+ __ASM_SRLIW "%1, %0, %5 \n"
+ __ASM_SLLIW "%2, %0, %5 \n"
+ " or %1, %2, %1 \n"
+ " li %2, 0 \n"
+ " sub %1, %1, %0 \n"
+ " bnez %1, 1f \n"
+ " addw %0, %0, %4 \n"
+ " sc.w %2, %0, %3 \n"
+ "1: \n"
+ : "=&r" (tmp), "=&r" (contended), "=&r" (res),
+ "+A" (lock->lock)
+ : "r" (1 << TICKET_NEXT), "I" (TICKET_NEXT)
+ : "memory");
+ } while (res);
+
+ if (!contended)
+ __atomic_acquire_fence();
+
+ return !contended;
+}
+
+static inline void arch_spin_unlock(arch_spinlock_t *lock)
+{
+ smp_store_release(&lock->tickets.owner, lock->tickets.owner + 1);
+}
+
+static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
+{
+ return lock.tickets.owner == lock.tickets.next;
+}
+
+static inline int arch_spin_is_locked(arch_spinlock_t *lock)
+{
+ return !arch_spin_value_unlocked(READ_ONCE(*lock));
+}
+
+static inline int arch_spin_is_contended(arch_spinlock_t *lock)
+{
+ struct __raw_tickets tickets = READ_ONCE(lock->tickets);
+
+ return (tickets.next - tickets.owner) > 1;
+}
+#define arch_spin_is_contended arch_spin_is_contended
+#else /* CONFIG_RISCV_TICKET_LOCK */
#include <asm/qspinlock.h>
+#endif /* CONFIG_RISCV_TICKET_LOCK */
+
#include <asm/qrwlock.h>
#endif /* _ASM_RISCV_SPINLOCK_H */
diff --git a/arch/riscv/include/asm/spinlock_types.h b/arch/riscv/include/asm/spinlock_types.h
index d033a973f287..afbb19841d0f 100644
--- a/arch/riscv/include/asm/spinlock_types.h
+++ b/arch/riscv/include/asm/spinlock_types.h
@@ -10,7 +10,24 @@
# error "please don't include this file directly"
#endif
+#ifdef CONFIG_RISCV_TICKET_LOCK
+#define TICKET_NEXT 16
+
+typedef struct {
+ union {
+ u32 lock;
+ struct __raw_tickets {
+ /* little endian */
+ u16 owner;
+ u16 next;
+ } tickets;
+ };
+} arch_spinlock_t;
+
+#define __ARCH_SPIN_LOCK_UNLOCKED { { 0 } }
+#else
#include <asm-generic/qspinlock_types.h>
+#endif
#include <asm-generic/qrwlock_types.h>
#endif /* _ASM_RISCV_SPINLOCK_TYPES_H */
--
2.17.1
^ permalink raw reply related
* [PATCH v6 4/9] csky: locks: Optimize coding convention
From: guoren @ 2021-03-31 14:30 UTC (permalink / raw)
To: guoren
Cc: linux-arch, linux-xtensa, Guo Ren, Arnd Bergmann, Peter Zijlstra,
linux-kernel, linux-csky, openrisc, sparclinux, linux-riscv,
linuxppc-dev
In-Reply-To: <1617201040-83905-1-git-send-email-guoren@kernel.org>
From: Guo Ren <guoren@linux.alibaba.com>
- Using smp_cond_load_acquire in arch_spin_lock by Peter's
advice.
- Using __smp_acquire_fence in arch_spin_trylock
- Using smp_store_release in arch_spin_unlock
All above are just coding conventions and won't affect the
function.
TODO in smp_cond_load_acquire for architecture:
- current csky only has:
lr.w val, <p0>
sc.w <p0>. val2
(Any other stores to p0 will let sc.w failed)
- But smp_cond_load_acquire need:
lr.w val, <p0>
wfe
(Any stores to p0 will send the event to let wfe retired)
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Link: https://lore.kernel.org/linux-riscv/CAAhSdy1JHLUFwu7RuCaQ+RUWRBks2KsDva7EpRt8--4ZfofSUQ@mail.gmail.com/T/#m13adac285b7f51f4f879a5d6b65753ecb1a7524e
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
arch/csky/include/asm/spinlock.h | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/arch/csky/include/asm/spinlock.h b/arch/csky/include/asm/spinlock.h
index 69f5aa249c5f..69677167977a 100644
--- a/arch/csky/include/asm/spinlock.h
+++ b/arch/csky/include/asm/spinlock.h
@@ -26,10 +26,8 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
: "r"(p), "r"(ticket_next)
: "cc");
- while (lockval.tickets.next != lockval.tickets.owner)
- lockval.tickets.owner = READ_ONCE(lock->tickets.owner);
-
- smp_mb();
+ smp_cond_load_acquire(&lock->tickets.owner,
+ VAL == lockval.tickets.next);
}
static inline int arch_spin_trylock(arch_spinlock_t *lock)
@@ -55,15 +53,14 @@ static inline int arch_spin_trylock(arch_spinlock_t *lock)
} while (!res);
if (!contended)
- smp_mb();
+ __smp_acquire_fence();
return !contended;
}
static inline void arch_spin_unlock(arch_spinlock_t *lock)
{
- smp_mb();
- WRITE_ONCE(lock->tickets.owner, lock->tickets.owner + 1);
+ smp_store_release(&lock->tickets.owner, lock->tickets.owner + 1);
}
static inline int arch_spin_value_unlocked(arch_spinlock_t lock)
--
2.17.1
^ permalink raw reply related
* [PATCH v6 5/9] csky: Convert custom spinlock/rwlock to generic qspinlock/qrwlock
From: guoren @ 2021-03-31 14:30 UTC (permalink / raw)
To: guoren
Cc: linux-arch, linux-xtensa, Guo Ren, Arnd Bergmann, Peter Zijlstra,
Will Deacon, linux-kernel, linux-csky, openrisc, sparclinux,
Waiman Long, linux-riscv, linuxppc-dev
In-Reply-To: <1617201040-83905-1-git-send-email-guoren@kernel.org>
From: Guo Ren <guoren@linux.alibaba.com>
Update the C-SKY port to use the generic qspinlock and qrwlock.
C-SKY only support ldex.w/stex.w with word(double word) size &
align access. So it must select XCHG32 to let qspinlock only use
word atomic xchg_tail.
Default is still ticket lock.
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Cc: Waiman Long <longman@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
---
arch/csky/Kconfig | 8 ++++++++
arch/csky/include/asm/Kbuild | 2 ++
arch/csky/include/asm/spinlock.h | 4 ++++
arch/csky/include/asm/spinlock_types.h | 4 ++++
4 files changed, 18 insertions(+)
diff --git a/arch/csky/Kconfig b/arch/csky/Kconfig
index 34e91224adc3..ae12332edb7b 100644
--- a/arch/csky/Kconfig
+++ b/arch/csky/Kconfig
@@ -8,6 +8,8 @@ config CSKY
select ARCH_HAS_SYNC_DMA_FOR_DEVICE
select ARCH_USE_BUILTIN_BSWAP
select ARCH_USE_QUEUED_RWLOCKS
+ select ARCH_USE_QUEUED_SPINLOCKS if !CSKY_TICKET_LOCK
+ select ARCH_USE_QUEUED_SPINLOCKS_XCHG32
select ARCH_WANT_FRAME_POINTERS if !CPU_CK610
select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT
select COMMON_CLK
@@ -304,6 +306,12 @@ config NR_CPUS
depends on SMP
default "4"
+config CSKY_TICKET_LOCK
+ bool "Ticket-based spin-locking"
+ default y
+ help
+ Say Y here to use ticket-based spin-locking.
+
config HIGHMEM
bool "High Memory Support"
depends on !CPU_CK610
diff --git a/arch/csky/include/asm/Kbuild b/arch/csky/include/asm/Kbuild
index cc24bb8e539f..2a2d09963bb9 100644
--- a/arch/csky/include/asm/Kbuild
+++ b/arch/csky/include/asm/Kbuild
@@ -2,6 +2,8 @@
generic-y += asm-offsets.h
generic-y += gpio.h
generic-y += kvm_para.h
+generic-y += mcs_spinlock.h
generic-y += qrwlock.h
+generic-y += qspinlock.h
generic-y += user.h
generic-y += vmlinux.lds.h
diff --git a/arch/csky/include/asm/spinlock.h b/arch/csky/include/asm/spinlock.h
index 69677167977a..fe98ad8ece51 100644
--- a/arch/csky/include/asm/spinlock.h
+++ b/arch/csky/include/asm/spinlock.h
@@ -6,6 +6,7 @@
#include <linux/spinlock_types.h>
#include <asm/barrier.h>
+#ifdef CONFIG_CSKY_TICKET_LOCK
/*
* Ticket-based spin-locking.
*/
@@ -80,6 +81,9 @@ static inline int arch_spin_is_contended(arch_spinlock_t *lock)
return (tickets.next - tickets.owner) > 1;
}
#define arch_spin_is_contended arch_spin_is_contended
+#else /* CONFIG_CSKY_TICKET_LOCK */
+#include <asm/qspinlock.h>
+#endif /* CONFIG_CSKY_TICKET_LOCK */
#include <asm/qrwlock.h>
diff --git a/arch/csky/include/asm/spinlock_types.h b/arch/csky/include/asm/spinlock_types.h
index 8ff0f6ff3a00..547f035f6dd5 100644
--- a/arch/csky/include/asm/spinlock_types.h
+++ b/arch/csky/include/asm/spinlock_types.h
@@ -7,6 +7,7 @@
# error "please don't include this file directly"
#endif
+#ifdef CONFIG_CSKY_TICKET_LOCK
#define TICKET_NEXT 16
typedef struct {
@@ -21,6 +22,9 @@ typedef struct {
} arch_spinlock_t;
#define __ARCH_SPIN_LOCK_UNLOCKED { { 0 } }
+#else
+#include <asm-generic/qspinlock_types.h>
+#endif
#include <asm-generic/qrwlock_types.h>
--
2.17.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