[PATCHv2 1/2] powerpc/of: split out new_property() for reusing

Splitting out new_property() for coming reusing and moving it to
of_helpers.c.

Also do some coding style cleanup.

Signed-off-by: Pingfan Liu <redacted>
To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Aneesh Kumar K.V <redacted>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Dan Williams <redacted>
Cc: Andrew Donnellan <redacted>
Cc: Christophe Leroy <redacted>
Cc: kexec@lists.infradead.org
---
 arch/powerpc/platforms/pseries/of_helpers.c | 28 ++++++++++++++++++++++++++++
 arch/powerpc/platforms/pseries/of_helpers.h |  3 +++
 arch/powerpc/platforms/pseries/reconfig.c   | 26 --------------------------
 3 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
index 66dfd82..1022e0f 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c

@@ -7,6 +7,34 @@
 
 #include "of_helpers.h"
 
+struct property *new_property(const char *name, const int length,
+		const unsigned char *value, struct property *last)
+{
+	struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
+
+	if (!new)
+		return NULL;
+
+	new->name = kstrdup(name, GFP_KERNEL);
+	if (!new->name)
+		goto cleanup;
+	new->value = kmalloc(length + 1, GFP_KERNEL);
+	if (!new->value)
+		goto cleanup;
+
+	memcpy(new->value, value, length);
+	*(((char *)new->value) + length) = 0;
+	new->length = length;
+	new->next = last;
+	return new;
+
+cleanup:
+	kfree(new->name);
+	kfree(new->value);
+	kfree(new);
+	return NULL;
+}
+
 /**
  * pseries_of_derive_parent - basically like dirname(1)
  * @path:  the full_name of a node to be added to the tree

diff --git a/arch/powerpc/platforms/pseries/of_helpers.h b/arch/powerpc/platforms/pseries/of_helpers.h
index decad65..34add82 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.h
+++ b/arch/powerpc/platforms/pseries/of_helpers.h

@@ -4,6 +4,9 @@
 
 #include <linux/of.h>
 
+struct property *new_property(const char *name, const int length,
+		const unsigned char *value, struct property *last);
+
 struct device_node *pseries_of_derive_parent(const char *path);
 
 #endif /* _PSERIES_OF_HELPERS_H */

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 7f7369f..8e5a2ba 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c

@@ -165,32 +165,6 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
 	return tmp;
 }
 
-static struct property *new_property(const char *name, const int length,
-				     const unsigned char *value, struct property *last)
-{
-	struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
-
-	if (!new)
-		return NULL;
-
-	if (!(new->name = kstrdup(name, GFP_KERNEL)))
-		goto cleanup;
-	if (!(new->value = kmalloc(length + 1, GFP_KERNEL)))
-		goto cleanup;
-
-	memcpy(new->value, value, length);
-	*(((char *)new->value) + length) = 0;
-	new->length = length;
-	new->next = last;
-	return new;
-
-cleanup:
-	kfree(new->name);
-	kfree(new->value);
-	kfree(new);
-	return NULL;
-}
-
 static int do_add_node(char *buf, size_t bufsize)
 {
 	char *path, *end, *name;

-- 
2.7.5

At present, plpar_hcall(H_SCM_BIND_MEM, ...) takes a very long time, so
if dumping to fsdax, it will take a very long time.

Take a closer look, during the papr_scm initialization, the only
configuration is through drc_pmem_bind()-> plpar_hcall(H_SCM_BIND_MEM,
...), which helps to set up the bound address.

On pseries, for kexec -l/-p kernel, there is no reset of hardware, and this
step can be stepped around to save times.  So the pmem bound address can be
passed to the 2nd kernel through a dynamic added property "bound-addr" in
dt node 'ibm,pmemory'.

Signed-off-by: Pingfan Liu <redacted>
To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Aneesh Kumar K.V <redacted>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Dan Williams <redacted>
Cc: Andrew Donnellan <redacted>
Cc: Christophe Leroy <redacted>
Cc: kexec@lists.infradead.org
---
note: This patch has not been tested since I can not get such a pseries with pmem.
      Please kindly to give some suggestion, thanks.

 arch/powerpc/platforms/pseries/papr_scm.c | 32 +++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 0b4467e..40cd214 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c

@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 
 #include <asm/plpar_wrappers.h>
+#include "of_helpers.h"
 
 #define BIND_ANY_ADDR (~0ul)
 

@@ -383,7 +384,7 @@ static int papr_scm_probe(struct platform_device *pdev)
 {
 	struct device_node *dn = pdev->dev.of_node;
 	u32 drc_index, metadata_size;
-	u64 blocks, block_size;
+	u64 blocks, block_size, bound_addr = 0;
 	struct papr_scm_priv *p;
 	const char *uuid_str;
 	u64 uuid[2];

@@ -440,17 +441,28 @@ static int papr_scm_probe(struct platform_device *pdev)
 	p->metadata_size = metadata_size;
 	p->pdev = pdev;
 
-	/* request the hypervisor to bind this region to somewhere in memory */
-	rc = drc_pmem_bind(p);
+	of_property_read_u64(dn, "bound-addr", &bound_addr);
+	if (bound_addr) {
+		p->bound_addr = bound_addr;
+	} else {
+		struct property *property;
+		u64 big;
 
-	/* If phyp says drc memory still bound then force unbound and retry */
-	if (rc == H_OVERLAP)
-		rc = drc_pmem_query_n_bind(p);
+		/* request the hypervisor to bind this region to somewhere in memory */
+		rc = drc_pmem_bind(p);
 
-	if (rc != H_SUCCESS) {
-		dev_err(&p->pdev->dev, "bind err: %d\n", rc);
-		rc = -ENXIO;
-		goto err;
+		/* If phyp says drc memory still bound then force unbound and retry */
+		if (rc == H_OVERLAP)
+			rc = drc_pmem_query_n_bind(p);
+
+		if (rc != H_SUCCESS) {
+			dev_err(&p->pdev->dev, "bind err: %d\n", rc);
+			rc = -ENXIO;
+			goto err;
+		}
+		big = cpu_to_be64(p->bound_addr);
+		property = new_property("bound-addr", sizeof(u64), &big, NULL);
+		of_add_property(dn, property);
 	}
 
 	/* setup the resource for the newly bound range */

-- 
2.7.5

V2 -> V3:
   in [2/2], EXPORT_SYMBOL(new_property) and EXPORT_SYMBOL_GPL(of_add_property)

To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Aneesh Kumar K.V <redacted>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Dan Williams <redacted>
Cc: Andrew Donnellan <redacted>
Cc: Christophe Leroy <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Cc: kexec@lists.infradead.org

Pingfan Liu (2):
  powerpc/of: split out new_property() for reusing
  pseries/scm: buffer pmem's bound addr in dt for kexec kernel

 arch/powerpc/platforms/pseries/of_helpers.c | 29 +++++++++++++++++++++++++
 arch/powerpc/platforms/pseries/of_helpers.h |  3 +++
 arch/powerpc/platforms/pseries/papr_scm.c   | 33 ++++++++++++++++++++---------
 arch/powerpc/platforms/pseries/reconfig.c   | 26 -----------------------
 drivers/of/base.c                           |  1 +
 5 files changed, 56 insertions(+), 36 deletions(-)

-- 
2.7.5

Splitting out new_property() for coming reusing and moving it to
of_helpers.c.

Also do some coding style cleanup.

Signed-off-by: Pingfan Liu <redacted>
To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Aneesh Kumar K.V <redacted>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Dan Williams <redacted>
Cc: Andrew Donnellan <redacted>
Cc: Christophe Leroy <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Cc: kexec@lists.infradead.org
---
 arch/powerpc/platforms/pseries/of_helpers.c | 28 ++++++++++++++++++++++++++++
 arch/powerpc/platforms/pseries/of_helpers.h |  3 +++
 arch/powerpc/platforms/pseries/reconfig.c   | 26 --------------------------
 3 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
index 66dfd82..1022e0f 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c

@@ -7,6 +7,34 @@
 
 #include "of_helpers.h"
 
+struct property *new_property(const char *name, const int length,
+		const unsigned char *value, struct property *last)
+{
+	struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
+
+	if (!new)
+		return NULL;
+
+	new->name = kstrdup(name, GFP_KERNEL);
+	if (!new->name)
+		goto cleanup;
+	new->value = kmalloc(length + 1, GFP_KERNEL);
+	if (!new->value)
+		goto cleanup;
+
+	memcpy(new->value, value, length);
+	*(((char *)new->value) + length) = 0;
+	new->length = length;
+	new->next = last;
+	return new;
+
+cleanup:
+	kfree(new->name);
+	kfree(new->value);
+	kfree(new);
+	return NULL;
+}
+
 /**
  * pseries_of_derive_parent - basically like dirname(1)
  * @path:  the full_name of a node to be added to the tree

diff --git a/arch/powerpc/platforms/pseries/of_helpers.h b/arch/powerpc/platforms/pseries/of_helpers.h
index decad65..34add82 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.h
+++ b/arch/powerpc/platforms/pseries/of_helpers.h

@@ -4,6 +4,9 @@
 
 #include <linux/of.h>
 
+struct property *new_property(const char *name, const int length,
+		const unsigned char *value, struct property *last);
+
 struct device_node *pseries_of_derive_parent(const char *path);
 
 #endif /* _PSERIES_OF_HELPERS_H */

diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
index 7f7369f..8e5a2ba 100644
--- a/arch/powerpc/platforms/pseries/reconfig.c
+++ b/arch/powerpc/platforms/pseries/reconfig.c

@@ -165,32 +165,6 @@ static char * parse_next_property(char *buf, char *end, char **name, int *length
 	return tmp;
 }
 
-static struct property *new_property(const char *name, const int length,
-				     const unsigned char *value, struct property *last)
-{
-	struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
-
-	if (!new)
-		return NULL;
-
-	if (!(new->name = kstrdup(name, GFP_KERNEL)))
-		goto cleanup;
-	if (!(new->value = kmalloc(length + 1, GFP_KERNEL)))
-		goto cleanup;
-
-	memcpy(new->value, value, length);
-	*(((char *)new->value) + length) = 0;
-	new->length = length;
-	new->next = last;
-	return new;
-
-cleanup:
-	kfree(new->name);
-	kfree(new->value);
-	kfree(new);
-	return NULL;
-}
-
 static int do_add_node(char *buf, size_t bufsize)
 {
 	char *path, *end, *name;

-- 
2.7.5

At present, plpar_hcall(H_SCM_BIND_MEM, ...) takes a very long time, so
if dumping to fsdax, it will take a very long time.

Take a closer look, during the papr_scm initialization, the only
configuration is through drc_pmem_bind()-> plpar_hcall(H_SCM_BIND_MEM,
...), which helps to set up the bound address.

On pseries, for kexec -l/-p kernel, there is no reset of hardware, and this
step can be stepped around to save times.  So the pmem bound address can be
passed to the 2nd kernel through a dynamic added property "bound-addr" in
dt node 'ibm,pmemory'.

Signed-off-by: Pingfan Liu <redacted>
To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Aneesh Kumar K.V <redacted>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Dan Williams <redacted>
Cc: Andrew Donnellan <redacted>
Cc: Christophe Leroy <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Cc: kexec@lists.infradead.org
---
note: This patch has not been tested since I can not get such a pseries with pmem.
      Please kindly to give some suggestion, thanks.
---
 arch/powerpc/platforms/pseries/of_helpers.c |  1 +
 arch/powerpc/platforms/pseries/papr_scm.c   | 33 ++++++++++++++++++++---------
 drivers/of/base.c                           |  1 +
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
index 1022e0f..2c7bab4 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c

@@ -34,6 +34,7 @@ struct property *new_property(const char *name, const int length,
 	kfree(new);
 	return NULL;
 }
+EXPORT_SYMBOL(new_property);
 
 /**
  * pseries_of_derive_parent - basically like dirname(1)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 0b4467e..54ae903 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c

@@ -14,6 +14,7 @@
 #include <linux/delay.h>
 
 #include <asm/plpar_wrappers.h>
+#include "of_helpers.h"
 
 #define BIND_ANY_ADDR (~0ul)
 

@@ -383,7 +384,7 @@ static int papr_scm_probe(struct platform_device *pdev)
 {
 	struct device_node *dn = pdev->dev.of_node;
 	u32 drc_index, metadata_size;
-	u64 blocks, block_size;
+	u64 blocks, block_size, bound_addr = 0;
 	struct papr_scm_priv *p;
 	const char *uuid_str;
 	u64 uuid[2];

@@ -440,17 +441,29 @@ static int papr_scm_probe(struct platform_device *pdev)
 	p->metadata_size = metadata_size;
 	p->pdev = pdev;
 
-	/* request the hypervisor to bind this region to somewhere in memory */
-	rc = drc_pmem_bind(p);
+	of_property_read_u64(dn, "bound-addr", &bound_addr);
+	if (bound_addr) {
+		p->bound_addr = bound_addr;
+	} else {
+		struct property *property;
+		u64 big;
 
-	/* If phyp says drc memory still bound then force unbound and retry */
-	if (rc == H_OVERLAP)
-		rc = drc_pmem_query_n_bind(p);
+		/* request the hypervisor to bind this region to somewhere in memory */
+		rc = drc_pmem_bind(p);
 
-	if (rc != H_SUCCESS) {
-		dev_err(&p->pdev->dev, "bind err: %d\n", rc);
-		rc = -ENXIO;
-		goto err;
+		/* If phyp says drc memory still bound then force unbound and retry */
+		if (rc == H_OVERLAP)
+			rc = drc_pmem_query_n_bind(p);
+
+		if (rc != H_SUCCESS) {
+			dev_err(&p->pdev->dev, "bind err: %d\n", rc);
+			rc = -ENXIO;
+			goto err;
+		}
+		big = cpu_to_be64(p->bound_addr);
+		property = new_property("bound-addr", sizeof(u64), (const unsigned char *)&big,
+			NULL);
+		of_add_property(dn, property);
 	}
 
 	/* setup the resource for the newly bound range */

diff --git a/drivers/of/base.c b/drivers/of/base.c
index ae03b12..602d2a9 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c

@@ -1817,6 +1817,7 @@ int of_add_property(struct device_node *np, struct property *prop)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(of_add_property);
 
 int __of_remove_property(struct device_node *np, struct property *prop)
 {

-- 
2.7.5

On 4/3/20 7:47 pm, Pingfan Liu wrote:

Splitting out new_property() for coming reusing and moving it to
of_helpers.c.

Also do some coding style cleanup.

Signed-off-by: Pingfan Liu <redacted>
To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Aneesh Kumar K.V <redacted>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Dan Williams <redacted>
Cc: Andrew Donnellan <redacted>
Cc: Christophe Leroy <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Cc: kexec@lists.infradead.org

Would this be useful to move into generic OF code?

Also if more functions are moving into of_helpers.c perhaps there should 
be a common function name prefix.

Otherwise your style cleanup looks good.

Reviewed-by: Andrew Donnellan <redacted>


-- 
Andrew Donnellan              OzLabs, ADL Canberra
ajd@linux.ibm.com             IBM Australia Limited

Hi,

Pingfan Liu [off-list ref] writes:

Splitting out new_property() for coming reusing and moving it to
of_helpers.c.

[...]

+struct property *new_property(const char *name, const int length,
+		const unsigned char *value, struct property *last)
+{
+	struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
+
+	if (!new)
+		return NULL;
+
+	new->name = kstrdup(name, GFP_KERNEL);
+	if (!new->name)
+		goto cleanup;
+	new->value = kmalloc(length + 1, GFP_KERNEL);
+	if (!new->value)
+		goto cleanup;
+
+	memcpy(new->value, value, length);
+	*(((char *)new->value) + length) = 0;
+	new->length = length;
+	new->next = last;
+	return new;
+
+cleanup:
+	kfree(new->name);
+	kfree(new->value);
+	kfree(new);
+	return NULL;
+}

This function in its current form isn't suitable for more general use:

* It appears to be tailored to string properties - note the char * value
  parameter, the length + 1 allocation and nul termination.

* Most code shouldn't need the 'last' argument. The code where this
  currently resides builds a list of properties and attaches it to a new
  node, bypassing of_add_property().

Let's look at the call site you add in your next patch:

+		big = cpu_to_be64(p->bound_addr);
+		property = new_property("bound-addr", sizeof(u64), (const unsigned char *)&big,
+			NULL);
+		of_add_property(dn, property);

So you have to use a cast, and this is going to allocate (sizeof(u64) + 1)
for the value, is that what you want?

I think you should leave that legacy pseries reconfig code undisturbed
(frankly that stuff should get deprecated and removed) and if you want a
generic helper it should look more like:

struct property *of_property_new(const char *name, size_t length,
                                 const void *value, gfp_t allocflags)

__of_prop_dup() looks like a good model/guide here.

On Sat, Mar 7, 2020 at 3:59 AM Nathan Lynch [off-list ref] wrote:

Hi,

Pingfan Liu [off-list ref] writes:

quoted

Splitting out new_property() for coming reusing and moving it to
of_helpers.c.

[...]

quoted

+struct property *new_property(const char *name, const int length,
+             const unsigned char *value, struct property *last)
+{
+     struct property *new = kzalloc(sizeof(*new), GFP_KERNEL);
+
+     if (!new)
+             return NULL;
+
+     new->name = kstrdup(name, GFP_KERNEL);
+     if (!new->name)
+             goto cleanup;
+     new->value = kmalloc(length + 1, GFP_KERNEL);
+     if (!new->value)
+             goto cleanup;
+
+     memcpy(new->value, value, length);
+     *(((char *)new->value) + length) = 0;
+     new->length = length;
+     new->next = last;
+     return new;
+
+cleanup:
+     kfree(new->name);
+     kfree(new->value);
+     kfree(new);
+     return NULL;
+}

This function in its current form isn't suitable for more general use:

* It appears to be tailored to string properties - note the char * value
  parameter, the length + 1 allocation and nul termination.

* Most code shouldn't need the 'last' argument. The code where this
  currently resides builds a list of properties and attaches it to a new
  node, bypassing of_add_property().

Let's look at the call site you add in your next patch:

+               big = cpu_to_be64(p->bound_addr);
+               property = new_property("bound-addr", sizeof(u64), (const unsigned char *)&big,
+                       NULL);
+               of_add_property(dn, property);

So you have to use a cast, and this is going to allocate (sizeof(u64) + 1)
for the value, is that what you want?

I think you should leave that legacy pseries reconfig code undisturbed
(frankly that stuff should get deprecated and removed) and if you want a
generic helper it should look more like:

struct property *of_property_new(const char *name, size_t length,
                                 const void *value, gfp_t allocflags)

__of_prop_dup() looks like a good model/guide here.

Thanks for your good suggestion.
I will re-code based on your suggestion, if [2/2] turns out acceptable.

Regards,
Pingfan

On Wed, Mar 4, 2020 at 7:50 PM Pingfan Liu [off-list ref] wrote:

At present, plpar_hcall(H_SCM_BIND_MEM, ...) takes a very long time, so
if dumping to fsdax, it will take a very long time.

Take a closer look, during the papr_scm initialization, the only
configuration is through drc_pmem_bind()-> plpar_hcall(H_SCM_BIND_MEM,
...), which helps to set up the bound address.

On pseries, for kexec -l/-p kernel, there is no reset of hardware, and this
step can be stepped around to save times.  So the pmem bound address can be
passed to the 2nd kernel through a dynamic added property "bound-addr" in
dt node 'ibm,pmemory'.

Signed-off-by: Pingfan Liu <redacted>
To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Aneesh Kumar K.V <redacted>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Dan Williams <redacted>
Cc: Andrew Donnellan <redacted>
Cc: Christophe Leroy <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Cc: kexec@lists.infradead.org
---
note: This patch has not been tested since I can not get such a pseries with pmem.
      Please kindly to give some suggestion, thanks.

There was some qemu patches to implement the Hcall interface floating
around a while ago. I'm not sure they ever made it into upstream qemu
though.

That should probably be "linux,bound-addr"

The other thing that stands out to me is that you aren't removing the
property when the region is unbound. As a general rule I'd prefer we
didn't hack the DT at runtime, but if we are going to then we should
make sure we're not putting anything wrong in there.

Appreciate for your kind review. And I have some comment as below.

On Fri, Mar 13, 2020 at 11:18 AM Oliver O'Halloran [off-list ref] wrote:

On Wed, Mar 4, 2020 at 7:50 PM Pingfan Liu [off-list ref] wrote:

quoted

At present, plpar_hcall(H_SCM_BIND_MEM, ...) takes a very long time, so
if dumping to fsdax, it will take a very long time.

Take a closer look, during the papr_scm initialization, the only
configuration is through drc_pmem_bind()-> plpar_hcall(H_SCM_BIND_MEM,
...), which helps to set up the bound address.

On pseries, for kexec -l/-p kernel, there is no reset of hardware, and this
step can be stepped around to save times.  So the pmem bound address can be
passed to the 2nd kernel through a dynamic added property "bound-addr" in
dt node 'ibm,pmemory'.

Signed-off-by: Pingfan Liu <redacted>
To: linuxppc-dev@lists.ozlabs.org
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <redacted>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Hari Bathini <hbathini@linux.ibm.com>
Cc: Aneesh Kumar K.V <redacted>
Cc: Oliver O'Halloran <oohall@gmail.com>
Cc: Dan Williams <redacted>
Cc: Andrew Donnellan <redacted>
Cc: Christophe Leroy <redacted>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <redacted>
Cc: kexec@lists.infradead.org
---
note: This patch has not been tested since I can not get such a pseries with pmem.
      Please kindly to give some suggestion, thanks.

There was some qemu patches to implement the Hcall interface floating
around a while ago. I'm not sure they ever made it into upstream qemu
though.

Unfortunately, it does not appear in latest qemu code. I think
probably virt-pmem has achieved the same feature.

quoted

---
 arch/powerpc/platforms/pseries/of_helpers.c |  1 +
 arch/powerpc/platforms/pseries/papr_scm.c   | 33 ++++++++++++++++++++---------
 drivers/of/base.c                           |  1 +
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
index 1022e0f..2c7bab4 100644
--- a/arch/powerpc/platforms/pseries/of_helpers.c
+++ b/arch/powerpc/platforms/pseries/of_helpers.c

@@ -34,6 +34,7 @@ struct property *new_property(const char *name, const int length,
        kfree(new);
        return NULL;
 }
+EXPORT_SYMBOL(new_property);

 /**
  * pseries_of_derive_parent - basically like dirname(1)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 0b4467e..54ae903 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c

@@ -14,6 +14,7 @@
 #include <linux/delay.h>

 #include <asm/plpar_wrappers.h>
+#include "of_helpers.h"

 #define BIND_ANY_ADDR (~0ul)

@@ -383,7 +384,7 @@ static int papr_scm_probe(struct platform_device *pdev)
 {
        struct device_node *dn = pdev->dev.of_node;
        u32 drc_index, metadata_size;
-       u64 blocks, block_size;
+       u64 blocks, block_size, bound_addr = 0;
        struct papr_scm_priv *p;
        const char *uuid_str;
        u64 uuid[2];

@@ -440,17 +441,29 @@ static int papr_scm_probe(struct platform_device *pdev)
        p->metadata_size = metadata_size;
        p->pdev = pdev;

-       /* request the hypervisor to bind this region to somewhere in memory */
-       rc = drc_pmem_bind(p);
+       of_property_read_u64(dn, "bound-addr", &bound_addr);
+       if (bound_addr) {
+               p->bound_addr = bound_addr;
+       } else {
+               struct property *property;
+               u64 big;

-       /* If phyp says drc memory still bound then force unbound and retry */
-       if (rc == H_OVERLAP)
-               rc = drc_pmem_query_n_bind(p);
+               /* request the hypervisor to bind this region to somewhere in memory */
+               rc = drc_pmem_bind(p);

-       if (rc != H_SUCCESS) {
-               dev_err(&p->pdev->dev, "bind err: %d\n", rc);
-               rc = -ENXIO;
-               goto err;
+               /* If phyp says drc memory still bound then force unbound and retry */
+               if (rc == H_OVERLAP)
+                       rc = drc_pmem_query_n_bind(p);
+
+               if (rc != H_SUCCESS) {
+                       dev_err(&p->pdev->dev, "bind err: %d\n", rc);
+                       rc = -ENXIO;
+                       goto err;
+               }
+               big = cpu_to_be64(p->bound_addr);
+               property = new_property("bound-addr", sizeof(u64), (const unsigned char *)&big,
+                       NULL);

That should probably be "linux,bound-addr"

OK, thanks for suggestion.

The other thing that stands out to me is that you aren't removing the

Yes, you are right. I will fix it in V2.

property when the region is unbound. As a general rule I'd prefer we
didn't hack the DT at runtime, but if we are going to then we should
make sure we're not putting anything wrong in there.

Actually, the dynamically building of DT is widely used by "kexec -l".

The pre-condition for the hacked method is that the bound pmem-addr
will not change. And on pseries, during kexec -l/-p, a machine reset
will not be invoked, so the bound address should be changed.

Thanks,
Pingfan
[...]

On 3/4/20 2:17 PM, Pingfan Liu wrote:

At present, plpar_hcall(H_SCM_BIND_MEM, ...) takes a very long time, so
if dumping to fsdax, it will take a very long time.

that should be fixed by

faa6d21153fd11e139dd880044521389b34a24f2
Author:       Aneesh Kumar K.V [off-list ref]
AuthorDate:   Tue Sep 3 18:04:52 2019 +0530
Commit:       Michael Ellerman [off-list ref]
CommitDate:   Wed Sep 25 08:32:59 2019 +1000

powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error

Right now we force an unbind of SCM memory at drcindex on H_OVERLAP error.
This really slows down operations like kexec where we get the H_OVERLAP
error because we don't go through a full hypervisor re init.

H_OVERLAP error for a H_SCM_BIND_MEM hcall indicates that SCM memory at
drc index is already bound. Since we don't specify a logical memory
address for bind hcall, we can use the H_SCM_QUERY hcall to query
the already bound logical address.

Take a closer look, during the papr_scm initialization, the only
configuration is through drc_pmem_bind()-> plpar_hcall(H_SCM_BIND_MEM,
...), which helps to set up the bound address.

On pseries, for kexec -l/-p kernel, there is no reset of hardware, and this
step can be stepped around to save times.  So the pmem bound address can be
passed to the 2nd kernel through a dynamic added property "bound-addr" in
dt node 'ibm,pmemory'.

-aneesh

On Mon, Mar 16, 2020 at 10:53 AM Aneesh Kumar K.V
[off-list ref] wrote:

On 3/4/20 2:17 PM, Pingfan Liu wrote:

quoted

At present, plpar_hcall(H_SCM_BIND_MEM, ...) takes a very long time, so
if dumping to fsdax, it will take a very long time.

that should be fixed by

faa6d21153fd11e139dd880044521389b34a24f2
Author:       Aneesh Kumar K.V [off-list ref]
AuthorDate:   Tue Sep 3 18:04:52 2019 +0530
Commit:       Michael Ellerman [off-list ref]
CommitDate:   Wed Sep 25 08:32:59 2019 +1000

powerpc/nvdimm: use H_SCM_QUERY hcall on H_OVERLAP error

Right now we force an unbind of SCM memory at drcindex on H_OVERLAP error.
This really slows down operations like kexec where we get the H_OVERLAP
error because we don't go through a full hypervisor re init.

H_OVERLAP error for a H_SCM_BIND_MEM hcall indicates that SCM memory at
drc index is already bound. Since we don't specify a logical memory
address for bind hcall, we can use the H_SCM_QUERY hcall to query
the already bound logical address.

Good to know it.

Thanks,
Pingfan

quoted

Take a closer look, during the papr_scm initialization, the only
configuration is through drc_pmem_bind()-> plpar_hcall(H_SCM_BIND_MEM,
...), which helps to set up the bound address.

On pseries, for kexec -l/-p kernel, there is no reset of hardware, and this
step can be stepped around to save times.  So the pmem bound address can be
passed to the 2nd kernel through a dynamic added property "bound-addr" in
dt node 'ibm,pmemory'.

-aneesh