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

[PATCH 0/2] 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 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
$ sudo ndctl list -R
[
  {
    "dev":"region0",
    <snip>
    ....
  }
]


Vaibhav Jain (2):
  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         |  2 +-
 arch/powerpc/platforms/pseries/papr_scm.c | 60 ++++++++++++++++++++---
 2 files changed, 53 insertions(+), 9 deletions(-)

-- 
2.21.0

[PATCH 1/2] 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 the memory drc memory-blocks assigned to an lpar. This is
more efficient than using H_SCM_UNBIND_MEM as currently we don't
support partial unbind of drc memory-blocks.

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. In case it takes more
      than 1 second to unbind the memory log a warning.

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

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
 arch/powerpc/include/asm/hvcall.h         |  2 +-
 arch/powerpc/platforms/pseries/papr_scm.c | 37 ++++++++++++++++++++---
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
index 463c63a9fcf1..bb56fa0f976c 100644
--- a/arch/powerpc/include/asm/hvcall.h
+++ b/arch/powerpc/include/asm/hvcall.h
@@ -302,7 +302,7 @@
 #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_UNBIND_ALL        0x3FC
 #define H_SCM_BLOCK_CLEAR       0x400
 #define MAX_HCALL_OPCODE	H_SCM_BLOCK_CLEAR
 
diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index 96c53b23e58f..d790e4e4ffb3 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -11,11 +11,16 @@
 #include <linux/sched.h>
 #include <linux/libnvdimm.h>
 #include <linux/platform_device.h>
+#include <linux/delay.h>
 
 #include <asm/plpar_wrappers.h>
 
 #define BIND_ANY_ADDR (~0ul)
 
+/* Scope args for H_SCM_UNBIND_ALL */
+#define H_UNBIND_SCOPE_ALL (0x1)
+#define H_UNBIND_SCOPE_DRC (0x2)
+
 #define PAPR_SCM_DIMM_CMD_MASK \
 	((1ul << ND_CMD_GET_CONFIG_SIZE) | \
 	 (1ul << ND_CMD_GET_CONFIG_DATA) | \
@@ -78,21 +83,43 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
 {
 	unsigned long ret[PLPAR_HCALL_BUFSIZE];
 	uint64_t rc, token;
+	unsigned long starttime;
 
 	token = 0;
 
-	/* NB: unbind has the same retry requirements mentioned above */
+	dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index);
+
+	/* NB: unbind_all has the same retry requirements as drc_pmem_bind() */
+	starttime = HZ;
 	do {
-		rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
-				p->bound_addr, p->blocks, token);
+		/* If this is taking too much time then log warning */
+		if (jiffies_to_msecs(HZ - starttime) > 1000) {
+			dev_warn(&p->pdev->dev,
+				 "unbind taking > 1s to complete\n");
+			starttime = HZ;
+		}
+
+		/* 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;
+			token = 0;
+
+		} else if (rc == H_BUSY) {
+			cond_resched();
+		}
+
 	} while (rc == H_BUSY);
 
 	if (rc)
 		dev_err(&p->pdev->dev, "unbind error: %lld\n", rc);
 
-	return !!rc;
+	return rc == H_SUCCESS ? 0 : -ENXIO;
 }
 
 static int papr_scm_meta_get(struct papr_scm_priv *p,
-- 
2.21.0

[PATCH 2/2] 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 reset. In
such cases the H_SCM_BIND_MEM return a H_OVERLAP error.

To mitigate such cases the patch updates drc_pmem_bind() to force a
call to drc_pmem_unbind() in case the initial bind of scm memory fails
with H_OVERLAP error. In case scm-bind operation again fails after the
forced scm-unbind then we follow the existing error path.

Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
---
 arch/powerpc/platforms/pseries/papr_scm.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
index d790e4e4ffb3..049d7927c0a4 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -44,19 +44,26 @@ struct papr_scm_priv {
 	struct nd_interleave_set nd_set;
 };
 
+/* Forward declaration */
+static int drc_pmem_unbind(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 saved;
+	bool tried_unbind = false;
 
+	dev_dbg(&p->pdev->dev, "bind drc %x\n", p->drc_index);
 	/*
 	 * When the hypervisor cannot map all the requested memory in a single
 	 * hcall it returns H_BUSY and we call again with the token until
 	 * we get H_SUCCESS. Aborting the retry loop before getting H_SUCCESS
 	 * leave the system in an undefined state, so we wait.
 	 */
+retry:
 	token = 0;
+	saved = 0;
 
 	do {
 		rc = plpar_hcall(H_SCM_BIND_MEM, ret, p->drc_index, 0,
@@ -68,8 +75,18 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
 	} while (rc == H_BUSY);
 
 	if (rc) {
-		dev_err(&p->pdev->dev, "bind err: %lld\n", rc);
-		return -ENXIO;
+		/* retry after unbinding */
+		if (rc == H_OVERLAP &&  !tried_unbind) {
+			dev_warn(&p->pdev->dev, "Un-binding and retrying\n");
+			/* Try unbind and ignore any errors */
+			tried_unbind = true;
+			drc_pmem_unbind(p);
+			goto retry;
+
+		} else {
+			dev_err(&p->pdev->dev, "bind err: %lld\n", rc);
+			return -ENXIO;
+		}
 	}
 
 	p->bound_addr = saved;
-- 
2.21.0

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

[Oliver O'Halloran]

On Tue, Jun 25, 2019 at 1:03 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>
> The new hcall named H_SCM_UNBIND_ALL has been introduce that can
> unbind all the memory drc memory-blocks assigned to an lpar. This is
> more efficient than using H_SCM_UNBIND_MEM as currently we don't
> support partial unbind of drc memory-blocks.
>
> 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. In case it takes more
>       than 1 second to unbind the memory log a warning.

Have you benchmarked how long a typical bind operation takes for very
large (1+TB) volumes? I remember it taking a while for the driver to
finish probing and I'd rather we didn't add spurious warnings. That
might have been due to the time taken to online memory rather than the
bind though.

>     * Ensure appropriate error code is returned back from the function
>       in case of an error.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/hvcall.h         |  2 +-
>  arch/powerpc/platforms/pseries/papr_scm.c | 37 ++++++++++++++++++++---
>  2 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hvcall.h b/arch/powerpc/include/asm/hvcall.h
> index 463c63a9fcf1..bb56fa0f976c 100644
> --- a/arch/powerpc/include/asm/hvcall.h
> +++ b/arch/powerpc/include/asm/hvcall.h
> @@ -302,7 +302,7 @@
>  #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_UNBIND_ALL        0x3FC
>  #define H_SCM_BLOCK_CLEAR       0x400
>  #define MAX_HCALL_OPCODE       H_SCM_BLOCK_CLEAR

We should probably update all these to match the latest spec. Can you
do that in a separate patch?

> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 96c53b23e58f..d790e4e4ffb3 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -11,11 +11,16 @@
>  #include <linux/sched.h>
>  #include <linux/libnvdimm.h>
>  #include <linux/platform_device.h>
> +#include <linux/delay.h>
>
>  #include <asm/plpar_wrappers.h>
>
>  #define BIND_ANY_ADDR (~0ul)
>
> +/* Scope args for H_SCM_UNBIND_ALL */
> +#define H_UNBIND_SCOPE_ALL (0x1)
> +#define H_UNBIND_SCOPE_DRC (0x2)
> +
>  #define PAPR_SCM_DIMM_CMD_MASK \
>         ((1ul << ND_CMD_GET_CONFIG_SIZE) | \
>          (1ul << ND_CMD_GET_CONFIG_DATA) | \
> @@ -78,21 +83,43 @@ static int drc_pmem_unbind(struct papr_scm_priv *p)
>  {
>         unsigned long ret[PLPAR_HCALL_BUFSIZE];
>         uint64_t rc, token;
> +       unsigned long starttime;
>
>         token = 0;
>
> -       /* NB: unbind has the same retry requirements mentioned above */
> +       dev_dbg(&p->pdev->dev, "unbind drc %x\n", p->drc_index);
> +
> +       /* NB: unbind_all has the same retry requirements as drc_pmem_bind() */
> +       starttime = HZ;
>         do {
> -               rc = plpar_hcall(H_SCM_UNBIND_MEM, ret, p->drc_index,
> -                               p->bound_addr, p->blocks, token);
> +               /* If this is taking too much time then log warning */
> +               if (jiffies_to_msecs(HZ - starttime) > 1000) {
> +                       dev_warn(&p->pdev->dev,
> +                                "unbind taking > 1s to complete\n");
> +                       starttime = HZ;
> +               }
> +
> +               /* 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;

> +                       token = 0;

The hypervisor should be returning a continue token if the return code
is H_BUSY or any of the H_LONG_BUSY codes. The spec says that if we
get a busy return value then we must use the continue token for the
next unbind, so why are you zeroing it here? If this is required to
make it work then it's a bug in the hypervisor.

> +               } else if (rc == H_BUSY) {
> +                       cond_resched();
> +               }
> +
>         } while (rc == H_BUSY);
>
>         if (rc)
>                 dev_err(&p->pdev->dev, "unbind error: %lld\n", rc);

A dev_dbg() for the unbind time might be nice.

>
> -       return !!rc;
> +       return rc == H_SUCCESS ? 0 : -ENXIO;
>  }
>
>  static int papr_scm_meta_get(struct papr_scm_priv *p,
> --
> 2.21.0
>

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

[Oliver O'Halloran]

On Tue, Jun 25, 2019 at 12:59 AM Vaibhav Jain <vaibhav@linux.ibm.com> wrote:
>
> 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 reset. In
> such cases the H_SCM_BIND_MEM return a H_OVERLAP error.

What is a forced lpar reset? fadump?

> To mitigate such cases the patch updates drc_pmem_bind() to force a
> call to drc_pmem_unbind() in case the initial bind of scm memory fails
> with H_OVERLAP error. In case scm-bind operation again fails after the
> forced scm-unbind then we follow the existing error path.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
>  arch/powerpc/platforms/pseries/papr_scm.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index d790e4e4ffb3..049d7927c0a4 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -44,19 +44,26 @@ struct papr_scm_priv {
>         struct nd_interleave_set nd_set;
>  };

> +/* Forward declaration */
pointless comment.

> +static int drc_pmem_unbind(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 saved;
> +       bool tried_unbind = false;

nit: kernel style uses reverse christmas tree declarations, so this should be:

unsigned long ret[PLPAR_HCALL_BUFSIZE];
bool tried_unbind = false;
uint64_t rc, token;
uint64_t saved;

Come to think of it rc should probably be signed since the hcall
return codes are negative. I'm surprised that's not causing a warning
since we use %lld to print rc rather than %llu.

> +       dev_dbg(&p->pdev->dev, "bind drc %x\n", p->drc_index);
>         /*
>          * When the hypervisor cannot map all the requested memory in a single
>          * hcall it returns H_BUSY and we call again with the token until
>          * we get H_SUCCESS. Aborting the retry loop before getting H_SUCCESS
>          * leave the system in an undefined state, so we wait.
>          */
> +retry:
>         token = 0;
> +       saved = 0;
>
>         do {
>                 rc = plpar_hcall(H_SCM_BIND_MEM, ret, p->drc_index, 0,
> @@ -68,8 +75,18 @@ static int drc_pmem_bind(struct papr_scm_priv *p)
>         } while (rc == H_BUSY);
>
>         if (rc) {
> -               dev_err(&p->pdev->dev, "bind err: %lld\n", rc);
> -               return -ENXIO;

> +               /* retry after unbinding */
> +               if (rc == H_OVERLAP &&  !tried_unbind) {
> +                       dev_warn(&p->pdev->dev, "Un-binding and retrying\n");
> +                       /* Try unbind and ignore any errors */
> +                       tried_unbind = true;
> +                       drc_pmem_unbind(p);
> +                       goto retry;

I think It'd be cleaner if we put the unbind-and-retry logic into the
probe function, e.g:

diff --git a/arch/powerpc/platforms/pseries/papr_scm.c
b/arch/powerpc/platforms/pseries/papr_scm.c
index 96c53b23e58f..d113779fc27c 100644
--- a/arch/powerpc/platforms/pseries/papr_scm.c
+++ b/arch/powerpc/platforms/pseries/papr_scm.c
@@ -316,6 +316,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 (rc == -EBUSY) {
+               /*
+                * If we kexec()ed the previous kernel might have left the DRC
+                * bound in memory. Unbind it and try again.
+                */
+               drc_pmem_unbind(p);
+               rc = drc_pmem_bind(p);
+       }
        if (rc)
                goto err;

Up to you though.