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

From: Sheng Yang <sheng@linux.intel.com>
To: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>, kvm@vger.kernel.org
Subject: Re: [PATCH 05/10] KVM: Merge MSI handling to kvm_set_irq
Date: Tue, 30 Dec 2008 19:00:57 +0800	[thread overview]
Message-ID: <200812301900.58066.sheng@linux.intel.com> (raw)
In-Reply-To: <4959FC8A.70808@redhat.com>

On Tuesday 30 December 2008 18:48:42 Avi Kivity wrote:
> 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?

Um? For gsi_msg_lock?
>
> > +	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.

You means this part which would merged with ioapic, or something else?
>
> Also, the CONFIG_X86 worries me, can we have IA64 enable this as well?

IA64 MSI enabling is on the task list, but it's pity we are too busy 
recently... 

-- 
regards
Yang, Sheng