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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
