Re: [PATCH 07/10] KVM: VMX: Introduce test mode related to EPT violation VE

[Chao Gao]

> 
>-	if (cpu_has_secondary_exec_ctrls())
>+	if (cpu_has_secondary_exec_ctrls()) {
> 		secondary_exec_controls_set(vmx, vmx_secondary_exec_control(vmx));
>+		if (secondary_exec_controls_get(vmx) &
>+		    SECONDARY_EXEC_EPT_VIOLATION_VE) {
>+			if (!vmx->ve_info) {

how about allocating ve_info in vmx_vcpu_create()? It is better to me because:

a. symmetry. ve_info is free'd in vmx_vcpu_free().
b. no need to check if this is the first call of init_vmcs(). and ENOMEM can
be returned on allocation failure.

>+				/* ve_info must be page aligned. */
>+				struct page *page;
>+
>+				BUILD_BUG_ON(sizeof(*vmx->ve_info) > PAGE_SIZE);
>+				page = alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
>+				if (page)
>+					vmx->ve_info = page_to_virt(page);
>+			}
>+			if (vmx->ve_info) {
>+				/*
>+				 * Allow #VE delivery. CPU sets this field to
>+				 * 0xFFFFFFFF on #VE delivery.  Another #VE can
>+				 * occur only if software clears the field.
>+				 */
>+				vmx->ve_info->delivery = 0;

Is it necessary to reset ve_info->delivery to 0 given __GFP_ZERO?

>+				vmcs_write64(VE_INFORMATION_ADDRESS,
>+					     __pa(vmx->ve_info));

I think the logic here should just be:

		if (secondary_exec_controls_get(vmx) & SECONDARY_EXEC_EPT_VIOLATION_VE)
			vmcs_write64(VE_INFORMATION_ADDRESS, __pa(vmx->ve_info));

>+			} else {
>+				/*
>+				 * Because SECONDARY_EXEC_EPT_VIOLATION_VE is
>+				 * used only for debugging, it's okay to leave
>+				 * it disabled.
>+				 */
>+				pr_err("Failed to allocate ve_info. disabling EPT_VIOLATION_VE.\n");
>+				secondary_exec_controls_clearbit(vmx,
>+								 SECONDARY_EXEC_EPT_VIOLATION_VE);
>+			}
>+		}
>+	}
> 
> 	if (cpu_has_tertiary_exec_ctrls())
> 		tertiary_exec_controls_set(vmx, vmx_tertiary_exec_control(vmx));
>@@ -5200,6 +5243,12 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
> 	if (is_invalid_opcode(intr_info))
> 		return handle_ud(vcpu);
> 
>+	/*
>+	 * #VE isn't supposed to happen.  Block the VM if it does.
>+	 */
>+	if (KVM_BUG_ON(is_ve_fault(intr_info), vcpu->kvm))
>+		return -EIO;
>+
> 	error_code = 0;
> 	if (intr_info & INTR_INFO_DELIVER_CODE_MASK)
> 		error_code = vmcs_read32(VM_EXIT_INTR_ERROR_CODE);
>@@ -7474,6 +7523,8 @@ void vmx_vcpu_free(struct kvm_vcpu *vcpu)
> 	free_vpid(vmx->vpid);
> 	nested_vmx_free_vcpu(vcpu);
> 	free_loaded_vmcs(vmx->loaded_vmcs);
>+	if (vmx->ve_info)
>+		free_page((unsigned long)vmx->ve_info);
> }
> 
> int vmx_vcpu_create(struct kvm_vcpu *vcpu)
>diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
>index 65786dbe7d60..0da79a386825 100644
>--- a/arch/x86/kvm/vmx/vmx.h
>+++ b/arch/x86/kvm/vmx/vmx.h
>@@ -362,6 +362,9 @@ struct vcpu_vmx {
> 		DECLARE_BITMAP(read, MAX_POSSIBLE_PASSTHROUGH_MSRS);
> 		DECLARE_BITMAP(write, MAX_POSSIBLE_PASSTHROUGH_MSRS);
> 	} shadow_msr_intercept;
>+
>+	/* ve_info must be page aligned. */

this comment is not so useful. I think this should be placed above the call
of alloc_page().

>+	struct vmx_ve_information *ve_info;
> };
> 
> struct kvm_vmx {
>@@ -574,7 +577,8 @@ static inline u8 vmx_get_rvi(void)
> 	 SECONDARY_EXEC_ENABLE_VMFUNC |					\
> 	 SECONDARY_EXEC_BUS_LOCK_DETECTION |				\
> 	 SECONDARY_EXEC_NOTIFY_VM_EXITING |				\
>-	 SECONDARY_EXEC_ENCLS_EXITING)
>+	 SECONDARY_EXEC_ENCLS_EXITING |					\
>+	 SECONDARY_EXEC_EPT_VIOLATION_VE)
> 
> #define KVM_REQUIRED_VMX_TERTIARY_VM_EXEC_CONTROL 0
> #define KVM_OPTIONAL_VMX_TERTIARY_VM_EXEC_CONTROL			\
>-- 
>2.43.0
>
>
>