Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Sangmoon Kim <sangmoon.kim@samsung.com>
To:     broonie@kernel.org
Cc:     0x7f454c46@gmail.com, Dave.Martin@arm.com, amit.kachhap@arm.com,
        catalin.marinas@arm.com, gshan@redhat.com, jordan.lim@samsung.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        mark.rutland@arm.com, mingo@redhat.com, pcc@google.com,
        rostedt@goodmis.org, sangmoon.kim@samsung.com, will@kernel.org
Subject: Re: [PATCH] arm64: traps: add tracepoints for unusal exception
 cases
Date:   Tue, 16 Mar 2021 22:37:20 +0900
Message-Id: <20210316133720.9432-1-sangmoon.kim@samsung.com>
In-Reply-To: <20210308133149.GA4656@sirena.org.uk>
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20210308133149.GA4656@sirena.org.uk>
        <CGME20210316134622epcas1p488fe019ee343dd156dc077c6df9322da@epcas1p4.samsung.com>
Precedence: bulk

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Monday, March 8, 2021 10:32 PM
> Subject: Re: [PATCH] arm64: traps: add tracepoints for unusal exception cases
> 
> On Fri, Mar 05, 2021 at 09:36:30PM +0900, Sangmoon Kim wrote:
> 
> > When kernel panic occurs, a kernel module can use either the
> > panic_notifier or die_notifier to obtain the debugging information.
> 
> > However, in case of these exceptions like do_undefinstr(), regs and
> > esr data are not passed on. Although a module might be able to find
> > those data in the console messages, parsing text messages is very
> > expensive behavior for a module especially on mobile devices.
> 
> > These bare tracepoints allow a module to probe regs and esr information
> > for debugging purpose. _tp suffix comes from bare tracepoints of
> > sched/core.c
> 
> This use case sounds a lot like what the enterprise and Android people
> do via pstore - it seems like it would be better for this to integrate
> via the interfaces that other systems are using for similar purposes and
> then ensure that whatever information is useful is getting passed
> through in a format that makes sense.  That'd be more structured and
> more readily usable by a wider range of systems than something that's
> more of a building block, going via the trace infrastructure seems like
> a bit of an indirection.
> 
> > @@ -832,6 +846,7 @@ void __noreturn arm64_serror_panic(struct pt_regs *regs, u32 esr)
> >  	if (regs)
> >  		__show_regs(regs);
> >
> > +	trace_traps_serror_panic_tp(regs, esr);
> >  	nmi_panic(regs, "Asynchronous SError Interrupt");
> 
> One of the concerns people have with adding tracepoints is that they can
> end up defining ABI so if we *are* going to add any then we need to
> think carefully about how they're defined.  As things currently stand
> they'll pass in the full pt_regs struct which includes not only what's
> defined by the hardware but also additional software defined information
> we store along with it like the stackframe which would be even more of a
> problem if it ends up getting used by someone in a way that ends up as
> ABI.  These are defined as bare tracehooks which does mitigate against
> things ending up getting used in ways that cause problems but people are
> still going to worry about things ending up getting relied on one way or
> another.
> 
> That said it's not clear to me that this will record anything beyond the
> pointer directly in the trace buffer so the value might not be useful
> for terribly long, that itself feels like it might not be as robust an
> interface as it should be.

Dear Mark,

Thank you for your review. I learned a lot about the concerns when using
tracepoint.

Thanks,
Sangmoon