Re: irqdevice INTR example

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

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.


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

?

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.


> +	 * FIXME: Do we need to do anything additional to mask IPI/NMIs? 
>   

I'm fairly certain you don't.  I keep hearing about the deadlockability
of sending ipis with interrupts disabled.

> +	 */
> +	local_irq_save(irq_flags);
> +
> +	spin_lock(&vcpu->irq.lock);
> +
> +	/*
> +	 * There are optimizations we can make when signaling interrupts
> +	 * if we know the VCPU is in GUEST mode, so mark that here
> +	 */
> +	vcpu->irq.guest_mode = 1;
> +
> +	/*
> +	 * We also must inject interrupts (if any) while the irq_lock
> +	 * is held
> +	 */
>  	if (!vcpu->mmio_read_completed)
>  		do_interrupt_requests(vcpu, kvm_run);
>  
> +	spin_unlock(&vcpu->irq.lock);
> +
>  	clgi();
>   

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.


> @@ -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.

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


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.


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