* [PATCH V3 0/2] arm64/mm: Clean ups for do_page_fault() @ 2019-06-07 9:13 Anshuman Khandual 2019-06-07 9:13 ` [PATCH V3 1/2] arm64/mm: Consolidate page fault information capture Anshuman Khandual 2019-06-07 9:13 ` [PATCH V3 2/2] arm64/mm: Refactor __do_page_fault() Anshuman Khandual 0 siblings, 2 replies; 6+ messages in thread From: Anshuman Khandual @ 2019-06-07 9:13 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: Mark Rutland, Anshuman Khandual, Catalin Marinas, Will Deacon, Christoph Hellwig, James Morse, Andrey Konovalov This contains some clean ups for page fault handling in do_page_fault(). This has been boot tested on arm64 platform along with some stress tests but just build tested on others. Contains remaining two patches from v3 series after the first two patches were merged. This series applies on arm64 tree: git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git (for-next/core) Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: James Morse <james.morse@arm.com> Cc: Andrey Konovalov <andreyknvl@google.com> Cc: Christoph Hellwig <hch@infradead.org> Changes in V3: - Dropped first two patches which got queued up for 5.3 - Rebased on branch for-next/core on arm64 tree - s/is_el0_write_abort/is_write_abort/ with a comment per Mark and Catalin - s/is_el0_write/is_write following the above change - Updated commit message on [PATCH V3 2/2] per Catalin Changes in V2: (https://lkml.org/lkml/2019/6/3/79) - s/is_write_abort()/is_el0_write_abort() with a comment per Mark Rutland - s/is_write/is_el0_write/ & updated commit message as required - s/argument/local variable/ per Christoph Hellwig - Preserved VMA check order for VM_FAULT_BADMAP & VM_FAULT_BADACCESS per Mark - Preserved all existing __do_page_fault() in code comments per Mark - Dropped 'fixes' from the series's subject line per Will Deacon V1: https://lkml.org/lkml/2019/5/29/431 Anshuman Khandual (2): arm64/mm: Consolidate page fault information capture arm64/mm: Refactor __do_page_fault() arch/arm64/mm/fault.c | 56 +++++++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 26 deletions(-) -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V3 1/2] arm64/mm: Consolidate page fault information capture 2019-06-07 9:13 [PATCH V3 0/2] arm64/mm: Clean ups for do_page_fault() Anshuman Khandual @ 2019-06-07 9:13 ` Anshuman Khandual 2019-06-07 10:30 ` Mark Rutland 2019-06-07 9:13 ` [PATCH V3 2/2] arm64/mm: Refactor __do_page_fault() Anshuman Khandual 1 sibling, 1 reply; 6+ messages in thread From: Anshuman Khandual @ 2019-06-07 9:13 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: Mark Rutland, Anshuman Khandual, Catalin Marinas, Will Deacon, James Morse, Andrey Konovalov This consolidates page fault information capture and move them bit earlier. While here it also adds an wrapper is_write_abort(). It also saves some cycles by replacing multiple user_mode() calls into a single one earlier during the fault. Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: James Morse <james.morse@arm.com> Cc: Andrey Konovalov <andreyknvl@google.com> --- arch/arm64/mm/fault.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 7c1c8f4..95cac4a 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -435,6 +435,15 @@ static bool is_el0_instruction_abort(unsigned int esr) return ESR_ELx_EC(esr) == ESR_ELx_EC_IABT_LOW; } +/* + * Note: not valid for EL1 DC IVAC, but we never use that such that it + * should fault. EL0 cannot issue DC IVAC (undef). + */ +static bool is_write_abort(unsigned int esr) +{ + return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM); +} + static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, struct pt_regs *regs) { @@ -443,6 +452,9 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, vm_fault_t fault, major = 0; unsigned long vm_flags = VM_READ | VM_WRITE; unsigned int mm_flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; + bool is_user = user_mode(regs); + bool is_el0_exec = is_el0_instruction_abort(esr); + bool is_write = is_write_abort(esr); if (notify_page_fault(regs, esr)) return 0; @@ -454,13 +466,13 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, if (faulthandler_disabled() || !mm) goto no_context; - if (user_mode(regs)) + if (is_user) mm_flags |= FAULT_FLAG_USER; - if (is_el0_instruction_abort(esr)) { + if (is_el0_exec) { vm_flags = VM_EXEC; mm_flags |= FAULT_FLAG_INSTRUCTION; - } else if ((esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM)) { + } else if (is_write) { vm_flags = VM_WRITE; mm_flags |= FAULT_FLAG_WRITE; } @@ -488,7 +500,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, * we can bug out early if this is from code which shouldn't. */ if (!down_read_trylock(&mm->mmap_sem)) { - if (!user_mode(regs) && !search_exception_tables(regs->pc)) + if (!is_user && !search_exception_tables(regs->pc)) goto no_context; retry: down_read(&mm->mmap_sem); @@ -499,7 +511,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, */ might_sleep(); #ifdef CONFIG_DEBUG_VM - if (!user_mode(regs) && !search_exception_tables(regs->pc)) { + if (!is_user && !search_exception_tables(regs->pc)) { up_read(&mm->mmap_sem); goto no_context; } @@ -517,7 +529,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, * in __lock_page_or_retry in mm/filemap.c. */ if (fatal_signal_pending(current)) { - if (!user_mode(regs)) + if (!is_user) goto no_context; return 0; } @@ -562,7 +574,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, * If we are in kernel mode at this point, we have no context to * handle this fault with. */ - if (!user_mode(regs)) + if (!is_user) goto no_context; if (fault & VM_FAULT_OOM) { -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V3 1/2] arm64/mm: Consolidate page fault information capture 2019-06-07 9:13 ` [PATCH V3 1/2] arm64/mm: Consolidate page fault information capture Anshuman Khandual @ 2019-06-07 10:30 ` Mark Rutland 2019-06-07 13:34 ` Catalin Marinas 0 siblings, 1 reply; 6+ messages in thread From: Mark Rutland @ 2019-06-07 10:30 UTC (permalink / raw) To: Anshuman Khandual Cc: Andrey Konovalov, Will Deacon, linux-kernel, James Morse, Catalin Marinas, linux-arm-kernel On Fri, Jun 07, 2019 at 02:43:05PM +0530, Anshuman Khandual wrote: > This consolidates page fault information capture and move them bit earlier. > While here it also adds an wrapper is_write_abort(). It also saves some > cycles by replacing multiple user_mode() calls into a single one earlier > during the fault. > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: James Morse <james.morse@arm.com> > Cc: Andrey Konovalov <andreyknvl@google.com> > --- > arch/arm64/mm/fault.c | 26 +++++++++++++++++++------- > 1 file changed, 19 insertions(+), 7 deletions(-) As I mentioned previously, I doubt that this has any measureable impact on performance, and other than commenting the caveats w.r.t. cache maintenance, I'm not sure this makes things any clearer. However, AFAICT it is correct, so I'll leave that to Catalin: Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark. > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 7c1c8f4..95cac4a 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -435,6 +435,15 @@ static bool is_el0_instruction_abort(unsigned int esr) > return ESR_ELx_EC(esr) == ESR_ELx_EC_IABT_LOW; > } > > +/* > + * Note: not valid for EL1 DC IVAC, but we never use that such that it > + * should fault. EL0 cannot issue DC IVAC (undef). > + */ > +static bool is_write_abort(unsigned int esr) > +{ > + return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM); > +} > + > static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, > struct pt_regs *regs) > { > @@ -443,6 +452,9 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, > vm_fault_t fault, major = 0; > unsigned long vm_flags = VM_READ | VM_WRITE; > unsigned int mm_flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE; > + bool is_user = user_mode(regs); > + bool is_el0_exec = is_el0_instruction_abort(esr); > + bool is_write = is_write_abort(esr); > > if (notify_page_fault(regs, esr)) > return 0; > @@ -454,13 +466,13 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, > if (faulthandler_disabled() || !mm) > goto no_context; > > - if (user_mode(regs)) > + if (is_user) > mm_flags |= FAULT_FLAG_USER; > > - if (is_el0_instruction_abort(esr)) { > + if (is_el0_exec) { > vm_flags = VM_EXEC; > mm_flags |= FAULT_FLAG_INSTRUCTION; > - } else if ((esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM)) { > + } else if (is_write) { > vm_flags = VM_WRITE; > mm_flags |= FAULT_FLAG_WRITE; > } > @@ -488,7 +500,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, > * we can bug out early if this is from code which shouldn't. > */ > if (!down_read_trylock(&mm->mmap_sem)) { > - if (!user_mode(regs) && !search_exception_tables(regs->pc)) > + if (!is_user && !search_exception_tables(regs->pc)) > goto no_context; > retry: > down_read(&mm->mmap_sem); > @@ -499,7 +511,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, > */ > might_sleep(); > #ifdef CONFIG_DEBUG_VM > - if (!user_mode(regs) && !search_exception_tables(regs->pc)) { > + if (!is_user && !search_exception_tables(regs->pc)) { > up_read(&mm->mmap_sem); > goto no_context; > } > @@ -517,7 +529,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, > * in __lock_page_or_retry in mm/filemap.c. > */ > if (fatal_signal_pending(current)) { > - if (!user_mode(regs)) > + if (!is_user) > goto no_context; > return 0; > } > @@ -562,7 +574,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, > * If we are in kernel mode at this point, we have no context to > * handle this fault with. > */ > - if (!user_mode(regs)) > + if (!is_user) > goto no_context; > > if (fault & VM_FAULT_OOM) { > -- > 2.7.4 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V3 1/2] arm64/mm: Consolidate page fault information capture 2019-06-07 10:30 ` Mark Rutland @ 2019-06-07 13:34 ` Catalin Marinas 0 siblings, 0 replies; 6+ messages in thread From: Catalin Marinas @ 2019-06-07 13:34 UTC (permalink / raw) To: Mark Rutland Cc: Anshuman Khandual, Andrey Konovalov, Will Deacon, linux-kernel, James Morse, linux-arm-kernel On Fri, Jun 07, 2019 at 11:30:46AM +0100, Mark Rutland wrote: > On Fri, Jun 07, 2019 at 02:43:05PM +0530, Anshuman Khandual wrote: > > This consolidates page fault information capture and move them bit earlier. > > While here it also adds an wrapper is_write_abort(). It also saves some > > cycles by replacing multiple user_mode() calls into a single one earlier > > during the fault. > > > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will.deacon@arm.com> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: James Morse <james.morse@arm.com> > > Cc: Andrey Konovalov <andreyknvl@google.com> > > --- > > arch/arm64/mm/fault.c | 26 +++++++++++++++++++------- > > 1 file changed, 19 insertions(+), 7 deletions(-) > > As I mentioned previously, I doubt that this has any measureable impact > on performance, and other than commenting the caveats w.r.t. cache > maintenance, I'm not sure this makes things any clearer. > > However, AFAICT it is correct, so I'll leave that to Catalin: I only merged the is_write_abort() wrapper from this patch (changed the commit log as well). -- Catalin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V3 2/2] arm64/mm: Refactor __do_page_fault() 2019-06-07 9:13 [PATCH V3 0/2] arm64/mm: Clean ups for do_page_fault() Anshuman Khandual 2019-06-07 9:13 ` [PATCH V3 1/2] arm64/mm: Consolidate page fault information capture Anshuman Khandual @ 2019-06-07 9:13 ` Anshuman Khandual 2019-06-07 10:23 ` Mark Rutland 1 sibling, 1 reply; 6+ messages in thread From: Anshuman Khandual @ 2019-06-07 9:13 UTC (permalink / raw) To: linux-kernel, linux-arm-kernel Cc: Mark Rutland, Anshuman Khandual, Catalin Marinas, Will Deacon, Christoph Hellwig, James Morse, Andrey Konovalov __do_page_fault() is over complicated with multiple goto statements. This cleans up the code flow and while there drops local variable vm_fault_t. Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: James Morse <james.morse@arm.com> Cc: Andrey Konovalov <andreyknvl@google.com> Cc: Christoph Hellwig <hch@infradead.org> --- arch/arm64/mm/fault.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 95cac4a..235b7c0 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -397,37 +397,29 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr, unsigned int mm_flags, unsigned long vm_flags) { - struct vm_area_struct *vma; - vm_fault_t fault; + struct vm_area_struct *vma = find_vma(mm, addr); - vma = find_vma(mm, addr); - fault = VM_FAULT_BADMAP; if (unlikely(!vma)) - goto out; - if (unlikely(vma->vm_start > addr)) - goto check_stack; + return VM_FAULT_BADMAP; /* * Ok, we have a good vm_area for this memory access, so we can handle * it. */ -good_area: + if (unlikely(vma->vm_start > addr)) { + if (!(vma->vm_flags & VM_GROWSDOWN)) + return VM_FAULT_BADMAP; + if (expand_stack(vma, addr)) + return VM_FAULT_BADMAP; + } + /* * Check that the permissions on the VMA allow for the fault which * occurred. */ - if (!(vma->vm_flags & vm_flags)) { - fault = VM_FAULT_BADACCESS; - goto out; - } - + if (!(vma->vm_flags & vm_flags)) + return VM_FAULT_BADACCESS; return handle_mm_fault(vma, addr & PAGE_MASK, mm_flags); - -check_stack: - if (vma->vm_flags & VM_GROWSDOWN && !expand_stack(vma, addr)) - goto good_area; -out: - return fault; } static bool is_el0_instruction_abort(unsigned int esr) -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V3 2/2] arm64/mm: Refactor __do_page_fault() 2019-06-07 9:13 ` [PATCH V3 2/2] arm64/mm: Refactor __do_page_fault() Anshuman Khandual @ 2019-06-07 10:23 ` Mark Rutland 0 siblings, 0 replies; 6+ messages in thread From: Mark Rutland @ 2019-06-07 10:23 UTC (permalink / raw) To: Anshuman Khandual Cc: Andrey Konovalov, Will Deacon, linux-kernel, Christoph Hellwig, James Morse, Catalin Marinas, linux-arm-kernel On Fri, Jun 07, 2019 at 02:43:06PM +0530, Anshuman Khandual wrote: > __do_page_fault() is over complicated with multiple goto statements. This > cleans up the code flow and while there drops local variable vm_fault_t. > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: James Morse <james.morse@arm.com> > Cc: Andrey Konovalov <andreyknvl@google.com> > Cc: Christoph Hellwig <hch@infradead.org> This keeps the existing behaviour, and is certainly cleaner, so: Reviewed-by: Mark Rutland <Mark.rutland@arm.com> Mark. > --- > arch/arm64/mm/fault.c | 30 +++++++++++------------------- > 1 file changed, 11 insertions(+), 19 deletions(-) > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c > index 95cac4a..235b7c0 100644 > --- a/arch/arm64/mm/fault.c > +++ b/arch/arm64/mm/fault.c > @@ -397,37 +397,29 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re > static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr, > unsigned int mm_flags, unsigned long vm_flags) > { > - struct vm_area_struct *vma; > - vm_fault_t fault; > + struct vm_area_struct *vma = find_vma(mm, addr); > > - vma = find_vma(mm, addr); > - fault = VM_FAULT_BADMAP; > if (unlikely(!vma)) > - goto out; > - if (unlikely(vma->vm_start > addr)) > - goto check_stack; > + return VM_FAULT_BADMAP; > > /* > * Ok, we have a good vm_area for this memory access, so we can handle > * it. > */ > -good_area: > + if (unlikely(vma->vm_start > addr)) { > + if (!(vma->vm_flags & VM_GROWSDOWN)) > + return VM_FAULT_BADMAP; > + if (expand_stack(vma, addr)) > + return VM_FAULT_BADMAP; > + } > + > /* > * Check that the permissions on the VMA allow for the fault which > * occurred. > */ > - if (!(vma->vm_flags & vm_flags)) { > - fault = VM_FAULT_BADACCESS; > - goto out; > - } > - > + if (!(vma->vm_flags & vm_flags)) > + return VM_FAULT_BADACCESS; > return handle_mm_fault(vma, addr & PAGE_MASK, mm_flags); > - > -check_stack: > - if (vma->vm_flags & VM_GROWSDOWN && !expand_stack(vma, addr)) > - goto good_area; > -out: > - return fault; > } > > static bool is_el0_instruction_abort(unsigned int esr) > -- > 2.7.4 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-07 13:34 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-06-07 9:13 [PATCH V3 0/2] arm64/mm: Clean ups for do_page_fault() Anshuman Khandual 2019-06-07 9:13 ` [PATCH V3 1/2] arm64/mm: Consolidate page fault information capture Anshuman Khandual 2019-06-07 10:30 ` Mark Rutland 2019-06-07 13:34 ` Catalin Marinas 2019-06-07 9:13 ` [PATCH V3 2/2] arm64/mm: Refactor __do_page_fault() Anshuman Khandual 2019-06-07 10:23 ` Mark Rutland
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).