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)

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

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