diff mbox

KVM: VMX: shadow VM_(ENTRY|EXIT)_CONTROLS vmcs field

Message ID 20131209065550.GB16543@minantech.com (mailing list archive)
State New, archived
Headers show

Commit Message

Gleb Natapov Dec. 9, 2013, 6:55 a.m. UTC
On Sun, Dec 08, 2013 at 02:00:11PM +0100, Jan Kiszka wrote:
> On 2013-11-27 17:58, Paolo Bonzini wrote:
> > Il 25/11/2013 14:37, Gleb Natapov ha scritto:
> >> VM_(ENTRY|EXIT)_CONTROLS vmcs fields are read/written on each guest
> >> entry but most times it can be avoided since values do not changes.
> >> Keep fields copy in memory to avoid unnecessary reads from vmcs.
> >>
> >> Signed-off-by: Gleb Natapov <gleb@redhat.com>
> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> >> index b2fe1c2..29c1c7f 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -418,6 +418,8 @@ struct vcpu_vmx {
> >>  	u64 		      msr_host_kernel_gs_base;
> >>  	u64 		      msr_guest_kernel_gs_base;
> >>  #endif
> >> +	u32 vm_entry_controls_shadow;
> >> +	u32 vm_exit_controls_shadow;
> >>  	/*
> >>  	 * loaded_vmcs points to the VMCS currently used in this vcpu. For a
> >>  	 * non-nested (L1) guest, it always points to vmcs01. For a nested
> >> @@ -1326,6 +1328,62 @@ static void vmcs_set_bits(unsigned long field, u32 mask)
> >>  	vmcs_writel(field, vmcs_readl(field) | mask);
> >>  }
> >>  
> >> +static inline void vm_entry_controls_init(struct vcpu_vmx *vmx, u32 val)
> >> +{
> >> +	vmcs_write32(VM_ENTRY_CONTROLS, val);
> >> +	vmx->vm_entry_controls_shadow = val;
> >> +}
> >> +
> >> +static inline void vm_entry_controls_set(struct vcpu_vmx *vmx, u32 val)
> >> +{
> >> +	if (vmx->vm_entry_controls_shadow != val)
> >> +		vm_entry_controls_init(vmx, val);
> >> +}
> >> +
> >> +static inline u32 vm_entry_controls_get(struct vcpu_vmx *vmx)
> >> +{
> >> +	return vmx->vm_entry_controls_shadow;
> >> +}
> >> +
> >> +
> >> +static inline void vm_entry_controls_setbit(struct vcpu_vmx *vmx, u32 val)
> >> +{
> >> +	vm_entry_controls_set(vmx, vm_entry_controls_get(vmx) | val);
> >> +}
> >> +
> >> +static inline void vm_entry_controls_clearbit(struct vcpu_vmx *vmx, u32 val)
> >> +{
> >> +	vm_entry_controls_set(vmx, vm_entry_controls_get(vmx) & ~val);
> >> +}
> >> +
> >> +static inline void vm_exit_controls_init(struct vcpu_vmx *vmx, u32 val)
> >> +{
> >> +	vmcs_write32(VM_EXIT_CONTROLS, val);
> >> +	vmx->vm_exit_controls_shadow = val;
> >> +}
> >> +
> >> +static inline void vm_exit_controls_set(struct vcpu_vmx *vmx, u32 val)
> >> +{
> >> +	if (vmx->vm_exit_controls_shadow != val)
> >> +		vm_exit_controls_init(vmx, val);
> >> +}
> >> +
> >> +static inline u32 vm_exit_controls_get(struct vcpu_vmx *vmx)
> >> +{
> >> +	return vmx->vm_exit_controls_shadow;
> >> +}
> >> +
> >> +
> >> +static inline void vm_exit_controls_setbit(struct vcpu_vmx *vmx, u32 val)
> >> +{
> >> +	vm_exit_controls_set(vmx, vm_exit_controls_get(vmx) | val);
> >> +}
> >> +
> >> +static inline void vm_exit_controls_clearbit(struct vcpu_vmx *vmx, u32 val)
> >> +{
> >> +	vm_exit_controls_set(vmx, vm_exit_controls_get(vmx) & ~val);
> >> +}
> >> +
> >>  static void vmx_segment_cache_clear(struct vcpu_vmx *vmx)
> >>  {
> >>  	vmx->segment_cache.bitmask = 0;
> >> @@ -1410,11 +1468,11 @@ static void update_exception_bitmap(struct kvm_vcpu *vcpu)
> >>  	vmcs_write32(EXCEPTION_BITMAP, eb);
> >>  }
> >>  
> >> -static void clear_atomic_switch_msr_special(unsigned long entry,
> >> -		unsigned long exit)
> >> +static void clear_atomic_switch_msr_special(struct vcpu_vmx *vmx,
> >> +		unsigned long entry, unsigned long exit)
> >>  {
> >> -	vmcs_clear_bits(VM_ENTRY_CONTROLS, entry);
> >> -	vmcs_clear_bits(VM_EXIT_CONTROLS, exit);
> >> +	vm_entry_controls_clearbit(vmx, entry);
> >> +	vm_exit_controls_clearbit(vmx, exit);
> >>  }
> >>  
> >>  static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
> >> @@ -1425,14 +1483,15 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
> >>  	switch (msr) {
> >>  	case MSR_EFER:
> >>  		if (cpu_has_load_ia32_efer) {
> >> -			clear_atomic_switch_msr_special(VM_ENTRY_LOAD_IA32_EFER,
> >> +			clear_atomic_switch_msr_special(vmx,
> >> +					VM_ENTRY_LOAD_IA32_EFER,
> >>  					VM_EXIT_LOAD_IA32_EFER);
> >>  			return;
> >>  		}
> >>  		break;
> >>  	case MSR_CORE_PERF_GLOBAL_CTRL:
> >>  		if (cpu_has_load_perf_global_ctrl) {
> >> -			clear_atomic_switch_msr_special(
> >> +			clear_atomic_switch_msr_special(vmx,
> >>  					VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
> >>  					VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL);
> >>  			return;
> >> @@ -1453,14 +1512,15 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
> >>  	vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, m->nr);
> >>  }
> >>  
> >> -static void add_atomic_switch_msr_special(unsigned long entry,
> >> -		unsigned long exit, unsigned long guest_val_vmcs,
> >> -		unsigned long host_val_vmcs, u64 guest_val, u64 host_val)
> >> +static void add_atomic_switch_msr_special(struct vcpu_vmx *vmx,
> >> +		unsigned long entry, unsigned long exit,
> >> +		unsigned long guest_val_vmcs, unsigned long host_val_vmcs,
> >> +		u64 guest_val, u64 host_val)
> >>  {
> >>  	vmcs_write64(guest_val_vmcs, guest_val);
> >>  	vmcs_write64(host_val_vmcs, host_val);
> >> -	vmcs_set_bits(VM_ENTRY_CONTROLS, entry);
> >> -	vmcs_set_bits(VM_EXIT_CONTROLS, exit);
> >> +	vm_entry_controls_setbit(vmx, entry);
> >> +	vm_exit_controls_setbit(vmx, exit);
> >>  }
> >>  
> >>  static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
> >> @@ -1472,7 +1532,8 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
> >>  	switch (msr) {
> >>  	case MSR_EFER:
> >>  		if (cpu_has_load_ia32_efer) {
> >> -			add_atomic_switch_msr_special(VM_ENTRY_LOAD_IA32_EFER,
> >> +			add_atomic_switch_msr_special(vmx,
> >> +					VM_ENTRY_LOAD_IA32_EFER,
> >>  					VM_EXIT_LOAD_IA32_EFER,
> >>  					GUEST_IA32_EFER,
> >>  					HOST_IA32_EFER,
> >> @@ -1482,7 +1543,7 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
> >>  		break;
> >>  	case MSR_CORE_PERF_GLOBAL_CTRL:
> >>  		if (cpu_has_load_perf_global_ctrl) {
> >> -			add_atomic_switch_msr_special(
> >> +			add_atomic_switch_msr_special(vmx,
> >>  					VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
> >>  					VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL,
> >>  					GUEST_IA32_PERF_GLOBAL_CTRL,
> >> @@ -3182,14 +3243,10 @@ static void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
> >>  	vmx_load_host_state(to_vmx(vcpu));
> >>  	vcpu->arch.efer = efer;
> >>  	if (efer & EFER_LMA) {
> >> -		vmcs_write32(VM_ENTRY_CONTROLS,
> >> -			     vmcs_read32(VM_ENTRY_CONTROLS) |
> >> -			     VM_ENTRY_IA32E_MODE);
> >> +		vm_entry_controls_setbit(to_vmx(vcpu), VM_ENTRY_IA32E_MODE);
> >>  		msr->data = efer;
> >>  	} else {
> >> -		vmcs_write32(VM_ENTRY_CONTROLS,
> >> -			     vmcs_read32(VM_ENTRY_CONTROLS) &
> >> -			     ~VM_ENTRY_IA32E_MODE);
> >> +		vm_entry_controls_clearbit(to_vmx(vcpu), VM_ENTRY_IA32E_MODE);
> >>  
> >>  		msr->data = efer & ~EFER_LME;
> >>  	}
> >> @@ -3217,9 +3274,7 @@ static void enter_lmode(struct kvm_vcpu *vcpu)
> >>  
> >>  static void exit_lmode(struct kvm_vcpu *vcpu)
> >>  {
> >> -	vmcs_write32(VM_ENTRY_CONTROLS,
> >> -		     vmcs_read32(VM_ENTRY_CONTROLS)
> >> -		     & ~VM_ENTRY_IA32E_MODE);
> >> +	vm_entry_controls_clearbit(to_vmx(vcpu), VM_ENTRY_IA32E_MODE);
> >>  	vmx_set_efer(vcpu, vcpu->arch.efer & ~EFER_LMA);
> >>  }
> >>  
> >> @@ -4346,10 +4401,11 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
> >>  		++vmx->nmsrs;
> >>  	}
> >>  
> >> -	vmcs_write32(VM_EXIT_CONTROLS, vmcs_config.vmexit_ctrl);
> >> +
> >> +	vm_exit_controls_init(vmx, vmcs_config.vmexit_ctrl);
> >>  
> >>  	/* 22.2.1, 20.8.1 */
> >> -	vmcs_write32(VM_ENTRY_CONTROLS, vmcs_config.vmentry_ctrl);
> >> +	vm_entry_controls_init(vmx, vmcs_config.vmentry_ctrl);
> >>  
> >>  	vmcs_writel(CR0_GUEST_HOST_MASK, ~0UL);
> >>  	set_cr4_guest_host_mask(vmx);
> >> @@ -7759,12 +7815,12 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> >>  	exit_control = vmcs_config.vmexit_ctrl;
> >>  	if (vmcs12->pin_based_vm_exec_control & PIN_BASED_VMX_PREEMPTION_TIMER)
> >>  		exit_control |= VM_EXIT_SAVE_VMX_PREEMPTION_TIMER;
> >> -	vmcs_write32(VM_EXIT_CONTROLS, exit_control);
> >> +	vm_exit_controls_init(vmx, exit_control);
> >>  
> >>  	/* vmcs12's VM_ENTRY_LOAD_IA32_EFER and VM_ENTRY_IA32E_MODE are
> >>  	 * emulated by vmx_set_efer(), below.
> >>  	 */
> >> -	vmcs_write32(VM_ENTRY_CONTROLS,
> >> +	vm_entry_controls_init(vmx, 
> >>  		(vmcs12->vm_entry_controls & ~VM_ENTRY_LOAD_IA32_EFER &
> >>  			~VM_ENTRY_IA32E_MODE) |
> >>  		(vmcs_config.vmentry_ctrl & ~VM_ENTRY_IA32E_MODE));
> >> @@ -8186,7 +8242,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
> >>  
> >>  	vmcs12->vm_entry_controls =
> >>  		(vmcs12->vm_entry_controls & ~VM_ENTRY_IA32E_MODE) |
> >> -		(vmcs_read32(VM_ENTRY_CONTROLS) & VM_ENTRY_IA32E_MODE);
> >> +		(vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE);
> >>  
> >>  	/* TODO: These cannot have changed unless we have MSR bitmaps and
> >>  	 * the relevant bit asks not to trap the change */
> >> @@ -8390,6 +8446,7 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu)
> >>  	vcpu->cpu = cpu;
> >>  	put_cpu();
> >>  
> >> +	vm_entry_controls_init(vmx, vmcs_read32(VM_ENTRY_CONTROLS));
> >>  	vmx_segment_cache_clear(vmx);
> >>  
> >>  	/* if no vmcs02 cache requested, remove the one we used */
> >> --
> >> 			Gleb.
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe kvm" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>
> > 
> > Applied to kvm/queue, thanks.
> 
> Seems to have or cause an issue with nVMX. The (still not merged) vmx
> unit test breaks here after apply this commit:
> 
What about this patch?


--
			Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jan Kiszka Dec. 9, 2013, 1:48 p.m. UTC | #1
On 2013-12-09 07:55, Gleb Natapov wrote:
> On Sun, Dec 08, 2013 at 02:00:11PM +0100, Jan Kiszka wrote:
>> On 2013-11-27 17:58, Paolo Bonzini wrote:
>>> Il 25/11/2013 14:37, Gleb Natapov ha scritto:
>>>> VM_(ENTRY|EXIT)_CONTROLS vmcs fields are read/written on each guest
>>>> entry but most times it can be avoided since values do not changes.
>>>> Keep fields copy in memory to avoid unnecessary reads from vmcs.
>>>>
>>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index b2fe1c2..29c1c7f 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -418,6 +418,8 @@ struct vcpu_vmx {
>>>>  	u64 		      msr_host_kernel_gs_base;
>>>>  	u64 		      msr_guest_kernel_gs_base;
>>>>  #endif
>>>> +	u32 vm_entry_controls_shadow;
>>>> +	u32 vm_exit_controls_shadow;
>>>>  	/*
>>>>  	 * loaded_vmcs points to the VMCS currently used in this vcpu. For a
>>>>  	 * non-nested (L1) guest, it always points to vmcs01. For a nested
>>>> @@ -1326,6 +1328,62 @@ static void vmcs_set_bits(unsigned long field, u32 mask)
>>>>  	vmcs_writel(field, vmcs_readl(field) | mask);
>>>>  }
>>>>  
>>>> +static inline void vm_entry_controls_init(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vmcs_write32(VM_ENTRY_CONTROLS, val);
>>>> +	vmx->vm_entry_controls_shadow = val;
>>>> +}
>>>> +
>>>> +static inline void vm_entry_controls_set(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	if (vmx->vm_entry_controls_shadow != val)
>>>> +		vm_entry_controls_init(vmx, val);
>>>> +}
>>>> +
>>>> +static inline u32 vm_entry_controls_get(struct vcpu_vmx *vmx)
>>>> +{
>>>> +	return vmx->vm_entry_controls_shadow;
>>>> +}
>>>> +
>>>> +
>>>> +static inline void vm_entry_controls_setbit(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vm_entry_controls_set(vmx, vm_entry_controls_get(vmx) | val);
>>>> +}
>>>> +
>>>> +static inline void vm_entry_controls_clearbit(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vm_entry_controls_set(vmx, vm_entry_controls_get(vmx) & ~val);
>>>> +}
>>>> +
>>>> +static inline void vm_exit_controls_init(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vmcs_write32(VM_EXIT_CONTROLS, val);
>>>> +	vmx->vm_exit_controls_shadow = val;
>>>> +}
>>>> +
>>>> +static inline void vm_exit_controls_set(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	if (vmx->vm_exit_controls_shadow != val)
>>>> +		vm_exit_controls_init(vmx, val);
>>>> +}
>>>> +
>>>> +static inline u32 vm_exit_controls_get(struct vcpu_vmx *vmx)
>>>> +{
>>>> +	return vmx->vm_exit_controls_shadow;
>>>> +}
>>>> +
>>>> +
>>>> +static inline void vm_exit_controls_setbit(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vm_exit_controls_set(vmx, vm_exit_controls_get(vmx) | val);
>>>> +}
>>>> +
>>>> +static inline void vm_exit_controls_clearbit(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vm_exit_controls_set(vmx, vm_exit_controls_get(vmx) & ~val);
>>>> +}
>>>> +
>>>>  static void vmx_segment_cache_clear(struct vcpu_vmx *vmx)
>>>>  {
>>>>  	vmx->segment_cache.bitmask = 0;
>>>> @@ -1410,11 +1468,11 @@ static void update_exception_bitmap(struct kvm_vcpu *vcpu)
>>>>  	vmcs_write32(EXCEPTION_BITMAP, eb);
>>>>  }
>>>>  
>>>> -static void clear_atomic_switch_msr_special(unsigned long entry,
>>>> -		unsigned long exit)
>>>> +static void clear_atomic_switch_msr_special(struct vcpu_vmx *vmx,
>>>> +		unsigned long entry, unsigned long exit)
>>>>  {
>>>> -	vmcs_clear_bits(VM_ENTRY_CONTROLS, entry);
>>>> -	vmcs_clear_bits(VM_EXIT_CONTROLS, exit);
>>>> +	vm_entry_controls_clearbit(vmx, entry);
>>>> +	vm_exit_controls_clearbit(vmx, exit);
>>>>  }
>>>>  
>>>>  static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
>>>> @@ -1425,14 +1483,15 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
>>>>  	switch (msr) {
>>>>  	case MSR_EFER:
>>>>  		if (cpu_has_load_ia32_efer) {
>>>> -			clear_atomic_switch_msr_special(VM_ENTRY_LOAD_IA32_EFER,
>>>> +			clear_atomic_switch_msr_special(vmx,
>>>> +					VM_ENTRY_LOAD_IA32_EFER,
>>>>  					VM_EXIT_LOAD_IA32_EFER);
>>>>  			return;
>>>>  		}
>>>>  		break;
>>>>  	case MSR_CORE_PERF_GLOBAL_CTRL:
>>>>  		if (cpu_has_load_perf_global_ctrl) {
>>>> -			clear_atomic_switch_msr_special(
>>>> +			clear_atomic_switch_msr_special(vmx,
>>>>  					VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
>>>>  					VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL);
>>>>  			return;
>>>> @@ -1453,14 +1512,15 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
>>>>  	vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, m->nr);
>>>>  }
>>>>  
>>>> -static void add_atomic_switch_msr_special(unsigned long entry,
>>>> -		unsigned long exit, unsigned long guest_val_vmcs,
>>>> -		unsigned long host_val_vmcs, u64 guest_val, u64 host_val)
>>>> +static void add_atomic_switch_msr_special(struct vcpu_vmx *vmx,
>>>> +		unsigned long entry, unsigned long exit,
>>>> +		unsigned long guest_val_vmcs, unsigned long host_val_vmcs,
>>>> +		u64 guest_val, u64 host_val)
>>>>  {
>>>>  	vmcs_write64(guest_val_vmcs, guest_val);
>>>>  	vmcs_write64(host_val_vmcs, host_val);
>>>> -	vmcs_set_bits(VM_ENTRY_CONTROLS, entry);
>>>> -	vmcs_set_bits(VM_EXIT_CONTROLS, exit);
>>>> +	vm_entry_controls_setbit(vmx, entry);
>>>> +	vm_exit_controls_setbit(vmx, exit);
>>>>  }
>>>>  
>>>>  static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
>>>> @@ -1472,7 +1532,8 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
>>>>  	switch (msr) {
>>>>  	case MSR_EFER:
>>>>  		if (cpu_has_load_ia32_efer) {
>>>> -			add_atomic_switch_msr_special(VM_ENTRY_LOAD_IA32_EFER,
>>>> +			add_atomic_switch_msr_special(vmx,
>>>> +					VM_ENTRY_LOAD_IA32_EFER,
>>>>  					VM_EXIT_LOAD_IA32_EFER,
>>>>  					GUEST_IA32_EFER,
>>>>  					HOST_IA32_EFER,
>>>> @@ -1482,7 +1543,7 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
>>>>  		break;
>>>>  	case MSR_CORE_PERF_GLOBAL_CTRL:
>>>>  		if (cpu_has_load_perf_global_ctrl) {
>>>> -			add_atomic_switch_msr_special(
>>>> +			add_atomic_switch_msr_special(vmx,
>>>>  					VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
>>>>  					VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL,
>>>>  					GUEST_IA32_PERF_GLOBAL_CTRL,
>>>> @@ -3182,14 +3243,10 @@ static void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
>>>>  	vmx_load_host_state(to_vmx(vcpu));
>>>>  	vcpu->arch.efer = efer;
>>>>  	if (efer & EFER_LMA) {
>>>> -		vmcs_write32(VM_ENTRY_CONTROLS,
>>>> -			     vmcs_read32(VM_ENTRY_CONTROLS) |
>>>> -			     VM_ENTRY_IA32E_MODE);
>>>> +		vm_entry_controls_setbit(to_vmx(vcpu), VM_ENTRY_IA32E_MODE);
>>>>  		msr->data = efer;
>>>>  	} else {
>>>> -		vmcs_write32(VM_ENTRY_CONTROLS,
>>>> -			     vmcs_read32(VM_ENTRY_CONTROLS) &
>>>> -			     ~VM_ENTRY_IA32E_MODE);
>>>> +		vm_entry_controls_clearbit(to_vmx(vcpu), VM_ENTRY_IA32E_MODE);
>>>>  
>>>>  		msr->data = efer & ~EFER_LME;
>>>>  	}
>>>> @@ -3217,9 +3274,7 @@ static void enter_lmode(struct kvm_vcpu *vcpu)
>>>>  
>>>>  static void exit_lmode(struct kvm_vcpu *vcpu)
>>>>  {
>>>> -	vmcs_write32(VM_ENTRY_CONTROLS,
>>>> -		     vmcs_read32(VM_ENTRY_CONTROLS)
>>>> -		     & ~VM_ENTRY_IA32E_MODE);
>>>> +	vm_entry_controls_clearbit(to_vmx(vcpu), VM_ENTRY_IA32E_MODE);
>>>>  	vmx_set_efer(vcpu, vcpu->arch.efer & ~EFER_LMA);
>>>>  }
>>>>  
>>>> @@ -4346,10 +4401,11 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
>>>>  		++vmx->nmsrs;
>>>>  	}
>>>>  
>>>> -	vmcs_write32(VM_EXIT_CONTROLS, vmcs_config.vmexit_ctrl);
>>>> +
>>>> +	vm_exit_controls_init(vmx, vmcs_config.vmexit_ctrl);
>>>>  
>>>>  	/* 22.2.1, 20.8.1 */
>>>> -	vmcs_write32(VM_ENTRY_CONTROLS, vmcs_config.vmentry_ctrl);
>>>> +	vm_entry_controls_init(vmx, vmcs_config.vmentry_ctrl);
>>>>  
>>>>  	vmcs_writel(CR0_GUEST_HOST_MASK, ~0UL);
>>>>  	set_cr4_guest_host_mask(vmx);
>>>> @@ -7759,12 +7815,12 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>>>>  	exit_control = vmcs_config.vmexit_ctrl;
>>>>  	if (vmcs12->pin_based_vm_exec_control & PIN_BASED_VMX_PREEMPTION_TIMER)
>>>>  		exit_control |= VM_EXIT_SAVE_VMX_PREEMPTION_TIMER;
>>>> -	vmcs_write32(VM_EXIT_CONTROLS, exit_control);
>>>> +	vm_exit_controls_init(vmx, exit_control);
>>>>  
>>>>  	/* vmcs12's VM_ENTRY_LOAD_IA32_EFER and VM_ENTRY_IA32E_MODE are
>>>>  	 * emulated by vmx_set_efer(), below.
>>>>  	 */
>>>> -	vmcs_write32(VM_ENTRY_CONTROLS,
>>>> +	vm_entry_controls_init(vmx, 
>>>>  		(vmcs12->vm_entry_controls & ~VM_ENTRY_LOAD_IA32_EFER &
>>>>  			~VM_ENTRY_IA32E_MODE) |
>>>>  		(vmcs_config.vmentry_ctrl & ~VM_ENTRY_IA32E_MODE));
>>>> @@ -8186,7 +8242,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>>>>  
>>>>  	vmcs12->vm_entry_controls =
>>>>  		(vmcs12->vm_entry_controls & ~VM_ENTRY_IA32E_MODE) |
>>>> -		(vmcs_read32(VM_ENTRY_CONTROLS) & VM_ENTRY_IA32E_MODE);
>>>> +		(vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE);
>>>>  
>>>>  	/* TODO: These cannot have changed unless we have MSR bitmaps and
>>>>  	 * the relevant bit asks not to trap the change */
>>>> @@ -8390,6 +8446,7 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu)
>>>>  	vcpu->cpu = cpu;
>>>>  	put_cpu();
>>>>  
>>>> +	vm_entry_controls_init(vmx, vmcs_read32(VM_ENTRY_CONTROLS));
>>>>  	vmx_segment_cache_clear(vmx);
>>>>  
>>>>  	/* if no vmcs02 cache requested, remove the one we used */
>>>> --
>>>> 			Gleb.
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>> Applied to kvm/queue, thanks.
>>
>> Seems to have or cause an issue with nVMX. The (still not merged) vmx
>> unit test breaks here after apply this commit:
>>
> What about this patch?
> 
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 1d95384..f90320b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8452,6 +8452,7 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu)
>  	put_cpu();
>  
>  	vm_entry_controls_init(vmx, vmcs_read32(VM_ENTRY_CONTROLS));
> +	vm_exit_controls_init(vmx, vmcs_read32(VM_EXIT_CONTROLS));
>  	vmx_segment_cache_clear(vmx);
>  
>  	/* if no vmcs02 cache requested, remove the one we used */
> --
> 			Gleb.
> 

Yep, works and makes sense as well.

Thanks,
Jan
Paolo Bonzini Dec. 12, 2013, 9:50 a.m. UTC | #2
Il 09/12/2013 07:55, Gleb Natapov ha scritto:
> On Sun, Dec 08, 2013 at 02:00:11PM +0100, Jan Kiszka wrote:
>> On 2013-11-27 17:58, Paolo Bonzini wrote:
>>> Il 25/11/2013 14:37, Gleb Natapov ha scritto:
>>>> VM_(ENTRY|EXIT)_CONTROLS vmcs fields are read/written on each guest
>>>> entry but most times it can be avoided since values do not changes.
>>>> Keep fields copy in memory to avoid unnecessary reads from vmcs.
>>>>
>>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index b2fe1c2..29c1c7f 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -418,6 +418,8 @@ struct vcpu_vmx {
>>>>  	u64 		      msr_host_kernel_gs_base;
>>>>  	u64 		      msr_guest_kernel_gs_base;
>>>>  #endif
>>>> +	u32 vm_entry_controls_shadow;
>>>> +	u32 vm_exit_controls_shadow;
>>>>  	/*
>>>>  	 * loaded_vmcs points to the VMCS currently used in this vcpu. For a
>>>>  	 * non-nested (L1) guest, it always points to vmcs01. For a nested
>>>> @@ -1326,6 +1328,62 @@ static void vmcs_set_bits(unsigned long field, u32 mask)
>>>>  	vmcs_writel(field, vmcs_readl(field) | mask);
>>>>  }
>>>>  
>>>> +static inline void vm_entry_controls_init(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vmcs_write32(VM_ENTRY_CONTROLS, val);
>>>> +	vmx->vm_entry_controls_shadow = val;
>>>> +}
>>>> +
>>>> +static inline void vm_entry_controls_set(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	if (vmx->vm_entry_controls_shadow != val)
>>>> +		vm_entry_controls_init(vmx, val);
>>>> +}
>>>> +
>>>> +static inline u32 vm_entry_controls_get(struct vcpu_vmx *vmx)
>>>> +{
>>>> +	return vmx->vm_entry_controls_shadow;
>>>> +}
>>>> +
>>>> +
>>>> +static inline void vm_entry_controls_setbit(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vm_entry_controls_set(vmx, vm_entry_controls_get(vmx) | val);
>>>> +}
>>>> +
>>>> +static inline void vm_entry_controls_clearbit(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vm_entry_controls_set(vmx, vm_entry_controls_get(vmx) & ~val);
>>>> +}
>>>> +
>>>> +static inline void vm_exit_controls_init(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vmcs_write32(VM_EXIT_CONTROLS, val);
>>>> +	vmx->vm_exit_controls_shadow = val;
>>>> +}
>>>> +
>>>> +static inline void vm_exit_controls_set(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	if (vmx->vm_exit_controls_shadow != val)
>>>> +		vm_exit_controls_init(vmx, val);
>>>> +}
>>>> +
>>>> +static inline u32 vm_exit_controls_get(struct vcpu_vmx *vmx)
>>>> +{
>>>> +	return vmx->vm_exit_controls_shadow;
>>>> +}
>>>> +
>>>> +
>>>> +static inline void vm_exit_controls_setbit(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vm_exit_controls_set(vmx, vm_exit_controls_get(vmx) | val);
>>>> +}
>>>> +
>>>> +static inline void vm_exit_controls_clearbit(struct vcpu_vmx *vmx, u32 val)
>>>> +{
>>>> +	vm_exit_controls_set(vmx, vm_exit_controls_get(vmx) & ~val);
>>>> +}
>>>> +
>>>>  static void vmx_segment_cache_clear(struct vcpu_vmx *vmx)
>>>>  {
>>>>  	vmx->segment_cache.bitmask = 0;
>>>> @@ -1410,11 +1468,11 @@ static void update_exception_bitmap(struct kvm_vcpu *vcpu)
>>>>  	vmcs_write32(EXCEPTION_BITMAP, eb);
>>>>  }
>>>>  
>>>> -static void clear_atomic_switch_msr_special(unsigned long entry,
>>>> -		unsigned long exit)
>>>> +static void clear_atomic_switch_msr_special(struct vcpu_vmx *vmx,
>>>> +		unsigned long entry, unsigned long exit)
>>>>  {
>>>> -	vmcs_clear_bits(VM_ENTRY_CONTROLS, entry);
>>>> -	vmcs_clear_bits(VM_EXIT_CONTROLS, exit);
>>>> +	vm_entry_controls_clearbit(vmx, entry);
>>>> +	vm_exit_controls_clearbit(vmx, exit);
>>>>  }
>>>>  
>>>>  static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
>>>> @@ -1425,14 +1483,15 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
>>>>  	switch (msr) {
>>>>  	case MSR_EFER:
>>>>  		if (cpu_has_load_ia32_efer) {
>>>> -			clear_atomic_switch_msr_special(VM_ENTRY_LOAD_IA32_EFER,
>>>> +			clear_atomic_switch_msr_special(vmx,
>>>> +					VM_ENTRY_LOAD_IA32_EFER,
>>>>  					VM_EXIT_LOAD_IA32_EFER);
>>>>  			return;
>>>>  		}
>>>>  		break;
>>>>  	case MSR_CORE_PERF_GLOBAL_CTRL:
>>>>  		if (cpu_has_load_perf_global_ctrl) {
>>>> -			clear_atomic_switch_msr_special(
>>>> +			clear_atomic_switch_msr_special(vmx,
>>>>  					VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
>>>>  					VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL);
>>>>  			return;
>>>> @@ -1453,14 +1512,15 @@ static void clear_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr)
>>>>  	vmcs_write32(VM_EXIT_MSR_LOAD_COUNT, m->nr);
>>>>  }
>>>>  
>>>> -static void add_atomic_switch_msr_special(unsigned long entry,
>>>> -		unsigned long exit, unsigned long guest_val_vmcs,
>>>> -		unsigned long host_val_vmcs, u64 guest_val, u64 host_val)
>>>> +static void add_atomic_switch_msr_special(struct vcpu_vmx *vmx,
>>>> +		unsigned long entry, unsigned long exit,
>>>> +		unsigned long guest_val_vmcs, unsigned long host_val_vmcs,
>>>> +		u64 guest_val, u64 host_val)
>>>>  {
>>>>  	vmcs_write64(guest_val_vmcs, guest_val);
>>>>  	vmcs_write64(host_val_vmcs, host_val);
>>>> -	vmcs_set_bits(VM_ENTRY_CONTROLS, entry);
>>>> -	vmcs_set_bits(VM_EXIT_CONTROLS, exit);
>>>> +	vm_entry_controls_setbit(vmx, entry);
>>>> +	vm_exit_controls_setbit(vmx, exit);
>>>>  }
>>>>  
>>>>  static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
>>>> @@ -1472,7 +1532,8 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
>>>>  	switch (msr) {
>>>>  	case MSR_EFER:
>>>>  		if (cpu_has_load_ia32_efer) {
>>>> -			add_atomic_switch_msr_special(VM_ENTRY_LOAD_IA32_EFER,
>>>> +			add_atomic_switch_msr_special(vmx,
>>>> +					VM_ENTRY_LOAD_IA32_EFER,
>>>>  					VM_EXIT_LOAD_IA32_EFER,
>>>>  					GUEST_IA32_EFER,
>>>>  					HOST_IA32_EFER,
>>>> @@ -1482,7 +1543,7 @@ static void add_atomic_switch_msr(struct vcpu_vmx *vmx, unsigned msr,
>>>>  		break;
>>>>  	case MSR_CORE_PERF_GLOBAL_CTRL:
>>>>  		if (cpu_has_load_perf_global_ctrl) {
>>>> -			add_atomic_switch_msr_special(
>>>> +			add_atomic_switch_msr_special(vmx,
>>>>  					VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
>>>>  					VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL,
>>>>  					GUEST_IA32_PERF_GLOBAL_CTRL,
>>>> @@ -3182,14 +3243,10 @@ static void vmx_set_efer(struct kvm_vcpu *vcpu, u64 efer)
>>>>  	vmx_load_host_state(to_vmx(vcpu));
>>>>  	vcpu->arch.efer = efer;
>>>>  	if (efer & EFER_LMA) {
>>>> -		vmcs_write32(VM_ENTRY_CONTROLS,
>>>> -			     vmcs_read32(VM_ENTRY_CONTROLS) |
>>>> -			     VM_ENTRY_IA32E_MODE);
>>>> +		vm_entry_controls_setbit(to_vmx(vcpu), VM_ENTRY_IA32E_MODE);
>>>>  		msr->data = efer;
>>>>  	} else {
>>>> -		vmcs_write32(VM_ENTRY_CONTROLS,
>>>> -			     vmcs_read32(VM_ENTRY_CONTROLS) &
>>>> -			     ~VM_ENTRY_IA32E_MODE);
>>>> +		vm_entry_controls_clearbit(to_vmx(vcpu), VM_ENTRY_IA32E_MODE);
>>>>  
>>>>  		msr->data = efer & ~EFER_LME;
>>>>  	}
>>>> @@ -3217,9 +3274,7 @@ static void enter_lmode(struct kvm_vcpu *vcpu)
>>>>  
>>>>  static void exit_lmode(struct kvm_vcpu *vcpu)
>>>>  {
>>>> -	vmcs_write32(VM_ENTRY_CONTROLS,
>>>> -		     vmcs_read32(VM_ENTRY_CONTROLS)
>>>> -		     & ~VM_ENTRY_IA32E_MODE);
>>>> +	vm_entry_controls_clearbit(to_vmx(vcpu), VM_ENTRY_IA32E_MODE);
>>>>  	vmx_set_efer(vcpu, vcpu->arch.efer & ~EFER_LMA);
>>>>  }
>>>>  
>>>> @@ -4346,10 +4401,11 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
>>>>  		++vmx->nmsrs;
>>>>  	}
>>>>  
>>>> -	vmcs_write32(VM_EXIT_CONTROLS, vmcs_config.vmexit_ctrl);
>>>> +
>>>> +	vm_exit_controls_init(vmx, vmcs_config.vmexit_ctrl);
>>>>  
>>>>  	/* 22.2.1, 20.8.1 */
>>>> -	vmcs_write32(VM_ENTRY_CONTROLS, vmcs_config.vmentry_ctrl);
>>>> +	vm_entry_controls_init(vmx, vmcs_config.vmentry_ctrl);
>>>>  
>>>>  	vmcs_writel(CR0_GUEST_HOST_MASK, ~0UL);
>>>>  	set_cr4_guest_host_mask(vmx);
>>>> @@ -7759,12 +7815,12 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>>>>  	exit_control = vmcs_config.vmexit_ctrl;
>>>>  	if (vmcs12->pin_based_vm_exec_control & PIN_BASED_VMX_PREEMPTION_TIMER)
>>>>  		exit_control |= VM_EXIT_SAVE_VMX_PREEMPTION_TIMER;
>>>> -	vmcs_write32(VM_EXIT_CONTROLS, exit_control);
>>>> +	vm_exit_controls_init(vmx, exit_control);
>>>>  
>>>>  	/* vmcs12's VM_ENTRY_LOAD_IA32_EFER and VM_ENTRY_IA32E_MODE are
>>>>  	 * emulated by vmx_set_efer(), below.
>>>>  	 */
>>>> -	vmcs_write32(VM_ENTRY_CONTROLS,
>>>> +	vm_entry_controls_init(vmx, 
>>>>  		(vmcs12->vm_entry_controls & ~VM_ENTRY_LOAD_IA32_EFER &
>>>>  			~VM_ENTRY_IA32E_MODE) |
>>>>  		(vmcs_config.vmentry_ctrl & ~VM_ENTRY_IA32E_MODE));
>>>> @@ -8186,7 +8242,7 @@ static void prepare_vmcs12(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
>>>>  
>>>>  	vmcs12->vm_entry_controls =
>>>>  		(vmcs12->vm_entry_controls & ~VM_ENTRY_IA32E_MODE) |
>>>> -		(vmcs_read32(VM_ENTRY_CONTROLS) & VM_ENTRY_IA32E_MODE);
>>>> +		(vm_entry_controls_get(to_vmx(vcpu)) & VM_ENTRY_IA32E_MODE);
>>>>  
>>>>  	/* TODO: These cannot have changed unless we have MSR bitmaps and
>>>>  	 * the relevant bit asks not to trap the change */
>>>> @@ -8390,6 +8446,7 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu)
>>>>  	vcpu->cpu = cpu;
>>>>  	put_cpu();
>>>>  
>>>> +	vm_entry_controls_init(vmx, vmcs_read32(VM_ENTRY_CONTROLS));
>>>>  	vmx_segment_cache_clear(vmx);
>>>>  
>>>>  	/* if no vmcs02 cache requested, remove the one we used */
>>>> --
>>>> 			Gleb.
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>> Applied to kvm/queue, thanks.
>>
>> Seems to have or cause an issue with nVMX. The (still not merged) vmx
>> unit test breaks here after apply this commit:
>>
> What about this patch?
> 
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 1d95384..f90320b 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8452,6 +8452,7 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu)
>  	put_cpu();
>  
>  	vm_entry_controls_init(vmx, vmcs_read32(VM_ENTRY_CONTROLS));
> +	vm_exit_controls_init(vmx, vmcs_read32(VM_EXIT_CONTROLS));
>  	vmx_segment_cache_clear(vmx);
>  
>  	/* if no vmcs02 cache requested, remove the one we used */
> --
> 			Gleb.

Squashed in the first patch, and merged to kvm-next.

Paolo

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 1d95384..f90320b 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8452,6 +8452,7 @@  static void nested_vmx_vmexit(struct kvm_vcpu *vcpu)
 	put_cpu();
 
 	vm_entry_controls_init(vmx, vmcs_read32(VM_ENTRY_CONTROLS));
+	vm_exit_controls_init(vmx, vmcs_read32(VM_EXIT_CONTROLS));
 	vmx_segment_cache_clear(vmx);
 
 	/* if no vmcs02 cache requested, remove the one we used */