[PATCH 0/4] vmx: miscellaneous cleanups

[PATCH 0/4] vmx: miscellaneous cleanups

[Bandan Das]

These are some very minor vmx cleanups.
2/4 is necessary for nested esx to be able to
successfully launch a guest of its own. Without single context
invalidation present, it just disables ept altogether.

Bandan Das (4):
  nvmx: use warn_on for buggy cases when emulating invept/invvpid
  nvmx: mark ept single context invalidation as supported
  mmu: don't pass *kvm to spte_write_protect
  nvmx: check for shadow vmcs check on entry

 arch/x86/kvm/mmu.c |  4 ++--
 arch/x86/kvm/vmx.c | 23 +++++++++++++----------
 2 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.5.5

[PATCH 1/4] nvmx: use warn_on for buggy cases when emulating invept/invvpid

[Bandan Das]

If L1 hypervisor decides to try out something weird, alert the
host but only less aggressively. Also, remove the comment
regarding nested vpid support since it is no longer valid.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kvm/vmx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 64a79f2..9fd0681 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2854,7 +2854,6 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
 			vmx->nested.nested_vmx_secondary_ctls_high);
 		break;
 	case MSR_IA32_VMX_EPT_VPID_CAP:
-		/* Currently, no nested vpid support */
 		*pdata = vmx->nested.nested_vmx_ept_caps |
 			((u64)vmx->nested.nested_vmx_vpid_caps << 32);
 		break;
@@ -7462,7 +7461,7 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 		break;
 	default:
 		/* Trap single context invalidation invept calls */
-		BUG_ON(1);
+		WARN_ON(1);
 		break;
 	}
 
@@ -7525,7 +7524,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
 		break;
 	default:
 		/* Trap individual address invalidation invvpid calls */
-		BUG_ON(1);
+		WARN_ON(1);
 		break;
 	}
 
-- 
2.5.5

Re: [PATCH 1/4] nvmx: use warn_on for buggy cases when emulating invept/invvpid

[Paolo Bonzini]

On 21/07/2016 00:25, Bandan Das wrote:
> If L1 hypervisor decides to try out something weird, alert the
> host but only less aggressively. Also, remove the comment
> regarding nested vpid support since it is no longer valid.
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  arch/x86/kvm/vmx.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 64a79f2..9fd0681 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2854,7 +2854,6 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
>  			vmx->nested.nested_vmx_secondary_ctls_high);
>  		break;
>  	case MSR_IA32_VMX_EPT_VPID_CAP:
> -		/* Currently, no nested vpid support */

This is okay.

>  		*pdata = vmx->nested.nested_vmx_ept_caps |
>  			((u64)vmx->nested.nested_vmx_vpid_caps << 32);
>  		break;
> @@ -7462,7 +7461,7 @@ static int handle_invept(struct kvm_vcpu *vcpu)
>  		break;
>  	default:
>  		/* Trap single context invalidation invept calls */
> -		BUG_ON(1);
> +		WARN_ON(1);
>  		break;
>  	}
>  
> @@ -7525,7 +7524,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>  		break;
>  	default:
>  		/* Trap individual address invalidation invvpid calls */
> -		BUG_ON(1);
> +		WARN_ON(1);
>  		break;
>  	}
>  
> 

These are BUGs because they are checked above:

        if (!(types & (1UL << type))) {
                nested_vmx_failValid(vcpu,
                                VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
                skip_emulated_instruction(vcpu);
                return 1;
        }

Guest-triggerable WARNs are only just a little better than
guest-triggerable BUGs.  Guest-triggerable messages should be
rate-limited printk.

I don't object to the change, but the commit message should be
modified (or the change dropped).

Paolo

Re: [PATCH 1/4] nvmx: use warn_on for buggy cases when emulating invept/invvpid

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 21/07/2016 00:25, Bandan Das wrote:
>> If L1 hypervisor decides to try out something weird, alert the
>> host but only less aggressively. Also, remove the comment
>> regarding nested vpid support since it is no longer valid.
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  arch/x86/kvm/vmx.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 64a79f2..9fd0681 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2854,7 +2854,6 @@ static int vmx_get_vmx_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
>>  			vmx->nested.nested_vmx_secondary_ctls_high);
>>  		break;
>>  	case MSR_IA32_VMX_EPT_VPID_CAP:
>> -		/* Currently, no nested vpid support */
>
> This is okay.
>
>>  		*pdata = vmx->nested.nested_vmx_ept_caps |
>>  			((u64)vmx->nested.nested_vmx_vpid_caps << 32);
>>  		break;
>> @@ -7462,7 +7461,7 @@ static int handle_invept(struct kvm_vcpu *vcpu)
>>  		break;
>>  	default:
>>  		/* Trap single context invalidation invept calls */
>> -		BUG_ON(1);
>> +		WARN_ON(1);
>>  		break;
>>  	}
>>  
>> @@ -7525,7 +7524,7 @@ static int handle_invvpid(struct kvm_vcpu *vcpu)
>>  		break;
>>  	default:
>>  		/* Trap individual address invalidation invvpid calls */
>> -		BUG_ON(1);
>> +		WARN_ON(1);
>>  		break;
>>  	}
>>  
>> 
>
> These are BUGs because they are checked above:
>
>         if (!(types & (1UL << type))) {
>                 nested_vmx_failValid(vcpu,
>                                 VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID);
>                 skip_emulated_instruction(vcpu);
>                 return 1;
>         }

Ah ok, this should be sufficient I think.

> Guest-triggerable WARNs are only just a little better than
> guest-triggerable BUGs.  Guest-triggerable messages should be

Yeah, a trace isn't really necessary. We know where it's from.
BUG() can also leave the module in an unclean state and prevent
it from getting unloaded which I why I think it shouldn't be on
any path that can be guest triggered.

> rate-limited printk.
>
> I don't object to the change, but the commit message should be
> modified (or the change dropped).

I will drop it and modify the commit message accordingly.

> Paolo

[PATCH 2/4] nvmx: mark ept single context invalidation as supported

[Bandan Das]

Commit 4b855078601f ("KVM: nVMX: Don't advertise single
context invalidation for invept") removed advertising
single context invalidation since the spec does not mandate it.
However, some hypervisors (such as ESX) require it to be present
before willing to use ept in a nested environment. Advertise it
and fallback to the global case.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kvm/vmx.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 9fd0681..6291143 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2718,12 +2718,8 @@ static void nested_vmx_setup_ctls_msrs(struct vcpu_vmx *vmx)
 			 VMX_EPTP_WB_BIT | VMX_EPT_2MB_PAGE_BIT |
 			 VMX_EPT_INVEPT_BIT;
 		vmx->nested.nested_vmx_ept_caps &= vmx_capability.ept;
-		/*
-		 * For nested guests, we don't do anything specific
-		 * for single context invalidation. Hence, only advertise
-		 * support for global context invalidation.
-		 */
-		vmx->nested.nested_vmx_ept_caps |= VMX_EPT_EXTENT_GLOBAL_BIT;
+		vmx->nested.nested_vmx_ept_caps |= VMX_EPT_EXTENT_GLOBAL_BIT |
+			VMX_EPT_EXTENT_CONTEXT_BIT;
 	} else
 		vmx->nested.nested_vmx_ept_caps = 0;
 
@@ -7455,12 +7451,16 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 
 	switch (type) {
 	case VMX_EPT_EXTENT_GLOBAL:
+	/*
+	 * TODO: track mappings and invalidate
+	 * single context requests appropriately
+	 */
+	case VMX_EPT_EXTENT_CONTEXT:
 		kvm_mmu_sync_roots(vcpu);
 		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 		nested_vmx_succeed(vcpu);
 		break;
 	default:
-		/* Trap single context invalidation invept calls */
 		WARN_ON(1);
 		break;
 	}
-- 
2.5.5

[PATCH 3/4] mmu: don't pass *kvm to spte_write_protect

[Bandan Das]

That parameter isn't used in the function,
it's probably a historical artifact.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kvm/mmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index def97b3..aaecd10 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1204,7 +1204,7 @@ static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
  *
  * Return true if tlb need be flushed.
  */
-static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
+static bool spte_write_protect(u64 *sptep, bool pt_protect)
 {
 	u64 spte = *sptep;
 
@@ -1230,7 +1230,7 @@ static bool __rmap_write_protect(struct kvm *kvm,
 	bool flush = false;
 
 	for_each_rmap_spte(rmap_head, &iter, sptep)
-		flush |= spte_write_protect(kvm, sptep, pt_protect);
+		flush |= spte_write_protect(sptep, pt_protect);
 
 	return flush;
 }
-- 
2.5.5

Re: [PATCH 3/4] mmu: don't pass *kvm to spte_write_protect

[Paolo Bonzini]

On 21/07/2016 00:25, Bandan Das wrote:
> That parameter isn't used in the function,
> it's probably a historical artifact.

Same for spte_clear_dirty and spte_set_dirty, please.

Paolo

> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  arch/x86/kvm/mmu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index def97b3..aaecd10 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1204,7 +1204,7 @@ static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
>   *
>   * Return true if tlb need be flushed.
>   */
> -static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
> +static bool spte_write_protect(u64 *sptep, bool pt_protect)
>  {
>  	u64 spte = *sptep;
>  
> @@ -1230,7 +1230,7 @@ static bool __rmap_write_protect(struct kvm *kvm,
>  	bool flush = false;
>  
>  	for_each_rmap_spte(rmap_head, &iter, sptep)
> -		flush |= spte_write_protect(kvm, sptep, pt_protect);
> +		flush |= spte_write_protect(sptep, pt_protect);
>  
>  	return flush;
>  }
> 

Re: [PATCH 3/4] mmu: don't pass *kvm to spte_write_protect

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 21/07/2016 00:25, Bandan Das wrote:
>> That parameter isn't used in the function,
>> it's probably a historical artifact.
>
> Same for spte_clear_dirty and spte_set_dirty, please.

Sure! I will fix this in a follow-up.

Bandan

> Paolo
>
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  arch/x86/kvm/mmu.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index def97b3..aaecd10 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -1204,7 +1204,7 @@ static void drop_large_spte(struct kvm_vcpu *vcpu, u64 *sptep)
>>   *
>>   * Return true if tlb need be flushed.
>>   */
>> -static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool pt_protect)
>> +static bool spte_write_protect(u64 *sptep, bool pt_protect)
>>  {
>>  	u64 spte = *sptep;
>>  
>> @@ -1230,7 +1230,7 @@ static bool __rmap_write_protect(struct kvm *kvm,
>>  	bool flush = false;
>>  
>>  	for_each_rmap_spte(rmap_head, &iter, sptep)
>> -		flush |= spte_write_protect(kvm, sptep, pt_protect);
>> +		flush |= spte_write_protect(sptep, pt_protect);
>>  
>>  	return flush;
>>  }
>> 

[PATCH 4/4] nvmx: check for shadow vmcs check on entry

[Bandan Das]

vmentry should check whether the vmcs provided by
the guest hypervisor is a shadow vmcs and fail.
Also, vmptrld should check whether a shadow vmcs
is being loaded by the guest without support being present
but this check happens as part of checking the revision_id.

Signed-off-by: Bandan Das <bsd@redhat.com>
---
 arch/x86/kvm/vmx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 6291143..1b6f624 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -9924,6 +9924,10 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
 
 	skip_emulated_instruction(vcpu);
 	vmcs12 = get_vmcs12(vcpu);
+	if ((vmcs12->revision_id >> 31) & 1u) {
+		nested_vmx_failInvalid(vcpu);
+		return 1;
+	}
 
 	if (enable_shadow_vmcs)
 		copy_shadow_to_vmcs12(vmx);
-- 
2.5.5

Re: [PATCH 4/4] nvmx: check for shadow vmcs check on entry

[Paolo Bonzini]

On 21/07/2016 00:25, Bandan Das wrote:
> vmentry should check whether the vmcs provided by
> the guest hypervisor is a shadow vmcs and fail.

How can this happen, since vmptrld checks the revision_id as you said below?

Paolo

> Also, vmptrld should check whether a shadow vmcs
> is being loaded by the guest without support being present
> but this check happens as part of checking the revision_id.

Re: [PATCH 4/4] nvmx: check for shadow vmcs check on entry

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 21/07/2016 00:25, Bandan Das wrote:
>> vmentry should check whether the vmcs provided by
>> the guest hypervisor is a shadow vmcs and fail.
>
> How can this happen, since vmptrld checks the revision_id as you said below?

This is more of a change that adheres to the spec
(26.1 Basic VM-Entry Checks); the failure path
is slightly different compared to vmptrld though.
It's small and harmless but I am ok if you prefer dropping it.

Thanks for the review!

> Paolo
>
>> Also, vmptrld should check whether a shadow vmcs
>> is being loaded by the guest without support being present
>> but this check happens as part of checking the revision_id.

Re: [PATCH 4/4] nvmx: check for shadow vmcs check on entry

[Paolo Bonzini]

> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > On 21/07/2016 00:25, Bandan Das wrote:
> >> vmentry should check whether the vmcs provided by
> >> the guest hypervisor is a shadow vmcs and fail.
> >
> > How can this happen, since vmptrld checks the revision_id as you said
> > below?
> 
> This is more of a change that adheres to the spec
> (26.1 Basic VM-Entry Checks); the failure path
> is slightly different compared to vmptrld though.
> It's small and harmless but I am ok if you prefer dropping it.

Do you mean that this could happen if the VMCS is modified by L1
after VMPTRLD?  That makes sense, but with David Matlack's change
to cache the VMCS it wouldn't be possible to trigger it anymore.

Paolo

Re: [PATCH 4/4] nvmx: check for shadow vmcs check on entry

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > On 21/07/2016 00:25, Bandan Das wrote:
>> >> vmentry should check whether the vmcs provided by
>> >> the guest hypervisor is a shadow vmcs and fail.
>> >
>> > How can this happen, since vmptrld checks the revision_id as you said
>> > below?
>> 
>> This is more of a change that adheres to the spec
>> (26.1 Basic VM-Entry Checks); the failure path
>> is slightly different compared to vmptrld though.
>> It's small and harmless but I am ok if you prefer dropping it.
>
> Do you mean that this could happen if the VMCS is modified by L1
> after VMPTRLD?  That makes sense, but with David Matlack's change

Yeah that's the only way I can see it happen. If there's a separate path
that takes care of this, should I drop this one ?

> to cache the VMCS it wouldn't be possible to trigger it anymore.
>
> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html