Re: [PATCH 05/10] KVM: Merge MSI handling to kvm_set_irq

[Avi Kivity <avi@redhat.com>]

Sheng Yang wrote:
> Using kvm_set_irq to handle all interrupt injection.
>
>  
>  /* This should be called with the kvm->lock mutex held */
> -void kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
> +void kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 gsi, int level)
>  {
> -	unsigned long *irq_state = (unsigned long *)&kvm->arch.irq_states[irq];
> -
> -	/* Logical OR for level trig interrupt */
> -	if (level)
> -		set_bit(irq_source_id, irq_state);
> -	else
> -		clear_bit(irq_source_id, irq_state);
> -
> -	/* Not possible to detect if the guest uses the PIC or the
> -	 * IOAPIC.  So set the bit in both. The guest will ignore
> -	 * writes to the unused one.
> -	 */
> -	kvm_ioapic_set_irq(ioapic_irqchip(kvm), irq, !!(*irq_state));
> +	unsigned long *irq_state;
> +#ifdef CONFIG_X86
> +	int vcpu_id;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_ioapic *ioapic = ioapic_irqchip(kvm);
> +	struct kvm_gsi_msg *gsi_msg;
> +	int dest_id, vector, dest_mode, trig_mode, delivery_mode;
> +	u32 deliver_bitmask;
> +
> +	BUG_ON(!ioapic);
> +#endif
> +
> +	if (!(gsi & KVM_GSI_MSG_MASK)) {
> +		int irq = gsi;
> +
> +		irq_state = (unsigned long *)&kvm->arch.irq_states[irq];
> +
> +		/* Logical OR for level trig interrupt */
> +		if (level)
> +			set_bit(irq_source_id, irq_state);
> +		else
> +			clear_bit(irq_source_id, irq_state);
> +
> +		/* Not possible to detect if the guest uses the PIC or the
> +		 * IOAPIC.  So set the bit in both. The guest will ignore
> +		 * writes to the unused one.
> +		 */
> +		kvm_ioapic_set_irq(ioapic, irq, !!(*irq_state));
>  #ifdef CONFIG_X86
> -	kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state));
> +		kvm_pic_set_irq(pic_irqchip(kvm), irq, !!(*irq_state));
> +#endif
> +		return;
> +	}
> +
> +#ifdef CONFIG_X86
> +	mutex_lock(&kvm->gsi_msg_lock);
>   

The lock is already taken here?

> +	gsi_msg = kvm_find_gsi_msg(kvm, gsi);
> +	mutex_unlock(&kvm->gsi_msg_lock);
> +	if (!gsi_msg) {
> +		printk(KERN_WARNING "kvm: fail to find correlated gsi_msg\n");
> +		return;
> +	}
> +
> +	dest_id = (gsi_msg->msg.address_lo & MSI_ADDR_DEST_ID_MASK)
> +			>> MSI_ADDR_DEST_ID_SHIFT;
> +	vector = (gsi_msg->msg.data & MSI_DATA_VECTOR_MASK)
> +			>> MSI_DATA_VECTOR_SHIFT;
> +	dest_mode = test_bit(MSI_ADDR_DEST_MODE_SHIFT,
> +				(unsigned long *)&gsi_msg->msg.address_lo);
> +	trig_mode = test_bit(MSI_DATA_TRIGGER_SHIFT,
> +				(unsigned long *)&gsi_msg->msg.data);
> +	delivery_mode = test_bit(MSI_DATA_DELIVERY_MODE_SHIFT,
> +				(unsigned long *)&gsi_msg->msg.data);
> +	deliver_bitmask = kvm_ioapic_get_delivery_bitmask(ioapic,
> +				dest_id, dest_mode);
> +	/* IOAPIC delivery mode value is the same as MSI here */
> +	switch (delivery_mode) {
> +	case IOAPIC_LOWEST_PRIORITY:
> +		vcpu = kvm_get_lowest_prio_vcpu(ioapic->kvm, vector,
> +				deliver_bitmask);
> +		if (vcpu != NULL)
> +			kvm_apic_set_irq(vcpu, vector, trig_mode);
> +		else
> +			printk(KERN_INFO "kvm: null lowest priority vcpu!\n");
> +		break;
> +	case IOAPIC_FIXED:
> +		for (vcpu_id = 0; deliver_bitmask != 0; vcpu_id++) {
> +			if (!(deliver_bitmask & (1 << vcpu_id)))
> +				continue;
> +			deliver_bitmask &= ~(1 << vcpu_id);
> +			vcpu = ioapic->kvm->vcpus[vcpu_id];
> +			if (vcpu)
> +				kvm_apic_set_irq(vcpu, vector, trig_mode);
> +		}
> +		break;
> +	default:
> +		printk(KERN_INFO "kvm: unsupported MSI delivery mode\n");
> +	}
>  #endif
>  }
>  
>   

This looks very messy.  Would be better to have the in-kernel irq 
structure contain a (*set_level)() callback that can take the 
appropriate action.

Also, the CONFIG_X86 worries me, can we have IA64 enable this as well?

-- 
error compiling committee.c: too many arguments to function