lkml.org 
[lkml]   [2017]   [Jan]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 3/6] KVM: x86: make pic setup code look like ioapic setup
From
Date
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

\
 
 \ /
  Last update: 2017-01-03 14:08    [W:0.049 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site