>>> On Mon, May 14, 2007 at 5:34 AM, in message <[EMAIL PROTECTED]>,
Avi Kivity <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]>
>> ---
>>
>> 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/
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
