From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9B86EC28CF6 for ; Fri, 3 Aug 2018 08:45:03 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 58C5C2174D for ; Fri, 3 Aug 2018 08:45:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 58C5C2174D Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ellerman.id.au Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732407AbeHCKkR convert rfc822-to-8bit (ORCPT ); Fri, 3 Aug 2018 06:40:17 -0400 Received: from ozlabs.org ([203.11.71.1]:56655 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728442AbeHCKkR (ORCPT ); Fri, 3 Aug 2018 06:40:17 -0400 Received: from authenticated.ozlabs.org (localhost [127.0.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPSA id 41hgbC6N6Gz9s4V; Fri, 3 Aug 2018 18:44:55 +1000 (AEST) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ellerman.id.au From: Michael Ellerman To: Christophe LEROY , Murilo Opsfelder Araujo Cc: linux-kernel@vger.kernel.org, Alastair D'Silva , Andrew Donnellan , Balbir Singh , Benjamin Herrenschmidt , Cyril Bur , "Eric W . Biederman" , Joe Perches , Michael Neuling , Nicholas Piggin , Paul Mackerras , Simon Guo , Sukadev Bhattiprolu , "Tobin C . Harding" , linuxppc-dev@lists.ozlabs.org, Segher Boessenkool Subject: Re: [PATCH v4 5/6] powerpc: Add show_user_instructions() In-Reply-To: <69cf990b-d4aa-97e7-be3b-7936caa91688@c-s.fr> References: <20180801213320.11203-1-muriloo@linux.ibm.com> <20180801213320.11203-6-muriloo@linux.ibm.com> <7209fa95-8d40-14ca-f27a-ce3edb64191e@c-s.fr> <20180803004201.GA5891@kermit-br-ibm-com> <69cf990b-d4aa-97e7-be3b-7936caa91688@c-s.fr> Date: Fri, 03 Aug 2018 18:44:54 +1000 Message-ID: <87pnyzhm9l.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Christophe LEROY writes: > Le 03/08/2018 à 02:42, Murilo Opsfelder Araujo a écrit : >> Hi, Christophe. >> On Thu, Aug 02, 2018 at 07:26:20AM +0200, Christophe LEROY wrote: >>> Le 01/08/2018 à 23:33, Murilo Opsfelder Araujo a écrit : >>>> show_user_instructions() is a slightly modified version of >>>> show_instructions() that allows userspace instruction dump. >>>> >>>> This will be useful within show_signal_msg() to dump userspace >>>> instructions of the faulty location. >>>> >>>> Here is a sample of what show_user_instructions() outputs: >>>> >>>> pandafault[10850]: code: 4bfffeec 4bfffee8 3c401002 38427f00 fbe1fff8 f821ffc1 7c3f0b78 3d22fffe >>>> pandafault[10850]: code: 392988d0 f93f0020 e93f0020 39400048 <99490000> 39200000 7d234b78 383f0040 >>>> >>>> The current->comm and current->pid printed can serve as a glue that >>>> links the instructions dump to its originator, allowing messages to be >>>> interleaved in the logs. >>>> >>>> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c >>>> index e9533b4d2f08..364645ac732c 100644 >>>> --- a/arch/powerpc/kernel/process.c >>>> +++ b/arch/powerpc/kernel/process.c >>>> @@ -1299,6 +1299,46 @@ static void show_instructions(struct pt_regs *regs) >>>> pr_cont("\n"); >>>> } >>>> +void show_user_instructions(struct pt_regs *regs) >>>> +{ >>>> + int i; >>>> + const char *prefix = KERN_INFO "%s[%d]: code: "; >>>> + unsigned long pc = regs->nip - (instructions_to_print * 3 / 4 * >>>> + sizeof(int)); >>>> + >>>> + printk(prefix, current->comm, current->pid); >>> >>> Why not use pr_info() and remove KERN_INFO from *prefix ? >> >> Because it doesn't compile: >> >> arch/powerpc/kernel/process.c:1317:10: error: expected ‘)’ before ‘prefix’ >> pr_info(prefix, current->comm, current->pid); >> ^ >> ./include/linux/printk.h:288:21: note: in definition of macro ‘pr_fmt’ >> #define pr_fmt(fmt) fmt >> ^ >> >> `pr_info(prefix, ...)` expands to `printk("\001" "6" prefix, ...)`, >> which is an invalid string concatenation. >> >> `pr_info("%s", ...)` expands to `printk("\001" "6" "%s", ...)`, which is >> valid. > > Then what about using directly: > > pr_info("%s[%d]: code: ", ...); Yeah that's better, I'll fix it up when applying. >>>> +#if !defined(CONFIG_BOOKE) >>>> + /* If executing with the IMMU off, adjust pc rather >>>> + * than print XXXXXXXX. >>>> + */ >>>> + if (!(regs->msr & MSR_IR)) >>>> + pc = (unsigned long)phys_to_virt(pc); >>> >>> Shouldn't this be done outside of the loop, only once ? >> >> I don't think so. >> >> pc gets incremented at the bottom of the loop: >> >> pc += sizeof(int); >> >> Adjusting pc is necessary at each iteration. Leaving this block inside >> the loop seems correct. > > This looks pretty strange. > The first time, pc is a physical address, that you change to a virtual > address. Then when you increment it it is still a virtual address. > So when you call phys_to_virt(pc) for the second time, pc is already a > virt address, so what happens indeed ? Yeah that's a bit fishy. On 64-bit it works because phys_to_virt() == __va() which is: #define __va(x) ((void *)(unsigned long)((phys_addr_t)(x) | PAGE_OFFSET)) ie. it uses bitwise or, so __va(__va(x)) == __va(x). But it looks like on 32-bit it's going to do the wrong thing. Do we ever actually hit that case though, I'm not sure? However for this patch I'll just remove the whole thing, because we don't expect to be dumping user instructions in realmode. cheers