Re: [PATCH 3/8] 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.
>
>   

Comments below are fairly minor, but worthwhile IMO.



> Signed-off-by: Gregory Haskins <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>
> ---
>
>  drivers/kvm/kvm.h      |    2 ++
>  drivers/kvm/kvm_main.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/kvm/svm.c      |   43 +++++++++++++++++++++++++++++++++++
>  drivers/kvm/vmx.c      |   43 +++++++++++++++++++++++++++++++++++
>  4 files changed, 146 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
> index 059f074..0f6cc32 100644
> --- a/drivers/kvm/kvm.h
> +++ b/drivers/kvm/kvm.h
> @@ -329,6 +329,8 @@ struct kvm_vcpu_irq {
>  	struct kvm_irqdevice dev;
>  	int                  pending;
>  	int                  deferred;
> +	struct task_struct  *task;
> +	int                  guest_mode;
>   

->guest_mode can be folded into ->task, by specifying that ->task != 
NULL is equivalent to ->guest_mode != 0.  This will make the rest of the 
code easier to read.

>  };
>  
>  struct kvm_vcpu {
> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> index 199489b..a160638 100644
> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -1868,6 +1868,9 @@ static int kvm_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  		kvm_arch_ops->decache_regs(vcpu);
>  	}
>  
> +	vcpu->irq.task = current;
> +	smp_wmb();
> +
>   

This is best moved where ->guest_mode is set.

> +/*
>   * This function will be invoked whenever the vcpu->irq.dev raises its INTR
>   * line
>   */
> @@ -2318,10 +2335,50 @@ 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);
>   

irqs are always enabled here, so spin_lock_irq() (and a corresponding 
spin_unlock_irq) is sufficient.

>  static void kvm_vcpu_irqsink_init(struct kvm_vcpu *vcpu)
> diff --git a/drivers/kvm/svm.c b/drivers/kvm/svm.c
> index 4c03881..91546ae 100644
> --- a/drivers/kvm/svm.c
> +++ b/drivers/kvm/svm.c
> @@ -1542,11 +1542,40 @@ static int svm_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	u16 gs_selector;
>  	u16 ldt_selector;
>  	int r;
> +	unsigned long irq_flags;
>  
>  again:
> +	/*
> +	 * We disable interrupts until the next VMEXIT to eliminate a race
> +	 * condition for delivery of virtual interrutps.  Note that this is
> +	 * probably not as bad as it sounds, as interrupts will still invoke
> +	 * a VMEXIT once transitioned to GUEST mode (and thus exit this lock
> +	 * scope) even if they are disabled.
> +	 *
> +	 * FIXME: Do we need to do anything additional to mask IPI/NMIs?
>   

You can remove the FIXME.

> +	 */
> +	local_irq_save(irq_flags);
>   

Interrupts are always enabled here, so local_irq_disable() suffices.

> @@ -1688,6 +1717,13 @@ again:
>  #endif
>  		: "cc", "memory" );
>  
> +	/*
> +	 * FIXME: We'd like to turn on interrupts ASAP, but is this so early
> +	 * that we will mess up the state of the CPU before we fully
> +	 * transition from guest to host?
> +	 */
>   

You can remove the FIXME.  Pre-patch enabled interrupts much earlier.

> +	local_irq_restore(irq_flags);
> +
>  	if (vcpu->fpu_active) {
>  		fx_save(vcpu->guest_fx_image);
>  		fx_restore(vcpu->host_fx_image);
> @@ -1710,6 +1746,13 @@ again:
>  	reload_tss(vcpu);
>  
>  	/*
> +	 * 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);
>   

 >> Don't you need to check interrupts here?
 > No, we assume that host userspace won't sleep.
Right, I forgot again.


> (prof_on == KVM_PROFILING))
> diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
> index ca858cb..7b81fff 100644
> --- a/drivers/kvm/vmx.c
> +++ b/drivers/kvm/vmx.c
> @@ -1895,6 +1895,7 @@ static int vmx_vcpu_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>  	u16 fs_sel, gs_sel, ldt_sel;
>  	int fs_gs_ldt_reload_needed;
>  	int r;
> +	unsigned long irq_flags;
>  
>  preempted:
>  	/*
> @@ -1929,9 +1930,37 @@ preempted:
>  	if (vcpu->guest_debug.enabled)
>  		kvm_guest_debug_pre(vcpu);
>  
> +	/*
> +	 * We disable interrupts until the next VMEXIT to eliminate a race
> +	 * condition for delivery of virtual interrutps.  Note that this is
> +	 * probably not as bad as it sounds, as interrupts will still invoke
> +	 * a VMEXIT once transitioned to GUEST mode (and thus exit this lock
> +	 * scope) even if they are disabled.
> +	 *
> +	 * FIXME: Do we need to do anything additional to mask IPI/NMIs?
> +	 */
> +	local_irq_save(irq_flags);
> +
>   

Pretty much same comments apply here.  One day we'll unify some of this 
code.


[...]


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