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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 15.07.15 at 11:48, <andrew.cooper3@citrix.com> wrote:
> 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.

But otoh the file/line pair shouldn't cause a lot of confusion - debug
build users can certainly be expected to cope with that. Which isn't
to say that I'd even consider making dprintk() by default not print
file/line, and instead have a dprintk_at() or DPRINTK() doing so for
those who really can't write distinguishable messages.

>>> @@ -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.

I'm of the opposite opinion: Knowing that (just like we do) guest
kernels may access MSRs being prepared to get a #GP, and this
(naturally) being more likely on RDMSR (why would one try to write
an MSR one can't read?), the asymmetry has a reason.

> 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.

Which is nice to know, but not nearly enough to assume we won't get
flooded (ignoring the rate limiting) by these for other guest kernels.

Jan