Jan Kiszka <jan.kiszka@xxxxxx> writes:
> On 2014-03-20 21:58, Bandan Das wrote:
>> Jan Kiszka <jan.kiszka@xxxxxxxxxxx> 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@xxxxxxxxxx>
>>>> ---
>>>> 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);
>>>>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
