Re: [PATCH RFC v2 01/10] MIPS: Add SysRq operation to dump TLBs on all CPUs

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]

On 26/05/15 12:58, Maciej W. Rozycki wrote:
> On Sun, 24 May 2015, Joshua Kinard wrote:
> 
>>> Add a MIPS specific SysRq operation to dump the TLB entries on all CPUs,
>>> using the 'x' trigger key.
>>
>> Thought: Would it make sense to split apart the data such that one SysRq key
>> dumps the CP0 registers of all CPUs, and another dumps the TLB info?
> 
>  That would be a large separate project, probing a CPU for its implemented 
> CP0 registers is a complex matter.
> 
>  I did it for GDB and a bare-iron debug stub a few years ago and back then 
> there were IIRC 53 register subsets already defined for MIPS architecture 
> processors, wired to various, sometimes overlapping feature bits of CP0 
> Config registers, and now there are more.  Plus legacy processors require 
> fixed register maps according to CP0.PRId.
> 
>  James, I think what you proposed is good enough for TLB diagnostics (I'm 
> not sure if dumping EntryLo0 and EntryLo1 registers has any use, but it 
> surely does not hurt either).
> 
>>> +	pr_info("CPU%d:\n", smp_processor_id());
>>> +	pr_info("Index	: %0x\n", read_c0_index());
>>> +	pr_info("Pagemask: %0x\n", read_c0_pagemask());
>>> +	pr_info("EntryHi : %0*lx\n", field, read_c0_entryhi());
>>> +	pr_info("EntryLo0: %0*lx\n", field, read_c0_entrylo0());
>>> +	pr_info("EntryLo1: %0*lx\n", field, read_c0_entrylo1());
>>> +	pr_info("Wired   : %0x\n", read_c0_wired());
>>> +	pr_info("Pagegrain: %0x\n", read_c0_pagegrain());
> 
>  Please capitalise these consistently: PageMask and PageGrain.
> 
>> The older CPUs, like the R10000 don't have a PageGrain register I believe (at
>> least R10K doesn't),  Does that need to be stuffed behind a conditional?  Also,
>> R10K (and newer?) CPUs have a FrameMask CP0 register ($21).  Linux currently
>> scribbles a 0 to the writable bits, though, so I'm not sure if it matters.
> 
>  First of all I suggest that this part is split off into separate small 
> helper functions within dump_tlb.c and r3k_dump_tlb.c.  This code is not 
> performance-critical, so the overhead of an extra function call isn't of 
> a concern.
> 
>  Then R3k processors have Index, EntryHi and EntryLo (rather than 
> EntryLo0) registers only; some Toshiba processors have Wired too (cf. 
> `r3k_have_wired_reg').
> 
>  And for the R4k-style TLB the PageGrain register does need to be probed 
> for.  I think including FrameMask would be good too, that shouldn't be 
> difficult (switch on `current_cpu_type'?).

Thanks for all the feedback Joshua and Maciej, and sorry for the delay
getting back to this. I think I've addressed it all now, so I'll submit
a second patchset soon.

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]