diff mbox

[8/9] KVM: VMX: introduce set_clear_2nd_exec_ctrl()

Message ID 1440132611-26052-9-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Xiao Guangrong Aug. 21, 2015, 4:50 a.m. UTC 
It's used to clean up the code

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 arch/x86/kvm/vmx.c | 42 +++++++++++++++++++-----------------------
 1 file changed, 19 insertions(+), 23 deletions(-)

Comments

Paolo Bonzini Sept. 7, 2015, 11:27 a.m. UTC | #1 
On 21/08/2015 06:50, Xiao Guangrong wrote:
>  
> +static void set_clear_2nd_exec_ctrl(u32 ctrls, bool set)
> +{
> +	u32 exec_ctrl = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
> +
> +	if (set)
> +		exec_ctrl |= ctrls;
> +	else
> +		exec_ctrl &= ~ctrls;
> +
> +	vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_ctrl);
> +}

The second argument is always true.  Do you have any plans for it?

Should we instead add functions like vmcs_or32 and vmcs_clear32?

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
Xiao Guangrong Sept. 8, 2015, 2:24 p.m. UTC | #2 
On 09/07/2015 07:27 PM, Paolo Bonzini wrote:
>
>
> On 21/08/2015 06:50, Xiao Guangrong wrote:
>>
>> +static void set_clear_2nd_exec_ctrl(u32 ctrls, bool set)
>> +{
>> +	u32 exec_ctrl = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
>> +
>> +	if (set)
>> +		exec_ctrl |= ctrls;
>> +	else
>> +		exec_ctrl &= ~ctrls;
>> +
>> +	vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_ctrl);
>> +}
>
> The second argument is always true.

No...

There are 3 places calling this function with set=false?
nested_release_vmcs12(), vmx_disable_pml() and
vmx_cpuid_update()


> Do you have any plans for it?
>
> Should we instead add functions like vmcs_or32 and vmcs_clear32?
>

Sounds good to me, will do it.
--
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
Paolo Bonzini Sept. 8, 2015, 8:32 p.m. UTC | #3 
On 08/09/2015 16:24, Xiao Guangrong wrote:
>>
>> The second argument is always true.
> 
> No...
> 
> There are 3 places calling this function with set=false?
> nested_release_vmcs12(), vmx_disable_pml() and
> vmx_cpuid_update()

You're right.  It's always constant---I don't know why I wrote it's
always true, and then suggested vmcs_clear32...

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 4f238b7..58f7b89 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4589,6 +4589,18 @@  static void ept_set_mmio_spte_mask(void)
 	kvm_mmu_set_mmio_spte_mask((0x3ull << 62) | 0x6ull);
 }
 
+static void set_clear_2nd_exec_ctrl(u32 ctrls, bool set)
+{
+	u32 exec_ctrl = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
+
+	if (set)
+		exec_ctrl |= ctrls;
+	else
+		exec_ctrl &= ~ctrls;
+
+	vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_ctrl);
+}
+
 #define VMX_XSS_EXIT_BITMAP 0
 /*
  * Sets up the vmcs for emulated real mode.
@@ -6632,7 +6644,6 @@  static int nested_vmx_check_permission(struct kvm_vcpu *vcpu)
 
 static inline void nested_release_vmcs12(struct vcpu_vmx *vmx)
 {
-	u32 exec_control;
 	if (vmx->nested.current_vmptr == -1ull)
 		return;
 
@@ -6645,9 +6656,7 @@  static inline void nested_release_vmcs12(struct vcpu_vmx *vmx)
 		   they were modified */
 		copy_shadow_to_vmcs12(vmx);
 		vmx->nested.sync_shadow_vmcs = false;
-		exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
-		exec_control &= ~SECONDARY_EXEC_SHADOW_VMCS;
-		vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
+		set_clear_2nd_exec_ctrl(SECONDARY_EXEC_SHADOW_VMCS, false);
 		vmcs_write64(VMCS_LINK_POINTER, -1ull);
 	}
 	vmx->nested.posted_intr_nv = -1;
@@ -7043,7 +7052,6 @@  static int handle_vmptrld(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 	gpa_t vmptr;
-	u32 exec_control;
 
 	if (!nested_vmx_check_permission(vcpu))
 		return 1;
@@ -7075,9 +7083,8 @@  static int handle_vmptrld(struct kvm_vcpu *vcpu)
 		vmx->nested.current_vmcs12 = new_vmcs12;
 		vmx->nested.current_vmcs12_page = page;
 		if (enable_shadow_vmcs) {
-			exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
-			exec_control |= SECONDARY_EXEC_SHADOW_VMCS;
-			vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
+			set_clear_2nd_exec_ctrl(SECONDARY_EXEC_SHADOW_VMCS,
+						true);
 			vmcs_write64(VMCS_LINK_POINTER,
 				     __pa(vmx->nested.current_shadow_vmcs));
 			vmx->nested.sync_shadow_vmcs = true;
@@ -7587,7 +7594,6 @@  static void vmx_get_exit_info(struct kvm_vcpu *vcpu, u64 *info1, u64 *info2)
 static int vmx_enable_pml(struct vcpu_vmx *vmx)
 {
 	struct page *pml_pg;
-	u32 exec_control;
 
 	pml_pg = alloc_page(GFP_KERNEL | __GFP_ZERO);
 	if (!pml_pg)
@@ -7598,24 +7604,18 @@  static int vmx_enable_pml(struct vcpu_vmx *vmx)
 	vmcs_write64(PML_ADDRESS, page_to_phys(vmx->pml_pg));
 	vmcs_write16(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
 
-	exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
-	exec_control |= SECONDARY_EXEC_ENABLE_PML;
-	vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
+	set_clear_2nd_exec_ctrl(SECONDARY_EXEC_ENABLE_PML, true);
 
 	return 0;
 }
 
 static void vmx_disable_pml(struct vcpu_vmx *vmx)
 {
-	u32 exec_control;
-
 	ASSERT(vmx->pml_pg);
 	__free_page(vmx->pml_pg);
 	vmx->pml_pg = NULL;
 
-	exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
-	exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
-	vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
+	set_clear_2nd_exec_ctrl(SECONDARY_EXEC_ENABLE_PML, false);
 }
 
 static void vmx_flush_pml_buffer(struct kvm_vcpu *vcpu)
@@ -8689,12 +8689,8 @@  static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
 			best->ebx &= ~bit(X86_FEATURE_INVPCID);
 	}
 
-	if (clear_exe_ctrl) {
-		u32 exec_ctl = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
-
-		exec_ctl &= ~clear_exe_ctrl;
-		vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_ctl);
-	}
+	if (clear_exe_ctrl)
+		set_clear_2nd_exec_ctrl(clear_exe_ctrl, false);
 
 	if (!guest_cpuid_has_pcommit(vcpu) && nested)
 		vmx->nested.nested_vmx_secondary_ctls_high &=