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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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