Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org A620B602FC
Subject: Re: [PATCH v2 2/4] ioremap: Implement TLB_INV before huge mapping
To:     "Kani, Toshi" <toshi.kani@hpe.com>,
        "catalin.marinas@arm.com" <catalin.marinas@arm.com>,
        "will.deacon@arm.com" <will.deacon@arm.com>,
        "arnd@arndb.de" <arnd@arndb.de>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "ard.biesheuvel@linaro.org" <ard.biesheuvel@linaro.org>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "takahiro.akashi@linaro.org" <takahiro.akashi@linaro.org>,
        "james.morse@arm.com" <james.morse@arm.com>,
        "kristina.martsenko@arm.com" <kristina.martsenko@arm.com>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "marc.zyngier@arm.com" <marc.zyngier@arm.com>,
        "linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>
References: <1521117906-20107-1-git-send-email-cpandya@codeaurora.org>
 <1521117906-20107-3-git-send-email-cpandya@codeaurora.org>
 <1521130368.2693.177.camel@hpe.com>
From:   Chintan Pandya <cpandya@codeaurora.org>
Message-ID: <0cec2b79-1668-68d1-32db-531f5a8a9db2@codeaurora.org>
Date:   Fri, 16 Mar 2018 13:10:15 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101
 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <1521130368.2693.177.camel@hpe.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 3/15/2018 9:42 PM, Kani, Toshi wrote:
> On Thu, 2018-03-15 at 18:15 +0530, Chintan Pandya wrote:
>> Huge mapping changes PMD/PUD which could have
>> valid previous entries. This requires proper
>> TLB maintanance on some architectures, like
>> ARM64.
>>
>> Implent BBM (break-before-make) safe TLB
>> invalidation.
>>
>> Here, I've used flush_tlb_pgtable() instead
>> of flush_kernel_range() because invalidating
>> intermediate page_table entries could have
>> been optimized for specific arch. That's the
>> case with ARM64 at least.
>>
>> Signed-off-by: Chintan Pandya <cpandya@codeaurora.org>
>> ---
>>   lib/ioremap.c | 25 +++++++++++++++++++------
>>   1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/lib/ioremap.c b/lib/ioremap.c
>> index 54e5bba..55f8648 100644
>> --- a/lib/ioremap.c
>> +++ b/lib/ioremap.c
>> @@ -13,6 +13,7 @@
>>   #include <linux/export.h>
>>   #include <asm/cacheflush.h>
>>   #include <asm/pgtable.h>
>> +#include <asm-generic/tlb.h>
>>   
>>   #ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
>>   static int __read_mostly ioremap_p4d_capable;
>> @@ -80,6 +81,7 @@ static inline int ioremap_pmd_range(pud_t *pud, unsigned long addr,
>>   		unsigned long end, phys_addr_t phys_addr, pgprot_t prot)
>>   {
>>   	pmd_t *pmd;
>> +	pmd_t old_pmd;
>>   	unsigned long next;
>>   
>>   	phys_addr -= addr;
>> @@ -91,10 +93,15 @@ static inline int ioremap_pmd_range(pud_t *pud, unsigned long addr,
>>   
>>   		if (ioremap_pmd_enabled() &&
>>   		    ((next - addr) == PMD_SIZE) &&
>> -		    IS_ALIGNED(phys_addr + addr, PMD_SIZE) &&
>> -		    pmd_free_pte_page(pmd)) {
>> -			if (pmd_set_huge(pmd, phys_addr + addr, prot))
>> +		    IS_ALIGNED(phys_addr + addr, PMD_SIZE)) {
>> +			old_pmd = *pmd;
>> +			pmd_clear(pmd);
> 
> pmd_clear() is one of the operations pmd_free_pte_page() needs to do.
> See the x86 version.
> 
>> +			flush_tlb_pgtable(&init_mm, addr);
> 
> You can call it in pmd_free_pte_page() on arm64 as well.
> 
>> +			if (pmd_set_huge(pmd, phys_addr + addr, prot)) {
>> +				pmd_free_pte_page(&old_pmd);
>>   				continue;
>> +			} else
>> +				set_pmd(pmd, old_pmd);
> 
> I do not understand why you needed to make this change.
> pmd_free_pte_page() is defined as an arch-specific function so that you
> can additionally perform TLB purges on arm64.  Please try to make proper
> arm64 implementation of this interface.  And if you find any issue in
> this interface, please let me know.
TLB ops require VA at least. And this interface passes just the PMD/PUD.

Second is, if we clear the previous table entry inside the arch specific
code and then we fail in pmd/pud_set_huge, we can't fallback (x86 case).

So, we can do something like this (following Mark's suggestion),

	if (ioremap_pmd_enabled() &&
         	((next - addr) == PMD_SIZE) &&
		IS_ALIGNED(phys_addr + addr, PMD_SIZE) &&
		pmd_can_set_huge(pmd, phys_addr + addr, prot)) {
			/*
			 * Clear existing table entry,
			 * Invalidate,
			 * Free the page table
			 * inside this code
			 */
			pmd_free_pte_page(pmd, addr, addr + PMD_SIZE);
			pmd_set_huge(...) //without fail
			continue;
	}


> 
> Same for pud.
> 
> Thanks,
> -Toshi
> 

Chintan
-- 
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation
Collaborative Project