* [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
@ 2019-01-28 17:31 Reza Arbab
2019-01-29 1:39 ` Alexey Kardashevskiy
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Reza Arbab @ 2019-01-28 17:31 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Charles Johns, Paul Mackerras, Aneesh Kumar K.V
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(-)
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
2019-01-28 17:31 [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask Reza Arbab
@ 2019-01-29 1:39 ` Alexey Kardashevskiy
2019-01-29 15:07 ` Aneesh Kumar K.V
2019-02-08 13:02 ` Michael Ellerman
2 siblings, 0 replies; 7+ messages in thread
From: Alexey Kardashevskiy @ 2019-01-29 1:39 UTC (permalink / raw)
To: Reza Arbab, linuxppc-dev; +Cc: Aneesh Kumar K.V, Charles Johns, Paul Mackerras
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)
>
--
Alexey
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
2019-01-28 17:31 [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask Reza Arbab
2019-01-29 1:39 ` Alexey Kardashevskiy
@ 2019-01-29 15:07 ` Aneesh Kumar K.V
2019-01-29 17:03 ` Reza Arbab
2019-02-08 13:02 ` Michael Ellerman
2 siblings, 1 reply; 7+ messages in thread
From: Aneesh Kumar K.V @ 2019-01-29 15:07 UTC (permalink / raw)
To: Reza Arbab, linuxppc-dev; +Cc: Charles Johns, Paul Mackerras
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)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
2019-01-29 15:07 ` Aneesh Kumar K.V
@ 2019-01-29 17:03 ` Reza Arbab
2019-01-30 13:35 ` Michael Ellerman
0 siblings, 1 reply; 7+ messages in thread
From: Reza Arbab @ 2019-01-29 17:03 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: Charles Johns, Alexey Kardashevskiy, Paul Mackerras, linuxppc-dev
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
2019-01-29 17:03 ` Reza Arbab
@ 2019-01-30 13:35 ` Michael Ellerman
2019-02-01 5:02 ` Alexey Kardashevskiy
0 siblings, 1 reply; 7+ messages in thread
From: Michael Ellerman @ 2019-01-30 13:35 UTC (permalink / raw)
To: Reza Arbab, Aneesh Kumar K.V
Cc: Alexey Kardashevskiy, Charles Johns, linuxppc-dev, Paul Mackerras
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
2019-01-30 13:35 ` Michael Ellerman
@ 2019-02-01 5:02 ` Alexey Kardashevskiy
0 siblings, 0 replies; 7+ messages in thread
From: Alexey Kardashevskiy @ 2019-02-01 5:02 UTC (permalink / raw)
To: Michael Ellerman, Reza Arbab, Aneesh Kumar K.V
Cc: Charles Johns, linuxppc-dev, Paul Mackerras
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.
--
Alexey
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask
2019-01-28 17:31 [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask Reza Arbab
2019-01-29 1:39 ` Alexey Kardashevskiy
2019-01-29 15:07 ` Aneesh Kumar K.V
@ 2019-02-08 13:02 ` Michael Ellerman
2 siblings, 0 replies; 7+ messages in thread
From: Michael Ellerman @ 2019-02-08 13:02 UTC (permalink / raw)
To: Reza Arbab, linuxppc-dev; +Cc: Aneesh Kumar K.V, Charles Johns, Paul Mackerras
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 <crjohns@us.ibm.com>
> Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
Applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/865a9432d16fe2f40a1a52005fd30778
cheers
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-02-08 14:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-28 17:31 [PATCH] powerpc/mm: Add _PAGE_SAO to _PAGE_CACHE_CTL mask Reza Arbab
2019-01-29 1:39 ` Alexey Kardashevskiy
2019-01-29 15:07 ` Aneesh Kumar K.V
2019-01-29 17:03 ` Reza Arbab
2019-01-30 13:35 ` Michael Ellerman
2019-02-01 5:02 ` Alexey Kardashevskiy
2019-02-08 13:02 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).