linux-kernel - Re: [RFC PATCH v2 08/69] KVM: TDX: add trace point before/after TDX SEAMCALLs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YO3vQY64R2uoj2ST@google.com>
Date:   Tue, 13 Jul 2021 19:53:37 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     isaku.yamahata@...el.com, Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H . Peter Anvin" <hpa@...or.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, erdemaktas@...gle.com,
        Connor Kuehl <ckuehl@...hat.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        isaku.yamahata@...il.com
Subject: Re: [RFC PATCH v2 08/69] KVM: TDX: add trace point before/after TDX
 SEAMCALLs

On Tue, Jul 06, 2021, Paolo Bonzini wrote:
> On 03/07/21 00:04, isaku.yamahata@...el.com 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;
 }