On Sun, Jul 23, 2023 at 3:17 PM Enze Li <lienze@xxxxxxxxxx> wrote: > > 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? Well, you can define the helpers in .c files at present, but I have another question. Though other archs (e.g., RISC-V) have no DMW addresses, they still have linear area. In other words, both LoongArch and RISC-V have linear area and vmalloc-like areas. The only difference is LoongArch's linear area is DMW-mapped but RISC-V's linear area is TLB-mapped. For linear area, the translation is pfn_to_page(virt_to_pfn(kaddr)), no matter LoongArch or RISC-V; For vmalloc-like areas, the translation is pte_page(*virt_to_kpte(kaddr)), no matter LoongArch or RISC-V. My question is: why RISC-V only care about the linear area for virt_to_page(), but you are caring about the vmalloc-like areas? Huacai > > 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 > >> >> > >> >> > >>
