[PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask

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 <redacted>
Signed-off-by: Reza Arbab <redacted>
---
 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)

-- 
1.8.3.1

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.

-- 
Alexey

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;

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

Reza Arbab [off-list ref] writes:

On Tue, Jan 29, 2019 at 08:37:28PM +0530, Aneesh Kumar K.V wrote:

quoted

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 [off-list ref] writes:

quoted

On Tue, Jan 29, 2019 at 08:37:28PM +0530, Aneesh Kumar K.V wrote:

quoted

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.

quoted

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

On Mon, 2019-01-28 at 17:31:42 UTC, 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.

Suggested-by: Charles Johns <redacted>
Signed-off-by: Reza Arbab <redacted>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/865a9432d16fe2f40a1a52005fd30778

cheers