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

[Jan Beulich <jbeulich@suse.com>]

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