[PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Anshuman Khandual]

Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
entry does not have a pointer to the next level page table. This had been
made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
section maps"). Hence explicitly check for a table entry rather than just
testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
[pud|pmd]_table() making the semantics clear.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
This applies on v5.13-rc1.

 arch/arm64/include/asm/pgtable.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 25f5c04b43ce..69f8183bef29 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -509,13 +509,12 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
 
 #define pmd_none(pmd)		(!pmd_val(pmd))
 
-#define pmd_bad(pmd)		(!(pmd_val(pmd) & PMD_TABLE_BIT))
-
 #define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
 				 PMD_TYPE_TABLE)
 #define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
 				 PMD_TYPE_SECT)
 #define pmd_leaf(pmd)		pmd_sect(pmd)
+#define pmd_bad(pmd)		(!pmd_table(pmd))
 
 #define pmd_leaf_size(pmd)	(pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
 #define pte_leaf_size(pte)	(pte_cont(pte) ? CONT_PTE_SIZE : PAGE_SIZE)
@@ -602,7 +601,7 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
 	pr_err("%s:%d: bad pmd %016llx.\n", __FILE__, __LINE__, pmd_val(e))
 
 #define pud_none(pud)		(!pud_val(pud))
-#define pud_bad(pud)		(!(pud_val(pud) & PUD_TABLE_BIT))
+#define pud_bad(pud)		(!pud_table(pud))
 #define pud_present(pud)	pte_present(pud_pte(pud))
 #define pud_leaf(pud)		pud_sect(pud)
 #define pud_valid(pud)		pte_valid(pud_pte(pud))
-- 
2.20.1

[PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Anshuman Khandual]

Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
entry does not have a pointer to the next level page table. This had been
made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
section maps"). Hence explicitly check for a table entry rather than just
testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
[pud|pmd]_table() making the semantics clear.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
This applies on v5.13-rc1.

 arch/arm64/include/asm/pgtable.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 25f5c04b43ce..69f8183bef29 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -509,13 +509,12 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
 
 #define pmd_none(pmd)		(!pmd_val(pmd))
 
-#define pmd_bad(pmd)		(!(pmd_val(pmd) & PMD_TABLE_BIT))
-
 #define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
 				 PMD_TYPE_TABLE)
 #define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
 				 PMD_TYPE_SECT)
 #define pmd_leaf(pmd)		pmd_sect(pmd)
+#define pmd_bad(pmd)		(!pmd_table(pmd))
 
 #define pmd_leaf_size(pmd)	(pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
 #define pte_leaf_size(pte)	(pte_cont(pte) ? CONT_PTE_SIZE : PAGE_SIZE)
@@ -602,7 +601,7 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
 	pr_err("%s:%d: bad pmd %016llx.\n", __FILE__, __LINE__, pmd_val(e))
 
 #define pud_none(pud)		(!pud_val(pud))
-#define pud_bad(pud)		(!(pud_val(pud) & PUD_TABLE_BIT))
+#define pud_bad(pud)		(!pud_table(pud))
 #define pud_present(pud)	pte_present(pud_pte(pud))
 #define pud_leaf(pud)		pud_sect(pud)
 #define pud_valid(pud)		pte_valid(pud_pte(pud))
-- 
2.20.1


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

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Mark Rutland]

On Mon, May 10, 2021 at 04:37:51PM +0530, Anshuman Khandual wrote:
> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
> entry does not have a pointer to the next level page table. This had been
> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
> section maps"). Hence explicitly check for a table entry rather than just
> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
> [pud|pmd]_table() making the semantics clear.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

I have no strong feelings either way, so: 

Acked-by: Mark Rutland <mark.rutland@arm.com>

... that said, I think that the "bad" naming is unclear and misleading,
and it'd be really nice if we could clean that up treewide with
something clearer than "bad".

It does seem that would roughly fit p??_leaf() if we had
p??_clear_leaf() and p??_none_or_clear_leaf() helpers.

Thanks,
Mark.

> ---
> This applies on v5.13-rc1.
> 
>  arch/arm64/include/asm/pgtable.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 25f5c04b43ce..69f8183bef29 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -509,13 +509,12 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>  
>  #define pmd_none(pmd)		(!pmd_val(pmd))
>  
> -#define pmd_bad(pmd)		(!(pmd_val(pmd) & PMD_TABLE_BIT))
> -
>  #define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>  				 PMD_TYPE_TABLE)
>  #define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>  				 PMD_TYPE_SECT)
>  #define pmd_leaf(pmd)		pmd_sect(pmd)
> +#define pmd_bad(pmd)		(!pmd_table(pmd))
>  
>  #define pmd_leaf_size(pmd)	(pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
>  #define pte_leaf_size(pte)	(pte_cont(pte) ? CONT_PTE_SIZE : PAGE_SIZE)
> @@ -602,7 +601,7 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
>  	pr_err("%s:%d: bad pmd %016llx.\n", __FILE__, __LINE__, pmd_val(e))
>  
>  #define pud_none(pud)		(!pud_val(pud))
> -#define pud_bad(pud)		(!(pud_val(pud) & PUD_TABLE_BIT))
> +#define pud_bad(pud)		(!pud_table(pud))
>  #define pud_present(pud)	pte_present(pud_pte(pud))
>  #define pud_leaf(pud)		pud_sect(pud)
>  #define pud_valid(pud)		pte_valid(pud_pte(pud))
> -- 
> 2.20.1
> 

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Mark Rutland]

On Mon, May 10, 2021 at 04:37:51PM +0530, Anshuman Khandual wrote:
> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
> entry does not have a pointer to the next level page table. This had been
> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
> section maps"). Hence explicitly check for a table entry rather than just
> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
> [pud|pmd]_table() making the semantics clear.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

I have no strong feelings either way, so: 

Acked-by: Mark Rutland <mark.rutland@arm.com>

... that said, I think that the "bad" naming is unclear and misleading,
and it'd be really nice if we could clean that up treewide with
something clearer than "bad".

It does seem that would roughly fit p??_leaf() if we had
p??_clear_leaf() and p??_none_or_clear_leaf() helpers.

Thanks,
Mark.

> ---
> This applies on v5.13-rc1.
> 
>  arch/arm64/include/asm/pgtable.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 25f5c04b43ce..69f8183bef29 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -509,13 +509,12 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>  
>  #define pmd_none(pmd)		(!pmd_val(pmd))
>  
> -#define pmd_bad(pmd)		(!(pmd_val(pmd) & PMD_TABLE_BIT))
> -
>  #define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>  				 PMD_TYPE_TABLE)
>  #define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>  				 PMD_TYPE_SECT)
>  #define pmd_leaf(pmd)		pmd_sect(pmd)
> +#define pmd_bad(pmd)		(!pmd_table(pmd))
>  
>  #define pmd_leaf_size(pmd)	(pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
>  #define pte_leaf_size(pte)	(pte_cont(pte) ? CONT_PTE_SIZE : PAGE_SIZE)
> @@ -602,7 +601,7 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
>  	pr_err("%s:%d: bad pmd %016llx.\n", __FILE__, __LINE__, pmd_val(e))
>  
>  #define pud_none(pud)		(!pud_val(pud))
> -#define pud_bad(pud)		(!(pud_val(pud) & PUD_TABLE_BIT))
> +#define pud_bad(pud)		(!pud_table(pud))
>  #define pud_present(pud)	pte_present(pud_pte(pud))
>  #define pud_leaf(pud)		pud_sect(pud)
>  #define pud_valid(pud)		pte_valid(pud_pte(pud))
> -- 
> 2.20.1
> 

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

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Anshuman Khandual]

On 5/10/21 8:13 PM, Mark Rutland wrote:
> On Mon, May 10, 2021 at 04:37:51PM +0530, Anshuman Khandual wrote:
>> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
>> entry does not have a pointer to the next level page table. This had been
>> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
>> section maps"). Hence explicitly check for a table entry rather than just
>> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
>> [pud|pmd]_table() making the semantics clear.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> 
> I have no strong feelings either way, so: 
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> ... that said, I think that the "bad" naming is unclear and misleading,
> and it'd be really nice if we could clean that up treewide with
> something clearer than "bad".

Agreed, the name is misleading.

> 
> It does seem that would roughly fit p??_leaf() if we had

But what if the platform does not support huge page aka leaf mapping
at the given level ? Also a non table i.e bad entry might not always
mean a leaf/section/huge page mapping, it could simply imply that the
entry is not just pointing to next level and might be just in an bad
intermediate or invalid state.

> p??_clear_leaf() and p??_none_or_clear_leaf() helpers.

Could you please elaborate how these new helpers might help define
pxx_bad() ?

> 
> Thanks,
> Mark.
> 
>> ---
>> This applies on v5.13-rc1.
>>
>>  arch/arm64/include/asm/pgtable.h | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 25f5c04b43ce..69f8183bef29 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -509,13 +509,12 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>>  
>>  #define pmd_none(pmd)		(!pmd_val(pmd))
>>  
>> -#define pmd_bad(pmd)		(!(pmd_val(pmd) & PMD_TABLE_BIT))
>> -
>>  #define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>>  				 PMD_TYPE_TABLE)
>>  #define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>>  				 PMD_TYPE_SECT)
>>  #define pmd_leaf(pmd)		pmd_sect(pmd)
>> +#define pmd_bad(pmd)		(!pmd_table(pmd))
>>  
>>  #define pmd_leaf_size(pmd)	(pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
>>  #define pte_leaf_size(pte)	(pte_cont(pte) ? CONT_PTE_SIZE : PAGE_SIZE)
>> @@ -602,7 +601,7 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
>>  	pr_err("%s:%d: bad pmd %016llx.\n", __FILE__, __LINE__, pmd_val(e))
>>  
>>  #define pud_none(pud)		(!pud_val(pud))
>> -#define pud_bad(pud)		(!(pud_val(pud) & PUD_TABLE_BIT))
>> +#define pud_bad(pud)		(!pud_table(pud))
>>  #define pud_present(pud)	pte_present(pud_pte(pud))
>>  #define pud_leaf(pud)		pud_sect(pud)
>>  #define pud_valid(pud)		pte_valid(pud_pte(pud))
>> -- 
>> 2.20.1
>>

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Anshuman Khandual]

On 5/10/21 8:13 PM, Mark Rutland wrote:
> On Mon, May 10, 2021 at 04:37:51PM +0530, Anshuman Khandual wrote:
>> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
>> entry does not have a pointer to the next level page table. This had been
>> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
>> section maps"). Hence explicitly check for a table entry rather than just
>> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
>> [pud|pmd]_table() making the semantics clear.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> 
> I have no strong feelings either way, so: 
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>
> 
> ... that said, I think that the "bad" naming is unclear and misleading,
> and it'd be really nice if we could clean that up treewide with
> something clearer than "bad".

Agreed, the name is misleading.

> 
> It does seem that would roughly fit p??_leaf() if we had

But what if the platform does not support huge page aka leaf mapping
at the given level ? Also a non table i.e bad entry might not always
mean a leaf/section/huge page mapping, it could simply imply that the
entry is not just pointing to next level and might be just in an bad
intermediate or invalid state.

> p??_clear_leaf() and p??_none_or_clear_leaf() helpers.

Could you please elaborate how these new helpers might help define
pxx_bad() ?

> 
> Thanks,
> Mark.
> 
>> ---
>> This applies on v5.13-rc1.
>>
>>  arch/arm64/include/asm/pgtable.h | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 25f5c04b43ce..69f8183bef29 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -509,13 +509,12 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>>  
>>  #define pmd_none(pmd)		(!pmd_val(pmd))
>>  
>> -#define pmd_bad(pmd)		(!(pmd_val(pmd) & PMD_TABLE_BIT))
>> -
>>  #define pmd_table(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>>  				 PMD_TYPE_TABLE)
>>  #define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>>  				 PMD_TYPE_SECT)
>>  #define pmd_leaf(pmd)		pmd_sect(pmd)
>> +#define pmd_bad(pmd)		(!pmd_table(pmd))
>>  
>>  #define pmd_leaf_size(pmd)	(pmd_cont(pmd) ? CONT_PMD_SIZE : PMD_SIZE)
>>  #define pte_leaf_size(pte)	(pte_cont(pte) ? CONT_PTE_SIZE : PAGE_SIZE)
>> @@ -602,7 +601,7 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
>>  	pr_err("%s:%d: bad pmd %016llx.\n", __FILE__, __LINE__, pmd_val(e))
>>  
>>  #define pud_none(pud)		(!pud_val(pud))
>> -#define pud_bad(pud)		(!(pud_val(pud) & PUD_TABLE_BIT))
>> +#define pud_bad(pud)		(!pud_table(pud))
>>  #define pud_present(pud)	pte_present(pud_pte(pud))
>>  #define pud_leaf(pud)		pud_sect(pud)
>>  #define pud_valid(pud)		pte_valid(pud_pte(pud))
>> -- 
>> 2.20.1
>>

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

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Mark Rutland]

On Tue, May 11, 2021 at 09:21:46AM +0530, Anshuman Khandual wrote:
> 
> On 5/10/21 8:13 PM, Mark Rutland wrote:
> > On Mon, May 10, 2021 at 04:37:51PM +0530, Anshuman Khandual wrote:
> >> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
> >> entry does not have a pointer to the next level page table. This had been
> >> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
> >> section maps"). Hence explicitly check for a table entry rather than just
> >> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
> >> [pud|pmd]_table() making the semantics clear.
> >>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Cc: Will Deacon <will@kernel.org>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> > 
> > I have no strong feelings either way, so: 
> > 
> > Acked-by: Mark Rutland <mark.rutland@arm.com>
> > 
> > ... that said, I think that the "bad" naming is unclear and misleading,
> > and it'd be really nice if we could clean that up treewide with
> > something clearer than "bad".
> 
> Agreed, the name is misleading.
> 
> > It does seem that would roughly fit p??_leaf() if we had
> 
> But what if the platform does not support huge page aka leaf mapping
> at the given level ? Also a non table i.e bad entry might not always
> mean a leaf/section/huge page mapping, it could simply imply that the
> entry is not just pointing to next level and might be just in an bad
> intermediate or invalid state.

Ah, so that's also covering swap entries, too? It's not entirely clear
to me what "bad intermediate or invalid state" means, because I assume
it's not arbitrary junk or this wouldn't be sound genrally.

I had assumed it was only covering *valid* non-table entries.

> > p??_clear_leaf() and p??_none_or_clear_leaf() helpers.
> 
> Could you please elaborate how these new helpers might help define
> pxx_bad() ?

This was based on my (evidently wrong) prior understanding above.

Thanks,
Mark.

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Mark Rutland]

On Tue, May 11, 2021 at 09:21:46AM +0530, Anshuman Khandual wrote:
> 
> On 5/10/21 8:13 PM, Mark Rutland wrote:
> > On Mon, May 10, 2021 at 04:37:51PM +0530, Anshuman Khandual wrote:
> >> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
> >> entry does not have a pointer to the next level page table. This had been
> >> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
> >> section maps"). Hence explicitly check for a table entry rather than just
> >> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
> >> [pud|pmd]_table() making the semantics clear.
> >>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Cc: Will Deacon <will@kernel.org>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> Cc: linux-kernel@vger.kernel.org
> >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> > 
> > I have no strong feelings either way, so: 
> > 
> > Acked-by: Mark Rutland <mark.rutland@arm.com>
> > 
> > ... that said, I think that the "bad" naming is unclear and misleading,
> > and it'd be really nice if we could clean that up treewide with
> > something clearer than "bad".
> 
> Agreed, the name is misleading.
> 
> > It does seem that would roughly fit p??_leaf() if we had
> 
> But what if the platform does not support huge page aka leaf mapping
> at the given level ? Also a non table i.e bad entry might not always
> mean a leaf/section/huge page mapping, it could simply imply that the
> entry is not just pointing to next level and might be just in an bad
> intermediate or invalid state.

Ah, so that's also covering swap entries, too? It's not entirely clear
to me what "bad intermediate or invalid state" means, because I assume
it's not arbitrary junk or this wouldn't be sound genrally.

I had assumed it was only covering *valid* non-table entries.

> > p??_clear_leaf() and p??_none_or_clear_leaf() helpers.
> 
> Could you please elaborate how these new helpers might help define
> pxx_bad() ?

This was based on my (evidently wrong) prior understanding above.

Thanks,
Mark.

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

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Anshuman Khandual]

On 5/11/21 7:37 PM, Mark Rutland wrote:
> On Tue, May 11, 2021 at 09:21:46AM +0530, Anshuman Khandual wrote:
>>
>> On 5/10/21 8:13 PM, Mark Rutland wrote:
>>> On Mon, May 10, 2021 at 04:37:51PM +0530, Anshuman Khandual wrote:
>>>> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
>>>> entry does not have a pointer to the next level page table. This had been
>>>> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
>>>> section maps"). Hence explicitly check for a table entry rather than just
>>>> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
>>>> [pud|pmd]_table() making the semantics clear.
>>>>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>
>>> I have no strong feelings either way, so: 
>>>
>>> Acked-by: Mark Rutland <mark.rutland@arm.com>
>>>
>>> ... that said, I think that the "bad" naming is unclear and misleading,
>>> and it'd be really nice if we could clean that up treewide with
>>> something clearer than "bad".
>>
>> Agreed, the name is misleading.
>>
>>> It does seem that would roughly fit p??_leaf() if we had
>>
>> But what if the platform does not support huge page aka leaf mapping
>> at the given level ? Also a non table i.e bad entry might not always
>> mean a leaf/section/huge page mapping, it could simply imply that the
>> entry is not just pointing to next level and might be just in an bad
>> intermediate or invalid state.
> 
> Ah, so that's also covering swap entries, too? It's not entirely clear
> to me what "bad intermediate or invalid state" means, because I assume
> it's not arbitrary junk or this wouldn't be sound genrally.

Intermediate states like swap, migration or probably even splitting THP.
Though I am not really sure whether pxx_bad() only gets used for valid
table or leaf entries i.e things which are mapped. Hence checking just
for non table entry is better and even safer, than looking out for what
other states the entry could be in.

> 
> I had assumed it was only covering *valid* non-table entries.
> 
>>> p??_clear_leaf() and p??_none_or_clear_leaf() helpers.
>>
>> Could you please elaborate how these new helpers might help define
>> pxx_bad() ?
> 
> This was based on my (evidently wrong) prior understanding above.
> 
> Thanks,
> Mark.
> 

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Anshuman Khandual]

On 5/11/21 7:37 PM, Mark Rutland wrote:
> On Tue, May 11, 2021 at 09:21:46AM +0530, Anshuman Khandual wrote:
>>
>> On 5/10/21 8:13 PM, Mark Rutland wrote:
>>> On Mon, May 10, 2021 at 04:37:51PM +0530, Anshuman Khandual wrote:
>>>> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
>>>> entry does not have a pointer to the next level page table. This had been
>>>> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
>>>> section maps"). Hence explicitly check for a table entry rather than just
>>>> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
>>>> [pud|pmd]_table() making the semantics clear.
>>>>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>
>>> I have no strong feelings either way, so: 
>>>
>>> Acked-by: Mark Rutland <mark.rutland@arm.com>
>>>
>>> ... that said, I think that the "bad" naming is unclear and misleading,
>>> and it'd be really nice if we could clean that up treewide with
>>> something clearer than "bad".
>>
>> Agreed, the name is misleading.
>>
>>> It does seem that would roughly fit p??_leaf() if we had
>>
>> But what if the platform does not support huge page aka leaf mapping
>> at the given level ? Also a non table i.e bad entry might not always
>> mean a leaf/section/huge page mapping, it could simply imply that the
>> entry is not just pointing to next level and might be just in an bad
>> intermediate or invalid state.
> 
> Ah, so that's also covering swap entries, too? It's not entirely clear
> to me what "bad intermediate or invalid state" means, because I assume
> it's not arbitrary junk or this wouldn't be sound genrally.

Intermediate states like swap, migration or probably even splitting THP.
Though I am not really sure whether pxx_bad() only gets used for valid
table or leaf entries i.e things which are mapped. Hence checking just
for non table entry is better and even safer, than looking out for what
other states the entry could be in.

> 
> I had assumed it was only covering *valid* non-table entries.
> 
>>> p??_clear_leaf() and p??_none_or_clear_leaf() helpers.
>>
>> Could you please elaborate how these new helpers might help define
>> pxx_bad() ?
> 
> This was based on my (evidently wrong) prior understanding above.
> 
> Thanks,
> Mark.
> 

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

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Catalin Marinas]

On Thu, May 13, 2021 at 10:44:04AM +0530, Anshuman Khandual wrote:
> On 5/11/21 7:37 PM, Mark Rutland wrote:
> > On Tue, May 11, 2021 at 09:21:46AM +0530, Anshuman Khandual wrote:
> >> On 5/10/21 8:13 PM, Mark Rutland wrote:
> >>> On Mon, May 10, 2021 at 04:37:51PM +0530, Anshuman Khandual wrote:
> >>>> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
> >>>> entry does not have a pointer to the next level page table. This had been
> >>>> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
> >>>> section maps"). Hence explicitly check for a table entry rather than just
> >>>> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
> >>>> [pud|pmd]_table() making the semantics clear.
> >>>>
> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >>>> Cc: Will Deacon <will@kernel.org>
> >>>> Cc: Mark Rutland <mark.rutland@arm.com>
> >>>> Cc: linux-arm-kernel@lists.infradead.org
> >>>> Cc: linux-kernel@vger.kernel.org
> >>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >>>
> >>> I have no strong feelings either way, so: 
> >>>
> >>> Acked-by: Mark Rutland <mark.rutland@arm.com>
> >>>
> >>> ... that said, I think that the "bad" naming is unclear and misleading,
> >>> and it'd be really nice if we could clean that up treewide with
> >>> something clearer than "bad".
> >>
> >> Agreed, the name is misleading.
> >>
> >>> It does seem that would roughly fit p??_leaf() if we had
> >>
> >> But what if the platform does not support huge page aka leaf mapping
> >> at the given level ? Also a non table i.e bad entry might not always
> >> mean a leaf/section/huge page mapping, it could simply imply that the
> >> entry is not just pointing to next level and might be just in an bad
> >> intermediate or invalid state.
> > 
> > Ah, so that's also covering swap entries, too? It's not entirely clear
> > to me what "bad intermediate or invalid state" means, because I assume
> > it's not arbitrary junk or this wouldn't be sound genrally.
> 
> Intermediate states like swap, migration or probably even splitting THP.
> Though I am not really sure whether pxx_bad() only gets used for valid
> table or leaf entries i.e things which are mapped. Hence checking just
> for non table entry is better and even safer, than looking out for what
> other states the entry could be in.

I had a quick look through some of the uses and it seems the expectation
is that after a !pmd_bad(), the pmd is a table. The checks for
migration, huge page etc. are prior to the pmd_bad() check.

For this patch:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Catalin Marinas]

On Thu, May 13, 2021 at 10:44:04AM +0530, Anshuman Khandual wrote:
> On 5/11/21 7:37 PM, Mark Rutland wrote:
> > On Tue, May 11, 2021 at 09:21:46AM +0530, Anshuman Khandual wrote:
> >> On 5/10/21 8:13 PM, Mark Rutland wrote:
> >>> On Mon, May 10, 2021 at 04:37:51PM +0530, Anshuman Khandual wrote:
> >>>> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
> >>>> entry does not have a pointer to the next level page table. This had been
> >>>> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
> >>>> section maps"). Hence explicitly check for a table entry rather than just
> >>>> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
> >>>> [pud|pmd]_table() making the semantics clear.
> >>>>
> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >>>> Cc: Will Deacon <will@kernel.org>
> >>>> Cc: Mark Rutland <mark.rutland@arm.com>
> >>>> Cc: linux-arm-kernel@lists.infradead.org
> >>>> Cc: linux-kernel@vger.kernel.org
> >>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >>>
> >>> I have no strong feelings either way, so: 
> >>>
> >>> Acked-by: Mark Rutland <mark.rutland@arm.com>
> >>>
> >>> ... that said, I think that the "bad" naming is unclear and misleading,
> >>> and it'd be really nice if we could clean that up treewide with
> >>> something clearer than "bad".
> >>
> >> Agreed, the name is misleading.
> >>
> >>> It does seem that would roughly fit p??_leaf() if we had
> >>
> >> But what if the platform does not support huge page aka leaf mapping
> >> at the given level ? Also a non table i.e bad entry might not always
> >> mean a leaf/section/huge page mapping, it could simply imply that the
> >> entry is not just pointing to next level and might be just in an bad
> >> intermediate or invalid state.
> > 
> > Ah, so that's also covering swap entries, too? It's not entirely clear
> > to me what "bad intermediate or invalid state" means, because I assume
> > it's not arbitrary junk or this wouldn't be sound genrally.
> 
> Intermediate states like swap, migration or probably even splitting THP.
> Though I am not really sure whether pxx_bad() only gets used for valid
> table or leaf entries i.e things which are mapped. Hence checking just
> for non table entry is better and even safer, than looking out for what
> other states the entry could be in.

I had a quick look through some of the uses and it seems the expectation
is that after a !pmd_bad(), the pmd is a table. The checks for
migration, huge page etc. are prior to the pmd_bad() check.

For this patch:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

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

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Will Deacon]

On Mon, 10 May 2021 16:37:51 +0530, Anshuman Khandual wrote:
> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
> entry does not have a pointer to the next level page table. This had been
> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
> section maps"). Hence explicitly check for a table entry rather than just
> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
> [pud|pmd]_table() making the semantics clear.

Applied to arm64 (for-next/mm), thanks!

[1/1] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()
      https://git.kernel.org/arm64/c/e377ab82311a

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

Re: [PATCH] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()

[Will Deacon]

On Mon, 10 May 2021 16:37:51 +0530, Anshuman Khandual wrote:
> Semantics wise, [pud|pmd]_bad() have always implied that a given [PUD|PMD]
> entry does not have a pointer to the next level page table. This had been
> made clear in the commit a1c76574f345 ("arm64: mm: use *_sect to check for
> section maps"). Hence explicitly check for a table entry rather than just
> testing a single bit. This basically redefines [pud|pmd]_bad() in terms of
> [pud|pmd]_table() making the semantics clear.

Applied to arm64 (for-next/mm), thanks!

[1/1] arm64/mm: Remove [PUD|PMD]_TABLE_BIT from [pud|pmd]_bad()
      https://git.kernel.org/arm64/c/e377ab82311a

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

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