From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 05/10] KVM: Merge MSI handling to kvm_set_irq Date: Tue, 30 Dec 2008 12:48:42 +0200 Message-ID: <4959FC8A.70808@redhat.com> References: <1230616562-18113-1-git-send-email-sheng@linux.intel.com> <1230616562-18113-6-git-send-email-sheng@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org To: Sheng Yang Return-path: Received: from mx2.redhat.com ([66.187.237.31]:45344 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752995AbYL3Ksq (ORCPT ); Tue, 30 Dec 2008 05:48:46 -0500 In-Reply-To: <1230616562-18113-6-git-send-email-sheng@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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