Messages in this thread | | | Subject | Re: [PATCH v2 3/6] KVM: x86: make pic setup code look like ioapic setup | From | David Hildenbrand <> | Date | Tue, 3 Jan 2017 14:04:22 +0100 |
| |
Am 16.12.2016 um 16:10 schrieb Radim Krčmář: > We don't treat kvm->arch.vpic specially anymore, so the setup can look > like ioapic. This gets a bit more information out of return values. > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > v2: r-b Paolo > --- > arch/x86/kvm/i8259.c | 16 +++++++++++----- > arch/x86/kvm/irq.h | 4 ++-- > arch/x86/kvm/x86.c | 30 +++++++++++++++--------------- > 3 files changed, 28 insertions(+), 22 deletions(-) > > diff --git a/arch/x86/kvm/i8259.c b/arch/x86/kvm/i8259.c > index 7cc2360f1848..73ea24d4f119 100644 > --- a/arch/x86/kvm/i8259.c > +++ b/arch/x86/kvm/i8259.c > @@ -598,14 +598,14 @@ static const struct kvm_io_device_ops picdev_eclr_ops = { > .write = picdev_eclr_write, > }; > > -struct kvm_pic *kvm_create_pic(struct kvm *kvm) > +int kvm_pic_init(struct kvm *kvm) > { > struct kvm_pic *s; > int ret; > > s = kzalloc(sizeof(struct kvm_pic), GFP_KERNEL); > if (!s) > - return NULL; > + return -ENOMEM; > spin_lock_init(&s->lock); > s->kvm = kvm; > s->pics[0].elcr_mask = 0xf8; > @@ -635,7 +635,9 @@ struct kvm_pic *kvm_create_pic(struct kvm *kvm) > > mutex_unlock(&kvm->slots_lock); > > - return s; > + kvm->arch.vpic = s; > + > + return 0; > > fail_unreg_1: > kvm_io_bus_unregister_dev(kvm, KVM_PIO_BUS, &s->dev_slave); > @@ -648,13 +650,17 @@ struct kvm_pic *kvm_create_pic(struct kvm *kvm) > > kfree(s); > > - return NULL; > + return ret; > } > > -void kvm_destroy_pic(struct kvm_pic *vpic) > +void kvm_pic_destroy(struct kvm *kvm) > { > + struct kvm_pic *vpic = kvm->arch.vpic; > + > kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_master); > kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_slave); > kvm_io_bus_unregister_dev(vpic->kvm, KVM_PIO_BUS, &vpic->dev_eclr); > + > + kvm->arch.vpic = NULL; > kfree(vpic); > } > diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h > index 79cfc945125c..13248d4d306c 100644 > --- a/arch/x86/kvm/irq.h > +++ b/arch/x86/kvm/irq.h > @@ -73,8 +73,8 @@ struct kvm_pic { > unsigned long irq_states[PIC_NUM_PINS]; > }; > > -struct kvm_pic *kvm_create_pic(struct kvm *kvm); > -void kvm_destroy_pic(struct kvm_pic *vpic); > +int kvm_pic_init(struct kvm *kvm); > +void kvm_pic_destroy(struct kvm *kvm); > int kvm_pic_read_irq(struct kvm *kvm); > void kvm_pic_update_irq(struct kvm_pic *s); > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index a8dbfb4129c5..2fa004029b37 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -3935,33 +3935,34 @@ long kvm_arch_vm_ioctl(struct file *filp, > r = kvm_vm_ioctl_get_nr_mmu_pages(kvm); > break; > case KVM_CREATE_IRQCHIP: { > - struct kvm_pic *vpic; > - > mutex_lock(&kvm->lock); > + > r = -EEXIST; > if (irqchip_in_kernel(kvm)) > goto create_irqchip_unlock; > + > r = -EINVAL; > if (kvm->created_vcpus) > goto create_irqchip_unlock; > - r = -ENOMEM; > - vpic = kvm_create_pic(kvm); > - if (vpic) { > - r = kvm_ioapic_init(kvm); > - if (r) { > - mutex_lock(&kvm->slots_lock); > - kvm_destroy_pic(vpic); > - mutex_unlock(&kvm->slots_lock); > - goto create_irqchip_unlock; > - } > - } else > + > + r = kvm_pic_init(kvm); > + if (r) > goto create_irqchip_unlock; > + > + r = kvm_ioapic_init(kvm); > + if (r) { > + mutex_lock(&kvm->slots_lock); > + kvm_pic_destroy(kvm); > + mutex_unlock(&kvm->slots_lock); > + goto create_irqchip_unlock; > + } > + > r = kvm_setup_default_irq_routing(kvm); > if (r) { > mutex_lock(&kvm->slots_lock); > mutex_lock(&kvm->irq_lock); > kvm_ioapic_destroy(kvm); > - kvm_destroy_pic(vpic); > + kvm_pic_destroy(kvm); > mutex_unlock(&kvm->irq_lock); > mutex_unlock(&kvm->slots_lock); > goto create_irqchip_unlock; > @@ -3969,7 +3970,6 @@ long kvm_arch_vm_ioctl(struct file *filp, > /* Write kvm->irq_routing before enabling irqchip_in_kernel. */ > smp_wmb(); > kvm->arch.irqchip_mode = KVM_IRQCHIP_KERNEL; > - kvm->arch.vpic = vpic;
This originally saved us from a race condition as far as I can reconstruct from the commit history. Think the problem was vpic being set but routes not being set up yet.
commit 71ba994c94a81c37185ef2fb5190844286ba9aca Author: Paolo Bonzini <pbonzini@redhat.com> Date: Wed Jul 29 12:31:15 2015 +0200
KVM: x86: clean/fix memory barriers in irqchip_in_kernel
The memory barriers are trying to protect against concurrent RCU-based interrupt injection, but the IRQ routing table is not valid at the time kvm->arch.vpic is written. Fix this by writing kvm->arch.vpic last. kvm_destroy_pic then need not set kvm->arch.vpic to NULL; modify it to take a struct kvm_pic* and reuse it if the IOAPIC creation fails.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
I assume that this is now fixed via the irqchip_mode, as it is stored last? If so, I really like this patch :)
> create_irqchip_unlock: > mutex_unlock(&kvm->lock); > break; >
--
David
| |