Re: [PATCH v2 1/2] x86/hvm/trace: Use a different trace type for AMD processors

2024-05-23 Thread Andrew Cooper
On 23/05/2024 3:10 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 interpret 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.
>
> Remove the `--svm-mode` command-line option, since it's now redundant.
>
> Signed-off-by: George Dunlap 

Acked-by: Andrew Cooper 



[PATCH v2 1/2] x86/hvm/trace: Use a different trace type for AMD processors

2024-05-23 Thread George Dunlap
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 interpret 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.

Remove the `--svm-mode` command-line option, since it's now redundant.

Signed-off-by: George Dunlap 
---
v2:
- Rebase to tip of staging
- Rebase over xentrace_format removal
- Fix typo in commit message
- Remove --svm-mode command-line flag

CC: Andrew Cooper 
CC: Jan Beulich 
CC: Roger Pau Monne 
CC: Anthony Perard 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Olaf Hering 
---
 tools/xentrace/xenalyze.c  | 37 +++--
 xen/arch/x86/hvm/svm/svm.c |  4 ++--
 xen/arch/x86/hvm/vmx/vmx.c |  4 ++--
 xen/include/public/trace.h |  6 --
 4 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/tools/xentrace/xenalyze.c b/tools/xentrace/xenalyze.c
index ce6a85d50b..9c4463b0e8 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 = 
((1ULLexit_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(>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;
@@ -10884,11 +10890,6 @@ const struct argp_option cmd_opts[] =  {
   .arg = "HZ",
   .doc = "Cpu speed of the tracing host, used to convert tsc into 
seconds.", },
 
-{ .name = "svm-mode",
-  .key = OPT_SVM_MODE,
-  .group = OPT_GROUP_HARDWARE,
-  .doc = "Assume AMD SVM-style vmexit error codes.  (Default is Intel 
VMX.)", },
-
 { .name = "progress",
   .key = OPT_PROGRESS,
   .doc = "Progress dialog.  Requires the zenity (GTK+) executable.", },
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 8ba996546f..f16faa6a61 100644
---