[PATCH 0/5] perfc: assorted adjustments

[PATCH 0/5] perfc: assorted adjustments

[Jan Beulich]

Addressing some observations made while reviewing other patches. I'm
including the last patch here despite it largely duplicating one that
Jürgen did submit - there's one extra adjustment plus an open question
there.

1: perfc: conditionalize credit/credit2 counters
2: x86/perfc: conditionalize HVM and shadow counters
3: VMX: sync VM-exit perf counters with known VM-exit reasons
4: SVM: sync VM-exit perf counters with known VM-exit reasons
5: xenperf: name "newer" hypercalls

Jan

[PATCH 1/5] perfc: conditionalize credit/credit2 counters

[Jan Beulich]

There's no point including them when the respective scheduler isn't
enabled in the build.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/xen/perfc_defn.h
+++ b/xen/include/xen/perfc_defn.h
@@ -37,6 +37,7 @@ PERFCOUNTER(tickled_busy_cpu,       "sch
 PERFCOUNTER(unit_check,             "sched: unit_check")
 
 /* credit specific counters */
+#ifdef CONFIG_SCHED_CREDIT
 PERFCOUNTER(delay_ms,               "csched: delay")
 PERFCOUNTER(acct_run,               "csched: acct_run")
 PERFCOUNTER(acct_no_work,           "csched: acct_no_work")
@@ -58,8 +59,10 @@ PERFCOUNTER(migrate_queued,         "csc
 PERFCOUNTER(migrate_running,        "csched: migrate_running")
 PERFCOUNTER(migrate_kicked_away,    "csched: migrate_kicked_away")
 PERFCOUNTER(unit_hot,               "csched: unit_hot")
+#endif
 
 /* credit2 specific counters */
+#ifdef CONFIG_SCHED_CREDIT2
 PERFCOUNTER(burn_credits_t2c,       "csched2: burn_credits_t2c")
 PERFCOUNTER(acct_load_balance,      "csched2: acct_load_balance")
 PERFCOUNTER(upd_max_weight_quick,   "csched2: update_max_weight_quick")
@@ -77,6 +80,7 @@ PERFCOUNTER(credit_reset,           "csc
 PERFCOUNTER(deferred_to_tickled_cpu,"csched2: deferred_to_tickled_cpu")
 PERFCOUNTER(tickled_cpu_overwritten,"csched2: tickled_cpu_overwritten")
 PERFCOUNTER(tickled_cpu_overridden, "csched2: tickled_cpu_overridden")
+#endif
 
 PERFCOUNTER(need_flush_tlb_flush,   "PG_need_flush tlb flushes")
 

Re: [PATCH 1/5] perfc: conditionalize credit/credit2 counters

[Luca Fancellu]

> On 3 Dec 2021, at 12:03, Jan Beulich <jbeulich@suse.com> wrote:
> 
> There's no point including them when the respective scheduler isn't
> enabled in the build.
> 

Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/include/xen/perfc_defn.h
> +++ b/xen/include/xen/perfc_defn.h
> @@ -37,6 +37,7 @@ PERFCOUNTER(tickled_busy_cpu,       "sch
> PERFCOUNTER(unit_check,             "sched: unit_check")
> 
> /* credit specific counters */
> +#ifdef CONFIG_SCHED_CREDIT
> PERFCOUNTER(delay_ms,               "csched: delay")
> PERFCOUNTER(acct_run,               "csched: acct_run")
> PERFCOUNTER(acct_no_work,           "csched: acct_no_work")
> @@ -58,8 +59,10 @@ PERFCOUNTER(migrate_queued,         "csc
> PERFCOUNTER(migrate_running,        "csched: migrate_running")
> PERFCOUNTER(migrate_kicked_away,    "csched: migrate_kicked_away")
> PERFCOUNTER(unit_hot,               "csched: unit_hot")
> +#endif
> 
> /* credit2 specific counters */
> +#ifdef CONFIG_SCHED_CREDIT2
> PERFCOUNTER(burn_credits_t2c,       "csched2: burn_credits_t2c")
> PERFCOUNTER(acct_load_balance,      "csched2: acct_load_balance")
> PERFCOUNTER(upd_max_weight_quick,   "csched2: update_max_weight_quick")
> @@ -77,6 +80,7 @@ PERFCOUNTER(credit_reset,           "csc
> PERFCOUNTER(deferred_to_tickled_cpu,"csched2: deferred_to_tickled_cpu")
> PERFCOUNTER(tickled_cpu_overwritten,"csched2: tickled_cpu_overwritten")
> PERFCOUNTER(tickled_cpu_overridden, "csched2: tickled_cpu_overridden")
> +#endif
> 
> PERFCOUNTER(need_flush_tlb_flush,   "PG_need_flush tlb flushes")
> 
> 
> 

[PATCH 2/5] x86/perfc: conditionalize HVM and shadow counters

[Jan Beulich]

There's no point including them when the respective functionality isn't
enabled in the build. Note that this covers only larger groups; more
fine grained exclusion may want to be done later on.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/asm-x86/perfc_defn.h
+++ b/xen/include/asm-x86/perfc_defn.h
@@ -4,6 +4,8 @@
 
 PERFCOUNTER_ARRAY(exceptions,           "exceptions", 32)
 
+#ifdef CONFIG_HVM
+
 #define VMX_PERF_EXIT_REASON_SIZE 56
 #define VMX_PERF_VECTOR_SIZE 0x20
 PERFCOUNTER_ARRAY(vmexits,              "vmexits", VMX_PERF_EXIT_REASON_SIZE)
@@ -13,6 +15,8 @@ PERFCOUNTER_ARRAY(cause_vector,
 #define SVM_PERF_EXIT_REASON_SIZE (1+141)
 PERFCOUNTER_ARRAY(svmexits,             "SVMexits", SVM_PERF_EXIT_REASON_SIZE)
 
+#endif /* CONFIG_HVM */
+
 PERFCOUNTER(seg_fixups,             "segmentation fixups")
 
 PERFCOUNTER(apic_timer,             "apic timer interrupts")
@@ -37,6 +41,8 @@ PERFCOUNTER(exception_fixed,        "pre
 PERFCOUNTER(guest_walk,            "guest pagetable walks")
 
 /* Shadow counters */
+#ifdef CONFIG_SHADOW_PAGING
+
 PERFCOUNTER(shadow_alloc,          "calls to shadow_alloc")
 PERFCOUNTER(shadow_alloc_tlbflush, "shadow_alloc flushed TLBs")
 
@@ -112,6 +118,8 @@ PERFCOUNTER(shadow_unsync,         "shad
 PERFCOUNTER(shadow_unsync_evict,   "shadow OOS evictions")
 PERFCOUNTER(shadow_resync,         "shadow OOS resyncs")
 
+#endif /* CONFIG_SHADOW_PAGING */
+
 PERFCOUNTER(realmode_emulations, "realmode instructions emulated")
 PERFCOUNTER(realmode_exits,      "vmexits from realmode")
 

Re: [PATCH 2/5] x86/perfc: conditionalize HVM and shadow counters

[Andrew Cooper]

On 03/12/2021 12:04, Jan Beulich wrote:
> There's no point including them when the respective functionality isn't
> enabled in the build. Note that this covers only larger groups; more
> fine grained exclusion may want to be done later on.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH 3/5] VMX: sync VM-exit perf counters with known VM-exit reasons

[Jan Beulich]

This has gone out of sync over time. Introduce a simplistic mechanism to
hopefully keep things in sync going forward.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I wasn't sure about the #ifdef: Using CONFIG_PERF_COUNTERS there would
seem slightly odd next to a construct which specifically abstracts away
this aspect.

--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3869,7 +3869,10 @@ void vmx_vmexit_handler(struct cpu_user_
     else
         HVMTRACE_ND(VMEXIT, 0, 1/*cycles*/, exit_reason, regs->eip);
 
-    perfc_incra(vmexits, exit_reason);
+#ifdef VMX_PERF_EXIT_REASON_SIZE
+    BUILD_BUG_ON(VMX_PERF_EXIT_REASON_SIZE != EXIT_REASON_LAST + 1);
+#endif
+    perfc_incra(vmexits, (uint16_t)exit_reason);
 
     /* Handle the interrupt we missed before allowing any more in. */
     switch ( (uint16_t)exit_reason )
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -220,6 +220,8 @@ static inline void pi_clear_sn(struct pi
 #define EXIT_REASON_XSAVES              63
 #define EXIT_REASON_XRSTORS             64
 
+#define EXIT_REASON_LAST                EXIT_REASON_XRSTORS
+
 /*
  * Interruption-information format
  */
--- a/xen/include/asm-x86/perfc_defn.h
+++ b/xen/include/asm-x86/perfc_defn.h
@@ -6,7 +6,7 @@ PERFCOUNTER_ARRAY(exceptions,
 
 #ifdef CONFIG_HVM
 
-#define VMX_PERF_EXIT_REASON_SIZE 56
+#define VMX_PERF_EXIT_REASON_SIZE 65
 #define VMX_PERF_VECTOR_SIZE 0x20
 PERFCOUNTER_ARRAY(vmexits,              "vmexits", VMX_PERF_EXIT_REASON_SIZE)
 PERFCOUNTER_ARRAY(cause_vector,         "cause vector", VMX_PERF_VECTOR_SIZE)

Re: [PATCH 3/5] VMX: sync VM-exit perf counters with known VM-exit reasons

[Andrew Cooper]

On 03/12/2021 12:05, Jan Beulich wrote:
> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
> @@ -220,6 +220,8 @@ static inline void pi_clear_sn(struct pi
>  #define EXIT_REASON_XSAVES              63
>  #define EXIT_REASON_XRSTORS             64
>  
> +#define EXIT_REASON_LAST                EXIT_REASON_XRSTORS
> +

Given the problems with sentinals like this in the domctl/sysctl
headers, I think this scheme would be less fragile if EXIT_REASON was an
enum.  (Also, we seriously need to reduce the scope of these headers. 
It's only just dawned on me why Intel uses EXIT_* and AMD uses VMEXIT_*)

Alternatively, what about simply this:

 #define EXIT_REASON_XSAVES              63
 #define EXIT_REASON_XRSTORS             64
+/* Remember to update VMX_PERF_EXIT_REASON_SIZE too. */

?

This avoids having yet another sentinel in the mix, and will be visible
in *every* patch review.  It also gets rid of the ifdefary in the vmexit
handler.

Another good move might be to have perfc_incra() have a printk_once()
for out-of-range indices.  That way, people using perfcounters will have
an easier time noticing if something is wrong.

~Andrew

Re: [PATCH 3/5] VMX: sync VM-exit perf counters with known VM-exit reasons

[Jan Beulich]

On 17.12.2021 16:00, Andrew Cooper wrote:
> On 03/12/2021 12:05, Jan Beulich wrote:
>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>> @@ -220,6 +220,8 @@ static inline void pi_clear_sn(struct pi
>>  #define EXIT_REASON_XSAVES              63
>>  #define EXIT_REASON_XRSTORS             64
>>  
>> +#define EXIT_REASON_LAST                EXIT_REASON_XRSTORS
>> +
> 
> Given the problems with sentinals like this in the domctl/sysctl
> headers, I think this scheme would be less fragile if EXIT_REASON was an
> enum.

Enums have some "downsides", so I'm not sure I'd want to go that route,
at least not right here.

>  (Also, we seriously need to reduce the scope of these headers. 
> It's only just dawned on me why Intel uses EXIT_* and AMD uses VMEXIT_*)

Funny, isn't it? Otoh the PM specifically uses VMEXIT_*, while the SDM
specifically talks about "exit reason" everywhere. So there may be more
to this than just the need to avoid name space collisions.

And yes, I fully agree with the need of scope reduction. These
definitions living in a private header in xen/arch/x86/hvm/vmx/ should
be completely sufficient for the build to continue to work. Question
is if, while doing so, we wouldn't want to alter the name prefixes (but
see above).

> Alternatively, what about simply this:
> 
>  #define EXIT_REASON_XSAVES              63
>  #define EXIT_REASON_XRSTORS             64
> +/* Remember to update VMX_PERF_EXIT_REASON_SIZE too. */
> 
> ?
> 
> This avoids having yet another sentinel in the mix, and will be visible
> in *every* patch review.  It also gets rid of the ifdefary in the vmexit
> handler.

I can do it that way, sure, but then there'll again be no build time
check. As long as exit reasons all get added sequentially here, the
comment in context should raise enough attention, but what if Intel
start a second range like AMD did with NPF?

Jan

[PATCH 4/5] SVM: sync VM-exit perf counters with known VM-exit reasons

[Jan Beulich]

This has gone out of sync over time, resulting in NPF and XSETBV exits
incrementing the same counter. Introduce a simplistic mechanism to
hopefully keep things in better sync going forward.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Given their large (and growing) number, I wonder whether we shouldn't
fold "SVMexits" and "vmexits". They can't both be active at the same
time.

I wasn't sure about the #ifdef: Using CONFIG_PERF_COUNTERS there would
seem slightly odd next to a construct which specifically abstracts away
this aspect.

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2644,6 +2644,10 @@ void svm_vmexit_handler(struct cpu_user_
         goto out;
     }
 
+    /* Note: "+2" to account for VMEXIT_NPF_PERFC. */
+#ifdef SVM_PERF_EXIT_REASON_SIZE
+    BUILD_BUG_ON(SVM_PERF_EXIT_REASON_SIZE != VMEXIT_LAST + 2);
+#endif
     perfc_incra(svmexits, exit_reason);
 
     hvm_maybe_deassert_evtchn_irq();
--- a/xen/include/asm-x86/hvm/svm/vmcb.h
+++ b/xen/include/asm-x86/hvm/svm/vmcb.h
@@ -302,6 +302,7 @@ enum VMEXIT_EXITCODE
     VMEXIT_MWAIT_CONDITIONAL= 140, /* 0x8c */
     VMEXIT_XSETBV           = 141, /* 0x8d */
     VMEXIT_RDPRU            = 142, /* 0x8e */
+#define VMEXIT_LAST VMEXIT_RDPRU
     VMEXIT_NPF              = 1024, /* 0x400, nested paging fault */
     VMEXIT_INVALID          =  -1
 };
--- a/xen/include/asm-x86/perfc_defn.h
+++ b/xen/include/asm-x86/perfc_defn.h
@@ -11,8 +11,8 @@ PERFCOUNTER_ARRAY(exceptions,
 PERFCOUNTER_ARRAY(vmexits,              "vmexits", VMX_PERF_EXIT_REASON_SIZE)
 PERFCOUNTER_ARRAY(cause_vector,         "cause vector", VMX_PERF_VECTOR_SIZE)
 
-#define VMEXIT_NPF_PERFC 141
-#define SVM_PERF_EXIT_REASON_SIZE (1+141)
+#define VMEXIT_NPF_PERFC 143
+#define SVM_PERF_EXIT_REASON_SIZE (VMEXIT_NPF_PERFC + 1)
 PERFCOUNTER_ARRAY(svmexits,             "SVMexits", SVM_PERF_EXIT_REASON_SIZE)
 
 #endif /* CONFIG_HVM */

Re: [PATCH 4/5] SVM: sync VM-exit perf counters with known VM-exit reasons

[Andrew Cooper]

On 03/12/2021 12:06, Jan Beulich wrote:
> This has gone out of sync over time, resulting in NPF and XSETBV exits
> incrementing the same counter. Introduce a simplistic mechanism to
> hopefully keep things in better sync going forward.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Given their large (and growing) number, I wonder whether we shouldn't
> fold "SVMexits" and "vmexits". They can't both be active at the same
> time.

Oh yeah - that's just silly having them split like this, especially as
there's no associated element name.

> --- a/xen/include/asm-x86/perfc_defn.h
> +++ b/xen/include/asm-x86/perfc_defn.h
> @@ -11,8 +11,8 @@ PERFCOUNTER_ARRAY(exceptions,
>  PERFCOUNTER_ARRAY(vmexits,              "vmexits", VMX_PERF_EXIT_REASON_SIZE)
>  PERFCOUNTER_ARRAY(cause_vector,         "cause vector", VMX_PERF_VECTOR_SIZE)
>  
> -#define VMEXIT_NPF_PERFC 141
> -#define SVM_PERF_EXIT_REASON_SIZE (1+141)
> +#define VMEXIT_NPF_PERFC 143
> +#define SVM_PERF_EXIT_REASON_SIZE (VMEXIT_NPF_PERFC + 1)

How does this work in the first place?  perfc_incra() is still passed 1024.

Furthermore, it's already worse than this.

401/402 are AVIC exits, and 403 is an SEV exit.  We've also gained -2 as
"busy" for transient SEV events too.

~Andrew

Re: [PATCH 4/5] SVM: sync VM-exit perf counters with known VM-exit reasons

[Jan Beulich]

On 17.12.2021 16:02, Andrew Cooper wrote:
> On 03/12/2021 12:06, Jan Beulich wrote:
>> This has gone out of sync over time, resulting in NPF and XSETBV exits
>> incrementing the same counter. Introduce a simplistic mechanism to
>> hopefully keep things in better sync going forward.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Given their large (and growing) number, I wonder whether we shouldn't
>> fold "SVMexits" and "vmexits". They can't both be active at the same
>> time.
> 
> Oh yeah - that's just silly having them split like this, especially as
> there's no associated element name.

Okay, will do. Albeit I was thinking to add naming to xenperf ...

>> --- a/xen/include/asm-x86/perfc_defn.h
>> +++ b/xen/include/asm-x86/perfc_defn.h
>> @@ -11,8 +11,8 @@ PERFCOUNTER_ARRAY(exceptions,
>>  PERFCOUNTER_ARRAY(vmexits,              "vmexits", VMX_PERF_EXIT_REASON_SIZE)
>>  PERFCOUNTER_ARRAY(cause_vector,         "cause vector", VMX_PERF_VECTOR_SIZE)
>>  
>> -#define VMEXIT_NPF_PERFC 141
>> -#define SVM_PERF_EXIT_REASON_SIZE (1+141)
>> +#define VMEXIT_NPF_PERFC 143
>> +#define SVM_PERF_EXIT_REASON_SIZE (VMEXIT_NPF_PERFC + 1)
> 
> How does this work in the first place?  perfc_incra() is still passed 1024.

In

    case VMEXIT_NPF:
        perfc_incra(svmexits, VMEXIT_NPF_PERFC);

I don't see any use of 1024. And the earlier blanket

    perfc_incra(svmexits, exit_reason);

doesn't do anything afaict, due to how perfc_incra() works.

> Furthermore, it's already worse than this.
> 
> 401/402 are AVIC exits, and 403 is an SEV exit.

In which way is this "worse"? We don't support either AVIC or SEV (which
is a shame in particular for the former, but what do you do with vendors
having given up all engagement).

>  We've also gained -2 as "busy" for transient SEV events too.

I'm sorry, I'm not enough up to speed with SEV yet to even vaguely know
what you're referring to here.

Jan

[PATCH 5/5] xenperf: name "newer" hypercalls

[Jan Beulich]

This table must not have got updated in quite a while; tmem_op for
example has managed to not only appear since then, but also disappear
again (adding a name for it nevertheless, to make more obvious that
something strange is going on if the slot would ever have a non-zero
value).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I wasn't really sure about x86's real names for arch_0 and arch_1. The
tool runs on the same host as the hypervisor, so __i386__ / __x86_64__
conditionals wouldn't be inappropriate to use ...

--- a/tools/misc/xenperf.c
+++ b/tools/misc/xenperf.c
@@ -18,7 +18,7 @@
 #include <string.h>
 
 #define X(name) [__HYPERVISOR_##name] = #name
-const char *hypercall_name_table[64] =
+static const char *const hypercall_name_table[64] =
 {
     X(set_trap_table),
     X(mmu_update),
@@ -57,6 +57,11 @@ const char *hypercall_name_table[64] =
     X(sysctl),
     X(domctl),
     X(kexec_op),
+    X(tmem_op),
+    X(argo_op),
+    X(xenpmu_op),
+    X(dm_op),
+    X(hypfs_op),
     X(arch_0),
     X(arch_1),
     X(arch_2),

Re: [PATCH 5/5] xenperf: name "newer" hypercalls

[Andrew Cooper]

On 03/12/2021 12:07, Jan Beulich wrote:
> This table must not have got updated in quite a while; tmem_op for
> example has managed to not only appear since then, but also disappear
> again (adding a name for it nevertheless, to make more obvious that
> something strange is going on if the slot would ever have a non-zero
> value).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wasn't really sure about x86's real names for arch_0 and arch_1. The
> tool runs on the same host as the hypervisor, so __i386__ / __x86_64__
> conditionals wouldn't be inappropriate to use ...

This is a developer tool.  Noone is going to have a perfcounter enabled
hypervisor in production.

Therefore, I think the ifdef's will be fine.

Preferably with, but either way, Acked-by: Andrew Cooper
<andrew.cooper3@citrix.com>