Hi Radim, You're right: this commit is unnecessary. I do think it's helpful, though, after initializing exec_control to "L0's desires," to explicitly clear those desires which are not relevant. I like your version. On Thu, May 10, 2018 at 8:46 AM, Radim Krčmář <rkrcmar@xxxxxxxxxx> wrote: > 2018-05-07 10:55-0700, Jim Mattson: >> If vmcs12 doesn't specify the "use MSR bitmaps" VM-execution control, >> then vmcs02 should not specify this control either. When the MSR >> bitmaps are not used, all executions of RDMSR and WRMSR cause >> VM-exits. > > We already clear it at the end of nested_get_vmcs12_pages() in this > case. > > I don't think that adding it here improves readability of the code. > Maybe if we made sure that vmcs02 always begins with disabled MSR > bitmaps and only set it afterwards, i.e. > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index c7668806163f..3938573cfb19 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -10435,9 +10435,6 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu, > if (nested_vmx_prepare_msr_bitmap(vcpu, vmcs12)) > vmcs_set_bits(CPU_BASED_VM_EXEC_CONTROL, > CPU_BASED_USE_MSR_BITMAPS); > - else > - vmcs_clear_bits(CPU_BASED_VM_EXEC_CONTROL, > - CPU_BASED_USE_MSR_BITMAPS); > } > > static void vmx_start_preemption_timer(struct kvm_vcpu *vcpu) > @@ -11139,6 +11136,9 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, > exec_control &= ~CPU_BASED_USE_IO_BITMAPS; > exec_control |= CPU_BASED_UNCOND_IO_EXITING; > > + /* MSR bitmaps are potentially enabled after recomputing the bitmap. */ > + exec_control &= ~CPU_BASED_USE_MSR_BITMAPS; > + > vmcs_write32(CPU_BASED_VM_EXEC_CONTROL, exec_control); > > /* EXCEPTION_BITMAP and CR0_GUEST_HOST_MASK should basically be the
