Re: [PATCH 2/3] KVM: Add irqdevice object

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

Gregory Haskins wrote:
> From: None <None>
>
> The current code is geared towards using a user-mode (A)PIC.  This patch adds
> an "irqdevice" abstraction, and implements a "userint" model to handle the
> duties of the original code.  Later, we can develop other irqdevice models 
> to handle objects like LAPIC, IOAPIC, i8259, etc, as appropriate
>
>   

Viewed in light of 3/3, various races are exposed.


> @@ -2044,13 +2043,11 @@ static int kvm_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
>  	if (mmu_reset_needed)
>  		kvm_mmu_reset_context(vcpu);
>  
> -	memcpy(vcpu->irq_pending, sregs->interrupt_bitmap,
> -	       sizeof vcpu->irq_pending);
> -	vcpu->irq_summary = 0;
> -	for (i = 0; i < NR_IRQ_WORDS; ++i)
> -		if (vcpu->irq_pending[i])
> -			__set_bit(i, &vcpu->irq_summary);
> -
> +	/* walk the interrupt-bitmap and inject an IRQ for each bit found */
> +	for (i = 0; i < 256; ++i)
> +		if (test_bit(i, &sregs->interrupt_bitmap[0]))
> +			kvm_irqdevice_set_pin(&vcpu->irq_dev, i, 1);
> + 
>   

You need to lower a pin here if it was previously set.


> diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
> index 61a6116..a0fdf02 100644
> --- a/drivers/kvm/vmx.c
> +++ b/drivers/kvm/vmx.c
> @@ -1219,13 +1219,8 @@ static void inject_rmode_irq(struct kvm_vcpu *vcpu, int irq)
>  
>  static void kvm_do_inject_irq(struct kvm_vcpu *vcpu)
>  {
> -	int word_index = __ffs(vcpu->irq_summary);
> -	int bit_index = __ffs(vcpu->irq_pending[word_index]);
> -	int irq = word_index * BITS_PER_LONG + bit_index;
> -
> -	clear_bit(bit_index, &vcpu->irq_pending[word_index]);
> -	if (!vcpu->irq_pending[word_index])
> -		clear_bit(word_index, &vcpu->irq_summary);
> +	int irq = kvm_irqdevice_read_vector(&vcpu->irq_dev, 0);
> +	BUG_ON(irq < 0);
>   

This BUG can trigger.  A level-triggered irq was asserted, then deasserted.

>  
>  	if (vcpu->rmode.active) {
>  		inject_rmode_irq(vcpu, irq);
> @@ -1246,7 +1241,7 @@ static void do_interrupt_requests(struct kvm_vcpu *vcpu,
>  		 (vmcs_read32(GUEST_INTERRUPTIBILITY_INFO) & 3) == 0);
>  
>  	if (vcpu->interrupt_window_open &&
> -	    vcpu->irq_summary &&
> +	    kvm_irqdevice_pending(&vcpu->irq_dev, 0) &&
>  	    !(vmcs_read32(VM_ENTRY_INTR_INFO_FIELD) & INTR_INFO_VALID_MASK))
>   

What if an irq is made pending here?

>  		/*
>  		 * If interrupts enabled, and not blocked by sti or mov ss. Good.
> @@ -1255,7 +1250,8 @@ static void do_interrupt_requests(struct kvm_vcpu *vcpu,
>  
>  	cpu_based_vm_exec_control = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL);
>  	if (!vcpu->interrupt_window_open &&
> -	    (vcpu->irq_summary || kvm_run->request_interrupt_window))
> +	    (kvm_irqdevice_pending(&vcpu->irq_dev, 0) ||
> +	     kvm_run->request_interrupt_window))
>  		/*
>  		 * Interrupts blocked.  Wait for unblock.
>  		 */
>   

or here?

> @@ -1619,8 +1615,9 @@ static void post_kvm_run_save(struct kvm_vcpu *vcpu,
>  	kvm_run->if_flag = (vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF) != 0;
>  	kvm_run->cr8 = vcpu->cr8;
>  	kvm_run->apic_base = vcpu->apic_base;
> -	kvm_run->ready_for_interrupt_injection = (vcpu->interrupt_window_open &&
> -						  vcpu->irq_summary == 0);
> +	kvm_run->ready_for_interrupt_injection = 
> +		(vcpu->interrupt_window_open && 
> +		 !kvm_irqdevice_pending(&vcpu->irq_dev, 0));
>  }
>   

or here? possibly a good answer is "don't rely on r_f_i_i if not using 
userint".

>  
>  static int handle_interrupt_window(struct kvm_vcpu *vcpu,
> @@ -1631,7 +1628,7 @@ static int handle_interrupt_window(struct kvm_vcpu *vcpu,
>  	 * possible
>  	 */
>  	if (kvm_run->request_interrupt_window &&
> -	    !vcpu->irq_summary) {
> +	    !kvm_irqdevice_pending(&vcpu->irq_dev, 0)) {
>  		kvm_run->exit_reason = KVM_EXIT_IRQ_WINDOW_OPEN;
>  		++kvm_stat.irq_window_exits;
>  		return 0;
>   

ditto.

> @@ -1713,7 +1710,7 @@ static int kvm_handle_exit(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
>  static int dm_request_for_irq_injection(struct kvm_vcpu *vcpu,
>  					  struct kvm_run *kvm_run)
>  {
> -	return (!vcpu->irq_summary &&
> +	return (!kvm_irqdevice_pending(&vcpu->irq_dev, 0) &&
>  		kvm_run->request_interrupt_window &&
>  		vcpu->interrupt_window_open &&
>  		(vmcs_readl(GUEST_RFLAGS) & X86_EFLAGS_IF));
>
>
>   

ditto.


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