[v5,0/2] arm64: Fix pending single-step debugging issues

  This patch-set reworks pending fixes from Wei's series [1] to make
single-step debugging via kgdb/kdb on arm64 work as expected. There was
a prior discussion on ML [2] regarding if we should keep the interrupts
enabled during single-stepping. So patch #1 follows suggestion from Will
[3] to not disable interrupts during single stepping but rather skip
single stepping within interrupt handler.

[1] https://lore.kernel.org/all/20200509214159.19680-1-liwei391@huawei.com/
[2] https://lore.kernel.org/all/CAD=FV=Voyfq3Qz0T3RY+aYWYJ0utdH=P_AweB=13rcV8GDBeyQ@mail.gmail.com/
[3] https://lore.kernel.org/all/20200626095551.GA9312@willie-the-truck/

Changes in v5:
- Incorporated misc. comments from Mark.

Changes in v4:
- Rebased to the tip of mainline.
- Picked up Doug's Tested-by tag.

Changes in v3:
- Reword commit descriptions as per Daniel's suggestions.

Changes in v2:
- Replace patch #1 to rather follow Will's suggestion.

Sumit Garg (2):
  arm64: entry: Skip single stepping into interrupt handlers
  arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step

 arch/arm64/include/asm/debug-monitors.h |  1 +
 arch/arm64/kernel/debug-monitors.c      |  5 +++++
 arch/arm64/kernel/entry-common.c        | 22 ++++++++++++++++++++--
 arch/arm64/kernel/kgdb.c                |  2 ++
 4 files changed, 28 insertions(+), 2 deletions(-)
  

Hi Will, Catalin,

On Mon, 19 Dec 2022 at 15:55, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> This patch-set reworks pending fixes from Wei's series [1] to make
> single-step debugging via kgdb/kdb on arm64 work as expected. There was
> a prior discussion on ML [2] regarding if we should keep the interrupts
> enabled during single-stepping. So patch #1 follows suggestion from Will
> [3] to not disable interrupts during single stepping but rather skip
> single stepping within interrupt handler.
>
> [1] https://lore.kernel.org/all/20200509214159.19680-1-liwei391@huawei.com/
> [2] https://lore.kernel.org/all/CAD=FV=Voyfq3Qz0T3RY+aYWYJ0utdH=P_AweB=13rcV8GDBeyQ@mail.gmail.com/
> [3] https://lore.kernel.org/all/20200626095551.GA9312@willie-the-truck/
>
> Changes in v5:
> - Incorporated misc. comments from Mark.
>

Since patch #1 has already been reviewed/acked by Mark and the
complete patchset has been tested by Doug, would it be fine for you to
pick up this patchset? It fixes a real single stepping problem for
kgdb on arm64.

-Sumit

> Changes in v4:
> - Rebased to the tip of mainline.
> - Picked up Doug's Tested-by tag.
>
> Changes in v3:
> - Reword commit descriptions as per Daniel's suggestions.
>
> Changes in v2:
> - Replace patch #1 to rather follow Will's suggestion.
>
> Sumit Garg (2):
>   arm64: entry: Skip single stepping into interrupt handlers
>   arm64: kgdb: Set PSTATE.SS to 1 to re-enable single-step
>
>  arch/arm64/include/asm/debug-monitors.h |  1 +
>  arch/arm64/kernel/debug-monitors.c      |  5 +++++
>  arch/arm64/kernel/entry-common.c        | 22 ++++++++++++++++++++--
>  arch/arm64/kernel/kgdb.c                |  2 ++
>  4 files changed, 28 insertions(+), 2 deletions(-)
>
> --
> 2.34.1
>
  

On Thu, Jan 12, 2023 at 02:52:49PM +0530, Sumit Garg wrote:
> Hi Will, Catalin,
>
> On Mon, 19 Dec 2022 at 15:55, Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > This patch-set reworks pending fixes from Wei's series [1] to make
> > single-step debugging via kgdb/kdb on arm64 work as expected. There was
> > a prior discussion on ML [2] regarding if we should keep the interrupts
> > enabled during single-stepping. So patch #1 follows suggestion from Will
> > [3] to not disable interrupts during single stepping but rather skip
> > single stepping within interrupt handler.
> >
> > [1] https://lore.kernel.org/all/20200509214159.19680-1-liwei391@huawei.com/
> > [2] https://lore.kernel.org/all/CAD=FV=Voyfq3Qz0T3RY+aYWYJ0utdH=P_AweB=13rcV8GDBeyQ@mail.gmail.com/
> > [3] https://lore.kernel.org/all/20200626095551.GA9312@willie-the-truck/
> >
> > Changes in v5:
> > - Incorporated misc. comments from Mark.
> >
>
> Since patch #1 has already been reviewed/acked by Mark and the
> complete patchset has been tested by Doug, would it be fine for you to
> pick up this patchset? It fixes a real single stepping problem for
> kgdb on arm64.

Sorry to be quiet for so long.

Testing this patch set has proven to be a little difficult.

It certainly fixes the single step tests in the kgdbtest suite.
That's a good start.

Unfortunately when testing using qemu/KVM (hosted on NXP
2k/Solidrun Honeycomb) the patch set is resulting in instability
running the built-in self tests (specifically this one:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/kgdbts.c#n74 ). Running this test using the kgdbtest harness
results in the test failing roughly a third of the time.

The error reported is that the trap handler tried to unlock a spinlock
that isn't currently locked. To be honest I suspect this is a generic
problem that the new feature happens to tickle (this test has
historically been unreliable on x86 too... and x86 is noteworthy for
being the only other platform I test using KVM rather than pure qemu).
Of course the only way to prove that would be to find and fix the
problem in the trap handler (which probably involves rewriting it) and I
haven't managed to do that yet.

In short, I think the debugger is more useful with this patchset than
without so, although it is caveated by the above, I'd call this:

Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
Tested-by: Daniel Thompson <daniel.thompson@linaro.org>


Daniel.
  

Hi,

Is this expected to change single-stepping operation in usespace for debuggers (gdb/lldb)? If so, it would be nice to at least
test it a little to make sure it works.

On 1/24/23 18:04, Daniel Thompson wrote:
> On Thu, Jan 12, 2023 at 02:52:49PM +0530, Sumit Garg wrote:
>> Hi Will, Catalin,
>>
>> On Mon, 19 Dec 2022 at 15:55, Sumit Garg <sumit.garg@linaro.org> wrote:
>>>
>>> This patch-set reworks pending fixes from Wei's series [1] to make
>>> single-step debugging via kgdb/kdb on arm64 work as expected. There was
>>> a prior discussion on ML [2] regarding if we should keep the interrupts
>>> enabled during single-stepping. So patch #1 follows suggestion from Will
>>> [3] to not disable interrupts during single stepping but rather skip
>>> single stepping within interrupt handler.
>>>
>>> [1] https://lore.kernel.org/all/20200509214159.19680-1-liwei391@huawei.com/
>>> [2] https://lore.kernel.org/all/CAD=FV=Voyfq3Qz0T3RY+aYWYJ0utdH=P_AweB=13rcV8GDBeyQ@mail.gmail.com/
>>> [3] https://lore.kernel.org/all/20200626095551.GA9312@willie-the-truck/
>>>
>>> Changes in v5:
>>> - Incorporated misc. comments from Mark.
>>>
>>
>> Since patch #1 has already been reviewed/acked by Mark and the
>> complete patchset has been tested by Doug, would it be fine for you to
>> pick up this patchset? It fixes a real single stepping problem for
>> kgdb on arm64.
> 
> Sorry to be quiet for so long.
> 
> Testing this patch set has proven to be a little difficult.
> 
> It certainly fixes the single step tests in the kgdbtest suite.
> That's a good start.
> 
> Unfortunately when testing using qemu/KVM (hosted on NXP
> 2k/Solidrun Honeycomb) the patch set is resulting in instability
> running the built-in self tests (specifically this one:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/kgdbts.c#n74 ). Running this test using the kgdbtest harness
> results in the test failing roughly a third of the time.
> 
> The error reported is that the trap handler tried to unlock a spinlock
> that isn't currently locked. To be honest I suspect this is a generic
> problem that the new feature happens to tickle (this test has
> historically been unreliable on x86 too... and x86 is noteworthy for
> being the only other platform I test using KVM rather than pure qemu).
> Of course the only way to prove that would be to find and fix the
> problem in the trap handler (which probably involves rewriting it) and I
> haven't managed to do that yet.
> 
> In short, I think the debugger is more useful with this patchset than
> without so, although it is caveated by the above, I'd call this:
> 
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
> 
> 
> Daniel.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
  

On Tue, 24 Jan 2023 at 23:34, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> On Thu, Jan 12, 2023 at 02:52:49PM +0530, Sumit Garg wrote:
> > Hi Will, Catalin,
> >
> > On Mon, 19 Dec 2022 at 15:55, Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > This patch-set reworks pending fixes from Wei's series [1] to make
> > > single-step debugging via kgdb/kdb on arm64 work as expected. There was
> > > a prior discussion on ML [2] regarding if we should keep the interrupts
> > > enabled during single-stepping. So patch #1 follows suggestion from Will
> > > [3] to not disable interrupts during single stepping but rather skip
> > > single stepping within interrupt handler.
> > >
> > > [1] https://lore.kernel.org/all/20200509214159.19680-1-liwei391@huawei.com/
> > > [2] https://lore.kernel.org/all/CAD=FV=Voyfq3Qz0T3RY+aYWYJ0utdH=P_AweB=13rcV8GDBeyQ@mail.gmail.com/
> > > [3] https://lore.kernel.org/all/20200626095551.GA9312@willie-the-truck/
> > >
> > > Changes in v5:
> > > - Incorporated misc. comments from Mark.
> > >
> >
> > Since patch #1 has already been reviewed/acked by Mark and the
> > complete patchset has been tested by Doug, would it be fine for you to
> > pick up this patchset? It fixes a real single stepping problem for
> > kgdb on arm64.
>
> Sorry to be quiet for so long.
>
> Testing this patch set has proven to be a little difficult.
>
> It certainly fixes the single step tests in the kgdbtest suite.
> That's a good start.
>
> Unfortunately when testing using qemu/KVM (hosted on NXP
> 2k/Solidrun Honeycomb) the patch set is resulting in instability
> running the built-in self tests (specifically this one:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/kgdbts.c#n74 ). Running this test using the kgdbtest harness
> results in the test failing roughly a third of the time.
>
> The error reported is that the trap handler tried to unlock a spinlock
> that isn't currently locked. To be honest I suspect this is a generic
> problem that the new feature happens to tickle (this test has
> historically been unreliable on x86 too... and x86 is noteworthy for
> being the only other platform I test using KVM rather than pure qemu).
> Of course the only way to prove that would be to find and fix the
> problem in the trap handler (which probably involves rewriting it) and I
> haven't managed to do that yet.
>
> In short, I think the debugger is more useful with this patchset than
> without so, although it is caveated by the above, I'd call this:
>
> Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
>

Thanks Daniel for the in-depth testing.

-Sumit

>
> Daniel.
  

Hi Luis,

On Wed, 25 Jan 2023 at 14:48, Luis Machado <luis.machado@arm.com> wrote:
>
> Hi,
>
> Is this expected to change single-stepping operation in usespace for debuggers (gdb/lldb)?

No it won't affect user-space debuggers as we are only touching the
interrupt path in EL1 mode.

-Sumit

> If so, it would be nice to at least
> test it a little to make sure it works.
>
> On 1/24/23 18:04, Daniel Thompson wrote:
> > On Thu, Jan 12, 2023 at 02:52:49PM +0530, Sumit Garg wrote:
> >> Hi Will, Catalin,
> >>
> >> On Mon, 19 Dec 2022 at 15:55, Sumit Garg <sumit.garg@linaro.org> wrote:
> >>>
> >>> This patch-set reworks pending fixes from Wei's series [1] to make
> >>> single-step debugging via kgdb/kdb on arm64 work as expected. There was
> >>> a prior discussion on ML [2] regarding if we should keep the interrupts
> >>> enabled during single-stepping. So patch #1 follows suggestion from Will
> >>> [3] to not disable interrupts during single stepping but rather skip
> >>> single stepping within interrupt handler.
> >>>
> >>> [1] https://lore.kernel.org/all/20200509214159.19680-1-liwei391@huawei.com/
> >>> [2] https://lore.kernel.org/all/CAD=FV=Voyfq3Qz0T3RY+aYWYJ0utdH=P_AweB=13rcV8GDBeyQ@mail.gmail.com/
> >>> [3] https://lore.kernel.org/all/20200626095551.GA9312@willie-the-truck/
> >>>
> >>> Changes in v5:
> >>> - Incorporated misc. comments from Mark.
> >>>
> >>
> >> Since patch #1 has already been reviewed/acked by Mark and the
> >> complete patchset has been tested by Doug, would it be fine for you to
> >> pick up this patchset? It fixes a real single stepping problem for
> >> kgdb on arm64.
> >
> > Sorry to be quiet for so long.
> >
> > Testing this patch set has proven to be a little difficult.
> >
> > It certainly fixes the single step tests in the kgdbtest suite.
> > That's a good start.
> >
> > Unfortunately when testing using qemu/KVM (hosted on NXP
> > 2k/Solidrun Honeycomb) the patch set is resulting in instability
> > running the built-in self tests (specifically this one:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/kgdbts.c#n74 ). Running this test using the kgdbtest harness
> > results in the test failing roughly a third of the time.
> >
> > The error reported is that the trap handler tried to unlock a spinlock
> > that isn't currently locked. To be honest I suspect this is a generic
> > problem that the new feature happens to tickle (this test has
> > historically been unreliable on x86 too... and x86 is noteworthy for
> > being the only other platform I test using KVM rather than pure qemu).
> > Of course the only way to prove that would be to find and fix the
> > problem in the trap handler (which probably involves rewriting it) and I
> > haven't managed to do that yet.
> >
> > In short, I think the debugger is more useful with this patchset than
> > without so, although it is caveated by the above, I'd call this:
> >
> > Acked-by: Daniel Thompson <daniel.thompson@linaro.org>
> > Tested-by: Daniel Thompson <daniel.thompson@linaro.org>
> >
> >
> > Daniel.
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>