| Message ID | 20221118150751.469393-4-nathanl@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ed2213bfb192ab51f09f12e9b49b5d482c6493f3 |
| Headers | show |
| Series | RTAS maintenance | expand |
On Fri, 2022-11-18 at 09:07 -0600, Nathan Lynch wrote: > rtas_os_term() is called during panic. Its behavior depends on a > couple of conditions in the /rtas node of the device tree, the > traversal of which entails locking and local IRQ state changes. If > the > kernel panics while devtree_lock is held, rtas_os_term() as currently > written could hang. > > Instead of discovering the relevant characteristics at panic time, > cache them in file-static variables at boot. Note the lookup for > "ibm,extended-os-term" is converted to of_property_read_bool() since > it is a boolean property, not a RTAS function token. > > Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> This seems sensible, minor comment below. Reviewed-by: Andrew Donnellan <ajd@linux.ibm.com> > --- > arch/powerpc/kernel/rtas.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index c12dd5ed5e00..81e4996012b7 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -947,6 +947,8 @@ void __noreturn rtas_halt(void) > > /* Must be in the RMO region, so we place it here */ > static char rtas_os_term_buf[2048]; > +static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE; s32 and int are obviously identical, but rtas_token is declared using int, as are the other variables where we cache various tokens. > +static bool ibm_extended_os_term; > > void rtas_os_term(char *str) > { > @@ -958,14 +960,13 @@ void rtas_os_term(char *str) > * this property may terminate the partition which we want to > avoid > * since it interferes with panic_timeout. > */ > - if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term") || > - RTAS_UNKNOWN_SERVICE == rtas_token("ibm,extended-os- > term")) > + if (ibm_os_term_token == RTAS_UNKNOWN_SERVICE || > !ibm_extended_os_term) > return; > > snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str); > > do { > - status = rtas_call(rtas_token("ibm,os-term"), 1, 1, > NULL, > + status = rtas_call(ibm_os_term_token, 1, 1, NULL, > __pa(rtas_os_term_buf)); > } while (rtas_busy_delay(status)); > > @@ -1335,6 +1336,13 @@ void __init rtas_initialize(void) > no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", > &entry); > rtas.entry = no_entry ? rtas.base : entry; > > + /* > + * Discover these now to avoid device tree lookups in the > + * panic path. > + */ > + ibm_os_term_token = rtas_token("ibm,os-term"); > + ibm_extended_os_term = of_property_read_bool(rtas.dev, > "ibm,extended-os-term"); > + > /* If RTAS was found, allocate the RMO buffer for it and look > for > * the stop-self token if any > */
On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote: > rtas_os_term() is called during panic. Its behavior depends on a > couple of conditions in the /rtas node of the device tree, the > traversal of which entails locking and local IRQ state changes. If the > kernel panics while devtree_lock is held, rtas_os_term() as currently > written could hang. Nice. > > Instead of discovering the relevant characteristics at panic time, > cache them in file-static variables at boot. Note the lookup for > "ibm,extended-os-term" is converted to of_property_read_bool() since > it is a boolean property, not a RTAS function token. Small nit, but you could do that at the query site unless you were going to start using ibm,os-term without the extended capability. Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > > Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> > --- > arch/powerpc/kernel/rtas.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c > index c12dd5ed5e00..81e4996012b7 100644 > --- a/arch/powerpc/kernel/rtas.c > +++ b/arch/powerpc/kernel/rtas.c > @@ -947,6 +947,8 @@ void __noreturn rtas_halt(void) > > /* Must be in the RMO region, so we place it here */ > static char rtas_os_term_buf[2048]; > +static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE; > +static bool ibm_extended_os_term; > > void rtas_os_term(char *str) > { > @@ -958,14 +960,13 @@ void rtas_os_term(char *str) > * this property may terminate the partition which we want to avoid > * since it interferes with panic_timeout. > */ > - if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term") || > - RTAS_UNKNOWN_SERVICE == rtas_token("ibm,extended-os-term")) > + if (ibm_os_term_token == RTAS_UNKNOWN_SERVICE || !ibm_extended_os_term) > return; > > snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str); > > do { > - status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL, > + status = rtas_call(ibm_os_term_token, 1, 1, NULL, > __pa(rtas_os_term_buf)); > } while (rtas_busy_delay(status)); > > @@ -1335,6 +1336,13 @@ void __init rtas_initialize(void) > no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", &entry); > rtas.entry = no_entry ? rtas.base : entry; > > + /* > + * Discover these now to avoid device tree lookups in the > + * panic path. > + */ > + ibm_os_term_token = rtas_token("ibm,os-term"); > + ibm_extended_os_term = of_property_read_bool(rtas.dev, "ibm,extended-os-term"); > + > /* If RTAS was found, allocate the RMO buffer for it and look for > * the stop-self token if any > */ > -- > 2.37.1
Andrew Donnellan <ajd@linux.ibm.com> writes: >> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c >> index c12dd5ed5e00..81e4996012b7 100644 >> --- a/arch/powerpc/kernel/rtas.c >> +++ b/arch/powerpc/kernel/rtas.c >> @@ -947,6 +947,8 @@ void __noreturn rtas_halt(void) >> >> /* Must be in the RMO region, so we place it here */ >> static char rtas_os_term_buf[2048]; >> +static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE; > > s32 and int are obviously identical, but rtas_token is declared using > int, as are the other variables where we cache various tokens. Right... I think it's better practice to use an explicitly sized type where the data is directly derived from the device tree and ultimately passed to the firmware call interface. Gradually enacting this while tolerating some cosmetic inconsistency in the code seems OK to me, but I'm open to other opinions.
"Nicholas Piggin" <npiggin@gmail.com> writes: > On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote: >> rtas_os_term() is called during panic. Its behavior depends on a >> couple of conditions in the /rtas node of the device tree, the >> traversal of which entails locking and local IRQ state changes. If the >> kernel panics while devtree_lock is held, rtas_os_term() as currently >> written could hang. > > Nice. > >> >> Instead of discovering the relevant characteristics at panic time, >> cache them in file-static variables at boot. Note the lookup for >> "ibm,extended-os-term" is converted to of_property_read_bool() since >> it is a boolean property, not a RTAS function token. > > Small nit, but you could do that at the query site unless you > were going to start using ibm,os-term without the extended > capability. I'm unsure that this is what you're suggesting, but we don't want to use of_property_read_bool() in this context either, because it has the same undesirable qualities as rtas_token(). > Reviewed-by: Nicholas Piggin <npiggin@gmail.com> Thanks!
On Tue Nov 29, 2022 at 4:26 AM AEST, Nathan Lynch wrote: > "Nicholas Piggin" <npiggin@gmail.com> writes: > > On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote: > >> rtas_os_term() is called during panic. Its behavior depends on a > >> couple of conditions in the /rtas node of the device tree, the > >> traversal of which entails locking and local IRQ state changes. If the > >> kernel panics while devtree_lock is held, rtas_os_term() as currently > >> written could hang. > > > > Nice. > > > >> > >> Instead of discovering the relevant characteristics at panic time, > >> cache them in file-static variables at boot. Note the lookup for > >> "ibm,extended-os-term" is converted to of_property_read_bool() since > >> it is a boolean property, not a RTAS function token. > > > > Small nit, but you could do that at the query site unless you > > were going to start using ibm,os-term without the extended > > capability. > > I'm unsure that this is what you're suggesting, but we don't want to use > of_property_read_bool() in this context either, because it has the same > undesirable qualities as rtas_token(). I mean rtas_initialize() could do if (of_property_read_bool(rtas.dev, "ibm,extended-os-term")) ibm_os_term_token = rtas_token("ibm,os-term"); Thanks, Nick
"Nicholas Piggin" <npiggin@gmail.com> writes: > On Tue Nov 29, 2022 at 4:26 AM AEST, Nathan Lynch wrote: >> "Nicholas Piggin" <npiggin@gmail.com> writes: >> > On Sat Nov 19, 2022 at 1:07 AM AEST, Nathan Lynch wrote: >> >> rtas_os_term() is called during panic. Its behavior depends on a >> >> couple of conditions in the /rtas node of the device tree, the >> >> traversal of which entails locking and local IRQ state changes. If the >> >> kernel panics while devtree_lock is held, rtas_os_term() as currently >> >> written could hang. >> > >> > Nice. >> > >> >> >> >> Instead of discovering the relevant characteristics at panic time, >> >> cache them in file-static variables at boot. Note the lookup for >> >> "ibm,extended-os-term" is converted to of_property_read_bool() since >> >> it is a boolean property, not a RTAS function token. >> > >> > Small nit, but you could do that at the query site unless you >> > were going to start using ibm,os-term without the extended >> > capability. >> >> I'm unsure that this is what you're suggesting, but we don't want to use >> of_property_read_bool() in this context either, because it has the same >> undesirable qualities as rtas_token(). > > I mean rtas_initialize() could do > > if (of_property_read_bool(rtas.dev, "ibm,extended-os-term")) > ibm_os_term_token = rtas_token("ibm,os-term"); Oh of course, thanks. Since I need to do a v2 anyway, I'll make that change.
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c index c12dd5ed5e00..81e4996012b7 100644 --- a/arch/powerpc/kernel/rtas.c +++ b/arch/powerpc/kernel/rtas.c @@ -947,6 +947,8 @@ void __noreturn rtas_halt(void) /* Must be in the RMO region, so we place it here */ static char rtas_os_term_buf[2048]; +static s32 ibm_os_term_token = RTAS_UNKNOWN_SERVICE; +static bool ibm_extended_os_term; void rtas_os_term(char *str) { @@ -958,14 +960,13 @@ void rtas_os_term(char *str) * this property may terminate the partition which we want to avoid * since it interferes with panic_timeout. */ - if (RTAS_UNKNOWN_SERVICE == rtas_token("ibm,os-term") || - RTAS_UNKNOWN_SERVICE == rtas_token("ibm,extended-os-term")) + if (ibm_os_term_token == RTAS_UNKNOWN_SERVICE || !ibm_extended_os_term) return; snprintf(rtas_os_term_buf, 2048, "OS panic: %s", str); do { - status = rtas_call(rtas_token("ibm,os-term"), 1, 1, NULL, + status = rtas_call(ibm_os_term_token, 1, 1, NULL, __pa(rtas_os_term_buf)); } while (rtas_busy_delay(status)); @@ -1335,6 +1336,13 @@ void __init rtas_initialize(void) no_entry = of_property_read_u32(rtas.dev, "linux,rtas-entry", &entry); rtas.entry = no_entry ? rtas.base : entry; + /* + * Discover these now to avoid device tree lookups in the + * panic path. + */ + ibm_os_term_token = rtas_token("ibm,os-term"); + ibm_extended_os_term = of_property_read_bool(rtas.dev, "ibm,extended-os-term"); + /* If RTAS was found, allocate the RMO buffer for it and look for * the stop-self token if any */
rtas_os_term() is called during panic. Its behavior depends on a couple of conditions in the /rtas node of the device tree, the traversal of which entails locking and local IRQ state changes. If the kernel panics while devtree_lock is held, rtas_os_term() as currently written could hang. Instead of discovering the relevant characteristics at panic time, cache them in file-static variables at boot. Note the lookup for "ibm,extended-os-term" is converted to of_property_read_bool() since it is a boolean property, not a RTAS function token. Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com> --- arch/powerpc/kernel/rtas.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)