Re: [PATCH 3/3] KVM: nVMX: check for null vmcs12 when L1 does invept

[Bandan Das <bsd@redhat.com>]

Jan Kiszka <jan.kiszka@web.de> writes:

> On 2014-03-20 21:58, Bandan Das wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>> 
>>> On 2014-03-20 04:28, Bandan Das wrote:
>>>> Some L1 hypervisors such as Xen seem to be calling invept after
>>>> vmclear or before vmptrld on L2. In this case, proceed with
>>>> falling through and syncing roots as a case where
>>>> context wide invalidation can't be supported
>>>
>>> Can we also base this behaviour on a statement in the SDM? But on first
>>> glance, I do not find anything like this over there.
>> 
>> The SDM has nothing of this sort explicitly mentioned but 28.3.3.1 
>> "Operations that invalidate Cached Mappings" does mention that
>> the instruction may invalidate mappings associated with other
>> EP4TAs (even in single context).
>
> Yes, "may". So we are implementing undefined behavior in order to please
> a broken hypervisor that relies on it? Then please state this in the
> patch and probably also inform Xen about their issue.

Why undefined behavior ? We don't do anything specific for 
the single context invalidation case ianyway .e If the eptp matches what 
vmcs12 has, single context invalidation does fall though to the global 
invalidation case already. All this change does is add the "L1 calls 
invept after vmclear and  before vmptrld" to the list of cases to fall 
though to global invalidation since nvmx doesn't have any knowledge of 
the current eptp for this case.

Or do you think we should rethink this approach ?

>> 
>> Note that I based this on what we currently do for context invalidation -
>> static inline void ept_sync_context(u64 eptp)
>> {
>> 	if (enable_ept) {
>> 		if (cpu_has_vmx_invept_context())
>> 			__invept(VMX_EPT_EXTENT_CONTEXT, eptp, 0);
>> 		else
>> 			ept_sync_global();
>> 	}
>> }
>
> Don't get your point. This test is about testing for the CPU support
> context invalidating, then falling back to global invalidation if there
> is no support.

Sorry, if this was confusing. All I was trying to say is switching to global
invalidation if we can't do single context invalidation for some reason 
is not unusual.

Thanks,
Bandan

> Jan
>
>> 
>> Seemed easier and cleaner than having a cached eptp after vmcs12 is 
>> long gone :)
>> 
>> If you prefer, I can modify the commit message to reflect this.
>> 
>>> Jan
>>>
>>>>
>>>> Signed-off-by: Bandan Das <bsd@redhat.com>
>>>> ---
>>>>  arch/x86/kvm/vmx.c | 5 +++--
>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>> index c707389..b407b3a 100644
>>>> --- a/arch/x86/kvm/vmx.c
>>>> +++ b/arch/x86/kvm/vmx.c
>>>> @@ -6435,8 +6435,9 @@ 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))
>>>> +		if (get_vmcs12(vcpu) &&
>>>> +		    ((operand.eptp & eptp_mask) != (nested_ept_get_cr3(vcpu) &
>>>> +						    eptp_mask)))
>>>>  			break;
>>>>  	case VMX_EPT_EXTENT_GLOBAL:
>>>>  		kvm_mmu_sync_roots(vcpu);
>>>>