From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chintan Pandya Subject: Re: [PATCH v2 2/4] ioremap: Implement TLB_INV before huge mapping Date: Fri, 16 Mar 2018 13:10:15 +0530 Message-ID: <0cec2b79-1668-68d1-32db-531f5a8a9db2@codeaurora.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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1521130368.2693.177.camel@hpe.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: "Kani, Toshi" , "catalin.marinas@arm.com" , "will.deacon@arm.com" , "arnd@arndb.de" Cc: "linux-kernel@vger.kernel.org" , "ard.biesheuvel@linaro.org" , "tglx@linutronix.de" , "takahiro.akashi@linaro.org" , "james.morse@arm.com" , "kristina.martsenko@arm.com" , "mark.rutland@arm.com" , "akpm@linux-foundation.org" , "gregkh@linuxfoundation.org" , "linux-arm-kernel@lists.infradead.org" , "marc.zyngier@arm.com" , "linux-arch@vger.kernel.org" List-Id: linux-arch.vger.kernel.org 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 >> --- >> 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 >> #include >> #include >> +#include >> >> #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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.codeaurora.org ([198.145.29.96]:43598 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753125AbeCPHkX (ORCPT ); Fri, 16 Mar 2018 03:40:23 -0400 Subject: Re: [PATCH v2 2/4] ioremap: Implement TLB_INV before huge mapping 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 Message-ID: <0cec2b79-1668-68d1-32db-531f5a8a9db2@codeaurora.org> Date: Fri, 16 Mar 2018 13:10:15 +0530 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-arch-owner@vger.kernel.org List-ID: To: "Kani, Toshi" , "catalin.marinas@arm.com" , "will.deacon@arm.com" , "arnd@arndb.de" Cc: "linux-kernel@vger.kernel.org" , "ard.biesheuvel@linaro.org" , "tglx@linutronix.de" , "takahiro.akashi@linaro.org" , "james.morse@arm.com" , "kristina.martsenko@arm.com" , "mark.rutland@arm.com" , "akpm@linux-foundation.org" , "gregkh@linuxfoundation.org" , "linux-arm-kernel@lists.infradead.org" , "marc.zyngier@arm.com" , "linux-arch@vger.kernel.org" Message-ID: <20180316074015.dKE0MOJhaPu5AL8TsQpjgdmuBIYI7jO8S0diS1Uzcro@z> 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 >> --- >> 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 >> #include >> #include >> +#include >> >> #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