Re: [PATCH v2] powerpc/book3s/hash: Drop _PAGE_PRIVILEGED from PAGE_NONE

[Michael Ellerman <mpe@ellerman.id.au>]

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:
> There used to be a dependency on _PAGE_PRIVILEGED with pte_savedwrite.
> But that got dropped by
> commit 6a56ccbcf6c6 ("mm/autonuma: use can_change_(pte|pmd)_writable() to replace savedwrite")
>
> With the change in this patch numa fault pte (pte_protnone()) gets mapped as regular user pte
> with RWX cleared (no-access) whereas earlier it used to be mapped _PAGE_PRIVILEGED.
>
> Hash fault handling code did get some WARN_ON added because those
> functions are not expected to get called with _PAGE_READ cleared.
> commit 18061c17c8ec ("powerpc/mm: Update PROTFAULT handling in the page fault path")
> explains the details.
 
You say "did get" which makes me think you're talking about the past.
But I think you're referring to the WARN_ON you are adding in this patch?

> Also revert commit 1abce0580b89 ("powerpc/64s: Fix __pte_needs_flush() false positive warning")

That could be done separately as a follow-up couldn't it? Would reduce
the diff size.

> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/book3s/64/pgtable.h  | 9 +++------
>  arch/powerpc/include/asm/book3s/64/tlbflush.h | 9 ++-------
>  arch/powerpc/mm/book3s64/hash_utils.c         | 7 +++++++
>  3 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index cb77eddca54b..2cc58ac74080 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -17,12 +17,6 @@
>  #define _PAGE_EXEC		0x00001 /* execute permission */
>  #define _PAGE_WRITE		0x00002 /* write access allowed */
>  #define _PAGE_READ		0x00004	/* read access allowed */
> -#define _PAGE_NA		_PAGE_PRIVILEGED
 
> -#define _PAGE_NAX		_PAGE_EXEC
> -#define _PAGE_RO		_PAGE_READ
> -#define _PAGE_ROX		(_PAGE_READ | _PAGE_EXEC)
> -#define _PAGE_RW		(_PAGE_READ | _PAGE_WRITE)
> -#define _PAGE_RWX		(_PAGE_READ | _PAGE_WRITE | _PAGE_EXEC)
 
Those are unrelated I think?

>  #define _PAGE_PRIVILEGED	0x00008 /* kernel access only */
>  #define _PAGE_SAO		0x00010 /* Strong access order */
>  #define _PAGE_NON_IDEMPOTENT	0x00020 /* non idempotent memory */
> @@ -529,6 +523,9 @@ static inline bool pte_user(pte_t pte)
>  }
>  
>  #define pte_access_permitted pte_access_permitted
> +/*
> + * execute-only mappings return false
> + */

That would fit better in the existing comment block inside the function
I think. Normally this location would be a function description comment.

>  static inline bool pte_access_permitted(pte_t pte, bool write)
>  {
>  	/*
          ie. here

cheers