Re: [PATCH v2 2/6] KVM: x86/mmu: Fix use of enums in trace_fast_page_fault

[David Matlack <dmatlack@google.com>]

On Mon, Jul 12, 2021 at 07:53:55PM +0000, Sean Christopherson wrote:
> On Mon, Jul 12, 2021, David Matlack wrote:
> > On Mon, Jul 12, 2021 at 09:14:01AM -0700, Ben Gardon wrote:
> > > On Wed, Jun 30, 2021 at 2:48 PM David Matlack <dmatlack@google.com> wrote:
> > > >
> > > > Enum values have to be exported to userspace since the formatting is not
> > > > done in the kernel. Without doing this perf maps RET_PF_FIXED and
> > > > RET_PF_SPURIOUS to 0, which results in incorrect output:
> 
> Oof, that's brutal.
> 
> > > >   $ perf record -a -e kvmmmu:fast_page_fault --filter "ret==3" -- ./access_tracking_perf_test
> > > >   $ perf script | head -1
> > > >    [...] new 610006048d25877 spurious 0 fixed 0  <------ should be 1
> > > >
> > > > Fix this by exporting the enum values to userspace with TRACE_DEFINE_ENUM.
> > > >
> > > > Fixes: c4371c2a682e ("KVM: x86/mmu: Return unique RET_PF_* values if the fault was fixed")
> > > > Signed-off-by: David Matlack <dmatlack@google.com>
> > > > ---
> > > >  arch/x86/kvm/mmu/mmutrace.h | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/arch/x86/kvm/mmu/mmutrace.h b/arch/x86/kvm/mmu/mmutrace.h
> > > > index efbad33a0645..55c7e0fcda52 100644
> > > > --- a/arch/x86/kvm/mmu/mmutrace.h
> > > > +++ b/arch/x86/kvm/mmu/mmutrace.h
> > > > @@ -244,6 +244,9 @@ TRACE_EVENT(
> > > >                   __entry->access)
> > > >  );
> > > >
> > > > +TRACE_DEFINE_ENUM(RET_PF_FIXED);
> > > > +TRACE_DEFINE_ENUM(RET_PF_SPURIOUS);
> > > > +
> > > 
> > > If you're planning to send out a v3 anyway, it might be worth adding
> > > all the PF return code enums:
> > > 
> > > enum {
> > > RET_PF_RETRY = 0,
> > > RET_PF_EMULATE,
> > > RET_PF_INVALID,
> > > RET_PF_FIXED,
> > > RET_PF_SPURIOUS,
> > > };
> > > 
> > > Just so that no one has to worry about this in the future.
> 
> Until someone adds a new enum :-/
> 
> > Will do in v3. Thanks.
> 
> What about converting the enums to #defines, with a blurb in the comment
> explaining that the values are arbitrary but aren't enums purely to avoid this
> tracepoint issue?

That will make it possible for someone to accidentally introduce a new
RET_PF symbol with a duplicate value which will result in incorrect
behavior. I am leaning towards keeping it as an enum but adding a
comment that any new enums should be reexported in mmutrace.h.