Re: [PATCH 1/4] LoongArch: mm: Add page table mapped mode support

[Huacai Chen <chenhuacai@kernel.org>]

On Sun, Jul 23, 2023 at 3:17 PM Enze Li <lienze@kylinos.cn> 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@kylinos.cn> 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@kylinos.cn> 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@kylinos.cn>
> >> >> ---
> >> >>  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
> >> >>
> >> >>
> >>