| Message ID | 1381128458-32140-2-git-send-email-victor.kamensky@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 07/10/13 08:47, Victor Kamensky wrote: > __flush_tlb_mm function need to use intermediate 'int' type 'asid' > variable int tlb_op macro call. Direct use of ASID macro produces > 64 bit unsigned long long type passed to inline assembler statement > as 'r' operand (32bit), and resulting behavior is not well specified. > It works in little endian case, but is broken in big endian case. In > big endian case gcc generate such code that 0 is passed to > 'mcr 15, 0, r4, cr8, cr3, {2}' operation. > > Note other functions like __local_flush_tlb_mm, and local_flush_tlb_mm > already use intermediate 'asid' variable in similar code. > > Signed-off-by: Victor Kamensky<victor.kamensky@linaro.org> do the __local_flush_tlb_mm() macros need to be changed to always ensure they take the lowest word of the two?
On 07/10/13 08:47, Victor Kamensky wrote: > __flush_tlb_mm function need to use intermediate 'int' type 'asid' > variable int tlb_op macro call. Direct use of ASID macro produces > 64 bit unsigned long long type passed to inline assembler statement > as 'r' operand (32bit), and resulting behavior is not well specified. > It works in little endian case, but is broken in big endian case. In > big endian case gcc generate such code that 0 is passed to > 'mcr 15, 0, r4, cr8, cr3, {2}' operation. > > Note other functions like __local_flush_tlb_mm, and local_flush_tlb_mm > already use intermediate 'asid' variable in similar code. > > Signed-off-by: Victor Kamensky<victor.kamensky@linaro.org> do the __local_flush_tlb_mm() macros need to be changed to always ensure they take the lowest word of the two?
On Sun, Oct 06, 2013 at 11:47:38PM -0700, Victor Kamensky wrote: > __flush_tlb_mm function need to use intermediate 'int' type 'asid' > variable int tlb_op macro call. Direct use of ASID macro produces > 64 bit unsigned long long type passed to inline assembler statement > as 'r' operand (32bit), and resulting behavior is not well specified. > It works in little endian case, but is broken in big endian case. In > big endian case gcc generate such code that 0 is passed to > 'mcr 15, 0, r4, cr8, cr3, {2}' operation. > > Note other functions like __local_flush_tlb_mm, and local_flush_tlb_mm > already use intermediate 'asid' variable in similar code. A much better solution would be to ensure that ASID() only returns the 'unsigned' type, not a long long type. #define ASID(mm) ((unsigned)(mm)->context.id.counter & ~ASID_MASK)
On Mon, Oct 07, 2013 at 11:45:24AM +0100, Russell King - ARM Linux wrote: > On Sun, Oct 06, 2013 at 11:47:38PM -0700, Victor Kamensky wrote: > > __flush_tlb_mm function need to use intermediate 'int' type 'asid' > > variable int tlb_op macro call. Direct use of ASID macro produces > > 64 bit unsigned long long type passed to inline assembler statement > > as 'r' operand (32bit), and resulting behavior is not well specified. > > It works in little endian case, but is broken in big endian case. In > > big endian case gcc generate such code that 0 is passed to > > 'mcr 15, 0, r4, cr8, cr3, {2}' operation. > > > > Note other functions like __local_flush_tlb_mm, and local_flush_tlb_mm > > already use intermediate 'asid' variable in similar code. > > A much better solution would be to ensure that ASID() only returns > the 'unsigned' type, not a long long type. > > #define ASID(mm) ((unsigned)(mm)->context.id.counter & ~ASID_MASK) Yup, that looks good to me. This is similar to the problem Ben already fixed in the mmid macro, so I think this should be included as part of his BE series. Speaking of which -- it's probably a good time to refresh and repost that if we're aiming for 3.13... Will
On 07/10/13 12:49, Will Deacon wrote: > On Mon, Oct 07, 2013 at 11:45:24AM +0100, Russell King - ARM Linux wrote: >> On Sun, Oct 06, 2013 at 11:47:38PM -0700, Victor Kamensky wrote: >>> __flush_tlb_mm function need to use intermediate 'int' type 'asid' >>> variable int tlb_op macro call. Direct use of ASID macro produces >>> 64 bit unsigned long long type passed to inline assembler statement >>> as 'r' operand (32bit), and resulting behavior is not well specified. >>> It works in little endian case, but is broken in big endian case. In >>> big endian case gcc generate such code that 0 is passed to >>> 'mcr 15, 0, r4, cr8, cr3, {2}' operation. >>> >>> Note other functions like __local_flush_tlb_mm, and local_flush_tlb_mm >>> already use intermediate 'asid' variable in similar code. >> >> A much better solution would be to ensure that ASID() only returns >> the 'unsigned' type, not a long long type. >> >> #define ASID(mm) ((unsigned)(mm)->context.id.counter& ~ASID_MASK) > > Yup, that looks good to me. This is similar to the problem Ben already fixed > in the mmid macro, so I think this should be included as part of his BE > series. > > Speaking of which -- it's probably a good time to refresh and repost that if > we're aiming for 3.13... I intended on rebasing the branch over the weekend, but ran out of time due to illness. I will try and look at a re-base tonight and if we can replace this ASID() issue then I can produce a new branch with it in.
diff --git a/arch/arm/include/asm/tlbflush.h b/arch/arm/include/asm/tlbflush.h index 3896026..b4d70ad 100644 --- a/arch/arm/include/asm/tlbflush.h +++ b/arch/arm/include/asm/tlbflush.h @@ -399,6 +399,7 @@ static inline void local_flush_tlb_mm(struct mm_struct *mm) static inline void __flush_tlb_mm(struct mm_struct *mm) { + const int asid = ASID(mm); const unsigned int __tlb_flag = __cpu_tlb_flags; if (tlb_flag(TLB_WB)) @@ -408,7 +409,7 @@ static inline void __flush_tlb_mm(struct mm_struct *mm) #ifdef CONFIG_ARM_ERRATA_720789 tlb_op(TLB_V7_UIS_ASID, "c8, c3, 0", 0); #else - tlb_op(TLB_V7_UIS_ASID, "c8, c3, 2", ASID(mm)); + tlb_op(TLB_V7_UIS_ASID, "c8, c3, 2", asid); #endif if (tlb_flag(TLB_BARRIER))
__flush_tlb_mm function need to use intermediate 'int' type 'asid' variable int tlb_op macro call. Direct use of ASID macro produces 64 bit unsigned long long type passed to inline assembler statement as 'r' operand (32bit), and resulting behavior is not well specified. It works in little endian case, but is broken in big endian case. In big endian case gcc generate such code that 0 is passed to 'mcr 15, 0, r4, cr8, cr3, {2}' operation. Note other functions like __local_flush_tlb_mm, and local_flush_tlb_mm already use intermediate 'asid' variable in similar code. Signed-off-by: Victor Kamensky <victor.kamensky@linaro.org> --- arch/arm/include/asm/tlbflush.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)