[V3,2/2] arm64/mm: Change offset base address in [pud|pmd]_free_[pmd|pte]_page()
diff mbox series

Message ID 1557377177-20695-3-git-send-email-anshuman.khandual@arm.com
State New
Headers show
Series
  • mm/ioremap: Check virtual address alignment
Related show

Commit Message

Anshuman Khandual May 9, 2019, 4:46 a.m. UTC
Pgtable page address can be fetched with [pmd|pte]_offset_[kernel] if input
address is PMD_SIZE or PTE_SIZE aligned. Input address is now guaranteed to
be aligned, hence fetched pgtable page address is always correct. But using
0UL as offset base address has been a standard practice across platforms.
It also makes more sense as it isolates pgtable page address computation
from input virtual address alignment. This does not change functionality.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
---
 arch/arm64/mm/mmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Catalin Marinas June 3, 2019, 3:36 p.m. UTC | #1
Hi Anshuman,


On Thu, May 09, 2019 at 10:16:17AM +0530, Anshuman Khandual wrote:
> Pgtable page address can be fetched with [pmd|pte]_offset_[kernel] if input
> address is PMD_SIZE or PTE_SIZE aligned. Input address is now guaranteed to
> be aligned, hence fetched pgtable page address is always correct. But using
> 0UL as offset base address has been a standard practice across platforms.
> It also makes more sense as it isolates pgtable page address computation
> from input virtual address alignment. This does not change functionality.
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>

What's the plan with this small series? I didn't find a v5 (unless I
deleted it by mistake). I can queue this patch through the arm64 tree or
they can both go in via the mm tree.
Catalin Marinas June 4, 2019, 2:24 p.m. UTC | #2
On Thu, May 09, 2019 at 10:16:17AM +0530, Anshuman Khandual wrote:
> Pgtable page address can be fetched with [pmd|pte]_offset_[kernel] if input
> address is PMD_SIZE or PTE_SIZE aligned. Input address is now guaranteed to
> be aligned, hence fetched pgtable page address is always correct. But using
> 0UL as offset base address has been a standard practice across platforms.
> It also makes more sense as it isolates pgtable page address computation
> from input virtual address alignment. This does not change functionality.
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> ---
>  arch/arm64/mm/mmu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index e97f018ff740..71bcb783aace 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -1005,7 +1005,7 @@ int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
>  		return 1;
>  	}
>  
> -	table = pte_offset_kernel(pmdp, addr);
> +	table = pte_offset_kernel(pmdp, 0UL);
>  	pmd_clear(pmdp);
>  	__flush_tlb_kernel_pgtable(addr);
>  	pte_free_kernel(NULL, table);
> @@ -1026,8 +1026,8 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>  		return 1;
>  	}
>  
> -	table = pmd_offset(pudp, addr);
> -	pmdp = table;
> +	table = pmd_offset(pudp, 0UL);
> +	pmdp = pmd_offset(pudp, addr);
>  	next = addr;
>  	end = addr + PUD_SIZE;
>  	do {

I have the same comment as last time:

https://lore.kernel.org/linux-arm-kernel/20190430161759.GI29799@arrakis.emea.arm.com/

I don't see why pmdp needs to be different from table. We get the
pointer to a pmd page and we want to iterate over it to free the pte
entries it contains. You can add a VM_WARN on addr alignment as in the
previous version of the patch but pmdp is just an iterator over table.
Anshuman Khandual June 6, 2019, 4:44 a.m. UTC | #3
On 06/04/2019 07:54 PM, Catalin Marinas wrote:
> On Thu, May 09, 2019 at 10:16:17AM +0530, Anshuman Khandual wrote:
>> Pgtable page address can be fetched with [pmd|pte]_offset_[kernel] if input
>> address is PMD_SIZE or PTE_SIZE aligned. Input address is now guaranteed to
>> be aligned, hence fetched pgtable page address is always correct. But using
>> 0UL as offset base address has been a standard practice across platforms.
>> It also makes more sense as it isolates pgtable page address computation
>> from input virtual address alignment. This does not change functionality.
>>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
>> ---
>>  arch/arm64/mm/mmu.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index e97f018ff740..71bcb783aace 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1005,7 +1005,7 @@ int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
>>  		return 1;
>>  	}
>>  
>> -	table = pte_offset_kernel(pmdp, addr);
>> +	table = pte_offset_kernel(pmdp, 0UL);
>>  	pmd_clear(pmdp);
>>  	__flush_tlb_kernel_pgtable(addr);
>>  	pte_free_kernel(NULL, table);
>> @@ -1026,8 +1026,8 @@ int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
>>  		return 1;
>>  	}
>>  
>> -	table = pmd_offset(pudp, addr);
>> -	pmdp = table;
>> +	table = pmd_offset(pudp, 0UL);
>> +	pmdp = pmd_offset(pudp, addr);
>>  	next = addr;
>>  	end = addr + PUD_SIZE;
>>  	do {
> 
> I have the same comment as last time:
> 
> https://lore.kernel.org/linux-arm-kernel/20190430161759.GI29799@arrakis.emea.arm.com/
> 
> I don't see why pmdp needs to be different from table. We get the
> pointer to a pmd page and we want to iterate over it to free the pte
> entries it contains. You can add a VM_WARN on addr alignment as in the
> previous version of the patch but pmdp is just an iterator over table.

Fair enough. I believe VM_WARN() is needed to check address alignment because
they are now guaranteed to be aligned because of the previous patch. I guess
we should probably drop this patch and consider only the previous one ?
Anshuman Khandual June 6, 2019, 8:08 a.m. UTC | #4
On 06/03/2019 09:06 PM, Catalin Marinas wrote:
> Hi Anshuman,
> 
> 
> On Thu, May 09, 2019 at 10:16:17AM +0530, Anshuman Khandual wrote:
>> Pgtable page address can be fetched with [pmd|pte]_offset_[kernel] if input
>> address is PMD_SIZE or PTE_SIZE aligned. Input address is now guaranteed to
>> be aligned, hence fetched pgtable page address is always correct. But using
>> 0UL as offset base address has been a standard practice across platforms.
>> It also makes more sense as it isolates pgtable page address computation
>> from input virtual address alignment. This does not change functionality.
>>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: James Morse <james.morse@arm.com>
>> Cc: Robin Murphy <robin.murphy@arm.com>
> 
> What's the plan with this small series? I didn't find a v5 (unless I
> deleted it by mistake). I can queue this patch through the arm64 tree or
> they can both go in via the mm tree.

As mentioned earlier [1] I believe the second patch is not needed anymore. Hence
only V4 of the first patch (https://patchwork.kernel.org/patch/10944191/) which
has a Reviewed-by tag should be considered.

[1] https://patchwork.kernel.org/patch/10936625/

Patch
diff mbox series

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index e97f018ff740..71bcb783aace 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -1005,7 +1005,7 @@  int pmd_free_pte_page(pmd_t *pmdp, unsigned long addr)
 		return 1;
 	}
 
-	table = pte_offset_kernel(pmdp, addr);
+	table = pte_offset_kernel(pmdp, 0UL);
 	pmd_clear(pmdp);
 	__flush_tlb_kernel_pgtable(addr);
 	pte_free_kernel(NULL, table);
@@ -1026,8 +1026,8 @@  int pud_free_pmd_page(pud_t *pudp, unsigned long addr)
 		return 1;
 	}
 
-	table = pmd_offset(pudp, addr);
-	pmdp = table;
+	table = pmd_offset(pudp, 0UL);
+	pmdp = pmd_offset(pudp, addr);
 	next = addr;
 	end = addr + PUD_SIZE;
 	do {