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

["Gregory Haskins" <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>]

>>> On Mon, May 14, 2007 at  5:34 AM, in message <46482D2E.7040809-atKUWr5tajBWk0Htik3J/w@public.gmane.org>,
Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org> wrote: 
> 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.

The problem with doing it this way is that its no longer possible to detect the optimizing condition of "irq.task != current" when injecting interrupts.  This means that userspace will be inadvertently sending itself a signal every time it injects interrupts, which IMHO is undesirable.

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

I can do this, but its common to all platforms so I figured it was best to be out here?

> 
>> +/*
>>   * 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.

This and the rest of your comments make sense.  Consider them all acked.

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




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