Re: [PATCH 1/2] No-exec support for ppc64

Olof Johansson
2005-03-10T03:30:06Z

On Tue, Mar 08, 2005 at 05:08:26PM -0600, Jake Moilanen wrote:
> No-exec base and user space support for PPC64.

Hi, a couple of comments below.

-Olof

> @@ -786,6 +786,7 @@ int hash_huge_page(struct mm_struct *mm,
> pte_t old_pte, new_pte;
> unsigned long hpteflags, prpn;
> long slot;
> + int is_exec;
> int err = 1;
>
> spin_lock(&mm->page_table_lock);
> @@ -796,6 +797,10 @@ int hash_huge_page(struct mm_struct *mm,
> va = (vsid << 28) | (ea & 0x0fffffff);
> vpn = va >> HPAGE_SHIFT;
>
> + is_exec = access & _PAGE_EXEC;
> + if (unlikely(is_exec && !(pte_val(*ptep) & _PAGE_EXEC)))
> + goto out;

You only use is_exec this one time, you can probably skip it and just
add the mask in the if statement.

> @@ -898,6 +908,7 @@ repeat:
> err = 0;
>
> out:
> +
> spin_unlock(&mm->page_table_lock);

Whitespace change

> diff -puN include/asm-ppc64/pgtable.h~nx-user-ppc64 include/asm-ppc64/pgtable.h
> --- linux-2.6-bk/include/asm-ppc64/pgtable.h~nx-user-ppc64 2005-03-08 16:08:54 -06:00
> +++ linux-2.6-bk-moilanen/include/asm-ppc64/pgtable.h 2005-03-08 16:08:54 -06:00
> @@ -82,14 +82,14 @@
> #define _PAGE_PRESENT 0x0001 /* software: pte contains a translation */
> #define _PAGE_USER 0x0002 /* matches one of the PP bits */
> #define _PAGE_FILE 0x0002 /* (!present only) software: pte holds file offset */
> -#define _PAGE_RW 0x0004 /* software: user write access allowed */
> +#define _PAGE_EXEC 0x0004 /* No execute on POWER4 and newer (we invert) */

Good to see the comment there, I remember we talked about that earlier.
It can be somewhat confusing. :-)

> #define _PAGE_GUARDED 0x0008
> #define _PAGE_COHERENT 0x0010 /* M: enforce memory coherence (SMP systems) */
> #define _PAGE_NO_CACHE 0x0020 /* I: cache inhibit */
> #define _PAGE_WRITETHRU 0x0040 /* W: cache write-through */
> #define _PAGE_DIRTY 0x0080 /* C: page changed */
> #define _PAGE_ACCESSED 0x0100 /* R: page referenced */
> -#define _PAGE_EXEC 0x0200 /* software: i-cache coherence required */
> +#define _PAGE_RW 0x0200 /* software: user write access allowed */
> #define _PAGE_HASHPTE 0x0400 /* software: pte has an associated HPTE */
> #define _PAGE_BUSY 0x0800 /* software: PTE & hash are busy */
> #define _PAGE_SECONDARY 0x8000 /* software: HPTE is in secondary group */
> @@ -100,7 +100,7 @@
> /* PAGE_MASK gives the right answer below, but only by accident */
> /* It should be preserving the high 48 bits and then specifically */
> /* preserving _PAGE_SECONDARY | _PAGE_GROUP_IX */
> -#define _PAGE_CHG_MASK (PAGE_MASK | _PAGE_ACCESSED | _PAGE_DIRTY | _PAGE_HPTEFLAGS)
> +#define _PAGE_CHG_MASK (_PAGE_GUARDED | _PAGE_COHERENT | _PAGE_NO_CACHE | _PAGE_WRITETHRU | _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_HPTEFLAGS | PAGE_MASK)

Can you break it into 80 columns with \ ?