>>> On Sat, Apr 14, 2007 at 10:30 AM, in message <[EMAIL PROTECTED]>,
Avi Kivity <[EMAIL PROTECTED]> wrote:
> Gregory Haskins wrote:
>>> It's really subtle.
>>>
>>> With respect to interrupts, VT^H^Hthe hardware provides an override over
>>> IF. If an interrupt happens while this override is enabled, we exit
>>> guest mode regardless of the state of IF. In effect interrupts are
>>> enabled but _delivery_ is disabled. Once we exit guest mode, interrupts
>>> become disabled again until we set IF explicitly.
>>>
>>
>> Heres a version that implements that idea. Note that its still a POC, as
> the kick_process still needs to be exported and handled in
> extern-module-compat (or whatever its called)
>>
>
> Hopefully Ingo will ack such a patch, as he suggested kick_process()
> originally.
Out of curiosity, did you review my kick_process() version or the newer
smp_call_function_single() based version of the patch?
>
>
>> @@ -1866,6 +1874,19 @@ static int kvm_vcpu_ioctl_run(struct kvm_vcpu *vcpu,
> struct kvm_run *kvm_run)
>> kvm_arch_ops->decache_regs(vcpu);
>> }
>>
>> + /*
>> + * Make sure the current task is accurately recorded. Most of the
>> + * time, it will be so we first check if its necessary without taking
>> + * the lock. If there is a mismatch, we must aquire the lock and
>> + * check again to eliminate races
>> + */
>> + if (unlikely(vcpu->irq.task != current)) {
>> + spin_lock(&vcpu->irq.lock);
>> + if (vcpu->irq.task != current)
>> + vcpu->irq.task = current;
>> + spin_unlock(&vcpu->irq.lock);
>> + }
>> +
>>
>
> Isn't this equivalent to
>
> vcpu->irq.task = current
Yeah, good point. I originally had a printk in there to report when it
changes, thus the conditional. I didn't collapse it when I took the printk
out. I suppose you could argue that updating the variable indiscriminately
will unnecessarily dirty a cache-line, but I doubt it would matter in the grand
scheme of things.
>
> ?
>
> btw, these double-check things are broken, see
> http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html.
> It will usually work on x86, and can be made to work elsewhere with
> memory barriers, but it's less simple than it appears.
Interesting. Thanks for the link!
> stgi() / clgi() is what implements interrupt handling under svm. I
> think you got this right, but please tell me you read the relevant
> sections in the manual a few times.
Actually, no. I have read the VMX doc pretty thoroughly but I haven't looked
at the SVN doc at all yet. I'm still drinking from a fire hose between VMX,
KVM, QEMU etc. ;) I will certainly rectify this situation before I submit the
patch for actual inclusion. If I do anything particularly egregious in SVN,
pointers are more than welcome.
>
>
>> @@ -1617,6 +1650,16 @@ again:
>> reload_tss(vcpu);
>>
>> /*
>> + * Signal that we have transitioned back to host mode
>> + */
>> + spin_lock(&vcpu->irq.lock);
>> +
>> + vcpu->irq.guest_mode = 0;
>> + vcpu->irq.pending = 0;
>> +
>> + spin_unlock(&vcpu->irq.lock);
>> +
>>
>
> If an interrupt happened just before here, then we need to check it,
> otherwise it's lost. Also an opportunity to see ->read_vector in
> action, which was the purpose of the exercise IIRC.
I'm not sure if you noticed this, but the read_vector() call already happened
as part of the do_interrupt_requests() call before the VMENTRY (assuming you
have applied the previous patch in the series, of course ;). You would never
"lose" the interrupt even if the interrupt came in before the critical section
because it would be safely queued in the irqdevice model for the next pass
through the run() loop. However, you do highlight a bug in this code that
could cause the interrupt to be deferred indefinitely until a second interrupt
came in if the processor happens to halt after this VMEXIT. I.e., since a
signal isn't being delivered while in irq.guest_mode = 1, we do need to handle
this at this layer by looping back into the CPU. I will fix this. Good find!
>
>>
>> asm ("mov %0, %%ds; mov %0, %%es" : : "r"(__USER_DS));
>>
>> + /*
>> + * Signal that we have transitioned back to host mode
>> + */
>> + spin_lock(&vcpu->irq.lock);
>> +
>> + vcpu->irq.guest_mode = 0;
>> + vcpu->irq.pending = 0;
>> +
>> + spin_unlock(&vcpu->irq.lock);
>> +
>> if (fail) {
>> kvm_run->exit_reason = KVM_EXIT_FAIL_ENTRY;
>> kvm_run->fail_entry.hardware_entry_failure_reason
>>
>
> Ditto.
>
> You also want spin_lock_irq() when interrupts are enabled, since some
> callers will lock it in interrupt context.
>
Good catch. I forgot to update the locks after the changeover to disabling
IRQs.
-------------------------------------------------------------------------
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
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
