Re: [RFC PATCH] powerpc/book3s/hash: Drop _PAGE_PRIVILEGED from PAGE_NONE

[Aneesh Kumar K V <aneesh.kumar@linux.ibm.com>]

On 11/13/23 5:17 PM, Nicholas Piggin wrote:
> On Mon Nov 13, 2023 at 8:45 PM AEST, Aneesh Kumar K V wrote:

....

>>>> diff --git a/arch/powerpc/mm/book3s64/hash_utils.c b/arch/powerpc/mm/book3s64/hash_utils.c
>>>> index ad2afa08e62e..b2eda22195f0 100644
>>>> --- a/arch/powerpc/mm/book3s64/hash_utils.c
>>>> +++ b/arch/powerpc/mm/book3s64/hash_utils.c
>>>> @@ -310,9 +310,26 @@ unsigned long htab_convert_pte_flags(unsigned long pteflags, unsigned long flags
>>>>  			else
>>>>  				rflags |= 0x3;
>>>>  		}
>>>> +		WARN_ON(!(pteflags & _PAGE_RWX));
>>>>  	} else {
>>>>  		if (pteflags & _PAGE_RWX)
>>>>  			rflags |= 0x2;
>>>> +		else {
>>>> +			/*
>>>> +			 * PAGE_NONE will get mapped to 0b110 (slb key 1 no access)
>>>> +			 * We picked 0b110 instead of 0b000 so that slb key 0 will
>>>> +			 * get only read only access for the same rflags.
>>>> +			 */
>>>> +			if (mmu_has_feature(MMU_FTR_KERNEL_RO))
>>>> +				rflags |= (HPTE_R_PP0 | 0x2);
>>>> +			/*
>>>> +			 * rflags = HPTE_R_N
>>>> +			 * Without KERNEL_RO feature this will result in slb
>>>> +			 * key 0 with read/write. But ISA only supports that.
>>>> +			 * There is no key 1 no-access and key 0 read-only
>>>> +			 * pp bit support.
>>>> +			 */
>>>> +		}
>>>>  		if (!((pteflags & _PAGE_WRITE) && (pteflags & _PAGE_DIRTY)))
>>>>  			rflags |= 0x1;
>>>>  	}
>>>
>>
>> V2 is also dropping the above change, because we will never have hash table entries inserted. 
>>
>> This is added to commit message.
>>
>>     Hash fault handling code did get some WARN_ON added because those
>>     functions are not expected to get called with _PAGE_READ cleared.
>>     commit 18061c17c8ec ("powerpc/mm: Update PROTFAULT handling in the page fault path")
>>     explains the details.
> 
> Should it be a WARN_ON_ONCE? Any useful way to recover from it? Could
> the added WARN come with some comments from that commit that explain
> it?
> 

This should never happen unless someone messed up check_pte_access(). The WARN_ON() is a way
to remind me not to add no access ppp mapping in the htab_convert_pte_flags() as I did above.
Initially I was planning to add only a comment, but then in the rare case of we getting it wrong
it is nice to do a console log.

-aneesh