[PATCH 1/2] powerpc/mm: Remove duplicated check in do_page_fault()

When the page fault happened in user space, we need check it's
caused by stack frame pointer update instruction and update
local variable @flag with FAULT_FLAG_USER. Currently, the code
has two separate check for the same condition. That's unnecessary.

This removes one of the duplicated check. No functinal changes
introduced.

Signed-off-by: Gavin Shan <redacted>
---
 arch/powerpc/mm/fault.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index a67c6d7..935f386 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c

@@ -294,11 +294,10 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
 	 * can result in fault, which will cause a deadlock when called with
 	 * mmap_sem held
 	 */
-	if (user_mode(regs))
-		store_update_sp = store_updates_sp(regs);
-
-	if (user_mode(regs))
+	if (user_mode(regs)) {
 		flags |= FAULT_FLAG_USER;
+		store_update_sp = store_updates_sp(regs);
+	}
 
 	/* When running in the kernel we expect faults to occur only to
 	 * addresses in user space.  All other faults represent errors in the

-- 
2.1.0

The function is used to update the MMU with software PTE. It can
be called by data access exception handler (0x300) or instruction
access exception handler (0x400). If the function is called by
0x400 handler , the local variable @access is set to _PAGE_EXEC
to indicate the software PTE should have that flag set. When the
function is called by 0x300 handler, @access is set to zero.

This improves the readability of the function by replacing if
statements with switch. No logical changes introduced.

Signed-off-by: Gavin Shan <redacted>
---
 arch/powerpc/mm/mem.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index d0f0a51..58b9b31 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c

@@ -492,7 +492,7 @@ void update_mmu_cache(struct vm_area_struct *vma, unsigned long address,
 	 * We don't need to worry about _PAGE_PRESENT here because we are
 	 * called with either mm->page_table_lock held or ptl lock held
 	 */
-	unsigned long access = 0, trap;
+	unsigned long access, trap;
 
 	/* We only want HPTEs for linux PTEs that have _PAGE_ACCESSED set */
 	if (!pte_young(*ptep) || address >= TASK_SIZE)

@@ -505,13 +505,18 @@ void update_mmu_cache(struct vm_area_struct *vma, unsigned long address,
 	 *
 	 * We also avoid filling the hash if not coming from a fault
 	 */
-	if (current->thread.regs == NULL)
-		return;
-	trap = TRAP(current->thread.regs);
-	if (trap == 0x400)
-		access |= _PAGE_EXEC;
-	else if (trap != 0x300)
+	trap = current->thread.regs ? TRAP(current->thread.regs) : 0UL;
+	switch (trap) {
+	case 0x300:
+		access = 0UL;
+		break;
+	case 0x400:
+		access = _PAGE_EXEC;
+		break;
+	default:
 		return;
+	}
+
 	hash_preload(vma->vm_mm, address, access, trap);
 #endif /* CONFIG_PPC_STD_MMU */
 #if (defined(CONFIG_PPC_BOOK3E_64) || defined(CONFIG_PPC_FSL_BOOK3E)) \

-- 
2.1.0

On Fri, 2016-26-02 at 00:26:25 UTC, Gavin Shan wrote:

When the page fault happened in user space, we need check it's
caused by stack frame pointer update instruction and update
local variable @flag with FAULT_FLAG_USER. Currently, the code
has two separate check for the same condition. That's unnecessary.

This removes one of the duplicated check. No functinal changes
introduced.

It's possible though that store_updates_sp() changes regs, and causes
user_mode(regs) to change, which would mean the second check is necessary.
That's not true with the current code, but you should mention that you confirmed
that in the change log.

It doesn't really matter in this case, but it would be better to keep the
ordering of the two statements the same as before.

cheers

On Mon, May 09, 2016 at 08:03:50PM +1000, Michael Ellerman wrote:

On Fri, 2016-26-02 at 00:26:25 UTC, Gavin Shan wrote:

quoted

When the page fault happened in user space, we need check it's
caused by stack frame pointer update instruction and update
local variable @flag with FAULT_FLAG_USER. Currently, the code
has two separate check for the same condition. That's unnecessary.

This removes one of the duplicated check. No functinal changes
introduced.

It's possible though that store_updates_sp() changes regs, and causes
user_mode(regs) to change, which would mean the second check is necessary.
That's not true with the current code, but you should mention that you confirmed
that in the change log.

Thanks for review. Yeah, store_updates_sp() checks the failing instruction
is the one updating stack frame pointer (stdu and the variable). The info
is used to expand the stack later. All of it should have been documented
in the commit log.

quoted

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index a67c6d7..935f386 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c

@@ -294,11 +294,10 @@ int __kprobes do_page_fault(struct pt_regs *regs, unsigned long address,
 	 * can result in fault, which will cause a deadlock when called with
 	 * mmap_sem held
 	 */
-	if (user_mode(regs))
-		store_update_sp = store_updates_sp(regs);
-
-	if (user_mode(regs))
+	if (user_mode(regs)) {
 		flags |= FAULT_FLAG_USER;
+		store_update_sp = store_updates_sp(regs);
+	}

It doesn't really matter in this case, but it would be better to keep the
ordering of the two statements the same as before.

Yep, It should have two checks in order as before here if store_updates_sp()
can alter MSR[PR] bit in future as you explained above :-)

Thanks,
Gavin

cheers

On Fri, 2016-26-02 at 00:26:26 UTC, Gavin Shan wrote:

The function is used to update the MMU with software PTE. It can
be called by data access exception handler (0x300) or instruction
access exception handler (0x400). If the function is called by
0x400 handler , the local variable @access is set to _PAGE_EXEC
to indicate the software PTE should have that flag set. When the
function is called by 0x300 handler, @access is set to zero.

This improves the readability of the function by replacing if
statements with switch. No logical changes introduced.

Signed-off-by: Gavin Shan <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/f3ad731cc08cc4fd3855f25c36

cheers