diff mbox

[PATCHv4,1/2] arm64: Introduce {set,clear}_pte_bit

Message ID 1408477303-2640-2-git-send-email-lauraa@codeaurora.org (mailing list archive)
State New, archived
Headers show

Commit Message

Laura Abbott Aug. 19, 2014, 7:41 p.m. UTC 
It's useful to be able to change individual bits in ptes at times.
Introduce functions for this and update existing pte_mk* functions
to use these primatives.

Acked-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 arch/arm64/include/asm/pgtable.h | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

Comments

Catalin Marinas Aug. 26, 2014, 2:27 p.m. UTC | #1 
On Tue, Aug 19, 2014 at 08:41:42PM +0100, Laura Abbott wrote:
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index ffe1ba0..ca41449 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -149,46 +149,51 @@ extern struct page *empty_zero_page;
>  #define pte_valid_not_user(pte) \
>  	((pte_val(pte) & (PTE_VALID | PTE_USER)) == PTE_VALID)
>  
> -static inline pte_t pte_wrprotect(pte_t pte)
> +static pte_t clear_pte_bit(pte_t pte, pgprot_t prot)
>  {
> -	pte_val(pte) &= ~PTE_WRITE;
> +	pte_val(pte) &= ~pgprot_val(prot);
>  	return pte;
>  }
>  
> -static inline pte_t pte_mkwrite(pte_t pte)
> +static pte_t set_pte_bit(pte_t pte, pgprot_t prot)
>  {
> -	pte_val(pte) |= PTE_WRITE;
> +	pte_val(pte) |= pgprot_val(prot);
>  	return pte;
>  }

Why these two functions don't have an "inline"?
Laura Abbott Aug. 26, 2014, 8:15 p.m. UTC | #2 
On 8/26/2014 7:27 AM, Catalin Marinas wrote:
> On Tue, Aug 19, 2014 at 08:41:42PM +0100, Laura Abbott wrote:
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index ffe1ba0..ca41449 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -149,46 +149,51 @@ extern struct page *empty_zero_page;
>>  #define pte_valid_not_user(pte) \
>>  	((pte_val(pte) & (PTE_VALID | PTE_USER)) == PTE_VALID)
>>  
>> -static inline pte_t pte_wrprotect(pte_t pte)
>> +static pte_t clear_pte_bit(pte_t pte, pgprot_t prot)
>>  {
>> -	pte_val(pte) &= ~PTE_WRITE;
>> +	pte_val(pte) &= ~pgprot_val(prot);
>>  	return pte;
>>  }
>>  
>> -static inline pte_t pte_mkwrite(pte_t pte)
>> +static pte_t set_pte_bit(pte_t pte, pgprot_t prot)
>>  {
>> -	pte_val(pte) |= PTE_WRITE;
>> +	pte_val(pte) |= pgprot_val(prot);
>>  	return pte;
>>  }
> 
> Why these two functions don't have an "inline"?
> 

That's an error on my part.

Will, you mentioned you applied these patches already, how
would you like to fix this up?

Laura
Will Deacon Aug. 27, 2014, 8:07 a.m. UTC | #3 
On Tue, Aug 26, 2014 at 09:15:40PM +0100, Laura Abbott wrote:
> On 8/26/2014 7:27 AM, Catalin Marinas wrote:
> > On Tue, Aug 19, 2014 at 08:41:42PM +0100, Laura Abbott wrote:
> >> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> >> index ffe1ba0..ca41449 100644
> >> --- a/arch/arm64/include/asm/pgtable.h
> >> +++ b/arch/arm64/include/asm/pgtable.h
> >> @@ -149,46 +149,51 @@ extern struct page *empty_zero_page;
> >>  #define pte_valid_not_user(pte) \
> >>  	((pte_val(pte) & (PTE_VALID | PTE_USER)) == PTE_VALID)
> >>  
> >> -static inline pte_t pte_wrprotect(pte_t pte)
> >> +static pte_t clear_pte_bit(pte_t pte, pgprot_t prot)
> >>  {
> >> -	pte_val(pte) &= ~PTE_WRITE;
> >> +	pte_val(pte) &= ~pgprot_val(prot);
> >>  	return pte;
> >>  }
> >>  
> >> -static inline pte_t pte_mkwrite(pte_t pte)
> >> +static pte_t set_pte_bit(pte_t pte, pgprot_t prot)
> >>  {
> >> -	pte_val(pte) |= PTE_WRITE;
> >> +	pte_val(pte) |= pgprot_val(prot);
> >>  	return pte;
> >>  }
> > 
> > Why these two functions don't have an "inline"?
> > 
> 
> That's an error on my part.
> 
> Will, you mentioned you applied these patches already, how
> would you like to fix this up?

Yup, I can easily add the missing inline keywords. Did you see Catalin's
other comment? It looks like we're missing a '-1' on the end address before
checking whether or not it sits in a module. If you confirm, I can add that
too.

Will
Laura Abbott Sept. 1, 2014, 3:42 p.m. UTC | #4 
On 8/27/2014 1:07 AM, Will Deacon wrote:
> On Tue, Aug 26, 2014 at 09:15:40PM +0100, Laura Abbott wrote:
>> On 8/26/2014 7:27 AM, Catalin Marinas wrote:
>>> On Tue, Aug 19, 2014 at 08:41:42PM +0100, Laura Abbott wrote:
>>>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>>>> index ffe1ba0..ca41449 100644
>>>> --- a/arch/arm64/include/asm/pgtable.h
>>>> +++ b/arch/arm64/include/asm/pgtable.h
>>>> @@ -149,46 +149,51 @@ extern struct page *empty_zero_page;
>>>>   #define pte_valid_not_user(pte) \
>>>>   	((pte_val(pte) & (PTE_VALID | PTE_USER)) == PTE_VALID)
>>>>
>>>> -static inline pte_t pte_wrprotect(pte_t pte)
>>>> +static pte_t clear_pte_bit(pte_t pte, pgprot_t prot)
>>>>   {
>>>> -	pte_val(pte) &= ~PTE_WRITE;
>>>> +	pte_val(pte) &= ~pgprot_val(prot);
>>>>   	return pte;
>>>>   }
>>>>
>>>> -static inline pte_t pte_mkwrite(pte_t pte)
>>>> +static pte_t set_pte_bit(pte_t pte, pgprot_t prot)
>>>>   {
>>>> -	pte_val(pte) |= PTE_WRITE;
>>>> +	pte_val(pte) |= pgprot_val(prot);
>>>>   	return pte;
>>>>   }
>>>
>>> Why these two functions don't have an "inline"?
>>>
>>
>> That's an error on my part.
>>
>> Will, you mentioned you applied these patches already, how
>> would you like to fix this up?
>
> Yup, I can easily add the missing inline keywords. Did you see Catalin's
> other comment? It looks like we're missing a '-1' on the end address before
> checking whether or not it sits in a module. If you confirm, I can add that
> too.
>
> Will
>

Yes, Catalin's review was correct, we need the -1 as well.

Thanks,
Laura
diff mbox

Patch

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index ffe1ba0..ca41449 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -149,46 +149,51 @@  extern struct page *empty_zero_page;
 #define pte_valid_not_user(pte) \
 	((pte_val(pte) & (PTE_VALID | PTE_USER)) == PTE_VALID)
 
-static inline pte_t pte_wrprotect(pte_t pte)
+static pte_t clear_pte_bit(pte_t pte, pgprot_t prot)
 {
-	pte_val(pte) &= ~PTE_WRITE;
+	pte_val(pte) &= ~pgprot_val(prot);
 	return pte;
 }
 
-static inline pte_t pte_mkwrite(pte_t pte)
+static pte_t set_pte_bit(pte_t pte, pgprot_t prot)
 {
-	pte_val(pte) |= PTE_WRITE;
+	pte_val(pte) |= pgprot_val(prot);
 	return pte;
 }
 
+static inline pte_t pte_wrprotect(pte_t pte)
+{
+	return clear_pte_bit(pte, __pgprot(PTE_WRITE));
+}
+
+static inline pte_t pte_mkwrite(pte_t pte)
+{
+	return set_pte_bit(pte, __pgprot(PTE_WRITE));
+}
+
 static inline pte_t pte_mkclean(pte_t pte)
 {
-	pte_val(pte) &= ~PTE_DIRTY;
-	return pte;
+	return clear_pte_bit(pte, __pgprot(PTE_DIRTY));
 }
 
 static inline pte_t pte_mkdirty(pte_t pte)
 {
-	pte_val(pte) |= PTE_DIRTY;
-	return pte;
+	return set_pte_bit(pte, __pgprot(PTE_DIRTY));
 }
 
 static inline pte_t pte_mkold(pte_t pte)
 {
-	pte_val(pte) &= ~PTE_AF;
-	return pte;
+	return clear_pte_bit(pte, __pgprot(PTE_AF));
 }
 
 static inline pte_t pte_mkyoung(pte_t pte)
 {
-	pte_val(pte) |= PTE_AF;
-	return pte;
+	return set_pte_bit(pte, __pgprot(PTE_AF));
 }
 
 static inline pte_t pte_mkspecial(pte_t pte)
 {
-	pte_val(pte) |= PTE_SPECIAL;
-	return pte;
+	return set_pte_bit(pte, __pgprot(PTE_SPECIAL));
 }
 
 static inline void set_pte(pte_t *ptep, pte_t pte)