lkml.org 
[lkml]   [2020]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] arm64: cacheflush: Fix KGDB trap detection
On Mon, May 04, 2020 at 09:48:04PM +0100, Will Deacon wrote:
> On Mon, May 04, 2020 at 06:05:18PM +0100, Daniel Thompson wrote:
> > flush_icache_range() contains a bodge to avoid issuing IPIs when the kgdb
> > trap handler is running because issuing IPIs is unsafe (and not needed)
> > in this execution context. However the current test, based on
> > kgdb_connected is flawed: it both over-matches and under-matches.
> >
> > The over match occurs because kgdb_connected is set when gdb attaches
> > to the stub and remains set during normal running. This is relatively
> > harmelss because in almost all cases irq_disabled() will be false.
> >
> > The under match is more serious. When kdb is used instead of kgdb to access
> > the debugger then kgdb_connected is not set in all the places that the
> > debug core updates sw breakpoints (and hence flushes the icache). This
> > can lead to deadlock.
> >
> > Fix by replacing the ad-hoc check with the proper kgdb macro. This also
> > allows us to drop the #ifdef wrapper.
> >
> > Fixes: 3b8c9f1cdfc5 ("arm64: IPI each CPU after invalidating the I-cache for kernel mappings")
> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Notes:
> > v2: Improve the commit message based based on feedback from Doug
> > Anderson
> >
> > arch/arm64/include/asm/cacheflush.h | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/cacheflush.h b/arch/arm64/include/asm/cacheflush.h
> > index e6cca3d4acf7..ce50c1f1f1ea 100644
> > --- a/arch/arm64/include/asm/cacheflush.h
> > +++ b/arch/arm64/include/asm/cacheflush.h
> > @@ -79,7 +79,7 @@ static inline void flush_icache_range(unsigned long start, unsigned long end)
> > * IPI all online CPUs so that they undergo a context synchronization
> > * event and are forced to refetch the new instructions.
> > */
> > -#ifdef CONFIG_KGDB
> > +
> > /*
> > * KGDB performs cache maintenance with interrupts disabled, so we
> > * will deadlock trying to IPI the secondary CPUs. In theory, we can
> > @@ -89,9 +89,9 @@ static inline void flush_icache_range(unsigned long start, unsigned long end)
> > * the patching operation, so we don't need extra IPIs here anyway.
> > * In which case, add a KGDB-specific bodge and return early.
> > */
> > - if (kgdb_connected && irqs_disabled())
> > + if (in_dbg_master())
>
> Does this imply that irqs are disabled?

Yes.

Assuming CONFIG_KGDB is enabled then in_dbg_master() expands to:

(raw_smp_processor_id() == atomic_read(&kgdb_active))

kgdb_active is written to from exactly four locations in the kernel and
all are within a single function, albeit a very big function with control
flow the that could politely be called "quirky". I try not to think about
what it might be impolitely called.

kgdb_active is only ever set to a value other than -1 when we are
executing the kgdb exception handler and interrupts have been
explicitly disabled.


Daniel.

\
 
 \ /
  Last update: 2020-05-05 16:22    [W:0.173 / U:0.544 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site