| Message ID | 20200415175519.14230-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | KVM: VMX: Unionize vcpu_vmx.exit_reason | expand |
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
On Thu, Apr 16, 2020 at 03:44:06PM +0200, Paolo Bonzini wrote: > On 15/04/20 19:55, Sean Christopherson wrote: > 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. Pushed a branch. Basically, take the union code and then make sure there aren't any vmcs_read32(VM_EXIT_INTR_INFO) or vmcs_readl(EXIT_QUALIFICATION) calls outside of the caching accessors or dump_vmcs(). https://github.com/sean-jc/linux for_paolo_merge_union_cache
On Thu, Apr 16, 2020 at 08:07:49AM -0700, Sean Christopherson wrote: > On Thu, Apr 16, 2020 at 03:44:06PM +0200, Paolo Bonzini wrote: > > On 15/04/20 19:55, Sean Christopherson wrote: > > 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. > > Pushed a branch. Basically, take the union code and then make sure there > aren't any vmcs_read32(VM_EXIT_INTR_INFO) or vmcs_readl(EXIT_QUALIFICATION) > calls outside of the caching accessors or dump_vmcs(). > > https://github.com/sean-jc/linux for_paolo_merge_union_cache Sent v3, seemed easier than having you decipher my merge resolution and then fix more conflicts with kvm/queue.
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 Sean Christopherson (10): KVM: nVMX: Move reflection check into nested_vmx_reflect_vmexit() KVM: nVMX: Uninline nested_vmx_reflect_vmexit(), i.e. move it to nested.c KVM: nVMX: Move VM-Fail check out of nested_vmx_exit_reflected() KVM: nVMX: Move nested VM-Exit tracepoint into nested_vmx_reflect_vmexit() KVM: nVMX: Split VM-Exit reflection logic into L0 vs. L1 wants KVM: nVMX: Drop a superfluous WARN on reflecting EXTERNAL_INTERRUPT KVM: nVMX: Pull exit_reason from vcpu_vmx in nested_vmx_reflect_vmexit() KVM: nVMX: Cast exit_reason to u16 to check for nested EXTERNAL_INTERRUPT KVM: nVMX: Rename exit_reason to vm_exit_reason for nested VM-Exit KVM: VMX: Convert vcpu_vmx.exit_reason to a union arch/x86/kvm/vmx/nested.c | 237 +++++++++++++++++++++++++------------- arch/x86/kvm/vmx/nested.h | 32 +---- arch/x86/kvm/vmx/vmx.c | 66 ++++++----- arch/x86/kvm/vmx/vmx.h | 25 +++- 4 files changed, 219 insertions(+), 141 deletions(-)