On Tue, May 12, 2009 at 9:12 PM, David Daney <ddaney@xxxxxxxxxxxxxxxxxx> wrote:
> David VomLehn wrote:
>>
>> On Tue, May 12, 2009 at 03:45:16PM -0700, David Daney wrote:
>>>
>>> If the TLB refill handler is too bit and needs to be split, there is
minor nit - s/bit/big/
>>> no need to branch around the split if the branch target would be an
>>> eret. Since the eret returns from the handler, control flow never
>>> passes it. A branch to an eret is equivalent to the eret itself.
>>>
>>> Signed-off-by: David Daney <ddaney@xxxxxxxxxxxxxxxxxx>
...
>>> + u32 *split;
>>> + if (split_on_eret) {
>>> + split = tlb_handler + 32;
>>> + } else {
>>> + split = tlb_handler + 30;
>>
>> It would be a shame to pass up an opportunity to eliminate some of the
>> pile of magic constants we have in the MIPS tree. Given that the MIPS
>> documentation describes the size of the TLB Refill handler as 0x80 bytes,
>> we could add something like:
>>
>
> That would be a different patch according to the one patch per problem rule.
I don't see that as a showstopper; just replace the 30 with DVL's
definition in one separate patch that does nothing more, and then do
the conditional "-2" part in a separate patch. Sure, it may seem like
extra cycles for nothing at this point in time, but it pays off in the
long run for folks doing a bisect on changes etc. and it improves the
readability of changesets (by having cleanups separated from technical
changes). If you can spare the cycles, I know I would at least
appreciate the effort (and so would anyone else who ends up doing a
back or forward port.)
>
>> /* Maximum # of instructions in exception vector for TLB Refill handler */
>> #define MIPS64_TLB_REFILL_OPS (0x80 / sizeof(union mips_instruction))
>>
>> and could change the last few lines of the code above to, for example:
>>
>> if (split_on_eret) {
>> split = tlb_handler + MIPS64_TLB_REFILL_OPS;
>> } else {
>> split = tlb_handler + MIPS64_TLB_REFILL_OPS - 2;
>>
>> (you'd need to include asm/inst.h to get union mips_instruction defined)
>
> It is certainly possible to do something like that, but it isn't clear to me
> that it makes it any easier to understand.
Well, for someone like me who doesn't know the low-level arch details,
I'd agree with DVL that it is easier to google "mips tlb refill" than
it is to google "32". So it is probably a worthwhile change IMHO.
Paul.
