[PATCH 0/3] nVMX: Fixes to run Xen as L1

[PATCH 0/3] nVMX: Fixes to run Xen as L1

[Bandan Das]

Minor changes to enable Xen as a L1 hypervisor.

Tested with a Haswell host, Xen-4.3 L1 and debian6 L2

Bandan Das (3):
  KVM: nVMX: Advertise support for interrupt acknowledgement
  KVM: nVMX: Ack and write vector info to intr_info if L1 asks us to
  KVM: nVMX: check for null vmcs12 when L1 does invept

 arch/x86/kvm/irq.c |  1 +
 arch/x86/kvm/vmx.c | 25 ++++++++++++++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] KVM: nVMX: Advertise support for interrupt acknowledgement

[Bandan Das]

Some Type 1 hypervisors such as XEN won't enable VMX without it present

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

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 3927528..26c1d38 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -2273,7 +2273,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 		nested_vmx_pinbased_ctls_high &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
 	}
 	nested_vmx_exit_ctls_high |= (VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
-		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER);
+		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER |
+				      VM_EXIT_ACK_INTR_ON_EXIT);
 
 	/* entry controls */
 	rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
-- 
1.8.3.1

Re: [PATCH 1/3] KVM: nVMX: Advertise support for interrupt acknowledgement

[Jan Kiszka]

On 2014-03-20 04:28, Bandan Das wrote:
> Some Type 1 hypervisors such as XEN won't enable VMX without it present
> 
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  arch/x86/kvm/vmx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 3927528..26c1d38 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -2273,7 +2273,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>  		nested_vmx_pinbased_ctls_high &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>  	}
>  	nested_vmx_exit_ctls_high |= (VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
> -		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER);
> +		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER |
> +				      VM_EXIT_ACK_INTR_ON_EXIT);
>  
>  	/* entry controls */
>  	rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
> 

Shouldn't this come after patch 2?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 1/3] KVM: nVMX: Advertise support for interrupt acknowledgement

[Bandan Das]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 2014-03-20 04:28, Bandan Das wrote:
>> Some Type 1 hypervisors such as XEN won't enable VMX without it present
>> 
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  arch/x86/kvm/vmx.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 3927528..26c1d38 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2273,7 +2273,8 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>>  		nested_vmx_pinbased_ctls_high &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>  	}
>>  	nested_vmx_exit_ctls_high |= (VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR |
>> -		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER);
>> +		VM_EXIT_LOAD_IA32_EFER | VM_EXIT_SAVE_IA32_EFER |
>> +				      VM_EXIT_ACK_INTR_ON_EXIT);
>>  
>>  	/* entry controls */
>>  	rdmsr(MSR_IA32_VMX_ENTRY_CTLS,
>> 
>
> Shouldn't this come after patch 2?

Right, that makes sense. Will fix in v2.

> Jan

[PATCH 2/3] KVM: nVMX: Ack and write vector info to intr_info if L1 asks us to

[Bandan Das]

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

diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
index 484bc87..bd0da43 100644
--- a/arch/x86/kvm/irq.c
+++ b/arch/x86/kvm/irq.c
@@ -113,6 +113,7 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
 
 	return kvm_get_apic_interrupt(v);	/* APIC */
 }
+EXPORT_SYMBOL_GPL(kvm_cpu_get_interrupt);
 
 void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu)
 {
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 26c1d38..c707389 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4491,6 +4491,16 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+/*
+ * In nested virtualization, check if L1 has set
+ * VM_EXIT_ACK_INTR_ON_EXIT
+ */
+static bool nested_exit_intr_ack_set(struct kvm_vcpu *vcpu)
+{
+	return get_vmcs12(vcpu)->vm_exit_controls &
+		VM_EXIT_ACK_INTR_ON_EXIT;
+}
+
 static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
 {
 	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
@@ -8448,6 +8458,13 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 	prepare_vmcs12(vcpu, vmcs12, exit_reason, exit_intr_info,
 		       exit_qualification);
 
+	if ((exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
+	    && nested_exit_intr_ack_set(vcpu)) {
+		int irq = kvm_cpu_get_interrupt(vcpu);
+		vmcs12->vm_exit_intr_info = irq |
+			INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR;
+	}
+
 	trace_kvm_nested_vmexit_inject(vmcs12->vm_exit_reason,
 				       vmcs12->exit_qualification,
 				       vmcs12->idt_vectoring_info_field,
-- 
1.8.3.1

Re: [PATCH 2/3] KVM: nVMX: Ack and write vector info to intr_info if L1 asks us to

[Jan Kiszka]

Commit description is missing.

On 2014-03-20 04:28, Bandan Das wrote:
> Signed-off-by: Bandan Das <bsd@redhat.com>
> ---
>  arch/x86/kvm/irq.c |  1 +
>  arch/x86/kvm/vmx.c | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
> index 484bc87..bd0da43 100644
> --- a/arch/x86/kvm/irq.c
> +++ b/arch/x86/kvm/irq.c
> @@ -113,6 +113,7 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
>  
>  	return kvm_get_apic_interrupt(v);	/* APIC */
>  }
> +EXPORT_SYMBOL_GPL(kvm_cpu_get_interrupt);
>  
>  void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu)
>  {
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 26c1d38..c707389 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4491,6 +4491,16 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
>  		PIN_BASED_EXT_INTR_MASK;
>  }
>  
> +/*
> + * In nested virtualization, check if L1 has set
> + * VM_EXIT_ACK_INTR_ON_EXIT

This comment does not really contribute much information compared to the
code below.

> + */
> +static bool nested_exit_intr_ack_set(struct kvm_vcpu *vcpu)
> +{
> +	return get_vmcs12(vcpu)->vm_exit_controls &
> +		VM_EXIT_ACK_INTR_ON_EXIT;
> +}
> +
>  static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
>  {
>  	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
> @@ -8448,6 +8458,13 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>  	prepare_vmcs12(vcpu, vmcs12, exit_reason, exit_intr_info,
>  		       exit_qualification);
>  
> +	if ((exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
> +	    && nested_exit_intr_ack_set(vcpu)) {
> +		int irq = kvm_cpu_get_interrupt(vcpu);
> +		vmcs12->vm_exit_intr_info = irq |
> +			INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR;
> +	}
> +
>  	trace_kvm_nested_vmexit_inject(vmcs12->vm_exit_reason,
>  				       vmcs12->exit_qualification,
>  				       vmcs12->idt_vectoring_info_field,
> 

Looks correct. What about adding a corresponding unit test
kvm-unit-tests/x86/vmx_tests.c)? I would highly recommend this.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH 2/3] KVM: nVMX: Ack and write vector info to intr_info if L1 asks us to

[Bandan Das]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> Commit description is missing.
>
> On 2014-03-20 04:28, Bandan Das wrote:
>> Signed-off-by: Bandan Das <bsd@redhat.com>
>> ---
>>  arch/x86/kvm/irq.c |  1 +
>>  arch/x86/kvm/vmx.c | 17 +++++++++++++++++
>>  2 files changed, 18 insertions(+)
>> 
>> diff --git a/arch/x86/kvm/irq.c b/arch/x86/kvm/irq.c
>> index 484bc87..bd0da43 100644
>> --- a/arch/x86/kvm/irq.c
>> +++ b/arch/x86/kvm/irq.c
>> @@ -113,6 +113,7 @@ int kvm_cpu_get_interrupt(struct kvm_vcpu *v)
>>  
>>  	return kvm_get_apic_interrupt(v);	/* APIC */
>>  }
>> +EXPORT_SYMBOL_GPL(kvm_cpu_get_interrupt);
>>  
>>  void kvm_inject_pending_timer_irqs(struct kvm_vcpu *vcpu)
>>  {
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index 26c1d38..c707389 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -4491,6 +4491,16 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
>>  		PIN_BASED_EXT_INTR_MASK;
>>  }
>>  
>> +/*
>> + * In nested virtualization, check if L1 has set
>> + * VM_EXIT_ACK_INTR_ON_EXIT
>
> This comment does not really contribute much information compared to the
> code below.
>
>> + */
>> +static bool nested_exit_intr_ack_set(struct kvm_vcpu *vcpu)
>> +{
>> +	return get_vmcs12(vcpu)->vm_exit_controls &
>> +		VM_EXIT_ACK_INTR_ON_EXIT;
>> +}
>> +
>>  static bool nested_exit_on_nmi(struct kvm_vcpu *vcpu)
>>  {
>>  	return get_vmcs12(vcpu)->pin_based_vm_exec_control &
>> @@ -8448,6 +8458,13 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
>>  	prepare_vmcs12(vcpu, vmcs12, exit_reason, exit_intr_info,
>>  		       exit_qualification);
>>  
>> +	if ((exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
>> +	    && nested_exit_intr_ack_set(vcpu)) {
>> +		int irq = kvm_cpu_get_interrupt(vcpu);
>> +		vmcs12->vm_exit_intr_info = irq |
>> +			INTR_INFO_VALID_MASK | INTR_TYPE_EXT_INTR;
>> +	}
>> +
>>  	trace_kvm_nested_vmexit_inject(vmcs12->vm_exit_reason,
>>  				       vmcs12->exit_qualification,
>>  				       vmcs12->idt_vectoring_info_field,
>> 
>
> Looks correct. What about adding a corresponding unit test
> kvm-unit-tests/x86/vmx_tests.c)? I would highly recommend this.

Thanks for pointing this out, I will take a look.

> Jan

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

[Bandan Das]

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

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

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

[Jan Kiszka]

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.

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

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

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

[Bandan Das]

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

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

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

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

[Jan Kiszka]

[-- 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 --]

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

[Bandan Das]

Jan Kiszka <jan.kiszka@web.de> writes:

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

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

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

[Jan Kiszka]

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

On 2014-03-22 17:43, Bandan Das wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> 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.
> 
> 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.

OK, I think I misunderstood what the guest expects and how we currently
achieve this: we do not track the mapping between guest and host eptp,
thus cannot properly emulate its behaviour. We therefore need to flush
everything.

> 
> Or do you think we should rethink this approach ?

Well, I wonder if we should expose single-context invept support at all.

I'm also wondering if we are returning proper flags on

    if ((operand.eptp & eptp_mask) !=
                    (nested_ept_get_cr3(vcpu) & eptp_mask))
            break;

Neither nested_vmx_succeed nor nested_vmx_fail* is called if this
condition evaluates to true.

Jan



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

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

[Bandan Das]

Jan Kiszka <jan.kiszka@web.de> writes:

> On 2014-03-22 17:43, Bandan Das wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> 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.
>> 
>> 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.
>
> OK, I think I misunderstood what the guest expects and how we currently
> achieve this: we do not track the mapping between guest and host eptp,
> thus cannot properly emulate its behaviour. We therefore need to flush
> everything.
>
>> 
>> Or do you think we should rethink this approach ?
>
> Well, I wonder if we should expose single-context invept support at all.
>
> I'm also wondering if we are returning proper flags on
>
>     if ((operand.eptp & eptp_mask) !=
>                     (nested_ept_get_cr3(vcpu) & eptp_mask))
>             break;

That does sound plausible but then we would have to get rid of this 
little optimization in the code you have quoted above. We would have
to flush and sync roots for all invept calls. So, my preference is to 
keep it.

> Neither nested_vmx_succeed nor nested_vmx_fail* is called if this
> condition evaluates to true.

It appears we should always call nested_vmx_succeed(). If you are ok,
I will send a v2.

Thanks,
Bandan

> Jan

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

[Jan Kiszka]

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

On 2014-03-26 21:22, Bandan Das wrote:
> Jan Kiszka <jan.kiszka@web.de> writes:
> 
>> On 2014-03-22 17:43, Bandan Das wrote:
>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>
>>>> 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.
>>>
>>> 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.
>>
>> OK, I think I misunderstood what the guest expects and how we currently
>> achieve this: we do not track the mapping between guest and host eptp,
>> thus cannot properly emulate its behaviour. We therefore need to flush
>> everything.
>>
>>>
>>> Or do you think we should rethink this approach ?
>>
>> Well, I wonder if we should expose single-context invept support at all.
>>
>> I'm also wondering if we are returning proper flags on
>>
>>     if ((operand.eptp & eptp_mask) !=
>>                     (nested_ept_get_cr3(vcpu) & eptp_mask))
>>             break;
> 
> That does sound plausible but then we would have to get rid of this 
> little optimization in the code you have quoted above. We would have
> to flush and sync roots for all invept calls. So, my preference is to 
> keep it.

Do you have any numbers on how many per-context invept calls we are able
to ignore? At least for KVM this is should be 0 as we only flush on
changes to the current EPT structures.

Jan

> 
>> Neither nested_vmx_succeed nor nested_vmx_fail* is called if this
>> condition evaluates to true.
> 
> It appears we should always call nested_vmx_succeed(). If you are ok,
> I will send a v2.
> 
> Thanks,
> Bandan
> 
>> Jan



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

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

[Bandan Das]

Jan Kiszka <jan.kiszka@web.de> writes:

> On 2014-03-26 21:22, Bandan Das wrote:
>> Jan Kiszka <jan.kiszka@web.de> writes:
>> 
>>> On 2014-03-22 17:43, Bandan Das wrote:
>>>> Jan Kiszka <jan.kiszka@web.de> writes:
>>>>
>>>>> 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.
>>>>
>>>> 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.
>>>
>>> OK, I think I misunderstood what the guest expects and how we currently
>>> achieve this: we do not track the mapping between guest and host eptp,
>>> thus cannot properly emulate its behaviour. We therefore need to flush
>>> everything.
>>>
>>>>
>>>> Or do you think we should rethink this approach ?
>>>
>>> Well, I wonder if we should expose single-context invept support at all.
>>>
>>> I'm also wondering if we are returning proper flags on
>>>
>>>     if ((operand.eptp & eptp_mask) !=
>>>                     (nested_ept_get_cr3(vcpu) & eptp_mask))
>>>             break;
>> 
>> That does sound plausible but then we would have to get rid of this 
>> little optimization in the code you have quoted above. We would have
>> to flush and sync roots for all invept calls. So, my preference is to 
>> keep it.
>
> Do you have any numbers on how many per-context invept calls we are able
> to ignore? At least for KVM this is should be 0 as we only flush on
> changes to the current EPT structures.

Just instrumented a Debian L2 bootup under L1 Xen and there were 0 
per-context invept calls (with a valid vmcs12). I will spin out a 
version for testing where we don't advertise single context invept and 
if everything works, put a comment as to why we are removing support.

Thanks,
Bandan

> Jan
>
>> 
>>> Neither nested_vmx_succeed nor nested_vmx_fail* is called if this
>>> condition evaluates to true.
>> 
>> It appears we should always call nested_vmx_succeed(). If you are ok,
>> I will send a v2.
>> 
>> Thanks,
>> Bandan
>> 
>>> Jan

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

[Paolo Bonzini]

Il 20/03/2014 04:28, Bandan Das ha scritto:
> 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
>
> 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);
>

Please add a /* fall through */ comment as well.

Paolo

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

[Bandan Das]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 20/03/2014 04:28, Bandan Das ha scritto:
>> 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
>>
>> 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);
>>
>
> Please add a /* fall through */ comment as well.

Thanks for reviewing Paolo, will fix in v2.

> Paolo