| Message ID | 20200913234730.23145-1-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] arm64/mm: Refactor {pgd, pud, pmd, pte}_ERROR() | expand |
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. > > 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 ! > > 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. > > #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 > - > /* GENERIC_BUG traps */ > > int is_valid_bugaddr(unsigned long addr) >
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
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. > > 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. > > 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)) > > #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); > -} > - > /* GENERIC_BUG traps */ > > int is_valid_bugaddr(unsigned long addr) > Looks good to me. Seems like a sensible clean up which reduces code. Tried booting on multiple page size configs and see no regression. Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com>
On Mon, 14 Sep 2020 09:47:30 +1000, 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. > > 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. Applied to arm64 (for-next/misc), thanks! [1/1] arm64/mm: Refactor {pgd, pud, pmd, pte}_ERROR() https://git.kernel.org/arm64/c/2cf660eb81e9 Cheers,
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)) #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); -} - /* GENERIC_BUG traps */ int is_valid_bugaddr(unsigned long addr)
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. 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. 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(-)