[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <1589614523.yfijifo1n6.astroid@bobo.none>
Date: Sat, 16 May 2020 17:36:33 +1000
From: Nicholas Piggin <npiggin@...il.com>
To: Allison Randal <allison@...utok.net>,
Thiago Jung Bauermann <bauerman@...ux.ibm.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Daniel Axtens <dja@...ens.net>,
"Gautham R. Shenoy" <ego@...ux.vnet.ibm.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Anshuman Khandual <khandual@...ux.vnet.ibm.com>,
Leonardo Bras <leobras.c@...il.com>,
Michael Ellerman <mpe@...erman.id.au>,
Nadav Amit <namit@...are.com>,
Nathan Lynch <nathanl@...ux.ibm.com>,
Paul Mackerras <paulus@...ba.org>,
Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v5 2/2] powerpc/rtas: Implement reentrant rtas call
Excerpts from Leonardo Bras's message of May 16, 2020 3:21 pm:
> Implement rtas_call_reentrant() for reentrant rtas-calls:
> "ibm,int-on", "ibm,int-off",ibm,get-xive" and "ibm,set-xive".
>
> On LoPAPR Version 1.1 (March 24, 2016), from 7.3.10.1 to 7.3.10.4,
> items 2 and 3 say:
>
> 2 - For the PowerPC External Interrupt option: The * call must be
> reentrant to the number of processors on the platform.
> 3 - For the PowerPC External Interrupt option: The * argument call
> buffer for each simultaneous call must be physically unique.
>
> So, these rtas-calls can be called in a lockless way, if using
> a different buffer for each cpu doing such rtas call.
>
> For this, it was suggested to add the buffer (struct rtas_args)
> in the PACA struct, so each cpu can have it's own buffer.
> The PACA struct received a pointer to rtas buffer, which is
> allocated in the memory range available to rtas 32-bit.
>
> Reentrant rtas calls are useful to avoid deadlocks in crashing,
> where rtas-calls are needed, but some other thread crashed holding
> the rtas.lock.
>
> This is a backtrace of a deadlock from a kdump testing environment:
>
> #0 arch_spin_lock
> #1 lock_rtas ()
> #2 rtas_call (token=8204, nargs=1, nret=1, outputs=0x0)
> #3 ics_rtas_mask_real_irq (hw_irq=4100)
> #4 machine_kexec_mask_interrupts
> #5 default_machine_crash_shutdown
> #6 machine_crash_shutdown
> #7 __crash_kexec
> #8 crash_kexec
> #9 oops_end
>
> Signed-off-by: Leonardo Bras <leobras.c@...il.com>
> ---
> arch/powerpc/include/asm/paca.h | 2 ++
> arch/powerpc/include/asm/rtas.h | 1 +
> arch/powerpc/kernel/paca.c | 20 +++++++++++
> arch/powerpc/kernel/rtas.c | 52 +++++++++++++++++++++++++++++
> arch/powerpc/sysdev/xics/ics-rtas.c | 22 ++++++------
> 5 files changed, 86 insertions(+), 11 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
> index e3cc9eb9204d..87cd9c2220cc 100644
> --- a/arch/powerpc/include/asm/paca.h
> +++ b/arch/powerpc/include/asm/paca.h
> @@ -29,6 +29,7 @@
> #include <asm/hmi.h>
> #include <asm/cpuidle.h>
> #include <asm/atomic.h>
> +#include <asm/rtas-types.h>
>
> #include <asm-generic/mmiowb_types.h>
>
> @@ -270,6 +271,7 @@ struct paca_struct {
> #ifdef CONFIG_MMIOWB
> struct mmiowb_state mmiowb_state;
> #endif
> + struct rtas_args *reentrant_args;
> } ____cacheline_aligned;
>
> extern void copy_mm_to_paca(struct mm_struct *mm);
> diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
> index c35c5350b7e4..fa7509c85881 100644
> --- a/arch/powerpc/include/asm/rtas.h
> +++ b/arch/powerpc/include/asm/rtas.h
> @@ -236,6 +236,7 @@ extern struct rtas_t rtas;
> extern int rtas_token(const char *service);
> extern int rtas_service_present(const char *service);
> extern int rtas_call(int token, int, int, int *, ...);
> +int rtas_call_reentrant(int token, int nargs, int nret, int *outputs, ...);
> void rtas_call_unlocked(struct rtas_args *args, int token, int nargs,
> int nret, ...);
> extern void __noreturn rtas_restart(char *cmd);
> diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
> index 3f91ccaa9c74..88c9b61489fc 100644
> --- a/arch/powerpc/kernel/paca.c
> +++ b/arch/powerpc/kernel/paca.c
> @@ -16,6 +16,7 @@
> #include <asm/kexec.h>
> #include <asm/svm.h>
> #include <asm/ultravisor.h>
> +#include <asm/rtas.h>
>
> #include "setup.h"
>
> @@ -164,6 +165,23 @@ static struct slb_shadow * __init new_slb_shadow(int cpu, unsigned long limit)
>
> #endif /* CONFIG_PPC_BOOK3S_64 */
>
> +/**
> + * new_rtas_args() - Allocates rtas args
> + * @cpu: CPU number
> + * @limit: Memory limit for this allocation
> + *
> + * Allocates a struct rtas_args and return it's pointer.
> + *
> + * Return: Pointer to allocated rtas_args
> + */
> +static struct rtas_args * __init new_rtas_args(int cpu, unsigned long limit)
> +{
> + limit = min_t(unsigned long, limit, RTAS_INSTANTIATE_MAX);
> +
> + return alloc_paca_data(sizeof(struct rtas_args), L1_CACHE_BYTES,
> + limit, cpu);
> +}
> +
> /* The Paca is an array with one entry per processor. Each contains an
> * lppaca, which contains the information shared between the
> * hypervisor and Linux.
> @@ -202,6 +220,7 @@ void __init __nostackprotector initialise_paca(struct paca_struct *new_paca, int
> /* For now -- if we have threads this will be adjusted later */
> new_paca->tcd_ptr = &new_paca->tcd;
> #endif
> + new_paca->reentrant_args = NULL;
> }
>
> /* Put the paca pointer into r13 and SPRG_PACA */
> @@ -274,6 +293,7 @@ void __init allocate_paca(int cpu)
> #ifdef CONFIG_PPC_BOOK3S_64
> paca->slb_shadow_ptr = new_slb_shadow(cpu, limit);
> #endif
> + paca->reentrant_args = new_rtas_args(cpu, limit);
Good, I think tihs should work as you want now. Can you allocate it like
lppacas? Put it under PSERIES (and in the paca) and check for !HV?
Thanks,
Nick
Powered by blists - more mailing lists