| Message ID | 1548696702-20686-1-git-send-email-arbab@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 865a9432d16fe2f40a1a52005fd30778056c7921 |
| Headers | show |
| Series | powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/build-ppc64le | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64be | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-ppc64e | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/build-pmac32 | success | build succeeded & removed 0 sparse warning(s) |
| snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 1 warnings, 0 checks, 8 lines checked |
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_IDEMPOTENT 0x00020 /* 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 <crjohns@us.ibm.com> > Signed-off-by: Reza Arbab <arbab@linux.ibm.com> > --- > 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) >
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_IDEMPOTENT 0x00020 /* 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 <crjohns@us.ibm.com> > Signed-off-by: Reza Arbab <arbab@linux.ibm.com> > --- > 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) >
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 <arbab@linux.ibm.com> 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
On 31/01/2019 00:35, Michael Ellerman wrote: > Reza Arbab <arbab@linux.ibm.com> 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.
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)
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_IDEMPOTENT 0x00020 /* 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. Suggested-by: Charles Johns <crjohns@us.ibm.com> Signed-off-by: Reza Arbab <arbab@linux.ibm.com> --- arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)