[PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[Huacai Chen]

In commit 2c8c53e28f1 (MIPS: Optimize TLB handlers for Octeon CPUs)
build_r4000_tlb_refill_handler() is modified. But it doesn't compatible
with the original code in HUGETLB case. Because there is a copy & paste
error and one line of code is missing. It is very easy to produce a bug
with LTP's hugemmap05 test.

Signed-off-by: Huacai Chen <chenhc@lemote.com>
Signed-off-by: Binbin Zhou <zhoubb@lemote.com>
Cc: <stable@vger.kernel.org>
---
 arch/mips/mm/tlbex.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
index e80e10b..343fe0f 100644
--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -1299,6 +1299,7 @@ static void build_r4000_tlb_refill_handler(void)
 	}
 #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
 	uasm_l_tlb_huge_update(&l, p);
+	UASM_i_LW(&p, K0, 0, K1);
 	build_huge_update_entries(&p, htlb_info.huge_pte, K1);
 	build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
 				   htlb_info.restore_scratch);
-- 
1.9.0

[PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[Huacai Chen]

In commit 2c8c53e28f1 (MIPS: Optimize TLB handlers for Octeon CPUs)
build_r4000_tlb_refill_handler() is modified. But it doesn't compatible
with the original code in HUGETLB case. Because there is a copy & paste
error and one line of code is missing. It is very easy to produce a bug
with LTP's hugemmap05 test.

Signed-off-by: Huacai Chen <chenhc@lemote.com>
Signed-off-by: Binbin Zhou <zhoubb@lemote.com>
Cc: <stable@vger.kernel.org>
---
 arch/mips/mm/tlbex.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
index e80e10b..343fe0f 100644
--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -1299,6 +1299,7 @@ static void build_r4000_tlb_refill_handler(void)
 	}
 #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
 	uasm_l_tlb_huge_update(&l, p);
+	UASM_i_LW(&p, K0, 0, K1);
 	build_huge_update_entries(&p, htlb_info.huge_pte, K1);
 	build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
 				   htlb_info.restore_scratch);
-- 
1.9.0

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[Aurelien Jarno]

On Tue, Jul 29, 2014 at 02:54:40PM +0800, Huacai Chen wrote:
> In commit 2c8c53e28f1 (MIPS: Optimize TLB handlers for Octeon CPUs)
> build_r4000_tlb_refill_handler() is modified. But it doesn't compatible
> with the original code in HUGETLB case. Because there is a copy & paste
> error and one line of code is missing. It is very easy to produce a bug
> with LTP's hugemmap05 test.
> 
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> Signed-off-by: Binbin Zhou <zhoubb@lemote.com>
> Cc: <stable@vger.kernel.org>
> ---
>  arch/mips/mm/tlbex.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> index e80e10b..343fe0f 100644
> --- a/arch/mips/mm/tlbex.c
> +++ b/arch/mips/mm/tlbex.c
> @@ -1299,6 +1299,7 @@ static void build_r4000_tlb_refill_handler(void)
>  	}
>  #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
>  	uasm_l_tlb_huge_update(&l, p);
> +	UASM_i_LW(&p, K0, 0, K1);
>  	build_huge_update_entries(&p, htlb_info.huge_pte, K1);
>  	build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
>  				   htlb_info.restore_scratch);

Thanks for this patch. It also fixes TRANSPARENT_HUGEPAGE=y, which we
had to disable on Loongson 3 as it was causing kernel crashes. This
option is quite interesting for most MIPS systems, which are using
a software driven TLB.

Tested-by: Aurelien Jarno <aurelien@aurel32.net>
Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[James Hogan]

Hi Huacai,

On Tuesday 29 July 2014 14:54:40 Huacai Chen wrote:
> In commit 2c8c53e28f1 (MIPS: Optimize TLB handlers for Octeon CPUs)
> build_r4000_tlb_refill_handler() is modified. But it doesn't compatible
> with the original code in HUGETLB case. Because there is a copy & paste
> error and one line of code is missing. It is very easy to produce a bug
> with LTP's hugemmap05 test.
> 
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> Signed-off-by: Binbin Zhou <zhoubb@lemote.com>
> Cc: <stable@vger.kernel.org>
> ---
>  arch/mips/mm/tlbex.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> index e80e10b..343fe0f 100644
> --- a/arch/mips/mm/tlbex.c
> +++ b/arch/mips/mm/tlbex.c
> @@ -1299,6 +1299,7 @@ static void build_r4000_tlb_refill_handler(void)
>  	}
>  #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
>  	uasm_l_tlb_huge_update(&l, p);
> +	UASM_i_LW(&p, K0, 0, K1);
>  	build_huge_update_entries(&p, htlb_info.huge_pte, K1);
>  	build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
>  				   htlb_info.restore_scratch);

build_huge_tlb_write_entry only uses K0 as a temp and clobbers without using 
the value, so the K0 must be being used by the code generated by 
build_huge_update_entires, but the patch you mentioned changed the second 
argument from K0 to htlb_info.huge_pte.

So should the K0 in the new UASM_i_LW call be changed to htlb_info.huge_pte 
too?

(David Daney on Cc)

Thanks
James

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[David Daney]

On 07/30/2014 02:41 PM, James Hogan wrote:
> Hi Huacai,
>
> On Tuesday 29 July 2014 14:54:40 Huacai Chen wrote:
>> In commit 2c8c53e28f1 (MIPS: Optimize TLB handlers for Octeon CPUs)
>> build_r4000_tlb_refill_handler() is modified. But it doesn't compatible
>> with the original code in HUGETLB case. Because there is a copy & paste
>> error and one line of code is missing. It is very easy to produce a bug
>> with LTP's hugemmap05 test.
>>
>> Signed-off-by: Huacai Chen <chenhc@lemote.com>
>> Signed-off-by: Binbin Zhou <zhoubb@lemote.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>>   arch/mips/mm/tlbex.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
>> index e80e10b..343fe0f 100644
>> --- a/arch/mips/mm/tlbex.c
>> +++ b/arch/mips/mm/tlbex.c
>> @@ -1299,6 +1299,7 @@ static void build_r4000_tlb_refill_handler(void)
>>   	}
>>   #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
>>   	uasm_l_tlb_huge_update(&l, p);
>> +	UASM_i_LW(&p, K0, 0, K1);
>>   	build_huge_update_entries(&p, htlb_info.huge_pte, K1);
>>   	build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
>>   				   htlb_info.restore_scratch);
>
> build_huge_tlb_write_entry only uses K0 as a temp and clobbers without using
> the value, so the K0 must be being used by the code generated by
> build_huge_update_entires, but the patch you mentioned changed the second
> argument from K0 to htlb_info.huge_pte.
>
> So should the K0 in the new UASM_i_LW call be changed to htlb_info.huge_pte
> too?
>

I don't know.  You have to dump out the generated handler (by #define 
DEBUG at the top of the file), then assemble/disassemble it.  Looking at 
the disassembly, we could make sensible statements about what is happening.


> (David Daney on Cc)
>
> Thanks
> James
>

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[David Daney]

On 07/30/2014 02:41 PM, James Hogan wrote:
> Hi Huacai,
>
> On Tuesday 29 July 2014 14:54:40 Huacai Chen wrote:
>> In commit 2c8c53e28f1 (MIPS: Optimize TLB handlers for Octeon CPUs)
>> build_r4000_tlb_refill_handler() is modified. But it doesn't compatible
>> with the original code in HUGETLB case. Because there is a copy & paste
>> error and one line of code is missing. It is very easy to produce a bug
>> with LTP's hugemmap05 test.
>>
>> Signed-off-by: Huacai Chen <chenhc@lemote.com>
>> Signed-off-by: Binbin Zhou <zhoubb@lemote.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>>   arch/mips/mm/tlbex.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
>> index e80e10b..343fe0f 100644
>> --- a/arch/mips/mm/tlbex.c
>> +++ b/arch/mips/mm/tlbex.c
>> @@ -1299,6 +1299,7 @@ static void build_r4000_tlb_refill_handler(void)
>>   	}
>>   #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
>>   	uasm_l_tlb_huge_update(&l, p);
>> +	UASM_i_LW(&p, K0, 0, K1);
>>   	build_huge_update_entries(&p, htlb_info.huge_pte, K1);
>>   	build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
>>   				   htlb_info.restore_scratch);
>
> build_huge_tlb_write_entry only uses K0 as a temp and clobbers without using
> the value, so the K0 must be being used by the code generated by
> build_huge_update_entires, but the patch you mentioned changed the second
> argument from K0 to htlb_info.huge_pte.
>
> So should the K0 in the new UASM_i_LW call be changed to htlb_info.huge_pte
> too?
>

I don't know.  You have to dump out the generated handler (by #define 
DEBUG at the top of the file), then assemble/disassemble it.  Looking at 
the disassembly, we could make sensible statements about what is happening.


> (David Daney on Cc)
>
> Thanks
> James
>

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[Huacai Chen]

Hi, David,

For non-Octeon CPU, htlb_info.huge_pte is equal to K0, but I don't
know much about Octeon. So I think you know whether we should use K0
or htlb_info.huge_pte here, since you are the original author.

On Thu, Jul 31, 2014 at 5:44 AM, David Daney <ddaney@caviumnetworks.com> wrote:
> On 07/30/2014 02:41 PM, James Hogan wrote:
>>
>> Hi Huacai,
>>
>> On Tuesday 29 July 2014 14:54:40 Huacai Chen wrote:
>>>
>>> In commit 2c8c53e28f1 (MIPS: Optimize TLB handlers for Octeon CPUs)
>>> build_r4000_tlb_refill_handler() is modified. But it doesn't compatible
>>> with the original code in HUGETLB case. Because there is a copy & paste
>>> error and one line of code is missing. It is very easy to produce a bug
>>> with LTP's hugemmap05 test.
>>>
>>> Signed-off-by: Huacai Chen <chenhc@lemote.com>
>>> Signed-off-by: Binbin Zhou <zhoubb@lemote.com>
>>> Cc: <stable@vger.kernel.org>
>>> ---
>>>   arch/mips/mm/tlbex.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
>>> index e80e10b..343fe0f 100644
>>> --- a/arch/mips/mm/tlbex.c
>>> +++ b/arch/mips/mm/tlbex.c
>>> @@ -1299,6 +1299,7 @@ static void build_r4000_tlb_refill_handler(void)
>>>         }
>>>   #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
>>>         uasm_l_tlb_huge_update(&l, p);
>>> +       UASM_i_LW(&p, K0, 0, K1);
>>>         build_huge_update_entries(&p, htlb_info.huge_pte, K1);
>>>         build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
>>>                                    htlb_info.restore_scratch);
>>
>>
>> build_huge_tlb_write_entry only uses K0 as a temp and clobbers without
>> using
>> the value, so the K0 must be being used by the code generated by
>> build_huge_update_entires, but the patch you mentioned changed the second
>> argument from K0 to htlb_info.huge_pte.
>>
>> So should the K0 in the new UASM_i_LW call be changed to
>> htlb_info.huge_pte
>> too?
>>
>
> I don't know.  You have to dump out the generated handler (by #define DEBUG
> at the top of the file), then assemble/disassemble it.  Looking at the
> disassembly, we could make sensible statements about what is happening.
>
>
>
>> (David Daney on Cc)
>>
>> Thanks
>> James
>>
>
>

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[David Daney]

On 07/30/2014 05:48 PM, Huacai Chen wrote:
> Hi, David,
>
> For non-Octeon CPU, htlb_info.huge_pte is equal to K0, but I don't
> know much about Octeon. So I think you know whether we should use K0
>  or htlb_info.huge_pte here, since you are the original author.
>

This is why I requested that somebody show me a disassembly of the
faulty handler.  I cannot tell where the problem is unless I see that.

Really I think the problem is in build_is_huge_pte(), where we are
clobbering 'tmp' which is K0.

So you could reload tmp/K0 in build_is_huge_pte().

> On Thu, Jul 31, 2014 at 5:44 AM, David Daney
> <ddaney@caviumnetworks.com> wrote:
>> On 07/30/2014 02:41 PM, James Hogan wrote:
>>>
>>> Hi Huacai,
>>>
>>> On Tuesday 29 July 2014 14:54:40 Huacai Chen wrote:
>>>>
>>>> In commit 2c8c53e28f1 (MIPS: Optimize TLB handlers for Octeon
>>>> CPUs) build_r4000_tlb_refill_handler() is modified. But it
>>>> doesn't compatible with the original code in HUGETLB case.
>>>> Because there is a copy & paste error and one line of code is
>>>> missing. It is very easy to produce a bug with LTP's
>>>> hugemmap05 test.
>>>>
>>>> Signed-off-by: Huacai Chen <chenhc@lemote.com> Signed-off-by:
>>>> Binbin Zhou <zhoubb@lemote.com> Cc: <stable@vger.kernel.org>
>>>> --- arch/mips/mm/tlbex.c | 1 + 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c index
>>>> e80e10b..343fe0f 100644 --- a/arch/mips/mm/tlbex.c +++
>>>> b/arch/mips/mm/tlbex.c @@ -1299,6 +1299,7 @@ static void
>>>> build_r4000_tlb_refill_handler(void) } #ifdef
>>>> CONFIG_MIPS_HUGE_TLB_SUPPORT uasm_l_tlb_huge_update(&l, p); +
>>>> UASM_i_LW(&p, K0, 0, K1); build_huge_update_entries(&p,
>>>> htlb_info.huge_pte, K1); build_huge_tlb_write_entry(&p, &l,
>>>> &r, K0, tlb_random, htlb_info.restore_scratch);
>>>
>>>
>>> build_huge_tlb_write_entry only uses K0 as a temp and clobbers
>>> without using the value, so the K0 must be being used by the
>>> code generated by build_huge_update_entires, but the patch you
>>> mentioned changed the second argument from K0 to
>>> htlb_info.huge_pte.
>>>
>>> So should the K0 in the new UASM_i_LW call be changed to
>>> htlb_info.huge_pte too?
>>>
>>
>> I don't know.  You have to dump out the generated handler (by
>> #define DEBUG at the top of the file), then assemble/disassemble
>> it.  Looking at the disassembly, we could make sensible statements
>> about what is happening.
>>
>>
>>
>>> (David Daney on Cc)
>>>
>>> Thanks James
>>>
>>
>>

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[James Hogan]

Hi,

On 31/07/14 02:13, David Daney wrote:
> On 07/30/2014 05:48 PM, Huacai Chen wrote:
>> For non-Octeon CPU, htlb_info.huge_pte is equal to K0, but I don't
>> know much about Octeon. So I think you know whether we should use K0
>>  or htlb_info.huge_pte here, since you are the original author.
>>
> 
> This is why I requested that somebody show me a disassembly of the
> faulty handler.  I cannot tell where the problem is unless I see that.
> 
> Really I think the problem is in build_is_huge_pte(), where we are
> clobbering 'tmp' which is K0.
> 
> So you could reload tmp/K0 in build_is_huge_pte().

Here's the difference with this patch (using k0) on an Octeon I have to
hand, with some slightly munged offsets for nicer diffing:

#define _PAGE_PRESENT_SHIFT 0
#define _PAGE_READ_SHIFT 0
#define _PAGE_WRITE_SHIFT 1
#define _PAGE_ACCESSED_SHIFT 2
#define _PAGE_MODIFIED_SHIFT 3
#define _PAGE_HUGE_SHIFT 4
#define _PAGE_SPLITTING_SHIFT 5
#define _PAGE_NO_EXEC_SHIFT 6
#define _PAGE_NO_READ_SHIFT 7
#define _PAGE_GLOBAL_SHIFT 8
#define _PAGE_VALID_SHIFT 9
#define _PAGE_DIRTY_SHIFT 10
#define _PFN_SHIFT 14

 00000000 <r4000_tlb_refill>:
+   0:	df7a0000 	ld	k0,0(k1)
    4:	00210a3a 	dror	at,at,0x8
    8:	40a11000 	dmtc0	at,c0_entrylo0
    c:	64214000 	daddiu	at,at,16384
   10:	40a11800 	dmtc0	at,c0_entrylo1
   14:	3c1a001f 	lui	k0,0x1f
   18:	375ae000 	ori	k0,k0,0xe000
   1c:	409a2800 	mtc0	k0,c0_pagemask
   20:	000000c0 	ehb
   24:	42000006 	tlbwr
   28:	40802800 	mtc0	zero,c0_pagemask
-  28:	1000002e 	b	e4 <r4000_tlb_refill+0xe4>
+  2c:	1000002d 	b	e4 <r4000_tlb_refill+0xe4>
   30:	4021f802 	dmfc0	at,$31,2
-  30:	07400019 	bltz	k0,98 <r4000_tlb_refill+0x98>
+  34:	07400018 	bltz	k0,98 <r4000_tlb_refill+0x98>
   38:	3c1b81da 	lui	k1,0x81da
   3c:	3c1b8113 	lui	k1,0x8113
-  3c:	277b7ef0 	addiu	k1,k1,32496
+  40:	277b7f00 	addiu	k1,k1,32512
   44:	03600008 	jr	k1
   48:	4021f802 	dmfc0	at,$31,2
 	...
   80:	403a4000 	dmfc0	k0,c0_badvaddr
   84:	403bf803 	dmfc0	k1,$31,3
   88:	40a1f802 	dmtc0	at,$31,2
   8c:	001a0a3e 	dsrl32	at,k0,0x8
-  90:	1420ffe7 	bnez	at,30 <r4000_tlb_refill+0x30>
+  90:	1420ffe8 	bnez	at,34 <r4000_tlb_refill+0x34>
   94:	001a0efa 	dsrl	at,k0,0x1b
   98:	30211ff8 	andi	at,at,0x1ff8
   9c:	7c3bda0a 	ldx	k1,k1(at)
   a0:	001a0cba 	dsrl	at,k0,0x12
   a4:	30210ff8 	andi	at,at,0xff8
   a8:	403aa000 	dmfc0	k0,c0_xcontext
   ac:	7c3b0a0a 	ldx	at,k1(at)
   b0:	335a0ff0 	andi	k0,k0,0xff0
   b4:	e824ffd2 	bbit1	at,0x4,0 <r4000_tlb_refill>
   b8:	00000000 	nop
   bc:	7c3ada0a 	ldx	k1,k0(at)
   c0:	675a0008 	daddiu	k0,k0,8
   c4:	7c3ad20a 	ldx	k0,k0(at)
   c8:	003bda3a 	dror	k1,k1,0x8
   cc:	40bb1000 	dmtc0	k1,c0_entrylo0
   d0:	003ad23a 	dror	k0,k0,0x8
   d4:	40ba1800 	dmtc0	k0,c0_entrylo1
   d8:	4021f802 	dmfc0	at,$31,2
   dc:	000000c0 	ehb
   e0:	42000006 	tlbwr
   e4:	42000018 	eret


b4 is apparently where it branches back to the huge page case at the
beginning. In that case the at register (htlb_info.huge_pte) is set to
*(k1+at) instead of *(k1), so loading to htlb_info.huge_pte instead of
k0 would I think be bad and change the behaviour. So forget my suggestion!

On the other hand loading the pte to k0 is redundant when
build_fast_tlb_refill_handler is used (which depends on bbit1), and also
in the other case if bbit1 is available since it won't get clobbered by
build_is_huge_pte().

Maybe the reload should simply be conditional on !use_bbit_insns()?

Cheers
James

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[David Daney]

On 07/31/2014 04:54 AM, James Hogan wrote:
> Hi,
>
> On 31/07/14 02:13, David Daney wrote:
>> On 07/30/2014 05:48 PM, Huacai Chen wrote:
>>> For non-Octeon CPU, htlb_info.huge_pte is equal to K0, but I don't
>>> know much about Octeon. So I think you know whether we should use K0
>>>   or htlb_info.huge_pte here, since you are the original author.
>>>
>>
>> This is why I requested that somebody show me a disassembly of the
>> faulty handler.  I cannot tell where the problem is unless I see that.
>>
>> Really I think the problem is in build_is_huge_pte(), where we are
>> clobbering 'tmp' which is K0.
>>
>> So you could reload tmp/K0 in build_is_huge_pte().
>
> Here's the difference with this patch (using k0) on an Octeon I have to
> hand, with some slightly munged offsets for nicer diffing:
>
> #define _PAGE_PRESENT_SHIFT 0
> #define _PAGE_READ_SHIFT 0
> #define _PAGE_WRITE_SHIFT 1
> #define _PAGE_ACCESSED_SHIFT 2
> #define _PAGE_MODIFIED_SHIFT 3
> #define _PAGE_HUGE_SHIFT 4
> #define _PAGE_SPLITTING_SHIFT 5
> #define _PAGE_NO_EXEC_SHIFT 6
> #define _PAGE_NO_READ_SHIFT 7
> #define _PAGE_GLOBAL_SHIFT 8
> #define _PAGE_VALID_SHIFT 9
> #define _PAGE_DIRTY_SHIFT 10
> #define _PFN_SHIFT 14
>
>   00000000 <r4000_tlb_refill>:
> +   0:	df7a0000 	ld	k0,0(k1)

Completely redundant, it is not used and then clobbered...

>      4:	00210a3a 	dror	at,at,0x8
>      8:	40a11000 	dmtc0	at,c0_entrylo0
>      c:	64214000 	daddiu	at,at,16384
>     10:	40a11800 	dmtc0	at,c0_entrylo1
>     14:	3c1a001f 	lui	k0,0x1f

... here.

>     18:	375ae000 	ori	k0,k0,0xe000
>     1c:	409a2800 	mtc0	k0,c0_pagemask
>     20:	000000c0 	ehb
>     24:	42000006 	tlbwr
>     28:	40802800 	mtc0	zero,c0_pagemask
> -  28:	1000002e 	b	e4 <r4000_tlb_refill+0xe4>
> +  2c:	1000002d 	b	e4 <r4000_tlb_refill+0xe4>
>     30:	4021f802 	dmfc0	at,$31,2
> -  30:	07400019 	bltz	k0,98 <r4000_tlb_refill+0x98>
> +  34:	07400018 	bltz	k0,98 <r4000_tlb_refill+0x98>
>     38:	3c1b81da 	lui	k1,0x81da
>     3c:	3c1b8113 	lui	k1,0x8113
> -  3c:	277b7ef0 	addiu	k1,k1,32496
> +  40:	277b7f00 	addiu	k1,k1,32512
>     44:	03600008 	jr	k1
>     48:	4021f802 	dmfc0	at,$31,2
>   	...
>     80:	403a4000 	dmfc0	k0,c0_badvaddr
>     84:	403bf803 	dmfc0	k1,$31,3
>     88:	40a1f802 	dmtc0	at,$31,2
>     8c:	001a0a3e 	dsrl32	at,k0,0x8
> -  90:	1420ffe7 	bnez	at,30 <r4000_tlb_refill+0x30>
> +  90:	1420ffe8 	bnez	at,34 <r4000_tlb_refill+0x34>
>     94:	001a0efa 	dsrl	at,k0,0x1b
>     98:	30211ff8 	andi	at,at,0x1ff8
>     9c:	7c3bda0a 	ldx	k1,k1(at)
>     a0:	001a0cba 	dsrl	at,k0,0x12
>     a4:	30210ff8 	andi	at,at,0xff8
>     a8:	403aa000 	dmfc0	k0,c0_xcontext
>     ac:	7c3b0a0a 	ldx	at,k1(at)
>     b0:	335a0ff0 	andi	k0,k0,0xff0
>     b4:	e824ffd2 	bbit1	at,0x4,0 <r4000_tlb_refill>
>     b8:	00000000 	nop
>     bc:	7c3ada0a 	ldx	k1,k0(at)
>     c0:	675a0008 	daddiu	k0,k0,8
>     c4:	7c3ad20a 	ldx	k0,k0(at)
>     c8:	003bda3a 	dror	k1,k1,0x8
>     cc:	40bb1000 	dmtc0	k1,c0_entrylo0
>     d0:	003ad23a 	dror	k0,k0,0x8
>     d4:	40ba1800 	dmtc0	k0,c0_entrylo1
>     d8:	4021f802 	dmfc0	at,$31,2
>     dc:	000000c0 	ehb
>     e0:	42000006 	tlbwr
>     e4:	42000018 	eret
>
>
> b4 is apparently where it branches back to the huge page case at the
> beginning. In that case the at register (htlb_info.huge_pte) is set to
> *(k1+at) instead of *(k1), so loading to htlb_info.huge_pte instead of
> k0 would I think be bad and change the behaviour. So forget my suggestion!
>
> On the other hand loading the pte to k0 is redundant when
> build_fast_tlb_refill_handler is used (which depends on bbit1), and also
> in the other case if bbit1 is available since it won't get clobbered by
> build_is_huge_pte().
>
> Maybe the reload should simply be conditional on !use_bbit_insns()?
>

That was kind of my suggestion.  What happens if you do something like 
(untested):

--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -716,6 +716,7 @@ build_is_huge_pte(u32 **p, struct uasm_reloc **r, 
unsigned int tmp,
         } else {
                 uasm_i_andi(p, tmp, tmp, _PAGE_HUGE);
                 uasm_il_bnez(p, r, tmp, lid);
+               UASM_i_LW(p, tmp, 0, pmd);
         }
  }

or

diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
index f99ec587..341add1 100644
--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -1299,6 +1299,8 @@ static void build_r4000_tlb_refill_handler(void)
         }
  #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
         uasm_l_tlb_huge_update(&l, p);
+       if (!use_bbit_insns())
+               UASM_i_LW(&p, K0, 0, K1);
         build_huge_update_entries(&p, htlb_info.huge_pte, K1);
         build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
                                    htlb_info.restore_scratch);


> Cheers
> James
>
>

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[Aurelien Jarno]

On Thu, Jul 31, 2014 at 10:33:55AM -0700, David Daney wrote:
> On 07/31/2014 04:54 AM, James Hogan wrote:
> >Hi,
> >
> >On 31/07/14 02:13, David Daney wrote:
> >>On 07/30/2014 05:48 PM, Huacai Chen wrote:
> >>>For non-Octeon CPU, htlb_info.huge_pte is equal to K0, but I don't
> >>>know much about Octeon. So I think you know whether we should use K0
> >>>  or htlb_info.huge_pte here, since you are the original author.
> >>>
> >>
> >>This is why I requested that somebody show me a disassembly of the
> >>faulty handler.  I cannot tell where the problem is unless I see that.

Here is the faulty handler, that is a dump on a machine affected by the
bug:

| #define _PAGE_PRESENT_SHIFT 0
| #define _PAGE_READ_SHIFT 1
| #define _PAGE_WRITE_SHIFT 2
| #define _PAGE_ACCESSED_SHIFT 3
| #define _PAGE_MODIFIED_SHIFT 4
| #define _PAGE_HUGE_SHIFT 5
| #define _PAGE_SPLITTING_SHIFT 6
| #define _PAGE_GLOBAL_SHIFT 7
| #define _PAGE_VALID_SHIFT 8
| #define _PAGE_DIRTY_SHIFT 9
| #define _PFN_SHIFT 13
| 
| 0000000000000000 <r4000_tlb_refill>:
|    0:	001ad1fa 	dsrl	k0,k0,0x7
|    4:	40ba1000 	dmtc0	k0,c0_entrylo0
|    8:	675a4000 	daddiu	k0,k0,16384
|    c:	40ba1800 	dmtc0	k0,c0_entrylo1
|   10:	3c1a001f 	lui	k0,0x1f
|   14:	375ae000 	ori	k0,k0,0xe000
|   18:	409a2800 	mtc0	k0,c0_pagemask
|   1c:	42000006 	tlbwr
|   20:	10000031 	b	e8 <r4000_tlb_refill+0xe8>
|   24:	40802800 	mtc0	zero,c0_pagemask
|   28:	10000019 	b	90 <r4000_tlb_refill+0x90>
|   2c:	3c1b8095 	lui	k1,0x8095
| 	...
|   80:	403a4000 	dmfc0	k0,c0_badvaddr
|   84:	0740ffe8 	bltz	k0,28 <r4000_tlb_refill+0x28>
|   88:	3c1b8095 	lui	k1,0x8095
|   8c:	df7b4fb0 	ld	k1,20400(k1)
|   90:	001ad6fa 	dsrl	k0,k0,0x1b
|   94:	335a1ff8 	andi	k0,k0,0x1ff8
|   98:	037ad82d 	daddu	k1,k1,k0
|   9c:	403a4000 	dmfc0	k0,c0_badvaddr
|   a0:	df7b0000 	ld	k1,0(k1)
|   a4:	001ad4ba 	dsrl	k0,k0,0x12
|   a8:	335a0ff8 	andi	k0,k0,0xff8
|   ac:	037ad82d 	daddu	k1,k1,k0
|   b0:	df7a0000 	ld	k0,0(k1)
|   b4:	335a0020 	andi	k0,k0,0x20
|   b8:	1740ffd1 	bnez	k0,0 <r4000_tlb_refill>
|   bc:	403aa000 	dmfc0	k0,c0_xcontext
|   c0:	df7b0000 	ld	k1,0(k1)
|   c4:	335a0ff0 	andi	k0,k0,0xff0
|   c8:	037ad82d 	daddu	k1,k1,k0
|   cc:	df7a0000 	ld	k0,0(k1)
|   d0:	df7b0008 	ld	k1,8(k1)
|   d4:	001ad1fa 	dsrl	k0,k0,0x7
|   d8:	40ba1000 	dmtc0	k0,c0_entrylo0
|   dc:	001bd9fa 	dsrl	k1,k1,0x7
|   e0:	40bb1800 	dmtc0	k1,c0_entrylo1
|   e4:	42000006 	tlbwr
|   e8:	42000018 	eret
| 	...

> >>Really I think the problem is in build_is_huge_pte(), where we are
> >>clobbering 'tmp' which is K0.

Indeed, that is the problem. In the above code build_is_huge_pte()
corresponds to addresses b4 and b8. It needs huge_pte loaded in K0, but
at the same time it clobbers it. That's the reason why prior to commit
2c8c53e28f1, k0 was reloaded before calling build_huge_update_entries.

> >>So you could reload tmp/K0 in build_is_huge_pte().
> >
> >b4 is apparently where it branches back to the huge page case at the
> >beginning. In that case the at register (htlb_info.huge_pte) is set to
> >*(k1+at) instead of *(k1), so loading to htlb_info.huge_pte instead of
> >k0 would I think be bad and change the behaviour. So forget my suggestion!
> >
> >On the other hand loading the pte to k0 is redundant when
> >build_fast_tlb_refill_handler is used (which depends on bbit1), and also
> >in the other case if bbit1 is available since it won't get clobbered by
> >build_is_huge_pte().
> >
> >Maybe the reload should simply be conditional on !use_bbit_insns()?
> >
> 
> That was kind of my suggestion.  What happens if you do something
> like (untested):
> 
> --- a/arch/mips/mm/tlbex.c
> +++ b/arch/mips/mm/tlbex.c
> @@ -716,6 +716,7 @@ build_is_huge_pte(u32 **p, struct uasm_reloc
> **r, unsigned int tmp,
>         } else {
>                 uasm_i_andi(p, tmp, tmp, _PAGE_HUGE);
>                 uasm_il_bnez(p, r, tmp, lid);
> +               UASM_i_LW(p, tmp, 0, pmd);
>         }
>  }

I haven't tested it, but it my opinion this patch won't work or at least
is suboptimal given build_is_huge_pte is also used to build
r4000_tlb_load, r4000_tlb_store and r4000_tlb_modify handlers.

> or
> 
> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> index f99ec587..341add1 100644
> --- a/arch/mips/mm/tlbex.c
> +++ b/arch/mips/mm/tlbex.c
> @@ -1299,6 +1299,8 @@ static void build_r4000_tlb_refill_handler(void)
>         }
>  #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
>         uasm_l_tlb_huge_update(&l, p);
> +       if (!use_bbit_insns())
> +               UASM_i_LW(&p, K0, 0, K1);
>         build_huge_update_entries(&p, htlb_info.huge_pte, K1);
>         build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
>                                    htlb_info.restore_scratch);

This patch fixes the issue, thanks. That said it doesn't look fully
correct. The test should be done the same way as for
build_fast_tlb_refill_handler. For example the fast handler is not
called on a 32-bit machine with bbit instructions, so it would need
to reload K0.

Now I do wonder if we should add yet another test there, or simply move
and duplicate these 3 or 4 lines in the fast and non-fast branches. At 
least it will improve readability.

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[James Hogan]

Hi Aurelien,

On 02/08/14 22:35, Aurelien Jarno wrote:
> On Thu, Jul 31, 2014 at 10:33:55AM -0700, David Daney wrote:
>> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
>> index f99ec587..341add1 100644
>> --- a/arch/mips/mm/tlbex.c
>> +++ b/arch/mips/mm/tlbex.c
>> @@ -1299,6 +1299,8 @@ static void build_r4000_tlb_refill_handler(void)
>>         }
>>  #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
>>         uasm_l_tlb_huge_update(&l, p);
>> +       if (!use_bbit_insns())
>> +               UASM_i_LW(&p, K0, 0, K1);
>>         build_huge_update_entries(&p, htlb_info.huge_pte, K1);
>>         build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
>>                                    htlb_info.restore_scratch);
> 
> This patch fixes the issue, thanks. That said it doesn't look fully
> correct. The test should be done the same way as for
> build_fast_tlb_refill_handler. For example the fast handler is not
> called on a 32-bit machine with bbit instructions, so it would need
> to reload K0.

In the non fast case build_is_huge_pte() will still use bbit1 if
available after restoring K0, and I don't think the bbit1 would clobber
K0 when the branch is taken, so I think the test for !use_bbit_insns()
is correct.

Cheers
James

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[Aurelien Jarno]

On Mon, Aug 04, 2014 at 11:08:50AM +0100, James Hogan wrote:
> Hi Aurelien,
> 
> On 02/08/14 22:35, Aurelien Jarno wrote:
> > On Thu, Jul 31, 2014 at 10:33:55AM -0700, David Daney wrote:
> >> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
> >> index f99ec587..341add1 100644
> >> --- a/arch/mips/mm/tlbex.c
> >> +++ b/arch/mips/mm/tlbex.c
> >> @@ -1299,6 +1299,8 @@ static void build_r4000_tlb_refill_handler(void)
> >>         }
> >>  #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
> >>         uasm_l_tlb_huge_update(&l, p);
> >> +       if (!use_bbit_insns())
> >> +               UASM_i_LW(&p, K0, 0, K1);
> >>         build_huge_update_entries(&p, htlb_info.huge_pte, K1);
> >>         build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
> >>                                    htlb_info.restore_scratch);
> > 
> > This patch fixes the issue, thanks. That said it doesn't look fully
> > correct. The test should be done the same way as for
> > build_fast_tlb_refill_handler. For example the fast handler is not
> > called on a 32-bit machine with bbit instructions, so it would need
> > to reload K0.
> 
> In the non fast case build_is_huge_pte() will still use bbit1 if
> available after restoring K0, and I don't think the bbit1 would clobber
> K0 when the branch is taken, so I think the test for !use_bbit_insns()
> is correct.
> 
Oh you are right! Therefore this second patch is:

Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
Tested-by: Aurelien Jarno <aurelien@aurel32.net>
-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[James Hogan]

On 04/08/14 14:05, Aurelien Jarno wrote:
> On Mon, Aug 04, 2014 at 11:08:50AM +0100, James Hogan wrote:
>> Hi Aurelien,
>>
>> On 02/08/14 22:35, Aurelien Jarno wrote:
>>> On Thu, Jul 31, 2014 at 10:33:55AM -0700, David Daney wrote:
>>>> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
>>>> index f99ec587..341add1 100644
>>>> --- a/arch/mips/mm/tlbex.c
>>>> +++ b/arch/mips/mm/tlbex.c
>>>> @@ -1299,6 +1299,8 @@ static void build_r4000_tlb_refill_handler(void)
>>>>         }
>>>>  #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
>>>>         uasm_l_tlb_huge_update(&l, p);
>>>> +       if (!use_bbit_insns())
>>>> +               UASM_i_LW(&p, K0, 0, K1);
>>>>         build_huge_update_entries(&p, htlb_info.huge_pte, K1);
>>>>         build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
>>>>                                    htlb_info.restore_scratch);
>>>
>>> This patch fixes the issue, thanks. That said it doesn't look fully
>>> correct. The test should be done the same way as for
>>> build_fast_tlb_refill_handler. For example the fast handler is not
>>> called on a 32-bit machine with bbit instructions, so it would need
>>> to reload K0.
>>
>> In the non fast case build_is_huge_pte() will still use bbit1 if
>> available after restoring K0, and I don't think the bbit1 would clobber
>> K0 when the branch is taken, so I think the test for !use_bbit_insns()
>> is correct.
>>
> Oh you are right! Therefore this second patch is:
> 
> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>

Likewise:

Reviewed-by: James Hogan <james.hogan@imgtec.com>

Cheers
James

> Tested-by: Aurelien Jarno <aurelien@aurel32.net>
> 

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[Huacai Chen]

Now what should we do? My original patch is not perfect, but it has
already merged into Ralf's tree (but hasn't merged in Linus's tree).
Let me send Ralf a new version of this patch? Or let David send
another patch on top of my original one?

Huacai

On Mon, Aug 4, 2014 at 9:10 PM, James Hogan <james.hogan@imgtec.com> wrote:
> On 04/08/14 14:05, Aurelien Jarno wrote:
>> On Mon, Aug 04, 2014 at 11:08:50AM +0100, James Hogan wrote:
>>> Hi Aurelien,
>>>
>>> On 02/08/14 22:35, Aurelien Jarno wrote:
>>>> On Thu, Jul 31, 2014 at 10:33:55AM -0700, David Daney wrote:
>>>>> diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
>>>>> index f99ec587..341add1 100644
>>>>> --- a/arch/mips/mm/tlbex.c
>>>>> +++ b/arch/mips/mm/tlbex.c
>>>>> @@ -1299,6 +1299,8 @@ static void build_r4000_tlb_refill_handler(void)
>>>>>         }
>>>>>  #ifdef CONFIG_MIPS_HUGE_TLB_SUPPORT
>>>>>         uasm_l_tlb_huge_update(&l, p);
>>>>> +       if (!use_bbit_insns())
>>>>> +               UASM_i_LW(&p, K0, 0, K1);
>>>>>         build_huge_update_entries(&p, htlb_info.huge_pte, K1);
>>>>>         build_huge_tlb_write_entry(&p, &l, &r, K0, tlb_random,
>>>>>                                    htlb_info.restore_scratch);
>>>>
>>>> This patch fixes the issue, thanks. That said it doesn't look fully
>>>> correct. The test should be done the same way as for
>>>> build_fast_tlb_refill_handler. For example the fast handler is not
>>>> called on a 32-bit machine with bbit instructions, so it would need
>>>> to reload K0.
>>>
>>> In the non fast case build_is_huge_pte() will still use bbit1 if
>>> available after restoring K0, and I don't think the bbit1 would clobber
>>> K0 when the branch is taken, so I think the test for !use_bbit_insns()
>>> is correct.
>>>
>> Oh you are right! Therefore this second patch is:
>>
>> Reviewed-by: Aurelien Jarno <aurelien@aurel32.net>
>
> Likewise:
>
> Reviewed-by: James Hogan <james.hogan@imgtec.com>
>
> Cheers
> James
>
>> Tested-by: Aurelien Jarno <aurelien@aurel32.net>
>>
>
>

Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB

[Ralf Baechle]

On Tue, Aug 05, 2014 at 03:09:04PM +0800, Huacai Chen wrote:

> Now what should we do? My original patch is not perfect, but it has
> already merged into Ralf's tree (but hasn't merged in Linus's tree).
> Let me send Ralf a new version of this patch? Or let David send
> another patch on top of my original one?

I'll create an incremental patch so you don't need to do anything.

  Ralf