Re: [PATCH 05/12] MIPS: mm: Standardise on _PAGE_NO_READ, drop _PAGE_READ

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 8903 bytes --]

On Fri, Apr 15, 2016 at 11:36:53AM +0100, Paul Burton wrote:
> Ever since support for RI/XI was implemented by commit 6dd9344cfc41
> ("MIPS: Implement Read Inhibit/eXecute Inhibit") we've had a mixture of
> _PAGE_READ & _PAGE_NO_READ bits. Rather than keep both around, switch
> away from using _PAGE_READ to determine page presence & instead invert
> the use to _PAGE_NO_READ. Wherever we formerly had no definition for
> _PAGE_NO_READ, change what was _PAGE_READ to _PAGE_NO_READ. The end
> result is that we consistently use _PAGE_NO_READ to determine whether a
> page is readable, regardless of whether RI/XI is implemented.
> 
> Signed-off-by: Paul Burton <paul.burton@imgtec.com>
> ---
> 
>  arch/mips/include/asm/pgtable-bits.h | 19 ++++---------------
>  arch/mips/include/asm/pgtable.h      | 23 +++++++----------------
>  arch/mips/mm/tlbex.c                 | 13 ++++---------
>  3 files changed, 15 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/mips/include/asm/pgtable-bits.h b/arch/mips/include/asm/pgtable-bits.h
> index c81fc17..5bc663d 100644
> --- a/arch/mips/include/asm/pgtable-bits.h
> +++ b/arch/mips/include/asm/pgtable-bits.h
> @@ -49,7 +49,6 @@ enum pgtable_bits {
>  
>  	/* Used only by software (masked out before writing EntryLo*) */
>  	_PAGE_PRESENT_SHIFT = 24,
> -	_PAGE_READ_SHIFT,
>  	_PAGE_WRITE_SHIFT,
>  	_PAGE_ACCESSED_SHIFT,
>  	_PAGE_MODIFIED_SHIFT,
> @@ -66,7 +65,7 @@ enum pgtable_bits {
>  enum pgtable_bits {
>  	/* Used only by software (writes to EntryLo ignored) */
>  	_PAGE_PRESENT_SHIFT,
> -	_PAGE_READ_SHIFT,
> +	_PAGE_NO_READ_SHIFT,
>  	_PAGE_WRITE_SHIFT,
>  	_PAGE_ACCESSED_SHIFT,
>  	_PAGE_MODIFIED_SHIFT,
> @@ -85,7 +84,7 @@ enum pgtable_bits {
>  	/* Used only by software (masked out before writing EntryLo*) */
>  	_PAGE_PRESENT_SHIFT,
>  #if !defined(CONFIG_CPU_MIPSR2) && !defined(CONFIG_CPU_MIPSR6)
> -	_PAGE_READ_SHIFT,
> +	_PAGE_NO_READ_SHIFT,
>  #endif
>  	_PAGE_WRITE_SHIFT,
>  	_PAGE_ACCESSED_SHIFT,
> @@ -98,7 +97,6 @@ enum pgtable_bits {
>  #if defined(CONFIG_CPU_MIPSR2) || defined(CONFIG_CPU_MIPSR6)
>  	_PAGE_NO_EXEC_SHIFT,
>  	_PAGE_NO_READ_SHIFT,
> -	_PAGE_READ_SHIFT = _PAGE_NO_READ_SHIFT,
>  #endif
>  	_PAGE_GLOBAL_SHIFT,
>  	_PAGE_VALID_SHIFT,
> @@ -110,11 +108,6 @@ enum pgtable_bits {
>  
>  /* Used only by software */
>  #define _PAGE_PRESENT		(1 << _PAGE_PRESENT_SHIFT)
> -#if defined(CONFIG_CPU_MIPSR2) || defined(CONFIG_CPU_MIPSR6)
> -# define _PAGE_READ		(cpu_has_rixi ? 0 : (1 << _PAGE_READ_SHIFT))
> -#else
> -# define _PAGE_READ		(1 << _PAGE_READ_SHIFT)
> -#endif
>  #define _PAGE_WRITE		(1 << _PAGE_WRITE_SHIFT)
>  #define _PAGE_ACCESSED		(1 << _PAGE_ACCESSED_SHIFT)
>  #define _PAGE_MODIFIED		(1 << _PAGE_MODIFIED_SHIFT)
> @@ -125,11 +118,10 @@ enum pgtable_bits {
>  /* Used by TLB hardware (placed in EntryLo*) */
>  #if (defined(CONFIG_PHYS_ADDR_T_64BIT) && defined(CONFIG_CPU_MIPS32))
>  # define _PAGE_NO_EXEC		(1 << _PAGE_NO_EXEC_SHIFT)
> -# define _PAGE_NO_READ		(1 << _PAGE_NO_READ_SHIFT)
>  #elif defined(CONFIG_CPU_MIPSR2) || defined(CONFIG_CPU_MIPSR6)
>  # define _PAGE_NO_EXEC		(cpu_has_rixi ? (1 << _PAGE_NO_EXEC_SHIFT) : 0)
> -# define _PAGE_NO_READ		(cpu_has_rixi ? (1 << _PAGE_NO_READ_SHIFT) : 0)
>  #endif
> +#define _PAGE_NO_READ		(1 << _PAGE_NO_READ_SHIFT)
>  #define _PAGE_GLOBAL		(1 << _PAGE_GLOBAL_SHIFT)
>  #define _PAGE_VALID		(1 << _PAGE_VALID_SHIFT)
>  #define _PAGE_DIRTY		(1 << _PAGE_DIRTY_SHIFT)
> @@ -145,9 +137,6 @@ enum pgtable_bits {
>  #ifndef _PAGE_NO_EXEC
>  #define _PAGE_NO_EXEC		0
>  #endif
> -#ifndef _PAGE_NO_READ
> -#define _PAGE_NO_READ		0
> -#endif
>  
>  #define _PAGE_SILENT_READ	_PAGE_VALID
>  #define _PAGE_SILENT_WRITE	_PAGE_DIRTY
> @@ -245,7 +234,7 @@ static inline uint64_t pte_to_entrylo(unsigned long pte_val)
>  #define _CACHE_UNCACHED_ACCELERATED	(7<<_CACHE_SHIFT)
>  #endif
>  
> -#define __READABLE	(_PAGE_SILENT_READ | _PAGE_READ | _PAGE_ACCESSED)
> +#define __READABLE	(_PAGE_SILENT_READ | _PAGE_ACCESSED)
>  #define __WRITEABLE	(_PAGE_SILENT_WRITE | _PAGE_WRITE | _PAGE_MODIFIED)
>  
>  #define _PAGE_CHG_MASK	(_PAGE_ACCESSED | _PAGE_MODIFIED |	\
> diff --git a/arch/mips/include/asm/pgtable.h b/arch/mips/include/asm/pgtable.h
> index 9a4fe01..1459ee9 100644
> --- a/arch/mips/include/asm/pgtable.h
> +++ b/arch/mips/include/asm/pgtable.h
> @@ -23,18 +23,19 @@
>  struct mm_struct;
>  struct vm_area_struct;
>  
> -#define PAGE_NONE	__pgprot(_PAGE_PRESENT | _CACHE_CACHABLE_NONCOHERENT)
> -#define PAGE_SHARED	__pgprot(_PAGE_PRESENT | _PAGE_WRITE | _PAGE_READ | \
> +#define PAGE_NONE	__pgprot(_PAGE_PRESENT | _PAGE_NO_READ | \
> +				 _CACHE_CACHABLE_NONCOHERENT)
> +#define PAGE_SHARED	__pgprot(_PAGE_PRESENT | _PAGE_WRITE | \
>  				 _page_cachable_default)
> -#define PAGE_COPY	__pgprot(_PAGE_PRESENT | _PAGE_READ | _PAGE_NO_EXEC | \
> +#define PAGE_COPY	__pgprot(_PAGE_PRESENT | _PAGE_NO_EXEC | \
>  				 _page_cachable_default)
> -#define PAGE_READONLY	__pgprot(_PAGE_PRESENT | _PAGE_READ | \
> +#define PAGE_READONLY	__pgprot(_PAGE_PRESENT | \
>  				 _page_cachable_default)
>  #define PAGE_KERNEL	__pgprot(_PAGE_PRESENT | __READABLE | __WRITEABLE | \
>  				 _PAGE_GLOBAL | _page_cachable_default)
>  #define PAGE_KERNEL_NC	__pgprot(_PAGE_PRESENT | __READABLE | __WRITEABLE | \
>  				 _PAGE_GLOBAL | _CACHE_CACHABLE_NONCOHERENT)
> -#define PAGE_USERIO	__pgprot(_PAGE_PRESENT | _PAGE_READ | _PAGE_WRITE | \
> +#define PAGE_USERIO	__pgprot(_PAGE_PRESENT | _PAGE_WRITE | \
>  				 _page_cachable_default)
>  #define PAGE_KERNEL_UNCACHED __pgprot(_PAGE_PRESENT | __READABLE | \
>  			__WRITEABLE | _PAGE_GLOBAL | _CACHE_UNCACHED)
> @@ -307,7 +308,7 @@ static inline pte_t pte_mkdirty(pte_t pte)
>  static inline pte_t pte_mkyoung(pte_t pte)
>  {
>  	pte.pte_low |= _PAGE_ACCESSED;
> -	if (pte.pte_low & _PAGE_READ)
> +	if (!(pte.pte_low & _PAGE_NO_READ))
>  		pte.pte_high |= _PAGE_SILENT_READ;
>  	return pte;
>  }
> @@ -353,13 +354,8 @@ static inline pte_t pte_mkdirty(pte_t pte)
>  static inline pte_t pte_mkyoung(pte_t pte)
>  {
>  	pte_val(pte) |= _PAGE_ACCESSED;
> -#if defined(CONFIG_CPU_MIPSR2) || defined(CONFIG_CPU_MIPSR6)
>  	if (!(pte_val(pte) & _PAGE_NO_READ))
>  		pte_val(pte) |= _PAGE_SILENT_READ;
> -	else
> -#endif
> -	if (pte_val(pte) & _PAGE_READ)
> -		pte_val(pte) |= _PAGE_SILENT_READ;
>  	return pte;
>  }
>  
> @@ -542,13 +538,8 @@ static inline pmd_t pmd_mkyoung(pmd_t pmd)
>  {
>  	pmd_val(pmd) |= _PAGE_ACCESSED;
>  
> -#if defined(CONFIG_CPU_MIPSR2) || defined(CONFIG_CPU_MIPSR6)
>  	if (!(pmd_val(pmd) & _PAGE_NO_READ))
>  		pmd_val(pmd) |= _PAGE_SILENT_READ;
> -	else
> -#endif
> -	if (pmd_val(pmd) & _PAGE_READ)
> -		pmd_val(pmd) |= _PAGE_SILENT_READ;
>  
>  	return pmd;
>  }
> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> index 86aa7c2..7e3272f 100644
> --- a/arch/mips/mm/tlbex.c
> +++ b/arch/mips/mm/tlbex.c
> @@ -234,20 +234,16 @@ static void output_pgtable_bits_defines(void)
>  	pr_debug("\n");
>  
>  	pr_define("_PAGE_PRESENT_SHIFT %d\n", _PAGE_PRESENT_SHIFT);
> -	pr_define("_PAGE_READ_SHIFT %d\n", _PAGE_READ_SHIFT);
> +	pr_define("_PAGE_NO_READ_SHIFT %d\n", _PAGE_NO_READ_SHIFT);
>  	pr_define("_PAGE_WRITE_SHIFT %d\n", _PAGE_WRITE_SHIFT);
>  	pr_define("_PAGE_ACCESSED_SHIFT %d\n", _PAGE_ACCESSED_SHIFT);
>  	pr_define("_PAGE_MODIFIED_SHIFT %d\n", _PAGE_MODIFIED_SHIFT);
>  #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
>  	pr_define("_PAGE_HUGE_SHIFT %d\n", _PAGE_HUGE_SHIFT);
>  #endif
> -#if defined(CONFIG_CPU_MIPSR2) || defined(CONFIG_CPU_MIPSR6)
> -	if (cpu_has_rixi) {
>  #ifdef _PAGE_NO_EXEC_SHIFT
> +	if (cpu_has_rixi)
>  		pr_define("_PAGE_NO_EXEC_SHIFT %d\n", _PAGE_NO_EXEC_SHIFT);
> -		pr_define("_PAGE_NO_READ_SHIFT %d\n", _PAGE_NO_READ_SHIFT);
> -#endif
> -	}
>  #endif
>  	pr_define("_PAGE_GLOBAL_SHIFT %d\n", _PAGE_GLOBAL_SHIFT);
>  	pr_define("_PAGE_VALID_SHIFT %d\n", _PAGE_VALID_SHIFT);
> @@ -1615,9 +1611,8 @@ build_pte_present(u32 **p, struct uasm_reloc **r,
>  			cur = t;
>  		}
>  		uasm_i_andi(p, t, cur,
> -			(_PAGE_PRESENT | _PAGE_READ) >> _PAGE_PRESENT_SHIFT);
> -		uasm_i_xori(p, t, t,
> -			(_PAGE_PRESENT | _PAGE_READ) >> _PAGE_PRESENT_SHIFT);
> +			(_PAGE_PRESENT | _PAGE_NO_READ) >> _PAGE_PRESENT_SHIFT);
> +		uasm_i_xori(p, t, t, _PAGE_PRESENT >> _PAGE_PRESENT_SHIFT);

This code makes the assumption that _PAGE_READ was always at a higher
bit number than _PAGE_PRESENT, however this isn't true for _PAGE_NO_READ
in the defined(CONFIG_PHYS_ADDR_T_64BIT) && defined(CONFIG_CPU_MIPS32)
case, where the no read bit will have been shifted off the end of the
register value.

Other than that, I can't fault this patch.

Cheers
James

>  		uasm_il_bnez(p, r, t, lid);
>  		if (pte == t)
>  			/* You lose the SMP race :-(*/
> -- 
> 2.8.0
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]