Messages in this thread | | | Date | Wed, 17 Apr 2024 16:00:19 -0700 | Subject | Re: [PATCH v2 07/10] KVM: VMX: Introduce test mode related to EPT violation VE | From | Sean Christopherson <> |
| |
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);
| |