| Message ID | 20240426143231.4007671-2-george.dunlap@cloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Further trace improvements | expand |
On 26/04/2024 3:32 pm, George Dunlap wrote: > A long-standing usability sub-optimality with xenalyze is the > necessity to specify `--svm-mode` when analyzing AMD processors. This > fundamentally comes about because the same trace event ID is used for > both VMX and SVM, but the contents of the trace must be interpreted > differently. > > Instead, allocate separate trace events for VMX and SVM vmexits in > Xen; this will allow all readers to properly intrepret the meaning of interpret ? > the vmexit reason. > > In xenalyze, first remove the redundant call to init_hvm_data(); > there's no way to get to hvm_vmexit_process() without it being already > initialized by the set_vcpu_type call in hvm_process(). > > Replace this with set_hvm_exit_reson_data(), and move setting of > hvm->exit_reason_* into that function. > > Modify hvm_process and hvm_vmexit_process to handle all four potential > values appropriately. > > If SVM entries are encountered, set opt.svm_mode so that other > SVM-specific functionality is triggered. Given that xenalyze is now closely tied to Xen, and that we're technically changing the ABI here, is there any point keeping `--svm-mode` ? I'm unsure of the utility of reading the buggy trace records from an older version of Xen. > Also add lines in `formats` for xentrace_format. Personally I'd have put patch 3 first, and reduced the churn here. > Signed-off-by: George Dunlap <george.dunlap@cloud.com> > --- > NB that this patch goes on top of Andrew's trace cleanup series: > > https://lore.kernel.org/xen-devel/20240318163552.3808695-1-andrew.cooper3@citrix.com/ The delta in Xen is trivial. I'm happy if you want to commit this, and I can rebase over it. ~Andrew
On Fri, Apr 26, 2024 at 4:18 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 26/04/2024 3:32 pm, George Dunlap wrote: > > A long-standing usability sub-optimality with xenalyze is the > > necessity to specify `--svm-mode` when analyzing AMD processors. This > > fundamentally comes about because the same trace event ID is used for > > both VMX and SVM, but the contents of the trace must be interpreted > > differently. > > > > Instead, allocate separate trace events for VMX and SVM vmexits in > > Xen; this will allow all readers to properly intrepret the meaning of > > interpret ? Oops, yes. > > In xenalyze, first remove the redundant call to init_hvm_data(); > > there's no way to get to hvm_vmexit_process() without it being already > > initialized by the set_vcpu_type call in hvm_process(). > > > > Replace this with set_hvm_exit_reson_data(), and move setting of > > hvm->exit_reason_* into that function. > > > > Modify hvm_process and hvm_vmexit_process to handle all four potential > > values appropriately. > > > > If SVM entries are encountered, set opt.svm_mode so that other > > SVM-specific functionality is triggered. > > Given that xenalyze is now closely tied to Xen, and that we're > technically changing the ABI here, is there any point keeping `--svm-mode` ? > > I'm unsure of the utility of reading the buggy trace records from an > older version of Xen. Yeah, I thought about that. If nobody argues to keep it, I guess I'll rip it out for v2. > > Also add lines in `formats` for xentrace_format. > > Personally I'd have put patch 3 first, and reduced the churn here. I actually meant to add an `RFC` to the third patch. I'd already implemented this by the time you suggested ripping it out. Doing it this way doesn't add much overhead if we do end up ripping out xentrace_format, and does save time if we don't. > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> > > --- > > NB that this patch goes on top of Andrew's trace cleanup series: > > > > https://lore.kernel.org/xen-devel/20240318163552.3808695-1-andrew.cooper3@citrix.com/ > > The delta in Xen is trivial. I'm happy if you want to commit this, and > I can rebase over it. It's trivial in part *because of* your series. Without your series I would have had to do a bunch of munging around with DO_TRC_BLAH_BLAH to make it compile. In fact, I started doing it directly on staging, but quickly moved onto your series to save myself some time. :-) Since I'm planning on re-submitting your series if you don't (unless you really object), this way should minimize churn. I'm happy to post a branch to gitlab if anyone wants to see the combined result. -George
On 26/04/2024 4:29 pm, George Dunlap wrote: > On Fri, Apr 26, 2024 at 4:18 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> On 26/04/2024 3:32 pm, George Dunlap wrote: >>> In xenalyze, first remove the redundant call to init_hvm_data(); >>> there's no way to get to hvm_vmexit_process() without it being already >>> initialized by the set_vcpu_type call in hvm_process(). >>> >>> Replace this with set_hvm_exit_reson_data(), and move setting of >>> hvm->exit_reason_* into that function. >>> >>> Modify hvm_process and hvm_vmexit_process to handle all four potential >>> values appropriately. >>> >>> If SVM entries are encountered, set opt.svm_mode so that other >>> SVM-specific functionality is triggered. >> Given that xenalyze is now closely tied to Xen, and that we're >> technically changing the ABI here, is there any point keeping `--svm-mode` ? >> >> I'm unsure of the utility of reading the buggy trace records from an >> older version of Xen. > Yeah, I thought about that. If nobody argues to keep it, I guess I'll > rip it out for v2. That's the way I'd suggest going. >>> Signed-off-by: George Dunlap <george.dunlap@cloud.com> >>> --- >>> NB that this patch goes on top of Andrew's trace cleanup series: >>> >>> https://lore.kernel.org/xen-devel/20240318163552.3808695-1-andrew.cooper3@citrix.com/ >> The delta in Xen is trivial. I'm happy if you want to commit this, and >> I can rebase over it. > It's trivial in part *because of* your series. Without your series I > would have had to do a bunch of munging around with DO_TRC_BLAH_BLAH > to make it compile. In fact, I started doing it directly on staging, > but quickly moved onto your series to save myself some time. :-) Ah. I'll be refreshing mine next week. Given that it's missed everything since 4.16, I'm not intending to let it miss this one... But I've still got a few things which need to go out before the past-post deadline. ~Andrew
diff --git a/tools/xentrace/formats b/tools/xentrace/formats index afb5ee0112..3381c1cda5 100644 --- a/tools/xentrace/formats +++ b/tools/xentrace/formats @@ -90,8 +90,10 @@ 0x00041002 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) domain_destroy [ dom = 0x%(1)08x ] 0x00081001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) VMENTRY -0x00081002 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) VMEXIT [ exitcode = 0x%(1)08x, rIP = 0x%(2)08x ] -0x00081102 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) VMEXIT [ exitcode = 0x%(1)08x, rIP = 0x%(3)08x%(2)08x ] +0x00081002 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) VMX_EXIT [ exitcode = 0x%(1)08x, rIP = 0x%(2)08x ] +0x00081102 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) VMX_EXIT [ exitcode = 0x%(1)08x, rIP = 0x%(3)08x%(2)08x ] +0x00081003 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) SVM_EXIT [ exitcode = 0x%(1)08x, rIP = 0x%(2)08x ] +0x00081103 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) SVM_EXIT [ exitcode = 0x%(1)08x, rIP = 0x%(3)08x%(2)08x ] 0x00081401 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) nVMENTRY 0x00081402 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) nVMEXIT [ exitcode = 0x%(1)08x, rIP = 0x%(2)08x ] 0x00081502 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) nVMEXIT [ exitcode = 0x%(1)08x, rIP = 0x%(3)08x%(2)08x ] diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c index ce6a85d50b..ceb07229d1 100644 --- a/tools/xentrace/xenalyze.c +++ b/tools/xentrace/xenalyze.c @@ -1437,14 +1437,6 @@ void init_hvm_data(struct hvm_data *h, struct vcpu_data *v) { h->init = 1; - if(opt.svm_mode) { - h->exit_reason_max = HVM_SVM_EXIT_REASON_MAX; - h->exit_reason_name = hvm_svm_exit_reason_name; - } else { - h->exit_reason_max = HVM_VMX_EXIT_REASON_MAX; - h->exit_reason_name = hvm_vmx_exit_reason_name; - } - if(opt.histogram_interrupt_eip) { int count = ((1ULL<<ADDR_SPACE_BITS)/opt.histogram_interrupt_increment); size_t size = count * sizeof(int); @@ -1462,6 +1454,18 @@ void init_hvm_data(struct hvm_data *h, struct vcpu_data *v) { h->summary.guest_interrupt[i].count=0; } +void set_hvm_exit_reason_data(struct hvm_data *h, unsigned event) { + if (((event & ~TRC_64_FLAG) == TRC_HVM_SVM_EXIT) || + opt.svm_mode) { + opt.svm_mode = 1; + h->exit_reason_max = HVM_SVM_EXIT_REASON_MAX; + h->exit_reason_name = hvm_svm_exit_reason_name; + } else { + h->exit_reason_max = HVM_VMX_EXIT_REASON_MAX; + h->exit_reason_name = hvm_vmx_exit_reason_name; + } +} + /* PV data */ enum { PV_HYPERCALL=1, @@ -5088,13 +5092,13 @@ void hvm_vmexit_process(struct record_info *ri, struct hvm_data *h, r = (typeof(r))ri->d; - if(!h->init) - init_hvm_data(h, v); + if(!h->exit_reason_name) + set_hvm_exit_reason_data(h, ri->event); h->vmexit_valid=1; bzero(&h->inflight, sizeof(h->inflight)); - if(ri->event == TRC_HVM_VMEXIT64) { + if(ri->event & TRC_64_FLAG) { if(v->guest_paging_levels != 4) { if ( verbosity >= 6 ) @@ -5316,8 +5320,10 @@ void hvm_process(struct pcpu_info *p) break; default: switch(ri->event) { - case TRC_HVM_VMEXIT: - case TRC_HVM_VMEXIT64: + case TRC_HVM_VMX_EXIT: + case TRC_HVM_VMX_EXIT64: + case TRC_HVM_SVM_EXIT: + case TRC_HVM_SVM_EXIT64: UPDATE_VOLUME(p, hvm[HVM_VOL_VMEXIT], ri->size); hvm_vmexit_process(ri, h, v); break; diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c index db530d55f2..988250dbc1 100644 --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -2571,10 +2571,10 @@ void asmlinkage svm_vmexit_handler(void) exit_reason = vmcb->exitcode; if ( hvm_long_mode_active(v) ) - TRACE_TIME(TRC_HVM_VMEXIT64 | (vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0), + TRACE_TIME(TRC_HVM_SVM_EXIT64 | (vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0), exit_reason, regs->rip, regs->rip >> 32); else - TRACE_TIME(TRC_HVM_VMEXIT | (vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0), + TRACE_TIME(TRC_HVM_SVM_EXIT | (vcpu_guestmode ? TRC_HVM_NESTEDFLAG : 0), exit_reason, regs->eip); if ( vcpu_guestmode ) diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index e5bfb2421e..c2b94e343f 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -4105,9 +4105,9 @@ void asmlinkage vmx_vmexit_handler(struct cpu_user_regs *regs) __vmread(VM_EXIT_REASON, &exit_reason); if ( hvm_long_mode_active(v) ) - TRACE_TIME(TRC_HVM_VMEXIT64, exit_reason, regs->rip, regs->rip >> 32); + TRACE_TIME(TRC_HVM_VMX_EXIT64, exit_reason, regs->rip, regs->rip >> 32); else - TRACE_TIME(TRC_HVM_VMEXIT, exit_reason, regs->eip); + TRACE_TIME(TRC_HVM_VMX_EXIT, exit_reason, regs->eip); perfc_incra(vmexits, (uint16_t)exit_reason); diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h index 3c9f9c3c18..141efa0ea7 100644 --- a/xen/include/public/trace.h +++ b/xen/include/public/trace.h @@ -178,8 +178,10 @@ /* trace events per subclass */ #define TRC_HVM_NESTEDFLAG (0x400) #define TRC_HVM_VMENTRY (TRC_HVM_ENTRYEXIT + 0x01) -#define TRC_HVM_VMEXIT (TRC_HVM_ENTRYEXIT + 0x02) -#define TRC_HVM_VMEXIT64 (TRC_HVM_ENTRYEXIT + TRC_64_FLAG + 0x02) +#define TRC_HVM_VMX_EXIT (TRC_HVM_ENTRYEXIT + 0x02) +#define TRC_HVM_VMX_EXIT64 (TRC_HVM_ENTRYEXIT + TRC_64_FLAG + 0x02) +#define TRC_HVM_SVM_EXIT (TRC_HVM_ENTRYEXIT + 0x03) +#define TRC_HVM_SVM_EXIT64 (TRC_HVM_ENTRYEXIT + TRC_64_FLAG + 0x03) #define TRC_HVM_PF_XEN (TRC_HVM_HANDLER + 0x01) #define TRC_HVM_PF_XEN64 (TRC_HVM_HANDLER + TRC_64_FLAG + 0x01) #define TRC_HVM_PF_INJECT (TRC_HVM_HANDLER + 0x02)
A long-standing usability sub-optimality with xenalyze is the necessity to specify `--svm-mode` when analyzing AMD processors. This fundamentally comes about because the same trace event ID is used for both VMX and SVM, but the contents of the trace must be interpreted differently. Instead, allocate separate trace events for VMX and SVM vmexits in Xen; this will allow all readers to properly intrepret the meaning of the vmexit reason. In xenalyze, first remove the redundant call to init_hvm_data(); there's no way to get to hvm_vmexit_process() without it being already initialized by the set_vcpu_type call in hvm_process(). Replace this with set_hvm_exit_reson_data(), and move setting of hvm->exit_reason_* into that function. Modify hvm_process and hvm_vmexit_process to handle all four potential values appropriately. If SVM entries are encountered, set opt.svm_mode so that other SVM-specific functionality is triggered. Also add lines in `formats` for xentrace_format. Signed-off-by: George Dunlap <george.dunlap@cloud.com> --- NB that this patch goes on top of Andrew's trace cleanup series: https://lore.kernel.org/xen-devel/20240318163552.3808695-1-andrew.cooper3@citrix.com/ CC: Andrew Cooper <andrew.cooper3@citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Roger Pau Monne <roger.pau@cloud.com> CC: Anthony Perard <anthony.perard@cloud.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Olaf Hering <olaf@aepfle.de> --- tools/xentrace/formats | 6 ++++-- tools/xentrace/xenalyze.c | 32 +++++++++++++++++++------------- xen/arch/x86/hvm/svm/svm.c | 4 ++-- xen/arch/x86/hvm/vmx/vmx.c | 4 ++-- xen/include/public/trace.h | 6 ++++-- 5 files changed, 31 insertions(+), 21 deletions(-)