Re: [PATCH V2 04/12] MIPS: c-r4k: Add r4k_blast_scache_node for Loongson-3

[James Hogan <jhogan@kernel.org>]

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

On Sat, Jan 27, 2018 at 11:19:05AM +0800, Huacai Chen wrote:
> For multi-node Loongson-3 (NUMA configuration), r4k_blast_scache() can
> only flush Node-0's scache. So we add r4k_blast_scache_node() by using
> (CAC_BASE | (node_id << NODE_ADDRSPACE_SHIFT)) instead of CKSEG0 as the
> start address.
> 
> Cc: <stable@vger.kernel.org> # 3.15+
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
> ---
>  arch/mips/include/asm/r4kcache.h | 34 ++++++++++++++++++++++++++++++++
>  arch/mips/mm/c-r4k.c             | 42 +++++++++++++++++++++++++++++++++-------
>  2 files changed, 69 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/mips/include/asm/r4kcache.h b/arch/mips/include/asm/r4kcache.h
> index 7f12d7e..c1f2806 100644
> --- a/arch/mips/include/asm/r4kcache.h
> +++ b/arch/mips/include/asm/r4kcache.h
> @@ -747,4 +747,38 @@ __BUILD_BLAST_CACHE_RANGE(s, scache, Hit_Writeback_Inv_SD, , )
>  __BUILD_BLAST_CACHE_RANGE(inv_d, dcache, Hit_Invalidate_D, , )
>  __BUILD_BLAST_CACHE_RANGE(inv_s, scache, Hit_Invalidate_SD, , )
>  
> +#ifndef pa_to_nid
> +#define pa_to_nid(addr) 0
> +#endif
> +
> +#ifndef NODE_ADDRSPACE_SHIFT

To be sure you get the right definition of both of these if they exist,
and wherever this header is included, we should explicitly #include the
appropriate header (asm/mmzone.h?) from this header.

> +#define nid_to_addrbase(nid) 0
> +#else
> +#define nid_to_addrbase(nid) (nid << NODE_ADDRSPACE_SHIFT)

Technically this should have parentheses around nid.

It seems slightly inconsistent to have pa_to_nid() defined in mmzone.h,
but not the reverse nid_to_addrbase(). NODE_ADDRSPACE_SHIFT is very
loongson specific afterall.

Would it make sense to move it into
arch/mips/include/asm/mach-loongson64/mmzone.h and put the 0 definition
in #ifndef nid_to_addrbase?

> +#endif
> +
> +#define __BUILD_BLAST_CACHE_NODE(pfx, desc, indexop, hitop, lsize)	\

I think this is worthy of a quick comment to explain that this is very
specific to Loongson3.

>  #endif /* _ASM_R4KCACHE_H */
> diff --git a/arch/mips/mm/c-r4k.c b/arch/mips/mm/c-r4k.c
> index 6f534b20..155f5f5 100644
> --- a/arch/mips/mm/c-r4k.c
> +++ b/arch/mips/mm/c-r4k.c

...

> @@ -480,6 +497,10 @@ static inline void local_r4k___flush_cache_all(void * args)
>  		r4k_blast_scache();
>  		break;
>  
> +	case CPU_LOONGSON3:
> +		r4k_blast_scache_node(get_ebase_cpunum() >> 2);

I assume this can't use cpu_to_node() because it needs to work even when
NUMA=n? If so, I think it deserves a brief comment to explain that.

> +		break;
> +
>  	case CPU_BMIPS5000:
>  		r4k_blast_scache();
>  		__sync();
> @@ -839,9 +860,12 @@ static void r4k_dma_cache_wback_inv(unsigned long addr, unsigned long size)
>  
>  	preempt_disable();
>  	if (cpu_has_inclusive_pcaches) {
> -		if (size >= scache_size)
> -			r4k_blast_scache();
> -		else
> +		if (size >= scache_size) {
> +			if (current_cpu_type() != CPU_LOONGSON3)
> +				r4k_blast_scache();
> +			else
> +				r4k_blast_scache_node(pa_to_nid(addr));

If I read this right, addr is a virtual address so this feels a bit
hacky, but I suppose its harmless since it'll probably always be memory
in xkphys space where pa_to_nid() will do the right thing. Perhaps a
comment along the lines of:
/* This assumes that addr is in XKPhys */

> +		} else

Please keep braces consistent, i.e. add to the trailing else statement
too.

Other than those niggles, the actual mechanism looks reasonable to me.

Thanks
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]