On Fri, Jul 21 2023 at 10:21:38 AM +0800, Huacai Chen wrote: > On Fri, Jul 21, 2023 at 10:12 AM Enze Li <lienze@xxxxxxxxxx> wrote: >> >> On Wed, Jul 19 2023 at 11:29:37 PM +0800, Huacai Chen wrote: >> >> > Hi, Enze, >> > >> > On Wed, Jul 19, 2023 at 4:34 PM Enze Li <lienze@xxxxxxxxxx> wrote: >> >> >> >> According to LoongArch documentation online, there are two types of address >> >> translation modes: direct mapped address translation mode (direct mapped mode) >> >> and page table mapped address translation mode (page table mapped mode). >> >> >> >> Currently, the upstream code only supports DMM (Direct Mapped Mode). >> >> This patch adds a function that determines whether PTMM (Page Table >> >> Mapped Mode) should be used, and also adds the corresponding handler >> >> funcitons for both modes. >> >> >> >> For more details on the two modes, see [1]. >> >> >> >> [1] >> >> https://loongson.github.io/LoongArch-Documentation/LoongArch-Vol1-EN.html#virtual-address-space-and-address-translation-mode >> >> >> >> Signed-off-by: Enze Li <lienze@xxxxxxxxxx> >> >> --- >> >> arch/loongarch/include/asm/page.h | 10 ++++++++++ >> >> arch/loongarch/include/asm/pgtable.h | 6 ++++++ >> >> arch/loongarch/mm/pgtable.c | 25 +++++++++++++++++++++++++ >> >> 3 files changed, 41 insertions(+) >> >> >> >> diff --git a/arch/loongarch/include/asm/page.h b/arch/loongarch/include/asm/page.h >> >> index 26e8dccb6619..05919be15801 100644 >> >> --- a/arch/loongarch/include/asm/page.h >> >> +++ b/arch/loongarch/include/asm/page.h >> >> @@ -84,7 +84,17 @@ typedef struct { unsigned long pgprot; } pgprot_t; >> >> #define sym_to_pfn(x) __phys_to_pfn(__pa_symbol(x)) >> >> >> >> #define virt_to_pfn(kaddr) PFN_DOWN(PHYSADDR(kaddr)) >> >> + >> >> +#ifdef CONFIG_64BIT >> >> +#define virt_to_page(kaddr) \ >> >> +({ \ >> >> + is_PTMM_addr((unsigned long)kaddr) ? \ >> >> + PTMM_virt_to_page((unsigned long)kaddr) : \ >> >> + DMM_virt_to_page((unsigned long)kaddr); \ >> >> +}) >> > 1, Rename these helpers to >> > is_dmw_addr()/dmw_virt_to_page()/tlb_virt_to_page() will be better. >> > 2, These helpers are so simple so can be defined as inline function or >> > macros in page.h. >> >> Hi Huacai, >> >> Except for tlb_virt_to_page(), the remaining two modifications are easy. >> >> I've run into a lot of problems when trying to make tlb_virt_to_page() >> as a macro or inline function. That's because we need to export this >> symbol in order for it to be used by the module that called the >> virt_to_page() function, other wise, we got the following errors, >> >> ----------------------------------------------------------------------- >> MODPOST Module.symvers >> ERROR: modpost: "tlb_virt_to_page" [fs/hfsplus/hfsplus.ko] undefined! >> ERROR: modpost: "tlb_virt_to_page" [fs/smb/client/cifs.ko] undefined! >> ERROR: modpost: "tlb_virt_to_page" [crypto/gcm.ko] undefined! >> ERROR: modpost: "tlb_virt_to_page" [crypto/ccm.ko] undefined! >> ERROR: modpost: "tlb_virt_to_page" [crypto/essiv.ko] undefined! >> ERROR: modpost: "tlb_virt_to_page" [lib/crypto/libchacha20poly1305.ko] undefined! >> ERROR: modpost: "tlb_virt_to_page" [drivers/gpu/drm/ttm/ttm.ko] undefined! >> ERROR: modpost: "tlb_virt_to_page" [drivers/gpu/drm/amd/amdgpu/amdgpu.ko] undefined! >> ERROR: modpost: "tlb_virt_to_page" [drivers/scsi/iscsi_tcp.ko] undefined! >> ERROR: modpost: "tlb_virt_to_page" [drivers/scsi/qla2xxx/qla2xxx.ko] undefined! >> WARNING: modpost: suppressed 44 unresolved symbol warnings because there were too many) >> ----------------------------------------------------------------------- >> >> It seems to me that wrapping it into a common function might be the only >> way to successfully compile or link with this modification. >> >> ========================================================= >> --- a/arch/loongarch/include/asm/pgtable.h >> +++ b/arch/loongarch/include/asm/pgtable.h >> @@ -360,6 +360,8 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt >> #define PMD_T_LOG2 (__builtin_ffs(sizeof(pmd_t)) - 1) >> #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) >> >> +inline struct page *tlb_virt_to_page(unsigned long kaddr); >> + >> >> --- a/arch/loongarch/mm/pgtable.c >> +++ b/arch/loongarch/mm/pgtable.c >> @@ -9,6 +9,12 @@ >> #include <asm/pgtable.h> >> #include <asm/tlbflush.h> >> >> +inline struct page *tlb_virt_to_page(unsigned long kaddr) >> +{ >> + return pte_page(*virt_to_kpte(kaddr)); >> +} >> +EXPORT_SYMBOL_GPL(tlb_virt_to_page); >> ========================================================= >> >> WDYT? >> >> Best Regards, >> Enze > If you define "static inline" functions in page.h, there will be no problems. > Hi Huacai, After failed over and over and over again, I think I've found the reason why we can't define tlb_virt_to_page as macro or inline function in asm/page.h or asm/pgtable.h. :) I'll go through this step by step. If I put tlb_virt_to_page in asm/page.h as following, ===================================================== +static inline struct page *tlb_virt_to_page(unsigned long kaddr) +{ + return pte_page(*virt_to_kpte(kaddr)); +} ===================================================== and compile kernel, gcc says to me the following error. -------------------------------------------------------------------- CC arch/loongarch/kernel/asm-offsets.s In file included from ./include/linux/shm.h:6, from ./include/linux/sched.h:16, from arch/loongarch/kernel/asm-offsets.c:8: ./arch/loongarch/include/asm/page.h: In function ‘tlb_virt_to_page’: ./arch/loongarch/include/asm/page.h:126:16: error: implicit declaration of function ‘pte_page’ [-Werror=implicit-function-declaration] 126 | return pte_page(*virt_to_kpte(kaddr)); | ^~~~~~~~ --------------------------------------------------------------------- "pte_page" is declared in asm/pgtable.h, so I put "#include <asm/pgtable.h>" ahead, like this, ===================================================== +#include <asm/pgtable.h> +static inline struct page *tlb_virt_to_page(unsigned long kaddr) +{ + return pte_page(*virt_to_kpte(kaddr)); +} ===================================================== then compile again, gcc says, --------------------------------------------------------------------- CC arch/loongarch/kernel/asm-offsets.s In file included from ./arch/loongarch/include/asm/page.h:98, from ./include/linux/shm.h:6, from ./include/linux/sched.h:16, from arch/loongarch/kernel/asm-offsets.c:8: ./arch/loongarch/include/asm/page.h: In function ‘tlb_virt_to_page’: ./arch/loongarch/include/asm/page.h:127:26: error: implicit declaration of function ‘virt_to_kpte’; did you mean ‘virt_to_pfn’? [-Werror=implicit-function-declaration] 127 | return pte_page(*virt_to_kpte(kaddr)); | ^~~~~~~~~~~~ --------------------------------------------------------------------- "virt_to_kpte" is defined in linux/pgtable.h, consequently I add "#include <linux/pgtable.h>" as well, ===================================================== +#include <asm/pgtable.h> +#include <linux/pgtable.h> +static inline struct page *tlb_virt_to_page(unsigned long kaddr) +{ + return pte_page(*virt_to_kpte(kaddr)); +} ===================================================== and continue, --------------------------------------------------------------------- CC arch/loongarch/kernel/asm-offsets.s CALL scripts/checksyscalls.sh CC arch/loongarch/vdso/vgetcpu.o CC arch/loongarch/vdso/vgettimeofday.o In file included from ./arch/loongarch/include/asm/page.h:124, from ./include/linux/mm_types_task.h:16, from ./include/linux/mm_types.h:5, from ./include/linux/mmzone.h:22, from ./include/linux/gfp.h:7, from ./include/linux/mm.h:7, from ./arch/loongarch/include/asm/vdso.h:10, from arch/loongarch/vdso/vgetcpu.c:6: ./arch/loongarch/include/asm/pgtable.h: In function ‘pte_accessible’: ./arch/loongarch/include/asm/pgtable.h:436:40: error: invalid use of undefined type ‘struct mm_struct’ 436 | atomic_read(&mm->tlb_flush_pending)) | ^~ --------------------------------------------------------------------- The first line above shows that it compiled successfully for the asm-offsets module. That's fair enough. Actually, the point is the next one (invalid use of undefined type 'struct mm_struct'). As we all know, before the compiler compiles, it expands the header files first. For this example, it firstly expands from the header file vdso.h, then the mm.h file and so on. We can see that the line 436 of asm/pgtable.h are using 'struct mm_struct'. When we backtrack to a file that has been previously expanded, it's obvious that the definition of mm_struct does not appear in the expanded file. Instead, it appears afterward (mm_types.h). To be clear, I'll exemplify this case with a cheap ASCII diagram. ... <-| we're using 'mm_struct' here >>> asm/pgtable.h <-| ... <-| | |->... | |->asm/page.h |->... |->... | |->... |->mm_types_task.h |->... |->mm_types.h-|->... |->... |->mmzone.h-|->... | |->... |->gfp.h-|->... | |->... |->mm.h-|->... But 'mm_struct' is defined here. |->vdso.h-|->... |->... vgetcpu.c I've also tried to include mm_types.h in advance, but in this case that doesn't work because the _LINUX_MM_TYPES_H macro already exists. The "forward declaration" was also taken into account, in the end it was found to be unavailable as well. In summary, I'm afraid that rewriting tlb_virt_to_page in asm/page.h as a macro or inline function is not possible. The root case of this is that both 'struct mm_struct' and 'virt_to_kpte' belong to high-level data structures, and if they are referenced in asm/page.h at the low-level, dependency problems arise. Anyway, we can at least define it as a normal function in asm/pgtable.h, is that Okay with you? It may be a bit wordy, so please bear with me. In addition, all of the above is my understanding, am I missing something? Best Regards, Enze >> >> > 3, CONFIG_64BIT can be removed here. >> > >> > Huacai >> > >> >> +#else >> >> #define virt_to_page(kaddr) pfn_to_page(virt_to_pfn(kaddr)) >> >> +#endif >> >> >> >> extern int __virt_addr_valid(volatile void *kaddr); >> >> #define virt_addr_valid(kaddr) __virt_addr_valid((volatile void *)(kaddr)) >> >> diff --git a/arch/loongarch/include/asm/pgtable.h b/arch/loongarch/include/asm/pgtable.h >> >> index ed6a37bb55b5..0fc074b8bd48 100644 >> >> --- a/arch/loongarch/include/asm/pgtable.h >> >> +++ b/arch/loongarch/include/asm/pgtable.h >> >> @@ -360,6 +360,12 @@ static inline void pte_clear(struct mm_struct *mm, unsigned long addr, pte_t *pt >> >> #define PMD_T_LOG2 (__builtin_ffs(sizeof(pmd_t)) - 1) >> >> #define PTE_T_LOG2 (__builtin_ffs(sizeof(pte_t)) - 1) >> >> >> >> +#ifdef CONFIG_64BIT >> >> +struct page *DMM_virt_to_page(unsigned long kaddr); >> >> +struct page *PTMM_virt_to_page(unsigned long kaddr); >> >> +bool is_PTMM_addr(unsigned long kaddr); >> >> +#endif >> >> + >> >> extern pgd_t swapper_pg_dir[]; >> >> extern pgd_t invalid_pg_dir[]; >> >> >> >> diff --git a/arch/loongarch/mm/pgtable.c b/arch/loongarch/mm/pgtable.c >> >> index 36a6dc0148ae..4c6448f996b6 100644 >> >> --- a/arch/loongarch/mm/pgtable.c >> >> +++ b/arch/loongarch/mm/pgtable.c >> >> @@ -9,6 +9,31 @@ >> >> #include <asm/pgtable.h> >> >> #include <asm/tlbflush.h> >> >> >> >> +#ifdef CONFIG_64BIT >> >> +/* DMM stands for Direct Mapped Mode. */ >> >> +struct page *DMM_virt_to_page(unsigned long kaddr) >> >> +{ >> >> + return pfn_to_page(virt_to_pfn(kaddr)); >> >> +} >> >> +EXPORT_SYMBOL_GPL(DMM_virt_to_page); >> >> + >> >> +/* PTMM stands for Page Table Mapped Mode. */ >> >> +struct page *PTMM_virt_to_page(unsigned long kaddr) >> >> +{ >> >> + return pte_page(*virt_to_kpte(kaddr)); >> >> +} >> >> +EXPORT_SYMBOL_GPL(PTMM_virt_to_page); >> >> + >> >> +bool is_PTMM_addr(unsigned long kaddr) >> >> +{ >> >> + if (unlikely((kaddr & GENMASK(BITS_PER_LONG - 1, cpu_vabits)) == >> >> + GENMASK(BITS_PER_LONG - 1, cpu_vabits))) >> >> + return true; >> >> + return false; >> >> +} >> >> +EXPORT_SYMBOL_GPL(is_PTMM_addr); >> >> +#endif >> >> + >> >> pgd_t *pgd_alloc(struct mm_struct *mm) >> >> { >> >> pgd_t *ret, *init; >> >> -- >> >> 2.34.1 >> >> >> >> >>
