| Message ID | 20210630214802.1902448-3-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86/mmu: Fast page fault support for the TDP MMU | expand |
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: > > $ 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. > TRACE_EVENT( > fast_page_fault, > TP_PROTO(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u32 error_code, > -- > 2.32.0.93.g670b81a890-goog >
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: > > > > $ 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. Will do in v3. Thanks. > > > TRACE_EVENT( > > fast_page_fault, > > TP_PROTO(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u32 error_code, > > -- > > 2.32.0.93.g670b81a890-goog > >
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?
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.
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); + TRACE_EVENT( fast_page_fault, TP_PROTO(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u32 error_code,
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: $ 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(+)