Re: [PATCH 3/4] KVM: Adds ability to preempt an executing VCPU

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Gregory Haskins wrote:
> The VCPU executes synchronously w.r.t. userspace today, and therefore
> interrupt injection is pretty straight forward.  However, we will soon need
> to be able to inject interrupts asynchronous to the execution of the VCPU
> due to the introduction of SMP, paravirtualized drivers, and asynchronous
> hypercalls.  This patch adds support to the interrupt mechanism to force
> a VCPU to VMEXIT when a new interrupt is pending.
>
> Signed-off-by: Gregory Haskins <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>
> ---
>
>  drivers/kvm/kvm.h      |    5 +++
>  drivers/kvm/kvm_main.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/kvm/svm.c      |   43 ++++++++++++++++++++++++++++
>  drivers/kvm/vmx.c      |   43 ++++++++++++++++++++++++++++
>  4 files changed, 164 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
> index d41d653..15c8bec 100644
> --- a/drivers/kvm/kvm.h
> +++ b/drivers/kvm/kvm.h
> @@ -321,6 +321,8 @@ void kvm_io_bus_register_dev(struct kvm_io_bus *bus,
>  
>  #define NR_IRQ_WORDS KVM_IRQ_BITMAP_SIZE(unsigned long)
>  
> +#define KVM_SIGNAL_VIRTUAL_INTERRUPT 33 /* Hardcoded for now */
> +
>  /*
>   * structure for maintaining info for interrupting an executing VCPU
>   */
> @@ -329,6 +331,9 @@ struct kvm_vcpu_irq {
>  	struct kvm_irqdevice dev;
>  	int                  pending;
>  	int                  deferred;
> +	struct task_struct  *task;
> +	int                  signo;
> +	int                  guest_mode;
>  };
>  
>  struct kvm_vcpu {
> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> index 9aeb2f7..6acbd9b 100644
> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -304,6 +304,10 @@ static struct kvm *kvm_create_vm(void)
>  		memset(&vcpu->irq, 0, sizeof(vcpu->irq));
>  		spin_lock_init(&vcpu->irq.lock);
>  		vcpu->irq.deferred = -1;
> +		/*
> +		 * This should be settable by userspace someday
> +		 */
> +		vcpu->irq.signo = KVM_SIGNAL_VIRTUAL_INTERRUPT;
>   

This needs to be fixed prior to merging.  Hopefully not by setting the 
signal number, bit by making the vcpu fd writable (userspace can attach 
a signal to the fd if it wishes).

>  
>  		vcpu->cpu = -1;
>  		vcpu->kvm = kvm;
> @@ -366,13 +370,20 @@ static void free_pio_guest_pages(struct kvm_vcpu *vcpu)
>  
>  static void kvm_free_vcpu(struct kvm_vcpu *vcpu)
>  {
> +	unsigned long irqsave;
> +
>  	if (!vcpu->vmcs)
>  		return;
>  
>  	vcpu_load(vcpu);
>  	kvm_mmu_destroy(vcpu);
>  	vcpu_put(vcpu);
> +
> +	spin_lock_irqsave(&vcpu->irq.lock, irqsave);
> +	vcpu->irq.task = NULL;
> +	spin_unlock_irqrestore(&vcpu->irq.lock, irqsave);
>   

Can irq.task be non-NULL here at all?  Also, we only free vcpus when we 
destroy the vm, and paravirt drivers would hopefully hold a ref to the 
vm, so there's nobody to race against here.

>  	kvm_irqdevice_destructor(&vcpu->irq.dev);
>  
> @@ -1868,6 +1880,10 @@ static int kvm_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  		kvm_arch_ops->decache_regs(vcpu);
>  	}
>  
> +	spin_lock_irqsave(&vcpu->irq.lock, irqsaved);
> +	vcpu->irq.task = current;
> +	spin_unlock_irqrestore(&vcpu->irq.lock, irqsaved);
> +
>   

Just assignment + __smp_wmb().

> +/*
>   * This function will be invoked whenever the vcpu->irq.dev raises its INTR
>   * line
>   */
> @@ -2318,10 +2348,52 @@ static void kvm_vcpu_intr(struct kvm_irqsink *this,
>  {
>  	struct kvm_vcpu *vcpu = (struct kvm_vcpu*)this->private;
>  	unsigned long flags;
> +	int direct_ipi = -1;
>  
>  	spin_lock_irqsave(&vcpu->irq.lock, flags);
> -	__set_bit(pin, &vcpu->irq.pending);
> +
> +	if (!test_bit(pin, &vcpu->irq.pending)) {
> +		/*
> +		 * Record the change..
> +		 */
> +		__set_bit(pin, &vcpu->irq.pending);
> +
> +		/*
> +		 * then wake up the vcpu (if necessary)
> +		 */
> +		if (vcpu->irq.task && (vcpu->irq.task != current)) {
> +			if (vcpu->irq.guest_mode) {
> +				/*
> +				 * If we are in guest mode, we can optimize
> +				 * the IPI by executing a function directly
> +				 * on the owning processor.
> +				 */
> +				direct_ipi = task_cpu(vcpu->irq.task);
> +				BUG_ON(direct_ipi == smp_processor_id());
> +			} else
> +				/*
> +				 * otherwise, we must assume that we could be
> +				 * blocked anywhere, including userspace. Send
> +				 * a signal to give everyone a chance to get
> +				 * notification
> +				 */
> +				send_sig(vcpu->irq.signo, vcpu->irq.task, 0);
> +		}
> +	}
> +
>  	spin_unlock_irqrestore(&vcpu->irq.lock, flags);
> +
> +	if (direct_ipi != -1) {
> +		/*
> +		 * Not sure if disabling preemption is needed.
> +		 * The kick_process() code does this so I copied it
> +		 */
> +		preempt_disable();
> +		smp_call_function_single(direct_ipi,
> +					 kvm_vcpu_guest_intr,
> +					 vcpu, 0, 0);
> +		preempt_enable();
> +	}
>   

I see why you must issue the IPI outside the spin_lock_irqsave(), but 
aren't you now opening a race?  vcpu enters guest mode, irq on other 
cpu, irq sets direct_ipi to wakeup guest, releases lock, vcpu exits to 
userspace (or migrates to another cpu), ipi is issued but nobody cares.


>  	/*
> +	 * Signal that we have transitioned back to host mode
> +	 */
> +	spin_lock_irqsave(&vcpu->irq.lock, irq_flags);
> +	vcpu->irq.guest_mode = 0;
> +	spin_unlock_irqrestore(&vcpu->irq.lock, irq_flags);
> +
>   

Assign + __smp_wmb().

> +	/*
> +	 * Signal that we have transitioned back to host mode
> +	 */
> +	spin_lock_irqsave(&vcpu->irq.lock, irq_flags);
> +	vcpu->irq.guest_mode = 0;
> +	spin_unlock_irqrestore(&vcpu->irq.lock, irq_flags);
>   

Again.


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/