Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

[Bandan Das <bsd@redhat.com>]

Marcelo Tosatti <mtosatti@redhat.com> writes:

> On Mon, Mar 31, 2014 at 05:00:23PM -0400, Bandan Das wrote:
>> For single context invalidation, we fall through to global
>> invalidation in handle_invept() except for one case - when
>> the operand supplied by L1 is different from what we have in
>> vmcs12. However, typically hypervisors will only call invept
>> for the currently loaded eptp, so the condition will
>> never be true.
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>
> Bandan,
>
> Why not fix INVEPT single-context rather than removing it entirely?
>
> "Single-context. If the INVEPT type is 1, the logical processor
> invalidates all guest-physical mappings and combined mappings associated
> with the EP4TA specified in the INVEPT descriptor. Combined mappings for
> that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction
> may invalidate mappings associated with other EP4TAs.)"
>
> So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.

The single context invalidation in handle_invept() doesn't do 
anything different. It just falls down to the global case.
And the invept code in Xen and KVM both seemed to fall back
to global invalidation if support for single context wasn't found.
So, it was proposed not to advertise it at all.

But rethinking this again, I agree with you. If there's a hypervisor
with a  single context invept implmentation that does not fallback,
this will unfortunately not work. Jan, do you agree with this ?

Bandan

>> ---
>>  arch/x86/kvm/vmx.c | 15 +++++----------
>>  1 file changed, 5 insertions(+), 10 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 3927528..3e7f60c 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2331,12 +2331,11 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>>  			 VMX_EPT_INVEPT_BIT;
>>  		nested_vmx_ept_caps &= vmx_capability.ept;
>>  		/*
>> -		 * Since invept is completely emulated we support both global
>> -		 * and context invalidation independent of what host cpu
>> -		 * supports
>> +		 * For nested guests, we don't do anything specific
>> +		 * for single context invalidation. Hence, only advertise
>> +		 * support for global context invalidation.
>>  		 */
>> -		nested_vmx_ept_caps |= VMX_EPT_EXTENT_GLOBAL_BIT |
>> -			VMX_EPT_EXTENT_CONTEXT_BIT;
>> +		nested_vmx_ept_caps |= VMX_EPT_EXTENT_GLOBAL_BIT;
>>  	} else
>>  		nested_vmx_ept_caps = 0;
>>  
>> @@ -6383,7 +6382,6 @@ static int handle_invept(struct kvm_vcpu *vcpu)
>>  	struct {
>>  		u64 eptp, gpa;
>>  	} operand;
>> -	u64 eptp_mask = ((1ull << 51) - 1) & PAGE_MASK;
>>  
>>  	if (!(nested_vmx_secondary_ctls_high & SECONDARY_EXEC_ENABLE_EPT) ||
>>  	    !(nested_vmx_ept_caps & VMX_EPT_INVEPT_BIT)) {
>> @@ -6423,16 +6421,13 @@ static int handle_invept(struct kvm_vcpu *vcpu)
>>  	}
>>  
>>  	switch (type) {
>> -	case VMX_EPT_EXTENT_CONTEXT:
>> -		if ((operand.eptp & eptp_mask) !=
>> -				(nested_ept_get_cr3(vcpu) & eptp_mask))
>> -			break;
>>  	case VMX_EPT_EXTENT_GLOBAL:
>>  		kvm_mmu_sync_roots(vcpu);
>>  		kvm_mmu_flush_tlb(vcpu);
>>  		nested_vmx_succeed(vcpu);
>>  		break;
>>  	default:
>> +		/* Trap single context invalidation invept calls */
>>  		BUG_ON(1);
>>  		break;
>>  	}
>> -- 
>> 1.8.3.1
>> 
>> --
>> 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