|To:||"Maciej W. Rozycki" <email@example.com>|
|Subject:||Re: [PATCH RFC v2 41/70] MIPS: mm: tlbex: Use cpu_has_mips_r2_exec_hazard for the EHB instruction|
|From:||David Daney <firstname.lastname@example.org>|
|Date:||Mon, 23 Feb 2015 16:53:37 -0800|
|Cc:||Markos Chandras <email@example.com>, Ralf Baechle <firstname.lastname@example.org>, email@example.com, Leonid Yegoshin <Leonid.Yegoshin@imgtec.com>|
|Dkim-signature:||v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=PGuutdCIwtPRK41BR3YzNJcatiPB2rdMvxMhpepkMKk=; b=FT0o5yl8tNS/w+g29V5AGt/5B9ULNU3PQxnUp9bS6SyBGWex96D6QWZyRUE/SLiGir 7PwhDXuB9FoRV2S9zfGJEj/Z0iHRMJWTH1MXGfjwUe+QCMoamHUQikNgk36HJiozS63v D63+4Fez63K0K1lKl3fjMSrETTSnr1pdgYyhfg2j1UzlltcTkiF+fy0of21mM6wRHlTp iSpoIjhlJ1jiLIFwuxHdZi0xL1nCXoUMqzz+3t7NhCi0q3q0lBqHrAiviT9c+paCJLbN gKCvMJKXZzTduE5QpH2lp3Mg42O/eLaEdMxkUFlKGhBAVrm0k2D0kJFHaP+MFP2Ge9Fn dbuw==|
|List-software:||Ecartis version 1.0.0|
|References:||<firstname.lastname@example.org> <email@example.com> <54EBA3C3.firstname.lastname@example.org> <alpine.LFD.email@example.com>|
|User-agent:||Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7|
On 02/23/2015 04:33 PM, Maciej W. Rozycki wrote:
On Mon, 23 Feb 2015, David Daney wrote:For the version of this patch currently in mips-for-linux-next: NACK There are two problems: 1) It breaks OCTEON, which will now crash in early boot with: Kernel panic - not syncing: No TLB refill handler yet (CPU type: 80) 2) The logic is broken. The meaning of cpu_has_mips_r2_exec_hazard is that the EHB instruction is required. You change the meaning to be that EHB is part of the ISA.Well, the macro is nowhere used I'm afraid, its last use was dropped with 625c0a21, so it's rather difficult to assume any meaning to the macro. Also the intended meaning is clear from the commit message of 41f0e4d0, where the macro comes from, however unfortunately not from the definition of the macro itself. It's a pity that along your change you did not include an explanatory note in arch/mips/include/asm/cpu-features.h. Finally, I think the change made to `build_tlb_write_entry' with 625c0a21 may need to be reconsidered, as may perhaps the name itself of `cpu_has_mips_r2_exec_hazard' (why is it this place only that the macro was used? -- would it be better called `cpu_has_tlbw_exec_hazard' instead?), and then we'll need `cpu_has_ehb' or suchlike across all the other places.
I agree with all the points you make.The parts of tlbex.c dealing with EHB are an inconsistent mess. Since it is one of my favorite playgrounds I am probably as guilty as anyone.
The idea behind `cpu_has_mips_r2_exec_hazard' (although a better name could have been chosen, as you point out) was to get rid of the case statements based on CPU model in tlbex.c, and move the logic into the individual cpu-feature files. I thought this would make it easier to add new CPUs in the future, but it turns out that now we have a random assortment of both :-(.
|<Prev in Thread]||Current Thread||[Next in Thread>|
|Previous by Date:||Re: [PATCH RFC v2 41/70] MIPS: mm: tlbex: Use cpu_has_mips_r2_exec_hazard for the EHB instruction, Maciej W. Rozycki|
|Next by Date:||Re: [PATCH V2 1/3] MIPS: Fix cache flushing for swap pages with non-DMA I/O., Leonid Yegoshin|
|Previous by Thread:||Re: [PATCH RFC v2 41/70] MIPS: mm: tlbex: Use cpu_has_mips_r2_exec_hazard for the EHB instruction, Maciej W. Rozycki|
|Next by Thread:||[PATCH] Revert "MIPS: mm: tlbex: Use cpu_has_mips_r2_exec_hazard for the EHB instruction", David Daney|
|Indexes:||[Date] [Thread] [Top] [All Lists]|