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 :-(.
David Daney
