On Tue, Jul 06, 2021, Paolo Bonzini wrote:
> On 03/07/21 00:04, isaku.yamahata@xxxxxxxxx wrote:
> > + trace_kvm_tdx_seamcall_enter(smp_processor_id(), op,
> > + rcx, rdx, r8, r9, r10);
> > + err = __seamcall(op, rcx, rdx, r8, r9, r10, ex);
> > + if (ex)
> > + trace_kvm_tdx_seamcall_exit(smp_processor_id(), op, err, ex->rcx,
> > + ex->rdx, ex->r8, ex->r9, ex->r10,
> > + ex->r11);
> > + else
> > + trace_kvm_tdx_seamcall_exit(smp_processor_id(), op, err,
> > + 0, 0, 0, 0, 0, 0);
>
> Would it make sense to do the zeroing of ex directly in __seamcall in case
> there is an error?
A better option would be to pass "ex" into the tracepoint. tdx_arch.h is already
included by trace.h (though I'm not sure that's a good thing), and the cost of
checking ex against NULL over and over is a non-issue because it's buried in the
tracepoint, i.e. hidden behind a patch nop. The below reduces the footprint of
_seamcall by 100+ bytes of code, presumably due to avoiding even more register
shuffling (I didn't look too closely).
That said, I'm not sure adding generic tracepoints is a good idea. The flows
that truly benefit from tracepoints will likely want to provide more/different
information, e.g. the entry/exit flow already uses kvm_trace_entry/exit, and the
SEPT flows have dedicated tracepoints. For flows like tdh_vp_flush(), which
might benefit from a tracepoint, they'll only provide the host PA of the TDVPR,
which is rather useless on its own. It's probably possible to cross-reference
everything to understand what's going on, but it certainly won't be easy.
I can see the generic tracepoint being somewhat useful for debugging early
development and/or a new TDX module, but otherwise I think it will be mostly
overhead. E.g. if a TDX failure pops up in production, enabling the tracepoint
might not even be viable. And even for the cases where the tracepoint is useful,
I would be quite surprised if additional instrumentation wasn't needed to debug
non-trivial issues.
diff --git a/arch/x86/kvm/trace.h b/arch/x86/kvm/trace.h
index 58631124f08d..e2868f6d84f8 100644
--- a/arch/x86/kvm/trace.h
+++ b/arch/x86/kvm/trace.h
@@ -701,9 +701,8 @@ TRACE_EVENT(kvm_tdx_seamcall_enter,
* Tracepoint for the end of TDX SEAMCALLs.
*/
TRACE_EVENT(kvm_tdx_seamcall_exit,
- TP_PROTO(int cpuid, __u64 op, __u64 err, __u64 rcx, __u64 rdx, __u64 r8,
- __u64 r9, __u64 r10, __u64 r11),
- TP_ARGS(cpuid, op, err, rcx, rdx, r8, r9, r10, r11),
+ TP_PROTO(int cpuid, __u64 op, __u64 err, struct tdx_ex_ret *ex),
+ TP_ARGS(cpuid, op, err, ex),
TP_STRUCT__entry(
__field( int, cpuid )
@@ -721,12 +720,12 @@ TRACE_EVENT(kvm_tdx_seamcall_exit,
__entry->cpuid = cpuid;
__entry->op = op;
__entry->err = err;
- __entry->rcx = rcx;
- __entry->rdx = rdx;
- __entry->r8 = r8;
- __entry->r9 = r9;
- __entry->r10 = r10;
- __entry->r11 = r11;
+ __entry->rcx = ex ? ex->rcx : 0;
+ __entry->rdx = ex ? ex->rdx : 0;
+ __entry->r8 = ex ? ex->r8 : 0;
+ __entry->r9 = ex ? ex->r9 : 0;
+ __entry->r10 = ex ? ex->r10 : 0;
+ __entry->r11 = ex ? ex->r11 : 0;
),
TP_printk("cpu: %d op: %s err %s 0x%llx rcx: 0x%llx rdx: 0x%llx r8: 0x%llx r9: 0x%llx r10: 0x%llx r11: 0x%llx",
diff --git a/arch/x86/kvm/vmx/seamcall.h b/arch/x86/kvm/vmx/seamcall.h
index 85eeedc06a4f..b2067f7e6a9d 100644
--- a/arch/x86/kvm/vmx/seamcall.h
+++ b/arch/x86/kvm/vmx/seamcall.h
@@ -23,13 +23,8 @@ static inline u64 _seamcall(u64 op, u64 rcx, u64 rdx, u64 r8, u64 r9, u64 r10,
trace_kvm_tdx_seamcall_enter(smp_processor_id(), op,
rcx, rdx, r8, r9, r10);
err = __seamcall(op, rcx, rdx, r8, r9, r10, ex);
- if (ex)
- trace_kvm_tdx_seamcall_exit(smp_processor_id(), op, err, ex->rcx,
- ex->rdx, ex->r8, ex->r9, ex->r10,
- ex->r11);
- else
- trace_kvm_tdx_seamcall_exit(smp_processor_id(), op, err,
- 0, 0, 0, 0, 0, 0);
+ trace_kvm_tdx_seamcall_exit(smp_processor_id(), op, err, ex);
+
return err;
}
