Re: [PATCH v2 00/10] KVM: VMX: Unionize vcpu_vmx.exit_reason

[Paolo Bonzini]

On 15/04/20 19:55, Sean Christopherson wrote:
> Convert the exit_reason field in struct vcpu_vmx from a vanilla u32 to a
> union, (ab)using the union to provide access to the basic exit reason and
> flags.
> 
> There is a fairly substantial delta relative to v1, as I ran with Vitaly's
> suggestion to split nested_vmx_exit_reflected() into VM-Fail, "L0 wants"
> and "L1 wants", and move the tracepoint into nested_vmx_reflect_vmexit().
> IMO, this yields cleaner and more understandable code overall, and helps
> eliminate caching the basic exit reason (see below) by avoiding large
> functions that repeatedly query the basic exit reason.  The refactoring
> isn't strictly related to making exit_reason a union, but the code would
> conflict horribly and the end code nicely demonstrates the value of using
> a union for the exit reason.
> 
> There are three motivating factors for making exit_reason a union:
> 
>   - Help avoid bugs where a basic exit reason is compared against the full
>     exit reason, e.g. there have been two bugs where MCE_DURING_VMENTRY
>     was incorrectly compared against the full exit reason.
> 
>   - Clarify the intent of related flows, e.g. exit_reason is used for both
>     "basic exit reason" and "full exit reason", and it's not always clear
>     which of the two is intended without a fair bit of digging.
> 
>   - Prepare for future Intel features, e.g. SGX, that add new exit flags
>     that are less restricted than FAILED_VMENTRY, i.e. can be set on what
>     is otherwise a standard VM-Exit.
> 
> v2:
>   - Don't snapshot the basic exit reason, i.e. either use vmx->exit_reason
>     directly or snapshot the whole thing.  The resulting code is similar
>     to Xiaoyao's original patch, e.g. vmx_handle_exit() now uses
>     "exit_reason.basic" instead of "exit_reason" to reference the basic
>     exit reason.
>   - Split nested_vmx_exit_reflected() into VM-Fail, "L0 wants" and "L1
>     wants", and move the tracepoint into nested_vmx_reflect_vmexit().
>     [Vitaly]
>   - Use a "union vmx_exit_reason exit_reason" to handle a consistency
>     check VM-Exit on VM-Enter in nested_vmx_enter_non_root_mode() to avoid
>     some implicit casting shenanigans. [Vitaly]
>   - Collect tags. [Vitaly]
> 
> v1: https://lkml.kernel.org/r/20200312184521.24579-1-sean.j.christopherson@xxxxxxxxx

For now I committed only patches 1-9, just to limit the conflicts with
the other series.  I would like to understand how you think the
conflicts should be fixed with the union.

Paolo