[PATCH] KVM/VMX: Check ept_pointer before flushing ept tlb

[PATCH] KVM/VMX: Check ept_pointer before flushing ept tlb

[ltykernel]

From: Lan Tianyu <Tianyu.Lan@microsoft.com>

This patch is to initialize ept_pointer to INVALID_PAGE and check it
before flushing ept tlb. If ept_pointer is invalidated, bypass the flush
request.

Signed-off-by: Lan Tianyu <Tianyu.Lan@microsoft.com>
---
 arch/x86/kvm/vmx.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4555077d69ce..edbc96cb990a 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1580,14 +1580,22 @@ static int vmx_hv_remote_flush_tlb(struct kvm *kvm)
 	/*
 	 * FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE hypercall needs the address of the
 	 * base of EPT PML4 table, strip off EPT configuration information.
+	 * If ept_pointer is invalid pointer, bypass the flush request.
 	 */
 	if (to_kvm_vmx(kvm)->ept_pointers_match != EPT_POINTERS_MATCH) {
-		kvm_for_each_vcpu(i, vcpu, kvm)
+		kvm_for_each_vcpu(i, vcpu, kvm) {
+			if (!VALID_PAGE(to_vmx(vcpu)->ept_pointer))
+				return 0;
+
 			ret |= hyperv_flush_guest_mapping(
-				to_vmx(kvm_get_vcpu(kvm, i))->ept_pointer & PAGE_MASK);
+				to_vmx(vcpu)->ept_pointer & PAGE_MASK);
+		}
 	} else {
+		if (!VALID_PAGE(to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer))
+			return 0;
+
 		ret = hyperv_flush_guest_mapping(
-				to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer & PAGE_MASK);
+			to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer & PAGE_MASK);
 	}
 
 	spin_unlock(&to_kvm_vmx(kvm)->ept_pointer_lock);
@@ -11568,6 +11576,8 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
 	vmx->pi_desc.nv = POSTED_INTR_VECTOR;
 	vmx->pi_desc.sn = 1;
 
+	vmx->ept_pointer = INVALID_PAGE;
+
 	return &vmx->vcpu;
 
 free_vmcs:
-- 
2.14.4

Re: [PATCH] KVM/VMX: Check ept_pointer before flushing ept tlb

[Vitaly Kuznetsov]

ltykernel@gmail.com writes:

> From: Lan Tianyu <Tianyu.Lan@microsoft.com>
>
> This patch is to initialize ept_pointer to INVALID_PAGE and check it
> before flushing ept tlb. If ept_pointer is invalidated, bypass the flush
> request.
>
> Signed-off-by: Lan Tianyu <Tianyu.Lan@microsoft.com>
> ---
>  arch/x86/kvm/vmx.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4555077d69ce..edbc96cb990a 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1580,14 +1580,22 @@ static int vmx_hv_remote_flush_tlb(struct kvm *kvm)
>  	/*
>  	 * FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE hypercall needs the address of the
>  	 * base of EPT PML4 table, strip off EPT configuration information.
> +	 * If ept_pointer is invalid pointer, bypass the flush request.
>  	 */
>  	if (to_kvm_vmx(kvm)->ept_pointers_match != EPT_POINTERS_MATCH) {
> -		kvm_for_each_vcpu(i, vcpu, kvm)
> +		kvm_for_each_vcpu(i, vcpu, kvm) {
> +			if (!VALID_PAGE(to_vmx(vcpu)->ept_pointer))
> +				return 0;
> +

To be honest I fail to understand the reason behind the patch: instead
of doing one unneeded flush request with ept_pointer==0 (after vCPU is
initialized) we now do the check every time. Could you please elaborate
on why this is needed?


>  			ret |= hyperv_flush_guest_mapping(
> -				to_vmx(kvm_get_vcpu(kvm, i))->ept_pointer & PAGE_MASK);
> +				to_vmx(vcpu)->ept_pointer & PAGE_MASK);

I would use a local variable for 'to_vmx(vcpu)->ept_pointer' or even
'to_vmx(vcpu)->ept_pointer & PAGE_MASK' and use it in VALID_PAGE() - as
lower bits are unrelated;


> +		}
>  	} else {
> +		if (!VALID_PAGE(to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer))
> +			return 0;

Ditto.

> +
>  		ret = hyperv_flush_guest_mapping(
> -				to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer & PAGE_MASK);
> +			to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer & PAGE_MASK);

This doesn't belong to this patch.

>  	}
>  
>  	spin_unlock(&to_kvm_vmx(kvm)->ept_pointer_lock);
> @@ -11568,6 +11576,8 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>  	vmx->pi_desc.nv = POSTED_INTR_VECTOR;
>  	vmx->pi_desc.sn = 1;
>  
> +	vmx->ept_pointer = INVALID_PAGE;
> +
>  	return &vmx->vcpu;
>  
>  free_vmcs:

-- 
Vitaly

Re: [PATCH] KVM/VMX: Check ept_pointer before flushing ept tlb

[Tianyu Lan]

Hi Vitaly:
	Thanks for your review.

On 11/6/2018 11:50 PM, Vitaly Kuznetsov wrote:
> ltykernel@gmail.com writes:
> 
>> From: Lan Tianyu <Tianyu.Lan@microsoft.com>
>>
>> This patch is to initialize ept_pointer to INVALID_PAGE and check it
>> before flushing ept tlb. If ept_pointer is invalidated, bypass the flush
>> request.
>>
>> Signed-off-by: Lan Tianyu <Tianyu.Lan@microsoft.com>
>> ---
>>   arch/x86/kvm/vmx.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 4555077d69ce..edbc96cb990a 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -1580,14 +1580,22 @@ static int vmx_hv_remote_flush_tlb(struct kvm *kvm)
>>   	/*
>>   	 * FLUSH_GUEST_PHYSICAL_ADDRESS_SPACE hypercall needs the address of the
>>   	 * base of EPT PML4 table, strip off EPT configuration information.
>> +	 * If ept_pointer is invalid pointer, bypass the flush request.
>>   	 */
>>   	if (to_kvm_vmx(kvm)->ept_pointers_match != EPT_POINTERS_MATCH) {
>> -		kvm_for_each_vcpu(i, vcpu, kvm)
>> +		kvm_for_each_vcpu(i, vcpu, kvm) {
>> +			if (!VALID_PAGE(to_vmx(vcpu)->ept_pointer))
>> +				return 0;
>> +
> 
> To be honest I fail to understand the reason behind the patch: instead
> of doing one unneeded flush request with ept_pointer==0 (after vCPU is
> initialized) we now do the check every time. Could you please elaborate
> on why this is needed?

The reason to introduce the check here is to avoid flushing ept tlb
without valid ept table. When nested guest boots up and only BP is
active, we should not do flush for APs and L1 hypervisor hasn't set
valid EPT table for APs.

> 
>>   			ret |= hyperv_flush_guest_mapping(
>> -				to_vmx(kvm_get_vcpu(kvm, i))->ept_pointer & PAGE_MASK);
>> +				to_vmx(vcpu)->ept_pointer & PAGE_MASK);
> 
> I would use a local variable for 'to_vmx(vcpu)->ept_pointer' or even
> 'to_vmx(vcpu)->ept_pointer & PAGE_MASK' and use it in VALID_PAGE() - as
> lower bits are unrelated;

Yes, that makes sense. INVALID_PAGE also contains lower bits and so a 
local variable for 'to_vmx(vcpu)->ept_pointer' maybe better.


> 
> 
>> +		}
>>   	} else {
>> +		if (!VALID_PAGE(to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer))
>> +			return 0;
> 
> Ditto.
> 
>> +
>>   		ret = hyperv_flush_guest_mapping(
>> -				to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer & PAGE_MASK);
>> +			to_vmx(kvm_get_vcpu(kvm, 0))->ept_pointer & PAGE_MASK);
> 
> This doesn't belong to this patch.

I found the line exceeds 80 chars and so adjust indent. Maybe I should 
change it in a separate patch despite it's a small change.

> 
>>   	}
>>   
>>   	spin_unlock(&to_kvm_vmx(kvm)->ept_pointer_lock);
>> @@ -11568,6 +11576,8 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
>>   	vmx->pi_desc.nv = POSTED_INTR_VECTOR;
>>   	vmx->pi_desc.sn = 1;
>>   
>> +	vmx->ept_pointer = INVALID_PAGE;
>> +
>>   	return &vmx->vcpu;
>>   
>>   free_vmcs:
> 

Re: [PATCH] KVM/VMX: Check ept_pointer before flushing ept tlb

[Vitaly Kuznetsov]

Tianyu Lan <ltykernel@gmail.com> writes:

> Hi Vitaly:
> 	Thanks for your review.
>
> On 11/6/2018 11:50 PM, Vitaly Kuznetsov wrote:
>> ltykernel@gmail.com writes:
>> 
>>> From: Lan Tianyu <Tianyu.Lan@microsoft.com>
>>>
>>> This patch is to initialize ept_pointer to INVALID_PAGE and check it
>>> before flushing ept tlb. If ept_pointer is invalidated, bypass the flush
>>> request.
>>>
>> 
>> To be honest I fail to understand the reason behind the patch: instead
>> of doing one unneeded flush request with ept_pointer==0 (after vCPU is
>> initialized) we now do the check every time. Could you please elaborate
>> on why this is needed?
>
> The reason to introduce the check here is to avoid flushing ept tlb
> without valid ept table. When nested guest boots up and only BP is
> active, we should not do flush for APs and L1 hypervisor hasn't set
> valid EPT table for APs.

Yes, I understand that but I'm trying to avoid additional checks on
hotpath as during normal operation EPT pointer is always set.

Could we just initialize ept_pointers_match to something like
EPT_POINTERS_NOTSET and achive the same result?

-- 
Vitaly

Re: [PATCH] KVM/VMX: Check ept_pointer before flushing ept tlb

[Tianyu Lan]

On 11/7/2018 6:49 PM, Vitaly Kuznetsov wrote:
> Tianyu Lan <ltykernel@gmail.com> writes:
> 
>> Hi Vitaly:
>> 	Thanks for your review.
>>
>> On 11/6/2018 11:50 PM, Vitaly Kuznetsov wrote:
>>> ltykernel@gmail.com writes:
>>>
>>>> From: Lan Tianyu <Tianyu.Lan@microsoft.com>
>>>>
>>>> This patch is to initialize ept_pointer to INVALID_PAGE and check it
>>>> before flushing ept tlb. If ept_pointer is invalidated, bypass the flush
>>>> request.
>>>>
>>>
>>> To be honest I fail to understand the reason behind the patch: instead
>>> of doing one unneeded flush request with ept_pointer==0 (after vCPU is
>>> initialized) we now do the check every time. Could you please elaborate
>>> on why this is needed?
>>
>> The reason to introduce the check here is to avoid flushing ept tlb
>> without valid ept table. When nested guest boots up and only BP is
>> active, we should not do flush for APs and L1 hypervisor hasn't set
>> valid EPT table for APs.
> 
> Yes, I understand that but I'm trying to avoid additional checks on
> hotpath as during normal operation EPT pointer is always set.
>
> Could we just initialize ept_pointers_match to something like
> EPT_POINTERS_NOTSET and achive the same result?

vmx->ept_pointers_match presents match status of all vcpus' ept table. 
EPT_POINTER_NOSET should be per cpu status and so I select ept_pointer 
as check condition.

BTW, I think we may remove the check for match case which is normal 
status and all ept pointers should be set at that point. Mismatch status 
should be corner case when VM runs and this will not affect a lot.