Re: [PATCH] x86/traps: Misc tweaks to several printk()s

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/07/15 10:03, Jan Beulich wrote:
>>>> On 14.07.15 at 19:54, <andrew.cooper3@citrix.com> 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