Re: [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
On 31/01/2019 00:35, Michael Ellerman wrote: > Reza Arbab writes: > >> On Tue, Jan 29, 2019 at 08:37:28PM +0530, Aneesh Kumar K.V wrote: >>> Not sure what the fix is about. We set the related hash pte flags via >>> >>> if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_TOLERANT) >>> rflags |= HPTE_R_I; >>> else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_NON_IDEMPOTENT) >>> rflags |= (HPTE_R_I | HPTE_R_G); >>> else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_SAO) >>> rflags |= (HPTE_R_W | HPTE_R_I | HPTE_R_M); >> >> Again, nothing broken here, just a code readability thing. As Alexey >> (and Charlie) noted, given the above it is a little confusing to define >> _PAGE_CACHE_CTL this way: >> >> #define _PAGE_CACHE_CTL (_PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT) > > Yeah that's confusing I agree. > > It's not really a maintainability thing, because those bits are in the > architecture, so they can't change. > >> I like Alexey's idea, maybe just use a literal? >> >> #define _PAGE_CACHE_CTL 0x30 > > I prefer your original patch. It serves as documentation on what values > we expect to see in that field. As documentation, it gives an idea that there can be both _PAGE_NON_IDEMPOTENT and _PAGE_TOLERANT set which is not true. Putting possible values in the comment next to "#define _PAGE_CACHE_CTL" will document it properly imho. -- Alexey
Re: [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
Reza Arbab writes: > On Tue, Jan 29, 2019 at 08:37:28PM +0530, Aneesh Kumar K.V wrote: >>Not sure what the fix is about. We set the related hash pte flags via >> >> if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_TOLERANT) >> rflags |= HPTE_R_I; >> else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_NON_IDEMPOTENT) >> rflags |= (HPTE_R_I | HPTE_R_G); >> else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_SAO) >> rflags |= (HPTE_R_W | HPTE_R_I | HPTE_R_M); > > Again, nothing broken here, just a code readability thing. As Alexey > (and Charlie) noted, given the above it is a little confusing to define > _PAGE_CACHE_CTL this way: > > #define _PAGE_CACHE_CTL (_PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT) Yeah that's confusing I agree. It's not really a maintainability thing, because those bits are in the architecture, so they can't change. > I like Alexey's idea, maybe just use a literal? > > #define _PAGE_CACHE_CTL 0x30 I prefer your original patch. It serves as documentation on what values we expect to see in that field. cheers
Re: [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
On Tue, Jan 29, 2019 at 08:37:28PM +0530, Aneesh Kumar K.V wrote: Not sure what the fix is about. We set the related hash pte flags via if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_TOLERANT) rflags |= HPTE_R_I; else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_NON_IDEMPOTENT) rflags |= (HPTE_R_I | HPTE_R_G); else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_SAO) rflags |= (HPTE_R_W | HPTE_R_I | HPTE_R_M); Again, nothing broken here, just a code readability thing. As Alexey (and Charlie) noted, given the above it is a little confusing to define _PAGE_CACHE_CTL this way: #define _PAGE_CACHE_CTL (_PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT) I like Alexey's idea, maybe just use a literal? #define _PAGE_CACHE_CTL 0x30 -- Reza Arbab
Re: [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
On 1/28/19 11:01 PM, Reza Arbab wrote: In htab_convert_pte_flags(), _PAGE_CACHE_CTL is used to check for the _PAGE_SAO flag: else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_SAO) rflags |= (HPTE_R_W | HPTE_R_I | HPTE_R_M); But, it isn't defined to include that flag: #define _PAGE_CACHE_CTL (_PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT) This happens to work, but only because of the flag values: #define _PAGE_SAO 0x00010 /* Strong access order */ #define _PAGE_NON_IDEMPOTENT0x00020 /* non idempotent memory */ #define _PAGE_TOLERANT 0x00030 /* tolerant memory, cache inhibited */ To prevent any issues if these particulars ever change, add _PAGE_SAO to the mask. Not sure what the fix is about. We set the related hash pte flags via if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_TOLERANT) rflags |= HPTE_R_I; else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_NON_IDEMPOTENT) rflags |= (HPTE_R_I | HPTE_R_G); else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_SAO) rflags |= (HPTE_R_W | HPTE_R_I | HPTE_R_M); else /* * Add memory coherence if cache inhibited is not set */ rflags |= HPTE_R_M; Suggested-by: Charles Johns Signed-off-by: Reza Arbab --- arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h index 2e6ada2..1d97a28 100644 --- a/arch/powerpc/include/asm/book3s/64/pgtable.h +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h @@ -811,7 +811,7 @@ static inline void __set_pte_at(struct mm_struct *mm, unsigned long addr, return hash__set_pte_at(mm, addr, ptep, pte, percpu); } -#define _PAGE_CACHE_CTL (_PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT) +#define _PAGE_CACHE_CTL(_PAGE_SAO | _PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT) #define pgprot_noncached pgprot_noncached static inline pgprot_t pgprot_noncached(pgprot_t prot)
Re: [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
On 29/01/2019 04:31, Reza Arbab wrote: > In htab_convert_pte_flags(), _PAGE_CACHE_CTL is used to check for the > _PAGE_SAO flag: > > else if ((pteflags & _PAGE_CACHE_CTL) == _PAGE_SAO) > rflags |= (HPTE_R_W | HPTE_R_I | HPTE_R_M); > > But, it isn't defined to include that flag: > > #define _PAGE_CACHE_CTL (_PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT) > > This happens to work, but only because of the flag values: > > #define _PAGE_SAO 0x00010 /* Strong access order */ > #define _PAGE_NON_IDEMPOTENT0x00020 /* non idempotent memory */ > #define _PAGE_TOLERANT 0x00030 /* tolerant memory, cache inhibited > */ > > To prevent any issues if these particulars ever change, add _PAGE_SAO to > the mask. This does not feel right, doing #define _PAGE_CACHE_CTL 0x30 would make more sense as SAO/NI/TOLERANT is enum so applying "|" to them just seems wrong. > > Suggested-by: Charles Johns > Signed-off-by: Reza Arbab > --- > arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h > b/arch/powerpc/include/asm/book3s/64/pgtable.h > index 2e6ada2..1d97a28 100644 > --- a/arch/powerpc/include/asm/book3s/64/pgtable.h > +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h > @@ -811,7 +811,7 @@ static inline void __set_pte_at(struct mm_struct *mm, > unsigned long addr, > return hash__set_pte_at(mm, addr, ptep, pte, percpu); > } > > -#define _PAGE_CACHE_CTL (_PAGE_NON_IDEMPOTENT | _PAGE_TOLERANT) > +#define _PAGE_CACHE_CTL (_PAGE_SAO | _PAGE_NON_IDEMPOTENT | > _PAGE_TOLERANT) > > #define pgprot_noncached pgprot_noncached > static inline pgprot_t pgprot_noncached(pgprot_t prot) > -- Alexey
