nVMX: Fixes to run Xen as L1

PATCH Superseded Bandan Das 4 participants 21 messages Mar 31, 2014 – Apr 14, 2014

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Bandan Das

author

2014-03-31 21:00 UTC #0 lore raw

v2 cover nVMX: Fixes to run Xen as L1

Minor changes to enable Xen as a L1 hypervisor.

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

v2:
* Remove advertising single context invalidation for emulated invept Patch "KVM: nVMX: check for null vmcs12 when L1 does invept" from v1 is now obsolete and is removed
* Reorder patches "KVM: nVMX: Advertise support for interrupt acknowledgement" and "nVMX: Ack and write vector info to intr_info if L1 asks us to"
* Add commit description to 2/3 and change comment for nested_exit_intr_ack_set

Jan, I will send a separate unit-test patch

Bandan Das (3):
KVM: nVMX: Don't advertise single context invalidation for invept
KVM: nVMX: Ack and write vector info to intr_info if L1 asks us to
KVM: nVMX: Advertise support for interrupt acknowledgement

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

Signature · 1 line

1.8.3.1

Bandan Das

author

2014-03-31 21:00 UTC #1 lore raw

PATCH v2 1/3 KVM: nVMX: Don't advertise single context invalidation for invept

For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/kvm/vmx.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

arch/x86/kvm/vmx.c +5 ~~-10~~

index 3927528..3e7f60c 100644
@@ -2331,12 +2331,11 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 			 VMX_EPT_INVEPT_BIT;
 		nested_vmx_ept_caps &= vmx_capability.ept;
 		/*
-		 * Since invept is completely emulated we support both global
-		 * and context invalidation independent of what host cpu
-		 * supports
+		 * For nested guests, we don't do anything specific
+		 * for single context invalidation. Hence, only advertise
+		 * support for global context invalidation.
 		 */
-		nested_vmx_ept_caps |= VMX_EPT_EXTENT_GLOBAL_BIT |
-			VMX_EPT_EXTENT_CONTEXT_BIT;
+		nested_vmx_ept_caps |= VMX_EPT_EXTENT_GLOBAL_BIT;
 	} else
 		nested_vmx_ept_caps = 0;

@@ -6383,7 +6382,6 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 	struct {
 		u64 eptp, gpa;
 	} operand;
-	u64 eptp_mask = ((1ull << 51) - 1) & PAGE_MASK;
 
 	if (!(nested_vmx_secondary_ctls_high & SECONDARY_EXEC_ENABLE_EPT) ||
 	    !(nested_vmx_ept_caps & VMX_EPT_INVEPT_BIT)) {

@@ -6423,16 +6421,13 @@ 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))
-			break;
 	case VMX_EPT_EXTENT_GLOBAL:
 		kvm_mmu_sync_roots(vcpu);
 		kvm_mmu_flush_tlb(vcpu);
 		nested_vmx_succeed(vcpu);
 		break;
 	default:
+		/* Trap single context invalidation invept calls */
 		BUG_ON(1);
 		break;
 	}

Signature · 1 line

1.8.3.1

Marcelo Tosatti

2014-04-10 20:47 UTC #2 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

Quoted 8 lines · Bandan Das

For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>

Bandan,

Why not fix INVEPT single-context rather than removing it entirely?

"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"

So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.

Quoted 58 lines

---
 arch/x86/kvm/vmx.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

arch/x86/kvm/vmx.c +5 ~~-10~~

index 3927528..3e7f60c 100644
@@ -2331,12 +2331,11 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 			 VMX_EPT_INVEPT_BIT;
 		nested_vmx_ept_caps &= vmx_capability.ept;
 		/*
-		 * Since invept is completely emulated we support both global
-		 * and context invalidation independent of what host cpu
-		 * supports
+		 * For nested guests, we don't do anything specific
+		 * for single context invalidation. Hence, only advertise
+		 * support for global context invalidation.
 		 */
-		nested_vmx_ept_caps |= VMX_EPT_EXTENT_GLOBAL_BIT |
-			VMX_EPT_EXTENT_CONTEXT_BIT;
+		nested_vmx_ept_caps |= VMX_EPT_EXTENT_GLOBAL_BIT;
 	} else
 		nested_vmx_ept_caps = 0;

@@ -6383,7 +6382,6 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 	struct {
 		u64 eptp, gpa;
 	} operand;
-	u64 eptp_mask = ((1ull << 51) - 1) & PAGE_MASK;
 
 	if (!(nested_vmx_secondary_ctls_high & SECONDARY_EXEC_ENABLE_EPT) ||
 	    !(nested_vmx_ept_caps & VMX_EPT_INVEPT_BIT)) {

@@ -6423,16 +6421,13 @@ 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))
-			break;
 	case VMX_EPT_EXTENT_GLOBAL:
 		kvm_mmu_sync_roots(vcpu);
 		kvm_mmu_flush_tlb(vcpu);
 		nested_vmx_succeed(vcpu);
 		break;
 	default:
+		/* Trap single context invalidation invept calls */
 		BUG_ON(1);
 		break;
 	}

Signature · 6 lines

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bandan Das

author

2014-04-11 00:27 UTC #3 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

Quoted 14 lines · Marcelo Tosatti

Quoted 8 lines · Bandan Das
For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>
Bandan,
Why not fix INVEPT single-context rather than removing it entirely?

Quoted 7 lines

"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"
So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.

The single context invalidation in handle_invept() doesn't do anything different. It just falls down to the global case. And the invept code in Xen and KVM both seemed to fall back to global invalidation if support for single context wasn't found. So, it was proposed not to advertise it at all.

But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback, this will unfortunately not work. Jan, do you agree with this ?

Bandan

Quoted 58 lines

---
 arch/x86/kvm/vmx.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

arch/x86/kvm/vmx.c +5 ~~-10~~

index 3927528..3e7f60c 100644
@@ -2331,12 +2331,11 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 			 VMX_EPT_INVEPT_BIT;
 		nested_vmx_ept_caps &= vmx_capability.ept;
 		/*
-		 * Since invept is completely emulated we support both global
-		 * and context invalidation independent of what host cpu
-		 * supports
+		 * For nested guests, we don't do anything specific
+		 * for single context invalidation. Hence, only advertise
+		 * support for global context invalidation.
 		 */
-		nested_vmx_ept_caps |= VMX_EPT_EXTENT_GLOBAL_BIT |
-			VMX_EPT_EXTENT_CONTEXT_BIT;
+		nested_vmx_ept_caps |= VMX_EPT_EXTENT_GLOBAL_BIT;
 	} else
 		nested_vmx_ept_caps = 0;

@@ -6383,7 +6382,6 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 	struct {
 		u64 eptp, gpa;
 	} operand;
-	u64 eptp_mask = ((1ull << 51) - 1) & PAGE_MASK;
 
 	if (!(nested_vmx_secondary_ctls_high & SECONDARY_EXEC_ENABLE_EPT) ||
 	    !(nested_vmx_ept_caps & VMX_EPT_INVEPT_BIT)) {

@@ -6423,16 +6421,13 @@ 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))
-			break;
 	case VMX_EPT_EXTENT_GLOBAL:
 		kvm_mmu_sync_roots(vcpu);
 		kvm_mmu_flush_tlb(vcpu);
 		nested_vmx_succeed(vcpu);
 		break;
 	default:
+		/* Trap single context invalidation invept calls */
 		BUG_ON(1);
 		break;
 	}

Signature · 6 lines

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jan Kiszka

2014-04-11 06:22 UTC #4 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

Quoted 30 lines · Bandan Das

Quoted 14 lines · Marcelo Tosatti
Quoted 8 lines · Bandan Das
For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>
Bandan,
Why not fix INVEPT single-context rather than removing it entirely?
Quoted 7 lines
"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"
So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.
The single context invalidation in handle_invept() doesn't do anything different. It just falls down to the global case. And the invept code in Xen and KVM both seemed to fall back to global invalidation if support for single context wasn't found. So, it was proposed not to advertise it at all.

Quoted 3 lines

But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback, this will unfortunately not work. Jan, do you agree with this ?

A hypervisor that doesn't properly check the HW caps is just broken. And one that mandates single context invalidation support is silly.

Jan

Signature · 2 lines

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

Bandan Das

author

2014-04-11 17:26 UTC #5 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

Quoted 31 lines · Jan Kiszka

Quoted 30 lines · Bandan Das
Quoted 14 lines · Marcelo Tosatti
Quoted 8 lines · Bandan Das
For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>
Bandan,
Why not fix INVEPT single-context rather than removing it entirely?
Quoted 7 lines
"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"
So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.
The single context invalidation in handle_invept() doesn't do anything different. It just falls down to the global case. And the invept code in Xen and KVM both seemed to fall back to global invalidation if support for single context wasn't found. So, it was proposed not to advertise it at all.

Quoted 6 lines

Quoted 3 lines
But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback, this will unfortunately not work. Jan, do you agree with this ?
A hypervisor that doesn't properly check the HW caps is just broken. And one that mandates single context invalidation support is silly.

Well, but we could make life a little bit easier for the unfortunate user using the broken hypervisor :) And advertising single context inavalidation doesn't really seem to have any downsides.

Quoted 1 line

Jan

Jan Kiszka

2014-04-11 18:01 UTC #6 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

Quoted 37 lines · Bandan Das

Quoted 35 lines · Jan Kiszka
Quoted 33 lines · Bandan Das
Quoted 14 lines · Marcelo Tosatti
Quoted 8 lines · Bandan Das
For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>
Bandan,
Why not fix INVEPT single-context rather than removing it entirely?
Quoted 7 lines
"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"
So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.
The single context invalidation in handle_invept() doesn't do anything different. It just falls down to the global case. And the invept code in Xen and KVM both seemed to fall back to global invalidation if support for single context wasn't found. So, it was proposed not to advertise it at all.
But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback, this will unfortunately not work. Jan, do you agree with this ?

Quoted 6 lines

Quoted 2 lines
A hypervisor that doesn't properly check the HW caps is just broken. And one that mandates single context invalidation support is silly.
Well, but we could make life a little bit easier for the unfortunate user using the broken hypervisor :) And advertising single context inavalidation doesn't really seem to have any downsides.

Ok, let's try it this way: single-context invalidation is inherently tied to VPID support (that's how you address a context). However, KVM does not expose VPID to its guest. So this discussion is mood: no hypervisor will make use of this feature as it has no means to fill in the required parameter.

Once we start supporting VPID, we can also think about how to address single-context invalidation reasonably.

Jan

Signature · 2 lines

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

Bandan Das

author

2014-04-11 18:35 UTC #7 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

Quoted 45 lines · Jan Kiszka

Quoted 37 lines · Bandan Das
Quoted 35 lines · Jan Kiszka
Quoted 33 lines · Bandan Das
Quoted 14 lines · Marcelo Tosatti
Quoted 8 lines · Bandan Das
For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>
Bandan,
Why not fix INVEPT single-context rather than removing it entirely?
Quoted 7 lines
"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"
So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.
The single context invalidation in handle_invept() doesn't do anything different. It just falls down to the global case. And the invept code in Xen and KVM both seemed to fall back to global invalidation if support for single context wasn't found. So, it was proposed not to advertise it at all.
But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback, this will unfortunately not work. Jan, do you agree with this ?
Quoted 6 lines
Quoted 2 lines
A hypervisor that doesn't properly check the HW caps is just broken. And one that mandates single context invalidation support is silly.
Well, but we could make life a little bit easier for the unfortunate user using the broken hypervisor :) And advertising single context inavalidation doesn't really seem to have any downsides.

Quoted 5 lines

Ok, let's try it this way: single-context invalidation is inherently tied to VPID support (that's how you address a context). However, KVM does not expose VPID to its guest. So this discussion is mood: no hypervisor will make use of this feature as it has no means to fill in the required parameter.

I thought (from the spec) invept single context invalidation takes the EP4TA as the second argument. invvpid single context however takes the VPID as its descriptor.

The Xen L1 hypervisor was actually calling single context invept multiple times. That's how I hit this bug.

Quoted 4 lines

Once we start supporting VPID, we can also think about how to address single-context invalidation reasonably.
Jan

Jan Kiszka

2014-04-11 18:53 UTC #8 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

Quoted 53 lines · Bandan Das

Quoted 45 lines · Jan Kiszka
Quoted 37 lines · Bandan Das
Quoted 35 lines · Jan Kiszka
Quoted 33 lines · Bandan Das
Quoted 14 lines · Marcelo Tosatti
Quoted 8 lines · Bandan Das
For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>
Bandan,
Why not fix INVEPT single-context rather than removing it entirely?
Quoted 7 lines
"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"
So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.
The single context invalidation in handle_invept() doesn't do anything different. It just falls down to the global case. And the invept code in Xen and KVM both seemed to fall back to global invalidation if support for single context wasn't found. So, it was proposed not to advertise it at all.
But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback, this will unfortunately not work. Jan, do you agree with this ?
Quoted 6 lines
Quoted 2 lines
A hypervisor that doesn't properly check the HW caps is just broken. And one that mandates single context invalidation support is silly.
Well, but we could make life a little bit easier for the unfortunate user using the broken hypervisor :) And advertising single context inavalidation doesn't really seem to have any downsides.
Quoted 5 lines
Ok, let's try it this way: single-context invalidation is inherently tied to VPID support (that's how you address a context). However, KVM does not expose VPID to its guest. So this discussion is mood: no hypervisor will make use of this feature as it has no means to fill in the required parameter.

Quoted 3 lines

I thought (from the spec) invept single context invalidation takes the EP4TA as the second argument. invvpid single context however takes the VPID as its descriptor.

Oops, invept/invvpid mess-up while re-reading the spec - sorry.

Quoted 3 lines

The Xen L1 hypervisor was actually calling single context invept multiple times. That's how I hit this bug.

...and it's no longer doing it now, I suppose. The question remains, which hypervisor we want to cater with a "single-context-that-is-current-context" invalidation (that is my understanding of Marcelo's proposal). On the other hand, if some hypervisor actually uses invept to invalidate a non-current mapping, we would regress compared to not exposing single context invept. Hope I got this conclusion right. ;)

Jan

Signature · 2 lines

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

Marcelo Tosatti

2014-04-11 19:35 UTC #9 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

Quoted 61 lines · Jan Kiszka

Quoted 53 lines · Bandan Das
Quoted 45 lines · Jan Kiszka
Quoted 37 lines · Bandan Das
Quoted 35 lines · Jan Kiszka
Quoted 33 lines · Bandan Das
Quoted 14 lines · Marcelo Tosatti
Quoted 8 lines · Bandan Das
For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>
Bandan,
Why not fix INVEPT single-context rather than removing it entirely?
Quoted 7 lines
"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"
So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.
The single context invalidation in handle_invept() doesn't do anything different. It just falls down to the global case. And the invept code in Xen and KVM both seemed to fall back to global invalidation if support for single context wasn't found. So, it was proposed not to advertise it at all.
But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback, this will unfortunately not work. Jan, do you agree with this ?
Quoted 6 lines
Quoted 2 lines
A hypervisor that doesn't properly check the HW caps is just broken. And one that mandates single context invalidation support is silly.
Well, but we could make life a little bit easier for the unfortunate user using the broken hypervisor :) And advertising single context inavalidation doesn't really seem to have any downsides.
Quoted 5 lines
Ok, let's try it this way: single-context invalidation is inherently tied to VPID support (that's how you address a context). However, KVM does not expose VPID to its guest. So this discussion is mood: no hypervisor will make use of this feature as it has no means to fill in the required parameter.
Quoted 3 lines
I thought (from the spec) invept single context invalidation takes the EP4TA as the second argument. invvpid single context however takes the VPID as its descriptor.
Oops, invept/invvpid mess-up while re-reading the spec - sorry.
Quoted 1 line

Quoted 7 lines

Quoted 2 lines
The Xen L1 hypervisor was actually calling single context invept multiple times. That's how I hit this bug.
...and it's no longer doing it now, I suppose. The question remains, which hypervisor we want to cater with a "single-context-that-is-current-context" invalidation (that is my understanding of Marcelo's proposal).

My proposal is to implement what is in the spec.

Quoted 3 lines

On the other hand, if some hypervisor actually uses invept to invalidate a non-current mapping, we would regress compared to not exposing single context invept. Hope I got this conclusion right. ;)

In that case INVEPT global would also be broken.

Jan Kiszka

2014-04-14 05:46 UTC #10 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

Quoted 70 lines · Marcelo Tosatti

Quoted 61 lines · Jan Kiszka
Quoted 53 lines · Bandan Das
Quoted 45 lines · Jan Kiszka
Quoted 37 lines · Bandan Das
Quoted 35 lines · Jan Kiszka
Quoted 33 lines · Bandan Das
Quoted 14 lines · Marcelo Tosatti
Quoted 8 lines · Bandan Das
For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>
Bandan,
Why not fix INVEPT single-context rather than removing it entirely?
Quoted 7 lines
"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"
So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.
The single context invalidation in handle_invept() doesn't do anything different. It just falls down to the global case. And the invept code in Xen and KVM both seemed to fall back to global invalidation if support for single context wasn't found. So, it was proposed not to advertise it at all.
But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback, this will unfortunately not work. Jan, do you agree with this ?
Quoted 6 lines
Quoted 2 lines
A hypervisor that doesn't properly check the HW caps is just broken. And one that mandates single context invalidation support is silly.
Well, but we could make life a little bit easier for the unfortunate user using the broken hypervisor :) And advertising single context inavalidation doesn't really seem to have any downsides.
Quoted 5 lines
Ok, let's try it this way: single-context invalidation is inherently tied to VPID support (that's how you address a context). However, KVM does not expose VPID to its guest. So this discussion is mood: no hypervisor will make use of this feature as it has no means to fill in the required parameter.
Quoted 3 lines
I thought (from the spec) invept single context invalidation takes the EP4TA as the second argument. invvpid single context however takes the VPID as its descriptor.
Oops, invept/invvpid mess-up while re-reading the spec - sorry.
Quoted 1 line
Quoted 7 lines
Quoted 2 lines
The Xen L1 hypervisor was actually calling single context invept multiple times. That's how I hit this bug.
...and it's no longer doing it now, I suppose. The question remains, which hypervisor we want to cater with a "single-context-that-is-current-context" invalidation (that is my understanding of Marcelo's proposal).

Quoted 7 lines

My proposal is to implement what is in the spec.
Quoted 3 lines
On the other hand, if some hypervisor actually uses invept to invalidate a non-current mapping, we would regress compared to not exposing single context invept. Hope I got this conclusion right. ;)
In that case INVEPT global would also be broken.

I'm all for having a proper invept single context support but that, first of all, requires tracking the vEPTP->EPTP mappings.

Jan

Signature · 2 lines

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

Bandan Das

author

2014-04-11 19:38 UTC #11 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

In reply to Jan Kiszka

Quoted 58 lines · Jan Kiszka

Quoted 53 lines · Bandan Das
Quoted 45 lines · Jan Kiszka
Quoted 37 lines · Bandan Das
Quoted 35 lines · Jan Kiszka
Quoted 33 lines · Bandan Das
Quoted 14 lines · Marcelo Tosatti
Quoted 8 lines · Bandan Das
For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>
Bandan,
Why not fix INVEPT single-context rather than removing it entirely?
Quoted 7 lines
"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"
So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.
The single context invalidation in handle_invept() doesn't do anything different. It just falls down to the global case. And the invept code in Xen and KVM both seemed to fall back to global invalidation if support for single context wasn't found. So, it was proposed not to advertise it at all.
But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback, this will unfortunately not work. Jan, do you agree with this ?
Quoted 6 lines
Quoted 2 lines
A hypervisor that doesn't properly check the HW caps is just broken. And one that mandates single context invalidation support is silly.
Well, but we could make life a little bit easier for the unfortunate user using the broken hypervisor :) And advertising single context inavalidation doesn't really seem to have any downsides.
Quoted 5 lines
Ok, let's try it this way: single-context invalidation is inherently tied to VPID support (that's how you address a context). However, KVM does not expose VPID to its guest. So this discussion is mood: no hypervisor will make use of this feature as it has no means to fill in the required parameter.
Quoted 3 lines
I thought (from the spec) invept single context invalidation takes the EP4TA as the second argument. invvpid single context however takes the VPID as its descriptor.

Quoted 7 lines

Oops, invept/invvpid mess-up while re-reading the spec - sorry.
Quoted 3 lines
The Xen L1 hypervisor was actually calling single context invept multiple times. That's how I hit this bug.
...and it's no longer doing it now, I suppose. The question remains,

Yes.

Quoted 6 lines

which hypervisor we want to cater with a "single-context-that-is-current-context" invalidation (that is my understanding of Marcelo's proposal). On the other hand, if some hypervisor actually uses invept to invalidate a non-current mapping, we would regress compared to not exposing single context invept. Hope I got this conclusion right. ;)

Yep, not sure if this holds true for any hypervisor. I traced this change down to http://www.spinics.net/lists/kvm/msg94802.html but the conversation doesn't mention the reasoning

Quoted 1 line

Jan

Marcelo Tosatti

2014-04-11 19:02 UTC #12 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

In reply to Jan Kiszka

Quoted 25 lines · Jan Kiszka

Quoted 24 lines · Bandan Das
Quoted 10 lines · Marcelo Tosatti
Quoted 8 lines · Bandan Das
For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>
Bandan,
Quoted 10 lines
Why not fix INVEPT single-context rather than removing it entirely?
"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"
So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.

Quoted 8 lines

Quoted 8 lines
The single context invalidation in handle_invept() doesn't do anything different. It just falls down to the global case. And the invept code in Xen and KVM both seemed to fall back to global invalidation if support for single context wasn't found. So, it was proposed not to advertise it at all.
But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback,

What do you mean "does not fallback" ? The hypervisor cannot detect fallback because:

"(The instruction may invalidate mappings associated with other EP4TAs.)"

So the spec says single context can behave as global context (similar with TLB entries and INVLPG).

So it is valid to implement single context as global context.

Quoted 6 lines

Quoted 1 line
this will unfortunately not work. Jan, do you agree with this ?
A hypervisor that doesn't properly check the HW caps is just broken. And one that mandates single context invalidation support is silly.
Jan

I imagined Xen broke because broken KVM's implementation of INVEPT single context (so that should be fixed).

If with the proper implementation of INVEPT single context in KVM Xen still fails for some reason, would have to understand why it is failing.

Bandan Das

author

2014-04-11 19:33 UTC #13 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

Quoted 38 lines · Marcelo Tosatti

Quoted 25 lines · Jan Kiszka
Quoted 24 lines · Bandan Das
Quoted 10 lines · Marcelo Tosatti
Quoted 8 lines · Bandan Das
For single context invalidation, we fall through to global invalidation in handle_invept() except for one case - when the operand supplied by L1 is different from what we have in vmcs12. However, typically hypervisors will only call invept for the currently loaded eptp, so the condition will never be true.
Signed-off-by Bandan Das <[email protected]>
Bandan,
Quoted 10 lines
Why not fix INVEPT single-context rather than removing it entirely?
"Single-context. If the INVEPT type is 1, the logical processor invalidates all guest-physical mappings and combined mappings associated with the EP4TA specified in the INVEPT descriptor. Combined mappings for that EP4TA are invalidated for all VPIDs and all PCIDs. (The instruction may invalidate mappings associated with other EP4TAs.)"
So just removing the "if (EPTP != CURRENT.EPTP) BREAK" should be enough.
Quoted 8 lines
Quoted 8 lines
The single context invalidation in handle_invept() doesn't do anything different. It just falls down to the global case. And the invept code in Xen and KVM both seemed to fall back to global invalidation if support for single context wasn't found. So, it was proposed not to advertise it at all.
But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback,
What do you mean "does not fallback" ? The hypervisor cannot detect fallback because:

Quoted 6 lines

"(The instruction may invalidate mappings associated with other EP4TAs.)"
So the spec says single context can behave as global context (similar with TLB entries and INVLPG).
So it is valid to implement single context as global context.

I meant if single context invalidation isn't supported, the hypervisor falls back to global invalidation like in kvm -

static inline void ept_sync_context(u64 eptp)
{
...

		if (cpu_has_vmx_invept_context())
			__invept(VMX_EPT_EXTENT_CONTEXT, eptp, 0);
		else
			ept_sync_global();

...

Quoted 9 lines

Quoted 6 lines
Quoted 1 line
this will unfortunately not work. Jan, do you agree with this ?
A hypervisor that doesn't properly check the HW caps is just broken. And one that mandates single context invalidation support is silly.
Jan
I imagined Xen broke because broken KVM's implementation of INVEPT single context (so that should be fixed).

It's failing because of this check in handle_invept - if ((operand.eptp & eptp_mask) !=

	(nested_ept_get_cr3(vcpu) & eptp_mask))
			break;

Problem is invept can get called even after a vmclear and Jan pointed out that there's probably no case where this if will evaluate to true (atleast not for kvm/xen).

Quoted 2 lines

If with the proper implementation of INVEPT single context in KVM Xen still fails for some reason, would have to understand why it is failing.

The argument was that since kvm doesn't do anything different for single context invalidation, does it make sense to not advertise it at all assuming that the above snippet of invept code is used by all hypervisors ?

Marcelo Tosatti

2014-04-11 19:02 UTC #14 lore raw

Re: [PATCH v2 1/3] KVM: nVMX: Don't advertise single context invalidation for invept

In reply to Jan Kiszka

Quoted 6 lines · Jan Kiszka

Quoted 3 lines
But rethinking this again, I agree with you. If there's a hypervisor with a single context invept implmentation that does not fallback, this will unfortunately not work. Jan, do you agree with this ?
A hypervisor that doesn't properly check the HW caps is just broken. And one that mandates single context invalidation support is silly.

Is this a justification for removing INVEPT single-context until it is implemented as single-context?

Bandan Das

author

2014-03-31 21:01 UTC #15 lore raw

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

In reply to Bandan Das

This feature emulates the "Acknowledge interrupt on exit" behavior. We can safely emulate it for L1 to run L2 even if L0 itself has it disabled (to run L1).

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/kvm/irq.c |  1 +
 arch/x86/kvm/vmx.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

arch/x86/kvm/irq.c +1 -0

index 484bc87..bd0da43 100644
@@ -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)
 {

arch/x86/kvm/vmx.c +19 -0

index 3e7f60c..bdc8f2d 100644
@@ -4489,6 +4489,18 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+/*
+ * In nested virtualization, check if L1 has enabled
+ * interrupt acknowledgement that writes the interrupt vector
+ * info on vmexit
+ * 
+ */
+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 &

@@ -8442,6 +8454,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,

Signature · 1 line

1.8.3.1

Marcelo Tosatti

2014-04-11 19:02 UTC #16 lore raw

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

Quoted 46 lines · Bandan Das

This feature emulates the "Acknowledge interrupt on exit" behavior. We can safely emulate it for L1 to run L2 even if L0 itself has it disabled (to run L1).

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/kvm/irq.c |  1 +
 arch/x86/kvm/vmx.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

arch/x86/kvm/irq.c +1 -0

index 484bc87..bd0da43 100644
@@ -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)
 {

arch/x86/kvm/vmx.c +12 -0

index 3e7f60c..bdc8f2d 100644
@@ -4489,6 +4489,18 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+/*
+ * In nested virtualization, check if L1 has enabled
+ * interrupt acknowledgement that writes the interrupt vector
+ * info on vmexit
+ * 
+ */
+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 &

arch/x86/kvm/vmx.c static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason, · line 8459

Quoted 6 lines

arch/x86/kvm/vmx.c +3 -0

@@ -8442,6 +8454,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);

Can irq be -1 ?

Quoted 14 lines

arch/x86/kvm/vmx.c +4 -0

+		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,

Signature · 6 lines

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bandan Das

author

2014-04-11 19:17 UTC #17 lore raw

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

Quoted 50 lines · Marcelo Tosatti

Quoted 49 lines · Bandan Das

This feature emulates the "Acknowledge interrupt on exit" behavior. We can safely emulate it for L1 to run L2 even if L0 itself has it disabled (to run L1).

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/kvm/irq.c |  1 +
 arch/x86/kvm/vmx.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

arch/x86/kvm/irq.c +1 -0

index 484bc87..bd0da43 100644
@@ -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)
 {

arch/x86/kvm/vmx.c +12 -0

index 3e7f60c..bdc8f2d 100644
@@ -4489,6 +4489,18 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+/*
+ * In nested virtualization, check if L1 has enabled
+ * interrupt acknowledgement that writes the interrupt vector
+ * info on vmexit
+ * 
+ */
+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 &

@@ -8442,6 +8454,13 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 	prepare_vmcs12(vcpu, vmcs12, exit_reason, exit_intr_info,
 		       exit_qualification);

Quoted 5 lines

arch/x86/kvm/vmx.c static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason, · line 8459
Quoted 3 lines
arch/x86/kvm/vmx.c +3 -0
+	if ((exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
+	    && nested_exit_intr_ack_set(vcpu)) {
+		int irq = kvm_cpu_get_interrupt(vcpu);
Can irq be -1 ?

If it is, I think that's a bug because if we exited for this reason with INTR_ACK set, the hypervisor expects a valid vector number to be available.

What about adding a BUG_ON ?

Quoted 14 lines

arch/x86/kvm/vmx.c +4 -0

+		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,

Signature · 6 lines

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marcelo Tosatti

2014-04-11 19:31 UTC #18 lore raw

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

Quoted 58 lines · Bandan Das

Quoted 50 lines · Marcelo Tosatti

Quoted 49 lines · Bandan Das

This feature emulates the "Acknowledge interrupt on exit" behavior. We can safely emulate it for L1 to run L2 even if L0 itself has it disabled (to run L1).

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/kvm/irq.c |  1 +
 arch/x86/kvm/vmx.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

arch/x86/kvm/irq.c +1 -0

index 484bc87..bd0da43 100644
@@ -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)
 {

arch/x86/kvm/vmx.c +12 -0

index 3e7f60c..bdc8f2d 100644
@@ -4489,6 +4489,18 @@ static bool nested_exit_on_intr(struct kvm_vcpu *vcpu)
 		PIN_BASED_EXT_INTR_MASK;
 }
 
+/*
+ * In nested virtualization, check if L1 has enabled
+ * interrupt acknowledgement that writes the interrupt vector
+ * info on vmexit
+ * 
+ */
+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 &

@@ -8442,6 +8454,13 @@ static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason,
 	prepare_vmcs12(vcpu, vmcs12, exit_reason, exit_intr_info,
 		       exit_qualification);

Quoted 5 lines

arch/x86/kvm/vmx.c static void nested_vmx_vmexit(struct kvm_vcpu *vcpu, u32 exit_reason, · line 8459
Quoted 3 lines
arch/x86/kvm/vmx.c +3 -0
+	if ((exit_reason == EXIT_REASON_EXTERNAL_INTERRUPT)
+	    && nested_exit_intr_ack_set(vcpu)) {
+		int irq = kvm_cpu_get_interrupt(vcpu);
Can irq be -1 ?

Quoted 5 lines

If it is, I think that's a bug because if we exited for this reason with INTR_ACK set, the hypervisor expects a valid vector number to be available.
What about adding a BUG_ON ?

Sounds good.

Paolo Bonzini

maintainer

2014-04-12 16:57 UTC #19 lore raw

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

Il 11/04/2014 15:20, Marcelo Tosatti ha scritto:

Quoted 9 lines

Quoted 8 lines
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
Can irq be -1 ?
If it is, I think that's a bug because if we exited for this reason with INTR_ACK set, the hypervisor expects a valid vector number to be available.
What about adding a BUG_ON ?
Sounds good.

Each BUG_ON we add to KVM is a potential guest-kill-host vulnerability.
WARN_ON is better.

Paolo

Bandan Das

author

2014-03-31 21:01 UTC #20 lore raw

PATCH v2 3/3 KVM: nVMX: Advertise support for interrupt acknowledgement

In reply to Bandan Das

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

Signed-off-by Bandan Das <[email protected]>

---
 arch/x86/kvm/vmx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/x86/kvm/vmx.c +2 -1

index e864b7a..a2a03c5 100644
@@ -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,

Signature · 1 line

1.8.3.1