arm64: tlbflush: avoid writing RES0 bits
PATCH v2 1/1 arm64: tlbflush: avoid writing RES0 bits
Several of the bits of the TLBI register operand are RES0 per the ARM ARM, so TLBI operations should avoid writing non-zero values to these bits.
This patch adds a macro __TLBI_VADDR(addr, asid) that creates the operand register in the correct format and honors the RES0 bits.
--- arch/arm64/include/asm/tlbflush.h | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)
arch/arm64/include/asm/tlbflush.h +16 -7
@@ -60,6 +60,15 @@__tlbi (op, (arg) |USER_ASID_FLAG ); \ }while (0 ) +/* This macro creates a properly formatted VA operand for the TLBI */ +#define __TLBI_VADDR(addr, asid) \ + ({ \ +unsigned long __ta = (addr) >>12 ; \ + __ta &=GENMASK_ULL (43 ,0 ); \ + __ta |= (unsigned long )(asid) <<48 ; \ + __ta; \ + }) +/* * TLB Management * ==============
@@ -117,7 +126,7 @@ static inline void flush_tlb_all(void)static inline void flush_tlb_mm (struct mm_struct *mm) { -unsigned long asid =ASID (mm) <<48 ; +unsigned long asid =__TLBI_VADDR (0 ,ASID (mm));dsb (ishst);__tlbi (aside1is, asid);
@@ -128,7 +137,7 @@ static inline void flush_tlb_mm(struct mm_struct *mm)static inline void flush_tlb_page (struct vm_area_struct *vma,unsigned long uaddr) { -unsigned long addr = uaddr >>12 | (ASID (vma->vm_mm) <<48 ); +unsigned long addr =__TLBI_VADDR (uaddr,ASID (vma->vm_mm));dsb (ishst);__tlbi (vale1is, addr);
@@ -154,8 +163,8 @@ static inline void __flush_tlb_range(struct vm_area_struct *vma,return ; } - start = asid | (start >>12 ); - end = asid | (end >>12 ); + start =__TLBI_VADDR (start, asid); + end =__TLBI_VADDR (end, asid);dsb (ishst);for (addr = start; addr < end; addr +=1 << (PAGE_SHIFT -12 )) {
@@ -185,8 +194,8 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long endreturn ; } - start >>=12 ; - end >>=12 ; + start =__TLBI_VADDR (start,0 ); + end =__TLBI_VADDR (end,0 );dsb (ishst);for (addr = start; addr < end; addr +=1 << (PAGE_SHIFT -12 ))
@@ -202,7 +211,7 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long endstatic inline void __flush_tlb_pgtable (struct mm_struct *mm,unsigned long uaddr) { -unsigned long addr = uaddr >>12 | (ASID (mm) <<48 ); +unsigned long addr =__TLBI_VADDR (uaddr,ASID (mm));__tlbi (vae1is, addr);__tlbi_user (vae1is, addr);
Signature · 3 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 18 lines · Philip Elcan
Several of the bits of the TLBI register operand are RES0 per the ARM ARM, so TLBI operations should avoid writing non-zero values to these bits.
This patch adds a macro
__TLBI_VADDR(addr, asid)that creates the operand register in the correct format and honors the RES0 bits.Signed-off-by Philip Elcan <[email protected]>--- arch/arm64/include/asm/tlbflush.h | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-)arch/arm64/include/asm/tlbflush.h +0
-0@@ -60,6 +60,15 @@__tlbi (op, (arg) |USER_ASID_FLAG ); \
arch/arm64/include/asm/tlbflush.h line 70
Quoted 10 lines
arch/arm64/include/asm/tlbflush.h +8
-0}while (0 ) +/* This macro creates a properly formatted VA operand for the TLBI */ +#define __TLBI_VADDR(addr, asid) \ + ({ \ +unsigned long __ta = (addr) >>12 ; \ + __ta &=GENMASK_ULL (43 ,0 ); \ + __ta |= (unsigned long )(asid) <<48 ; \ + __ta; \ + })
I'd be inclined to make this a static inline function rather than a macro, since it doesn't need to do any wacky type-dodging, but either way the overall change now looks appropriate;
Thanks,
Robin.
Quoted 54 lines
arch/arm64/include/asm/tlbflush.h +8
-7+/* * TLB Management * ============== @@ -117,7 +126,7 @@ static inline void flush_tlb_all(void)static inline void flush_tlb_mm (struct mm_struct *mm) { -unsigned long asid =ASID (mm) <<48 ; +unsigned long asid =__TLBI_VADDR (0 ,ASID (mm));dsb (ishst);__tlbi (aside1is, asid);@@ -128,7 +137,7 @@ static inline void flush_tlb_mm(struct mm_struct *mm)static inline void flush_tlb_page (struct vm_area_struct *vma,unsigned long uaddr) { -unsigned long addr = uaddr >>12 | (ASID (vma->vm_mm) <<48 ); +unsigned long addr =__TLBI_VADDR (uaddr,ASID (vma->vm_mm));dsb (ishst);__tlbi (vale1is, addr);@@ -154,8 +163,8 @@ static inline void __flush_tlb_range(struct vm_area_struct *vma,return ; } - start = asid | (start >>12 ); - end = asid | (end >>12 ); + start =__TLBI_VADDR (start, asid); + end =__TLBI_VADDR (end, asid);dsb (ishst);for (addr = start; addr < end; addr +=1 << (PAGE_SHIFT -12 )) {@@ -185,8 +194,8 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long endreturn ; } - start >>=12 ; - end >>=12 ; + start =__TLBI_VADDR (start,0 ); + end =__TLBI_VADDR (end,0 );dsb (ishst);for (addr = start; addr < end; addr +=1 << (PAGE_SHIFT -12 ))@@ -202,7 +211,7 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long endstatic inline void __flush_tlb_pgtable (struct mm_struct *mm,unsigned long uaddr) { -unsigned long addr = uaddr >>12 | (ASID (mm) <<48 ); +unsigned long addr =__TLBI_VADDR (uaddr,ASID (mm));__tlbi (vae1is, addr);__tlbi_user (vae1is, addr);
Shanker Donthineni
Tested-byQuoted 31 lines · Robin Murphy
Quoted 19 lines · Philip Elcan
Several of the bits of the TLBI register operand are RES0 per the ARM ARM, so TLBI operations should avoid writing non-zero values to these bits.
This patch adds a macro
__TLBI_VADDR(addr, asid)that creates the operand register in the correct format and honors the RES0 bits.Signed-off-by Philip Elcan <[email protected]>---
?arch/arm64/include/asm/tlbflush.h| 23 ++++++++++++++++------- ? 1 file changed, 16 insertions(+), 7 deletions(-)arch/arm64/include/asm/tlbflush.h +0
-0@@ -60,6 +60,15 @@ ????????? __tlbi(op, (arg) | USER_ASID_FLAG);??????????????? \ ? } while (0)Quoted 8 lines
? +/* This macro creates a properly formatted VA operand for the TLBI */ +#define
__TLBI_VADDR(addr, asid)??????????????? \
+??? ({??????????????????????????? \
+??????? unsigned long__ta= (addr) >> 12;??????? \
+???????__ta&=GENMASK_ULL(43, 0);??????????? \
+???????__ta|= (unsigned long)(asid) << 48;??????? \
+???????__ta;??????????????????????? \
+??? })I'd be inclined to make this a static inline function rather than a macro, since it doesn't need to do any wacky type-dodging, but either way the overall change now looks appropriate;
Quoted 2 lines
Acked-by Robin Murphy <[email protected]>
Quoted 49 lines
Thanks,
Robin.Quoted 46 lines
+
? /*
?? *??? TLB Management
?? *??? ==============arch/arm64/include/asm/tlbflush.h +7
-5@@ -117,7 +126,7 @@ static inline void flush_tlb_all(void) ? ? static inline void flush_tlb_mm(struct mm_struct *mm) ? { -??? unsigned long asid = ASID(mm) << 48; +??? unsigned long asid = __TLBI_VADDR(0, ASID(mm)); ? ????? dsb(ishst); ????? __tlbi(aside1is, asid);@@ -128,7 +137,7 @@ static inline void flush_tlb_mm(struct mm_struct *mm) ? static inline void flush_tlb_page(struct vm_area_struct *vma, ??????????????????? unsigned long uaddr) ? { -??? unsigned long addr = uaddr >> 12 | (ASID(vma->vm_mm) << 48); +??? unsigned long addr = __TLBI_VADDR(uaddr, ASID(vma->vm_mm)); ? ????? dsb(ishst); ????? __tlbi(vale1is, addr);@@ -154,8 +163,8 @@ static inline void __flush_tlb_range(struct vm_area_struct *vma, ????????? return; ????? } ? -??? start = asid | (start >> 12); -??? end = asid | (end >> 12); +??? start = __TLBI_VADDR(start, asid); +??? end = __TLBI_VADDR(end, asid); ? ????? dsb(ishst); ????? for (addr = start; addr < end; addr += 1 << (PAGE_SHIFT - 12)) {@@ -185,8 +194,8 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end ????????? return; ????? } ? -??? start >>= 12; -??? end >>= 12; +??? start = __TLBI_VADDR(start, 0); +??? end = __TLBI_VADDR(end, 0); ? ????? dsb(ishst); ????? for (addr = start; addr < end; addr += 1 << (PAGE_SHIFT - 12))@@ -202,7 +211,7 @@ static inline void flush_tlb_kernel_range(unsigned long start, unsigned long end ? static inline void __flush_tlb_pgtable(struct mm_struct *mm, ???????????????????????? unsigned long uaddr) ? { -??? unsigned long addr = uaddr >> 12 | (ASID(mm) << 48); +??? unsigned long addr = __TLBI_VADDR(uaddr, ASID(mm)); ? ????? __tlbi(vae1is, addr); ????? __tlbi_user(vae1is, addr);
Signature · 3 lines
Shanker Donthineni Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi Shanker,
Quoted 38 lines · Shanker Donthineni
Quoted 33 lines · Robin Murphy
Quoted 19 lines · Philip Elcan
Several of the bits of the TLBI register operand are RES0 per the ARM ARM, so TLBI operations should avoid writing non-zero values to these bits.
This patch adds a macro
__TLBI_VADDR(addr, asid)that creates the operand register in the correct format and honors the RES0 bits.Signed-off-by Philip Elcan <[email protected]>---
arch/arm64/include/asm/tlbflush.h| 23 ++++++++++++++++-------1 file changed, 16 insertions(+), 7 deletions(-)arch/arm64/include/asm/tlbflush.h +0
-0@@ -60,6 +60,15 @@ __tlbi(op, (arg) | USER_ASID_FLAG); \ } while (0)Quoted 8 lines
+/* This macro creates a properly formatted VA operand for the TLBI */ +#define
__TLBI_VADDR(addr, asid)\
+ ({ \
+ unsigned long__ta= (addr) >> 12; \
+__ta&=GENMASK_ULL(43, 0); \
+__ta|= (unsigned long)(asid) << 48; \
+ __ta; \
+ })I'd be inclined to make this a static inline function rather than a macro, since it doesn't need to do any wacky type-dodging, but either way the overall change now looks appropriate;
Quoted 2 lines
Acked-by Robin Murphy <[email protected]>Tested-by Shanker Donthineni <[email protected]>
[...]
Quoted 7 lines
Quoted 7 lines
arch/arm64/include/asm/tlbflush.h static inline void __flush_tlb_range(struct vm_area_struct *vma, · line 167
Quoted 7 lines
arch/arm64/include/asm/tlbflush.h +2
-1@@ -154,8 +163,8 @@ static inline void __flush_tlb_range(struct vm_area_struct *vma, return; } - start = asid | (start >> 12); - end = asid | (end >> 12); + start = __TLBI_VADDR(start, asid); + end = __TLBI_VADDR(end, asid);
Can you test this bit too, please? ;)
Will
Hi Will,
Quoted 44 lines · Will Deacon
Hi Shanker,
Quoted 38 lines · Shanker Donthineni
Quoted 33 lines · Robin Murphy
Quoted 19 lines · Philip Elcan
Several of the bits of the TLBI register operand are RES0 per the ARM ARM, so TLBI operations should avoid writing non-zero values to these bits.
This patch adds a macro
__TLBI_VADDR(addr, asid)that creates the operand register in the correct format and honors the RES0 bits.Signed-off-by Philip Elcan <[email protected]>---
?arch/arm64/include/asm/tlbflush.h| 23 ++++++++++++++++------- ? 1 file changed, 16 insertions(+), 7 deletions(-)arch/arm64/include/asm/tlbflush.h +0
-0@@ -60,6 +60,15 @@ ????????? __tlbi(op, (arg) | USER_ASID_FLAG);??????????????? \ ? } while (0)Quoted 8 lines
? +/* This macro creates a properly formatted VA operand for the TLBI */ +#define
__TLBI_VADDR(addr, asid)??????????????? \
+??? ({??????????????????????????? \
+??????? unsigned long__ta= (addr) >> 12;??????? \
+???????__ta&=GENMASK_ULL(43, 0);??????????? \
+???????__ta|= (unsigned long)(asid) << 48;??????? \
+???????__ta;??????????????????????? \
+??? })I'd be inclined to make this a static inline function rather than a macro, since it doesn't need to do any wacky type-dodging, but either way the overall change now looks appropriate;
Quoted 2 lines
Acked-by Robin Murphy <[email protected]>Tested-by Shanker Donthineni <[email protected]>[...]
Quoted 10 lines
Quoted 7 lines
Quoted 7 lines
arch/arm64/include/asm/tlbflush.h static inline void __flush_tlb_range(struct vm_area_struct *vma, · line 167
Quoted 7 lines
arch/arm64/include/asm/tlbflush.h +2
-1@@ -154,8 +163,8 @@ static inline void __flush_tlb_range(struct vm_area_struct *vma, ????????? return; ????? } ? -??? start = asid | (start >> 12); -??? end = asid | (end >> 12); +??? start = __TLBI_VADDR(start, asid); +??? end = __TLBI_VADDR(end, asid);Can you test this bit too, please? ;)
I've verified the basic boot functionality on QDF2400 platform. But I can see now after your comments, it leads to TLB conflicts because of ASID is truncated to zero due to two times 48bit shift.
Thanks for catching this one.
@@ -146,7 +155,7 @@ static inline void __flush_tlb_range(structvm_area_struct *
unsigned long start, unsigned long end, bool last_level)
{
- unsigned long asid = ASID(vma->vm_mm) << 48;
+ unsigned long asid = ASID(vma->vm_mm);
unsigned long addr;
if ((end - start) > MAX_TLB_RANGE) {
@@ -154,8 +163,8 @@ static inline void __flush_tlb_range(struct
vm_area_struct *
return;
}- start = asid | (start >> 12); - end = asid | (end >> 12); + start = __TLBI_VADDR(start, asid); + end = __TLBI_VADDR(end, asid);
Quoted 7 lines
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 3 lines
Shanker Donthineni Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 84 lines · Shanker Donthineni
Hi Will,
Quoted 44 lines · Will Deacon
Hi Shanker,
Quoted 38 lines · Shanker Donthineni
Quoted 33 lines · Robin Murphy
Quoted 19 lines · Philip Elcan
Several of the bits of the TLBI register operand are RES0 per the ARM ARM, so TLBI operations should avoid writing non-zero values to these bits.
This patch adds a macro
__TLBI_VADDR(addr, asid)that creates the operand register in the correct format and honors the RES0 bits.Signed-off-by Philip Elcan <[email protected]>---
?arch/arm64/include/asm/tlbflush.h| 23 ++++++++++++++++------- ? 1 file changed, 16 insertions(+), 7 deletions(-)arch/arm64/include/asm/tlbflush.h +0
-0@@ -60,6 +60,15 @@ ????????? __tlbi(op, (arg) | USER_ASID_FLAG);??????????????? \ ? } while (0)Quoted 8 lines
? +/* This macro creates a properly formatted VA operand for the TLBI */ +#define
__TLBI_VADDR(addr, asid)??????????????? \
+??? ({??????????????????????????? \
+??????? unsigned long__ta= (addr) >> 12;??????? \
+???????__ta&=GENMASK_ULL(43, 0);??????????? \
+???????__ta|= (unsigned long)(asid) << 48;??????? \
+???????__ta;??????????????????????? \
+??? })I'd be inclined to make this a static inline function rather than a macro, since it doesn't need to do any wacky type-dodging, but either way the overall change now looks appropriate;
Quoted 2 lines
Acked-by Robin Murphy <[email protected]>Tested-by Shanker Donthineni <[email protected]>[...]
Quoted 10 lines
Quoted 7 lines
Quoted 7 lines
arch/arm64/include/asm/tlbflush.h static inline void __flush_tlb_range(struct vm_area_struct *vma, · line 167
Quoted 7 lines
arch/arm64/include/asm/tlbflush.h +2
-1@@ -154,8 +163,8 @@ static inline void __flush_tlb_range(struct vm_area_struct *vma, ????????? return; ????? } ? -??? start = asid | (start >> 12); -??? end = asid | (end >> 12); +??? start = __TLBI_VADDR(start, asid); +??? end = __TLBI_VADDR(end, asid);Can you test this bit too, please? ;)
I've verified the basic boot functionality on QDF2400 platform. But I can see now after your comments, it leads to TLB conflicts because of ASID is truncated to zero due to two times 48bit shift.
Thanks for catching this one.
(quoted diff) +1
-1@@ -146,7 +155,7 @@ static inline void __flush_tlb_range(structvm_area_struct * unsigned long start, unsigned long end, bool last_level) { - unsigned long asid = ASID(vma->vm_mm) << 48; + unsigned long asid = ASID(vma->vm_mm); unsigned long addr; if ((end - start) > MAX_TLB_RANGE) {@@ -154,8 +163,8 @@ static inline void __flush_tlb_range(struct
vm_area_struct*return; }arch/arm64/include/asm/tlbflush.h +2
-2- start = asid | (start >>12 ); - end = asid | (end >>12 ); + start =__TLBI_VADDR (start, asid); + end =__TLBI_VADDR (end, asid);
Quoted 8 lines
Quoted 7 lines
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Thanks for catching that. I'll address with a v3 patch.
Philip
Signature · 4 lines
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Quoted 13 lines · Shanker Donthineni
Quoted 11 lines · Will Deacon
Quoted 7 lines · Shanker Donthineni
Quoted 7 lines
arch/arm64/include/asm/tlbflush.h static inline void __flush_tlb_range(struct vm_area_struct *vma, · line 167
Quoted 7 lines
arch/arm64/include/asm/tlbflush.h +2
-1@@ -154,8 +163,8 @@ static inline void __flush_tlb_range(struct vm_area_struct *vma, return; } - start = asid | (start >> 12); - end = asid | (end >> 12); + start = __TLBI_VADDR(start, asid); + end = __TLBI_VADDR(end, asid);Can you test this bit too, please? ;)
Quoted 5 lines
I've verified the basic boot functionality on QDF2400 platform. But I can see now after your comments, it leads to TLB conflicts because of ASID is truncated to zero due to two times 48bit shift.
Thanks for catching this one.
I just noticed it during review. In general, if you're making changes relating to virtual memory stuff I'd *strongly* advise you to do more than a basic boot test. It's amazing how much appears to run fine when stuff like TLB invalidation is completely broken.
Anyway, thanks for turning around a new version so quickly.
Will