All of lore.kernel.org
 help / color / mirror / Atom feed
From: Enze Li <lienze@kylinos.cn>
To: Huacai Chen <chenhuacai@kernel.org>
Cc: kernel@xen0n.name,  loongarch@lists.linux.dev,
	 glider@google.com, elver@google.com,  akpm@linux-foundation.org,
	kasan-dev@googlegroups.com,  linux-mm@kvack.org,
	 zhangqing@loongson.cn, yangtiezhu@loongson.cn,
	 dvyukov@google.com
Subject: Re: [PATCH 1/4] LoongArch: mm: Add page table mapped mode support
Date: Sun, 23 Jul 2023 15:17:05 +0800	[thread overview]
Message-ID: <87lef7ayha.fsf@kylinos.cn> (raw)
In-Reply-To: <CAAhV-H4+8_gBMMdLhx=uEAsCN5wK7kFONsKDyGPqm0kxW8FU=A@mail.gmail.com> (Huacai Chen's message of "Fri, 21 Jul 2023 10:21:38 +0800")

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?

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
>> >>
>> >>
>>

  reply	other threads:[~2023-07-23  7:22 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-19  8:27 [PATCH 0/4] Add KFENCE support for LoongArch Enze Li
2023-07-19  8:27 ` [PATCH 1/4] LoongArch: mm: Add page table mapped mode support Enze Li
2023-07-19 15:29   ` Huacai Chen
2023-07-21  2:12     ` Enze Li
2023-07-21  2:21       ` Huacai Chen
2023-07-23  7:17         ` Enze Li [this message]
2023-07-25  2:06           ` Huacai Chen
2023-07-25  6:07             ` Enze Li
2023-07-19  8:27 ` [PATCH 2/4] LoongArch: Get stack without NMI when providing regs parameter Enze Li
2023-07-19 15:17   ` Huacai Chen
2023-07-21  1:49     ` Enze Li
2023-07-21  2:17       ` Huacai Chen
2023-07-19  8:27 ` [PATCH 3/4] KFENCE: Deferring the assignment of the local variable addr Enze Li
2023-07-19 10:54   ` Marco Elver
2023-07-19 15:06     ` Huacai Chen
2023-07-19 15:08       ` Marco Elver
2023-07-19  8:27 ` [PATCH 4/4] LoongArch: Add KFENCE support Enze Li
2023-07-19 15:27   ` Huacai Chen
2023-07-21  3:13     ` Enze Li
2023-07-21  3:19       ` Huacai Chen
2023-07-23  7:34         ` Enze Li

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87lef7ayha.fsf@kylinos.cn \
    --to=lienze@kylinos.cn \
    --cc=akpm@linux-foundation.org \
    --cc=chenhuacai@kernel.org \
    --cc=dvyukov@google.com \
    --cc=elver@google.com \
    --cc=glider@google.com \
    --cc=kasan-dev@googlegroups.com \
    --cc=kernel@xen0n.name \
    --cc=linux-mm@kvack.org \
    --cc=loongarch@lists.linux.dev \
    --cc=yangtiezhu@loongson.cn \
    --cc=zhangqing@loongson.cn \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.