Re: [PATCH v7 3/7] powerpc/pmem: Add flush routines using new pmem store and sync instruction

["Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com>]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:

> Le 01/07/2020 à 09:22, Aneesh Kumar K.V a écrit :
>> Start using dcbstps; phwsync; sequence for flushing persistent memory range.
>> The new instructions are implemented as a variant of dcbf and hwsync and on
>> P8 and P9 they will be executed as those instructions. We avoid using them on
>> older hardware. This helps to avoid difficult to debug bugs.
>> 
>
> Before this patch, the flush was done for all.
> After this patch, IIUC the flush is done only on CPUs having feature 
> CPU_FTR_ARCH_207S.
>
> What about other CPUs ?
>
> I don't know much about PMEM, my concern is about the UACCESS_FLUSHCACHE 
> API introduced by commit 6c44741d75a2 ("powerpc/lib: Implement 
> UACCESS_FLUSHCACHE API")
>
> After your patch, __copy_from_user_flushcache() and memcpy_flushcache() 
> are not doing cache flush anymore.
>
> Is that intended ?

yes, with the understanding that these functions are used with
persistent memory . We restrict the persistent memory usage to p8 and
above via commit c83040192f3763b243ece26073d61a895b4a230f

>
> I'm trying to optimise some ALSA driver that does copy_from_user + 
> cache_flush for DMA, and I was wondering if using 
> __copy_from_user_flushcache() was an alternative.
>
> Or is it __copy_from_user_inatomic_nocache() which has to be done for that ?
>
> Thanks
> Christophe
>
>
>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
>> ---
>>   arch/powerpc/include/asm/cacheflush.h |  1 +
>>   arch/powerpc/lib/pmem.c               | 50 ++++++++++++++++++++++++---
>>   2 files changed, 47 insertions(+), 4 deletions(-)
>> 
>> diff --git a/arch/powerpc/include/asm/cacheflush.h b/arch/powerpc/include/asm/cacheflush.h
>> index de600b915a3c..54764c6e922d 100644
>> --- a/arch/powerpc/include/asm/cacheflush.h
>> +++ b/arch/powerpc/include/asm/cacheflush.h
>> @@ -6,6 +6,7 @@
>>   
>>   #include <linux/mm.h>
>>   #include <asm/cputable.h>
>> +#include <asm/cpu_has_feature.h>
>>   
>>   #ifdef CONFIG_PPC_BOOK3S_64
>>   /*
>> diff --git a/arch/powerpc/lib/pmem.c b/arch/powerpc/lib/pmem.c
>> index 0666a8d29596..5a61aaeb6930 100644
>> --- a/arch/powerpc/lib/pmem.c
>> +++ b/arch/powerpc/lib/pmem.c
>> @@ -9,20 +9,62 @@
>>   
>>   #include <asm/cacheflush.h>
>>   
>> +static inline void __clean_pmem_range(unsigned long start, unsigned long stop)
>> +{
>> +	unsigned long shift = l1_dcache_shift();
>> +	unsigned long bytes = l1_dcache_bytes();
>> +	void *addr = (void *)(start & ~(bytes - 1));
>> +	unsigned long size = stop - (unsigned long)addr + (bytes - 1);
>> +	unsigned long i;
>> +
>> +	for (i = 0; i < size >> shift; i++, addr += bytes)
>> +		asm volatile(PPC_DCBSTPS(%0, %1): :"i"(0), "r"(addr): "memory");
>> +
>> +
>> +	asm volatile(PPC_PHWSYNC ::: "memory");
>> +}
>> +
>> +static inline void __flush_pmem_range(unsigned long start, unsigned long stop)
>> +{
>> +	unsigned long shift = l1_dcache_shift();
>> +	unsigned long bytes = l1_dcache_bytes();
>> +	void *addr = (void *)(start & ~(bytes - 1));
>> +	unsigned long size = stop - (unsigned long)addr + (bytes - 1);
>> +	unsigned long i;
>> +
>> +	for (i = 0; i < size >> shift; i++, addr += bytes)
>> +		asm volatile(PPC_DCBFPS(%0, %1): :"i"(0), "r"(addr): "memory");
>> +
>> +
>> +	asm volatile(PPC_PHWSYNC ::: "memory");
>> +}
>> +
>> +static inline void clean_pmem_range(unsigned long start, unsigned long stop)
>> +{
>> +	if (cpu_has_feature(CPU_FTR_ARCH_207S))
>> +		return __clean_pmem_range(start, stop);
>> +}
>> +
>> +static inline void flush_pmem_range(unsigned long start, unsigned long stop)
>> +{
>> +	if (cpu_has_feature(CPU_FTR_ARCH_207S))
>> +		return __flush_pmem_range(start, stop);
>> +}
>> +
>>   /*
>>    * CONFIG_ARCH_HAS_PMEM_API symbols
>>    */
>>   void arch_wb_cache_pmem(void *addr, size_t size)
>>   {
>>   	unsigned long start = (unsigned long) addr;
>> -	flush_dcache_range(start, start + size);
>> +	clean_pmem_range(start, start + size);
>>   }
>>   EXPORT_SYMBOL_GPL(arch_wb_cache_pmem);
>>   
>>   void arch_invalidate_pmem(void *addr, size_t size)
>>   {
>>   	unsigned long start = (unsigned long) addr;
>> -	flush_dcache_range(start, start + size);
>> +	flush_pmem_range(start, start + size);
>>   }
>>   EXPORT_SYMBOL_GPL(arch_invalidate_pmem);
>>   
>> @@ -35,7 +77,7 @@ long __copy_from_user_flushcache(void *dest, const void __user *src,
>>   	unsigned long copied, start = (unsigned long) dest;
>>   
>>   	copied = __copy_from_user(dest, src, size);
>> -	flush_dcache_range(start, start + size);
>> +	clean_pmem_range(start, start + size);
>>   
>>   	return copied;
>>   }
>> @@ -45,7 +87,7 @@ void *memcpy_flushcache(void *dest, const void *src, size_t size)
>>   	unsigned long start = (unsigned long) dest;
>>   
>>   	memcpy(dest, src, size);
>> -	flush_dcache_range(start, start + size);
>> +	clean_pmem_range(start, start + size);
>>   
>>   	return dest;
>>   }