[PATCH 0/4] powerpc/xive: add support for H_INT_RESET

[PATCH 0/4] powerpc/xive: add support for H_INT_RESET

[Cédric Le Goater]

H_INT_RESET performs a reset of the Hypervisor internal interrupt
structures, removing all settings done with H_INT_SET_SOURCE_CONFIG
and H_INT_SET_QUEUE_CONFIG. This is most important for kdump and kexec
to be able to restart in a clean state.

First patch closes a window in the kexec sequence in which the
H_INT_RESET hcall can be run concurrently with other hcalls when CPUs
are brought down. The other patches wire up the reset hcall making
sure it supports H_LONG_BUSY_* returned by the Hypervisor.

Cédric Le Goater (4):
  powerpc/64/kexec: fix race in kexec when XIVE is shutdowned
  powerpc/xive: fix hcall H_INT_RESET to support long busy delays
  powerpc/xive: shutdown XIVE when kexec or kdump is performed
  powerpc/xive: prepare all hcalls to support long busy delays

 arch/powerpc/kernel/machine_kexec_64.c |  8 +--
 arch/powerpc/platforms/pseries/kexec.c |  7 ++-
 arch/powerpc/sysdev/xive/spapr.c       | 89 +++++++++++++++++++++++++++++-----
 3 files changed, 85 insertions(+), 19 deletions(-)

-- 
2.13.6

[PATCH 1/4] powerpc/64/kexec: fix race in kexec when XIVE is shutdowned

[Cédric Le Goater]

The kexec_state KEXEC_STATE_IRQS_OFF barrier is reached by all
secondary CPUs before the kexec_cpu_down() operation is called on
secondaries. This can raise conflicts and provoque errors in the XIVE
hcalls when XIVE is shutdowned with H_INT_RESET on the primary CPU.

To synchronize the kexec_cpu_down() operations and make sure the
secondaries have completed their task before the primary starts doing
the same, let's move the primary kexec_cpu_down() after the
KEXEC_STATE_REAL_MODE barrier.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/powerpc/kernel/machine_kexec_64.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel/machine_kexec_64.c
index 49d34d7271e7..212ecb8e829c 100644
--- a/arch/powerpc/kernel/machine_kexec_64.c
+++ b/arch/powerpc/kernel/machine_kexec_64.c
@@ -230,16 +230,16 @@ static void kexec_prepare_cpus(void)
 	/* we are sure every CPU has IRQs off at this point */
 	kexec_all_irq_disabled = 1;
 
-	/* after we tell the others to go down */
-	if (ppc_md.kexec_cpu_down)
-		ppc_md.kexec_cpu_down(0, 0);
-
 	/*
 	 * Before removing MMU mappings make sure all CPUs have entered real
 	 * mode:
 	 */
 	kexec_prepare_cpus_wait(KEXEC_STATE_REAL_MODE);
 
+	/* after we tell the others to go down */
+	if (ppc_md.kexec_cpu_down)
+		ppc_md.kexec_cpu_down(0, 0);
+
 	put_cpu();
 }
 
-- 
2.13.6

[PATCH 2/4] powerpc/xive: fix hcall H_INT_RESET to support long busy delays

[Cédric Le Goater]

The hcall H_INT_RESET can take some time to complete and in such cases
it returns H_LONG_BUSY_* codes requiring the machine to sleep for a
while before retrying.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/powerpc/sysdev/xive/spapr.c | 53 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 48 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
index 091f1d0d0af1..7113f5d87952 100644
--- a/arch/powerpc/sysdev/xive/spapr.c
+++ b/arch/powerpc/sysdev/xive/spapr.c
@@ -19,6 +19,7 @@
 #include <linux/spinlock.h>
 #include <linux/cpumask.h>
 #include <linux/mm.h>
+#include <linux/delay.h>
 
 #include <asm/prom.h>
 #include <asm/io.h>
@@ -108,6 +109,52 @@ static void xive_irq_bitmap_free(int irq)
 	}
 }
 
+
+/* Based on the similar routines in RTAS */
+static unsigned int plpar_busy_delay_time(long rc)
+{
+	unsigned int ms = 0;
+
+	if (H_IS_LONG_BUSY(rc)) {
+		ms = get_longbusy_msecs(rc);
+	} else if (rc == H_BUSY) {
+		ms = 10; /* seems appropriate for XIVE hcalls */
+	}
+
+	return ms;
+}
+
+static unsigned int plpar_busy_delay(int rc)
+{
+	unsigned int ms;
+
+	might_sleep();
+	ms = plpar_busy_delay_time(rc);
+	if (ms && need_resched())
+		msleep(ms);
+
+	return ms;
+}
+
+/*
+ * Note: this call has a partition wide scope and can take a while to
+ * complete. If it returns H_LONG_BUSY_* it should be retried
+ * periodically.
+ */
+static long plpar_int_reset(unsigned long flags)
+{
+	long rc;
+
+	do {
+		rc = plpar_hcall_norets(H_INT_RESET, flags);
+	} while (plpar_busy_delay(rc));
+
+	if (rc)
+		pr_err("H_INT_RESET failed %ld\n", rc);
+
+	return rc;
+}
+
 static long plpar_int_get_source_info(unsigned long flags,
 				      unsigned long lisn,
 				      unsigned long *src_flags,
@@ -445,11 +492,7 @@ static void xive_spapr_put_ipi(unsigned int cpu, struct xive_cpu *xc)
 
 static void xive_spapr_shutdown(void)
 {
-	long rc;
-
-	rc = plpar_hcall_norets(H_INT_RESET, 0);
-	if (rc)
-		pr_err("H_INT_RESET failed %ld\n", rc);
+	plpar_int_reset(0);
 }
 
 /*
-- 
2.13.6

[PATCH 3/4] powerpc/xive: shutdown XIVE when kexec or kdump is performed

[Cédric Le Goater]

The hcall H_INT_RESET should be called to make sure XIVE is fully
reseted.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/powerpc/platforms/pseries/kexec.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/platforms/pseries/kexec.c b/arch/powerpc/platforms/pseries/kexec.c
index eeb13429d685..1d9bbf8e7357 100644
--- a/arch/powerpc/platforms/pseries/kexec.c
+++ b/arch/powerpc/platforms/pseries/kexec.c
@@ -52,8 +52,11 @@ void pseries_kexec_cpu_down(int crash_shutdown, int secondary)
 		}
 	}
 
-	if (xive_enabled())
+	if (xive_enabled()) {
 		xive_kexec_teardown_cpu(secondary);
-	else
+
+		if (!secondary)
+			xive_shutdown();
+	} else
 		xics_kexec_teardown_cpu(secondary);
 }
-- 
2.13.6

[PATCH 4/4] powerpc/xive: prepare all hcalls to support long busy delays

[Cédric Le Goater]

This is not the case for the moment, but future releases of pHyp might
need to introduce some synchronisation routines under the hood which
would make the XIVE hcalls longer to complete.

As this was done for H_INT_RESET, let's wrap the other hcalls in a
loop catching the H_LONG_BUSY_* codes.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 arch/powerpc/sysdev/xive/spapr.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
index 7113f5d87952..97ea0a67a173 100644
--- a/arch/powerpc/sysdev/xive/spapr.c
+++ b/arch/powerpc/sysdev/xive/spapr.c
@@ -165,7 +165,10 @@ static long plpar_int_get_source_info(unsigned long flags,
 	unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
 	long rc;
 
-	rc = plpar_hcall(H_INT_GET_SOURCE_INFO, retbuf, flags, lisn);
+	do {
+		rc = plpar_hcall(H_INT_GET_SOURCE_INFO, retbuf, flags, lisn);
+	} while (plpar_busy_delay(rc));
+
 	if (rc) {
 		pr_err("H_INT_GET_SOURCE_INFO lisn=%ld failed %ld\n", lisn, rc);
 		return rc;
@@ -198,8 +201,11 @@ static long plpar_int_set_source_config(unsigned long flags,
 		flags, lisn, target, prio, sw_irq);
 
 
-	rc = plpar_hcall_norets(H_INT_SET_SOURCE_CONFIG, flags, lisn,
-				target, prio, sw_irq);
+	do {
+		rc = plpar_hcall_norets(H_INT_SET_SOURCE_CONFIG, flags, lisn,
+					target, prio, sw_irq);
+	} while (plpar_busy_delay(rc));
+
 	if (rc) {
 		pr_err("H_INT_SET_SOURCE_CONFIG lisn=%ld target=%lx prio=%lx failed %ld\n",
 		       lisn, target, prio, rc);
@@ -218,7 +224,11 @@ static long plpar_int_get_queue_info(unsigned long flags,
 	unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
 	long rc;
 
-	rc = plpar_hcall(H_INT_GET_QUEUE_INFO, retbuf, flags, target, priority);
+	do {
+		rc = plpar_hcall(H_INT_GET_QUEUE_INFO, retbuf, flags, target,
+				 priority);
+	} while (plpar_busy_delay(rc));
+
 	if (rc) {
 		pr_err("H_INT_GET_QUEUE_INFO cpu=%ld prio=%ld failed %ld\n",
 		       target, priority, rc);
@@ -247,8 +257,11 @@ static long plpar_int_set_queue_config(unsigned long flags,
 	pr_devel("H_INT_SET_QUEUE_CONFIG flags=%lx target=%lx priority=%lx qpage=%lx qsize=%lx\n",
 		flags,  target, priority, qpage, qsize);
 
-	rc = plpar_hcall_norets(H_INT_SET_QUEUE_CONFIG, flags, target,
-				priority, qpage, qsize);
+	do {
+		rc = plpar_hcall_norets(H_INT_SET_QUEUE_CONFIG, flags, target,
+					priority, qpage, qsize);
+	} while (plpar_busy_delay(rc));
+
 	if (rc) {
 		pr_err("H_INT_SET_QUEUE_CONFIG cpu=%ld prio=%ld qpage=%lx returned %ld\n",
 		       target, priority, qpage, rc);
@@ -262,7 +275,10 @@ static long plpar_int_sync(unsigned long flags, unsigned long lisn)
 {
 	long rc;
 
-	rc = plpar_hcall_norets(H_INT_SYNC, flags, lisn);
+	do {
+		rc = plpar_hcall_norets(H_INT_SYNC, flags, lisn);
+	} while (plpar_busy_delay(rc));
+
 	if (rc) {
 		pr_err("H_INT_SYNC lisn=%ld returned %ld\n", lisn, rc);
 		return  rc;
@@ -285,7 +301,11 @@ static long plpar_int_esb(unsigned long flags,
 	pr_devel("H_INT_ESB flags=%lx lisn=%lx offset=%lx in=%lx\n",
 		flags,  lisn, offset, in_data);
 
-	rc = plpar_hcall(H_INT_ESB, retbuf, flags, lisn, offset, in_data);
+	do {
+		rc = plpar_hcall(H_INT_ESB, retbuf, flags, lisn, offset,
+				 in_data);
+	} while (plpar_busy_delay(rc));
+
 	if (rc) {
 		pr_err("H_INT_ESB lisn=%ld offset=%ld returned %ld\n",
 		       lisn, offset, rc);
-- 
2.13.6

Re: [PATCH 0/4] powerpc/xive: add support for H_INT_RESET

[Cédric Le Goater]

On 04/03/2018 09:15 AM, Cédric Le Goater wrote:
> H_INT_RESET performs a reset of the Hypervisor internal interrupt
> structures, removing all settings done with H_INT_SET_SOURCE_CONFIG
> and H_INT_SET_QUEUE_CONFIG. This is most important for kdump and kexec
> to be able to restart in a clean state.
> 
> First patch closes a window in the kexec sequence in which the
> H_INT_RESET hcall can be run concurrently with other hcalls when CPUs
> are brought down. The other patches wire up the reset hcall making
> sure it supports H_LONG_BUSY_* returned by the Hypervisor.

Does anyone has some time to take a look ? 

Thanks,

C. 


> Cédric Le Goater (4):
>   powerpc/64/kexec: fix race in kexec when XIVE is shutdowned
>   powerpc/xive: fix hcall H_INT_RESET to support long busy delays
>   powerpc/xive: shutdown XIVE when kexec or kdump is performed
>   powerpc/xive: prepare all hcalls to support long busy delays
> 
>  arch/powerpc/kernel/machine_kexec_64.c |  8 +--
>  arch/powerpc/platforms/pseries/kexec.c |  7 ++-
>  arch/powerpc/sysdev/xive/spapr.c       | 89 +++++++++++++++++++++++++++++-----
>  3 files changed, 85 insertions(+), 19 deletions(-)
> 

Re: [PATCH 1/4] powerpc/64/kexec: fix race in kexec when XIVE is shutdowned

[Michael Ellerman]

C=C3=A9dric Le Goater <clg@kaod.org> writes:

> The kexec_state KEXEC_STATE_IRQS_OFF barrier is reached by all
> secondary CPUs before the kexec_cpu_down() operation is called on
> secondaries. This can raise conflicts and provoque errors in the XIVE
> hcalls when XIVE is shutdowned with H_INT_RESET on the primary CPU.
>
> To synchronize the kexec_cpu_down() operations and make sure the
> secondaries have completed their task before the primary starts doing
> the same, let's move the primary kexec_cpu_down() after the
> KEXEC_STATE_REAL_MODE barrier.

This sounds reasonable, I'm sure you've tested it. I'm just a bit
worried that it could potentially break on other platforms because it
changes the sequence of operations.

Looking we only have kexec_cpu_down() implemented for pseries, powernv,
ps3 and 85xx.

We can easily test the first two. ps3 doesn't do much so hopefully
that's safe.

mpc85xx_smp_kexec_cpu_down() does very little on 32-bit, and on 64-bit
it seems to already wait for at least one other CPU to get into
KEXEC_STATE_REAL_MODE, so that's probably safe too.

So I guess I'm OK to merge this, and we'll fix any fallout. It would be
good for the change log to call out the change though, and that we think
it's a sensible change for all platforms.

cheers

> Signed-off-by: C=C3=A9dric Le Goater <clg@kaod.org>
> ---
>  arch/powerpc/kernel/machine_kexec_64.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel=
/machine_kexec_64.c
> index 49d34d7271e7..212ecb8e829c 100644
> --- a/arch/powerpc/kernel/machine_kexec_64.c
> +++ b/arch/powerpc/kernel/machine_kexec_64.c
> @@ -230,16 +230,16 @@ static void kexec_prepare_cpus(void)
>  	/* we are sure every CPU has IRQs off at this point */
>  	kexec_all_irq_disabled =3D 1;
>=20=20
> -	/* after we tell the others to go down */
> -	if (ppc_md.kexec_cpu_down)
> -		ppc_md.kexec_cpu_down(0, 0);
> -
>  	/*
>  	 * Before removing MMU mappings make sure all CPUs have entered real
>  	 * mode:
>  	 */
>  	kexec_prepare_cpus_wait(KEXEC_STATE_REAL_MODE);
>=20=20
> +	/* after we tell the others to go down */
> +	if (ppc_md.kexec_cpu_down)
> +		ppc_md.kexec_cpu_down(0, 0);
> +
>  	put_cpu();
>  }
>=20=20
> --=20
> 2.13.6

Re: [PATCH 2/4] powerpc/xive: fix hcall H_INT_RESET to support long busy delays

[Michael Ellerman]

C=C3=A9dric Le Goater <clg@kaod.org> writes:

> diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/=
spapr.c
> index 091f1d0d0af1..7113f5d87952 100644
> --- a/arch/powerpc/sysdev/xive/spapr.c
> +++ b/arch/powerpc/sysdev/xive/spapr.c
> @@ -108,6 +109,52 @@ static void xive_irq_bitmap_free(int irq)
>  	}
>  }
>=20=20
> +
> +/* Based on the similar routines in RTAS */
> +static unsigned int plpar_busy_delay_time(long rc)
> +{
> +	unsigned int ms =3D 0;
> +
> +	if (H_IS_LONG_BUSY(rc)) {
> +		ms =3D get_longbusy_msecs(rc);
> +	} else if (rc =3D=3D H_BUSY) {
> +		ms =3D 10; /* seems appropriate for XIVE hcalls */
> +	}
> +
> +	return ms;
> +}
> +
> +static unsigned int plpar_busy_delay(int rc)
> +{
> +	unsigned int ms;
> +
> +	might_sleep();
> +	ms =3D plpar_busy_delay_time(rc);
> +	if (ms && need_resched())
> +		msleep(ms);

This is called from kexec shutdown isn't it?

In which case I don't think msleep() is a great idea.

We could be crashing for example.

An mdelay would be safer I think?

cheers

Re: [PATCH 4/4] powerpc/xive: prepare all hcalls to support long busy delays

[Michael Ellerman]

C=C3=A9dric Le Goater <clg@kaod.org> writes:

> This is not the case for the moment, but future releases of pHyp might
> need to introduce some synchronisation routines under the hood which
> would make the XIVE hcalls longer to complete.
>
> As this was done for H_INT_RESET, let's wrap the other hcalls in a
> loop catching the H_LONG_BUSY_* codes.

Are we sure it's safe to msleep() in all these paths?

cheers

> diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/=
spapr.c
> index 7113f5d87952..97ea0a67a173 100644
> --- a/arch/powerpc/sysdev/xive/spapr.c
> +++ b/arch/powerpc/sysdev/xive/spapr.c
> @@ -165,7 +165,10 @@ static long plpar_int_get_source_info(unsigned long =
flags,
>  	unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
>  	long rc;
>=20=20
> -	rc =3D plpar_hcall(H_INT_GET_SOURCE_INFO, retbuf, flags, lisn);
> +	do {
> +		rc =3D plpar_hcall(H_INT_GET_SOURCE_INFO, retbuf, flags, lisn);
> +	} while (plpar_busy_delay(rc));
> +
>  	if (rc) {
>  		pr_err("H_INT_GET_SOURCE_INFO lisn=3D%ld failed %ld\n", lisn, rc);
>  		return rc;
> @@ -198,8 +201,11 @@ static long plpar_int_set_source_config(unsigned lon=
g flags,
>  		flags, lisn, target, prio, sw_irq);
>=20=20
>=20=20
> -	rc =3D plpar_hcall_norets(H_INT_SET_SOURCE_CONFIG, flags, lisn,
> -				target, prio, sw_irq);
> +	do {
> +		rc =3D plpar_hcall_norets(H_INT_SET_SOURCE_CONFIG, flags, lisn,
> +					target, prio, sw_irq);
> +	} while (plpar_busy_delay(rc));
> +
>  	if (rc) {
>  		pr_err("H_INT_SET_SOURCE_CONFIG lisn=3D%ld target=3D%lx prio=3D%lx fai=
led %ld\n",
>  		       lisn, target, prio, rc);
> @@ -218,7 +224,11 @@ static long plpar_int_get_queue_info(unsigned long f=
lags,
>  	unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
>  	long rc;
>=20=20
> -	rc =3D plpar_hcall(H_INT_GET_QUEUE_INFO, retbuf, flags, target, priorit=
y);
> +	do {
> +		rc =3D plpar_hcall(H_INT_GET_QUEUE_INFO, retbuf, flags, target,
> +				 priority);
> +	} while (plpar_busy_delay(rc));
> +
>  	if (rc) {
>  		pr_err("H_INT_GET_QUEUE_INFO cpu=3D%ld prio=3D%ld failed %ld\n",
>  		       target, priority, rc);
> @@ -247,8 +257,11 @@ static long plpar_int_set_queue_config(unsigned long=
 flags,
>  	pr_devel("H_INT_SET_QUEUE_CONFIG flags=3D%lx target=3D%lx priority=3D%l=
x qpage=3D%lx qsize=3D%lx\n",
>  		flags,  target, priority, qpage, qsize);
>=20=20
> -	rc =3D plpar_hcall_norets(H_INT_SET_QUEUE_CONFIG, flags, target,
> -				priority, qpage, qsize);
> +	do {
> +		rc =3D plpar_hcall_norets(H_INT_SET_QUEUE_CONFIG, flags, target,
> +					priority, qpage, qsize);
> +	} while (plpar_busy_delay(rc));
> +
>  	if (rc) {
>  		pr_err("H_INT_SET_QUEUE_CONFIG cpu=3D%ld prio=3D%ld qpage=3D%lx return=
ed %ld\n",
>  		       target, priority, qpage, rc);
> @@ -262,7 +275,10 @@ static long plpar_int_sync(unsigned long flags, unsi=
gned long lisn)
>  {
>  	long rc;
>=20=20
> -	rc =3D plpar_hcall_norets(H_INT_SYNC, flags, lisn);
> +	do {
> +		rc =3D plpar_hcall_norets(H_INT_SYNC, flags, lisn);
> +	} while (plpar_busy_delay(rc));
> +
>  	if (rc) {
>  		pr_err("H_INT_SYNC lisn=3D%ld returned %ld\n", lisn, rc);
>  		return  rc;
> @@ -285,7 +301,11 @@ static long plpar_int_esb(unsigned long flags,
>  	pr_devel("H_INT_ESB flags=3D%lx lisn=3D%lx offset=3D%lx in=3D%lx\n",
>  		flags,  lisn, offset, in_data);
>=20=20
> -	rc =3D plpar_hcall(H_INT_ESB, retbuf, flags, lisn, offset, in_data);
> +	do {
> +		rc =3D plpar_hcall(H_INT_ESB, retbuf, flags, lisn, offset,
> +				 in_data);
> +	} while (plpar_busy_delay(rc));
> +
>  	if (rc) {
>  		pr_err("H_INT_ESB lisn=3D%ld offset=3D%ld returned %ld\n",
>  		       lisn, offset, rc);
> --=20
> 2.13.6

Re: [PATCH 3/4] powerpc/xive: shutdown XIVE when kexec or kdump is performed

[Michael Ellerman]

C=C3=A9dric Le Goater <clg@kaod.org> writes:

> The hcall H_INT_RESET should be called to make sure XIVE is fully
> reseted.
>
> Signed-off-by: C=C3=A9dric Le Goater <clg@kaod.org>
> ---
>  arch/powerpc/platforms/pseries/kexec.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/kexec.c b/arch/powerpc/platfo=
rms/pseries/kexec.c
> index eeb13429d685..1d9bbf8e7357 100644
> --- a/arch/powerpc/platforms/pseries/kexec.c
> +++ b/arch/powerpc/platforms/pseries/kexec.c
> @@ -52,8 +52,11 @@ void pseries_kexec_cpu_down(int crash_shutdown, int se=
condary)
>  		}
>  	}
>=20=20
> -	if (xive_enabled())
> +	if (xive_enabled()) {
>  		xive_kexec_teardown_cpu(secondary);
> -	else
> +
> +		if (!secondary)
> +			xive_shutdown();

Couldn't that logic go in xive_kexec_teardown_cpu()?

Why do we not want to do it on powernv?

Actually we *do* do it on powernv, but elsewhere.

cheers

> +	} else
>  		xics_kexec_teardown_cpu(secondary);
>  }
> --=20
> 2.13.6

Re: [PATCH 1/4] powerpc/64/kexec: fix race in kexec when XIVE is shutdowned

[Cédric Le Goater]

On 05/04/2018 12:41 PM, Michael Ellerman wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> The kexec_state KEXEC_STATE_IRQS_OFF barrier is reached by all
>> secondary CPUs before the kexec_cpu_down() operation is called on
>> secondaries. This can raise conflicts and provoque errors in the XIVE
>> hcalls when XIVE is shutdowned with H_INT_RESET on the primary CPU.
>>
>> To synchronize the kexec_cpu_down() operations and make sure the
>> secondaries have completed their task before the primary starts doing
>> the same, let's move the primary kexec_cpu_down() after the
>> KEXEC_STATE_REAL_MODE barrier.
> 
> This sounds reasonable, I'm sure you've tested it. I'm just a bit
> worried that it could potentially break on other platforms because it
> changes the sequence of operations.

yes. We are adding a last barrier to be exact making the full sequence 
a little slower.

> Looking we only have kexec_cpu_down() implemented for pseries, powernv,
> ps3 and 85xx.
> 
> We can easily test the first two. > ps3 doesn't do much so hopefully that's safe.
> 
> mpc85xx_smp_kexec_cpu_down() does very little on 32-bit, and on 64-bit
> it seems to already wait for at least one other CPU to get into
> KEXEC_STATE_REAL_MODE, so that's probably safe too.
> 
> So I guess I'm OK to merge this, and we'll fix any fallout. It would be
> good for the change log to call out the change though, and that we think
> it's a sensible change for all platforms.

OK. 

Thanks,

C. 

 
> cheers
> 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  arch/powerpc/kernel/machine_kexec_64.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/machine_kexec_64.c b/arch/powerpc/kernel/machine_kexec_64.c
>> index 49d34d7271e7..212ecb8e829c 100644
>> --- a/arch/powerpc/kernel/machine_kexec_64.c
>> +++ b/arch/powerpc/kernel/machine_kexec_64.c
>> @@ -230,16 +230,16 @@ static void kexec_prepare_cpus(void)
>>  	/* we are sure every CPU has IRQs off at this point */
>>  	kexec_all_irq_disabled = 1;
>>  
>> -	/* after we tell the others to go down */
>> -	if (ppc_md.kexec_cpu_down)
>> -		ppc_md.kexec_cpu_down(0, 0);
>> -
>>  	/*
>>  	 * Before removing MMU mappings make sure all CPUs have entered real
>>  	 * mode:
>>  	 */
>>  	kexec_prepare_cpus_wait(KEXEC_STATE_REAL_MODE);
>>  
>> +	/* after we tell the others to go down */
>> +	if (ppc_md.kexec_cpu_down)
>> +		ppc_md.kexec_cpu_down(0, 0);
>> +
>>  	put_cpu();
>>  }
>>  
>> -- 
>> 2.13.6

Re: [PATCH 2/4] powerpc/xive: fix hcall H_INT_RESET to support long busy delays

[Cédric Le Goater]

On 05/04/2018 12:41 PM, Michael Ellerman wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
>> index 091f1d0d0af1..7113f5d87952 100644
>> --- a/arch/powerpc/sysdev/xive/spapr.c
>> +++ b/arch/powerpc/sysdev/xive/spapr.c
>> @@ -108,6 +109,52 @@ static void xive_irq_bitmap_free(int irq)
>>  	}
>>  }
>>  
>> +
>> +/* Based on the similar routines in RTAS */
>> +static unsigned int plpar_busy_delay_time(long rc)
>> +{
>> +	unsigned int ms = 0;
>> +
>> +	if (H_IS_LONG_BUSY(rc)) {
>> +		ms = get_longbusy_msecs(rc);
>> +	} else if (rc == H_BUSY) {
>> +		ms = 10; /* seems appropriate for XIVE hcalls */
>> +	}
>> +
>> +	return ms;
>> +}
>> +
>> +static unsigned int plpar_busy_delay(int rc)
>> +{
>> +	unsigned int ms;
>> +
>> +	might_sleep();
>> +	ms = plpar_busy_delay_time(rc);
>> +	if (ms && need_resched())
>> +		msleep(ms);
> 
> This is called from kexec shutdown isn't it?
> 
> In which case I don't think msleep() is a great idea.>> We could be crashing for example.

yes.

> An mdelay would be safer I think?

I agree but we would mdelay be OK ? The hcall can take up to 160ms.

Thanks,

C. 

Re: [PATCH 1/4] powerpc/64/kexec: fix race in kexec when XIVE is shutdowned

[Cédric Le Goater]

On 05/04/2018 01:13 PM, Cédric Le Goater wrote:
> On 05/04/2018 12:41 PM, Michael Ellerman wrote:
>> Cédric Le Goater <clg@kaod.org> writes:
>>
>>> The kexec_state KEXEC_STATE_IRQS_OFF barrier is reached by all
>>> secondary CPUs before the kexec_cpu_down() operation is called on
>>> secondaries. This can raise conflicts and provoque errors in the XIVE
>>> hcalls when XIVE is shutdowned with H_INT_RESET on the primary CPU.
>>>
>>> To synchronize the kexec_cpu_down() operations and make sure the
>>> secondaries have completed their task before the primary starts doing
>>> the same, let's move the primary kexec_cpu_down() after the
>>> KEXEC_STATE_REAL_MODE barrier.
>>
>> This sounds reasonable, I'm sure you've tested it. I'm just a bit
>> worried that it could potentially break on other platforms because it
>> changes the sequence of operations.
> 
> yes. We are adding a last barrier to be exact making the full sequence 
> a little slower.
> 
>> Looking we only have kexec_cpu_down() implemented for pseries, powernv,
>> ps3 and 85xx.
>>
>> We can easily test the first two. > ps3 doesn't do much so hopefully that's safe.
>>
>> mpc85xx_smp_kexec_cpu_down() does very little on 32-bit, and on 64-bit
>> it seems to already wait for at least one other CPU to get into
>> KEXEC_STATE_REAL_MODE, so that's probably safe too.
>>
>> So I guess I'm OK to merge this, and we'll fix any fallout. It would be
>> good for the change log to call out the change though, and that we think
>> it's a sensible change for all platforms.
> 
> OK. 

Ah and can you please fix the 'shutdowned' spelling ? it has been bugging me
since I sent the patch :) thx

C.

Re: [PATCH 3/4] powerpc/xive: shutdown XIVE when kexec or kdump is performed

[Cédric Le Goater]

On 05/04/2018 12:42 PM, Michael Ellerman wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> The hcall H_INT_RESET should be called to make sure XIVE is fully
>> reseted.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  arch/powerpc/platforms/pseries/kexec.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/kexec.c b/arch/powerpc/platforms/pseries/kexec.c
>> index eeb13429d685..1d9bbf8e7357 100644
>> --- a/arch/powerpc/platforms/pseries/kexec.c
>> +++ b/arch/powerpc/platforms/pseries/kexec.c
>> @@ -52,8 +52,11 @@ void pseries_kexec_cpu_down(int crash_shutdown, int secondary)
>>  		}
>>  	}
>>  
>> -	if (xive_enabled())
>> +	if (xive_enabled()) {
>>  		xive_kexec_teardown_cpu(secondary);
>> -	else
>> +
>> +		if (!secondary)
>> +			xive_shutdown();
> 
> Couldn't that logic go in xive_kexec_teardown_cpu()?

On powernv, we wait for the secondaries to reach OPAL before doing a 
XIVE shutdown. This is another kexec barrier but it is after the 
KEXEC_STATE_REAL_MODE barrier if I am correct.

So I don't think we can move the code in the  xive_kexec_teardown_cpu()

> Why do we not want to do it on powernv?
>> Actually we *do* do it on powernv, but elsewhere.

yes in a different file.

Thanks,

C.

> cheers
> 
>> +	} else
>>  		xics_kexec_teardown_cpu(secondary);
>>  }
>> -- 
>> 2.13.6

Re: [PATCH 4/4] powerpc/xive: prepare all hcalls to support long busy delays

[Cédric Le Goater]

On 05/04/2018 12:42 PM, Michael Ellerman wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> This is not the case for the moment, but future releases of pHyp might
>> need to introduce some synchronisation routines under the hood which
>> would make the XIVE hcalls longer to complete.
>>
>> As this was done for H_INT_RESET, let's wrap the other hcalls in a
>> loop catching the H_LONG_BUSY_* codes.
> 
> Are we sure it's safe to msleep() in all these paths?

Hmm. No. we could be under lock as these are called at the bottom of 
the stack of the irq layer. Should we use mdelay() then ?  

What about the rtas_busy_delay() in rtas.c ? I was wondering why we 
were using msleep() there also.

Thanks,

C.

> cheers
> 
>> diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
>> index 7113f5d87952..97ea0a67a173 100644
>> --- a/arch/powerpc/sysdev/xive/spapr.c
>> +++ b/arch/powerpc/sysdev/xive/spapr.c
>> @@ -165,7 +165,10 @@ static long plpar_int_get_source_info(unsigned long flags,
>>  	unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
>>  	long rc;
>>  
>> -	rc = plpar_hcall(H_INT_GET_SOURCE_INFO, retbuf, flags, lisn);
>> +	do {
>> +		rc = plpar_hcall(H_INT_GET_SOURCE_INFO, retbuf, flags, lisn);
>> +	} while (plpar_busy_delay(rc));
>> +
>>  	if (rc) {
>>  		pr_err("H_INT_GET_SOURCE_INFO lisn=%ld failed %ld\n", lisn, rc);
>>  		return rc;
>> @@ -198,8 +201,11 @@ static long plpar_int_set_source_config(unsigned long flags,
>>  		flags, lisn, target, prio, sw_irq);
>>  
>>  
>> -	rc = plpar_hcall_norets(H_INT_SET_SOURCE_CONFIG, flags, lisn,
>> -				target, prio, sw_irq);
>> +	do {
>> +		rc = plpar_hcall_norets(H_INT_SET_SOURCE_CONFIG, flags, lisn,
>> +					target, prio, sw_irq);
>> +	} while (plpar_busy_delay(rc));
>> +
>>  	if (rc) {
>>  		pr_err("H_INT_SET_SOURCE_CONFIG lisn=%ld target=%lx prio=%lx failed %ld\n",
>>  		       lisn, target, prio, rc);
>> @@ -218,7 +224,11 @@ static long plpar_int_get_queue_info(unsigned long flags,
>>  	unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
>>  	long rc;
>>  
>> -	rc = plpar_hcall(H_INT_GET_QUEUE_INFO, retbuf, flags, target, priority);
>> +	do {
>> +		rc = plpar_hcall(H_INT_GET_QUEUE_INFO, retbuf, flags, target,
>> +				 priority);
>> +	} while (plpar_busy_delay(rc));
>> +
>>  	if (rc) {
>>  		pr_err("H_INT_GET_QUEUE_INFO cpu=%ld prio=%ld failed %ld\n",
>>  		       target, priority, rc);
>> @@ -247,8 +257,11 @@ static long plpar_int_set_queue_config(unsigned long flags,
>>  	pr_devel("H_INT_SET_QUEUE_CONFIG flags=%lx target=%lx priority=%lx qpage=%lx qsize=%lx\n",
>>  		flags,  target, priority, qpage, qsize);
>>  
>> -	rc = plpar_hcall_norets(H_INT_SET_QUEUE_CONFIG, flags, target,
>> -				priority, qpage, qsize);
>> +	do {
>> +		rc = plpar_hcall_norets(H_INT_SET_QUEUE_CONFIG, flags, target,
>> +					priority, qpage, qsize);
>> +	} while (plpar_busy_delay(rc));
>> +
>>  	if (rc) {
>>  		pr_err("H_INT_SET_QUEUE_CONFIG cpu=%ld prio=%ld qpage=%lx returned %ld\n",
>>  		       target, priority, qpage, rc);
>> @@ -262,7 +275,10 @@ static long plpar_int_sync(unsigned long flags, unsigned long lisn)
>>  {
>>  	long rc;
>>  
>> -	rc = plpar_hcall_norets(H_INT_SYNC, flags, lisn);
>> +	do {
>> +		rc = plpar_hcall_norets(H_INT_SYNC, flags, lisn);
>> +	} while (plpar_busy_delay(rc));
>> +
>>  	if (rc) {
>>  		pr_err("H_INT_SYNC lisn=%ld returned %ld\n", lisn, rc);
>>  		return  rc;
>> @@ -285,7 +301,11 @@ static long plpar_int_esb(unsigned long flags,
>>  	pr_devel("H_INT_ESB flags=%lx lisn=%lx offset=%lx in=%lx\n",
>>  		flags,  lisn, offset, in_data);
>>  
>> -	rc = plpar_hcall(H_INT_ESB, retbuf, flags, lisn, offset, in_data);
>> +	do {
>> +		rc = plpar_hcall(H_INT_ESB, retbuf, flags, lisn, offset,
>> +				 in_data);
>> +	} while (plpar_busy_delay(rc));
>> +
>>  	if (rc) {
>>  		pr_err("H_INT_ESB lisn=%ld offset=%ld returned %ld\n",
>>  		       lisn, offset, rc);
>> -- 
>> 2.13.6

Re: [PATCH 4/4] powerpc/xive: prepare all hcalls to support long busy delays

[Benjamin Herrenschmidt]

On Fri, 2018-05-04 at 20:42 +1000, Michael Ellerman wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
> > This is not the case for the moment, but future releases of pHyp might
> > need to introduce some synchronisation routines under the hood which
> > would make the XIVE hcalls longer to complete.
> > 
> > As this was done for H_INT_RESET, let's wrap the other hcalls in a
> > loop catching the H_LONG_BUSY_* codes.
> 
> Are we sure it's safe to msleep() in all these paths?

Probably not. We can have the IRQ descriptor lock. We might need to
mdelay.

There's a Kconfig option (forgot which one) that will add checks for
attempts to sleep inside locks, you should run with that.

Cheers,
Ben.

> 
> cheers
> 
> > diff --git a/arch/powerpc/sysdev/xive/spapr.c b/arch/powerpc/sysdev/xive/spapr.c
> > index 7113f5d87952..97ea0a67a173 100644
> > --- a/arch/powerpc/sysdev/xive/spapr.c
> > +++ b/arch/powerpc/sysdev/xive/spapr.c
> > @@ -165,7 +165,10 @@ static long plpar_int_get_source_info(unsigned long flags,
> >  	unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
> >  	long rc;
> >  
> > -	rc = plpar_hcall(H_INT_GET_SOURCE_INFO, retbuf, flags, lisn);
> > +	do {
> > +		rc = plpar_hcall(H_INT_GET_SOURCE_INFO, retbuf, flags, lisn);
> > +	} while (plpar_busy_delay(rc));
> > +
> >  	if (rc) {
> >  		pr_err("H_INT_GET_SOURCE_INFO lisn=%ld failed %ld\n", lisn, rc);
> >  		return rc;
> > @@ -198,8 +201,11 @@ static long plpar_int_set_source_config(unsigned long flags,
> >  		flags, lisn, target, prio, sw_irq);
> >  
> >  
> > -	rc = plpar_hcall_norets(H_INT_SET_SOURCE_CONFIG, flags, lisn,
> > -				target, prio, sw_irq);
> > +	do {
> > +		rc = plpar_hcall_norets(H_INT_SET_SOURCE_CONFIG, flags, lisn,
> > +					target, prio, sw_irq);
> > +	} while (plpar_busy_delay(rc));
> > +
> >  	if (rc) {
> >  		pr_err("H_INT_SET_SOURCE_CONFIG lisn=%ld target=%lx prio=%lx failed %ld\n",
> >  		       lisn, target, prio, rc);
> > @@ -218,7 +224,11 @@ static long plpar_int_get_queue_info(unsigned long flags,
> >  	unsigned long retbuf[PLPAR_HCALL_BUFSIZE];
> >  	long rc;
> >  
> > -	rc = plpar_hcall(H_INT_GET_QUEUE_INFO, retbuf, flags, target, priority);
> > +	do {
> > +		rc = plpar_hcall(H_INT_GET_QUEUE_INFO, retbuf, flags, target,
> > +				 priority);
> > +	} while (plpar_busy_delay(rc));
> > +
> >  	if (rc) {
> >  		pr_err("H_INT_GET_QUEUE_INFO cpu=%ld prio=%ld failed %ld\n",
> >  		       target, priority, rc);
> > @@ -247,8 +257,11 @@ static long plpar_int_set_queue_config(unsigned long flags,
> >  	pr_devel("H_INT_SET_QUEUE_CONFIG flags=%lx target=%lx priority=%lx qpage=%lx qsize=%lx\n",
> >  		flags,  target, priority, qpage, qsize);
> >  
> > -	rc = plpar_hcall_norets(H_INT_SET_QUEUE_CONFIG, flags, target,
> > -				priority, qpage, qsize);
> > +	do {
> > +		rc = plpar_hcall_norets(H_INT_SET_QUEUE_CONFIG, flags, target,
> > +					priority, qpage, qsize);
> > +	} while (plpar_busy_delay(rc));
> > +
> >  	if (rc) {
> >  		pr_err("H_INT_SET_QUEUE_CONFIG cpu=%ld prio=%ld qpage=%lx returned %ld\n",
> >  		       target, priority, qpage, rc);
> > @@ -262,7 +275,10 @@ static long plpar_int_sync(unsigned long flags, unsigned long lisn)
> >  {
> >  	long rc;
> >  
> > -	rc = plpar_hcall_norets(H_INT_SYNC, flags, lisn);
> > +	do {
> > +		rc = plpar_hcall_norets(H_INT_SYNC, flags, lisn);
> > +	} while (plpar_busy_delay(rc));
> > +
> >  	if (rc) {
> >  		pr_err("H_INT_SYNC lisn=%ld returned %ld\n", lisn, rc);
> >  		return  rc;
> > @@ -285,7 +301,11 @@ static long plpar_int_esb(unsigned long flags,
> >  	pr_devel("H_INT_ESB flags=%lx lisn=%lx offset=%lx in=%lx\n",
> >  		flags,  lisn, offset, in_data);
> >  
> > -	rc = plpar_hcall(H_INT_ESB, retbuf, flags, lisn, offset, in_data);
> > +	do {
> > +		rc = plpar_hcall(H_INT_ESB, retbuf, flags, lisn, offset,
> > +				 in_data);
> > +	} while (plpar_busy_delay(rc));
> > +
> >  	if (rc) {
> >  		pr_err("H_INT_ESB lisn=%ld offset=%ld returned %ld\n",
> >  		       lisn, offset, rc);
> > -- 
> > 2.13.6

Re: [PATCH 4/4] powerpc/xive: prepare all hcalls to support long busy delays

[Michael Ellerman]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> On Fri, 2018-05-04 at 20:42 +1000, Michael Ellerman wrote:
>> C=C3=A9dric Le Goater <clg@kaod.org> writes:
>>=20
>> > This is not the case for the moment, but future releases of pHyp might
>> > need to introduce some synchronisation routines under the hood which
>> > would make the XIVE hcalls longer to complete.
>> >=20
>> > As this was done for H_INT_RESET, let's wrap the other hcalls in a
>> > loop catching the H_LONG_BUSY_* codes.
>>=20
>> Are we sure it's safe to msleep() in all these paths?
>
> Probably not. We can have the IRQ descriptor lock. We might need to
> mdelay.
>
> There's a Kconfig option (forgot which one) that will add checks for
> attempts to sleep inside locks, you should run with that.

CONFIG_DEBUG_ATOMIC_SLEEP

cheers

Re: [PATCH 4/4] powerpc/xive: prepare all hcalls to support long busy delays

[Cédric Le Goater]

On 05/07/2018 04:30 AM, Michael Ellerman wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>> On Fri, 2018-05-04 at 20:42 +1000, Michael Ellerman wrote:
>>> Cédric Le Goater <clg@kaod.org> writes:
>>>
>>>> This is not the case for the moment, but future releases of pHyp might
>>>> need to introduce some synchronisation routines under the hood which
>>>> would make the XIVE hcalls longer to complete.
>>>>
>>>> As this was done for H_INT_RESET, let's wrap the other hcalls in a
>>>> loop catching the H_LONG_BUSY_* codes.
>>>
>>> Are we sure it's safe to msleep() in all these paths?
>>
>> Probably not. We can have the IRQ descriptor lock. We might need to
>> mdelay.
>>
>> There's a Kconfig option (forgot which one) that will add checks for
>> attempts to sleep inside locks, you should run with that.
> 
> CONFIG_DEBUG_ATOMIC_SLEEP

[  435.757986] kexec_core: Starting new kernel
[  435.778162] BUG: scheduling while atomic: kexec/1633/0x00000003

That proved to be useful. Thanks.

I will resend the full series changing msleep() to mdelay() and fixing
a bit the changelog.

Cheers,

C.