Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161077AbcK3LQ0 (ORCPT ); Wed, 30 Nov 2016 06:16:26 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:34293 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752167AbcK3LQQ (ORCPT ); Wed, 30 Nov 2016 06:16:16 -0500 Subject: Re: [PATCH v3 1/5] KVM: nVMX: generate non-true VMX MSRs based on true versions To: David Matlack , kvm@vger.kernel.org References: <1480472050-58023-1-git-send-email-dmatlack@google.com> <1480472050-58023-2-git-send-email-dmatlack@google.com> Cc: linux-kernel@vger.kernel.org, jmattson@google.com, rkrcmar@redhat.com From: Paolo Bonzini Message-ID: <59bf4f3c-34ab-5cbc-9579-f1a8648804ea@redhat.com> Date: Wed, 30 Nov 2016 12:16:12 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1480472050-58023-2-git-send-email-dmatlack@google.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6328 Lines: 157 On 30/11/2016 03:14, David Matlack wrote: > The "non-true" VMX capability MSRs can be generated from their "true" > counterparts, by OR-ing the default1 bits. The default1 bits are fixed > and defined in the SDM. > > Since we can generate the non-true VMX MSRs from the true versions, > there's no need to store both in struct nested_vmx. This also lets > userspace avoid having to restore the non-true MSRs. > > Note this does not preclude emulating MSR_IA32_VMX_BASIC[55]=0. To do so, > we simply need to set all the default1 bits in the true MSRs (such that > the true MSRs and the generated non-true MSRs are equal). > > Signed-off-by: David Matlack > Suggested-by: Paolo Bonzini > --- > arch/x86/kvm/vmx.c | 45 +++++++++++++++++++-------------------------- > 1 file changed, 19 insertions(+), 26 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 5382b82..0beb56a 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -446,19 +446,21 @@ struct nested_vmx { > u16 vpid02; > u16 last_vpid; > > + /* > + * We only store the "true" versions of the VMX capability MSRs. We > + * generate the "non-true" versions by setting the must-be-1 bits > + * according to the SDM. > + */ > u32 nested_vmx_procbased_ctls_low; > u32 nested_vmx_procbased_ctls_high; > - u32 nested_vmx_true_procbased_ctls_low; > u32 nested_vmx_secondary_ctls_low; > u32 nested_vmx_secondary_ctls_high; > u32 nested_vmx_pinbased_ctls_low; > u32 nested_vmx_pinbased_ctls_high; > u32 nested_vmx_exit_ctls_low; > u32 nested_vmx_exit_ctls_high; > - u32 nested_vmx_true_exit_ctls_low; > u32 nested_vmx_entry_ctls_low; > u32 nested_vmx_entry_ctls_high; > - u32 nested_vmx_true_entry_ctls_low; > u32 nested_vmx_misc_low; > u32 nested_vmx_misc_high; > u32 nested_vmx_ept_caps; > @@ -2712,9 +2714,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx) > vmx->nested.nested_vmx_exit_ctls_high |= VM_EXIT_CLEAR_BNDCFGS; > > /* We support free control of debug control saving. */ > - vmx->nested.nested_vmx_true_exit_ctls_low = > - vmx->nested.nested_vmx_exit_ctls_low & > - ~VM_EXIT_SAVE_DEBUG_CONTROLS; > + vmx->nested.nested_vmx_exit_ctls_low &= ~VM_EXIT_SAVE_DEBUG_CONTROLS; > > /* entry controls */ > rdmsr(MSR_IA32_VMX_ENTRY_CTLS, > @@ -2733,9 +2733,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx) > vmx->nested.nested_vmx_entry_ctls_high |= VM_ENTRY_LOAD_BNDCFGS; > > /* We support free control of debug control loading. */ > - vmx->nested.nested_vmx_true_entry_ctls_low = > - vmx->nested.nested_vmx_entry_ctls_low & > - ~VM_ENTRY_LOAD_DEBUG_CONTROLS; > + vmx->nested.nested_vmx_entry_ctls_low &= ~VM_ENTRY_LOAD_DEBUG_CONTROLS; > > /* cpu-based controls */ > rdmsr(MSR_IA32_VMX_PROCBASED_CTLS, > @@ -2768,8 +2766,7 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx) > CPU_BASED_USE_MSR_BITMAPS; > > /* We support free control of CR3 access interception. */ > - vmx->nested.nested_vmx_true_procbased_ctls_low = > - vmx->nested.nested_vmx_procbased_ctls_low & > + vmx->nested.nested_vmx_procbased_ctls_low &= > ~(CPU_BASED_CR3_LOAD_EXITING | CPU_BASED_CR3_STORE_EXITING); > > /* secondary cpu-based controls */ > @@ -2868,36 +2865,32 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata) > *pdata = vmx_control_msr( > vmx->nested.nested_vmx_pinbased_ctls_low, > vmx->nested.nested_vmx_pinbased_ctls_high); > + if (msr_index == MSR_IA32_VMX_PINBASED_CTLS) > + *pdata |= PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR; Almost: PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR must be added to both the low and high parts. Likewise below. I guess you can use vmx_control_msr to generate it, too. Paolo > break; > case MSR_IA32_VMX_TRUE_PROCBASED_CTLS: > - *pdata = vmx_control_msr( > - vmx->nested.nested_vmx_true_procbased_ctls_low, > - vmx->nested.nested_vmx_procbased_ctls_high); > - break; > case MSR_IA32_VMX_PROCBASED_CTLS: > *pdata = vmx_control_msr( > vmx->nested.nested_vmx_procbased_ctls_low, > vmx->nested.nested_vmx_procbased_ctls_high); > + if (msr_index == MSR_IA32_VMX_PROCBASED_CTLS) > + *pdata |= CPU_BASED_ALWAYSON_WITHOUT_TRUE_MSR; > break; > case MSR_IA32_VMX_TRUE_EXIT_CTLS: > - *pdata = vmx_control_msr( > - vmx->nested.nested_vmx_true_exit_ctls_low, > - vmx->nested.nested_vmx_exit_ctls_high); > - break; > case MSR_IA32_VMX_EXIT_CTLS: > *pdata = vmx_control_msr( > vmx->nested.nested_vmx_exit_ctls_low, > vmx->nested.nested_vmx_exit_ctls_high); > + if (msr_index == MSR_IA32_VMX_EXIT_CTLS) > + *pdata |= VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR; > break; > case MSR_IA32_VMX_TRUE_ENTRY_CTLS: > - *pdata = vmx_control_msr( > - vmx->nested.nested_vmx_true_entry_ctls_low, > - vmx->nested.nested_vmx_entry_ctls_high); > - break; > case MSR_IA32_VMX_ENTRY_CTLS: > *pdata = vmx_control_msr( > vmx->nested.nested_vmx_entry_ctls_low, > vmx->nested.nested_vmx_entry_ctls_high); > + if (msr_index == MSR_IA32_VMX_ENTRY_CTLS) > + *pdata |= VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR; > break; > case MSR_IA32_VMX_MISC: > *pdata = vmx_control_msr( > @@ -10184,7 +10177,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) > } > > if (!vmx_control_verify(vmcs12->cpu_based_vm_exec_control, > - vmx->nested.nested_vmx_true_procbased_ctls_low, > + vmx->nested.nested_vmx_procbased_ctls_low, > vmx->nested.nested_vmx_procbased_ctls_high) || > !vmx_control_verify(vmcs12->secondary_vm_exec_control, > vmx->nested.nested_vmx_secondary_ctls_low, > @@ -10193,10 +10186,10 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) > vmx->nested.nested_vmx_pinbased_ctls_low, > vmx->nested.nested_vmx_pinbased_ctls_high) || > !vmx_control_verify(vmcs12->vm_exit_controls, > - vmx->nested.nested_vmx_true_exit_ctls_low, > + vmx->nested.nested_vmx_exit_ctls_low, > vmx->nested.nested_vmx_exit_ctls_high) || > !vmx_control_verify(vmcs12->vm_entry_controls, > - vmx->nested.nested_vmx_true_entry_ctls_low, > + vmx->nested.nested_vmx_entry_ctls_low, > vmx->nested.nested_vmx_entry_ctls_high)) > { > nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD); >