From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 6/8 v2] Move IO APIC to its own lock. Date: Wed, 12 Aug 2009 11:27:13 +0300 Message-ID: <4A827CE1.7040304@redhat.com> References: <1249993895-11119-1-git-send-email-gleb@redhat.com> <1249993895-11119-7-git-send-email-gleb@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Gleb Natapov Return-path: Received: from mx2.redhat.com ([66.187.237.31]:33766 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751216AbZHLI1Q (ORCPT ); Wed, 12 Aug 2009 04:27:16 -0400 Received: from int-mx2.corp.redhat.com (int-mx2.corp.redhat.com [172.16.27.26]) by mx2.redhat.com (8.13.8/8.13.8) with ESMTP id n7C8RHAV007381 for ; Wed, 12 Aug 2009 04:27:17 -0400 In-Reply-To: <1249993895-11119-7-git-send-email-gleb@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 08/11/2009 03:31 PM, Gleb Natapov wrote: What is the motivation for this change? Why a spinlock and not a mutex? > 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); > Better to add an accessor than to reach into internals like this. > + } 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; > + } > ... and better to deduplicate the code too. > break; > default: > r = -EINVAL; > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c > index 01f1516..a988c0e 100644 > --- a/arch/x86/kvm/i8259.c > +++ b/arch/x86/kvm/i8259.c > @@ -38,7 +38,9 @@ static void pic_clear_isr(struct kvm_kpic_state *s, int irq) > s->isr_ack |= (1<< irq); > if (s !=&s->pics_state->pics[0]) > irq += 8; > + spin_unlock(&s->pics_state->lock); > kvm_notify_acked_irq(s->pics_state->kvm, SELECT_PIC(irq), irq); > + spin_lock(&s->pics_state->lock); > } > Need to explain why this is safe. I'm not sure it is, because we touch state afterwards in pic_intack(). We need to do all vcpu-synchronous operations before dropping the lock. > void kvm_pic_clear_isr_ack(struct kvm *kvm) > @@ -238,7 +240,9 @@ void kvm_pic_reset(struct kvm_kpic_state *s) > if (vcpu0&& kvm_apic_accept_pic_intr(vcpu0)) > if (s->irr& (1<< irq) || s->isr& (1<< irq)) { > n = irq + irqbase; > + spin_unlock(&s->pics_state->lock); > kvm_notify_acked_irq(kvm, SELECT_PIC(n), n); > + spin_lock(&s->pics_state->lock); > Ditto here, needs to be moved until after done changing state. > > -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]; > + spin_unlock(&ioapic->lock); > + kvm_notify_acked_irq(ioapic->kvm, KVM_IRQCHIP_IOAPIC, i); > + spin_lock(&ioapic->lock); > > I *think* we need to clear remote_irr before dropping the lock. I *know* there's a missing comment here. > - 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); > } > } > To make the patch easier to read, suggest keeping the loop in the other function. > > 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); > } > > -- error compiling committee.c: too many arguments to function