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

[Jan Kiszka]

On 2014-03-20 21:58, Bandan Das wrote:
> Jan Kiszka <jan.kis...@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 <b...@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);
>>>

signature.asc
Description: OpenPGP digital signature