Re: [PATCH] KVM: x86/mmu: improve robustness of some functions

[Sean Christopherson <seanjc@google.com>]

On Mon, Jan 25, 2021, Paolo Bonzini wrote:
> On 25/01/21 10:54, Vitaly Kuznetsov wrote:
> > 
> > What if we do something like (completely untested):
> > 
> > diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
> > index bfc6389edc28..5ec15e4160b1 100644
> > --- a/arch/x86/kvm/mmu/mmu_internal.h
> > +++ b/arch/x86/kvm/mmu/mmu_internal.h
> > @@ -12,7 +12,7 @@
> >  extern bool dbg;
> >  #define pgprintk(x...) do { if (dbg) printk(x); } while (0)
> > -#define rmap_printk(x...) do { if (dbg) printk(x); } while (0)
> > +#define rmap_printk(fmt, args...) do { if (dbg) printk("%s: " fmt, __func__, ## args); } while (0)
> >  #define MMU_WARN_ON(x) WARN_ON(x)
> >  #else
> >  #define pgprintk(x...) do { } while (0)
> > 
> > and eliminate the need to pass '__func__,' explicitly? We can probably
> > do the same to pgprintk().
> 
> Nice indeed.  Though I wonder if anybody has ever used these.

I've used the ones in pte_list_add() and __pte_list_remove().  I had to add more
info to track down the bug I introduced, but their initial existence was helpful.

That being said, I definitely did not build with MMU_DEBUG defined, I simply
changed a handful of rmap_printks to pr_warn.  Blindly enabling MMU_DEBUG
activates far too much output to be useful.  That may not have been the case
when the core parts of the MMU were under heavy development, but it does feel
like the time has come to excise the bulk of the pgprintk and rmap_printk hooks.
Ditto for mmu_audit.c.

> For those that I actually needed in the past I created tracepoints instead.

Ya.  There are times where I prefer using the kernel log over tracepoints, but
it's easy enough to copy-paste the tracepoint into a pr_* when desired.

I'd be ok with converting a few select rmap_printks to tracepoints, but I vote
to completely remove the bulk of the existing code.  Tracepoints always make me
a bit wary, it's easy to forget/overlook that the inputs to the tracepoint are
still generated even if the tracepoint itself is disabled.  E.g. being too
liberal with tracepoints could theoretically degrade performance.

If we do yank them, I think it makes sense to git rid of mmu_audit.c in the same
commit.  In theory, that would make it easier for someone to restore the hooks
if they need the hooks to debug something in the future.