* Re: [RFC PATCH 0/3] Use per-CPU temporary mappings for patching
From: Christophe Leroy @ 2020-03-24 16:02 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev
In-Reply-To: <3bfcb682-a33a-b41c-74c6-4bae0d277ddf@c-s.fr>
Le 23/03/2020 à 15:04, Christophe Leroy a écrit :
>
>
> On 03/23/2020 11:30 AM, Christophe Leroy wrote:
>>
>>
>> On 03/23/2020 04:52 AM, Christopher M. Riedl wrote:
>>> When compiled with CONFIG_STRICT_KERNEL_RWX, the kernel must create
>>> temporary mappings when patching itself. These mappings temporarily
>>> override the strict RWX text protections to permit a write. Currently,
>>> powerpc allocates a per-CPU VM area for patching. Patching occurs as
>>> follows:
>>>
>>> 1. Map page of text to be patched to per-CPU VM area w/
>>> PAGE_KERNEL protection
>>> 2. Patch text
>>> 3. Remove the temporary mapping
>>>
>>> While the VM area is per-CPU, the mapping is actually inserted into the
>>> kernel page tables. Presumably, this could allow another CPU to access
>>> the normally write-protected text - either malicously or accidentally -
>>> via this same mapping if the address of the VM area is known. Ideally,
>>> the mapping should be kept local to the CPU doing the patching (or any
>>> other sensitive operations requiring temporarily overriding memory
>>> protections) [0].
>>>
>>> x86 introduced "temporary mm" structs which allow the creation of
>>> mappings local to a particular CPU [1]. This series intends to bring the
>>> notion of a temporary mm to powerpc and harden powerpc by using such a
>>> mapping for patching a kernel with strict RWX permissions.
>>>
>>> The first patch introduces the temporary mm struct and API for powerpc
>>> along with a new function to retrieve a current hw breakpoint.
>>>
>>> The second patch uses the `poking_init` init hook added by the x86
>>> patches to initialize a temporary mm and patching address. The patching
>>> address is randomized between 0 and DEFAULT_MAP_WINDOW-PAGE_SIZE. The
>>> upper limit is necessary due to how the hash MMU operates - by default
>>> the space above DEFAULT_MAP_WINDOW is not available. For now, both hash
>>> and radix randomize inside this range. The number of possible random
>>> addresses is dependent on PAGE_SIZE and limited by DEFAULT_MAP_WINDOW.
>>>
>>> Bits of entropy with 64K page size on BOOK3S_64:
>>>
>>> bits-o-entropy = log2(DEFAULT_MAP_WINDOW_USER64 / PAGE_SIZE)
>>>
>>> PAGE_SIZE=64K, DEFAULT_MAP_WINDOW_USER64=128TB
>>> bits-o-entropy = log2(128TB / 64K)
>>> bits-o-entropy = 31
>>>
>>> Currently, randomization occurs only once during initialization at boot.
>>>
>>> The third patch replaces the VM area with the temporary mm in the
>>> patching code. The page for patching has to be mapped PAGE_SHARED with
>>> the hash MMU since hash prevents the kernel from accessing userspace
>>> pages with PAGE_PRIVILEGED bit set. There is on-going work on my side to
>>> explore if this is actually necessary in the hash codepath.
>>>
>>> Testing so far is limited to booting on QEMU (power8 and power9 targets)
>>> and a POWER8 VM along with setting some simple xmon breakpoints (which
>>> makes use of code-patching). A POC lkdtm test is in-progress to actually
>>> exploit the existing vulnerability (ie. the mapping during patching is
>>> exposed in kernel page tables and accessible by other CPUS) - this will
>>> accompany a future v1 of this series.
>>
>> Got following failures on an 8xx. Note that "fault blocked by AP
>> register !" means an unauthorised access from Kernel to Userspace.
>>
>
> Still a problem even without CONFIG_PPC_KUAP:
>
I've been able to dig into the problem.
With CONFIG_PPC_KUAP, it can definitely not work. See why in commit
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=ef296729b735e083d8919e76ac213b8ff237eb78
Without CONFIG_PPC_KUAP, on the 8xx, __put_user_asm() in
__patch_instruction() returns -EFAULT. That's because _PAGE_DIRTY is not
set on the page. Normally it should be a minor fault and the fault
handler should set the _PAGE_DIRTY flag. It must be something in the way
the page is allocated and mapped which prevents that. If I forge
_PAGE_DIRTY in addition to PAGE_SHARED, it works. But I don't think it
is valid approach to solve the issue.
Christophe
^ permalink raw reply
* Re: [RFC PATCH 1/3] powerpc/mm: Introduce temporary mm
From: Christophe Leroy @ 2020-03-24 16:07 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev
In-Reply-To: <20200323045205.20314-2-cmr@informatik.wtf>
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
> x86 supports the notion of a temporary mm which restricts access to
> temporary PTEs to a single CPU. A temporary mm is useful for situations
> where a CPU needs to perform sensitive operations (such as patching a
> STRICT_KERNEL_RWX kernel) requiring temporary mappings without exposing
> said mappings to other CPUs. A side benefit is that other CPU TLBs do
> not need to be flushed when the temporary mm is torn down.
>
> Mappings in the temporary mm can be set in the userspace portion of the
> address-space.
>
> Interrupts must be disabled while the temporary mm is in use. HW
> breakpoints, which may have been set by userspace as watchpoints on
> addresses now within the temporary mm, are saved and disabled when
> loading the temporary mm. The HW breakpoints are restored when unloading
> the temporary mm. All HW breakpoints are indiscriminately disabled while
> the temporary mm is in use.
>
> Based on x86 implementation:
>
> commit cefa929c034e
> ("x86/mm: Introduce temporary mm structs")
>
> Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
> ---
> arch/powerpc/include/asm/debug.h | 1 +
> arch/powerpc/include/asm/mmu_context.h | 56 +++++++++++++++++++++++++-
> arch/powerpc/kernel/process.c | 5 +++
> 3 files changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/debug.h b/arch/powerpc/include/asm/debug.h
> index 7756026b95ca..b945bc16c932 100644
> --- a/arch/powerpc/include/asm/debug.h
> +++ b/arch/powerpc/include/asm/debug.h
> @@ -45,6 +45,7 @@ static inline int debugger_break_match(struct pt_regs *regs) { return 0; }
> static inline int debugger_fault_handler(struct pt_regs *regs) { return 0; }
> #endif
>
> +void __get_breakpoint(struct arch_hw_breakpoint *brk);
> void __set_breakpoint(struct arch_hw_breakpoint *brk);
> bool ppc_breakpoint_available(void);
> #ifdef CONFIG_PPC_ADV_DEBUG_REGS
> diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> index 360367c579de..3e6381d04c28 100644
> --- a/arch/powerpc/include/asm/mmu_context.h
> +++ b/arch/powerpc/include/asm/mmu_context.h
> @@ -7,9 +7,10 @@
> #include <linux/mm.h>
> #include <linux/sched.h>
> #include <linux/spinlock.h>
> -#include <asm/mmu.h>
> +#include <asm/mmu.h>
What's this change ?
I see you are removing a space at the end of the line, but it shouldn't
be part of this patch.
> #include <asm/cputable.h>
> #include <asm/cputhreads.h>
> +#include <asm/hw_breakpoint.h>
>
> /*
> * Most if the context management is out of line
> @@ -270,5 +271,58 @@ static inline int arch_dup_mmap(struct mm_struct *oldmm,
> return 0;
> }
>
> +struct temp_mm {
> + struct mm_struct *temp;
> + struct mm_struct *prev;
> + bool is_kernel_thread;
> + struct arch_hw_breakpoint brk;
> +};
> +
> +static inline void init_temp_mm(struct temp_mm *temp_mm, struct mm_struct *mm)
> +{
> + temp_mm->temp = mm;
> + temp_mm->prev = NULL;
> + temp_mm->is_kernel_thread = false;
> + memset(&temp_mm->brk, 0, sizeof(temp_mm->brk));
> +}
> +
> +static inline void use_temporary_mm(struct temp_mm *temp_mm)
> +{
> + lockdep_assert_irqs_disabled();
> +
> + temp_mm->is_kernel_thread = current->mm == NULL;
> + if (temp_mm->is_kernel_thread)
> + temp_mm->prev = current->active_mm;
> + else
> + temp_mm->prev = current->mm;
> +
> + /*
> + * Hash requires a non-NULL current->mm to allocate a userspace address
> + * when handling a page fault. Does not appear to hurt in Radix either.
> + */
> + current->mm = temp_mm->temp;
> + switch_mm_irqs_off(NULL, temp_mm->temp, current);
> +
> + if (ppc_breakpoint_available()) {
> + __get_breakpoint(&temp_mm->brk);
> + if (temp_mm->brk.type != 0)
> + hw_breakpoint_disable();
> + }
> +}
> +
> +static inline void unuse_temporary_mm(struct temp_mm *temp_mm)
> +{
> + lockdep_assert_irqs_disabled();
> +
> + if (temp_mm->is_kernel_thread)
> + current->mm = NULL;
> + else
> + current->mm = temp_mm->prev;
> + switch_mm_irqs_off(NULL, temp_mm->prev, current);
> +
> + if (ppc_breakpoint_available() && temp_mm->brk.type != 0)
> + __set_breakpoint(&temp_mm->brk);
> +}
> +
> #endif /* __KERNEL__ */
> #endif /* __ASM_POWERPC_MMU_CONTEXT_H */
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index fad50db9dcf2..5e5cf33fc358 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -793,6 +793,11 @@ static inline int set_breakpoint_8xx(struct arch_hw_breakpoint *brk)
> return 0;
> }
>
> +void __get_breakpoint(struct arch_hw_breakpoint *brk)
> +{
> + memcpy(brk, this_cpu_ptr(¤t_brk), sizeof(*brk));
> +}
> +
> void __set_breakpoint(struct arch_hw_breakpoint *brk)
> {
> memcpy(this_cpu_ptr(¤t_brk), brk, sizeof(*brk));
>
Christophe
^ permalink raw reply
* Re: [RFC PATCH 2/3] powerpc/lib: Initialize a temporary mm for code patching
From: Christophe Leroy @ 2020-03-24 16:10 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev
In-Reply-To: <20200323045205.20314-3-cmr@informatik.wtf>
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
> When code patching a STRICT_KERNEL_RWX kernel the page containing the
> address to be patched is temporarily mapped with permissive memory
> protections. Currently, a per-cpu vmalloc patch area is used for this
> purpose. While the patch area is per-cpu, the temporary page mapping is
> inserted into the kernel page tables for the duration of the patching.
> The mapping is exposed to CPUs other than the patching CPU - this is
> undesirable from a hardening perspective.
>
> Use the `poking_init` init hook to prepare a temporary mm and patching
> address. Initialize the temporary mm by copying the init mm. Choose a
> randomized patching address inside the temporary mm userspace address
> portion. The next patch uses the temporary mm and patching address for
> code patching.
>
> Based on x86 implementation:
>
> commit 4fc19708b165
> ("x86/alternatives: Initialize temporary mm for patching")
>
> Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
> ---
> arch/powerpc/lib/code-patching.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 3345f039a876..18b88ecfc5a8 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -11,6 +11,8 @@
> #include <linux/cpuhotplug.h>
> #include <linux/slab.h>
> #include <linux/uaccess.h>
> +#include <linux/sched/task.h>
> +#include <linux/random.h>
>
> #include <asm/pgtable.h>
> #include <asm/tlbflush.h>
> @@ -39,6 +41,30 @@ int raw_patch_instruction(unsigned int *addr, unsigned int instr)
> }
>
> #ifdef CONFIG_STRICT_KERNEL_RWX
> +
> +__ro_after_init struct mm_struct *patching_mm;
> +__ro_after_init unsigned long patching_addr;
Can we make those those static ?
> +
> +void __init poking_init(void)
> +{
> + spinlock_t *ptl; /* for protecting pte table */
> + pte_t *ptep;
> +
> + patching_mm = copy_init_mm();
> + BUG_ON(!patching_mm);
Does it needs to be a BUG_ON() ? Can't we fail gracefully with just a
WARN_ON ?
> +
> + /*
> + * In hash we cannot go above DEFAULT_MAP_WINDOW easily.
> + * XXX: Do we want additional bits of entropy for radix?
> + */
> + patching_addr = (get_random_long() & PAGE_MASK) %
> + (DEFAULT_MAP_WINDOW - PAGE_SIZE);
> +
> + ptep = get_locked_pte(patching_mm, patching_addr, &ptl);
> + BUG_ON(!ptep);
Same here, can we fail gracefully instead ?
> + pte_unmap_unlock(ptep, ptl);
> +}
> +
> static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
>
> static int text_area_cpu_up(unsigned int cpu)
>
Christophe
^ permalink raw reply
* Re: [RFC PATCH 3/3] powerpc/lib: Use a temporary mm for code patching
From: Christophe Leroy @ 2020-03-24 16:25 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev
In-Reply-To: <20200323045205.20314-4-cmr@informatik.wtf>
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
> Currently, code patching a STRICT_KERNEL_RWX exposes the temporary
> mappings to other CPUs. These mappings should be kept local to the CPU
> doing the patching. Use the pre-initialized temporary mm and patching
> address for this purpose. Also add a check after patching to ensure the
> patch succeeded.
>
> Based on x86 implementation:
>
> commit b3fd8e83ada0
> ("x86/alternatives: Use temporary mm for text poking")
>
> Signed-off-by: Christopher M. Riedl <cmr@informatik.wtf>
> ---
> arch/powerpc/lib/code-patching.c | 128 ++++++++++++++-----------------
> 1 file changed, 57 insertions(+), 71 deletions(-)
>
> diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
> index 18b88ecfc5a8..f156132e8975 100644
> --- a/arch/powerpc/lib/code-patching.c
> +++ b/arch/powerpc/lib/code-patching.c
> @@ -19,6 +19,7 @@
> #include <asm/page.h>
> #include <asm/code-patching.h>
> #include <asm/setup.h>
> +#include <asm/mmu_context.h>
>
> static int __patch_instruction(unsigned int *exec_addr, unsigned int instr,
> unsigned int *patch_addr)
> @@ -65,99 +66,79 @@ void __init poking_init(void)
> pte_unmap_unlock(ptep, ptl);
> }
>
> -static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
> -
> -static int text_area_cpu_up(unsigned int cpu)
> -{
> - struct vm_struct *area;
> -
> - area = get_vm_area(PAGE_SIZE, VM_ALLOC);
> - if (!area) {
> - WARN_ONCE(1, "Failed to create text area for cpu %d\n",
> - cpu);
> - return -1;
> - }
> - this_cpu_write(text_poke_area, area);
> -
> - return 0;
> -}
> -
> -static int text_area_cpu_down(unsigned int cpu)
> -{
> - free_vm_area(this_cpu_read(text_poke_area));
> - return 0;
> -}
> -
> -/*
> - * 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().
> - */
> -static int __init setup_text_poke_area(void)
> -{
> - BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> - "powerpc/text_poke:online", text_area_cpu_up,
> - text_area_cpu_down));
> -
> - return 0;
> -}
> -late_initcall(setup_text_poke_area);
> +struct patch_mapping {
> + spinlock_t *ptl; /* for protecting pte table */
> + struct temp_mm temp_mm;
> +};
>
> /*
> * This can be called for kernel text or a module.
> */
> -static int map_patch_area(void *addr, unsigned long text_poke_addr)
> +static int map_patch(const void *addr, struct patch_mapping *patch_mapping)
Why change the name ?
> {
> - unsigned long pfn;
> - int err;
> + struct page *page;
> + pte_t pte, *ptep;
> + pgprot_t pgprot;
>
> if (is_vmalloc_addr(addr))
> - pfn = vmalloc_to_pfn(addr);
> + page = vmalloc_to_page(addr);
> else
> - pfn = __pa_symbol(addr) >> PAGE_SHIFT;
> + page = virt_to_page(addr);
>
> - err = map_kernel_page(text_poke_addr, (pfn << PAGE_SHIFT), PAGE_KERNEL);
> + if (radix_enabled())
> + pgprot = __pgprot(pgprot_val(PAGE_KERNEL));
> + else
> + pgprot = PAGE_SHARED;
Can you explain the difference between radix and non radix ?
Why PAGE_KERNEL for a page that is mapped in userspace ?
Why do you need to do __pgprot(pgprot_val(PAGE_KERNEL)) instead of just
using PAGE_KERNEL ?
>
> - pr_devel("Mapped addr %lx with pfn %lx:%d\n", text_poke_addr, pfn, err);
> - if (err)
> + ptep = get_locked_pte(patching_mm, patching_addr, &patch_mapping->ptl);
> + if (unlikely(!ptep)) {
> + pr_warn("map patch: failed to allocate pte for patching\n");
> return -1;
> + }
> +
> + pte = mk_pte(page, pgprot);
> + set_pte_at(patching_mm, patching_addr, ptep, pte);
> +
> + init_temp_mm(&patch_mapping->temp_mm, patching_mm);
> + use_temporary_mm(&patch_mapping->temp_mm);
>
> return 0;
> }
>
> -static inline int unmap_patch_area(unsigned long addr)
> +static int unmap_patch(struct patch_mapping *patch_mapping)
> {
> pte_t *ptep;
> pmd_t *pmdp;
> pud_t *pudp;
> pgd_t *pgdp;
>
> - pgdp = pgd_offset_k(addr);
> + pgdp = pgd_offset(patching_mm, patching_addr);
> if (unlikely(!pgdp))
> return -EINVAL;
>
> - pudp = pud_offset(pgdp, addr);
> + pudp = pud_offset(pgdp, patching_addr);
> if (unlikely(!pudp))
> return -EINVAL;
>
> - pmdp = pmd_offset(pudp, addr);
> + pmdp = pmd_offset(pudp, patching_addr);
> if (unlikely(!pmdp))
> return -EINVAL;
>
> - ptep = pte_offset_kernel(pmdp, addr);
> + ptep = pte_offset_kernel(pmdp, patching_addr);
ptep should be stored in the patch_mapping struct instead of walking
again the page tables.
> if (unlikely(!ptep))
> return -EINVAL;
>
> - pr_devel("clearing mm %p, pte %p, addr %lx\n", &init_mm, ptep, addr);
> + /*
> + * In hash, pte_clear flushes the tlb
> + */
> + pte_clear(patching_mm, patching_addr, ptep);
> + unuse_temporary_mm(&patch_mapping->temp_mm);
>
> /*
> - * In hash, pte_clear flushes the tlb, in radix, we have to
> + * In radix, we have to explicitly flush the tlb (no-op in hash)
> */
> - pte_clear(&init_mm, addr, ptep);
> - flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
> + local_flush_tlb_mm(patching_mm);
> + pte_unmap_unlock(ptep, patch_mapping->ptl);
>
> return 0;
> }
> @@ -167,33 +148,38 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)
> int err;
> unsigned int *patch_addr = NULL;
> unsigned long flags;
> - unsigned long text_poke_addr;
> - unsigned long kaddr = (unsigned long)addr;
> + struct patch_mapping patch_mapping;
>
> /*
> - * During early early boot patch_instruction is called
> - * when text_poke_area is not ready, but we still need
> - * to allow patching. We just do the plain old patching
> + * The patching_mm is initialized before calling mark_rodata_ro. Prior
> + * to this, patch_instruction is called when we don't have (and don't
> + * need) the patching_mm so just do plain old patching.
> */
> - if (!this_cpu_read(text_poke_area))
> + if (!patching_mm)
> return raw_patch_instruction(addr, instr);
>
> local_irq_save(flags);
>
> - text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;
> - if (map_patch_area(addr, text_poke_addr)) {
> - err = -1;
> + err = map_patch(addr, &patch_mapping);
> + if (err)
> goto out;
> - }
>
> - patch_addr = (unsigned int *)(text_poke_addr) +
> - ((kaddr & ~PAGE_MASK) / sizeof(unsigned int));
> + patch_addr = (unsigned int *)(patching_addr) +
> + (offset_in_page((unsigned long)addr) /
> + sizeof(unsigned int));
>
> __patch_instruction(addr, instr, patch_addr);
The error returned by __patch_instruction() should be managed.
>
> - err = unmap_patch_area(text_poke_addr);
> + err = unmap_patch(&patch_mapping);
> if (err)
> - pr_warn("failed to unmap %lx\n", text_poke_addr);
> + pr_warn("unmap patch: failed to unmap patch\n");
> +
> + /*
> + * Something is wrong if what we just wrote doesn't match what we
> + * think we just wrote.
> + * XXX: BUG_ON() instead?
No, not a BUG_ON(). If patching fails, that's no a vital fault, we can
fail gracefully. You should return a fault instead.
> + */
> + WARN_ON(memcmp(addr, &instr, sizeof(instr)));
Come on. addr is an *int, instr is an int. By doing a memcmp() on
&instr, you for the compiler to write instr into the stack whereas local
vars are mainly in registers on RISC processors like powerpc. Following
should do it:
WARN_ON(*addr != instr);
>
> out:
> local_irq_restore(flags);
>
Christophe
^ permalink raw reply
* Re: [PATCH] Documentation: Clarify better about the rwsem non-owner release issue
From: Joel Fernandes @ 2020-03-24 18:26 UTC (permalink / raw)
To: Will Deacon
Cc: linux-doc, Peter Zijlstra, Sebastian Andrzej Siewior,
linux-kernel, Thomas Gleixner, Davidlohr Bueso, Arnd Bergmann,
Jonathan Corbet, Ingo Molnar, Linus Torvalds, Ingo Molnar,
Paul E . McKenney, linuxppc-dev, Steven Rostedt, Bjorn Helgaas,
Kurt Schwemmer, Kalle Valo,
linux-pci@vger.kernel.org Felipe Balbi, Greg Kroah-Hartman,
Randy Dunlap, linux-wireless, Oleg Nesterov, netdev,
Logan Gunthorpe, David S. Miller
In-Reply-To: <20200324081538.GA8696@willie-the-truck>
On Tue, Mar 24, 2020 at 08:15:39AM +0000, Will Deacon wrote:
> On Mon, Mar 23, 2020 at 02:23:49PM -0400, Joel Fernandes wrote:
> > On Sun, Mar 22, 2020 at 08:51:15AM +0200, Kalle Valo wrote:
> > > "Joel Fernandes (Google)" <joel@joelfernandes.org> writes:
> > >
> > > > Reword and clarify better about the rwsem non-owner release issue.
> > > >
> > > > Link: https://lore.kernel.org/linux-pci/20200321212144.GA6475@google.com/
> > > >
> > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > >
> > > There's something wrong with your linux-pci and linux-usb addresses:
> > >
> > > "linux-pci@vger.kernel.org Felipe Balbi" <balbi@kernel.org>,
> > >
> > >
> > > "linux-usb@vger.kernel.org Kalle Valo" <kvalo@codeaurora.org>,
> >
> > Not sure. It appears fine in the archive.
>
> Hmm, I don't think it does. Here's the copy from LKML:
>
> https://lore.kernel.org/lkml/20200322021938.175736-1-joel@joelfernandes.org/
>
> Which works because it's in the To: correctly. But both linux-pci and
> linux-usb were *not* CC'd:
>
> "linux-usb@vger.kernel.org Kalle Valo" <kvalo@codeaurora.org>
> "linux-pci@vger.kernel.org Felipe Balbi" <balbi@kernel.org>
>
> and searching for the message in the linux-pci archives doesn't find it:
>
> https://lore.kernel.org/linux-pci/?q=Reword+and+clarify+better+about+the+rwsem+non-owner+release+issue
>
> So it looks like there is an issue with your mail setup.
Hi Will and Kalle,
Thank you for confirming it. You are right, the archive shows the issue. I
will double check my client and see what's going on.
thanks,
- Joel
^ permalink raw reply
* [PATCH] powerpc/prom_init: Remove leftover comment
From: Fabiano Rosas @ 2020-03-24 18:29 UTC (permalink / raw)
To: linuxppc-dev
The if statement that this comment referred to was removed in
commit 11fdb309341c ("powerpc/prom_init: Remove support for OPAL v2").
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
arch/powerpc/kernel/prom_init.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 577345382b23..34ec630f3fc4 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -3474,7 +3474,6 @@ unsigned long __init prom_init(unsigned long r3, unsigned long r4,
*/
hdr = dt_header_start;
- /* Don't print anything after quiesce under OPAL, it crashes OFW */
prom_printf("Booting Linux via __start() @ 0x%lx ...\n", kbase);
prom_debug("->dt_header_start=0x%lx\n", hdr);
--
2.23.0
^ permalink raw reply related
* Re: [PATCH v11 5/8] powerpc/64: make buildable without CONFIG_COMPAT
From: Michal Suchánek @ 2020-03-24 19:30 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Mark Rutland, Gustavo Luiz Duarte, Alexander Shishkin,
Jordan Niethe, Sebastian Andrzej Siewior, Claudio Carvalho,
Paul Mackerras, Jiri Olsa, Rob Herring, Michael Neuling,
Eric Richter, Masahiro Yamada, Nayna Jain, Peter Zijlstra,
Ingo Molnar, Allison Randal, Valentin Schneider, Arnd Bergmann,
Arnaldo Carvalho de Melo, Alexander Viro, Jonathan Cameron,
Namhyung Kim, Thomas Gleixner, Andy Shevchenko, Hari Bathini,
Greg Kroah-Hartman, linux-kernel, Mauro Carvalho Chehab,
Eric W. Biederman, linux-fsdevel, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann
In-Reply-To: <1585039733.dm1rivvych.astroid@bobo.none>
On Tue, Mar 24, 2020 at 06:54:20PM +1000, Nicholas Piggin wrote:
> Michal Suchanek's on March 19, 2020 10:19 pm:
> > diff --git a/arch/powerpc/kernel/signal.c b/arch/powerpc/kernel/signal.c
> > index 4b0152108f61..a264989626fd 100644
> > --- a/arch/powerpc/kernel/signal.c
> > +++ b/arch/powerpc/kernel/signal.c
> > @@ -247,7 +247,6 @@ static void do_signal(struct task_struct *tsk)
> > sigset_t *oldset = sigmask_to_save();
> > struct ksignal ksig = { .sig = 0 };
> > int ret;
> > - int is32 = is_32bit_task();
> >
> > BUG_ON(tsk != current);
> >
> > @@ -277,7 +276,7 @@ static void do_signal(struct task_struct *tsk)
> >
> > rseq_signal_deliver(&ksig, tsk->thread.regs);
> >
> > - if (is32) {
> > + if (is_32bit_task()) {
> > if (ksig.ka.sa.sa_flags & SA_SIGINFO)
> > ret = handle_rt_signal32(&ksig, oldset, tsk);
> > else
>
> Unnecessary?
>
> > diff --git a/arch/powerpc/kernel/syscall_64.c b/arch/powerpc/kernel/syscall_64.c
> > index 87d95b455b83..2dcbfe38f5ac 100644
> > --- a/arch/powerpc/kernel/syscall_64.c
> > +++ b/arch/powerpc/kernel/syscall_64.c
> > @@ -24,7 +24,6 @@ notrace long system_call_exception(long r3, long r4, long r5,
> > long r6, long r7, long r8,
> > unsigned long r0, struct pt_regs *regs)
> > {
> > - unsigned long ti_flags;
> > syscall_fn f;
> >
> > if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
> > @@ -68,8 +67,7 @@ notrace long system_call_exception(long r3, long r4, long r5,
> >
> > local_irq_enable();
> >
> > - ti_flags = current_thread_info()->flags;
> > - if (unlikely(ti_flags & _TIF_SYSCALL_DOTRACE)) {
> > + if (unlikely(current_thread_info()->flags & _TIF_SYSCALL_DOTRACE)) {
> > /*
> > * We use the return value of do_syscall_trace_enter() as the
> > * syscall number. If the syscall was rejected for any reason
> > @@ -94,7 +92,7 @@ notrace long system_call_exception(long r3, long r4, long r5,
> > /* May be faster to do array_index_nospec? */
> > barrier_nospec();
> >
> > - if (unlikely(ti_flags & _TIF_32BIT)) {
> > + if (unlikely(is_32bit_task())) {
>
> Problem is, does this allow the load of ti_flags to be used for both
> tests, or does test_bit make it re-load?
>
> This could maybe be fixed by testing if(IS_ENABLED(CONFIG_COMPAT) &&
Both points already discussed here:
https://lore.kernel.org/linuxppc-dev/13fa324dc879a7f325290bf2e131b87eb491cd7b.1573576649.git.msuchanek@suse.de/
Thanks
Michal
^ permalink raw reply
* Re: [PATCH v11 3/8] powerpc/perf: consolidate read_user_stack_32
From: Michal Suchánek @ 2020-03-24 19:38 UTC (permalink / raw)
To: Nicholas Piggin
Cc: Mark Rutland, Gustavo Luiz Duarte, Alexander Shishkin,
Sebastian Andrzej Siewior, linux-kernel, Paul Mackerras,
Jiri Olsa, Rob Herring, Michael Neuling, Eric Richter,
Masahiro Yamada, Nayna Jain, Peter Zijlstra, Ingo Molnar,
Allison Randal, Jordan Niethe, Valentin Schneider, Arnd Bergmann,
Arnaldo Carvalho de Melo, Alexander Viro, Jonathan Cameron,
Namhyung Kim, Thomas Gleixner, Andy Shevchenko, Hari Bathini,
Greg Kroah-Hartman, Claudio Carvalho, Mauro Carvalho Chehab,
Eric W. Biederman, linux-fsdevel, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann
In-Reply-To: <1585039473.da4762n2s0.astroid@bobo.none>
On Tue, Mar 24, 2020 at 06:48:20PM +1000, Nicholas Piggin wrote:
> Michal Suchanek's on March 19, 2020 10:19 pm:
> > There are two almost identical copies for 32bit and 64bit.
> >
> > The function is used only in 32bit code which will be split out in next
> > patch so consolidate to one function.
> >
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
> > ---
> > v6: new patch
> > v8: move the consolidated function out of the ifdef block.
> > v11: rebase on top of def0bfdbd603
> > ---
> > arch/powerpc/perf/callchain.c | 48 +++++++++++++++++------------------
> > 1 file changed, 24 insertions(+), 24 deletions(-)
> >
> > diff --git a/arch/powerpc/perf/callchain.c b/arch/powerpc/perf/callchain.c
> > index cbc251981209..c9a78c6e4361 100644
> > --- a/arch/powerpc/perf/callchain.c
> > +++ b/arch/powerpc/perf/callchain.c
> > @@ -161,18 +161,6 @@ static int read_user_stack_64(unsigned long __user *ptr, unsigned long *ret)
> > return read_user_stack_slow(ptr, ret, 8);
> > }
> >
> > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> > -{
> > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
> > - ((unsigned long)ptr & 3))
> > - return -EFAULT;
> > -
> > - if (!probe_user_read(ret, ptr, sizeof(*ret)))
> > - return 0;
> > -
> > - return read_user_stack_slow(ptr, ret, 4);
> > -}
> > -
> > static inline int valid_user_sp(unsigned long sp, int is_64)
> > {
> > if (!sp || (sp & 7) || sp > (is_64 ? TASK_SIZE : 0x100000000UL) - 32)
> > @@ -277,19 +265,9 @@ static void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
> > }
> >
> > #else /* CONFIG_PPC64 */
> > -/*
> > - * On 32-bit we just access the address and let hash_page create a
> > - * HPTE if necessary, so there is no need to fall back to reading
> > - * the page tables. Since this is called at interrupt level,
> > - * do_page_fault() won't treat a DSI as a page fault.
> > - */
> > -static int read_user_stack_32(unsigned int __user *ptr, unsigned int *ret)
> > +static int read_user_stack_slow(void __user *ptr, void *buf, int nb)
> > {
> > - if ((unsigned long)ptr > TASK_SIZE - sizeof(unsigned int) ||
> > - ((unsigned long)ptr & 3))
> > - return -EFAULT;
> > -
> > - return probe_user_read(ret, ptr, sizeof(*ret));
> > + return 0;
> > }
> >
> > static inline void perf_callchain_user_64(struct perf_callchain_entry_ctx *entry,
> > @@ -312,6 +290,28 @@ static inline int valid_user_sp(unsigned long sp, int is_64)
> >
> > #endif /* CONFIG_PPC64 */
> >
> > +/*
> > + * On 32-bit we just access the address and let hash_page create a
> > + * HPTE if necessary, so there is no need to fall back to reading
> > + * the page tables. Since this is called at interrupt level,
> > + * do_page_fault() won't treat a DSI as a page fault.
> > + */
>
> The comment is actually probably better to stay in the 32-bit
> read_user_stack_slow implementation. Is that function defined
> on 32-bit purely so that you can use IS_ENABLED()? In that case
It documents the IS_ENABLED() and that's where it is. The 32bit
definition is only a technical detail.
> I would prefer to put a BUG() there which makes it self documenting.
Which will cause checkpatch complaints about introducing new BUG() which
is frowned on.
Thanks
Michal
^ permalink raw reply
* Re: Argh, can't find dcache properties !
From: Chris Packham @ 2020-03-24 20:06 UTC (permalink / raw)
To: christophe.leroy@c-s.fr, paulus@samba.org, mpe@ellerman.id.au,
benh@kernel.crashing.org, tglx@linutronix.de, cai@lca.pw
Cc: Hamish Martin, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <87tv2exst1.fsf@mpe.ellerman.id.au>
On Tue, 2020-03-24 at 15:47 +1100, Michael Ellerman wrote:
> Chris Packham <Chris.Packham@alliedtelesis.co.nz> writes:
> > Hi All,
> >
> > Just booting up v5.5.11 on a Freescale T2080RDB and I'm seeing the
> > following mesage.
> >
> > kern.warning linuxbox kernel: Argh, can't find dcache properties !
> > kern.warning linuxbox kernel: Argh, can't find icache properties !
> >
> > This was changed from DBG() to pr_warn() in commit 3b9176e9a874
> > ("powerpc/setup_64: fix -Wempty-body warnings") but the message
> > seems
> > to be much older than that. So it's probably been an issue on the
> > T2080
> > (and other QorIQ SoCs) for a while.
>
> That's an e6500 I think? So 64-bit Book3E.
>
Yes that's correct.
> You'll be getting the default values, which is 64 bytes so I guess
> that
> works in practice.
>
> > Looking at the code the t208x doesn't specifiy any of the d-cache-
> > size/i-cache-size properties. Should I add them to silence the
> > warning
> > or switch it to pr_debug()/pr_info()?
>
> Yeah ideally you'd add them to the device tree(s) for those boards.
>
I think the info I need is in the block diagram[0]. I'll whip up
a patch.
--
[1] - https://www.nxp.com/products/processors-and-microcontrollers/power-architecture/qoriq-communication-processors/t-series/qoriq-t2080-and-t2081-multicore-communications-processors:T2080
^ permalink raw reply
* [PATCH] powerpc/prom_init: Include the termination message in ibm, os-term RTAS call
From: Fabiano Rosas @ 2020-03-24 20:12 UTC (permalink / raw)
To: linuxppc-dev; +Cc: linuxram, paulus, kvm-ppc
QEMU can now print the ibm,os-term message[1], so let's include it in
the RTAS call. E.g.:
qemu-system-ppc64: OS terminated: Switch to secure mode failed.
1- https://git.qemu.org/?p=qemu.git;a=commitdiff;h=a4c3791ae0
Signed-off-by: Fabiano Rosas <farosas@linux.ibm.com>
---
arch/powerpc/kernel/prom_init.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index 577345382b23..d543fb6d29c5 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -1773,6 +1773,9 @@ static void __init prom_rtas_os_term(char *str)
if (token == 0)
prom_panic("Could not get token for ibm,os-term\n");
os_term_args.token = cpu_to_be32(token);
+ os_term_args.nargs = cpu_to_be32(1);
+ os_term_args.args[0] = cpu_to_be32(__pa(str));
+
prom_rtas_hcall((uint64_t)&os_term_args);
}
#endif /* CONFIG_PPC_SVM */
--
2.23.0
^ permalink raw reply related
* Re: Argh, can't find dcache properties !
From: Qian Cai @ 2020-03-24 20:13 UTC (permalink / raw)
To: Chris Packham
Cc: Hamish Martin, linux-kernel@vger.kernel.org, paulus@samba.org,
tglx@linutronix.de, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <876a5938fbad9d9e176e5f22f12e6b472d0dc4f7.camel@alliedtelesis.co.nz>
> On Mar 24, 2020, at 4:06 PM, Chris Packham <Chris.Packham@alliedtelesis.co.nz> wrote:
>
> On Tue, 2020-03-24 at 15:47 +1100, Michael Ellerman wrote:
>> Chris Packham <Chris.Packham@alliedtelesis.co.nz> writes:
>>> Hi All,
>>>
>>> Just booting up v5.5.11 on a Freescale T2080RDB and I'm seeing the
>>> following mesage.
>>>
>>> kern.warning linuxbox kernel: Argh, can't find dcache properties !
>>> kern.warning linuxbox kernel: Argh, can't find icache properties !
>>>
>>> This was changed from DBG() to pr_warn() in commit 3b9176e9a874
>>> ("powerpc/setup_64: fix -Wempty-body warnings") but the message
>>> seems
>>> to be much older than that. So it's probably been an issue on the
>>> T2080
>>> (and other QorIQ SoCs) for a while.
>>
>> That's an e6500 I think? So 64-bit Book3E.
>>
>
> Yes that's correct.
>
>> You'll be getting the default values, which is 64 bytes so I guess
>> that
>> works in practice.
>>
>>> Looking at the code the t208x doesn't specifiy any of the d-cache-
>>> size/i-cache-size properties. Should I add them to silence the
>>> warning
>>> or switch it to pr_debug()/pr_info()?
>>
>> Yeah ideally you'd add them to the device tree(s) for those boards.
>>
>
> I think the info I need is in the block diagram[0]. I'll whip up
> a patch.
>
> --
> [1] - https://www.nxp.com/products/processors-and-microcontrollers/power-architecture/qoriq-communication-processors/t-series/qoriq-t2080-and-t2081-multicore-communications-processors:T2080
BTW, POWER9 PowerNV would have the same thing.
[ 0.000000][ T0] Setting debug_guardpage_minorder to 1
[ 0.000000][ T0] Reserving 512MB of memory at 128MB for crashkernel (System RAM: 262144MB)
[ 0.000000][ T0] radix-mmu: Page sizes from device-tree:
[ 0.000000][ T0] radix-mmu: Page size shift = 12 AP=0x0
[ 0.000000][ T0] radix-mmu: Page size shift = 16 AP=0x5
[ 0.000000][ T0] radix-mmu: Page size shift = 21 AP=0x1
[ 0.000000][ T0] radix-mmu: Page size shift = 30 AP=0x2
[ 0.000000][ T0] radix-mmu: Activating Kernel Userspace Execution Prevention
[ 0.000000][ T0] radix-mmu: Activating Kernel Userspace Access Prevention
[ 0.000000][ T0] radix-mmu: Mapped 0x0000000000000000-0x0000000001600000 with 2.00 MiB pages (exec)
[ 0.000000][ T0] radix-mmu: Mapped 0x0000000001600000-0x0000000040000000 with 2.00 MiB pages
[ 0.000000][ T0] radix-mmu: Mapped 0x0000000040000000-0x0000002000000000 with 1.00 GiB pages
[ 0.000000][ T0] radix-mmu: Mapped 0x0000200000000000-0x0000202000000000 with 1.00 GiB pages
[ 0.000000][ T0] radix-mmu: Initializing Radix MMU
[ 0.000000][ T0] Linux version 5.6.0-rc7-next-20200324+ (root@ibm-p9wr) (gcc version 8.3.1 20191121 (Red Hat 8.3.1-5) (GCC)) #2 SMP Tue Mar 24 15:52:36 EDT 2020
[ 0.000000][ T0] Argh, can't find dcache properties !
[ 0.000000][ T0] Argh, can't find icache properties !
[ 0.000000][ T0] Found initrd at 0xc000000007850000:0xc00000000ad26142
[ 0.000000][ T0] OPAL: Found memory mapped LPC bus on chip 0
[ 0.000000][ T0] Using PowerNV machine description
[ 0.000000][ T0] printk: bootconsole [udbg0] enabled
[ 0.000000][ T0] CPU maps initialized for 4 threads per core
[ 0.000000][ T0] -----------------------------------------------------
[ 0.000000][ T0] phys_mem_size = 0x4000000000
[ 0.000000][ T0] dcache_bsize = 0x80
[ 0.000000][ T0] icache_bsize = 0x80
[ 0.000000][ T0] cpu_features = 0x0001f86f8f5fb1a7
[ 0.000000][ T0] possible = 0x0003fbffcf5fb1a7
[ 0.000000][ T0] always = 0x0000006f8b5c91a1
[ 0.000000][ T0] cpu_user_features = 0xdc0065c2 0xaee00000
[ 0.000000][ T0] mmu_features = 0xbc006041
[ 0.000000][ T0] firmware_features = 0x0000000010000000
[ 0.000000][ T0] vmalloc start = 0xc008000000000000
[ 0.000000][ T0] IO start = 0xc00a000000000000
[ 0.000000][ T0] vmemmap start = 0xc00c000000000000
[ 0.000000][ T0] -----------------------------------------------------
^ permalink raw reply
* Re: [PATCH v8 04/14] powerpc/vas: Alloc and setup IRQ and trigger port address
From: Haren Myneni @ 2020-03-24 21:06 UTC (permalink / raw)
To: Cédric Le Goater
Cc: mikey, Frederic Bonnard, herbert, npiggin, hch, oohall, sukadev,
linuxppc-dev, ajd
In-Reply-To: <7793f6d4-770b-5fd0-f177-651130c0ff53@kaod.org>
On Tue, 2020-03-24 at 15:48 +0100, Cédric Le Goater wrote:
> On 3/19/20 7:14 AM, Haren Myneni wrote:
> >
> > Alloc IRQ and get trigger port address for each VAS instance. Kernel
> > register this IRQ per VAS instance and sets this port for each send
> > window. NX interrupts the kernel when it sees page fault.
> >
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> > arch/powerpc/platforms/powernv/vas.c | 34 ++++++++++++++++++++++++++++------
> > arch/powerpc/platforms/powernv/vas.h | 2 ++
> > 2 files changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
> > index ed9cc6d..168ab68 100644
> > --- a/arch/powerpc/platforms/powernv/vas.c
> > +++ b/arch/powerpc/platforms/powernv/vas.c
> > @@ -15,6 +15,7 @@
> > #include <linux/of_address.h>
> > #include <linux/of.h>
> > #include <asm/prom.h>
> > +#include <asm/xive.h>
> >
> > #include "vas.h"
> >
> > @@ -25,10 +26,12 @@
> >
> > static int init_vas_instance(struct platform_device *pdev)
> > {
> > - int rc, cpu, vasid;
> > - struct resource *res;
> > - struct vas_instance *vinst;
> > struct device_node *dn = pdev->dev.of_node;
> > + struct vas_instance *vinst;
> > + uint32_t chipid, irq;
> > + struct resource *res;
> > + int rc, cpu, vasid;
> > + uint64_t port;
> >
> > rc = of_property_read_u32(dn, "ibm,vas-id", &vasid);
> > if (rc) {
> > @@ -36,6 +39,12 @@ static int init_vas_instance(struct platform_device *pdev)
> > return -ENODEV;
> > }
> >
> > + rc = of_property_read_u32(dn, "ibm,chip-id", &chipid);
> > + if (rc) {
> > + pr_err("No ibm,chip-id property for %s?\n", pdev->name);
> > + return -ENODEV;
> > + }
> > +
> > if (pdev->num_resources != 4) {
> > pr_err("Unexpected DT configuration for [%s, %d]\n",
> > pdev->name, vasid);
> > @@ -69,9 +78,22 @@ static int init_vas_instance(struct platform_device *pdev)
> >
> > vinst->paste_win_id_shift = 63 - res->end;
> >
> > - pr_devel("Initialized instance [%s, %d], paste_base 0x%llx, "
> > - "paste_win_id_shift 0x%llx\n", pdev->name, vasid,
> > - vinst->paste_base_addr, vinst->paste_win_id_shift);
> > + rc = xive_native_alloc_get_irq_info(chipid, &irq, &port);
> > + if (rc)
> > + return rc;
> > +
> > + vinst->virq = irq_create_mapping(NULL, irq);
> > + if (!vinst->virq) {
> > + pr_err("Inst%d: Unable to map global irq %d\n",
> > + vinst->vas_id, irq);
> > + return -EINVAL;
> > + }
> > +
> > + vinst->irq_port = port;
>
>
> I would prefer something like this :
>
> hwirq = xive_native_alloc_irq_on_chip(chip_id);
>
> vinst->virq = irq_create_mapping(NULL, hwirq);
> if (!vinst->virq) {
> ...
> }
>
> {
> struct irq_data *d = irq_get_irq_data(vinst->virq);
> struct xive_irq_data *xd = irq_data_get_irq_handler_data(d);
>
> vinst->irq_port = xd->trig_page;
> }
>
>
> and dump xive_native_alloc_get_irq_info().
Thanks Cedric, I will remove patch which adds this function.
>
> C.
>
^ permalink raw reply
* Re: [PATCH v3] powerpc/pseries: Handle UE event for memcpy_mcsafe
From: Ganesh @ 2020-03-24 11:31 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: mahesh, santosh
In-Reply-To: <87pnd2xqyr.fsf@mpe.ellerman.id.au>
[-- Attachment #1: Type: text/plain, Size: 3121 bytes --]
On 3/24/20 10:57 AM, Michael Ellerman wrote:
> Ganesh Goudar <ganeshgr@linux.ibm.com> writes:
>> If we hit UE at an instruction with a fixup entry, flag to
>> ignore the event and set nip to continue execution at the
>> fixup entry.
> You don't explain why we would want to do that. Or what the consequences
> are if we *don't* do it.
>
> As such it's unclear if this is an important fix or just a nice-to-have.
We want avoid panic if we hit MCE during memcpy from pmem devices because
the system is still recoverable and should just result -EIO, So we flag it here
to ignore the UE event. I will respin with better commit message.
>> For powernv these changes are already made by
>> commit 895e3dceeb97 ("powerpc/mce: Handle UE event for memcpy_mcsafe")
> We have masses of code that supposedly abstracts the MCE logic. How did
> we end up in the situation where we're having to write the same fix
> twice for different platforms?
What is common between pseries and powernv now is saving the MCE event for deferred
handling and deferred handling. According to me it becomes bit messy to return
disposition(UE RECOVERED) from common code. So what we can have is a common function
which searches the exception table entry and updates nip with fixup address, And call
it from different places for pseries and powernv. If you are ok ill spin next version.
> next
>
> cheers
>
>> Reviewed-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
>> Reviewed-by: Santosh S <santosh@fossix.org>
>> Signed-off-by: Ganesh Goudar <ganeshgr@linux.ibm.com>
>> ---
>> V2: Fixes a trivial checkpatch error in commit msg.
>> V3: Use proper subject prefix.
>> ---
>> arch/powerpc/platforms/pseries/ras.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/powerpc/platforms/pseries/ras.c b/arch/powerpc/platforms/pseries/ras.c
>> index 43710b69e09e..58e2483fbb1a 100644
>> --- a/arch/powerpc/platforms/pseries/ras.c
>> +++ b/arch/powerpc/platforms/pseries/ras.c
>> @@ -10,6 +10,7 @@
>> #include <linux/fs.h>
>> #include <linux/reboot.h>
>> #include <linux/irq_work.h>
>> +#include <linux/extable.h>
>>
>> #include <asm/machdep.h>
>> #include <asm/rtas.h>
>> @@ -505,6 +506,7 @@ static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
>> int initiator = rtas_error_initiator(errp);
>> int severity = rtas_error_severity(errp);
>> u8 error_type, err_sub_type;
>> + const struct exception_table_entry *entry;
>>
>> if (initiator == RTAS_INITIATOR_UNKNOWN)
>> mce_err.initiator = MCE_INITIATOR_UNKNOWN;
>> @@ -558,6 +560,12 @@ static int mce_handle_error(struct pt_regs *regs, struct rtas_error_log *errp)
>> switch (mce_log->error_type) {
>> case MC_ERROR_TYPE_UE:
>> mce_err.error_type = MCE_ERROR_TYPE_UE;
>> + entry = search_kernel_exception_table(regs->nip);
>> + if (entry) {
>> + mce_err.ignore_event = true;
>> + regs->nip = extable_fixup(entry);
>> + disposition = RTAS_DISP_FULLY_RECOVERED;
>> + }
>> switch (err_sub_type) {
>> case MC_ERROR_UE_IFETCH:
>> mce_err.u.ue_error_type = MCE_UE_ERROR_IFETCH;
>> --
>> 2.17.2
[-- Attachment #2: Type: text/html, Size: 4370 bytes --]
^ permalink raw reply
* Re: [PATCH V2 1/3] mm/debug: Add tests validating arch page table helpers for core features
From: Zi Yan @ 2020-03-24 13:29 UTC (permalink / raw)
To: Anshuman Khandual
Cc: Heiko Carstens, linux-mm, Paul Mackerras, H. Peter Anvin,
linux-riscv, Will Deacon, linux-arch, linux-s390, x86,
Mike Rapoport, Christian Borntraeger, Ingo Molnar,
Catalin Marinas, linux-snps-arc, Vasily Gorbik, Borislav Petkov,
Paul Walmsley, Kirill A . Shutemov, Thomas Gleixner,
linux-arm-kernel, Vineet Gupta, linux-kernel, Palmer Dabbelt,
Andrew Morton, linuxppc-dev
In-Reply-To: <1585027375-9997-2-git-send-email-anshuman.khandual@arm.com>
[-- Attachment #1: Type: text/plain, Size: 9961 bytes --]
On 24 Mar 2020, at 1:22, Anshuman Khandual wrote:
> This adds new tests validating arch page table helpers for these following
> core memory features. These tests create and test specific mapping types at
> various page table levels.
>
> 1. SPECIAL mapping
> 2. PROTNONE mapping
> 3. DEVMAP mapping
> 4. SOFTDIRTY mapping
> 5. SWAP mapping
> 6. MIGRATION mapping
> 7. HUGETLB mapping
> 8. THP mapping
>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mike Rapoport <rppt@linux.ibm.com>
> Cc: Vineet Gupta <vgupta@synopsys.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Kirill A. Shutemov <kirill@shutemov.name>
> Cc: Paul Walmsley <paul.walmsley@sifive.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: linux-snps-arc@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-s390@vger.kernel.org
> Cc: linux-riscv@lists.infradead.org
> Cc: x86@kernel.org
> Cc: linux-arch@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Suggested-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> mm/debug_vm_pgtable.c | 291 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 290 insertions(+), 1 deletion(-)
>
> diff --git a/mm/debug_vm_pgtable.c b/mm/debug_vm_pgtable.c
> index 98990a515268..15055a8f6478 100644
> --- a/mm/debug_vm_pgtable.c
> +++ b/mm/debug_vm_pgtable.c
> @@ -289,6 +289,267 @@ static void __init pmd_populate_tests(struct mm_struct *mm, pmd_t *pmdp,
> WARN_ON(pmd_bad(pmd));
> }
>
> +static void __init pte_special_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pte_t pte = pfn_pte(pfn, prot);
> +
> + if (!IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL))
> + return;
> +
> + WARN_ON(!pte_special(pte_mkspecial(pte)));
> +}
> +
> +static void __init pte_protnone_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pte_t pte = pfn_pte(pfn, prot);
> +
> + if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
> + return;
> +
> + WARN_ON(!pte_protnone(pte));
> + WARN_ON(!pte_present(pte));
> +}
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static void __init pmd_protnone_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pmd_t pmd = pfn_pmd(pfn, prot);
> +
> + if (!IS_ENABLED(CONFIG_NUMA_BALANCING))
> + return;
> +
> + WARN_ON(!pmd_protnone(pmd));
> + WARN_ON(!pmd_present(pmd));
> +}
> +#else
> +static void __init pmd_protnone_tests(unsigned long pfn, pgprot_t prot) { }
> +#endif
> +
> +#ifdef CONFIG_ARCH_HAS_PTE_DEVMAP
> +static void __init pte_devmap_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pte_t pte = pfn_pte(pfn, prot);
> +
> + WARN_ON(!pte_devmap(pte_mkdevmap(pte)));
> +}
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static void __init pmd_devmap_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pmd_t pmd = pfn_pmd(pfn, prot);
> +
> + WARN_ON(!pmd_devmap(pmd_mkdevmap(pmd)));
> +}
> +
> +#ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pud_t pud = pfn_pud(pfn, prot);
> +
> + WARN_ON(!pud_devmap(pud_mkdevmap(pud)));
> +}
> +#else
> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot) { }
> +#endif
> +#else
> +static void __init pmd_devmap_tests(unsigned long pfn, pgprot_t prot) { }
> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot) { }
> +#endif
> +#else
> +static void __init pte_devmap_tests(unsigned long pfn, pgprot_t prot) { }
> +static void __init pmd_devmap_tests(unsigned long pfn, pgprot_t prot) { }
> +static void __init pud_devmap_tests(unsigned long pfn, pgprot_t prot) { }
> +#endif
> +
> +static void __init pte_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pte_t pte = pfn_pte(pfn, prot);
> +
> + if (!IS_ENABLED(CONFIG_HAVE_ARCH_SOFT_DIRTY))
> + return;
> +
> + WARN_ON(!pte_soft_dirty(pte_mksoft_dirty(pte)));
> + WARN_ON(pte_soft_dirty(pte_clear_soft_dirty(pte)));
> +}
> +
> +static void __init pte_swap_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pte_t pte = pfn_pte(pfn, prot);
> +
> + if (!IS_ENABLED(CONFIG_HAVE_ARCH_SOFT_DIRTY))
> + return;
> +
> + WARN_ON(!pte_swp_soft_dirty(pte_swp_mksoft_dirty(pte)));
> + WARN_ON(pte_swp_soft_dirty(pte_swp_clear_soft_dirty(pte)));
> +}
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static void __init pmd_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pmd_t pmd = pfn_pmd(pfn, prot);
> +
> + if (!IS_ENABLED(CONFIG_HAVE_ARCH_SOFT_DIRTY))
> + return;
> +
> + WARN_ON(!pmd_soft_dirty(pmd_mksoft_dirty(pmd)));
> + WARN_ON(pmd_soft_dirty(pmd_clear_soft_dirty(pmd)));
> +}
> +
> +static void __init pmd_swap_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pmd_t pmd = pfn_pmd(pfn, prot);
> +
> + if (!IS_ENABLED(CONFIG_HAVE_ARCH_SOFT_DIRTY) ||
> + !IS_ENABLED(CONFIG_ARCH_ENABLE_THP_MIGRATION))
> + return;
> +
> + WARN_ON(!pmd_swp_soft_dirty(pmd_swp_mksoft_dirty(pmd)));
> + WARN_ON(pmd_swp_soft_dirty(pmd_swp_clear_soft_dirty(pmd)));
> +}
> +#else
> +static void __init pmd_soft_dirty_tests(unsigned long pfn, pgprot_t prot) { }
> +static void __init pmd_swap_soft_dirty_tests(unsigned long pfn, pgprot_t prot)
> +{
> +}
> +#endif
> +
> +static void __init pte_swap_tests(unsigned long pfn, pgprot_t prot)
> +{
> + swp_entry_t swp;
> + pte_t pte;
> +
> + pte = pfn_pte(pfn, prot);
> + swp = __pte_to_swp_entry(pte);
> + WARN_ON(!pte_same(pte, __swp_entry_to_pte(swp)));
> +}
> +
> +#ifdef CONFIG_ARCH_ENABLE_THP_MIGRATION
> +static void __init pmd_swap_tests(unsigned long pfn, pgprot_t prot)
> +{
> + swp_entry_t swp;
> + pmd_t pmd;
> +
> + pmd = pfn_pmd(pfn, prot);
> + swp = __pmd_to_swp_entry(pmd);
> + WARN_ON(!pmd_same(pmd, __swp_entry_to_pmd(swp)));
> +}
> +#else
> +static void __init pmd_swap_tests(unsigned long pfn, pgprot_t prot) { }
> +#endif
> +
> +static void __init swap_migration_tests(void)
> +{
> + struct page *page;
> + swp_entry_t swp;
> +
> + if (!IS_ENABLED(CONFIG_MIGRATION))
> + return;
> + /*
> + * swap_migration_tests() requires a dedicated page as it needs to
> + * be locked before creating a migration entry from it. Locking the
> + * page that actually maps kernel text ('start_kernel') can be real
> + * problematic. Lets allocate a dedicated page explicitly for this
> + * purpose that will be freed subsequently.
> + */
> + page = alloc_page(GFP_KERNEL);
> + if (!page) {
> + pr_err("page allocation failed\n");
> + return;
> + }
> +
> + /*
> + * make_migration_entry() expects given page to be
> + * locked, otherwise it stumbles upon a BUG_ON().
> + */
> + __SetPageLocked(page);
> + swp = make_migration_entry(page, 1);
> + WARN_ON(!is_migration_entry(swp));
> + WARN_ON(!is_write_migration_entry(swp));
> +
> + make_migration_entry_read(&swp);
> + WARN_ON(!is_migration_entry(swp));
> + WARN_ON(is_write_migration_entry(swp));
> +
> + swp = make_migration_entry(page, 0);
> + WARN_ON(!is_migration_entry(swp));
> + WARN_ON(is_write_migration_entry(swp));
> + __ClearPageLocked(page);
> + __free_page(page);
> +}
> +
> +#ifdef CONFIG_HUGETLB_PAGE
> +static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot)
> +{
> + struct page *page;
> + pte_t pte;
> +
> + /*
> + * Accessing the page associated with the pfn is safe here,
> + * as it was previously derived from a real kernel symbol.
> + */
> + page = pfn_to_page(pfn);
> + pte = mk_huge_pte(page, prot);
> +
> + WARN_ON(!huge_pte_dirty(huge_pte_mkdirty(pte)));
> + WARN_ON(!huge_pte_write(huge_pte_mkwrite(huge_pte_wrprotect(pte))));
> + WARN_ON(huge_pte_write(huge_pte_wrprotect(huge_pte_mkwrite(pte))));
> +
> +#ifdef CONFIG_ARCH_WANT_GENERAL_HUGETLB
> + pte = pfn_pte(pfn, prot);
> +
> + WARN_ON(!pte_huge(pte_mkhuge(pte)));
> +#endif
> +}
> +#else
> +static void __init hugetlb_basic_tests(unsigned long pfn, pgprot_t prot) { }
> +#endif
> +
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +static void __init pmd_thp_tests(unsigned long pfn, pgprot_t prot)
> +{
> + pmd_t pmd;
> +
> + /*
> + * pmd_trans_huge() and pmd_present() must return positive
> + * after MMU invalidation with pmd_mknotpresent().
> + */
> + pmd = pfn_pmd(pfn, prot);
> + WARN_ON(!pmd_trans_huge(pmd_mkhuge(pmd)));
> +
> +#ifndef __HAVE_ARCH_PMDP_INVALIDATE
> + WARN_ON(!pmd_trans_huge(pmd_mknotpresent(pmd_mkhuge(pmd))));
> + WARN_ON(!pmd_present(pmd_mknotpresent(pmd_mkhuge(pmd))));
> +#endif
I think we need a better comment here, because requiring pmd_trans_huge() and
pmd_present() returning true after pmd_mknotpresent() is not straightforward.
According to Andrea Arcangeli’s email (https://lore.kernel.org/linux-mm/20181017020930.GN30832@redhat.com/),
This behavior is an optimization for transparent huge page.
pmd_trans_huge() must be true if pmd_page() returns you a valid THP to avoid
taking the pmd_lock when others walk over non transhuge pmds (i.e. there are no
THP allocated). Especially when we split a THP, removing the present bit from
the pmd, pmd_trans_huge() still needs to return true. pmd_present() should
be true whenever pmd_trans_huge() returns true.
I think it is also worth either putting Andres’s email or the link to it
in the rst file in your 3rd patch. It is a good documentation for this special
case.
—
Best Regards,
Yan Zi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]
^ permalink raw reply
* [PATCH] powerpc/fsl: Add cache properties for T2080/T2081
From: Chris Packham @ 2020-03-24 21:36 UTC (permalink / raw)
To: mpe, robh+dt, mark.rutland, paulus, benh
Cc: devicetree, Hamish Martin, Chris Packham, linuxppc-dev,
linux-kernel
Add the d-cache/i-cache properties for the T208x SoCs. The L1 cache on
these SoCs is 32KiB and is split into 64 byte blocks (lines).
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi b/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
index 3f745de44284..2ad27e16ac16 100644
--- a/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
+++ b/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
@@ -81,6 +81,10 @@ cpus {
cpu0: PowerPC,e6500@0 {
device_type = "cpu";
reg = <0 1>;
+ d-cache-line-size = <64>;
+ i-cache-line-size = <64>;
+ d-cache-size = <32768>;
+ i-cache-size = <32768>;
clocks = <&clockgen 1 0>;
next-level-cache = <&L2_1>;
fsl,portid-mapping = <0x80000000>;
@@ -88,6 +92,10 @@ cpu0: PowerPC,e6500@0 {
cpu1: PowerPC,e6500@2 {
device_type = "cpu";
reg = <2 3>;
+ d-cache-line-size = <64>;
+ i-cache-line-size = <64>;
+ d-cache-size = <32768>;
+ i-cache-size = <32768>;
clocks = <&clockgen 1 0>;
next-level-cache = <&L2_1>;
fsl,portid-mapping = <0x80000000>;
@@ -95,6 +103,10 @@ cpu1: PowerPC,e6500@2 {
cpu2: PowerPC,e6500@4 {
device_type = "cpu";
reg = <4 5>;
+ d-cache-line-size = <64>;
+ i-cache-line-size = <64>;
+ d-cache-size = <32768>;
+ i-cache-size = <32768>;
clocks = <&clockgen 1 0>;
next-level-cache = <&L2_1>;
fsl,portid-mapping = <0x80000000>;
@@ -102,6 +114,10 @@ cpu2: PowerPC,e6500@4 {
cpu3: PowerPC,e6500@6 {
device_type = "cpu";
reg = <6 7>;
+ d-cache-line-size = <64>;
+ i-cache-line-size = <64>;
+ d-cache-size = <32768>;
+ i-cache-size = <32768>;
clocks = <&clockgen 1 0>;
next-level-cache = <&L2_1>;
fsl,portid-mapping = <0x80000000>;
--
2.25.1
^ permalink raw reply related
* Re: [patch V3 13/20] Documentation: Add lock ordering and nesting documentation
From: Thomas Gleixner @ 2020-03-24 23:13 UTC (permalink / raw)
To: paulmck
Cc: linux-usb, linux-ia64, Peter Zijlstra, linux-pci,
Sebastian Siewior, Oleg Nesterov, Guo Ren, Joel Fernandes,
Vincent Chen, Ingo Molnar, Davidlohr Bueso, linux-acpi,
Brian Cain, Jonathan Corbet, linux-hexagon, Rafael J. Wysocki,
linux-csky, Linus Torvalds, Darren Hart, Zhang Rui, Len Brown,
Fenghua Yu, Arnd Bergmann, linux-pm, linuxppc-dev, Greentime Hu,
Bjorn Helgaas, Kurt Schwemmer, platform-driver-x86, Kalle Valo,
kbuild test robot, Felipe Balbi, Michal Simek, Tony Luck, Nick Hu,
Geoff Levand, netdev, Randy Dunlap, linux-wireless, LKML,
Davidlohr Bueso, Greg Kroah-Hartman, Logan Gunthorpe,
David S. Miller, Andy Shevchenko
In-Reply-To: <20200323025501.GE3199@paulmck-ThinkPad-P72>
Paul,
"Paul E. McKenney" <paulmck@kernel.org> writes:
> On Sat, Mar 21, 2020 at 12:25:57PM +0100, Thomas Gleixner wrote:
> In the normal case where the task sleeps through the entire lock
> acquisition, the sequence of events is as follows:
>
> state = UNINTERRUPTIBLE
> lock()
> block()
> real_state = state
> state = SLEEPONLOCK
>
> lock wakeup
> state = real_state == UNINTERRUPTIBLE
>
> This sequence of events can occur when the task acquires spinlocks
> on its way to sleeping, for example, in a call to wait_event().
>
> The non-lock wakeup can occur when a wakeup races with this wait_event(),
> which can result in the following sequence of events:
>
> state = UNINTERRUPTIBLE
> lock()
> block()
> real_state = state
> state = SLEEPONLOCK
>
> non lock wakeup
> real_state = RUNNING
>
> lock wakeup
> state = real_state == RUNNING
>
> Without this real_state subterfuge, the wakeup might be lost.
I added this with a few modifications which reflect the actual
implementation. Conceptually the same.
> rwsems have grown special-purpose interfaces that allow non-owner release.
> This non-owner release prevents PREEMPT_RT from substituting RT-mutex
> implementations, for example, by defeating priority inheritance.
> After all, if the lock has no owner, whose priority should be boosted?
> As a result, PREEMPT_RT does not currently support rwsem, which in turn
> means that code using it must therefore be disabled until a workable
> solution presents itself.
>
> [ Note: Not as confident as I would like to be in the above. ]
I'm not confident either especially not after looking at the actual
code.
In fact I feel really stupid because the rw_semaphore reader non-owner
restriction on RT simply does not exist anymore and my history biased
memory tricked me.
The first rw_semaphore implementation of RT was simple and restricted
the reader side to a single reader to support PI on both the reader and
the writer side. That obviosuly did not scale well and made mmap_sem
heavy use cases pretty unhappy.
The short interlude with multi-reader boosting turned out to be a failed
experiment - Steven might still disagree though :)
At some point we gave up and I myself (sic!) reimplemented the RT
variant of rw_semaphore with a reader biased mechanism.
The reader never holds the underlying rt_mutex accross the read side
critical section. It merily increments the reader count and drops it on
release.
The only time a reader takes the rt_mutex is when it blocks on a
writer. Writers hold the rt_mutex across the write side critical section
to allow incoming readers to boost them. Once the writer releases the
rw_semaphore it unlocks the rt_mutex which is then handed off to the
readers. They increment the reader count and then drop the rt_mutex
before continuing in the read side critical section.
So while I changed the implementation it did obviously not occur to me
that this also lifted the non-owner release restriction. Nobody else
noticed either. So we kept dragging this along in both memory and
implementation. Both will be fixed now :)
The owner semantics of down/up_read() are only enforced by lockdep. That
applies to both RT and !RT. The up/down_read_non_owner() variants are
just there to tell lockdep about it.
So, I picked up your other suggestions with slight modifications and
adjusted the owner, semaphore and rw_semaphore docs accordingly.
Please have a close look at the patch below (applies on tip core/locking).
Thanks,
tglx, who is searching a brown paperbag
8<----------
Documentation/locking/locktypes.rst | 148 +++++++++++++++++++++++-------------
1 file changed, 98 insertions(+), 50 deletions(-)
--- a/Documentation/locking/locktypes.rst
+++ b/Documentation/locking/locktypes.rst
@@ -67,6 +67,17 @@ Spinning locks implicitly disable preemp
_irqsave/restore() Save and disable / restore interrupt disabled state
=================== ====================================================
+Owner semantics
+===============
+
+The aforementioned lock types except semaphores have strict owner
+semantics:
+
+ The context (task) that acquired the lock must release it.
+
+rw_semaphores have a special interface which allows non-owner release for
+readers.
+
rtmutex
=======
@@ -83,6 +94,51 @@ interrupt handlers and soft interrupts.
and rwlock_t to be implemented via RT-mutexes.
+sempahore
+=========
+
+semaphore is a counting semaphore implementation.
+
+Semaphores are often used for both serialization and waiting, but new use
+cases should instead use separate serialization and wait mechanisms, such
+as mutexes and completions.
+
+sempahores and PREEMPT_RT
+----------------------------
+
+PREEMPT_RT does not change the sempahore implementation. That's impossible
+due to the counting semaphore semantics which have no concept of owners.
+The lack of an owner conflicts with priority inheritance. After all an
+unknown owner cannot be boosted. As a consequence blocking on semaphores
+can be subject to priority inversion.
+
+
+rw_sempahore
+============
+
+rw_semaphore is a multiple readers and single writer lock mechanism.
+
+On non-PREEMPT_RT kernels the implementation is fair, thus preventing
+writer starvation.
+
+rw_semaphore complies by default with the strict owner semantics, but there
+exist special-purpose interfaces that allow non-owner release for readers.
+These work independent of the kernel configuration.
+
+rw_sempahore and PREEMPT_RT
+---------------------------
+
+PREEMPT_RT kernels map rw_sempahore to a separate rt_mutex-based
+implementation, thus changing the fairness:
+
+ Because an rw_sempaphore writer cannot grant its priority to multiple
+ readers, a preempted low-priority reader will continue holding its lock,
+ thus starving even high-priority writers. In contrast, because readers
+ can grant their priority to a writer, a preempted low-priority writer will
+ have its priority boosted until it releases the lock, thus preventing that
+ writer from starving readers.
+
+
raw_spinlock_t and spinlock_t
=============================
@@ -140,7 +196,16 @@ On a PREEMPT_RT enabled kernel spinlock_
kernels leave task state untouched. However, PREEMPT_RT must change
task state if the task blocks during acquisition. Therefore, it saves
the current task state before blocking and the corresponding lock wakeup
- restores it.
+ restores it::
+
+ task->state = TASK_INTERRUPTIBLE
+ lock()
+ block()
+ task->saved_state = task->state
+ task->state = TASK_UNINTERRUPTIBLE
+ schedule()
+ lock wakeup
+ task->state = task->saved_state
Other types of wakeups would normally unconditionally set the task state
to RUNNING, but that does not work here because the task must remain
@@ -148,7 +213,22 @@ On a PREEMPT_RT enabled kernel spinlock_
wakeup attempts to awaken a task blocked waiting for a spinlock, it
instead sets the saved state to RUNNING. Then, when the lock
acquisition completes, the lock wakeup sets the task state to the saved
- state, in this case setting it to RUNNING.
+ state, in this case setting it to RUNNING::
+
+ task->state = TASK_INTERRUPTIBLE
+ lock()
+ block()
+ task->saved_state = task->state
+ task->state = TASK_UNINTERRUPTIBLE
+ schedule()
+ non lock wakeup
+ task->saved_state = TASK_RUNNING
+
+ lock wakeup
+ task->state = task->saved_state
+
+ This ensures that the real wakeup cannot be lost.
+
rwlock_t
========
@@ -228,17 +308,16 @@ while holding normal non-raw spinlocks b
bit spinlocks
-------------
-Bit spinlocks are problematic for PREEMPT_RT as they cannot be easily
-substituted by an RT-mutex based implementation for obvious reasons.
-
-The semantics of bit spinlocks are preserved on PREEMPT_RT kernels and the
-caveats vs. raw_spinlock_t apply.
-
-Some bit spinlocks are substituted by regular spinlock_t for PREEMPT_RT but
-this requires conditional (#ifdef'ed) code changes at the usage site while
-the spinlock_t substitution is simply done by the compiler and the
-conditionals are restricted to header files and core implementation of the
-locking primitives and the usage sites do not require any changes.
+PREEMPT_RT cannot substitute bit spinlocks because a single bit is too
+small to accommodate an RT-mutex. Therefore, the semantics of bit
+spinlocks are preserved on PREEMPT_RT kernels, so that the raw_spinlock_t
+caveats also apply to bit spinlocks.
+
+Some bit spinlocks are replaced with regular spinlock_t for PREEMPT_RT
+using conditional (#ifdef'ed) code changes at the usage site. In contrast,
+usage-site changes are not needed for the spinlock_t substitution.
+Instead, conditionals in header files and the core locking implemementation
+enable the compiler to do the substitution transparently.
Lock type nesting rules
@@ -254,46 +333,15 @@ Lock type nesting rules
- Spinning lock types can nest inside sleeping lock types.
-These rules apply in general independent of CONFIG_PREEMPT_RT.
+These constraints apply both in CONFIG_PREEMPT_RT and otherwise.
-As PREEMPT_RT changes the lock category of spinlock_t and rwlock_t from
-spinning to sleeping this has obviously restrictions how they can nest with
-raw_spinlock_t.
-
-This results in the following nest ordering:
+The fact that PREEMPT_RT changes the lock category of spinlock_t and
+rwlock_t from spinning to sleeping means that they cannot be acquired while
+holding a raw spinlock. This results in the following nesting ordering:
1) Sleeping locks
2) spinlock_t and rwlock_t
3) raw_spinlock_t and bit spinlocks
-Lockdep is aware of these constraints to ensure that they are respected.
-
-
-Owner semantics
-===============
-
-Most lock types in the Linux kernel have strict owner semantics, i.e. the
-context (task) which acquires a lock has to release it.
-
-There are two exceptions:
-
- - semaphores
- - rwsems
-
-semaphores have no owner semantics for historical reason, and as such
-trylock and release operations can be called from any context. They are
-often used for both serialization and waiting purposes. That's generally
-discouraged and should be replaced by separate serialization and wait
-mechanisms, such as mutexes and completions.
-
-rwsems have grown interfaces which allow non owner release for special
-purposes. This usage is problematic on PREEMPT_RT because PREEMPT_RT
-substitutes all locking primitives except semaphores with RT-mutex based
-implementations to provide priority inheritance for all lock types except
-the truly spinning ones. Priority inheritance on ownerless locks is
-obviously impossible.
-
-For now the rwsem non-owner release excludes code which utilizes it from
-being used on PREEMPT_RT enabled kernels. In same cases this can be
-mitigated by disabling portions of the code, in other cases the complete
-functionality has to be disabled until a workable solution has been found.
+Lockdep will complain if these constraints are violated, both in
+CONFIG_PREEMPT_RT and otherwise.
^ permalink raw reply
* Re: [patch V3 13/20] Documentation: Add lock ordering and nesting documentation
From: Paul E. McKenney @ 2020-03-25 0:28 UTC (permalink / raw)
To: Thomas Gleixner
Cc: linux-usb, linux-ia64, Peter Zijlstra, linux-pci,
Sebastian Siewior, Oleg Nesterov, Guo Ren, Joel Fernandes,
Vincent Chen, Ingo Molnar, Davidlohr Bueso, linux-acpi,
Brian Cain, Jonathan Corbet, linux-hexagon, Rafael J. Wysocki,
linux-csky, Linus Torvalds, Darren Hart, Zhang Rui, Len Brown,
Fenghua Yu, Arnd Bergmann, linux-pm, linuxppc-dev, Greentime Hu,
Bjorn Helgaas, Kurt Schwemmer, platform-driver-x86, Kalle Valo,
kbuild test robot, Felipe Balbi, Michal Simek, Tony Luck, Nick Hu,
Geoff Levand, netdev, Randy Dunlap, linux-wireless, LKML,
Davidlohr Bueso, Greg Kroah-Hartman, Logan Gunthorpe,
David S. Miller, Andy Shevchenko
In-Reply-To: <87r1xhz6qp.fsf@nanos.tec.linutronix.de>
On Wed, Mar 25, 2020 at 12:13:34AM +0100, Thomas Gleixner wrote:
> Paul,
>
> "Paul E. McKenney" <paulmck@kernel.org> writes:
> > On Sat, Mar 21, 2020 at 12:25:57PM +0100, Thomas Gleixner wrote:
> > In the normal case where the task sleeps through the entire lock
> > acquisition, the sequence of events is as follows:
> >
> > state = UNINTERRUPTIBLE
> > lock()
> > block()
> > real_state = state
> > state = SLEEPONLOCK
> >
> > lock wakeup
> > state = real_state == UNINTERRUPTIBLE
> >
> > This sequence of events can occur when the task acquires spinlocks
> > on its way to sleeping, for example, in a call to wait_event().
> >
> > The non-lock wakeup can occur when a wakeup races with this wait_event(),
> > which can result in the following sequence of events:
> >
> > state = UNINTERRUPTIBLE
> > lock()
> > block()
> > real_state = state
> > state = SLEEPONLOCK
> >
> > non lock wakeup
> > real_state = RUNNING
> >
> > lock wakeup
> > state = real_state == RUNNING
> >
> > Without this real_state subterfuge, the wakeup might be lost.
>
> I added this with a few modifications which reflect the actual
> implementation. Conceptually the same.
Looks good!
> > rwsems have grown special-purpose interfaces that allow non-owner release.
> > This non-owner release prevents PREEMPT_RT from substituting RT-mutex
> > implementations, for example, by defeating priority inheritance.
> > After all, if the lock has no owner, whose priority should be boosted?
> > As a result, PREEMPT_RT does not currently support rwsem, which in turn
> > means that code using it must therefore be disabled until a workable
> > solution presents itself.
> >
> > [ Note: Not as confident as I would like to be in the above. ]
>
> I'm not confident either especially not after looking at the actual
> code.
>
> In fact I feel really stupid because the rw_semaphore reader non-owner
> restriction on RT simply does not exist anymore and my history biased
> memory tricked me.
I guess I am glad that it is not just me. ;-)
> The first rw_semaphore implementation of RT was simple and restricted
> the reader side to a single reader to support PI on both the reader and
> the writer side. That obviosuly did not scale well and made mmap_sem
> heavy use cases pretty unhappy.
>
> The short interlude with multi-reader boosting turned out to be a failed
> experiment - Steven might still disagree though :)
>
> At some point we gave up and I myself (sic!) reimplemented the RT
> variant of rw_semaphore with a reader biased mechanism.
>
> The reader never holds the underlying rt_mutex accross the read side
> critical section. It merily increments the reader count and drops it on
> release.
>
> The only time a reader takes the rt_mutex is when it blocks on a
> writer. Writers hold the rt_mutex across the write side critical section
> to allow incoming readers to boost them. Once the writer releases the
> rw_semaphore it unlocks the rt_mutex which is then handed off to the
> readers. They increment the reader count and then drop the rt_mutex
> before continuing in the read side critical section.
>
> So while I changed the implementation it did obviously not occur to me
> that this also lifted the non-owner release restriction. Nobody else
> noticed either. So we kept dragging this along in both memory and
> implementation. Both will be fixed now :)
>
> The owner semantics of down/up_read() are only enforced by lockdep. That
> applies to both RT and !RT. The up/down_read_non_owner() variants are
> just there to tell lockdep about it.
>
> So, I picked up your other suggestions with slight modifications and
> adjusted the owner, semaphore and rw_semaphore docs accordingly.
>
> Please have a close look at the patch below (applies on tip core/locking).
>
> Thanks,
>
> tglx, who is searching a brown paperbag
Sorry, used all the ones here over the past few days. :-/
Please see below for a wordsmithing patch to be applied on top of
or merged into the patch in your email.
Thanx, Paul
------------------------------------------------------------------------
commit e38c64ce8db45e2b0a19082f1e1f988c3b25fb81
Author: Paul E. McKenney <paulmck@kernel.org>
Date: Tue Mar 24 17:23:36 2020 -0700
Documentation: Wordsmith lock ordering and nesting documentation
This commit is strictly wordsmithing with no (intended) semantic
changes.
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
diff --git a/Documentation/locking/locktypes.rst b/Documentation/locking/locktypes.rst
index ca7bf84..8eb52e9 100644
--- a/Documentation/locking/locktypes.rst
+++ b/Documentation/locking/locktypes.rst
@@ -94,7 +94,7 @@ interrupt handlers and soft interrupts. This conversion allows spinlock_t
and rwlock_t to be implemented via RT-mutexes.
-sempahore
+semaphore
=========
semaphore is a counting semaphore implementation.
@@ -103,17 +103,17 @@ Semaphores are often used for both serialization and waiting, but new use
cases should instead use separate serialization and wait mechanisms, such
as mutexes and completions.
-sempahores and PREEMPT_RT
+semaphores and PREEMPT_RT
----------------------------
-PREEMPT_RT does not change the sempahore implementation. That's impossible
-due to the counting semaphore semantics which have no concept of owners.
-The lack of an owner conflicts with priority inheritance. After all an
-unknown owner cannot be boosted. As a consequence blocking on semaphores
-can be subject to priority inversion.
+PREEMPT_RT does not change the semaphore implementation because counting
+semaphores have no concept of owners, thus preventing PREEMPT_RT from
+providing priority inheritance for semaphores. After all, an unknown
+owner cannot be boosted. As a consequence, blocking on semaphores can
+result in priority inversion.
-rw_sempahore
+rw_semaphore
============
rw_semaphore is a multiple readers and single writer lock mechanism.
@@ -125,13 +125,13 @@ rw_semaphore complies by default with the strict owner semantics, but there
exist special-purpose interfaces that allow non-owner release for readers.
These work independent of the kernel configuration.
-rw_sempahore and PREEMPT_RT
+rw_semaphore and PREEMPT_RT
---------------------------
-PREEMPT_RT kernels map rw_sempahore to a separate rt_mutex-based
+PREEMPT_RT kernels map rw_semaphore to a separate rt_mutex-based
implementation, thus changing the fairness:
- Because an rw_sempaphore writer cannot grant its priority to multiple
+ Because an rw_semaphore writer cannot grant its priority to multiple
readers, a preempted low-priority reader will continue holding its lock,
thus starving even high-priority writers. In contrast, because readers
can grant their priority to a writer, a preempted low-priority writer will
@@ -158,7 +158,7 @@ critical section is tiny, thus avoiding RT-mutex overhead.
spinlock_t
----------
-The semantics of spinlock_t change with the state of CONFIG_PREEMPT_RT.
+The semantics of spinlock_t change with the state of PREEMPT_RT.
On a non PREEMPT_RT enabled kernel spinlock_t is mapped to raw_spinlock_t
and has exactly the same semantics.
@@ -196,7 +196,7 @@ PREEMPT_RT kernels preserve all other spinlock_t semantics:
kernels leave task state untouched. However, PREEMPT_RT must change
task state if the task blocks during acquisition. Therefore, it saves
the current task state before blocking and the corresponding lock wakeup
- restores it::
+ restores it, as shown below::
task->state = TASK_INTERRUPTIBLE
lock()
@@ -333,7 +333,7 @@ The most basic rules are:
- Spinning lock types can nest inside sleeping lock types.
-These constraints apply both in CONFIG_PREEMPT_RT and otherwise.
+These constraints apply both in PREEMPT_RT and otherwise.
The fact that PREEMPT_RT changes the lock category of spinlock_t and
rwlock_t from spinning to sleeping means that they cannot be acquired while
@@ -344,4 +344,4 @@ holding a raw spinlock. This results in the following nesting ordering:
3) raw_spinlock_t and bit spinlocks
Lockdep will complain if these constraints are violated, both in
-CONFIG_PREEMPT_RT and otherwise.
+PREEMPT_RT and otherwise.
^ permalink raw reply related
* Re: [PATCH 4/4] hugetlbfs: clean up command line processing
From: Mike Kravetz @ 2020-03-25 1:12 UTC (permalink / raw)
To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.),
Mina Almasry
Cc: linux-doc, Catalin Marinas, Dave Hansen, Heiko Carstens, Linux-MM,
Paul Mackerras, sparclinux, linux-riscv, Will Deacon, linux-s390,
Jonathan Corbet, Christian Borntraeger, Ingo Molnar, Albert Ou,
Vasily Gorbik, Paul Walmsley, Thomas Gleixner, linux-arm-kernel,
open list, Palmer Dabbelt, Andrew Morton, linuxppc-dev,
David S.Miller
In-Reply-To: <d067c5d1-89b8-a71b-7b71-a8bbbd613efa@huawei.com>
On 3/23/20 8:47 PM, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
>
>
> On 2020/3/24 8:43, Mina Almasry wrote:
>> On Wed, Mar 18, 2020 at 3:07 PM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>>> +default_hugepagesz - Specify the default huge page size. This parameter can
>>> + only be specified on the command line. No other hugetlb command line
>>> + parameter is associated with default_hugepagesz. Therefore, it can
>>> + appear anywhere on the command line. Valid default huge page size is
>>> + architecture dependent.
>>
>> Maybe specify what happens/should happen in a case like:
>>
>> hugepages=100 default_hugepagesz=1G
>>
>> Does that allocate 100 2MB pages or 100 1G pages? Assuming the default
>> size is 2MB.
That will allocate 100 1G pages as 1G is the default. However, if the
command line reads:
hugepages=100 default_hugepagesz=1G hugepages=200
You will get this warning,
HugeTLB: First hugepages=104857600 kB ignored
>>
>> Also, regarding Randy's comment. It may be nice to keep these docs in
>> one place only, so we don't have to maintain 2 docs in sync.
Let me think about that a bit. We should probably expand the
kernel-parameters doc. Or, we should at least make it more clear. This
doc also talks about the command line parameters and in general goes into
more detail. However, more people read kernel-parameters doc.
>>> +
>>> When multiple huge page sizes are supported, ``/proc/sys/vm/nr_hugepages``
>>> indicates the current number of pre-allocated huge pages of the default size.
>>> Thus, one can use the following command to dynamically allocate/deallocate
>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
>>> index cc85b4f156ca..2b9bf01db2b6 100644
>>> --- a/mm/hugetlb.c
>>> +++ b/mm/hugetlb.c
<snip>
>>> -static int __init hugetlb_nrpages_setup(char *s)
>>> +/*
>>> + * hugepages command line processing
>>> + * hugepages must normally follows a valid hugepagsz specification. If not,
>>
>> 'hugepages must' or 'hugepages normally follows'
>>> + * ignore the hugepages value. hugepages can also be the first huge page
>>> + * command line option in which case it specifies the number of huge pages
>>> + * for the default size.
>>> + */
>>> +static int __init hugepages_setup(char *s)
>>> {
>>> unsigned long *mhp;
>>> static unsigned long *last_mhp;
>>>
>>> if (!parsed_valid_hugepagesz) {
>>> - pr_warn("hugepages = %s preceded by "
>>> + pr_warn("HugeTLB: hugepages = %s preceded by "
>>> "an unsupported hugepagesz, ignoring\n", s);
>>> parsed_valid_hugepagesz = true;
>>> return 1;
>>> }
>>> /*
>>> - * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter yet,
>>> - * so this hugepages= parameter goes to the "default hstate".
>>> + * !hugetlb_max_hstate means we haven't parsed a hugepagesz= parameter
>>> + * yet, so this hugepages= parameter goes to the "default hstate".
>>> */
>>> else if (!hugetlb_max_hstate)
>>> mhp = &default_hstate_max_huge_pages;
>>
>> We don't set parsed_valid_hugepagesz to false at the end of this
>> function, shouldn't we? Parsing a hugepages= value should 'consume' a
>> previously defined hugepagesz= value, so that this is invalid IIUC:
>>
>> hugepagesz=x hugepages=z hugepages=y
>>
> In this case, we'll get:
> "HugeTLB: hugepages= specified twice without interleaving hugepagesz=, ignoring
> hugepages=y"
>
Thanks Longpeng (Mike),
I believe that is the desired message in this situation. The code uses saved
values of mhp (max hstate pointer) to catch this condition. Setting
parsed_valid_hugepagesz to false would result in the message:
HugeTLB: hugepages=y preceded by an unsupported hugepagesz, ignoring
Thanks for all your comments I will incorporate in v2 and send later this
week.
--
Mike Kravetz
^ permalink raw reply
* Re: [PATCH] powerpc/fsl: Add cache properties for T2080/T2081
From: Michael Ellerman @ 2020-03-25 1:59 UTC (permalink / raw)
To: Chris Packham, robh+dt, mark.rutland, paulus, benh
Cc: devicetree, Hamish Martin, linux-kernel, Scott Wood,
Chris Packham, linuxppc-dev
In-Reply-To: <20200324213612.31614-1-chris.packham@alliedtelesis.co.nz>
Chris Packham <chris.packham@alliedtelesis.co.nz> writes:
> Add the d-cache/i-cache properties for the T208x SoCs. The L1 cache on
> these SoCs is 32KiB and is split into 64 byte blocks (lines).
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
> arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
LGTM.
I'll wait a few days to see if Scott wants to ack it.
cheers
> diff --git a/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi b/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> index 3f745de44284..2ad27e16ac16 100644
> --- a/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> +++ b/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> @@ -81,6 +81,10 @@ cpus {
> cpu0: PowerPC,e6500@0 {
> device_type = "cpu";
> reg = <0 1>;
> + d-cache-line-size = <64>;
> + i-cache-line-size = <64>;
> + d-cache-size = <32768>;
> + i-cache-size = <32768>;
> clocks = <&clockgen 1 0>;
> next-level-cache = <&L2_1>;
> fsl,portid-mapping = <0x80000000>;
> @@ -88,6 +92,10 @@ cpu0: PowerPC,e6500@0 {
> cpu1: PowerPC,e6500@2 {
> device_type = "cpu";
> reg = <2 3>;
> + d-cache-line-size = <64>;
> + i-cache-line-size = <64>;
> + d-cache-size = <32768>;
> + i-cache-size = <32768>;
> clocks = <&clockgen 1 0>;
> next-level-cache = <&L2_1>;
> fsl,portid-mapping = <0x80000000>;
> @@ -95,6 +103,10 @@ cpu1: PowerPC,e6500@2 {
> cpu2: PowerPC,e6500@4 {
> device_type = "cpu";
> reg = <4 5>;
> + d-cache-line-size = <64>;
> + i-cache-line-size = <64>;
> + d-cache-size = <32768>;
> + i-cache-size = <32768>;
> clocks = <&clockgen 1 0>;
> next-level-cache = <&L2_1>;
> fsl,portid-mapping = <0x80000000>;
> @@ -102,6 +114,10 @@ cpu2: PowerPC,e6500@4 {
> cpu3: PowerPC,e6500@6 {
> device_type = "cpu";
> reg = <6 7>;
> + d-cache-line-size = <64>;
> + i-cache-line-size = <64>;
> + d-cache-size = <32768>;
> + i-cache-size = <32768>;
> clocks = <&clockgen 1 0>;
> next-level-cache = <&L2_1>;
> fsl,portid-mapping = <0x80000000>;
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH] powerpc/fsl: Add cache properties for T2080/T2081
From: Scott Wood @ 2020-03-25 2:08 UTC (permalink / raw)
To: Michael Ellerman, Chris Packham, robh+dt, mark.rutland, paulus,
benh
Cc: devicetree, Hamish Martin, linuxppc-dev, linux-kernel
In-Reply-To: <877dz9xkhr.fsf@mpe.ellerman.id.au>
On Wed, 2020-03-25 at 12:59 +1100, Michael Ellerman wrote:
> Chris Packham <chris.packham@alliedtelesis.co.nz> writes:
> > Add the d-cache/i-cache properties for the T208x SoCs. The L1 cache on
> > these SoCs is 32KiB and is split into 64 byte blocks (lines).
> >
> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > ---
> > arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
>
> LGTM.
>
> I'll wait a few days to see if Scott wants to ack it.
>
> cheers
>
>
> > diff --git a/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > b/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > index 3f745de44284..2ad27e16ac16 100644
> > --- a/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > +++ b/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > @@ -81,6 +81,10 @@ cpus {
> > cpu0: PowerPC,e6500@0 {
> > device_type = "cpu";
> > reg = <0 1>;
> > + d-cache-line-size = <64>;
> > + i-cache-line-size = <64>;
> > + d-cache-size = <32768>;
> > + i-cache-size = <32768>;
> > clocks = <&clockgen 1 0>;
> > next-level-cache = <&L2_1>;
> > fsl,portid-mapping = <0x80000000>;
U-Boot should be setting d/i-cache-size and d/i-cache-block-size -- are you
using something else?
The line size is the same as the block size so we don't need a separate d/i-
cache-line-size.
-Scott
^ permalink raw reply
* Re: [PATCH] powerpc/fsl: Add cache properties for T2080/T2081
From: Chris Packham @ 2020-03-25 2:38 UTC (permalink / raw)
To: mark.rutland@arm.com, oss@buserror.net, mpe@ellerman.id.au,
paulus@samba.org, robh+dt@kernel.org, benh@kernel.crashing.org
Cc: devicetree@vger.kernel.org, Hamish Martin,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <81c68751cb89bbff13a09467b94530a555d69552.camel@buserror.net>
On Tue, 2020-03-24 at 21:08 -0500, Scott Wood wrote:
> On Wed, 2020-03-25 at 12:59 +1100, Michael Ellerman wrote:
> > Chris Packham <chris.packham@alliedtelesis.co.nz> writes:
> > > Add the d-cache/i-cache properties for the T208x SoCs. The L1
> > > cache on
> > > these SoCs is 32KiB and is split into 64 byte blocks (lines).
> > >
> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > > ---
> > > arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi | 16 ++++++++++++++++
> > > 1 file changed, 16 insertions(+)
> >
> > LGTM.
> >
> > I'll wait a few days to see if Scott wants to ack it.
> >
> > cheers
> >
> >
> > > diff --git a/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > > b/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > > index 3f745de44284..2ad27e16ac16 100644
> > > --- a/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > > +++ b/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > > @@ -81,6 +81,10 @@ cpus {
> > > cpu0: PowerPC,e6500@0 {
> > > device_type = "cpu";
> > > reg = <0 1>;
> > > + d-cache-line-size = <64>;
> > > + i-cache-line-size = <64>;
> > > + d-cache-size = <32768>;
> > > + i-cache-size = <32768>;
> > > clocks = <&clockgen 1 0>;
> > > next-level-cache = <&L2_1>;
> > > fsl,portid-mapping = <0x80000000>;
>
> U-Boot should be setting d/i-cache-size and d/i-cache-block-size --
> are you
> using something else?
Nope it is u-boot specifically
U-Boot 2017.01-rc3-dirty
I'm pretty sure the '-dirty' is just a change to use a different cross-
compiler but I can't confirm and I'm a little hesitant to try updating
as I've only got remote access to the board right now.
>
> The line size is the same as the block size so we don't need a
> separate d/i-
> cache-line-size.
>
I'm not sure that'll work looking at the code[1]. It has logic to set
bsizep to lsizep if no block size is set but not the other way round.
Looking at the spec from devicetree.org this actually seems wrong.
Perhaps that is the real source of the error.
--
[1] - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/setup_64.c#n510
^ permalink raw reply
* Re: [PATCH] powerpc/fsl: Add cache properties for T2080/T2081
From: Chris Packham @ 2020-03-25 2:50 UTC (permalink / raw)
To: mark.rutland@arm.com, oss@buserror.net, mpe@ellerman.id.au,
paulus@samba.org, robh+dt@kernel.org, benh@kernel.crashing.org
Cc: devicetree@vger.kernel.org, Hamish Martin,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <ae2930cdc30779ec0c6183e73849b47dcf5d57b0.camel@alliedtelesis.co.nz>
On Wed, 2020-03-25 at 15:38 +1300, Chris Packham wrote:
> On Tue, 2020-03-24 at 21:08 -0500, Scott Wood wrote:
> > On Wed, 2020-03-25 at 12:59 +1100, Michael Ellerman wrote:
> > > Chris Packham <chris.packham@alliedtelesis.co.nz> writes:
> > > > Add the d-cache/i-cache properties for the T208x SoCs. The L1
> > > > cache on
> > > > these SoCs is 32KiB and is split into 64 byte blocks (lines).
> > > >
> > > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz
> > > > >
> > > > ---
> > > > arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi | 16
> > > > ++++++++++++++++
> > > > 1 file changed, 16 insertions(+)
> > >
> > > LGTM.
> > >
> > > I'll wait a few days to see if Scott wants to ack it.
> > >
> > > cheers
> > >
> > >
> > > > diff --git a/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > > > b/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > > > index 3f745de44284..2ad27e16ac16 100644
> > > > --- a/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > > > +++ b/arch/powerpc/boot/dts/fsl/t208xsi-pre.dtsi
> > > > @@ -81,6 +81,10 @@ cpus {
> > > > cpu0: PowerPC,e6500@0 {
> > > > device_type = "cpu";
> > > > reg = <0 1>;
> > > > + d-cache-line-size = <64>;
> > > > + i-cache-line-size = <64>;
> > > > + d-cache-size = <32768>;
> > > > + i-cache-size = <32768>;
> > > > clocks = <&clockgen 1 0>;
> > > > next-level-cache = <&L2_1>;
> > > > fsl,portid-mapping = <0x80000000>;
> >
> > U-Boot should be setting d/i-cache-size and d/i-cache-block-size --
> > are you
> > using something else?
>
> Nope it is u-boot specifically
>
> U-Boot 2017.01-rc3-dirty
>
> I'm pretty sure the '-dirty' is just a change to use a different
> cross-
> compiler but I can't confirm and I'm a little hesitant to try
> updating
> as I've only got remote access to the board right now.
>
> >
> > The line size is the same as the block size so we don't need a
> > separate d/i-
> > cache-line-size.
> >
>
> I'm not sure that'll work looking at the code[1]. It has logic to set
> bsizep to lsizep if no block size is set but not the other way round.
> Looking at the spec from devicetree.org this actually seems wrong.
> Perhaps that is the real source of the error.
Sure enough without my change
# ls /sys/firmware/devicetree/base/cpus/PowerPC,e6500@0/
bus-frequency d-cache-size name
cache-stash-id device_type next-level-cache
clock-frequency enable-method phandle
clocks fsl,portid-mapping reg
cpu-release-addr i-cache-block-size status
d-cache-block-size i-cache-sets timebase-frequency
d-cache-sets i-cache-size
So it's the lack of handling the optional line-size. Different patch
incomming.
>
> --
> [1] -
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/kernel/setup_64.c#n510
>
>
^ permalink raw reply
* Re: [RFC PATCH 0/3] Use per-CPU temporary mappings for patching
From: Andrew Donnellan @ 2020-03-25 2:51 UTC (permalink / raw)
To: Christopher M. Riedl, linuxppc-dev
In-Reply-To: <20200323045205.20314-1-cmr@informatik.wtf>
On 23/3/20 3:52 pm, Christopher M. Riedl wrote:
> When compiled with CONFIG_STRICT_KERNEL_RWX, the kernel must create
> temporary mappings when patching itself. These mappings temporarily
> override the strict RWX text protections to permit a write. Currently,
> powerpc allocates a per-CPU VM area for patching. Patching occurs as
> follows:
>
> 1. Map page of text to be patched to per-CPU VM area w/
> PAGE_KERNEL protection
> 2. Patch text
> 3. Remove the temporary mapping
>
> While the VM area is per-CPU, the mapping is actually inserted into the
> kernel page tables. Presumably, this could allow another CPU to access
> the normally write-protected text - either malicously or accidentally -
> via this same mapping if the address of the VM area is known. Ideally,
> the mapping should be kept local to the CPU doing the patching (or any
> other sensitive operations requiring temporarily overriding memory
> protections) [0].
>
> x86 introduced "temporary mm" structs which allow the creation of
> mappings local to a particular CPU [1]. This series intends to bring the
> notion of a temporary mm to powerpc and harden powerpc by using such a
> mapping for patching a kernel with strict RWX permissions.
>
> The first patch introduces the temporary mm struct and API for powerpc
> along with a new function to retrieve a current hw breakpoint.
>
> The second patch uses the `poking_init` init hook added by the x86
> patches to initialize a temporary mm and patching address. The patching
> address is randomized between 0 and DEFAULT_MAP_WINDOW-PAGE_SIZE. The
> upper limit is necessary due to how the hash MMU operates - by default
> the space above DEFAULT_MAP_WINDOW is not available. For now, both hash
> and radix randomize inside this range. The number of possible random
> addresses is dependent on PAGE_SIZE and limited by DEFAULT_MAP_WINDOW.
>
> Bits of entropy with 64K page size on BOOK3S_64:
>
> bits-o-entropy = log2(DEFAULT_MAP_WINDOW_USER64 / PAGE_SIZE)
>
> PAGE_SIZE=64K, DEFAULT_MAP_WINDOW_USER64=128TB
> bits-o-entropy = log2(128TB / 64K)
> bits-o-entropy = 31
>
> Currently, randomization occurs only once during initialization at boot.
>
> The third patch replaces the VM area with the temporary mm in the
> patching code. The page for patching has to be mapped PAGE_SHARED with
> the hash MMU since hash prevents the kernel from accessing userspace
> pages with PAGE_PRIVILEGED bit set. There is on-going work on my side to
> explore if this is actually necessary in the hash codepath.
>
> Testing so far is limited to booting on QEMU (power8 and power9 targets)
> and a POWER8 VM along with setting some simple xmon breakpoints (which
> makes use of code-patching). A POC lkdtm test is in-progress to actually
> exploit the existing vulnerability (ie. the mapping during patching is
> exposed in kernel page tables and accessible by other CPUS) - this will
> accompany a future v1 of this series.
>
> [0]: https://github.com/linuxppc/issues/issues/224
> [1]: https://lore.kernel.org/kernel-hardening/20190426232303.28381-1-nadav.amit@gmail.com/
>
> Christopher M. Riedl (3):
> powerpc/mm: Introduce temporary mm
> powerpc/lib: Initialize a temporary mm for code patching
> powerpc/lib: Use a temporary mm for code patching
>
> arch/powerpc/include/asm/debug.h | 1 +
> arch/powerpc/include/asm/mmu_context.h | 56 +++++++++-
> arch/powerpc/kernel/process.c | 5 +
> arch/powerpc/lib/code-patching.c | 140 ++++++++++++++-----------
> 4 files changed, 137 insertions(+), 65 deletions(-)
This series causes a build failure with ppc64e_defconfig
https://openpower.xyz/job/snowpatch/job/snowpatch-linux-sparse/16478//artifact/linux/report.txt
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
^ permalink raw reply
* Re: [PATCH 1/4] hugetlbfs: add arch_hugetlb_valid_size
From: Longpeng (Mike, Cloud Infrastructure Service Product Dept.) @ 2020-03-25 2:58 UTC (permalink / raw)
To: Mike Kravetz, Dave Hansen, linux-mm, linux-kernel,
linux-arm-kernel, linuxppc-dev, linux-riscv, linux-s390,
sparclinux, linux-doc
Cc: Albert Ou, Andrew Morton, Vasily Gorbik, Jonathan Corbet,
Catalin Marinas, Dave Hansen, Heiko Carstens,
Christian Borntraeger, Ingo Molnar, Palmer Dabbelt, Paul Walmsley,
Paul Mackerras, Thomas Gleixner, Will Deacon, David S.Miller
In-Reply-To: <5aceea6a-8dc0-a44b-80c6-94511b5c75ca@oracle.com>
On 2020/3/19 6:52, Mike Kravetz wrote:
> On 3/18/20 3:15 PM, Dave Hansen wrote:
>> Hi Mike,
>>
>> The series looks like a great idea to me. One nit on the x86 bits,
>> though...
>>
>>> diff --git a/arch/x86/mm/hugetlbpage.c b/arch/x86/mm/hugetlbpage.c
>>> index 5bfd5aef5378..51e6208fdeec 100644
>>> --- a/arch/x86/mm/hugetlbpage.c
>>> +++ b/arch/x86/mm/hugetlbpage.c
>>> @@ -181,16 +181,25 @@ hugetlb_get_unmapped_area(struct file *file, unsigned long addr,
>>> #endif /* CONFIG_HUGETLB_PAGE */
>>>
>>> #ifdef CONFIG_X86_64
>>> +bool __init arch_hugetlb_valid_size(unsigned long long size)
>>> +{
>>> + if (size == PMD_SIZE)
>>> + return true;
>>> + else if (size == PUD_SIZE && boot_cpu_has(X86_FEATURE_GBPAGES))
>>> + return true;
>>> + else
>>> + return false;
>>> +}
>>
>> I'm pretty sure it's possible to have a system without 2M/PMD page
>> support. We even have a handy-dandy comment about it in
>> arch/x86/include/asm/required-features.h:
>>
>> #ifdef CONFIG_X86_64
>> #ifdef CONFIG_PARAVIRT
>> /* Paravirtualized systems may not have PSE or PGE available */
>> #define NEED_PSE 0
>> ...
>>
>> I *think* you need an X86_FEATURE_PSE check here to be totally correct.
>>
>> if (size == PMD_SIZE && cpu_feature_enabled(X86_FEATURE_PSE))
>> return true;
>>
>> BTW, I prefer cpu_feature_enabled() to boot_cpu_has() because it
>> includes disabled-features checking. I don't think any of it matters
>> for these specific features, but I generally prefer it on principle.
>
> Sounds good. I'll incorporate those changes into a v2, unless someone
> else with has a different opinion.
>
> BTW, this patch should not really change the way the code works today.
> It is mostly a movement of code. Unless I am missing something, the
> existing code will always allow setup of PMD_SIZE hugetlb pages.
>
Hi Mike,
Inspired by Dave's opinion, it seems the x86-specific hugepages_supported should
also need to use cpu_feature_enabled instead.
Also, I wonder if the hugepages_supported is correct ? There're two arch
specific hugepages_supported:
x86:
#define hugepages_supported() boot_cpu_has(X86_FEATURE_PSE)
and
s390:
#define hugepages_supported() (MACHINE_HAS_EDAT1)
Is it possible that x86 has X86_FEATURE_GBPAGES but hasn't X86_FEATURE_GBPAGES
or s390 has MACHINE_HAS_EDAT2 but hasn't MACHINE_HAS_EDAT1 ?
---
Regards,
Longpeng(Mike)
^ permalink raw reply
* Re: [PATCH v8 06/14] powerpc/vas: Setup thread IRQ handler per VAS instance
From: Haren Myneni @ 2020-03-25 2:58 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: mikey, ajd, hch, oohall, sukadev, linuxppc-dev, herbert
In-Reply-To: <1584926621.6b1zd5e2im.astroid@bobo.none>
On Mon, 2020-03-23 at 12:23 +1000, Nicholas Piggin wrote:
> Haren Myneni's on March 19, 2020 4:15 pm:
> >
> > Setup thread IRQ handler per each VAS instance. When NX sees a fault
> > on CRB, kernel gets an interrupt and vas_fault_handler will be
> > executed to process fault CRBs. Read all valid CRBs from fault FIFO,
> > determine the corresponding send window from CRB and process fault
> > requests.
>
> Perhaps some more overview/why.
>
> "If NX encounters a translation error when accessing the CRB or one
> of addresses in the request, it raises an interrupt on the CPU to
> handle the fault.
>
>
> >
> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> > ---
> > arch/powerpc/platforms/powernv/vas-fault.c | 90 +++++++++++++++++++++++++++++
> > arch/powerpc/platforms/powernv/vas-window.c | 60 +++++++++++++++++++
> > arch/powerpc/platforms/powernv/vas.c | 49 +++++++++++++++-
> > arch/powerpc/platforms/powernv/vas.h | 6 ++
> > 4 files changed, 204 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/platforms/powernv/vas-fault.c b/arch/powerpc/platforms/powernv/vas-fault.c
> > index 4044998..1c6d5cc 100644
> > --- a/arch/powerpc/platforms/powernv/vas-fault.c
> > +++ b/arch/powerpc/platforms/powernv/vas-fault.c
> > @@ -11,6 +11,7 @@
> > #include <linux/slab.h>
> > #include <linux/uaccess.h>
> > #include <linux/kthread.h>
> > +#include <linux/mmu_context.h>
> > #include <asm/icswx.h>
> >
> > #include "vas.h"
> > @@ -25,6 +26,95 @@
> > #define VAS_FAULT_WIN_FIFO_SIZE (4 << 20)
> >
> > /*
> > + * Process valid CRBs in fault FIFO.
> > + */
> > +irqreturn_t vas_fault_thread_fn(int irq, void *data)
>
> Are page faults the only reason why VAS would raise this interrupt? Is
> NX really the only possible user of this, so you can have NX specifics
> in here?
Yes, When NX sees page faults, it generates interrupts on specific VAS
instance. Right now NX is the only user. So trying to make as
generalized as possible.
>
> > +{
> > + struct vas_instance *vinst = data;
> > + struct coprocessor_request_block *crb, *entry;
> > + struct coprocessor_request_block buf;
> > + struct vas_window *window;
> > + unsigned long flags;
> > + void *fifo;
> > +
> > + crb = &buf;
>
> The below comment could just be moved to replace the one at the top of the
> function. Can you explain slightly more about how the faults work, and
> be more clear about what the coprocessor does versus what the host does? The
> use of VAS and NX is a bit confusing too. VAS doesn't interrupt with
> page faults, does it? NX has the page fault(s), and it requests VAS to
> interrupt the host?
NX is the one who raises interrupt on specific VAS port that is
registered with fault window. I will make it clear in the comment.
>
> > +
> > + /*
> > + * VAS can interrupt with multiple page faults. So process all
> > + * valid CRBs within fault FIFO until reaches invalid CRB.
>
> When NX encounters a fault accessing a memory address for a particular
> CRB, it updates the nx_fault_stamp field in the CRB (to what?), and
> copies the CRB to the fault FIFO memory, then raises an interrupt on the
> CPU (memory ordering on the store and load sides are provided how?). NX
> can store multiple faults into the FIFO per interrupt (does it proceed
> asynchronously after the interrupt? what's the stopping condition?).
User space fills CRB and sends request (CRB). NX processes the request
and update CSB (in CRB struct). If NX sees any page fault either on
request buffers or csb address, updates nx_fault_stamp struct (in user
space CRB) and pastes CRB in fault_fifo. Then raises interrupt on port
defined in fault_window (which is per VAS instance).
NX can raise single interrupt for multiple faults and can paste in fault
FIFO. But OS and NX use credits to control fault FIFO.
Initially FIFO_SIZE/CRB_SIZE credits are available for fault window. For
example, When NX pastes CRB in fault fifo, credits will be reduced by 1.
It can continue paste CRBs in FIFO until credits reached to 0.
When OS handles the fault CRB, increments index in FIFO and returns the
credit so that NX knows one more CRB slot is available.
struct coprocessor_request_block {
__be32 ccw;
__be32 flags;
__be64 csb_addr;
struct data_descriptor_entry source;
struct data_descriptor_entry target;
struct coprocessor_completion_block ccb;
union {
struct nx_fault_stamp nx;
u8 reserved[16];
} stamp;
u8 reserved[32];
struct coprocessor_status_block csb;
} __packed __aligned(CRB_ALIGN);
>
> When the CPU takes this interrupt, it reads the faulting CRBs from the
> FIFO and processes them in order until it reaches an invalid entry, FIFO
> empty (memory ordering how?). After each FIFO entry is processed, store
> to mark them as invalid. (How does NX resume after this?)
NX should do atomic copy of CRB in fault FIFO. But we had barrier (as
part of spin_unlock()) for the safe side which is suggested by HW team.
NX stops pasting CRBs if credits are not available and start when credit
is returned by OS after handling fault.
>
> How is the fault actually even "handled" here? Nothing seems to be
> actually done for them.
>
> > + * NX updates nx_fault_stamp in CRB and pastes in fault FIFO.
> > + * kernel retrives send window from parition send window ID
> > + * (pswid) in nx_fault_stamp. So pswid should be valid and
> > + * ccw[0] (in be) should be zero since this bit is reserved.
> > + * If user space touches this bit, NX returns with "CRB format
> > + * error".
> > + *
> > + * After reading CRB entry, invalidate it with pswid (set
> > + * 0xffffffff) and ccw[0] (set to 1).
>
> Al this is very busy and hard to decipher unambiguously. It should read
> more like a spec, a precise sequence of things happening.
Sure, will make it clear
>
> > + *
> > + * In case kernel receives another interrupt with different page
> > + * fault, CRBs are already processed by the previous handling. So
> > + * will be returned from this function when it sees invalid CRB.
> > + */
>
> Ambiguous at best. Assuming the NX continues running asynchronously and
> it's a usual kind of FIFO, I assume this means if the kernel gets
> another interrupt for a page fault corresponding to a FIFO entry that
> has already been processed by this fault.
>
>
> > + do {
>
> Can you make this 'while (true)' or 'for (;;)' so you don't need to go
> to the bottom to see it's an infinite loop.
>
> > + mutex_lock(&vinst->mutex);
>
> What does this protect? Threaded handlers don't run concurrently for the
> same request_threaded_irq?
We can remove this mutex_lock.
>
> > +
> > + spin_lock_irqsave(&vinst->fault_lock, flags);
> > + /*
> > + * Advance the fault fifo pointer to next CRB.
>
> The code below the comment isn't advancing the fault fifo pointer, it's
> grabbing the current one. The pointer (fault_crbs) is advanced later.
> You presumabl don't want to advance over an invalid entry.
Advancing to next entry which is invalid means start processing from
this entry in the next fault.
>
> > + * Use CRB_SIZE rather than sizeof(*crb) since the latter is
> > + * aligned to CRB_ALIGN (256) but the CRB written to by VAS is
> > + * only CRB_SIZE in len.
> > + */
> > + fifo = vinst->fault_fifo + (vinst->fault_crbs * CRB_SIZE);
> > + entry = fifo;
>
> Don't think you should really do this. It may be harmless in this case,
> but the compiler expects the type to be aligned. Make it another type,
> like coprocessor_fault_block or something?
Can add like "entry = (struct coprocessor_request_block *)fifo;
>
> > +
> > + if ((entry->stamp.nx.pswid == cpu_to_be32(FIFO_INVALID_ENTRY))
> > + || (entry->ccw & cpu_to_be32(CCW0_INVALID))) {
> > + atomic_set(&vinst->faults_in_progress, 0);
> > + spin_unlock_irqrestore(&vinst->fault_lock, flags);
>
> So what does the fault_lock protect? The only data it protects is
> faults_in_progress (vs the hard interrupt handler), which doesn't
> achieve anything by itself, so I guess it also prevents the hard irq
> handler from returning until the handler here has checked that the
> fault FIFO is empty then returns IRQ_HANDLED? That seems fine (so long
> as memory ordering details are okay), but it should be documented
> that way.
In the case of using default_handler, wakes up thread if it is not in
progress and checks whether interrupts are handled with in some
duration. If un_handled interrupts reached 99000, display bad_irq trace
and disables IRQ. In our case we do not have one fault per interrupt.
We ran some test case which continuously generates NX faults, the
handler thread is busy processing fault CRBs in FIFO and the later
interrupts are not handled.
So added own handler which checks whether fault_thread is in progress.
If so returned IRQ_HANDLED. fault_lock is used to check valid entry
section and check faults_in_progress in handler.
Added comment in vas_fault_handler().
>
> Also why is the hard handler in a different file? Makes it harder to
> see how this works at a glance.
OK, Will change. I thought IRQ handler per VAS is added in vas.c since
it has VAS initialization and vas_fault.c is only for fault handling.
>
> faults_in_progress does not have to be atomic because it's always
> accessed under the lock. And IMO it should have a better name. If the
> NX can be causing more faults as we go, it really doesn't indicate
> anything about faults. It's whether or not the threaded handler is
> currently woken and processing faults.
Used atomic since not using spin_lock/unlock for failing case when
window (from CRB) is not valid.
Sure, How about fault_thread_in_progress? As it is long name, used
faults_in_progress.
>
> > + mutex_unlock(&vinst->mutex);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + spin_unlock_irqrestore(&vinst->fault_lock, flags);
> > + vinst->fault_crbs++;
> > + if (vinst->fault_crbs == (vinst->fault_fifo_size / CRB_SIZE))
> > + vinst->fault_crbs = 0;
> > +
> > + memcpy(crb, fifo, CRB_SIZE);
> > + entry->stamp.nx.pswid = cpu_to_be32(FIFO_INVALID_ENTRY);
> > + entry->ccw |= cpu_to_be32(CCW0_INVALID);
> > + mutex_unlock(&vinst->mutex);
> > +
> > + pr_devel("VAS[%d] fault_fifo %p, fifo %p, fault_crbs %d\n",
> > + vinst->vas_id, vinst->fault_fifo, fifo,
> > + vinst->fault_crbs);
> > +
> > + window = vas_pswid_to_window(vinst,
> > + be32_to_cpu(crb->stamp.nx.pswid));
> > +
> > + if (IS_ERR(window)) {
> > + /*
> > + * We got an interrupt about a specific send
> > + * window but we can't find that window and we can't
> > + * even clean it up (return credit).
> > + * But we should not get here.
> > + */
> > + pr_err("VAS[%d] fault_fifo %p, fifo %p, pswid 0x%x, fault_crbs %d bad CRB?\n",
> > + vinst->vas_id, vinst->fault_fifo, fifo,
> > + be32_to_cpu(crb->stamp.nx.pswid),
> > + vinst->fault_crbs);
> > +
> > + WARN_ON_ONCE(1);
> > + atomic_set(&vinst->faults_in_progress, 0);
> > + return IRQ_HANDLED;
>
> Shouldn't get here but you have a handler for it, so it should try to
> be graceful. Keep processing the rest of the FIFO until it's empty
> otherwise you have a missed wakeup here? Probably less code too, just
> delete the last 2 lines.
Derive window address from pswid which is pasted by NX. So if window
address is not valid means a bug, we should not be reached this. If
getting this failure, printed data in FIFO for around 10 CRBs. Not sure
whether proceeding with this failure. We may end up receiving lots of
messages on the console if we see similar failures in later CRBs.
Thinking may be disable IRQ so that kernel will not receive any
interrupts. I have not tested this case. I can change it to continue now
and Can I add disable IRQ as TODO?
Thanks for your detailed review.
>
> Thanks,
> Nick
>
> > + }
> > +
> > + } while (true);
> > +}
> > +
> > +/*
> > * Fault window is opened per VAS instance. NX pastes fault CRB in fault
> > * FIFO upon page faults.
> > */
> > diff --git a/arch/powerpc/platforms/powernv/vas-window.c b/arch/powerpc/platforms/powernv/vas-window.c
> > index 1783fa9..1f31c18 100644
> > --- a/arch/powerpc/platforms/powernv/vas-window.c
> > +++ b/arch/powerpc/platforms/powernv/vas-window.c
> > @@ -1040,6 +1040,15 @@ struct vas_window *vas_tx_win_open(int vasid, enum vas_cop_type cop,
> > }
> > } else {
> > /*
> > + * Interrupt hanlder or fault window setup failed. Means
> > + * NX can not generate fault for page fault. So not
> > + * opening for user space tx window.
> > + */
> > + if (!vinst->virq) {
> > + rc = -ENODEV;
> > + goto free_window;
> > + }
> > + /*
> > * A user mapping must ensure that context switch issues
> > * CP_ABORT for this thread.
> > */
> > @@ -1254,3 +1263,54 @@ int vas_win_close(struct vas_window *window)
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(vas_win_close);
> > +
> > +struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
> > + uint32_t pswid)
> > +{
> > + struct vas_window *window;
> > + int winid;
> > +
> > + if (!pswid) {
> > + pr_devel("%s: called for pswid 0!\n", __func__);
> > + return ERR_PTR(-ESRCH);
> > + }
> > +
> > + decode_pswid(pswid, NULL, &winid);
> > +
> > + if (winid >= VAS_WINDOWS_PER_CHIP)
> > + return ERR_PTR(-ESRCH);
> > +
> > + /*
> > + * If application closes the window before the hardware
> > + * returns the fault CRB, we should wait in vas_win_close()
> > + * for the pending requests. so the window must be active
> > + * and the process alive.
> > + *
> > + * If its a kernel process, we should not get any faults and
> > + * should not get here.
> > + */
> > + window = vinst->windows[winid];
> > +
> > + if (!window) {
> > + pr_err("PSWID decode: Could not find window for winid %d pswid %d vinst 0x%p\n",
> > + winid, pswid, vinst);
> > + return NULL;
> > + }
> > +
> > + /*
> > + * Do some sanity checks on the decoded window. Window should be
> > + * NX GZIP user send window. FTW windows should not incur faults
> > + * since their CRBs are ignored (not queued on FIFO or processed
> > + * by NX).
> > + */
> > + if (!window->tx_win || !window->user_win || !window->nx_win ||
> > + window->cop == VAS_COP_TYPE_FAULT ||
> > + window->cop == VAS_COP_TYPE_FTW) {
> > + pr_err("PSWID decode: id %d, tx %d, user %d, nx %d, cop %d\n",
> > + winid, window->tx_win, window->user_win,
> > + window->nx_win, window->cop);
> > + WARN_ON(1);
> > + }
> > +
> > + return window;
> > +}
> > diff --git a/arch/powerpc/platforms/powernv/vas.c b/arch/powerpc/platforms/powernv/vas.c
> > index 557c8e4..3d9ba58 100644
> > --- a/arch/powerpc/platforms/powernv/vas.c
> > +++ b/arch/powerpc/platforms/powernv/vas.c
> > @@ -14,6 +14,8 @@
> > #include <linux/of_platform.h>
> > #include <linux/of_address.h>
> > #include <linux/of.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/interrupt.h>
> > #include <asm/prom.h>
> > #include <asm/xive.h>
> >
> > @@ -24,9 +26,53 @@
> >
> > static DEFINE_PER_CPU(int, cpu_vas_id);
> >
> > +static irqreturn_t vas_fault_handler(int irq, void *dev_id)
> > +{
> > + struct vas_instance *vinst = dev_id;
> > + irqreturn_t ret = IRQ_WAKE_THREAD;
> > + unsigned long flags;
> > +
> > + /*
> > + * NX can generate an interrupt for multiple faults. So the
> > + * fault handler thread process all CRBs until finds invalid
> > + * entry. In case if NX sees continuous faults, it is possible
> > + * that the thread function entered with the first interrupt
> > + * can execute and process all valid CRBs.
> > + * So wake up thread only if the fault thread is not in progress.
> > + */
> > + spin_lock_irqsave(&vinst->fault_lock, flags);
> > +
> > + if (atomic_read(&vinst->faults_in_progress))
> > + ret = IRQ_HANDLED;
> > + else
> > + atomic_set(&vinst->faults_in_progress, 1);
> > +
> > + spin_unlock_irqrestore(&vinst->fault_lock, flags);
> > +
> > + return ret;
> > +}
> > +
> > static int vas_irq_fault_window_setup(struct vas_instance *vinst)
> > {
> > - return vas_setup_fault_window(vinst);
> > + char devname[64];
> > + int rc = 0;
> > +
> > + snprintf(devname, sizeof(devname), "vas-%d", vinst->vas_id);
> > + rc = request_threaded_irq(vinst->virq, vas_fault_handler,
> > + vas_fault_thread_fn, 0, devname, vinst);
> > +
> > + if (rc) {
> > + pr_err("VAS[%d]: Request IRQ(%d) failed with %d\n",
> > + vinst->vas_id, vinst->virq, rc);
> > + goto out;
> > + }
> > +
> > + rc = vas_setup_fault_window(vinst);
> > + if (rc)
> > + free_irq(vinst->virq, vinst);
> > +
> > +out:
> > + return rc;
> > }
> >
> > static int init_vas_instance(struct platform_device *pdev)
> > @@ -109,6 +155,7 @@ static int init_vas_instance(struct platform_device *pdev)
> > list_add(&vinst->node, &vas_instances);
> > mutex_unlock(&vas_mutex);
> >
> > + spin_lock_init(&vinst->fault_lock);
> > /*
> > * IRQ and fault handling setup is needed only for user space
> > * send windows.
> > diff --git a/arch/powerpc/platforms/powernv/vas.h b/arch/powerpc/platforms/powernv/vas.h
> > index 6c4baf5..ecae7cd 100644
> > --- a/arch/powerpc/platforms/powernv/vas.h
> > +++ b/arch/powerpc/platforms/powernv/vas.h
> > @@ -326,7 +326,10 @@ struct vas_instance {
> >
> > u64 irq_port;
> > int virq;
> > + int fault_crbs;
> > int fault_fifo_size;
> > + atomic_t faults_in_progress;
> > + spinlock_t fault_lock;
> > void *fault_fifo;
> > struct vas_window *fault_win; /* Fault window */
> >
> > @@ -424,6 +427,9 @@ struct vas_winctx {
> > extern void vas_window_init_dbgdir(struct vas_window *win);
> > extern void vas_window_free_dbgdir(struct vas_window *win);
> > extern int vas_setup_fault_window(struct vas_instance *vinst);
> > +extern irqreturn_t vas_fault_thread_fn(int irq, void *data);
> > +extern struct vas_window *vas_pswid_to_window(struct vas_instance *vinst,
> > + uint32_t pswid);
> >
> > static inline void vas_log_write(struct vas_window *win, char *name,
> > void *regptr, u64 val)
> > --
> > 1.8.3.1
> >
> >
> >
> >
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox