[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-mips
Subject:    Re: [PATCH] MIPS: tlbex: fix a missing statement for HUGETLB
From:       David Daney <ddaney () caviumnetworks ! com>
Date:       2014-07-31 17:33:55
Message-ID: 53DA7E03.9090306 () caviumnetworks ! com
[Download RAW message or body]

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


[prev in list] [next in list] [prev in thread] [next in thread]