diff mbox

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

Message ID 1396299625-8285-3-git-send-email-bsd@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bandan Das March 31, 2014, 9 p.m. UTC
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 <bsd@redhat.com>
---
 arch/x86/kvm/irq.c |  1 +
 arch/x86/kvm/vmx.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

Comments

Marcelo Tosatti April 11, 2014, 6:33 p.m. UTC | #1
On Mon, Mar 31, 2014 at 05:00:24PM -0400, Bandan Das wrote:
> 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 <bsd@redhat.com>
> ---
>  arch/x86/kvm/irq.c |  1 +
>  arch/x86/kvm/vmx.c | 19 +++++++++++++++++++
>  2 files changed, 20 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 3e7f60c..bdc8f2d 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -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);

Can irq be -1 ?

> +		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
> 
> --
> 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
--
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
Bandan Das April 11, 2014, 7:17 p.m. UTC | #2
Marcelo Tosatti <mtosatti@redhat.com> writes:

> On Mon, Mar 31, 2014 at 05:00:24PM -0400, Bandan Das wrote:
>> 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 <bsd@redhat.com>
>> ---
>>  arch/x86/kvm/irq.c |  1 +
>>  arch/x86/kvm/vmx.c | 19 +++++++++++++++++++
>>  2 files changed, 20 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 3e7f60c..bdc8f2d 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -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);
>
> 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 ?

>> +		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
>> 
>> --
>> 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
--
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
Marcelo Tosatti April 11, 2014, 7:20 p.m. UTC | #3
On Fri, Apr 11, 2014 at 03:17:47PM -0400, Bandan Das wrote:
> Marcelo Tosatti <mtosatti@redhat.com> writes:
> 
> > On Mon, Mar 31, 2014 at 05:00:24PM -0400, Bandan Das wrote:
> >> 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 <bsd@redhat.com>
> >> ---
> >>  arch/x86/kvm/irq.c |  1 +
> >>  arch/x86/kvm/vmx.c | 19 +++++++++++++++++++
> >>  2 files changed, 20 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 3e7f60c..bdc8f2d 100644
> >> --- a/arch/x86/kvm/vmx.c
> >> +++ b/arch/x86/kvm/vmx.c
> >> @@ -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);
> >
> > 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.

--
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
Paolo Bonzini April 12, 2014, 4:57 p.m. UTC | #4
Il 11/04/2014 15:20, Marcelo Tosatti ha scritto:
>>> > >
>>> > > 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
--
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
diff mbox

Patch

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 3e7f60c..bdc8f2d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -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,