[PATCH v3 0/3] powerpc/papr_scm: Workaround for failure of drc bind after kexec

[PATCH v3 0/3] powerpc/papr_scm: Workaround for failure of drc bind after kexec

[Vaibhav Jain]

Presently an error is returned in response to hcall H_SCM_BIND_MEM when a
new kernel boots on lpar via kexec. This prevents papr_scm from registering
drc memory regions with nvdimm. The error reported is of the form below:

"papr_scm ibm,persistent-memory:ibm,pmemory@44100002: bind err: -68"

On investigation it was revealed that phyp returns this error as previous
kernel did not completely release bindings for drc scm-memory blocks and
hence phyp rejected request for re-binding these block to lpar with error
H_OVERLAP. Also support for a new H_SCM_UNBIND_ALL is recently added which
is better suited for releasing all the bound scm-memory block from an lpar.

So leveraging new hcall H_SCM_UNBIND_ALL, we can workaround H_OVERLAP issue
during kexec by forcing an unbind of all drm scm-memory blocks and issuing
H_SCM_BIND_MEM to re-bind the drc scm-memory blocks to lpar. This sequence
will also be needed when a new kernel boot on lpar after previous kernel
panicked and it never got an opportunity to call H_SCM_UNBIND_MEM/ALL.

Hence this patch-set implements following changes to papr_scm module:

* Update hvcall.h to include opcodes for new hcall H_SCM_UNBIND_ALL.

* Update it to use H_SCM_UNBIND_ALL instead of H_SCM_UNBIND_MEM

* In case hcall H_SCM_BIND_MEM fails with error H_OVERLAP, force
  H_SCM_UNBIND_ALL and retry the bind operation again.

With the patch-set applied re-bind of drc scm-memory to lpar succeeds after
a kexec to new kernel as illustrated below:

# Old kernel
$ sudo ndctl list -R
[
  {
    "dev":"region0",
    <snip>
    ....
  }
]
# kexec to new kernel
$ sudo kexec --initrd=... vmlinux
...
...
I'm in purgatory
...
papr_scm ibm,persistent-memory:ibm,pmemory@44100002: Un-binding and retrying
...
# New kernel
$ sudo ndctl list -R
[
  {
    "dev":"region0",
    <snip>
    ....
  }
]

---
Change-log:
v3:
* Fixed a build warning reported by kbuild test robot.
* Updated the hcall opcode from latest papr-scm specification.
* Fixed a minor code comment & patch description as pointed out by Oliver.

v2:
* Addressed review comments from Oliver on v1 patchset.

Vaibhav Jain (3):
  powerpc/pseries: Update SCM hcall op-codes in hvcall.h
  powerpc/papr_scm: Update drc_pmem_unbind() to use H_SCM_UNBIND_ALL
  powerpc/papr_scm: Force a scm-unbind if initial scm-bind fails

 arch/powerpc/include/asm/hvcall.h         | 11 ++++--
 arch/powerpc/platforms/pseries/papr_scm.c | 44 ++++++++++++++++++-----
 2 files changed, 44 insertions(+), 11 deletions(-)

-- 
2.21.0

[PATCH v3 1/3] powerpc/pseries: Update SCM hcall op-codes in hvcall.h

[Vaibhav Jain]

Update the hvcalls.h to include op-codes for new hcalls introduce to
manage SCM memory. Also update existing hcall definitions to reflect
current papr specification for SCM.

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Change-log:

v3:
* Added updated opcode for H_SCM_HEALTH [Oliver]

v2:
* None new patch in this series.
---
 arch/powerpc/include/asm/hvcall.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index 463c63a9fcf1..11112023e327 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -302,9 +302,14 @@
 #define H_SCM_UNBIND_MEM        0x3F0
 #define H_SCM_QUERY_BLOCK_MEM_BINDING 0x3F4
 #define H_SCM_QUERY_LOGICAL_MEM_BINDING 0x3F8
-#define H_SCM_MEM_QUERY	        0x3FC
-#define H_SCM_BLOCK_CLEAR       0x400
-#define MAX_HCALL_OPCODE	H_SCM_BLOCK_CLEAR
+#define H_SCM_UNBIND_ALL        0x3FC
+#define H_SCM_HEALTH            0x400
+#define H_SCM_PERFORMANCE_STATS 0x418
+#define MAX_HCALL_OPCODE	H_SCM_PERFORMANCE_STATS
+
+/* Scope args for H_SCM_UNBIND_ALL */
+#define H_UNBIND_SCOPE_ALL (0x1)
+#define H_UNBIND_SCOPE_DRC (0x2)
 
 /* H_VIOCTL functions */
 #define H_GET_VIOA_DUMP_SIZE	0x01
-- 
2.21.0

[PATCH v3 2/3] powerpc/papr_scm: Update drc_pmem_unbind() to use H_SCM_UNBIND_ALL

[Vaibhav Jain]

The new hcall named H_SCM_UNBIND_ALL has been introduce that can
unbind all or specific scm memory assigned to an lpar. This is
more efficient than using H_SCM_UNBIND_MEM as currently we don't
support partial unbind of scm memory.

Hence this patch proposes following changes to drc_pmem_unbind():

    * Update drc_pmem_unbind() to replace hcall H_SCM_UNBIND_MEM to
      H_SCM_UNBIND_ALL.

    * Update drc_pmem_unbind() to handles cases when PHYP asks the guest
      kernel to wait for specific amount of time before retrying the
      hcall via the 'LONG_BUSY' return value.

    * Ensure appropriate error code is returned back from the function
      in case of an error.

Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
Change-log:

v3:
* Fixed a build warning reported by kbuild-robot.
* Updated patch description to put emphasis on 'scm memory' instead of
  'scm drc memory blocks' as for phyp there is a stark difference
  between how drc are managed for scm memory v/s regular memory. [Oliver]

v2:
* Added a dev_dbg when unbind operation succeeds [Oliver]
* Changed type of variable 'rc' to int64_t [Oliver]
* Removed the code that was logging a warning in case bind operation
  takes >1-seconds [Oliver]
* Spinned off changes to hvcall.h as a separate patch. [Oliver]
---
 arch/powerpc/platforms/pseries/papr_scm.c | 29 +++++++++++++++++------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 96c53b23e58f..c01a03fd3ee7 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -11,6 +11,7 @@
 #include <linux/sched.h>
 #include <linux/libnvdimm.h>
 #include <linux/platform_device.h>
+#include <linux/delay.h>
 
 #include <asm/plpar_wrappers.h>
 
@@ -77,22 +78,36 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
 static int drc_pmem_unbind(struct papr_scm_priv *p)
 {
 	unsigned long ret[PLPAR_HCALL_BUFSIZE];
-	uint64_t rc, token;
+	uint64_t token = 0;
+	int64_t rc;
 
-	token = 0;
+	dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index);
 
-	/* NB: unbind has the same retry requirements mentioned above */
+	/* NB: unbind has the same retry requirements as drc_pmem_bind() */
 	do {
-		rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
-				p->bound_addr, p->blocks, token);
+
+		/* Unbind of all SCM resources associated with drcIndex */
+		rc = plpar_hcall(H_SCM_UNBIND_ALL, ret, H_UNBIND_SCOPE_DRC,
+				 p->drc_index, token);
 		token = ret[0];
-		cond_resched();
+
+		/* Check if we are stalled for some time */
+		if (H_IS_LONG_BUSY(rc)) {
+			msleep(get_longbusy_msecs(rc));
+			rc = H_BUSY;
+		} else if (rc == H_BUSY) {
+			cond_resched();
+		}
+
 	} while (rc == H_BUSY);
 
 	if (rc)
 		dev_err(&p->pdev->dev, "unbind error: %lld\n", rc);
+	else
+		dev_dbg(&p->pdev->dev, "unbind drc %x complete\n",
+			p->drc_index);
 
-	return !!rc;
+	return rc == H_SUCCESS ? 0 : -ENXIO;
 }
 
 static int papr_scm_meta_get(struct papr_scm_priv *p,
-- 
2.21.0

[PATCH v3 3/3] powerpc/papr_scm: Force a scm-unbind if initial scm-bind fails

[Vaibhav Jain]

In some cases initial bind of scm memory for an lpar can fail if
previously it wasn't released using a scm-unbind hcall. This situation
can arise due to panic of the previous kernel or forced lpar
fadump. In such cases the H_SCM_BIND_MEM return a H_OVERLAP error.

To mitigate such cases the patch updates papr_scm_probe() to force a
call to drc_pmem_unbind() in case the initial bind of scm memory fails
with EBUSY error. In case scm-bind operation again fails after the
forced scm-unbind then we follow the existing error path. We also
update drc_pmem_bind() to handle the H_OVERLAP error returned by phyp
and indicate it as a EBUSY error back to the caller.

Suggested-by: "Oliver O'Halloran" <oohall@gmail.com>
Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
---
Change-log:
v3:
* Minor update to a code comment. [Oliver]

v2:
* Moved the retry code from drc_pmem_bind() to papr_scm_probe()
  [Oliver]
* Changed the type of variable 'rc' in drc_pmem_bind() to
  int64_t. [Oliver]
---
 arch/powerpc/platforms/pseries/papr_scm.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index c01a03fd3ee7..7c5e10c063a0 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -43,8 +43,9 @@ struct papr_scm_priv {
 static int drc_pmem_bind(struct papr_scm_priv *p)
 {
 	unsigned long ret[PLPAR_HCALL_BUFSIZE];
-	uint64_t rc, token;
 	uint64_t saved = 0;
+	uint64_t token;
+	int64_t rc;
 
 	/*
 	 * When the hypervisor cannot map all the requested memory in a single
@@ -64,6 +65,10 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
 	} while (rc == H_BUSY);
 
 	if (rc) {
+		/* H_OVERLAP needs a separate error path */
+		if (rc == H_OVERLAP)
+			return -EBUSY;
+
 		dev_err(&p->pdev->dev, "bind err: %lld\n", rc);
 		return -ENXIO;
 	}
@@ -331,6 +336,14 @@ static int papr_scm_probe(struct platform_device *pdev)
 
 	/* request the hypervisor to bind this region to somewhere in memory */
 	rc = drc_pmem_bind(p);
+
+	/* If phyp says drc memory still bound then force unbound and retry */
+	if (rc == -EBUSY) {
+		dev_warn(&pdev->dev, "Retrying bind after unbinding\n");
+		drc_pmem_unbind(p);
+		rc = drc_pmem_bind(p);
+	}
+
 	if (rc)
 		goto err;
 
-- 
2.21.0

Re: [PATCH v3 2/3] powerpc/papr_scm: Update drc_pmem_unbind() to use H_SCM_UNBIND_ALL

[Aneesh Kumar K.V]

On 6/26/19 7:34 PM, Vaibhav Jain wrote:
> The new hcall named H_SCM_UNBIND_ALL has been introduce that can
> unbind all or specific scm memory assigned to an lpar. This is
> more efficient than using H_SCM_UNBIND_MEM as currently we don't
> support partial unbind of scm memory.
> 
> Hence this patch proposes following changes to drc_pmem_unbind():
> 
>      * Update drc_pmem_unbind() to replace hcall H_SCM_UNBIND_MEM to
>        H_SCM_UNBIND_ALL.
> 
>      * Update drc_pmem_unbind() to handles cases when PHYP asks the guest
>        kernel to wait for specific amount of time before retrying the
>        hcall via the 'LONG_BUSY' return value.
> 
>      * Ensure appropriate error code is returned back from the function
>        in case of an error.
> 
> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Change-log:
> 
> v3:
> * Fixed a build warning reported by kbuild-robot.
> * Updated patch description to put emphasis on 'scm memory' instead of
>    'scm drc memory blocks' as for phyp there is a stark difference
>    between how drc are managed for scm memory v/s regular memory. [Oliver]
> 
> v2:
> * Added a dev_dbg when unbind operation succeeds [Oliver]
> * Changed type of variable 'rc' to int64_t [Oliver]
> * Removed the code that was logging a warning in case bind operation
>    takes >1-seconds [Oliver]
> * Spinned off changes to hvcall.h as a separate patch. [Oliver]
> ---
>   arch/powerpc/platforms/pseries/papr_scm.c | 29 +++++++++++++++++------
>   1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 96c53b23e58f..c01a03fd3ee7 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -11,6 +11,7 @@
>   #include <linux/sched.h>
>   #include <linux/libnvdimm.h>
>   #include <linux/platform_device.h>
> +#include <linux/delay.h>
>   
>   #include <asm/plpar_wrappers.h>
>   
> @@ -77,22 +78,36 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
>   static int drc_pmem_unbind(struct papr_scm_priv *p)
>   {
>   	unsigned long ret[PLPAR_HCALL_BUFSIZE];
> -	uint64_t rc, token;
> +	uint64_t token = 0;
> +	int64_t rc;
>   
> -	token = 0;
> +	dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index);
>   
> -	/* NB: unbind has the same retry requirements mentioned above */
> +	/* NB: unbind has the same retry requirements as drc_pmem_bind() */
>   	do {
> -		rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
> -				p->bound_addr, p->blocks, token);
> +
> +		/* Unbind of all SCM resources associated with drcIndex */
> +		rc = plpar_hcall(H_SCM_UNBIND_ALL, ret, H_UNBIND_SCOPE_DRC,
> +				 p->drc_index, token);
>   		token = ret[0];
> -		cond_resched();
> +
> +		/* Check if we are stalled for some time */
> +		if (H_IS_LONG_BUSY(rc)) {
> +			msleep(get_longbusy_msecs(rc));
> +			rc = H_BUSY;
> +		} else if (rc == H_BUSY) {
> +			cond_resched();
> +		}
> +
>   	} while (rc == H_BUSY);
>   
>   	if (rc)
>   		dev_err(&p->pdev->dev, "unbind error: %lld\n", rc);
> +	else
> +		dev_dbg(&p->pdev->dev, "unbind drc %x complete\n",
> +			p->drc_index);
>   
Can we add p->drc_index as part of these messages? Also s/%x/0x%x ?


> -	return !!rc;
> +	return rc == H_SUCCESS ? 0 : -ENXIO;
>   }
>   
The error -ENXIO is confusing. Can we keep the HCALL error as return for 
this? We don't check error from this. If we can't take any action based 
on the return. Then may be make it  void?


>   static int papr_scm_meta_get(struct papr_scm_priv *p,
> 


-aneesh

Re: [PATCH v3 1/3] powerpc/pseries: Update SCM hcall op-codes in hvcall.h

[Aneesh Kumar K.V]

Vaibhav Jain <vaibhav@linux.ibm.com> writes:

> Update the hvcalls.h to include op-codes for new hcalls introduce to
> manage SCM memory. Also update existing hcall definitions to reflect
> current papr specification for SCM.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>


Why split this as a separate patch? You should fold this to the next one
where we actually use the constant.

> ---
> Change-log:
>
> v3:
> * Added updated opcode for H_SCM_HEALTH [Oliver]
>
> v2:
> * None new patch in this series.
> ---
>  arch/powerpc/include/asm/hvcall.h | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index 463c63a9fcf1..11112023e327 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -302,9 +302,14 @@
>  #define H_SCM_UNBIND_MEM        0x3F0
>  #define H_SCM_QUERY_BLOCK_MEM_BINDING 0x3F4
>  #define H_SCM_QUERY_LOGICAL_MEM_BINDING 0x3F8
> -#define H_SCM_MEM_QUERY	        0x3FC
> -#define H_SCM_BLOCK_CLEAR       0x400
> -#define MAX_HCALL_OPCODE	H_SCM_BLOCK_CLEAR
> +#define H_SCM_UNBIND_ALL        0x3FC
> +#define H_SCM_HEALTH            0x400
> +#define H_SCM_PERFORMANCE_STATS 0x418
> +#define MAX_HCALL_OPCODE	H_SCM_PERFORMANCE_STATS
> +
> +/* Scope args for H_SCM_UNBIND_ALL */
> +#define H_UNBIND_SCOPE_ALL (0x1)
> +#define H_UNBIND_SCOPE_DRC (0x2)
>  
>  /* H_VIOCTL functions */
>  #define H_GET_VIOA_DUMP_SIZE	0x01
> -- 
> 2.21.0

Re: [PATCH v3 3/3] powerpc/papr_scm: Force a scm-unbind if initial scm-bind fails

[Aneesh Kumar K.V]

Vaibhav Jain <vaibhav@linux.ibm.com> writes:

> In some cases initial bind of scm memory for an lpar can fail if
> previously it wasn't released using a scm-unbind hcall. This situation
> can arise due to panic of the previous kernel or forced lpar
> fadump. In such cases the H_SCM_BIND_MEM return a H_OVERLAP error.
>
> To mitigate such cases the patch updates papr_scm_probe() to force a
> call to drc_pmem_unbind() in case the initial bind of scm memory fails
> with EBUSY error. In case scm-bind operation again fails after the
> forced scm-unbind then we follow the existing error path. We also
> update drc_pmem_bind() to handle the H_OVERLAP error returned by phyp
> and indicate it as a EBUSY error back to the caller.
>
> Suggested-by: "Oliver O'Halloran" <oohall@gmail.com>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
> ---
> Change-log:
> v3:
> * Minor update to a code comment. [Oliver]
>
> v2:
> * Moved the retry code from drc_pmem_bind() to papr_scm_probe()
>   [Oliver]
> * Changed the type of variable 'rc' in drc_pmem_bind() to
>   int64_t. [Oliver]
> ---
>  arch/powerpc/platforms/pseries/papr_scm.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index c01a03fd3ee7..7c5e10c063a0 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -43,8 +43,9 @@ struct papr_scm_priv {
>  static int drc_pmem_bind(struct papr_scm_priv *p)
>  {
>  	unsigned long ret[PLPAR_HCALL_BUFSIZE];
> -	uint64_t rc, token;
>  	uint64_t saved = 0;
> +	uint64_t token;
> +	int64_t rc;
>  
>  	/*
>  	 * When the hypervisor cannot map all the requested memory in a single
> @@ -64,6 +65,10 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
>  	} while (rc == H_BUSY);
>  
>  	if (rc) {
> +		/* H_OVERLAP needs a separate error path */
> +		if (rc == H_OVERLAP)
> +			return -EBUSY;
> +
>  		dev_err(&p->pdev->dev, "bind err: %lld\n", rc);
>  		return -ENXIO;
>  	}
> @@ -331,6 +336,14 @@ static int papr_scm_probe(struct platform_device *pdev)
>  
>  	/* request the hypervisor to bind this region to somewhere in memory */
>  	rc = drc_pmem_bind(p);
> +
> +	/* If phyp says drc memory still bound then force unbound and retry */
> +	if (rc == -EBUSY) {
> +		dev_warn(&pdev->dev, "Retrying bind after unbinding\n");
> +		drc_pmem_unbind(p);


This should only be caused by kexec right? And considering kernel nor
hypervisor won't change device binding details, can you check switching
this to H_SCM_QUERY_BLOCK_MEM_BINDING?  Will that result in faster boot? 



> +		rc = drc_pmem_bind(p);
> +	}
> +
>  	if (rc)
>  		goto err;
>  

I am also not sure about the module reference count here. Should we
increment the module reference count after a bind so that we can track
failures in ubind and fail the module unload?

-aneesh

Re: [PATCH v3 1/3] powerpc/pseries: Update SCM hcall op-codes in hvcall.h

[Oliver O'Halloran]

On Thu, Jun 27, 2019 at 2:53 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>
> > Update the hvcalls.h to include op-codes for new hcalls introduce to
> > manage SCM memory. Also update existing hcall definitions to reflect
> > current papr specification for SCM.
> >
> > Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>
>
> Why split this as a separate patch? You should fold this to the next one
> where we actually use the constant.

I figured if you're going to update the hcall numbers to reflect
reality then it should be done in a separate patch. Single logical
change per patch and all that.

Reviewed-by: Oliver O'Halloran <oohall@gmail.com>

Re: [PATCH v3 3/3] powerpc/papr_scm: Force a scm-unbind if initial scm-bind fails

[Oliver O'Halloran]

On Thu, Jun 27, 2019 at 2:58 AM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> > *snip*
> > +     /* If phyp says drc memory still bound then force unbound and retry */
> > +     if (rc == -EBUSY) {
> > +             dev_warn(&pdev->dev, "Retrying bind after unbinding\n");
> > +             drc_pmem_unbind(p);
> This should only be caused by kexec right?

We should only ever hit this path if there's an unclean shutdown, so
kdump or fadump. For a normal kexec the previous kernel should have
torn down the binding for us.

> And considering kernel nor
> hypervisor won't change device binding details, can you check switching
> this to H_SCM_QUERY_BLOCK_MEM_BINDING?

I thought about using the QUERY_BLOCK_MEM_BINDING call, but I'm not
sure it's a good idea. It bakes in assumptions about what the
*previous* kernel did with the SCM volume that might not be valid. A
panic while unbinding a region would result in a partially-bound
region which might break the query call. Also, it's possible that we
might have SCM drivers in the future that do something other than just
binding the volume in one contiguous chunk. UNBIND_ALL is robust
against all of these and robustness is what you want out of an error
handling mechanism.

> Will that result in faster boot?

As I said in the comments on v1, do we have any actual numbers on how
long the bind step takes? From memory, you could bind ~32GB in a
single bind h-call before phyp would hit it's time limit of 250us and
return a continue token. Assuming that holds we'll be saving a few
dozen milliseconds at best.

> > +             rc = drc_pmem_bind(p);
> > +     }
> > +
> >       if (rc)
> >               goto err;
> >
>
> I am also not sure about the module reference count here. Should we
> increment the module reference count after a bind so that we can track
> failures in ubind and fail the module unload?

I don't really get what you're concerned about here. The error
handling path calls drc_pmem_unbind() so if there's a bind error we
should never leave probe with memory still bound.

> -aneesh
>

Re: [PATCH v3 3/3] powerpc/papr_scm: Force a scm-unbind if initial scm-bind fails

[Aneesh Kumar K.V]

"Oliver O'Halloran" <oohall@gmail.com> writes:


> As I said in the comments on v1, do we have any actual numbers on how
> long the bind step takes? From memory, you could bind ~32GB in a
> single bind h-call before phyp would hit it's time limit of 250us and
> return a continue token. Assuming that holds we'll be saving a few
> dozen milliseconds at best.
>
>> > +             rc = drc_pmem_bind(p);
>> > +     }
>> > +
>> >       if (rc)
>> >               goto err;
>> >
>>
>> I am also not sure about the module reference count here. Should we
>> increment the module reference count after a bind so that we can track
>> failures in ubind and fail the module unload?
>
> I don't really get what you're concerned about here. The error
> handling path calls drc_pmem_unbind() so if there's a bind error we
> should never leave probe with memory still bound.
>

In the remove callback, if the ubind fail should we allow the module
unload?

-aneesh

Re: [PATCH v3 3/3] powerpc/papr_scm: Force a scm-unbind if initial scm-bind fails

[Oliver O'Halloran]

On Thu, Jun 27, 2019 at 12:58 PM Aneesh Kumar K.V
<aneesh.kumar@linux.ibm.com> wrote:
>
> "Oliver O'Halloran" <oohall@gmail.com> writes:
>
> >> > +             rc = drc_pmem_bind(p);
> >> > +     }
> >> > +
> >> >       if (rc)
> >> >               goto err;
> >> >
> >>
> >> I am also not sure about the module reference count here. Should we
> >> increment the module reference count after a bind so that we can track
> >> failures in ubind and fail the module unload?
> >
> > I don't really get what you're concerned about here. The error
> > handling path calls drc_pmem_unbind() so if there's a bind error we
> > should never leave probe with memory still bound.
> >
>
> In the remove callback, if the ubind fail should we allow the module
> unload?

If the drc_pmem_unbind() in the driver's remove function fails the
driver will still be unbound from the platform device for that DRC.
You can try re-bind the driver to the platform device and it'll hit
the unbind-then-bind again error path this series introduces (which
might also fail), but that's fine. The only reasons I can think of for
drc_pmem_unbind() failing are bugs in the hypervisor or bugs in the
driver. Forcing the module to stay loaded doesn't help either case so
I'm not seeing the benefit.

Oliver

Re: [PATCH v3 2/3] powerpc/papr_scm: Update drc_pmem_unbind() to use H_SCM_UNBIND_ALL

[Vaibhav Jain]

Thanks for reviewing this patch Aneesh,

"Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> writes:

> On 6/26/19 7:34 PM, Vaibhav Jain wrote:
>> The new hcall named H_SCM_UNBIND_ALL has been introduce that can
>> unbind all or specific scm memory assigned to an lpar. This is
>> more efficient than using H_SCM_UNBIND_MEM as currently we don't
>> support partial unbind of scm memory.
>> 
>> Hence this patch proposes following changes to drc_pmem_unbind():
>> 
>>      * Update drc_pmem_unbind() to replace hcall H_SCM_UNBIND_MEM to
>>        H_SCM_UNBIND_ALL.
>> 
>>      * Update drc_pmem_unbind() to handles cases when PHYP asks the guest
>>        kernel to wait for specific amount of time before retrying the
>>        hcall via the 'LONG_BUSY' return value.
>> 
>>      * Ensure appropriate error code is returned back from the function
>>        in case of an error.
>> 
>> Reviewed-by: Oliver O'Halloran <oohall@gmail.com>
>> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
>> ---
>> Change-log:
>> 
>> v3:
>> * Fixed a build warning reported by kbuild-robot.
>> * Updated patch description to put emphasis on 'scm memory' instead of
>>    'scm drc memory blocks' as for phyp there is a stark difference
>>    between how drc are managed for scm memory v/s regular memory. [Oliver]
>> 
>> v2:
>> * Added a dev_dbg when unbind operation succeeds [Oliver]
>> * Changed type of variable 'rc' to int64_t [Oliver]
>> * Removed the code that was logging a warning in case bind operation
>>    takes >1-seconds [Oliver]
>> * Spinned off changes to hvcall.h as a separate patch. [Oliver]
>> ---
>>   arch/powerpc/platforms/pseries/papr_scm.c | 29 +++++++++++++++++------
>>   1 file changed, 22 insertions(+), 7 deletions(-)
>> 
>> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
>> index 96c53b23e58f..c01a03fd3ee7 100644
>> --- a/arch/powerpc/platforms/pseries/papr_scm.c
>> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/sched.h>
>>   #include <linux/libnvdimm.h>
>>   #include <linux/platform_device.h>
>> +#include <linux/delay.h>
>>   
>>   #include <asm/plpar_wrappers.h>
>>   
>> @@ -77,22 +78,36 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
>>   static int drc_pmem_unbind(struct papr_scm_priv *p)
>>   {
>>   	unsigned long ret[PLPAR_HCALL_BUFSIZE];
>> -	uint64_t rc, token;
>> +	uint64_t token = 0;
>> +	int64_t rc;
>>   
>> -	token = 0;
>> +	dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index);
>>   
>> -	/* NB: unbind has the same retry requirements mentioned above */
>> +	/* NB: unbind has the same retry requirements as drc_pmem_bind() */
>>   	do {
>> -		rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
>> -				p->bound_addr, p->blocks, token);
>> +
>> +		/* Unbind of all SCM resources associated with drcIndex */
>> +		rc = plpar_hcall(H_SCM_UNBIND_ALL, ret, H_UNBIND_SCOPE_DRC,
>> +				 p->drc_index, token);
>>   		token = ret[0];
>> -		cond_resched();
>> +
>> +		/* Check if we are stalled for some time */
>> +		if (H_IS_LONG_BUSY(rc)) {
>> +			msleep(get_longbusy_msecs(rc));
>> +			rc = H_BUSY;
>> +		} else if (rc == H_BUSY) {
>> +			cond_resched();
>> +		}
>> +
>>   	} while (rc == H_BUSY);
>>   
>>   	if (rc)
>>   		dev_err(&p->pdev->dev, "unbind error: %lld\n", rc);
>> +	else
>> +		dev_dbg(&p->pdev->dev, "unbind drc %x complete\n",
>> +			p->drc_index);
>>   
> Can we add p->drc_index as part of these messages? Also s/%x/0x%x ?
the scm platform device has the name of the form
"ibm,persistent-memory:ibm,pmemory@44100002" which also contains the
drc_index. So i think printing it again in this message would be
redundant.

>
>
>> -	return !!rc;
>> +	return rc == H_SUCCESS ? 0 : -ENXIO;
>>   }
>>   
> The error -ENXIO is confusing. Can we keep the HCALL error as return for 
> this? We don't check error from this. If we can't take any action based 
> on the return. Then may be make it  void?
>
Wanted to keep the behaviour of this function symmetric to
drc_pmem_bind() which when updated in the next patch returns a kernel
error code instead of hcall error.

Agree that we arent using the return
value of this function right now but this may change in the future.

>
>>   static int papr_scm_meta_get(struct papr_scm_priv *p,
>> 
>
>
> -aneesh

-- 
Vaibhav Jain <vaibhav@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH v3 1/3] powerpc/pseries: Update SCM hcall op-codes in hvcall.h

[Michael Ellerman]

Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Update the hvcalls.h to include op-codes for new hcalls introduce to
> manage SCM memory. Also update existing hcall definitions to reflect
> current papr specification for SCM.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> Change-log:
>
> v3:
> * Added updated opcode for H_SCM_HEALTH [Oliver]
>
> v2:
> * None new patch in this series.
> ---
>  arch/powerpc/include/asm/hvcall.h | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index 463c63a9fcf1..11112023e327 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -302,9 +302,14 @@
>  #define H_SCM_UNBIND_MEM        0x3F0
>  #define H_SCM_QUERY_BLOCK_MEM_BINDING 0x3F4
>  #define H_SCM_QUERY_LOGICAL_MEM_BINDING 0x3F8
> -#define H_SCM_MEM_QUERY	        0x3FC
> -#define H_SCM_BLOCK_CLEAR       0x400
> -#define MAX_HCALL_OPCODE	H_SCM_BLOCK_CLEAR
> +#define H_SCM_UNBIND_ALL        0x3FC
> +#define H_SCM_HEALTH            0x400
> +#define H_SCM_PERFORMANCE_STATS 0x418
> +#define MAX_HCALL_OPCODE	H_SCM_PERFORMANCE_STATS

How can we be changing the meaning of HCALL numbers without breaking all
existing usages?

Where are these hypervisor calls documented?

cheers

Re: [PATCH v3 1/3] powerpc/pseries: Update SCM hcall op-codes in hvcall.h

[Oliver O'Halloran]

On Fri, Jun 28, 2019 at 1:39 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> > *snip*
>
> How can we be changing the meaning of HCALL numbers without breaking all
> existing usages?

The changed one being changed here were never used by linux or
implemented by either hypervisor.

> Where are these hypervisor calls documented?

In an internal spec from one the phyp guys. I have been complaining
about it not being open (or merged into PAPR at the very least), but
it doesn't look like there's been any movement there.

Oliver

Re: [PATCH v3 1/3] powerpc/pseries: Update SCM hcall op-codes in hvcall.h

[Michael Ellerman]

"Oliver O'Halloran" <oohall@gmail.com> writes:
> On Fri, Jun 28, 2019 at 1:39 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>
>> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>> > *snip*
>>
>> How can we be changing the meaning of HCALL numbers without breaking all
>> existing usages?
>
> The changed one being changed here were never used by linux or
> implemented by either hypervisor.

OK. Please say so in the change log so that it's clear it's not a
behaviour change.

>> Where are these hypervisor calls documented?
>
> In an internal spec from one the phyp guys. I have been complaining
> about it not being open (or merged into PAPR at the very least), but
> it doesn't look like there's been any movement there.

Please send a patch for Documentation/powerpc/ describing the API for
the hypercalls we're using.

cheers

Re: [PATCH v3 1/3] powerpc/pseries: Update SCM hcall op-codes in hvcall.h

[Vaibhav Jain]

Thanks for reviewing this patch Mpe.

Michael Ellerman <mpe@ellerman.id.au> writes:

> "Oliver O'Halloran" <oohall@gmail.com> writes:
>> On Fri, Jun 28, 2019 at 1:39 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>>
>>> Vaibhav Jain <vaibhav@linux.ibm.com> writes:
>>> > *snip*
>>>
>>> How can we be changing the meaning of HCALL numbers without breaking all
>>> existing usages?
>>
>> The changed one being changed here were never used by linux or
>> implemented by either hypervisor.
>
> OK. Please say so in the change log so that it's clear it's not a
> behaviour change.
I have respinned this patchset to v4 updating this patch's
description mentioning the hcall opcodes that are removed.

>
>>> Where are these hypervisor calls documented?
>>
>> In an internal spec from one the phyp guys. I have been complaining
>> about it not being open (or merged into PAPR at the very least), but
>> it doesn't look like there's been any movement there.
>
> Please send a patch for Documentation/powerpc/ describing the API for
> the hypercalls we're using.
Sure will send a separate patch documenting this.

> cheers
>

-- 
Vaibhav Jain <vaibhav@linux.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.