[PATCH V2] MIPS: clean up CONFIG_MIPS_PGD_C0_CONTEXT

[PATCH V2] MIPS: clean up CONFIG_MIPS_PGD_C0_CONTEXT

[Huang Pei]

In-Reply-To: 


fix waring when building 32bit kernel

Reported-by: kernel test robot <lkp@intel.com>

[PATCH] MIPS: clean up CONFIG_MIPS_PGD_C0_CONTEXT handling

[Huang Pei]

CP0 Context has enough room for wraping pgd into its 41-bit PTEBase field.

+. For XPHYS, the trick is that pgd is 4kB aligned, and the PABITS <= 48,
only save 48 - 12 + 5(for bit[63:59]) = 41 bits, aka. :

   bit[63:59] | 0000 0000 000 |  bit[47:12] | 0000 0000 0000

+. for CKSEG0, only save 29 - 12 = 17 bits

+. use CAC_BASE for accessing bit[63:59] of pgd

+. let CONFIG_MIPS_PGD_C0_CONTEXT depend on 64bit, and protect
build_fast_mips_refill_handler from 32bit building

Signed-off-by: Huang Pei <huangpei@loongson.cn>
---
 arch/mips/Kconfig    |  1 +
 arch/mips/mm/tlbex.c | 11 ++++++-----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 2000bb2b0220..517509ad8596 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -2142,6 +2142,7 @@ config CPU_SUPPORTS_HUGEPAGES
 	depends on !(32BIT && (ARCH_PHYS_ADDR_T_64BIT || EVA))
 config MIPS_PGD_C0_CONTEXT
 	bool
+	depends on 64BIT
 	default y if 64BIT && (CPU_MIPSR2 || CPU_MIPSR6) && !CPU_XLP
 
 #
diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
index a7521b8f7658..5bb9724578f7 100644
--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -848,8 +848,8 @@ void build_get_pmde64(u32 **p, struct uasm_label **l, struct uasm_reloc **r,
 		/* Clear lower 23 bits of context. */
 		uasm_i_dins(p, ptr, 0, 0, 23);
 
-		/* 1 0	1 0 1  << 6  xkphys cached */
-		uasm_i_ori(p, ptr, ptr, 0x540);
+		/* insert bit[63:59] of CAC_BASE into bit[11:6] of ptr */
+		uasm_i_ori(p, ptr, ptr, (CAC_BASE >> 53));
 		uasm_i_drotr(p, ptr, ptr, 11);
 #elif defined(CONFIG_SMP)
 		UASM_i_CPUID_MFC0(p, ptr, SMP_CPUID_REG);
@@ -1106,6 +1106,7 @@ struct mips_huge_tlb_info {
 	bool need_reload_pte;
 };
 
+#ifdef CONFIG_64BIT
 static struct mips_huge_tlb_info
 build_fast_tlb_refill_handler (u32 **p, struct uasm_label **l,
 			       struct uasm_reloc **r, unsigned int tmp,
@@ -1164,8 +1165,8 @@ build_fast_tlb_refill_handler (u32 **p, struct uasm_label **l,
 
 	if (pgd_reg == -1) {
 		vmalloc_branch_delay_filled = 1;
-		/* 1 0	1 0 1  << 6  xkphys cached */
-		uasm_i_ori(p, ptr, ptr, 0x540);
+		/* insert bit[63:59] of CAC_BASE into bit[11:6] of ptr */
+		uasm_i_ori(p, ptr, ptr, (CAC_BASE >> 53));
 		uasm_i_drotr(p, ptr, ptr, 11);
 	}
 
@@ -1292,7 +1293,7 @@ build_fast_tlb_refill_handler (u32 **p, struct uasm_label **l,
 
 	return rv;
 }
-
+#endif
 /*
  * For a 64-bit kernel, we are using the 64-bit XTLB refill exception
  * because EXL == 0.  If we wrap, we can also use the 32 instruction
-- 
2.25.1

Re: [PATCH] MIPS: clean up CONFIG_MIPS_PGD_C0_CONTEXT handling

[Maciej W. Rozycki]

On Sat, 27 Feb 2021, Huang Pei wrote:

> index 2000bb2b0220..517509ad8596 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -2142,6 +2142,7 @@ config CPU_SUPPORTS_HUGEPAGES
>  	depends on !(32BIT && (ARCH_PHYS_ADDR_T_64BIT || EVA))
>  config MIPS_PGD_C0_CONTEXT
>  	bool
> +	depends on 64BIT
>  	default y if 64BIT && (CPU_MIPSR2 || CPU_MIPSR6) && !CPU_XLP

 I guess you want:

	default y if (CPU_MIPSR2 || CPU_MIPSR6) && !CPU_XLP

at the same time too.  Otherwise you have cruft left behind.

> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> index a7521b8f7658..5bb9724578f7 100644
> --- a/arch/mips/mm/tlbex.c
> +++ b/arch/mips/mm/tlbex.c
> @@ -1106,6 +1106,7 @@ struct mips_huge_tlb_info {
>  	bool need_reload_pte;
>  };
>  
> +#ifdef CONFIG_64BIT
>  static struct mips_huge_tlb_info
>  build_fast_tlb_refill_handler (u32 **p, struct uasm_label **l,
>  			       struct uasm_reloc **r, unsigned int tmp,

 Does it actually build without a warning for !CONFIG_64BIT given the 
reference below?

> @@ -1164,8 +1165,8 @@ build_fast_tlb_refill_handler (u32 **p, struct uasm_label **l,
>  
>  	if (pgd_reg == -1) {
>  		vmalloc_branch_delay_filled = 1;
> -		/* 1 0	1 0 1  << 6  xkphys cached */
> -		uasm_i_ori(p, ptr, ptr, 0x540);
> +		/* insert bit[63:59] of CAC_BASE into bit[11:6] of ptr */
> +		uasm_i_ori(p, ptr, ptr, (CAC_BASE >> 53));

 Instead I'd paper the issue over by casting the constant to `s64'.

 Or better yet fixed it properly by defining CAC_BASE, etc. as `unsigned
long long' long rather than `unsigned long' to stop all this nonsense 
(e.g. PHYS_TO_XKPHYS already casts the result to `s64').  Thomas, WDYT?

  Maciej

Re: [PATCH] MIPS: clean up CONFIG_MIPS_PGD_C0_CONTEXT handling

[Huang Pei]

Hi, 
On Mon, Mar 01, 2021 at 12:00:28AM +0100, Maciej W. Rozycki wrote:
> On Sat, 27 Feb 2021, Huang Pei wrote:
> 
> > index 2000bb2b0220..517509ad8596 100644
> > --- a/arch/mips/Kconfig
> > +++ b/arch/mips/Kconfig
> > @@ -2142,6 +2142,7 @@ config CPU_SUPPORTS_HUGEPAGES
> >  	depends on !(32BIT && (ARCH_PHYS_ADDR_T_64BIT || EVA))
> >  config MIPS_PGD_C0_CONTEXT
> >  	bool
> > +	depends on 64BIT
> >  	default y if 64BIT && (CPU_MIPSR2 || CPU_MIPSR6) && !CPU_XLP
> 
>  I guess you want:
> 
> 	default y if (CPU_MIPSR2 || CPU_MIPSR6) && !CPU_XLP
> 
> at the same time too.  Otherwise you have cruft left behind.
> 
Yes, it is much better
> > diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> > index a7521b8f7658..5bb9724578f7 100644
> > --- a/arch/mips/mm/tlbex.c
> > +++ b/arch/mips/mm/tlbex.c
> > @@ -1106,6 +1106,7 @@ struct mips_huge_tlb_info {
> >  	bool need_reload_pte;
> >  };
> >  
> > +#ifdef CONFIG_64BIT
> >  static struct mips_huge_tlb_info
> >  build_fast_tlb_refill_handler (u32 **p, struct uasm_label **l,
> >  			       struct uasm_reloc **r, unsigned int tmp,
> 
>  Does it actually build without a warning for !CONFIG_64BIT given the 
> reference below?

No, my bad, my first reaction when seeing "IS_ENABLED(CONFIG_64BIT)" is
"#ifdef CONFIG_64BIT"
> 
> > @@ -1164,8 +1165,8 @@ build_fast_tlb_refill_handler (u32 **p, struct uasm_label **l,
> >  
> >  	if (pgd_reg == -1) {
> >  		vmalloc_branch_delay_filled = 1;
> > -		/* 1 0	1 0 1  << 6  xkphys cached */
> > -		uasm_i_ori(p, ptr, ptr, 0x540);
> > +		/* insert bit[63:59] of CAC_BASE into bit[11:6] of ptr */
> > +		uasm_i_ori(p, ptr, ptr, (CAC_BASE >> 53));
> 
>  Instead I'd paper the issue over by casting the constant to `s64'.
> 
>  Or better yet fixed it properly by defining CAC_BASE, etc. as `unsigned
> long long' long rather than `unsigned long' to stop all this nonsense 
> (e.g. PHYS_TO_XKPHYS already casts the result to `s64').  Thomas, WDYT?
Sorry, I do not get it , on MIPS32, how can CAC_BASE be unsigned long
long ?


If this did not work, how about one step back, just explicit comment
wihtout "(CAC_BASE << 53)" ?
> 
>   Maciej

Re: [PATCH] MIPS: clean up CONFIG_MIPS_PGD_C0_CONTEXT handling

[Maciej W. Rozycki]

On Thu, 4 Mar 2021, Huang Pei wrote:

> > > @@ -1164,8 +1165,8 @@ build_fast_tlb_refill_handler (u32 **p, struct uasm_label **l,
> > >  
> > >  	if (pgd_reg == -1) {
> > >  		vmalloc_branch_delay_filled = 1;
> > > -		/* 1 0	1 0 1  << 6  xkphys cached */
> > > -		uasm_i_ori(p, ptr, ptr, 0x540);
> > > +		/* insert bit[63:59] of CAC_BASE into bit[11:6] of ptr */
> > > +		uasm_i_ori(p, ptr, ptr, (CAC_BASE >> 53));
> > 
> >  Instead I'd paper the issue over by casting the constant to `s64'.
> > 
> >  Or better yet fixed it properly by defining CAC_BASE, etc. as `unsigned
> > long long' long rather than `unsigned long' to stop all this nonsense 
> > (e.g. PHYS_TO_XKPHYS already casts the result to `s64').  Thomas, WDYT?
> Sorry, I do not get it , on MIPS32, how can CAC_BASE be unsigned long
> long ?

 By using the `ULL' suffix with constants.  It won't change code produced, 
because they are unsigned anyway and the compiler will truncate them with 
no change to the actual value to fit in narrower data types as needed, but 
it will silence the warnings.

  Maciej

Re: [PATCH] MIPS: clean up CONFIG_MIPS_PGD_C0_CONTEXT handling

[Huang Pei]

Hi,
On Thu, Mar 04, 2021 at 02:40:54AM +0100, Maciej W. Rozycki wrote:
> On Thu, 4 Mar 2021, Huang Pei wrote:
> 
> > > > @@ -1164,8 +1165,8 @@ build_fast_tlb_refill_handler (u32 **p, struct uasm_label **l,
> > > >  
> > > >  	if (pgd_reg == -1) {
> > > >  		vmalloc_branch_delay_filled = 1;
> > > > -		/* 1 0	1 0 1  << 6  xkphys cached */
> > > > -		uasm_i_ori(p, ptr, ptr, 0x540);
> > > > +		/* insert bit[63:59] of CAC_BASE into bit[11:6] of ptr */
> > > > +		uasm_i_ori(p, ptr, ptr, (CAC_BASE >> 53));
> > > 
> > >  Instead I'd paper the issue over by casting the constant to `s64'.
> > > 
> > >  Or better yet fixed it properly by defining CAC_BASE, etc. as `unsigned
> > > long long' long rather than `unsigned long' to stop all this nonsense 
> > > (e.g. PHYS_TO_XKPHYS already casts the result to `s64').  Thomas, WDYT?
> > Sorry, I do not get it , on MIPS32, how can CAC_BASE be unsigned long
> > long ?
> 
>  By using the `ULL' suffix with constants.  It won't change code produced, 
> because they are unsigned anyway and the compiler will truncate them with 
> no change to the actual value to fit in narrower data types as needed, but 
> it will silence the warnings.
> 
>   Maciej


On Linux 5.11 with this patch and **ONLY** attaching ULL to CAC_BASE in 
arch/mips/include/asm/mach-generic/space.h for CONFIG_32BIT, cross gcc
7.5 in Ubuntu 18.04, loongon1c_defconfig

..........

make[1]: Entering directory '/home/hp/projects/Linux/temp/out_stable'
  GEN     Makefile
  CALL    /home/hp/projects/Linux/temp/linux-stable/scripts/atomic/check-atomics.sh
  CALL    /home/hp/projects/Linux/temp/linux-stable/scripts/checksyscalls.sh
  CC      arch/mips/mm/cache.o
  CC      arch/mips/kernel/branch.o
  CC      arch/mips/mm/context.o
  CC      arch/mips/loongson32/common/time.o
  CC      arch/mips/loongson32/ls1c/board.o
  CHK     include/generated/compile.h
  CC      arch/mips/vdso/vgettimeofday.o
  HOSTCC  arch/mips/vdso/genvdso
  CC      kernel/sched/cputime.o
In file included from /home/hp/projects/Linux/temp/linux-stable/arch/mips/include/asm/mmiowb.h:5:0,
                 from /home/hp/projects/Linux/temp/linux-stable/include/linux/spinlock.h:61,
                 from /home/hp/projects/Linux/temp/linux-stable/include/linux/wait.h:9,
                 from /home/hp/projects/Linux/temp/linux-stable/include/linux/wait_bit.h:8,
                 from /home/hp/projects/Linux/temp/linux-stable/include/linux/fs.h:6,
                 from /home/hp/projects/Linux/temp/linux-stable/arch/mips/mm/cache.c:9:
/home/hp/projects/Linux/temp/linux-stable/arch/mips/include/asm/io.h: In function ‘phys_to_virt’:
/home/hp/projects/Linux/temp/linux-stable/arch/mips/include/asm/io.h:122:9: error: cast to pointer 
from integer of different size [-Werror=int-to-pointer-cast]
  return (void *)(address + PAGE_OFFSET - PHYS_OFFSET);
         ^
In file included from /home/hp/projects/Linux/temp/linux-stable/arch/mips/include/asm/mmiowb.h:5:0,
                 from /home/hp/projects/Linux/temp/linux-stable/include/linux/spinlock.h:61,
                 from /home/hp/projects/Linux/temp/linux-stable/include/linux/wait.h:9,
                 from /home/hp/projects/Linux/temp/linux-stable/include/linux/pid.h:6,
                 from /home/hp/projects/Linux/temp/linux-stable/include/linux/sched.h:14,
                 from /home/hp/projects/Linux/temp/linux-stable/include/linux/sched/signal.h:7,
                 from /home/hp/projects/Linux/temp/linux-stable/arch/mips/kernel/branch.c:10:
/home/hp/projects/Linux/temp/linux-stable/arch/mips/include/asm/io.h: In function ‘phys_to_virt’:
/home/hp/projects/Linux/temp/linux-stable/arch/mips/include/asm/io.h:122:9: error: cast to pointer 
from integer of different size [-Werror=int-to-pointer-cast]
  return (void *)(address + PAGE_OFFSET - PHYS_OFFSET);


.........

Only change CAC_BASE Does NOT work 


Huang Pei

Re: [PATCH] MIPS: clean up CONFIG_MIPS_PGD_C0_CONTEXT handling

[Maciej W. Rozycki]

On Fri, 5 Mar 2021, Huang Pei wrote:

> /home/hp/projects/Linux/temp/linux-stable/arch/mips/include/asm/io.h: In function ‘phys_to_virt’:
> /home/hp/projects/Linux/temp/linux-stable/arch/mips/include/asm/io.h:122:9: error: cast to pointer 
> from integer of different size [-Werror=int-to-pointer-cast]
>   return (void *)(address + PAGE_OFFSET - PHYS_OFFSET);
> 
> 
> .........
> 
> Only change CAC_BASE Does NOT work 

 Thank you for checking.

 Right.  I don't know why it fails for `phys_to_virt' where `address' is 
of the `unsigned long' type, but there are other places where the macros 
themselves are cast to `void *'.  We may want to rework that stuff, but 
not necessarily on this occasion.

 Use an explicit cast of the macro to `s64' here then, as my other 
suggestion was.  Anything is better than hardcoded magic numbers.

  Maciej

Re: Re: [PATCH] MIPS: clean up CONFIG_MIPS_PGD_C0_CONTEXT handling

[huangpei]

Hi,
>On Fri, 5 Mar 2021, Huang Pei wrote:
>
>> /home/hp/projects/Linux/temp/linux-stable/arch/mips/include/asm/io.h: In function ‘phys_to_virt’:
>> /home/hp/projects/Linux/temp/linux-stable/arch/mips/include/asm/io.h:122:9: error: cast to pointer
>> from integer of different size [-Werror=int-to-pointer-cast]
>>   return (void *)(address + PAGE_OFFSET - PHYS_OFFSET);
>>
>>
>> .........
>>
>> Only change CAC_BASE Does NOT work
>
> Thank you for checking.
>
> Right.  I don't know why it fails for `phys_to_virt' where `address' is
>of the `unsigned long' type, but there are other places where the macros
>themselves are cast to `void *'.  We may want to rework that stuff, but
>not necessarily on this occasion.
>
> Use an explicit cast of the macro to `s64' here then, as my other
>suggestion was.  Anything is better than hardcoded magic numbers.
>
>  Maciej 

cast into s64 works. I will resend v3 soon.

Another issue, please take a look at GCC bug 99217, there is a partial fix, but I am not sure
that if it is advised mips16 should also be covered and how?

I found the ftrace on MIPS is not safe on SMP, since when disabling tracing,  the callsite of
_mcount need two writes to transform 

jal    _mcount
addiu sp, sp, -offset

into 

nop
nop

no matter in what order these two writes are seen, it is wrecked in these two case

jal    _mcount
nop

or

nop
addiu    sp, sp, -offset

so, I add a new ftrace implementation based on -fpatchable-function-entry=3, which is blocked by 
GCC bug 99217

see https://lore.kernel.org/linux-mips/20210305101933.9799-1-huangpei@loongson.cn/

At last, is it possible to add fix 99217 and 93242 into gcc 8.5?

-----------------
Huang Pei