Date:   Tue, 24 Jan 2023 18:04:42 +0000
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sumit Garg <sumit.garg@linaro.org>
Cc:     will@kernel.org, catalin.marinas@arm.com, liwei391@huawei.com,
        mhiramat@kernel.org, maz@kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        mark.rutland@arm.com, dianders@chromium.org
Subject: Re: [PATCH v5 0/2] arm64: Fix pending single-step debugging issues
Message-ID: <Y9AdunpawWzWKIXN@aspen.lan>
References: <20221219102452.2860088-1-sumit.garg@linaro.org>
 <CAFA6WYOucB3JdVATbydxYuAb0Q5P7ff_JMb=-BgiF+uip8pdpg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAFA6WYOucB3JdVATbydxYuAb0Q5P7ff_JMb=-BgiF+uip8pdpg@mail.gmail.com>
Precedence: bulk

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.