Re: [PATCH V2] arm64/mm: Implement pte_po_index() for permission overlay index

[Anshuman Khandual <anshuman.khandual@arm.com>]

On 4/29/25 22:15, Ryan Roberts wrote:
> On 29/04/2025 16:11, Will Deacon wrote:
>> On Tue, Apr 15, 2025 at 11:14:42AM +0530, Anshuman Khandual wrote:
>>> From: Ryan Roberts <ryan.roberts@arm.com>
>>>
>>> Previously pte_access_permitted() used FIELD_GET() directly to retrieve
>>> the permission overlay index from the pte. However, FIELD_GET() doesn't
>>> work for 128 bit quanitites. Since we are about to add support for D128
>>> pgtables, let's create a specific helper, pte_po_index() which can do
>>> the required mask and shift regardless of the data type width.
>>
>> You say:
>>
>> "we are about to add support for D128 pgtables"
> 
> Providing some context: Anshuman has a private branch that adds D128 pgtable
> support to the kernel (it does not yet do this for KVM). I originally created
> this patch to fix a bug on that branch, so the "we are about to add ..." comment
> really only makes sense in that context.
> 
> We are not yet ready to post D128 upstream - there are still a lot of questions
> to answer - but Anshuman has been posting some of the reshuffling and cleanups
> that prepare the way for D128 where (we think) it makes sense. The aim is to
> reduce the diff as much as we can.

Agreed. All these patches have been really harmless clean ups and re-orgs etc,
that do not affect existing 64 bit page table management or its functioning in
any manner. OTOH these changes help the kernel prepare for D128 enablement.

> 
>>
>> but all I've seen so far are piecemeal patches like this and it's hard
>> to know what to do with them, to be honest. Somebody could reasonably
>> turn up next week and clean this up to use FIELD_GET() again.
>>
>> Grepping around, I also see that the KVM page-table code uses the FIELD_*
>> macros on page-table entries, so perhaps we're better off adding support
>> for 128-bit types to those instead of trying to avoid them?
> 
> I think FIELD_* are always implicitly 64 bit, right? I could be wrong...
> 
> But I agree with the overall sentiment; where stuff is clearly crossing over
> into KVM, which hasn't been tackled yet, don't post until we have a good view on
> what KVM needs.
Keeping KVM changes separate was the motivation in V1 of this patch for similar
reasons. KVM uses FIELD_GET() for all the page table management purposes, hence
wanted to keep these S1 changes separate.