lkml.org 
[lkml]   [2020]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] arm64/mm: Refactor {pgd, pud, pmd, pte}_ERROR()
From
Date
Hi Anshuman,

On 9/14/20 3:31 PM, Anshuman Khandual wrote:
> On 09/14/2020 05:17 AM, Gavin Shan wrote:
>> The function __{pgd, pud, pmd, pte}_error() are introduced so that
>> they can be called by {pgd, pud, pmd, pte}_ERROR(). However, some
>> of the functions could never be called when the corresponding page
>> table level isn't enabled. For example, __{pud, pmd}_error() are
>> unused when PUD and PMD are folded to PGD.
>
> Right, it makes sense not to have these helpers generally available.
> Given pxx_ERROR() is enabled only when required page table level is
> available, with a CONFIG_PGTABLE_LEVEL check.
>

Yep.

>>
>> This removes __{pgd, pud, pmd, pte}_error() and call pr_err() from
>> {pgd, pud, pmd, pte}_ERROR() directly, similar to what x86/powerpc
>> are doing. With this, the code looks a bit simplified either.
>
> Do we need p4d_ERROR() here as well !
>

p4d_ERROR() is defined in include/asm-generic/pgtable-nop4d.h, which
is always included because we have 4 levels of page tables to the
maximum.

>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> v2: Fix build warning caused by wrong printk format
>> ---
>> arch/arm64/include/asm/pgtable.h | 17 ++++++++---------
>> arch/arm64/kernel/traps.c | 20 --------------------
>> 2 files changed, 8 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index d5d3fbe73953..e0ab81923c30 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -35,11 +35,6 @@
>>
>> extern struct page *vmemmap;
>>
>> -extern void __pte_error(const char *file, int line, unsigned long val);
>> -extern void __pmd_error(const char *file, int line, unsigned long val);
>> -extern void __pud_error(const char *file, int line, unsigned long val);
>> -extern void __pgd_error(const char *file, int line, unsigned long val);
>> -
>> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>> #define __HAVE_ARCH_FLUSH_PMD_TLB_RANGE
>>
>> @@ -57,7 +52,8 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
>> extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
>> #define ZERO_PAGE(vaddr) phys_to_page(__pa_symbol(empty_zero_page))
>>
>> -#define pte_ERROR(pte) __pte_error(__FILE__, __LINE__, pte_val(pte))
>> +#define pte_ERROR(e) \
>> + pr_err("%s:%d: bad pte %016llx.\n", __FILE__, __LINE__, pte_val(e))
>>
>> /*
>> * Macros to convert between a physical address and its placement in a
>> @@ -541,7 +537,8 @@ static inline unsigned long pmd_page_vaddr(pmd_t pmd)
>>
>> #if CONFIG_PGTABLE_LEVELS > 2
>>
>> -#define pmd_ERROR(pmd) __pmd_error(__FILE__, __LINE__, pmd_val(pmd))
>> +#define pmd_ERROR(e) \
>> + 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))
>> @@ -608,7 +605,8 @@ static inline unsigned long pud_page_vaddr(pud_t pud)
>>
>> #if CONFIG_PGTABLE_LEVELS > 3
>>
>> -#define pud_ERROR(pud) __pud_error(__FILE__, __LINE__, pud_val(pud))
>> +#define pud_ERROR(e) \
>> + pr_err("%s:%d: bad pud %016llx.\n", __FILE__, __LINE__, pud_val(e))
>>
>> #define p4d_none(p4d) (!p4d_val(p4d))
>> #define p4d_bad(p4d) (!(p4d_val(p4d) & 2))
>> @@ -667,7 +665,8 @@ static inline unsigned long p4d_page_vaddr(p4d_t p4d)
>>
>> #endif /* CONFIG_PGTABLE_LEVELS > 3 */
>>
>> -#define pgd_ERROR(pgd) __pgd_error(__FILE__, __LINE__, pgd_val(pgd))
>> +#define pgd_ERROR(e) \
>> + pr_err("%s:%d: bad pgd %016llx.\n", __FILE__, __LINE__, pgd_val(e))
>
> A line break in these macros might not be required any more, as checkpatch.pl
> now accepts bit longer lines.
>

Correct, but I guess it's nice to limit the width to 80 characters :)

>>
>> #define pgd_set_fixmap(addr) ((pgd_t *)set_fixmap_offset(FIX_PGD, addr))
>> #define pgd_clear_fixmap() clear_fixmap(FIX_PGD)
>> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
>> index 13ebd5ca2070..12fba7136dbd 100644
>> --- a/arch/arm64/kernel/traps.c
>> +++ b/arch/arm64/kernel/traps.c
>> @@ -935,26 +935,6 @@ asmlinkage void enter_from_user_mode(void)
>> }
>> NOKPROBE_SYMBOL(enter_from_user_mode);
>>
>> -void __pte_error(const char *file, int line, unsigned long val)
>> -{
>> - pr_err("%s:%d: bad pte %016lx.\n", file, line, val);
>> -}
>> -
>> -void __pmd_error(const char *file, int line, unsigned long val)
>> -{
>> - pr_err("%s:%d: bad pmd %016lx.\n", file, line, val);
>> -}
>> -
>> -void __pud_error(const char *file, int line, unsigned long val)
>> -{
>> - pr_err("%s:%d: bad pud %016lx.\n", file, line, val);
>> -}
>> -
>> -void __pgd_error(const char *file, int line, unsigned long val)
>> -{
>> - pr_err("%s:%d: bad pgd %016lx.\n", file, line, val);
>> -}
>
> While moving %016lx now becomes %016llx. I guess this should be okay.
> Looks much cleaner to have removed these helpers from trap.c
>

Yep.

>> -
>> /* GENERIC_BUG traps */
>>
>> int is_valid_bugaddr(unsigned long addr)
>>
>

Thanks,
Gavin

\
 
 \ /
  Last update: 2020-09-14 07:44    [W:0.059 / U:0.620 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site