From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934707AbdACNE6 (ORCPT ); Tue, 3 Jan 2017 08:04:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46932 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758729AbdACNE0 (ORCPT ); Tue, 3 Jan 2017 08:04:26 -0500 Subject: Re: [PATCH v2 3/6] KVM: x86: make pic setup code look like ioapic setup To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , linux-kernel@vger.kernel.org, kvm@vger.kernel.org References: <20161216151006.11776-1-rkrcmar@redhat.com> <20161216151006.11776-4-rkrcmar@redhat.com> Cc: Paolo Bonzini From: David Hildenbrand Organization: Red Hat GmbH Message-ID: <2cca2bc6-2702-0b2f-e744-f86cbdd80ef5@redhat.com> Date: Tue, 3 Jan 2017 14:04:22 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20161216151006.11776-4-rkrcmar@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Tue, 03 Jan 2017 13:04:26 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > Signed-off-by: Radim Krčmář > --- > 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 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 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