Date:   Tue, 27 Mar 2018 20:03:07 -0500
From:   Shanker Donthineni <shankerd@...eaurora.org>
To:     Will Deacon <will.deacon@....com>
Cc:     Mark Rutland <mark.rutland@....com>,
        Philip Elcan <pelcan@...eaurora.org>,
        Catalin Marinas <catalin.marinas@....com>,
        linux-kernel@...r.kernel.org,
        Thomas Speier <tspeier@...eaurora.org>,
        Robin Murphy <robin.murphy@....com>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH V2] arm64: tlbflush: avoid writing RES0 bits

Hi Will,

On 03/27/2018 12:36 PM, Will Deacon wrote:
> Hi Shanker,
> 
> On Tue, Mar 27, 2018 at 09:53:16AM -0500, Shanker Donthineni wrote:
>> On 03/27/2018 06:34 AM, Robin Murphy wrote:
>>> On 27/03/18 04:21, Philip Elcan wrote:
>>>> 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 <pelcan@...eaurora.org>
>>>> ---
>>>>   arch/arm64/include/asm/tlbflush.h | 23 ++++++++++++++++-------
>>>>   1 file changed, 16 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/include/asm/tlbflush.h b/arch/arm64/include/asm/tlbflush.h
>>>> index 9e82dd7..b1205e9 100644
>>>> --- a/arch/arm64/include/asm/tlbflush.h
>>>> +++ b/arch/arm64/include/asm/tlbflush.h
>>>> @@ -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;                        \
>>>> +    })
>>>
>>> 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;
>>>
>>> Acked-by: Robin Murphy <robin.murphy@....com>
>>>
>>
>> Tested-by: Shanker Donthineni <shankerd@...eaurora.org>
> 
> [...]
> 
>>>> @@ -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);


> Will
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

Powered by blists - more mailing lists