linux-kernel - Re: [PATCH v2 00/10] KVM: VMX: Unionize vcpu_vmx.exit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Apr 2020 15:44:06 +0200
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Xiaoyao Li <xiaoyao.li@...el.com>
Subject: Re: [PATCH v2 00/10] KVM: VMX: Unionize vcpu_vmx.exit_reason

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@intel.com

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