On Tue, Jul 14, 2009 at 05:30:43PM +0300, Gleb Natapov wrote: > > Signed-off-by: Gleb Natapov <gleb@xxxxxxxxxx> > --- > arch/ia64/kvm/kvm-ia64.c | 27 ++++++++++++++++++------ > arch/x86/kvm/lapic.c | 5 +--- > arch/x86/kvm/x86.c | 30 ++++++++++++++++++--------- > virt/kvm/ioapic.c | 50 ++++++++++++++++++++++++++++----------------- > virt/kvm/ioapic.h | 1 + > 5 files changed, 73 insertions(+), 40 deletions(-) > > diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c > index 0ad09f0..dd7ef2d 100644 > --- a/arch/ia64/kvm/kvm-ia64.c > +++ b/arch/ia64/kvm/kvm-ia64.c > @@ -850,9 +850,16 @@ static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, > > r = 0; > switch (chip->chip_id) { > - case KVM_IRQCHIP_IOAPIC: > - memcpy(&chip->chip.ioapic, ioapic_irqchip(kvm), > - sizeof(struct kvm_ioapic_state)); > + case KVM_IRQCHIP_IOAPIC: { > + struct kvm_ioapic *ioapic = ioapic_irqchip(kvm); > + if (ioapic) { > + spin_lock(&ioapic->lock); > + memcpy(&chip->chip.ioapic, ioapic, > + sizeof(struct kvm_ioapic_state)); > + spin_unlock(&ioapic->lock); > + } else > + r = -EINVAL; > + } > break; > default: > r = -EINVAL; > @@ -867,10 +874,16 @@ static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip) > > r = 0; > switch (chip->chip_id) { > - case KVM_IRQCHIP_IOAPIC: > - memcpy(ioapic_irqchip(kvm), > - &chip->chip.ioapic, > - sizeof(struct kvm_ioapic_state)); > + case KVM_IRQCHIP_IOAPIC: { > + struct kvm_ioapic *ioapic = ioapic_irqchip(kvm); > + if (ioapic) { > + spin_lock(&ioapic->lock); > + memcpy(ioapic, &chip->chip.ioapic, > + sizeof(struct kvm_ioapic_state)); > + spin_unlock(&ioapic->lock); > + } else > + r = -EINVAL; > + } > break; > default: > r = -EINVAL; > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index e2e2849..61ffcfc 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -471,11 +471,8 @@ static void apic_set_eoi(struct kvm_lapic *apic) > trigger_mode = IOAPIC_LEVEL_TRIG; > else > trigger_mode = IOAPIC_EDGE_TRIG; > - if (!(apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI)) { > - mutex_lock(&apic->vcpu->kvm->irq_lock); > + if (!(apic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI)) > kvm_ioapic_update_eoi(apic->vcpu->kvm, vector, trigger_mode); > - mutex_unlock(&apic->vcpu->kvm->irq_lock); > - } > } > > static void apic_send_ipi(struct kvm_lapic *apic) > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 48567fa..de99b84 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -2010,10 +2010,16 @@ static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm, struct kvm_irqchip *chip) > &pic_irqchip(kvm)->pics[1], > sizeof(struct kvm_pic_state)); > break; > - case KVM_IRQCHIP_IOAPIC: > - memcpy(&chip->chip.ioapic, > - ioapic_irqchip(kvm), > - sizeof(struct kvm_ioapic_state)); > + case KVM_IRQCHIP_IOAPIC: { > + struct kvm_ioapic *ioapic = ioapic_irqchip(kvm); > + if (ioapic) { > + spin_lock(&ioapic->lock); > + memcpy(&chip->chip.ioapic, ioapic, > + sizeof(struct kvm_ioapic_state)); > + spin_unlock(&ioapic->lock); > + } else > + r = -EINVAL; > + } > break; > default: > r = -EINVAL; > @@ -2042,12 +2048,16 @@ static int kvm_vm_ioctl_set_irqchip(struct kvm *kvm, struct kvm_irqchip *chip) > sizeof(struct kvm_pic_state)); > spin_unlock(&pic_irqchip(kvm)->lock); > break; > - case KVM_IRQCHIP_IOAPIC: > - mutex_lock(&kvm->irq_lock); > - memcpy(ioapic_irqchip(kvm), > - &chip->chip.ioapic, > - sizeof(struct kvm_ioapic_state)); > - mutex_unlock(&kvm->irq_lock); > + case KVM_IRQCHIP_IOAPIC: { > + struct kvm_ioapic *ioapic = ioapic_irqchip(kvm); > + if (ioapic) { > + spin_lock(&ioapic->lock); > + memcpy(ioapic, &chip->chip.ioapic, > + sizeof(struct kvm_ioapic_state)); > + spin_unlock(&ioapic->lock); > + } else > + r = -EINVAL; > + } > break; > default: > r = -EINVAL; > diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c > index fa05f67..300ee3b 100644 > --- a/virt/kvm/ioapic.c > +++ b/virt/kvm/ioapic.c > @@ -182,6 +182,7 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level) > union kvm_ioapic_redirect_entry entry; > int ret = 1; > > + spin_lock(&ioapic->lock); > if (irq >= 0 && irq < IOAPIC_NUM_PINS) { > entry = ioapic->redirtbl[irq]; > level ^= entry.fields.polarity; > @@ -196,34 +197,44 @@ int kvm_ioapic_set_irq(struct kvm_ioapic *ioapic, int irq, int level) > } > trace_kvm_ioapic_set_irq(entry.bits, irq, ret == 0); > } > + spin_unlock(&ioapic->lock); > + > return ret; > } > > -static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int pin, > - int trigger_mode) > +static void __kvm_ioapic_update_eoi(struct kvm_ioapic *ioapic, int vector, > + int trigger_mode) > { > - union kvm_ioapic_redirect_entry *ent; > + int i; > + > + for (i = 0; i < IOAPIC_NUM_PINS; i++) { > + union kvm_ioapic_redirect_entry *ent = &ioapic->redirtbl[i]; > + > + if (ent->fields.vector != vector) > + continue; > > - ent = &ioapic->redirtbl[pin]; > + /* ack notifier may call back into ioapic via kvm_set_irq() */ > + spin_unlock(&ioapic->lock); > + kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i); > + spin_lock(&ioapic->lock); While traversing the IOAPIC pins you drop the lock and acquire it again, which is error prone: what if the redirection table is changed in between, for example? Also, irq_lock was held during ack and mask notifier callbacks, so they (the callbacks) did not have to worry about concurrency. You questioned the purpose of irq_lock earlier, and thats what it is: synchronization between pic and ioapic blur at the irq notifiers. Using RCU to synchronize ack/mask notifier list walk versus list addition is good, but i'm not entirely sure that smaller locks like you are proposing is a benefit long term (things become much more tricky). Maybe it is the only way forward (and so you push this complexity to the ack/mask notifiers), but i think it can be avoided until its proven that there is contention on irq_lock (which is reduced by faster search). > - kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, pin); > + if (trigger_mode != IOAPIC_LEVEL_TRIG) > + continue; > > - if (trigger_mode == IOAPIC_LEVEL_TRIG) { > ASSERT(ent->fields.trig_mode == IOAPIC_LEVEL_TRIG); > ent->fields.remote_irr = 0; > - if (!ent->fields.mask && (ioapic->irr & (1 << pin))) > - ioapic_service(ioapic, pin); > + if (!ent->fields.mask && (ioapic->irr & (1 << i))) > + ioapic_service(ioapic, i); > } > } > > void kvm_ioapic_update_eoi(struct kvm *kvm, int vector, int trigger_mode) > { > struct kvm_ioapic *ioapic = kvm->arch.vioapic; > - int i; > > - for (i = 0; i < IOAPIC_NUM_PINS; i++) > - if (ioapic->redirtbl[i].fields.vector == vector) > - __kvm_ioapic_update_eoi(ioapic, i, trigger_mode); > + spin_lock(&ioapic->lock); > + __kvm_ioapic_update_eoi(ioapic, vector, trigger_mode); > + spin_unlock(&ioapic->lock); > } > > static inline struct kvm_ioapic *to_ioapic(struct kvm_io_device *dev) > @@ -248,8 +259,8 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len, > ioapic_debug("addr %lx\n", (unsigned long)addr); > ASSERT(!(addr & 0xf)); /* check alignment */ > > - mutex_lock(&ioapic->kvm->irq_lock); > addr &= 0xff; > + spin_lock(&ioapic->lock); > switch (addr) { > case IOAPIC_REG_SELECT: > result = ioapic->ioregsel; > @@ -263,6 +274,8 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len, > result = 0; > break; > } > + spin_unlock(&ioapic->lock); > + > switch (len) { > case 8: > *(u64 *) val = result; > @@ -275,7 +288,6 @@ static int ioapic_mmio_read(struct kvm_io_device *this, gpa_t addr, int len, > default: > printk(KERN_WARNING "ioapic: wrong length %d\n", len); > } > - mutex_unlock(&ioapic->kvm->irq_lock); > return 0; > } > > @@ -291,15 +303,15 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len, > (void*)addr, len, val); > ASSERT(!(addr & 0xf)); /* check alignment */ > > - mutex_lock(&ioapic->kvm->irq_lock); > if (len == 4 || len == 8) > data = *(u32 *) val; > else { > printk(KERN_WARNING "ioapic: Unsupported size %d\n", len); > - goto unlock; > + return 0; > } > > addr &= 0xff; > + spin_lock(&ioapic->lock); > switch (addr) { > case IOAPIC_REG_SELECT: > ioapic->ioregsel = data; > @@ -310,15 +322,14 @@ static int ioapic_mmio_write(struct kvm_io_device *this, gpa_t addr, int len, > break; > #ifdef CONFIG_IA64 > case IOAPIC_REG_EOI: > - kvm_ioapic_update_eoi(ioapic->kvm, data, IOAPIC_LEVEL_TRIG); > + __kvm_ioapic_update_eoi(ioapic, data, IOAPIC_LEVEL_TRIG); > break; > #endif > > default: > break; > } > -unlock: > - mutex_unlock(&ioapic->kvm->irq_lock); > + spin_unlock(&ioapic->lock); > return 0; > } > > @@ -347,6 +358,7 @@ int kvm_ioapic_init(struct kvm *kvm) > ioapic = kzalloc(sizeof(struct kvm_ioapic), GFP_KERNEL); > if (!ioapic) > return -ENOMEM; > + spin_lock_init(&ioapic->lock); > kvm->arch.vioapic = ioapic; > kvm_ioapic_reset(ioapic); > kvm_iodevice_init(&ioapic->dev, &ioapic_mmio_ops); > diff --git a/virt/kvm/ioapic.h b/virt/kvm/ioapic.h > index 7080b71..557107e 100644 > --- a/virt/kvm/ioapic.h > +++ b/virt/kvm/ioapic.h > @@ -44,6 +44,7 @@ struct kvm_ioapic { > struct kvm_io_device dev; > struct kvm *kvm; > void (*ack_notifier)(void *opaque, int irq); > + spinlock_t lock; > }; > > #ifdef DEBUG > -- > 1.6.2.1 > > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html