Received-SPF: pass (google.com: domain of linux-kernel+bounces-149297-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 17 Apr 2024 16:00:19 -0700
In-Reply-To: <20240416201935.3525739-8-pbonzini@redhat.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240416201935.3525739-1-pbonzini@redhat.com> <20240416201935.3525739-8-pbonzini@redhat.com>
Message-ID: <ZiBUg-81eIPUAy9P@google.com>
Subject: Re: [PATCH v2 07/10] KVM: VMX: Introduce test mode related to EPT
 violation VE
From: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, 
	isaku.yamahata@intel.com, xiaoyao.li@intel.com, binbin.wu@linux.intel.com, 
	chao.gao@intel.com
Content-Type: text/plain; charset="us-ascii"

On Tue, Apr 16, 2024, Paolo Bonzini wrote:
> @@ -4711,8 +4722,21 @@ static void init_vmcs(struct vcpu_vmx *vmx)
>  
>  	exec_controls_set(vmx, vmx_exec_control(vmx));
>  
> -	if (cpu_has_secondary_exec_ctrls())
> +	if (cpu_has_secondary_exec_ctrls()) {
>  		secondary_exec_controls_set(vmx, vmx_secondary_exec_control(vmx));
> +		if (vmx->ve_info) {
> +			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.
> +			 */
> +			secondary_exec_controls_clearbit(vmx,
> +							 SECONDARY_EXEC_EPT_VIOLATION_VE);

As below, this is silly.

> +		}
> +	}
>  
>  	if (cpu_has_tertiary_exec_ctrls())
>  		tertiary_exec_controls_set(vmx, vmx_tertiary_exec_control(vmx));
> @@ -5200,6 +5224,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.
> +	 */

Doesn't need to be a multi-line comment.  Though I would just drop the comment,
the KVM_BUG_ON() makes it pretty darn clear #VE is unexpected.

> +	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 +7504,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() handles '0', though hopefully this becomes a moot point.

> +		free_page((unsigned long)vmx->ve_info);
>  }
>  
>  int vmx_vcpu_create(struct kvm_vcpu *vcpu)
> @@ -7567,6 +7599,19 @@ int vmx_vcpu_create(struct kvm_vcpu *vcpu)
>  			goto free_vmcs;
>  	}
>  
> +	if (vmcs_config.cpu_based_2nd_exec_ctrl & SECONDARY_EXEC_EPT_VIOLATION_VE) {
> +		struct page *page;
> +
> +		BUILD_BUG_ON(sizeof(*vmx->ve_info) > PAGE_SIZE);
> +
> +		/* ve_info must be page aligned. */
> +		page = alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO);
> +		if (page)

Can we please just treat this as an error.  The odds of us screwing up checks
against vmx->ve_info are higher than the odds of someone enabling KVM_INTEL_PROVE_VE
on a machine with such high memory pressure that a 4KiB allocation fails, all
subequent memory allocations succeeding, *and* caring that VM creation fails.

The pr_err() in the failure path is even more ridiculous.

> +			vmx->ve_info = page_to_virt(page);
> +		else
> +			pr_err("Failed to allocate ve_info. disabling EPT_VIOLATION_VE.\n");
> +	}
> +
>  	if (vmx_can_use_ipiv(vcpu))
>  		WRITE_ONCE(to_kvm_vmx(vcpu->kvm)->pid_table[vcpu->vcpu_id],
>  			   __pa(&vmx->pi_desc) | PID_TABLE_ENTRY_VALID);