Re: [PATCH v6 5/8] powerpc/pmem/of_pmem: Update of_pmem to use the new barrier instruction.

2020-06-30 Thread Dan Williams
On Mon, Jun 29, 2020 at 10:05 PM Aneesh Kumar K.V
 wrote:
>
> Dan Williams  writes:
>
> > On Mon, Jun 29, 2020 at 6:58 AM Aneesh Kumar K.V
> >  wrote:
> >>
> >> of_pmem on POWER10 can now use phwsync instead of hwsync to ensure
> >> all previous writes are architecturally visible for the platform
> >> buffer flush.
> >>
> >> Signed-off-by: Aneesh Kumar K.V 
> >> ---
> >>  arch/powerpc/include/asm/cacheflush.h | 7 +++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/powerpc/include/asm/cacheflush.h 
> >> b/arch/powerpc/include/asm/cacheflush.h
> >> index 54764c6e922d..95782f77d768 100644
> >> --- a/arch/powerpc/include/asm/cacheflush.h
> >> +++ b/arch/powerpc/include/asm/cacheflush.h
> >> @@ -98,6 +98,13 @@ static inline void invalidate_dcache_range(unsigned 
> >> long start,
> >> mb();   /* sync */
> >>  }
> >>
> >> +#define arch_pmem_flush_barrier arch_pmem_flush_barrier
> >> +static inline void  arch_pmem_flush_barrier(void)
> >> +{
> >> +   if (cpu_has_feature(CPU_FTR_ARCH_207S))
> >> +   asm volatile(PPC_PHWSYNC ::: "memory");
> >
> > Shouldn't this fallback to a compatible store-fence in an else statement?
>
> The idea was to avoid calling this on anything else. We ensure that by
> making sure that pmem devices are not initialized on systems without that
> cpu feature. Patch 1 does that. Also, the last patch adds a WARN_ON() to
> catch the usage of this outside pmem devices and on systems without that
> cpu feature.

If patch1 handles this why re-check the cpu-feature in this helper? If
the intent is for these routines to be generic why not have them fall
back to the P8 barrier instructions for example like x86 clwb(). Any
kernel code can call it, and it falls back to a compatible clflush()
call on older cpus. I otherwise don't get the point of patch7.


Re: [PATCH v6 5/8] powerpc/pmem/of_pmem: Update of_pmem to use the new barrier instruction.

2020-06-29 Thread Aneesh Kumar K.V
Dan Williams  writes:

> On Mon, Jun 29, 2020 at 6:58 AM Aneesh Kumar K.V
>  wrote:
>>
>> of_pmem on POWER10 can now use phwsync instead of hwsync to ensure
>> all previous writes are architecturally visible for the platform
>> buffer flush.
>>
>> Signed-off-by: Aneesh Kumar K.V 
>> ---
>>  arch/powerpc/include/asm/cacheflush.h | 7 +++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/cacheflush.h 
>> b/arch/powerpc/include/asm/cacheflush.h
>> index 54764c6e922d..95782f77d768 100644
>> --- a/arch/powerpc/include/asm/cacheflush.h
>> +++ b/arch/powerpc/include/asm/cacheflush.h
>> @@ -98,6 +98,13 @@ static inline void invalidate_dcache_range(unsigned long 
>> start,
>> mb();   /* sync */
>>  }
>>
>> +#define arch_pmem_flush_barrier arch_pmem_flush_barrier
>> +static inline void  arch_pmem_flush_barrier(void)
>> +{
>> +   if (cpu_has_feature(CPU_FTR_ARCH_207S))
>> +   asm volatile(PPC_PHWSYNC ::: "memory");
>
> Shouldn't this fallback to a compatible store-fence in an else statement?

The idea was to avoid calling this on anything else. We ensure that by
making sure that pmem devices are not initialized on systems without that
cpu feature. Patch 1 does that. Also, the last patch adds a WARN_ON() to
catch the usage of this outside pmem devices and on systems without that
cpu feature.

-aneesh


Re: [PATCH v6 5/8] powerpc/pmem/of_pmem: Update of_pmem to use the new barrier instruction.

2020-06-29 Thread Dan Williams
On Mon, Jun 29, 2020 at 6:58 AM Aneesh Kumar K.V
 wrote:
>
> of_pmem on POWER10 can now use phwsync instead of hwsync to ensure
> all previous writes are architecturally visible for the platform
> buffer flush.
>
> Signed-off-by: Aneesh Kumar K.V 
> ---
>  arch/powerpc/include/asm/cacheflush.h | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/cacheflush.h 
> b/arch/powerpc/include/asm/cacheflush.h
> index 54764c6e922d..95782f77d768 100644
> --- a/arch/powerpc/include/asm/cacheflush.h
> +++ b/arch/powerpc/include/asm/cacheflush.h
> @@ -98,6 +98,13 @@ static inline void invalidate_dcache_range(unsigned long 
> start,
> mb();   /* sync */
>  }
>
> +#define arch_pmem_flush_barrier arch_pmem_flush_barrier
> +static inline void  arch_pmem_flush_barrier(void)
> +{
> +   if (cpu_has_feature(CPU_FTR_ARCH_207S))
> +   asm volatile(PPC_PHWSYNC ::: "memory");

Shouldn't this fallback to a compatible store-fence in an else statement?


[PATCH v6 5/8] powerpc/pmem/of_pmem: Update of_pmem to use the new barrier instruction.

2020-06-29 Thread Aneesh Kumar K.V
of_pmem on POWER10 can now use phwsync instead of hwsync to ensure
all previous writes are architecturally visible for the platform
buffer flush.

Signed-off-by: Aneesh Kumar K.V 
---
 arch/powerpc/include/asm/cacheflush.h | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/powerpc/include/asm/cacheflush.h 
b/arch/powerpc/include/asm/cacheflush.h
index 54764c6e922d..95782f77d768 100644
--- a/arch/powerpc/include/asm/cacheflush.h
+++ b/arch/powerpc/include/asm/cacheflush.h
@@ -98,6 +98,13 @@ static inline void invalidate_dcache_range(unsigned long 
start,
mb();   /* sync */
 }
 
+#define arch_pmem_flush_barrier arch_pmem_flush_barrier
+static inline void  arch_pmem_flush_barrier(void)
+{
+   if (cpu_has_feature(CPU_FTR_ARCH_207S))
+   asm volatile(PPC_PHWSYNC ::: "memory");
+}
+
 #include 
 
 #endif /* _ASM_POWERPC_CACHEFLUSH_H */
-- 
2.26.2