Re: [PATCH v6 10/15] nEPT: Nested INVEPT

[Gleb Natapov <gleb@redhat.com>]

On Fri, Aug 02, 2013 at 04:06:00PM +0800, Xiao Guangrong wrote:
> On 08/01/2013 10:08 PM, Gleb Natapov wrote:
> 
> > +/* Emulate the INVEPT instruction */
> > +static int handle_invept(struct kvm_vcpu *vcpu)
> > +{
> > +	u32 vmx_instruction_info;
> > +	bool ok;
> > +	unsigned long type;
> > +	gva_t gva;
> > +	struct x86_exception e;
> > +	struct {
> > +		u64 eptp, gpa;
> > +	} operand;
> > +
> > +	if (!(nested_vmx_secondary_ctls_high & SECONDARY_EXEC_ENABLE_EPT) ||
> > +	    !(nested_vmx_ept_caps & VMX_EPT_INVEPT_BIT)) {
> > +		kvm_queue_exception(vcpu, UD_VECTOR);
> > +		return 1;
> > +	}
> > +
> > +	if (!nested_vmx_check_permission(vcpu))
> > +		return 1;
> > +
> > +	if (!kvm_read_cr0_bits(vcpu, X86_CR0_PE)) {
> > +		kvm_queue_exception(vcpu, UD_VECTOR);
> > +		return 1;
> > +	}
> > +
> > +	/* According to the Intel VMX instruction reference, the memory
> > +	 * operand is read even if it isn't needed (e.g., for type==global)
> > +	 */
> > +	vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
> > +	if (get_vmx_mem_address(vcpu, vmcs_readl(EXIT_QUALIFICATION),
> > +			vmx_instruction_info, &gva))
> > +		return 1;
> > +	if (kvm_read_guest_virt(&vcpu->arch.emulate_ctxt, gva, &operand,
> > +				sizeof(operand), &e)) {
> > +		kvm_inject_page_fault(vcpu, &e);
> > +		return 1;
> > +	}
> > +
> > +	type = kvm_register_read(vcpu, (vmx_instruction_info >> 28) & 0xf);
> > +
> > +	switch (type) {
> > +	case VMX_EPT_EXTENT_GLOBAL:
> > +	case VMX_EPT_EXTENT_CONTEXT:
> > +		ok = !!(nested_vmx_ept_caps &
> > +				(1UL << (type + VMX_EPT_EXTENT_SHIFT)));
> > +		break;
> > +	default:
> > +		ok = false;
> > +	}
> > +
> > +	if (ok) {
> > +		kvm_mmu_sync_roots(vcpu);
> > +		kvm_mmu_flush_tlb(vcpu);
> > +		nested_vmx_succeed(vcpu);
> > +	}
> > +	else
> > +		nested_vmx_failValid(vcpu,
> > +				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> > +
> > +	skip_emulated_instruction(vcpu);
> > +	return 1;
> 
> Can this code be changed to:
> 
> 	switch (type) {
> 	case VMX_EPT_EXTENT_GLOBAL:
> 	case VMX_EPT_EXTENT_CONTEXT:
> 		if (nested_vmx_ept_caps &
> 				(1UL << (type + VMX_EPT_EXTENT_SHIFT) {
> 			kvm_mmu_sync_roots(vcpu);
> 			kvm_mmu_flush_tlb(vcpu);
> 			nested_vmx_succeed(vcpu);
> 			break;
> 		}
> 	default:
> 		nested_vmx_failValid(vcpu,
> 				VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
> 	}
> ?
> That's clearer i think.
> 
> Also, we can sync the shadow page table only if the current eptp is the required
> eptp, that means:
> 
> if (type == GLOBAL || operand.eptp == nested_ept_get_cr3(vcpu)) {
> 	kvm_mmu_sync_roots(vcpu);
> 	......
> }
Good point. Will do.

--
			Gleb.