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.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: kvmarm@lists.linux.dev
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
This patch applies on v6.15-rc2

Changes in V2:

- Replaced FIELD_GET() in compute_s1_overlay_permissions() per Ryan

Changes in V1:

https://lore.kernel.org/linux-arm-kernel/20250410052021.1533180-1-anshuman.khandual@arm.com/

 arch/arm64/include/asm/pgtable-hwdef.h | 1 +
 arch/arm64/include/asm/pgtable-prot.h  | 2 ++
 arch/arm64/include/asm/pgtable.h       | 2 +-
 arch/arm64/kvm/at.c                    | 2 +-
 4 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
index f3b77deedfa2..028a164924df 100644
--- a/arch/arm64/include/asm/pgtable-hwdef.h
+++ b/arch/arm64/include/asm/pgtable-hwdef.h
@@ -211,6 +211,7 @@
 #define PTE_PO_IDX_2	(_AT(pteval_t, 1) << 62)
 
 #define PTE_PO_IDX_MASK		GENMASK_ULL(62, 60)
+#define PTE_PO_IDX_SHIFT	60
 
 
 /*
diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index 7830d031742e..b53bc241e4e7 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -136,6 +136,8 @@ static inline bool __pure lpa2_is_enabled(void)
 	((pte & BIT(PTE_PI_IDX_1)) >> (PTE_PI_IDX_1 - 1)) | \
 	((pte & BIT(PTE_PI_IDX_0)) >> (PTE_PI_IDX_0 - 0)))
 
+#define pte_po_index(pte)	((pte_val(pte) & PTE_PO_IDX_MASK) >> PTE_PO_IDX_SHIFT)
+
 /*
  * Page types used via Permission Indirection Extension (PIE). PIE uses
  * the USER, DBM, PXN and UXN bits to to generate an index which is used
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index d3b538be1500..41979c0e6c21 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -182,7 +182,7 @@ static inline bool por_el0_allows_pkey(u8 pkey, bool write, bool execute)
 	(((pte_val(pte) & (PTE_VALID | PTE_USER)) == (PTE_VALID | PTE_USER)) && (!(write) || pte_write(pte)))
 #define pte_access_permitted(pte, write) \
 	(pte_access_permitted_no_overlay(pte, write) && \
-	por_el0_allows_pkey(FIELD_GET(PTE_PO_IDX_MASK, pte_val(pte)), write, false))
+	por_el0_allows_pkey(pte_po_index(pte), write, false))
 #define pmd_access_permitted(pmd, write) \
 	(pte_access_permitted(pmd_pte(pmd), (write)))
 #define pud_access_permitted(pud, write) \
diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
index f74a66ce3064..66bee3f2d7c2 100644
--- a/arch/arm64/kvm/at.c
+++ b/arch/arm64/kvm/at.c
@@ -1073,7 +1073,7 @@ static void compute_s1_overlay_permissions(struct kvm_vcpu *vcpu,
 {
 	u8 idx, pov_perms, uov_perms;
 
-	idx = FIELD_GET(PTE_PO_IDX_MASK, wr->desc);
+	idx = pte_po_index(__pte(wr->desc));
 
 	switch (wi->regime) {
 	case TR_EL10:
-- 
2.25.1

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"

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?

Will

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.

> 
> 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.

Thanks,
Ryan

> 
> Will

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.

On Mon, May 05, 2025 at 02:20:46PM +0530, Anshuman Khandual wrote:
> 
> 
> 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.

In this case, I wouldn't call it harmless clean-up. You're actively
removing the use of helper macros which is inevitably going to result in
churn.

Please teach FIELD_GET() to work with 128-bit types instead.

Will