diff mbox series

[4/9] ARC: mm: do_page_fault refactor #3: tidyup vma accesspermission code

Message ID 1557880176-24964-5-git-send-email-vgupta@synopsys.com
State New
Headers show
Series ARC do_page_fault rework | expand

Commit Message

Vineet Gupta May 15, 2019, 12:29 a.m. UTC 
The coding pattern to NOT intialize variables at declaration time but
rather near code which makes us eof them makes it much easier to grok
the overall logic, specially when the init is not simply 0 or 1

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/mm/fault.c | 39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

Comments

Eugeniy Paltsev May 16, 2019, 5:24 p.m. UTC | #1 
On Tue, 2019-05-14 at 17:29 -0700, Vineet Gupta wrote:
> The coding pattern to NOT intialize variables at declaration time but
> rather near code which makes us eof them makes it much easier to grok
> the overall logic, specially when the init is not simply 0 or 1
> 
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  arch/arc/mm/fault.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arc/mm/fault.c b/arch/arc/mm/fault.c
> index f1175685d914..ae890a8d5ebf 100644
> --- a/arch/arc/mm/fault.c
> +++ b/arch/arc/mm/fault.c
> @@ -67,9 +67,9 @@ void do_page_fault(unsigned long address, struct pt_regs *regs)
>  	struct task_struct *tsk = current;
>  	struct mm_struct *mm = tsk->mm;
>  	int si_code = SEGV_MAPERR;
> +	unsigned int write = 0, exec = 0, mask;

Probably it's better to use 'bool' type for 'write' and 'exec' as we really use them as a boolean variables.


>  	vm_fault_t fault;
> -	int write = regs->ecr_cause & ECR_C_PROTV_STORE;  /* ST/EX */
> -	unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
> +	unsigned int flags;
>  
>  	/*
>  	 * NOTE! We MUST NOT take any locks for this case. We may
> @@ -91,8 +91,18 @@ void do_page_fault(unsigned long address, struct pt_regs *regs)
>  	if (faulthandler_disabled() || !mm)
>  		goto no_context;
>  
> +	if (regs->ecr_cause & ECR_C_PROTV_STORE)	/* ST/EX */
> +		write = 1;
> +	else if ((regs->ecr_vec == ECR_V_PROTV) &&
> +	         (regs->ecr_cause == ECR_C_PROTV_INST_FETCH))
> +		exec = 1;
> +
> +	flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
>  	if (user_mode(regs))
>  		flags |= FAULT_FLAG_USER;
> +	if (write)
> +		flags |= FAULT_FLAG_WRITE;
> +
>  retry:
>  	down_read(&mm->mmap_sem);
>  
> @@ -105,24 +115,17 @@ void do_page_fault(unsigned long address, struct pt_regs *regs)
>  	}
>  
>  	/*
> -	 * Ok, we have a good vm_area for this memory access, so
> -	 * we can handle it..
> +	 * vm_area is good, now check permissions for this memory access
>  	 */
> -	si_code = SEGV_ACCERR;
> -
> -	/* Handle protection violation, execute on heap or stack */
> -
> -	if ((regs->ecr_vec == ECR_V_PROTV) &&
> -	    (regs->ecr_cause == ECR_C_PROTV_INST_FETCH))
> +	mask = VM_READ;
> +	if (write)
> +		mask = VM_WRITE;
> +	if (exec)
> +		mask = VM_EXEC;
> +
> +	if (!(vma->vm_flags & mask)) {
> +		si_code = SEGV_ACCERR;
>  		goto bad_area;
> -
> -	if (write) {
> -		if (!(vma->vm_flags & VM_WRITE))
> -			goto bad_area;
> -		flags |= FAULT_FLAG_WRITE;
> -	} else {
> -		if (!(vma->vm_flags & (VM_READ | VM_EXEC)))
> -			goto bad_area;
>  	}
>  
>  	/*
Vineet Gupta May 30, 2019, 5:58 p.m. UTC | #2 
On 5/17/19 3:23 PM, Eugeniy Paltsev wrote:
> Hmmm,
> 
> so load the bool variable from memory is converted to such asm code:
> 
> ----------------->8------------------- 
> ldb	r2,[some_bool_address]
> extb_s	r2,r2
> ----------------->8-------------------
> 
> Could you please describe that the magic is going on there?
> 
> This extb_s instruction looks completely useless here, according on the LDB description from PRM:
> ----------------->8-------------------
> LD LDH LDW LDB LDD:
> The size of the requested data is specified by the data size field <.zz> and by default, data is zero
> extended from the most-significant bit of the data to the most-significant bit of the destination
> register.
> ----------------->8-------------------
> 
> Am I missing something?


@Claudiu is that a target specific optimization/tuning in ARC backend ?


> 
> On Thu, 2019-05-16 at 17:37 +0000, Vineet Gupta wrote:
>> On 5/16/19 10:24 AM, Eugeniy Paltsev wrote:
>>>> +    unsigned int write = 0, exec = 0, mask;
>>>
>>> Probably it's better to use 'bool' type for 'write' and 'exec' as we really use them as a boolean variables.
>>
>> Right those are semantics, but the generated code for "bool" is not ideal - given
>> it is inherently a "char" it is promoted first to an int with an additional EXTB
>> which I really dislike.
>> Guess it is more of a style thing.
>>
>> -Vineet
diff mbox series

Patch

diff --git a/arch/arc/mm/fault.c b/arch/arc/mm/fault.c
index f1175685d914..ae890a8d5ebf 100644
--- a/arch/arc/mm/fault.c
+++ b/arch/arc/mm/fault.c
@@ -67,9 +67,9 @@  void do_page_fault(unsigned long address, struct pt_regs *regs)
 	struct task_struct *tsk = current;
 	struct mm_struct *mm = tsk->mm;
 	int si_code = SEGV_MAPERR;
+	unsigned int write = 0, exec = 0, mask;
 	vm_fault_t fault;
-	int write = regs->ecr_cause & ECR_C_PROTV_STORE;  /* ST/EX */
-	unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+	unsigned int flags;
 
 	/*
 	 * NOTE! We MUST NOT take any locks for this case. We may
@@ -91,8 +91,18 @@  void do_page_fault(unsigned long address, struct pt_regs *regs)
 	if (faulthandler_disabled() || !mm)
 		goto no_context;
 
+	if (regs->ecr_cause & ECR_C_PROTV_STORE)	/* ST/EX */
+		write = 1;
+	else if ((regs->ecr_vec == ECR_V_PROTV) &&
+	         (regs->ecr_cause == ECR_C_PROTV_INST_FETCH))
+		exec = 1;
+
+	flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
 	if (user_mode(regs))
 		flags |= FAULT_FLAG_USER;
+	if (write)
+		flags |= FAULT_FLAG_WRITE;
+
 retry:
 	down_read(&mm->mmap_sem);
 
@@ -105,24 +115,17 @@  void do_page_fault(unsigned long address, struct pt_regs *regs)
 	}
 
 	/*
-	 * Ok, we have a good vm_area for this memory access, so
-	 * we can handle it..
+	 * vm_area is good, now check permissions for this memory access
 	 */
-	si_code = SEGV_ACCERR;
-
-	/* Handle protection violation, execute on heap or stack */
-
-	if ((regs->ecr_vec == ECR_V_PROTV) &&
-	    (regs->ecr_cause == ECR_C_PROTV_INST_FETCH))
+	mask = VM_READ;
+	if (write)
+		mask = VM_WRITE;
+	if (exec)
+		mask = VM_EXEC;
+
+	if (!(vma->vm_flags & mask)) {
+		si_code = SEGV_ACCERR;
 		goto bad_area;
-
-	if (write) {
-		if (!(vma->vm_flags & VM_WRITE))
-			goto bad_area;
-		flags |= FAULT_FLAG_WRITE;
-	} else {
-		if (!(vma->vm_flags & (VM_READ | VM_EXEC)))
-			goto bad_area;
 	}
 
 	/*