Re: [PATCH v2 3/6] KVM: x86: make pic setup code look like ioapic setup

[David Hildenbrand]

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@xxxxxxxxxx>
> Signed-off-by: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxx>
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@xxxxxxxxxx>

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