From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/traps: Misc tweaks to several printk()s Date: Wed, 15 Jul 2015 10:48:09 +0100 Message-ID: <55A62C59.2080802@citrix.com> References: <1436896485-25577-1-git-send-email-andrew.cooper3@citrix.com> <55A63E0F0200007800091296@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <55A63E0F0200007800091296@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Xen-devel List-Id: xen-devel@lists.xenproject.org On 15/07/15 10:03, Jan Beulich wrote: >>>> On 14.07.15 at 19:54, wrote: >> @@ -626,8 +626,9 @@ static void do_trap(struct cpu_user_regs *regs, int use_error_code) >> >> if ( likely((fixup = search_exception_table(regs->eip)) != 0) ) >> { >> - dprintk(XENLOG_ERR, "Trap %d: %p -> %p\n", >> - trapnr, _p(regs->eip), _p(fixup)); >> + printk(XENLOG_INFO "Exception [#%d, ec=%04x] (%s): %ps %p -> %p\n", >> + trapnr, use_error_code ? regs->error_code : 0, trapstr(trapnr), >> + _p(regs->eip), _p(regs->eip), _p(fixup)); > But why the transition dprintk() -> printk()? The file/line reference here is not useful, but now that you point it out I had forgotten to consider that dprintk() now only exists in debug builds. It would be nice to have a variant on printk() which is restricted to debug builds, but doesn't have a file/line reference. > >> @@ -2677,9 +2678,9 @@ static int emulate_privileged_op(struct cpu_user_regs *regs) >> >> if ( (rdmsr_safe(regs->ecx, val) != 0) || (msr_content != val) ) >> invalid: >> - gdprintk(XENLOG_WARNING, "Domain attempted WRMSR %p from " >> - "0x%016"PRIx64" to 0x%016"PRIx64".\n", >> - _p(regs->ecx), val, msr_content); >> + gprintk(XENLOG_WARNING, >> + "attempted WRMSR 0x%08x: 0x%016"PRIx64" -> 0x%016"PRIx64"\n", >> + regs->_ecx, val, msr_content); > In cases where the values can't usefully be taken to be decimal I'd > prefer the 0x prefixes to be omitted. > >> @@ -2813,10 +2814,11 @@ static int emulate_privileged_op(struct cpu_user_regs *regs) >> case MSR_EFER: >> rdmsr_normal: >> /* Everyone can read the MSR space. */ >> - /* gdprintk(XENLOG_WARNING,"Domain attempted RDMSR %p.\n", >> - _p(regs->ecx));*/ >> if ( rdmsr_safe(regs->ecx, val) ) >> + { >> + gprintk(XENLOG_WARNING, "attempted RDMSR 0x%08x\n", regs->_ecx); >> goto fail; >> + } > Do you really see this to be useful in production builds? There is currently an asymmetry between the WRMSR and RDMSR paths, which shouldn't exist IMO. Guest warning is rate limited by default, and anecdotally, this path doesn't trigger by default on any of my test boxes with a 3.10 pvops kernel. ~Andrew