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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]

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.

> 
> 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.

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);
>>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]