[PATCH V5 2/2] arm: mm: Modify pte_write and pmd_write logic for LPAE

Steve Capper steve.capper at linaro.org
Fri Jun 27 05:42:00 PDT 2014


On Fri, Jun 27, 2014 at 12:39:34PM +0100, Will Deacon wrote:
> Hey Steve,
> 
> On Tue, Jun 24, 2014 at 01:23:24PM +0100, Steve Capper wrote:
> > For LPAE, we have the following means for encoding writable or dirty
> > ptes:
> >                               L_PTE_DIRTY       L_PTE_RDONLY
> >     !pte_dirty && !pte_write        0               1
> >     !pte_dirty && pte_write         0               1
> >     pte_dirty && !pte_write         1               1
> >     pte_dirty && pte_write          1               0
> > 
> > So we can't distinguish between writeable clean ptes and read only
> > ptes. This can cause problems with ptes being incorrectly flagged as
> > read only when they are writeable but not dirty.
> > 
> > This patch renumbers L_PTE_RDONLY from AP[2] to a software bit #58,
> > and adds additional logic to set AP[2] whenever the pte is read only
> > or not dirty. That way we can distinguish between clean writeable ptes
> > and read only ptes.
> > 
> > HugeTLB pages will use this new logic automatically.
> > 
> > We need to add some logic to Transparent HugePages to ensure that they
> > correctly interpret the revised pgprot permissions (L_PTE_RDONLY has
> > moved and no longer matches PMD_SECT_RDONLY). In the process of
> > revising THP, the names of the PMD software bits have been prefixed
> > with L_ to make them easier to distinguish from their hardware bit
> > counterparts.
> > 
> > Signed-off-by: Steve Capper <steve.capper at linaro.org>
> > ---
> > Changed in V5 -> rather than re-introduce L_PTE_WRITE we change
> > L_PTE_RDONLY to be a software bit #58. This allows us to leave the
> > two level code alone.
> 
> Damnit, you owe me a beer every time I have to review this patch ;)
> At least proc-macros.S is unscathed this time around!

It's thirsty work writing and testing it too, I have a cunning plan
that can fix both our problems.... :-)

> 
> > ---
> >  arch/arm/include/asm/pgtable-3level-hwdef.h |  1 +
> >  arch/arm/include/asm/pgtable-3level.h       | 37 +++++++++++++++++------------
> >  arch/arm/mm/proc-v7-3level.S                |  9 +++++--
> >  3 files changed, 30 insertions(+), 17 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/pgtable-3level-hwdef.h b/arch/arm/include/asm/pgtable-3level-hwdef.h
> > index 626989f..552a5f7 100644
> > --- a/arch/arm/include/asm/pgtable-3level-hwdef.h
> > +++ b/arch/arm/include/asm/pgtable-3level-hwdef.h
> > @@ -72,6 +72,7 @@
> >  #define PTE_TABLE_BIT		(_AT(pteval_t, 1) << 1)
> >  #define PTE_BUFFERABLE		(_AT(pteval_t, 1) << 2)		/* AttrIndx[0] */
> >  #define PTE_CACHEABLE		(_AT(pteval_t, 1) << 3)		/* AttrIndx[1] */
> > +#define PTE_AP2			(_AT(pteval_t, 1) << 7)		/* AP[2] */
> 
> PTE_RDONLY? AP2 is pretty cryptic.

That is probably better, but could it be confused with L_PRE_RDONLY?
(Please see below for the pmd analogue).

> 
> >  #define PTE_EXT_SHARED		(_AT(pteval_t, 3) << 8)		/* SH[1:0], inner shareable */
> >  #define PTE_EXT_AF		(_AT(pteval_t, 1) << 10)	/* Access Flag */
> >  #define PTE_EXT_NG		(_AT(pteval_t, 1) << 11)	/* nG */
> > diff --git a/arch/arm/include/asm/pgtable-3level.h b/arch/arm/include/asm/pgtable-3level.h
> > index 3b10ec6..24ba2d7 100644
> > --- a/arch/arm/include/asm/pgtable-3level.h
> > +++ b/arch/arm/include/asm/pgtable-3level.h
> > @@ -79,18 +79,19 @@
> >  #define L_PTE_PRESENT		(_AT(pteval_t, 3) << 0)		/* Present */
> >  #define L_PTE_FILE		(_AT(pteval_t, 1) << 2)		/* only when !PRESENT */
> >  #define L_PTE_USER		(_AT(pteval_t, 1) << 6)		/* AP[1] */
> > -#define L_PTE_RDONLY		(_AT(pteval_t, 1) << 7)		/* AP[2] */
> >  #define L_PTE_SHARED		(_AT(pteval_t, 3) << 8)		/* SH[1:0], inner shareable */
> >  #define L_PTE_YOUNG		(_AT(pteval_t, 1) << 10)	/* AF */
> >  #define L_PTE_XN		(_AT(pteval_t, 1) << 54)	/* XN */
> >  #define L_PTE_DIRTY		(_AT(pteval_t, 1) << 55)	/* unused */
> >  #define L_PTE_SPECIAL		(_AT(pteval_t, 1) << 56)	/* unused */
> >  #define L_PTE_NONE		(_AT(pteval_t, 1) << 57)	/* PROT_NONE */
> > +#define L_PTE_RDONLY		(_AT(pteval_t, 1) << 58)	/* READ ONLY */
> 
> Can you fix those `unused' comments above while you're here, please? Whilst
> they are unused by the hardware, they're very much used by Linux. Well,
> SPECIAL isn't yet, but it could be...

Sure.

> 
> > -#define PMD_SECT_VALID		(_AT(pmdval_t, 1) << 0)
> > -#define PMD_SECT_DIRTY		(_AT(pmdval_t, 1) << 55)
> > -#define PMD_SECT_SPLITTING	(_AT(pmdval_t, 1) << 56)
> > -#define PMD_SECT_NONE		(_AT(pmdval_t, 1) << 57)
> > +#define L_PMD_SECT_VALID	(_AT(pmdval_t, 1) << 0)
> > +#define L_PMD_SECT_DIRTY	(_AT(pmdval_t, 1) << 55)
> > +#define L_PMD_SECT_SPLITTING	(_AT(pmdval_t, 1) << 56)
> > +#define L_PMD_SECT_NONE		(_AT(pmdval_t, 1) << 57)
> > +#define L_PMD_SECT_RDONLY	(_AT(pteval_t, 1) << 58)
> 
> Boo-hoo, that's our last remaining software bit for LPAE. It's a sad day.
> 
> >  /*
> >   * To be used in assembly code with the upper page attributes.
> > @@ -214,24 +215,25 @@ static inline pmd_t *pmd_offset(pud_t *pud, unsigned long addr)
> >  #define pmd_young(pmd)		(pmd_isset((pmd), PMD_SECT_AF))
> >  
> >  #define __HAVE_ARCH_PMD_WRITE
> > -#define pmd_write(pmd)		(pmd_isclear((pmd), PMD_SECT_RDONLY))
> > +#define pmd_write(pmd)		(pmd_isclear((pmd), L_PMD_SECT_RDONLY))
> > +#define pmd_dirty(pmd)		(pmd_isset((pmd), L_PMD_SECT_DIRTY))
> >  
> >  #define pmd_hugewillfault(pmd)	(!pmd_young(pmd) || !pmd_write(pmd))
> >  #define pmd_thp_or_huge(pmd)	(pmd_huge(pmd) || pmd_trans_huge(pmd))
> >  
> >  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >  #define pmd_trans_huge(pmd)	(pmd_val(pmd) && pmd_isclear((pmd), PMD_TABLE_BIT))
> > -#define pmd_trans_splitting(pmd) (pmd_isset((pmd), PMD_SECT_SPLITTING))
> > +#define pmd_trans_splitting(pmd) (pmd_isset((pmd),  L_PMD_SECT_SPLITTING))
> 
> I take it we only call this on something we've decided is huge already (as
> opposed to, e.g. a swap entry)?

Yes that's correct, the sequence is pmd_trans_huge(blah), then later
on pmd_trans_splitting(blah).
 
> 
> >  #endif
> >  
> >  #define PMD_BIT_FUNC(fn,op) \
> >  static inline pmd_t pmd_##fn(pmd_t pmd) { pmd_val(pmd) op; return pmd; }
> >  
> > -PMD_BIT_FUNC(wrprotect,	|= PMD_SECT_RDONLY);
> > +PMD_BIT_FUNC(wrprotect,	|= L_PMD_SECT_RDONLY);
> >  PMD_BIT_FUNC(mkold,	&= ~PMD_SECT_AF);
> > -PMD_BIT_FUNC(mksplitting, |= PMD_SECT_SPLITTING);
> > -PMD_BIT_FUNC(mkwrite,   &= ~PMD_SECT_RDONLY);
> > -PMD_BIT_FUNC(mkdirty,   |= PMD_SECT_DIRTY);
> > +PMD_BIT_FUNC(mksplitting, |= L_PMD_SECT_SPLITTING);
> > +PMD_BIT_FUNC(mkwrite,   &= ~L_PMD_SECT_RDONLY);
> > +PMD_BIT_FUNC(mkdirty,   |= L_PMD_SECT_DIRTY);
> >  PMD_BIT_FUNC(mkyoung,   |= PMD_SECT_AF);
> >  
> >  #define pmd_mkhuge(pmd)		(__pmd(pmd_val(pmd) & ~PMD_TABLE_BIT))
> > @@ -245,8 +247,8 @@ PMD_BIT_FUNC(mkyoung,   |= PMD_SECT_AF);
> >  
> >  static inline pmd_t pmd_modify(pmd_t pmd, pgprot_t newprot)
> >  {
> > -	const pmdval_t mask = PMD_SECT_USER | PMD_SECT_XN | PMD_SECT_RDONLY |
> > -				PMD_SECT_VALID | PMD_SECT_NONE;
> > +	const pmdval_t mask = PMD_SECT_USER | PMD_SECT_XN | L_PMD_SECT_RDONLY |
> > +				L_PMD_SECT_VALID | L_PMD_SECT_NONE;
> >  	pmd_val(pmd) = (pmd_val(pmd) & ~mask) | (pgprot_val(newprot) & mask);
> >  	return pmd;
> >  }
> > @@ -257,8 +259,13 @@ static inline void set_pmd_at(struct mm_struct *mm, unsigned long addr,
> >  	BUG_ON(addr >= TASK_SIZE);
> >  
> >  	/* create a faulting entry if PROT_NONE protected */
> > -	if (pmd_val(pmd) & PMD_SECT_NONE)
> > -		pmd_val(pmd) &= ~PMD_SECT_VALID;
> > +	if (pmd_val(pmd) & L_PMD_SECT_NONE)
> > +		pmd_val(pmd) &= ~L_PMD_SECT_VALID;
> > +
> > +	if (pmd_write(pmd) && pmd_dirty(pmd))
> > +		pmd_val(pmd) &= ~PMD_SECT_RDONLY;
> 
> pmd_mkwrite?
> 
> > +	else
> > +		pmd_val(pmd) |= PMD_SECT_RDONLY;
> 
> pmd_wrprotect?

I'm setting the hardware bits here. I tried to distinguish:
PMD_SECT_RDONLY and L_PMD_SECT_RDONLY. I think I should probably rename
PMD_SECT_RDONLY to something else?

> 
> >  
> >  	*pmdp = __pmd(pmd_val(pmd) | PMD_SECT_nG);
> >  	flush_pmd_entry(pmdp);
> > diff --git a/arch/arm/mm/proc-v7-3level.S b/arch/arm/mm/proc-v7-3level.S
> > index 22e3ad6..eb81123 100644
> > --- a/arch/arm/mm/proc-v7-3level.S
> > +++ b/arch/arm/mm/proc-v7-3level.S
> > @@ -86,8 +86,13 @@ ENTRY(cpu_v7_set_pte_ext)
> >  	tst	rh, #1 << (57 - 32)		@ L_PTE_NONE
> >  	bicne	rl, #L_PTE_VALID
> >  	bne	1f
> > -	tst	rh, #1 << (55 - 32)		@ L_PTE_DIRTY
> > -	orreq	rl, #L_PTE_RDONLY
> > +
> > +	eor	ip, rh, #1 << (55 - 32)	@ toggle L_PTE_DIRTY in temp reg to
> > +					@ test for !L_PTE_DIRTY || L_PTE_RDONLY
> 
> Since you already set RDONLY for pmds based on dirty, can we do the same for
> ptes and avoid this check here...?

Isn't the pte check logically equivalent to the pmd case?
	if (pmd_write(pmd) && pmd_dirty(pmd))

> 
> > +	tst	ip, #1 << (55 - 32) | 1 << (58 - 32)
> > +	orrne	rl, #PTE_AP2
> > +	biceq	rl, #PTE_AP2
> 
> ... Then use Russell's ubfx trick to copy L_PTE_RDONLY into PTE_RDONLY?
> 

I will have another think about this.

Cheers,
-- 
Steve



More information about the linux-arm-kernel mailing list