Support new pmem flush and sync instructions for POWER

Hi "Aneesh,

I love your patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on linux-nvdimm/libnvdimm-for-next v5.8-rc3 next-20200629] [cannot apply to scottwood/next]
[If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Aneesh-Kumar-K-V/Support-new-pmem-flush-and-sync-instructions-for-POWER/20200629-223649
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: arc-allyesconfig (attached as .config)
compiler: arc-elf-gcc (GCC) 9.3.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc

If you fix the issue, kindly add following tag as appropriate

Reported-by kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

drivers/nvdimm/region_devs.c: In function 'generic_nvdimm_flush':

Quoted 1 line

Quoted 1 line

drivers/nvdimm/region_devs.c:1215:2: error: implicit declaration of function 'arch_pmem_flush_barrier' [-Werror=implicit-function-declaration]

    1215 |  arch_pmem_flush_barrier();
         |  ^~~~~~~~~~~~~~~~~~~~~~~

cc1: some warnings being treated as errors

Architectures like ppc64 provide persistent memory specific barriers that will ensure that all stores for which the modifications are written to persistent storage by preceding dcbfps and dcbstps instructions have updated persistent storage before any data access or data transfer caused by subsequent instructions is initiated. This is in addition to the ordering done by wmb()

Update nvdimm core such that architecture can use barriers other than wmb to ensure all previous writes are architecturally visible for the platform buffer flush.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
 drivers/md/dm-writecache.c   | 2 +-
 drivers/nvdimm/region_devs.c | 8 ++++----
 include/linux/libnvdimm.h    | 4 ++++
 3 files changed, 9 insertions(+), 5 deletions(-)

drivers/md/dm-writecache.c +1 -1

index 74f3c506f084..8c6b6dce64e2 100644
@@ -536,7 +536,7 @@ static void ssd_commit_superblock(struct dm_writecache *wc)
 static void writecache_commit_flushed(struct dm_writecache *wc, bool wait_for_ios)
 {
        if (WC_MODE_PMEM(wc))
-               wmb();
+               arch_pmem_flush_barrier();
        else
                ssd_commit_flushed(wc, wait_for_ios);
 }

drivers/nvdimm/region_devs.c +4 -4

index 4502f9c4708d..b308ad09b63d 100644
@@ -1206,13 +1206,13 @@ int generic_nvdimm_flush(struct nd_region *nd_region)
        idx = this_cpu_add_return(flush_idx, hash_32(current->pid + idx, 8));
 
        /*
-        * The first wmb() is needed to 'sfence' all previous writes
-        * such that they are architecturally visible for the platform
-        * buffer flush.  Note that we've already arranged for pmem
+        * The first arch_pmem_flush_barrier() is needed to 'sfence' all
+        * previous writes such that they are architecturally visible for
+        * the platform buffer flush. Note that we've already arranged for pmem
         * writes to avoid the cache via memcpy_flushcache().  The final
         * wmb() ensures ordering for the NVDIMM flush write.
         */
-       wmb();
+       arch_pmem_flush_barrier();
        for (i = 0; i < nd_region->ndr_mappings; i++)
                if (ndrd_get_flush_wpq(ndrd, i, 0))
                        writeq(1, ndrd_get_flush_wpq(ndrd, i, idx));

include/linux/libnvdimm.h +0 -0

index 18da4059be09..66f6c65bd789 100644

Quoted 59 lines · Aneesh Kumar K.V

Architectures like ppc64 provide persistent memory specific barriers that will ensure that all stores for which the modifications are written to persistent storage by preceding dcbfps and dcbstps instructions have updated persistent storage before any data access or data transfer caused by subsequent instructions is initiated. This is in addition to the ordering done by wmb()

Update nvdimm core such that architecture can use barriers other than wmb to ensure all previous writes are architecturally visible for the platform buffer flush.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
 drivers/md/dm-writecache.c   | 2 +-
 drivers/nvdimm/region_devs.c | 8 ++++----
 include/linux/libnvdimm.h    | 4 ++++
 3 files changed, 9 insertions(+), 5 deletions(-)

drivers/md/dm-writecache.c +1 -1

index 74f3c506f084..8c6b6dce64e2 100644
@@ -536,7 +536,7 @@ static void ssd_commit_superblock(struct dm_writecache *wc)
 static void writecache_commit_flushed(struct dm_writecache *wc, bool wait_for_ios)
 {
        if (WC_MODE_PMEM(wc))
-               wmb();
+               arch_pmem_flush_barrier();
        else
                ssd_commit_flushed(wc, wait_for_ios);
 }

drivers/nvdimm/region_devs.c +4 -4

index 4502f9c4708d..b308ad09b63d 100644
@@ -1206,13 +1206,13 @@ int generic_nvdimm_flush(struct nd_region *nd_region)
        idx = this_cpu_add_return(flush_idx, hash_32(current->pid + idx, 8));
 
        /*
-        * The first wmb() is needed to 'sfence' all previous writes
-        * such that they are architecturally visible for the platform
-        * buffer flush.  Note that we've already arranged for pmem
+        * The first arch_pmem_flush_barrier() is needed to 'sfence' all
+        * previous writes such that they are architecturally visible for
+        * the platform buffer flush. Note that we've already arranged for pmem
         * writes to avoid the cache via memcpy_flushcache().  The final
         * wmb() ensures ordering for the NVDIMM flush write.
         */
-       wmb();
+       arch_pmem_flush_barrier();
        for (i = 0; i < nd_region->ndr_mappings; i++)
                if (ndrd_get_flush_wpq(ndrd, i, 0))
                        writeq(1, ndrd_get_flush_wpq(ndrd, i, idx));

include/linux/libnvdimm.h +0 -0

index 18da4059be09..66f6c65bd789 100644

include/linux/libnvdimm.h static inline void arch_invalidate_pmem(void *addr, size_t size) · line 291

Quoted 6 lines

include/linux/libnvdimm.h +3 -0

@@ -286,4 +286,8 @@ static inline void arch_invalidate_pmem(void *addr, size_t size)
 }
 #endif
 
+#ifndef arch_pmem_flush_barrier
+#define arch_pmem_flush_barrier() wmb()
+#endif

I think it is out of place to define this in libnvdimm.h and it is odd to give it such a long name. The other pmem api helpers like arch_wb_cache_pmem() and arch_invalidate_pmem() are function calls for libnvdimm driver operations, this barrier is just an instruction and is closer to wmb() than the pmem api routine.

Since it is a store fence for pmem, so let's just call it pmem_wmb() and define the generic version in include/linux/compiler.h. It should probably also be documented alongside dma_wmb() in Documentation/memory-barriers.txt about why code would use it over wmb(), and why a symmetric pmem_rmb() is not needed.

Quoted 74 lines · Dan Williams

Quoted 59 lines · Aneesh Kumar K.V

Architectures like ppc64 provide persistent memory specific barriers that will ensure that all stores for which the modifications are written to persistent storage by preceding dcbfps and dcbstps instructions have updated persistent storage before any data access or data transfer caused by subsequent instructions is initiated. This is in addition to the ordering done by wmb()

Update nvdimm core such that architecture can use barriers other than wmb to ensure all previous writes are architecturally visible for the platform buffer flush.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
 drivers/md/dm-writecache.c   | 2 +-
 drivers/nvdimm/region_devs.c | 8 ++++----
 include/linux/libnvdimm.h    | 4 ++++
 3 files changed, 9 insertions(+), 5 deletions(-)

drivers/md/dm-writecache.c +1 -1

index 74f3c506f084..8c6b6dce64e2 100644
@@ -536,7 +536,7 @@ static void ssd_commit_superblock(struct dm_writecache *wc)
 static void writecache_commit_flushed(struct dm_writecache *wc, bool wait_for_ios)
 {
        if (WC_MODE_PMEM(wc))
-               wmb();
+               arch_pmem_flush_barrier();
        else
                ssd_commit_flushed(wc, wait_for_ios);
 }

drivers/nvdimm/region_devs.c +4 -4

index 4502f9c4708d..b308ad09b63d 100644
@@ -1206,13 +1206,13 @@ int generic_nvdimm_flush(struct nd_region *nd_region)
        idx = this_cpu_add_return(flush_idx, hash_32(current->pid + idx, 8));
 
        /*
-        * The first wmb() is needed to 'sfence' all previous writes
-        * such that they are architecturally visible for the platform
-        * buffer flush.  Note that we've already arranged for pmem
+        * The first arch_pmem_flush_barrier() is needed to 'sfence' all
+        * previous writes such that they are architecturally visible for
+        * the platform buffer flush. Note that we've already arranged for pmem
         * writes to avoid the cache via memcpy_flushcache().  The final
         * wmb() ensures ordering for the NVDIMM flush write.
         */
-       wmb();
+       arch_pmem_flush_barrier();
        for (i = 0; i < nd_region->ndr_mappings; i++)
                if (ndrd_get_flush_wpq(ndrd, i, 0))
                        writeq(1, ndrd_get_flush_wpq(ndrd, i, idx));

include/linux/libnvdimm.h +0 -0

index 18da4059be09..66f6c65bd789 100644

include/linux/libnvdimm.h static inline void arch_invalidate_pmem(void *addr, size_t size) · line 291

Quoted 6 lines

include/linux/libnvdimm.h +3 -0

@@ -286,4 +286,8 @@ static inline void arch_invalidate_pmem(void *addr, size_t size)
 }
 #endif
 
+#ifndef arch_pmem_flush_barrier
+#define arch_pmem_flush_barrier() wmb()
+#endif

I think it is out of place to define this in libnvdimm.h and it is odd to give it such a long name. The other pmem api helpers like arch_wb_cache_pmem() and arch_invalidate_pmem() are function calls for libnvdimm driver operations, this barrier is just an instruction and is closer to wmb() than the pmem api routine.

Quoted 5 lines

Since it is a store fence for pmem, so let's just call it pmem_wmb() and define the generic version in include/linux/compiler.h. It should probably also be documented alongside dma_wmb() in Documentation/memory-barriers.txt about why code would use it over wmb(), and why a symmetric pmem_rmb() is not needed.

How about the below? I used pmem_barrier() instead of pmem_wmb().

I
guess we wanted this to order() any data access not jus the following stores to persistent storage?

W.r.t why a symmetric pmem_rmb() is not
needed I was not sure how to explain that. Are you suggesting to explain why a read/load from persistent storage don't want to wait for pmem_barrier() ?

(quoted diff) +1 -0

+
 
 ===============================
 IMPLICIT KERNEL MEMORY BARRIERS

modified arch/powerpc/include/asm/barrier.h

(quoted diff) +13 -0

@@ -97,6 +97,19 @@ do {                                                                 \
 #define barrier_nospec()
 #endif /* CONFIG_PPC_BARRIER_NOSPEC */
 
+/*
+ * pmem_barrier() ensures that all stores for which the modification
+ * are written to persistent storage by preceding dcbfps/dcbstps
+ * instructions have updated persistent storage before any data
+ * access or data transfer caused by subsequent instructions is
+ * initiated.
+ */
+#define pmem_barrier pmem_barrier
+static inline void pmem_barrier(void)
+{
+       asm volatile(PPC_PHWSYNC ::: "memory");
+}
+
 #include <asm-generic/barrier.h>
 
 #endif /* _ASM_POWERPC_BARRIER_H */

modified include/asm-generic/barrier.h

(quoted diff) +3 -0

@@ -257,5 +257,16 @@ do {                                                                       \
 })
 #endif
 
+/*
+ * pmem_barrier() ensures that all stores for which the modification
+ * are written to persistent storage by preceding instructions have

Quoted 77 lines · Aneesh Kumar K.V

Quoted 74 lines · Dan Williams

Quoted 59 lines · Aneesh Kumar K.V

Architectures like ppc64 provide persistent memory specific barriers that will ensure that all stores for which the modifications are written to persistent storage by preceding dcbfps and dcbstps instructions have updated persistent storage before any data access or data transfer caused by subsequent instructions is initiated. This is in addition to the ordering done by wmb()

Update nvdimm core such that architecture can use barriers other than wmb to ensure all previous writes are architecturally visible for the platform buffer flush.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
  drivers/md/dm-writecache.c   | 2 +-
  drivers/nvdimm/region_devs.c | 8 ++++----
  include/linux/libnvdimm.h    | 4 ++++
  3 files changed, 9 insertions(+), 5 deletions(-)

drivers/md/dm-writecache.c +1 -1

index 74f3c506f084..8c6b6dce64e2 100644
@@ -536,7 +536,7 @@ static void ssd_commit_superblock(struct dm_writecache *wc)
  static void writecache_commit_flushed(struct dm_writecache *wc, bool wait_for_ios)
  {
         if (WC_MODE_PMEM(wc))
-               wmb();
+               arch_pmem_flush_barrier();
         else
                 ssd_commit_flushed(wc, wait_for_ios);
  }

drivers/nvdimm/region_devs.c +4 -4

index 4502f9c4708d..b308ad09b63d 100644
@@ -1206,13 +1206,13 @@ int generic_nvdimm_flush(struct nd_region *nd_region)
         idx = this_cpu_add_return(flush_idx, hash_32(current->pid + idx, 8));
 
         /*
-        * The first wmb() is needed to 'sfence' all previous writes
-        * such that they are architecturally visible for the platform
-        * buffer flush.  Note that we've already arranged for pmem
+        * The first arch_pmem_flush_barrier() is needed to 'sfence' all
+        * previous writes such that they are architecturally visible for
+        * the platform buffer flush. Note that we've already arranged for pmem
          * writes to avoid the cache via memcpy_flushcache().  The final
          * wmb() ensures ordering for the NVDIMM flush write.
          */
-       wmb();
+       arch_pmem_flush_barrier();
         for (i = 0; i < nd_region->ndr_mappings; i++)
                 if (ndrd_get_flush_wpq(ndrd, i, 0))
                         writeq(1, ndrd_get_flush_wpq(ndrd, i, idx));

include/linux/libnvdimm.h +0 -0

index 18da4059be09..66f6c65bd789 100644

include/linux/libnvdimm.h static inline void arch_invalidate_pmem(void *addr, size_t size) · line 291

Quoted 6 lines

include/linux/libnvdimm.h +3 -0

@@ -286,4 +286,8 @@ static inline void arch_invalidate_pmem(void *addr, size_t size)
  }
  #endif
 
+#ifndef arch_pmem_flush_barrier
+#define arch_pmem_flush_barrier() wmb()
+#endif

I think it is out of place to define this in libnvdimm.h and it is odd to give it such a long name. The other pmem api helpers like arch_wb_cache_pmem() and arch_invalidate_pmem() are function calls for libnvdimm driver operations, this barrier is just an instruction and is closer to wmb() than the pmem api routine.

Quoted 7 lines

Quoted 5 lines

Since it is a store fence for pmem, so let's just call it pmem_wmb() and define the generic version in include/linux/compiler.h. It should probably also be documented alongside dma_wmb() in Documentation/memory-barriers.txt about why code would use it over wmb(), and why a symmetric pmem_rmb() is not needed.

How about the below? I used pmem_barrier() instead of pmem_wmb().

Why? A barrier() is a bi-directional ordering mechanic for reads and writes, and the proposed semantics mechanism only orders writes + persistence. Otherwise the default fallback to wmb() on archs that don't override it does not make sense.

Quoted 3 lines

I
guess we wanted this to order() any data access not jus the following stores to persistent storage?

Why?

Quoted 4 lines

W.r.t why a symmetric pmem_rmb() is not
needed I was not sure how to explain that. Are you suggesting to explain why a read/load from persistent storage don't want to wait for pmem_barrier() ?

I would expect that the explanation is that a typical rmb() is sufficient and that there is nothing pmem specific semantic for read ordering for pmem vs normal read-barrier semantics.

Review There are some more advanced barrier functions: · line 1941

Quoted 10 lines

modified Documentation/memory-barriers.txt

(quoted diff) +4 -0

@@ -1935,6 +1935,16 @@ There are some more advanced barrier functions:
       relaxed I/O accessors and the Documentation/DMA-API.txt file for more
       information on consistent memory.
 
+ (*) pmem_barrier();
+
+     These are for use with persistent memory to esure the ordering of stores
+     to persistent memory region.

If it was just ordering I would expect a typical wmb() to be sufficient, why is the pmem-specific instruction needed? I thought it was handshaking with hardware to ensure acceptance into a persistence domain *in addition* to ordering the stores.

Review There are some more advanced barrier functions: · line 1944

Quoted 3 lines

(quoted diff) +3 -0

+     For example, after a non temporal write to persistent storage we use pmem_barrier()
+     to ensures that stores have updated the persistent storage before
+     any data access or data transfer caused by subsequent instructions is initiated.

Isn't the ordering aspect is irrelevant relative to traditional wmb()? For example if you used the wrong sync instruction the store ordering will still be correct it would just not persist at the same time as barrier completes. Or am I misunderstanding how these new instructions are distinct?

Quoted 110 lines · Dan Williams

Quoted 84 lines · Aneesh Kumar K.V

Quoted 81 lines · Dan Williams

Quoted 65 lines · Aneesh Kumar K.V

Architectures like ppc64 provide persistent memory specific barriers that will ensure that all stores for which the modifications are written to persistent storage by preceding dcbfps and dcbstps instructions have updated persistent storage before any data access or data transfer caused by subsequent instructions is initiated. This is in addition to the ordering done by wmb()

Update nvdimm core such that architecture can use barriers other than wmb to ensure all previous writes are architecturally visible for the platform buffer flush.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
  drivers/md/dm-writecache.c   | 2 +-
  drivers/nvdimm/region_devs.c | 8 ++++----
  include/linux/libnvdimm.h    | 4 ++++

  3 files changed, 9 insertions(+), 5 deletions(-)

drivers/md/dm-writecache.c +1 -1

index 74f3c506f084..8c6b6dce64e2 100644
@@ -536,7 +536,7 @@ static void ssd_commit_superblock(struct 
 dm_writecache *wc)
   static void writecache_commit_flushed(struct dm_writecache *wc, bool wait_for_ios)
   {
          if (WC_MODE_PMEM(wc))
-               wmb();
+               arch_pmem_flush_barrier();

         else
                 ssd_commit_flushed(wc, wait_for_ios); }

drivers/nvdimm/region_devs.c +0 -0

index 4502f9c4708d..b308ad09b63d 100644
@@ -1206,13 +1206,13 @@ int generic_nvdimm_flush(struct nd_region 
 *nd_region)

         idx = this_cpu_add_return(flush_idx, hash_32(current->pid
+ idx, 8));

         /*
-        * The first wmb() is needed to 'sfence' all previous writes -        * such that they are architecturally visible for the platform -        * buffer flush.  Note that we've already arranged for pmem +        * The first arch_pmem_flush_barrier() is needed to 'sfence' all
+        * previous writes such that they are architecturally visible for
+        * the platform buffer flush. Note that we've already arranged for pmem
          * writes to avoid the cache via memcpy_flushcache().  The final
          * wmb() ensures ordering for the NVDIMM flush write. */
-       wmb();
+       arch_pmem_flush_barrier();
         for (i = 0; i < nd_region->ndr_mappings; i++)
                 if (ndrd_get_flush_wpq(ndrd, i, 0)) writeq(1, ndrd_get_flush_wpq(ndrd, i, idx));

include/linux/libnvdimm.h +0 -0

index 18da4059be09..66f6c65bd789 100644

include/linux/libnvdimm.h static inline void arch_invalidate_pmem(void · line 292

Quoted 7 lines

include/linux/libnvdimm.h +3 -0

@@ -286,4 +286,8 @@ static inline void arch_invalidate_pmem(void 
 *addr, size_t size)
   }
   #endif
 
+#ifndef arch_pmem_flush_barrier
+#define arch_pmem_flush_barrier() wmb()
+#endif

I think it is out of place to define this in libnvdimm.h and it is odd to give it such a long name. The other pmem api helpers like arch_wb_cache_pmem() and arch_invalidate_pmem() are function calls for libnvdimm driver operations, this barrier is just an instruction and is closer to wmb() than the pmem api routine.

Quoted 7 lines

Quoted 5 lines

Since it is a store fence for pmem, so let's just call it pmem_wmb() and define the generic version in include/linux/compiler.h. It should probably also be documented alongside dma_wmb() in Documentation/memory-barriers.txt about why code would use it over wmb(), and why a symmetric pmem_rmb() is not needed.

How about the below? I used pmem_barrier() instead of pmem_wmb().

Why? A barrier() is a bi-directional ordering mechanic for reads and writes, and the proposed semantics mechanism only orders writes + persistence. Otherwise the default fallback to wmb() on archs that don't override it does not make sense.

Quoted 3 lines

I
guess we wanted this to order() any data access not jus the following stores to persistent storage?

Why?

Quoted 4 lines

W.r.t why a symmetric pmem_rmb() is not
needed I was not sure how to explain that. Are you suggesting to explain why a read/load from persistent storage don't want to wait for pmem_barrier() ?

Quoted 4 lines

I would expect that the explanation is that a typical rmb() is sufficient and that there is nothing pmem specific semantic for read ordering for pmem vs normal read-barrier semantics.

Update patch.

From 1e6aa6c4182e14ec5d6bf878ae44c3f69ebff745 Mon Sep 17 00:00:00 2001
From: "Aneesh Kumar K.V" <[email protected]>
Date: Tue, 12 May 2020 20:58:33 +0530
Subject: [PATCH] libnvdimm/nvdimm/flush: Allow architecture to override the
flush barrier

Architectures like ppc64 provide persistent memory specific barriers that will ensure that all stores for which the modifications are written to persistent storage by preceding dcbfps and dcbstps instructions have updated persistent storage before any data access or data transfer caused by subsequent instructions is initiated. This is in addition to the ordering done by wmb()

Update nvdimm core such that architecture can use barriers other than wmb to ensure all previous writes are architecturally visible for the platform buffer flush.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
 Documentation/memory-barriers.txt | 14 ++++++++++++++
 drivers/md/dm-writecache.c        |  2 +-
 drivers/nvdimm/region_devs.c      |  8 ++++----
 include/asm-generic/barrier.h     | 10 ++++++++++
 4 files changed, 29 insertions(+), 5 deletions(-)

Documentation/memory-barriers.txt +0 -0

index eaabc3134294..340273a6b18e 100644

Documentation/memory-barriers.txt +6 -0

+     that stores have updated persistent storage before any data access or
+     data transfer caused by subsequent instructions is initiated. This is
+     in addition to the ordering done by wmb().
+
+     For load from persistent memory, existing read memory barriers are sufficient
+     to ensure read ordering.
 
 ===============================
 IMPLICIT KERNEL MEMORY BARRIERS

drivers/md/dm-writecache.c +1 -1

index 74f3c506f084..00534fa4a384 100644
@@ -536,7 +536,7 @@ static void ssd_commit_superblock(struct dm_writecache *wc)
 static void writecache_commit_flushed(struct dm_writecache *wc, bool wait_for_ios)
 {
        if (WC_MODE_PMEM(wc))
-               wmb();
+               pmem_wmb();
        else
                ssd_commit_flushed(wc, wait_for_ios);
 }

drivers/nvdimm/region_devs.c +0 -0

index 4502f9c4708d..2333b290bdcf 100644

drivers/nvdimm/region_devs.c +3 -1

+        * previous writes such that they are architecturally visible for
+        * the platform buffer flush. Note that we've already arranged for pmem
         * writes to avoid the cache via memcpy_flushcache().  The final
         * wmb() ensures ordering for the NVDIMM flush write.
         */
-       wmb();
+       pmem_wmb();
        for (i = 0; i < nd_region->ndr_mappings; i++)
                if (ndrd_get_flush_wpq(ndrd, i, 0))
                        writeq(1, ndrd_get_flush_wpq(ndrd, i, idx));

include/asm-generic/barrier.h +0 -0

index 2eacaf7d62f6..879d68faec1d 100644

include/asm-generic/barrier.h +8 -1

+ * are written to persistent storage by preceding instructions have
+ * updated persistent storage before any data  access or data transfer
+ * caused by subsequent instructions is initiated.
+ */
+#ifndef pmem_wmb
+#define pmem_wmb()     wmb()
+#endif
+
 #endif /* !__ASSEMBLY__ */
 #endif /* __ASM_GENERIC_BARRIER_H */
--

2.26.2

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 <[email protected]>

---
 arch/powerpc/include/asm/cacheflush.h | 7 +++++++
 1 file changed, 7 insertions(+)

arch/powerpc/include/asm/cacheflush.h +0 -0

index 54764c6e922d..95782f77d768 100644

Quoted 18 lines · 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 <[email protected]>

---
 arch/powerpc/include/asm/cacheflush.h | 7 +++++++
 1 file changed, 7 insertions(+)

arch/powerpc/include/asm/cacheflush.h +0 -0

index 54764c6e922d..95782f77d768 100644
@@ -98,6 +98,13 @@ static inline void invalidate_dcache_range(unsigned long start,
        mb();   /* sync */
 }
 

arch/powerpc/include/asm/cacheflush.h static inline void invalidate_dcache_range(unsigned long start, · line 105

Quoted 5 lines

arch/powerpc/include/asm/cacheflush.h +5 -0

+#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?

Quoted 20 lines · Dan Williams

Quoted 18 lines · 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 <[email protected]>

---
 arch/powerpc/include/asm/cacheflush.h | 7 +++++++
 1 file changed, 7 insertions(+)

arch/powerpc/include/asm/cacheflush.h +0 -0

index 54764c6e922d..95782f77d768 100644
@@ -98,6 +98,13 @@ static inline void invalidate_dcache_range(unsigned long start,
        mb();   /* sync */
 }
 

Quoted 7 lines

arch/powerpc/include/asm/cacheflush.h static inline void invalidate_dcache_range(unsigned long start, · line 105

Quoted 5 lines

arch/powerpc/include/asm/cacheflush.h +5 -0

+#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.

nvdimm expect the flush routines to just mark the cache clean. The barrier that mark the store globally visible is done in nvdimm_flush().

Update the papr_scm driver to a simplified nvdim_flush callback that do only the required barrier.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
 arch/powerpc/lib/pmem.c                   |  6 ------
 arch/powerpc/platforms/pseries/papr_scm.c | 13 +++++++++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

arch/powerpc/lib/pmem.c +0 -6

index 5a61aaeb6930..21210fa676e5 100644
@@ -19,9 +19,6 @@ static inline void __clean_pmem_range(unsigned long start, unsigned long stop)
 
 	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)

@@ -34,9 +31,6 @@ static inline void __flush_pmem_range(unsigned long start, unsigned long stop)
 
 	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)

arch/powerpc/platforms/pseries/papr_scm.c +12 -0

index 9c569078a09f..9a9a0766f8b6 100644
@@ -630,6 +630,18 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
 
 	return 0;
 }
+/*
+ * We have made sure the pmem writes are done such that before calling this
+ * all the caches are flushed/clean. We use dcbf/dcbfps to ensure this. Here
+ * we just need to add the necessary barrier to make sure the above flushes
+ * are have updated persistent storage before any data access or data transfer
+ * caused by subsequent instructions is initiated.
+ */
+static int papr_scm_flush_sync(struct nd_region *nd_region, struct bio *bio)
+{
+	arch_pmem_flush_barrier();
+	return 0;
+}
 
 static ssize_t flags_show(struct device *dev,
 			  struct device_attribute *attr, char *buf)

Hello,

Quoted 57 lines · Aneesh Kumar K.V

nvdimm expect the flush routines to just mark the cache clean. The barrier that mark the store globally visible is done in nvdimm_flush().

Update the papr_scm driver to a simplified nvdim_flush callback that do only the required barrier.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
 arch/powerpc/lib/pmem.c                   |  6 ------
 arch/powerpc/platforms/pseries/papr_scm.c | 13 +++++++++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

arch/powerpc/lib/pmem.c +0 -6

index 5a61aaeb6930..21210fa676e5 100644
@@ -19,9 +19,6 @@ static inline void __clean_pmem_range(unsigned long start, unsigned long stop)
 
 	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)

@@ -34,9 +31,6 @@ static inline void __flush_pmem_range(unsigned long start, unsigned long stop)
 
 	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)

arch/powerpc/platforms/pseries/papr_scm.c +12 -0

index 9c569078a09f..9a9a0766f8b6 100644
@@ -630,6 +630,18 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
 
 	return 0;
 }
+/*
+ * We have made sure the pmem writes are done such that before calling this
+ * all the caches are flushed/clean. We use dcbf/dcbfps to ensure this. Here
+ * we just need to add the necessary barrier to make sure the above flushes
+ * are have updated persistent storage before any data access or data transfer
+ * caused by subsequent instructions is initiated.
+ */
+static int papr_scm_flush_sync(struct nd_region *nd_region, struct bio *bio)
+{
+	arch_pmem_flush_barrier();
+	return 0;
+}
 

arch/powerpc/platforms/pseries/papr_scm.c static int papr_scm_nvdimm_init(struct papr_scm_priv *p) · line 758

Quoted 7 lines

arch/powerpc/platforms/pseries/papr_scm.c +1 -0

 static ssize_t flags_show(struct device *dev,
 			  struct device_attribute *attr, char *buf)

@@ -743,6 +755,7 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
 	ndr_desc.mapping = &mapping;
 	ndr_desc.num_mappings = 1;
 	ndr_desc.nd_set = &p->nd_set;
+	ndr_desc.flush = papr_scm_flush_sync;

AFAICT currently the only device that implements flush is virtio_pmem. How does the nfit driver get away without implementing flush?

Also the flush takes arguments that are completely unused but a user of the pmem region must assume they are used, and call flush() on the region rather than arch_pmem_flush_barrier() directly.

This may not
work well with md as discussed with earlier iteration of the patchest.

Quoted 60 lines · Michal Suchánek

Hello,

Quoted 57 lines · Aneesh Kumar K.V

nvdimm expect the flush routines to just mark the cache clean. The barrier that mark the store globally visible is done in nvdimm_flush().

Update the papr_scm driver to a simplified nvdim_flush callback that do only the required barrier.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
 arch/powerpc/lib/pmem.c                   |  6 ------
 arch/powerpc/platforms/pseries/papr_scm.c | 13 +++++++++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

arch/powerpc/lib/pmem.c +0 -6

index 5a61aaeb6930..21210fa676e5 100644
@@ -19,9 +19,6 @@ static inline void __clean_pmem_range(unsigned long start, unsigned long stop)
 
     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)

@@ -34,9 +31,6 @@ static inline void __flush_pmem_range(unsigned long start, unsigned long stop)
 
     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)

arch/powerpc/platforms/pseries/papr_scm.c +12 -0

index 9c569078a09f..9a9a0766f8b6 100644
@@ -630,6 +630,18 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
 
     return 0;
 }
+/*
+ * We have made sure the pmem writes are done such that before calling this
+ * all the caches are flushed/clean. We use dcbf/dcbfps to ensure this. Here
+ * we just need to add the necessary barrier to make sure the above flushes
+ * are have updated persistent storage before any data access or data transfer
+ * caused by subsequent instructions is initiated.
+ */
+static int papr_scm_flush_sync(struct nd_region *nd_region, struct bio *bio)
+{
+    arch_pmem_flush_barrier();
+    return 0;
+}
 

Quoted 10 lines

arch/powerpc/platforms/pseries/papr_scm.c static int papr_scm_nvdimm_init(struct papr_scm_priv *p) · line 758

Quoted 7 lines

arch/powerpc/platforms/pseries/papr_scm.c +1 -0

 static ssize_t flags_show(struct device *dev,
                       struct device_attribute *attr, char *buf)

@@ -743,6 +755,7 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
     ndr_desc.mapping = &mapping;
     ndr_desc.num_mappings = 1;
     ndr_desc.nd_set = &p->nd_set;
+    ndr_desc.flush = papr_scm_flush_sync;

AFAICT currently the only device that implements flush is virtio_pmem. How does the nfit driver get away without implementing flush?

generic_nvdimm_flush does the required barrier for nfit. The reason for adding ndr_desc.flush call back for papr_scm was to avoid the usage of iomem based deep flushing (ndr_region_data.flush_wpq) which is not supported by papr_scm.

BTW we do return NULL for ndrd_get_flush_wpq() on power. So the upstream code also does the same thing, but in a different way.

Quoted 3 lines

Also the flush takes arguments that are completely unused but a user of the pmem region must assume they are used, and call flush() on the region rather than arch_pmem_flush_barrier() directly.

The bio argument can help a pmem driver to do range based flushing in case of pmem_make_request. If bio is null then we must assume a full device flush.

Quoted 87 lines · Aneesh Kumar K.V

Quoted 60 lines · Michal Suchánek

Hello,

Quoted 57 lines · Aneesh Kumar K.V

nvdimm expect the flush routines to just mark the cache clean. The barrier that mark the store globally visible is done in nvdimm_flush().

Update the papr_scm driver to a simplified nvdim_flush callback that do only the required barrier.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
 arch/powerpc/lib/pmem.c                   |  6 ------
 arch/powerpc/platforms/pseries/papr_scm.c | 13 +++++++++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

arch/powerpc/lib/pmem.c +0 -6

index 5a61aaeb6930..21210fa676e5 100644
@@ -19,9 +19,6 @@ static inline void __clean_pmem_range(unsigned long start, unsigned long stop)
 
     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)

@@ -34,9 +31,6 @@ static inline void __flush_pmem_range(unsigned long start, unsigned long stop)
 
     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)

arch/powerpc/platforms/pseries/papr_scm.c +12 -0

index 9c569078a09f..9a9a0766f8b6 100644
@@ -630,6 +630,18 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
 
     return 0;
 }
+/*
+ * We have made sure the pmem writes are done such that before calling this
+ * all the caches are flushed/clean. We use dcbf/dcbfps to ensure this. Here
+ * we just need to add the necessary barrier to make sure the above flushes
+ * are have updated persistent storage before any data access or data transfer
+ * caused by subsequent instructions is initiated.
+ */
+static int papr_scm_flush_sync(struct nd_region *nd_region, struct bio *bio)
+{
+    arch_pmem_flush_barrier();
+    return 0;
+}
 

Quoted 10 lines

arch/powerpc/platforms/pseries/papr_scm.c static int papr_scm_nvdimm_init(struct papr_scm_priv *p) · line 758

Quoted 7 lines

arch/powerpc/platforms/pseries/papr_scm.c +1 -0

 static ssize_t flags_show(struct device *dev,
                       struct device_attribute *attr, char *buf)

@@ -743,6 +755,7 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
     ndr_desc.mapping = &mapping;
     ndr_desc.num_mappings = 1;
     ndr_desc.nd_set = &p->nd_set;
+    ndr_desc.flush = papr_scm_flush_sync;

AFAICT currently the only device that implements flush is virtio_pmem. How does the nfit driver get away without implementing flush?

generic_nvdimm_flush does the required barrier for nfit. The reason for adding ndr_desc.flush call back for papr_scm was to avoid the usage of iomem based deep flushing (ndr_region_data.flush_wpq) which is not supported by papr_scm.

BTW we do return NULL for ndrd_get_flush_wpq() on power. So the upstream code also does the same thing, but in a different way.

Quoted 3 lines

Also the flush takes arguments that are completely unused but a user of the pmem region must assume they are used, and call flush() on the region rather than arch_pmem_flush_barrier() directly.

Quoted 3 lines

The bio argument can help a pmem driver to do range based flushing in case of pmem_make_request. If bio is null then we must assume a full device flush.

The bio argument isn't for range based flushing, it is for flush operations that need to complete asynchronously.

Quoted 89 lines · Dan Williams

Quoted 87 lines · Aneesh Kumar K.V

Quoted 60 lines · Michal Suchánek

Hello,

Quoted 57 lines · Aneesh Kumar K.V

nvdimm expect the flush routines to just mark the cache clean. The barrier that mark the store globally visible is done in nvdimm_flush().

Update the papr_scm driver to a simplified nvdim_flush callback that do only the required barrier.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
  arch/powerpc/lib/pmem.c                   |  6 ------
  arch/powerpc/platforms/pseries/papr_scm.c | 13 +++++++++++++
  2 files changed, 13 insertions(+), 6 deletions(-)

arch/powerpc/lib/pmem.c +0 -6

index 5a61aaeb6930..21210fa676e5 100644
@@ -19,9 +19,6 @@ static inline void __clean_pmem_range(unsigned long start, unsigned long stop)
 
      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)

@@ -34,9 +31,6 @@ static inline void __flush_pmem_range(unsigned long start, unsigned long stop)
 
      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)

arch/powerpc/platforms/pseries/papr_scm.c +12 -0

index 9c569078a09f..9a9a0766f8b6 100644
@@ -630,6 +630,18 @@ static int papr_scm_ndctl(struct nvdimm_bus_descriptor *nd_desc,
 
      return 0;
  }
+/*
+ * We have made sure the pmem writes are done such that before calling this
+ * all the caches are flushed/clean. We use dcbf/dcbfps to ensure this. Here
+ * we just need to add the necessary barrier to make sure the above flushes
+ * are have updated persistent storage before any data access or data transfer
+ * caused by subsequent instructions is initiated.
+ */
+static int papr_scm_flush_sync(struct nd_region *nd_region, struct bio *bio)
+{
+    arch_pmem_flush_barrier();
+    return 0;
+}
 

Quoted 10 lines

arch/powerpc/platforms/pseries/papr_scm.c static int papr_scm_nvdimm_init(struct papr_scm_priv *p) · line 758

Quoted 7 lines

arch/powerpc/platforms/pseries/papr_scm.c +1 -0

  static ssize_t flags_show(struct device *dev,
                        struct device_attribute *attr, char *buf)

@@ -743,6 +755,7 @@ static int papr_scm_nvdimm_init(struct papr_scm_priv *p)
      ndr_desc.mapping = &mapping;
      ndr_desc.num_mappings = 1;
      ndr_desc.nd_set = &p->nd_set;
+    ndr_desc.flush = papr_scm_flush_sync;

AFAICT currently the only device that implements flush is virtio_pmem. How does the nfit driver get away without implementing flush?

generic_nvdimm_flush does the required barrier for nfit. The reason for adding ndr_desc.flush call back for papr_scm was to avoid the usage of iomem based deep flushing (ndr_region_data.flush_wpq) which is not supported by papr_scm.

BTW we do return NULL for ndrd_get_flush_wpq() on power. So the upstream code also does the same thing, but in a different way.

Quoted 3 lines

Also the flush takes arguments that are completely unused but a user of the pmem region must assume they are used, and call flush() on the region rather than arch_pmem_flush_barrier() directly.

Quoted 6 lines

Quoted 3 lines

The bio argument can help a pmem driver to do range based flushing in case of pmem_make_request. If bio is null then we must assume a full device flush.

The bio argument isn't for range based flushing, it is for flush operations that need to complete asynchronously.

How does the block layer determine that the pmem device needs asynchronous fushing?

The flush() was designed for the purpose with the bio argument and only virtio_pmem which is fulshed asynchronously used it. Now that papr_scm resuses it fir different purpose how do you tell?

Quoted 5 lines

Quoted 2 lines

The bio argument isn't for range based flushing, it is for flush operations that need to complete asynchronously.

How does the block layer determine that the pmem device needs asynchronous fushing?

        set_bit(ND_REGION_ASYNC, &ndr_desc.flags);

and dax_synchronous(dev)

On Tue, Jun 30, 2020 at 2:21 AM Aneesh Kumar K.V
<[email protected]> wrote:
[..]

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

The bio argument isn't for range based flushing, it is for flush operations that need to complete asynchronously.

How does the block layer determine that the pmem device needs asynchronous fushing?

         set_bit(ND_REGION_ASYNC, &ndr_desc.flags);

and dax_synchronous(dev)

Yes, but I think it is overkill to have an indirect function call just for a single instruction.

How about something like this instead, to share a common pmem_wmb() across x86 and powerpc.

drivers/nvdimm/region_devs.c +7 -0

index 20ff30c2ab93..b14009060c83 100644
@@ -1180,6 +1180,13 @@ int nvdimm_flush(struct nd_region *nd_region,
 struct bio *bio)
  {
         int rc = 0;
 
+       /*
+        * pmem_wmb() is needed to 'sfence' all previous writes such
+        * that they are architecturally visible for the platform buffer
+        * flush.
+        */
+       pmem_wmb();
+
         if (!nd_region->flush)
                 rc = generic_nvdimm_flush(nd_region);

         else {

drivers/nvdimm/region_devs.c +0 -0

@@ -1206,17 +1213,14 @@ int generic_nvdimm_flush(struct nd_region *nd_region)
         idx = this_cpu_add_return(flush_idx, hash_32(current->pid + idx, 8));
 
         /*

Quoted 42 lines · Dan Williams

On Tue, Jun 30, 2020 at 2:21 AM Aneesh Kumar K.V
<[email protected]> wrote:
[..]

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

The bio argument isn't for range based flushing, it is for flush operations that need to complete asynchronously.

How does the block layer determine that the pmem device needs asynchronous fushing?

         set_bit(ND_REGION_ASYNC, &ndr_desc.flags);

and dax_synchronous(dev)

Yes, but I think it is overkill to have an indirect function call just for a single instruction.

How about something like this instead, to share a common pmem_wmb() across x86 and powerpc.

drivers/nvdimm/region_devs.c +7 -0

index 20ff30c2ab93..b14009060c83 100644
@@ -1180,6 +1180,13 @@ int nvdimm_flush(struct nd_region *nd_region,
 struct bio *bio)
  {
         int rc = 0;
 
+       /*
+        * pmem_wmb() is needed to 'sfence' all previous writes such
+        * that they are architecturally visible for the platform buffer
+        * flush.
+        */
+       pmem_wmb();
+
         if (!nd_region->flush)
                 rc = generic_nvdimm_flush(nd_region);

         else {

drivers/nvdimm/region_devs.c +0 -0

@@ -1206,17 +1213,14 @@ int generic_nvdimm_flush(struct nd_region *nd_region)
         idx = this_cpu_add_return(flush_idx, hash_32(current->pid + idx, 8));
 
         /*

drivers/nvdimm/region_devs.c int generic_nvdimm_flush(struct nd_region *nd_region) · line 1215

Quoted 10 lines

drivers/nvdimm/region_devs.c +3 -6

-        * The first wmb() is needed to 'sfence' all previous writes
-        * such that they are architecturally visible for the platform
-        * buffer flush.  Note that we've already arranged for pmem
-        * writes to avoid the cache via memcpy_flushcache().  The final
-        * wmb() ensures ordering for the NVDIMM flush write.
+        * Note that we've already arranged for pmem writes to avoid the
+        * cache via memcpy_flushcache().  The final wmb() ensures
+        * ordering for the NVDIMM flush write.
          */
-       wmb();

The series already convert this to pmem_wmb().

drivers/nvdimm/region_devs.c int generic_nvdimm_flush(struct nd_region *nd_region) · line 1223

Quoted 5 lines

drivers/nvdimm/region_devs.c +1 -1

         for (i = 0; i < nd_region->ndr_mappings; i++)
                 if (ndrd_get_flush_wpq(ndrd, i, 0))
                         writeq(1, ndrd_get_flush_wpq(ndrd, i, idx));
-       wmb();
+       pmem_wmb();

Should this be pmem_wmb()? This is ordering the above writeq() right?

drivers/nvdimm/region_devs.c int generic_nvdimm_flush(struct nd_region *nd_region) · line 1226

Quoted 5 lines

drivers/nvdimm/region_devs.c +0 -0

         return 0;
  }
 

This still results in two pmem_wmb() on platforms that doesn't have flush_wpq. I was trying to avoid that by adding a nd_region->flush call back.

We only support persistent memory on P8 and above. This is enforced by the firmware and further checked on virtualzied platform during platform init. Add WARN_ONCE in pmem flush routines to catch the wrong usage of these.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
 arch/powerpc/include/asm/cacheflush.h | 2 ++
 arch/powerpc/lib/pmem.c               | 2 ++
 2 files changed, 4 insertions(+)

arch/powerpc/include/asm/cacheflush.h +0 -0

index 95782f77d768..1ab0fa660497 100644

Quoted 11 lines · Aneesh Kumar K.V

We only support persistent memory on P8 and above. This is enforced by the firmware and further checked on virtualzied platform during platform init. Add WARN_ONCE in pmem flush routines to catch the wrong usage of these.

Signed-off-by Aneesh Kumar K.V <[email protected]>

---
 arch/powerpc/include/asm/cacheflush.h | 2 ++
 arch/powerpc/lib/pmem.c               | 2 ++
 2 files changed, 4 insertions(+)

arch/powerpc/include/asm/cacheflush.h static inline void arch_pmem_flush_barrier(void) · line 107

Quoted 10 lines

arch/powerpc/include/asm/cacheflush.h +2 -0

index 95782f77d768..1ab0fa660497 100644
@@ -103,6 +103,8 @@ static inline void  arch_pmem_flush_barrier(void)
 {
        if (cpu_has_feature(CPU_FTR_ARCH_207S))
                asm volatile(PPC_PHWSYNC ::: "memory");
+       else
+               WARN_ONCE(1, "Using pmem flush on older hardware.");

This seems too late to be making this determination. I'd expect the driver to fail to successfully bind default if this constraint is not met.