[PATCH v2 0/6] KVM: x86: minor irqchip improvements (API change)

[PATCH v2 0/6] KVM: x86: minor irqchip improvements (API change)

[Radim Krčmář]

v2:
  Uses enum kvm_irqchip_mode to encode state and renames irqchip_kvm to
  irqchip_kernel, which might be easily confused with irqchip_in_kernel.

v1: https://www.spinics.net/lists/kernel/msg2391115.html
> There are two API changes:
> 1) [1/6] forbids KVM_CREATE_IRQCHIP after KVM_CAP_SPLIT_IRQCHIP
> 2) [5/6] makes KVM_SET_GSI_ROUTING reject pic and ioapic routes in split
>    irqchip mode, because they make no sense and are currently "working" only
>    because of a hacky NULL check.
> 
> [1-4/6] are needed for [5/6]; [6/6] is just a cherry.


Radim Krčmář (6):
  KVM: x86: don't allow kernel irqchip with split irqchip
  KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()
  KVM: x86: make pic setup code look like ioapic setup
  KVM: x86: refactor pic setup in kvm_set_routing_entry
  KVM: x86: prevent setup of invalid routes
  KVM: x86: simplify conditions with split/kernel irqchip

 arch/x86/include/asm/kvm_host.h |  8 +++++++-
 arch/x86/kvm/i8259.c            | 16 +++++++++++-----
 arch/x86/kvm/irq.h              | 19 ++++++++++---------
 arch/x86/kvm/irq_comm.c         | 29 ++++++++++-------------------
 arch/x86/kvm/x86.c              | 41 +++++++++++++++++++++--------------------
 5 files changed, 59 insertions(+), 54 deletions(-)

-- 
2.11.0

[PATCH v2 1/6] KVM: x86: don't allow kernel irqchip with split irqchip

[Radim Krčmář]

Split irqchip cannot be created after creating the kernel irqchip, but
we forgot to restrict the other way.  This is an API change.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: r-b Paolo
---
 arch/x86/kvm/x86.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1f0d2383f5ee..e670591337af 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3939,7 +3939,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 
 		mutex_lock(&kvm->lock);
 		r = -EEXIST;
-		if (kvm->arch.vpic)
+		if (irqchip_in_kernel(kvm))
 			goto create_irqchip_unlock;
 		r = -EINVAL;
 		if (kvm->created_vcpus)
-- 
2.11.0

[PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()

[Radim Krčmář]

irqchip_in_kernel() tried to save a bit by reusing pic_irqchip(), but it
just complicated the code.
Add a separate state for the irqchip mode.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: change two bools into one enum and rename everything [Paolo]
---
 arch/x86/include/asm/kvm_host.h |  8 +++++++-
 arch/x86/kvm/irq.h              | 15 ++++++++-------
 arch/x86/kvm/x86.c              |  5 +++--
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 7892530cbacf..d3acd295446d 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -715,6 +715,12 @@ struct kvm_hv {
 	HV_REFERENCE_TSC_PAGE tsc_ref;
 };
 
+enum kvm_irqchip_mode {
+	KVM_IRQCHIP_NONE,
+	KVM_IRQCHIP_KERNEL,       /* created with KVM_CREATE_IRQCHIP */
+	KVM_IRQCHIP_SPLIT,        /* created with KVM_CAP_SPLIT_IRQCHIP */
+};
+
 struct kvm_arch {
 	unsigned int n_used_mmu_pages;
 	unsigned int n_requested_mmu_pages;
@@ -787,7 +793,7 @@ struct kvm_arch {
 
 	u64 disabled_quirks;
 
-	bool irqchip_split;
+	enum kvm_irqchip_mode irqchip_mode;
 	u8 nr_reserved_ioapic_pins;
 
 	bool disabled_lapic_found;
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 035731eb3897..79cfc945125c 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -93,18 +93,19 @@ static inline int pic_in_kernel(struct kvm *kvm)
 
 static inline int irqchip_split(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_split;
+	return kvm->arch.irqchip_mode == KVM_IRQCHIP_SPLIT;
+}
+
+static inline int irqchip_kernel(struct kvm *kvm)
+{
+	return kvm->arch.irqchip_mode == KVM_IRQCHIP_KERNEL;
 }
 
 static inline int irqchip_in_kernel(struct kvm *kvm)
 {
-	struct kvm_pic *vpic = pic_irqchip(kvm);
-	bool ret;
+	bool ret = irqchip_kernel(kvm) || irqchip_split(kvm);
 
-	ret = (vpic != NULL);
-	ret |= irqchip_split(kvm);
-
-	/* Read vpic before kvm->irq_routing.  */
+	/* Matches with wmb after initializing kvm->irq_routing. */
 	smp_rmb();
 	return ret;
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e670591337af..a8dbfb4129c5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3872,7 +3872,7 @@ static int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
 			goto split_irqchip_unlock;
 		/* Pairs with irqchip_in_kernel. */
 		smp_wmb();
-		kvm->arch.irqchip_split = true;
+		kvm->arch.irqchip_mode = KVM_IRQCHIP_SPLIT;
 		kvm->arch.nr_reserved_ioapic_pins = cap->args[0];
 		r = 0;
 split_irqchip_unlock:
@@ -3966,8 +3966,9 @@ long kvm_arch_vm_ioctl(struct file *filp,
 			mutex_unlock(&kvm->slots_lock);
 			goto create_irqchip_unlock;
 		}
-		/* Write kvm->irq_routing before kvm->arch.vpic.  */
+		/* Write kvm->irq_routing before enabling irqchip_in_kernel. */
 		smp_wmb();
+		kvm->arch.irqchip_mode = KVM_IRQCHIP_KERNEL;
 		kvm->arch.vpic = vpic;
 	create_irqchip_unlock:
 		mutex_unlock(&kvm->lock);
-- 
2.11.0

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

[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;
 	create_irqchip_unlock:
 		mutex_unlock(&kvm->lock);
 		break;
-- 
2.11.0

[PATCH v2 4/6] KVM: x86: refactor pic setup in kvm_set_routing_entry

[Radim Krčmář]

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: r-b Paolo
---
 arch/x86/kvm/irq_comm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 6c0191615f23..1dfeb185a1e3 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -297,15 +297,13 @@ int kvm_set_routing_entry(struct kvm *kvm,
 	case KVM_IRQ_ROUTING_IRQCHIP:
 		delta = 0;
 		switch (ue->u.irqchip.irqchip) {
+		case KVM_IRQCHIP_PIC_SLAVE:
+			delta = 8;
+			/* fall through */
 		case KVM_IRQCHIP_PIC_MASTER:
 			e->set = kvm_set_pic_irq;
 			max_pin = PIC_NUM_PINS;
 			break;
-		case KVM_IRQCHIP_PIC_SLAVE:
-			e->set = kvm_set_pic_irq;
-			max_pin = PIC_NUM_PINS;
-			delta = 8;
-			break;
 		case KVM_IRQCHIP_IOAPIC:
 			max_pin = KVM_IOAPIC_NUM_PINS;
 			e->set = kvm_set_ioapic_irq;
-- 
2.11.0

[PATCH v2 5/6] KVM: x86: prevent setup of invalid routes

[Radim Krčmář]

The check in kvm_set_pic_irq() and kvm_set_ioapic_irq() was just a
temporary measure until the code improved enough for us to do this.

This changes APIC in a case when KVM_SET_GSI_ROUTING is called to set up pic
and ioapic routes before KVM_CREATE_IRQCHIP.  Those rules would get overwritten
by KVM_CREATE_IRQCHIP at best, so it is pointless to allow it.  Userspaces
hopefully noticed that things don't work if they do that and don't do that.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: r-b Paolo
---
 arch/x86/kvm/irq_comm.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 1dfeb185a1e3..2639b8d3dce2 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -41,15 +41,6 @@ static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
 			   bool line_status)
 {
 	struct kvm_pic *pic = pic_irqchip(kvm);
-
-	/*
-	 * XXX: rejecting pic routes when pic isn't in use would be better,
-	 * but the default routing table is installed while kvm->arch.vpic is
-	 * NULL and KVM_CREATE_IRQCHIP can race with KVM_IRQ_LINE.
-	 */
-	if (!pic)
-		return -1;
-
 	return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
 }
 
@@ -58,10 +49,6 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
 			      bool line_status)
 {
 	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
-
-	if (!ioapic)
-		return -1;
-
 	return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level,
 				line_status);
 }
@@ -301,10 +288,16 @@ int kvm_set_routing_entry(struct kvm *kvm,
 			delta = 8;
 			/* fall through */
 		case KVM_IRQCHIP_PIC_MASTER:
+			if (!pic_in_kernel(kvm))
+				goto out;
+
 			e->set = kvm_set_pic_irq;
 			max_pin = PIC_NUM_PINS;
 			break;
 		case KVM_IRQCHIP_IOAPIC:
+			if (!ioapic_in_kernel(kvm))
+				goto out;
+
 			max_pin = KVM_IOAPIC_NUM_PINS;
 			e->set = kvm_set_ioapic_irq;
 			break;
-- 
2.11.0

[PATCH v2 6/6] KVM: x86: simplify conditions with split/kernel irqchip

[Radim Krčmář]

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 v2: r-b Paolo, irqchip_kvm -> irqchip_kernel due to earlier changes
---
 arch/x86/kvm/irq_comm.c | 2 +-
 arch/x86/kvm/x86.c      | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
index 2639b8d3dce2..b96d3893f121 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -400,7 +400,7 @@ int kvm_setup_empty_irq_routing(struct kvm *kvm)
 
 void kvm_arch_post_irq_routing_update(struct kvm *kvm)
 {
-	if (ioapic_in_kernel(kvm) || !irqchip_in_kernel(kvm))
+	if (!irqchip_split(kvm))
 		return;
 	kvm_make_scan_ioapic_request(kvm);
 }
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2fa004029b37..0801db551b4c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4005,7 +4005,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		}
 
 		r = -ENXIO;
-		if (!irqchip_in_kernel(kvm) || irqchip_split(kvm))
+		if (!irqchip_kernel(kvm))
 			goto get_irqchip_out;
 		r = kvm_vm_ioctl_get_irqchip(kvm, chip);
 		if (r)
@@ -4029,7 +4029,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		}
 
 		r = -ENXIO;
-		if (!irqchip_in_kernel(kvm) || irqchip_split(kvm))
+		if (!irqchip_kernel(kvm))
 			goto set_irqchip_out;
 		r = kvm_vm_ioctl_set_irqchip(kvm, chip);
 		if (r)
-- 
2.11.0

Re: [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()

[Paolo Bonzini]

On 16/12/2016 16:10, Radim Krčmář wrote:
> +	bool ret = irqchip_kernel(kvm) || irqchip_split(kvm);

        bool ret = (kvm->arch.irqchip_mode != KVM_IRQCHIP_NONE);

:)

Paolo

>  
> -	ret = (vpic != NULL);
> -	ret |= irqchip_split(kvm);
> -
> -	/* Read vpic before kvm->irq_routing.  */
> +	/* Matches with wmb after initializing kvm->irq_routing. */
>  	smp_rmb();

Re: [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()

[Radim Krčmář]

2016-12-16 16:25+0100, Paolo Bonzini:
> On 16/12/2016 16:10, Radim Krčmář wrote:
>> +	bool ret = irqchip_kernel(kvm) || irqchip_split(kvm);
> 
>         bool ret = (kvm->arch.irqchip_mode != KVM_IRQCHIP_NONE);
> 
> :)

(That imposes an assumption that we will only add in-kernel irqchips!
 Are we willing to sell our souls for faster code? :])

I'll use that line without parentheses, thanks.

Re: [PATCH v2 1/6] KVM: x86: don't allow kernel irqchip with split irqchip

[David Hildenbrand]

Am 16.12.2016 um 16:10 schrieb Radim Krčmář:
> Split irqchip cannot be created after creating the kernel irqchip, but
> we forgot to restrict the other way.  This is an API change.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: r-b Paolo
> ---
>  arch/x86/kvm/x86.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1f0d2383f5ee..e670591337af 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3939,7 +3939,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>
>  		mutex_lock(&kvm->lock);
>  		r = -EEXIST;
> -		if (kvm->arch.vpic)
> +		if (irqchip_in_kernel(kvm))
>  			goto create_irqchip_unlock;
>  		r = -EINVAL;
>  		if (kvm->created_vcpus)
>

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

David

Re: [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()

[David Hildenbrand]

Am 16.12.2016 um 16:10 schrieb Radim Krčmář:
> irqchip_in_kernel() tried to save a bit by reusing pic_irqchip(), but it
> just complicated the code.
> Add a separate state for the irqchip mode.
>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: change two bools into one enum and rename everything [Paolo]
> ---
>  arch/x86/include/asm/kvm_host.h |  8 +++++++-
>  arch/x86/kvm/irq.h              | 15 ++++++++-------
>  arch/x86/kvm/x86.c              |  5 +++--
>  3 files changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 7892530cbacf..d3acd295446d 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -715,6 +715,12 @@ struct kvm_hv {
>  	HV_REFERENCE_TSC_PAGE tsc_ref;
>  };
>
> +enum kvm_irqchip_mode {
> +	KVM_IRQCHIP_NONE,
> +	KVM_IRQCHIP_KERNEL,       /* created with KVM_CREATE_IRQCHIP */
> +	KVM_IRQCHIP_SPLIT,        /* created with KVM_CAP_SPLIT_IRQCHIP */

Was wondering if FULL/SPLIT would be a better naming. However I also
find irqchip_kernel() vs irqchip_in_kernel() slightly confusing.

Anyhow, with the suggestion of Paolo included,

Reviewed-by: David Hildenbrand <david@redhat.com>

> +};


-- 

David

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@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

Re: [PATCH v2 4/6] KVM: x86: refactor pic setup in kvm_set_routing_entry

[David Hildenbrand]

Am 16.12.2016 um 16:10 schrieb Radim Krčmář:
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: r-b Paolo
> ---
>  arch/x86/kvm/irq_comm.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 6c0191615f23..1dfeb185a1e3 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -297,15 +297,13 @@ int kvm_set_routing_entry(struct kvm *kvm,
>  	case KVM_IRQ_ROUTING_IRQCHIP:
>  		delta = 0;
>  		switch (ue->u.irqchip.irqchip) {
> +		case KVM_IRQCHIP_PIC_SLAVE:
> +			delta = 8;
> +			/* fall through */
>  		case KVM_IRQCHIP_PIC_MASTER:
>  			e->set = kvm_set_pic_irq;
>  			max_pin = PIC_NUM_PINS;
>  			break;
> -		case KVM_IRQCHIP_PIC_SLAVE:
> -			e->set = kvm_set_pic_irq;
> -			max_pin = PIC_NUM_PINS;
> -			delta = 8;
> -			break;
>  		case KVM_IRQCHIP_IOAPIC:
>  			max_pin = KVM_IOAPIC_NUM_PINS;
>  			e->set = kvm_set_ioapic_irq;
>

Had the exact same thing in mind when reading that piece of code.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

David

Re: [PATCH v2 5/6] KVM: x86: prevent setup of invalid routes

[David Hildenbrand]

Am 16.12.2016 um 16:10 schrieb Radim Krčmář:
> The check in kvm_set_pic_irq() and kvm_set_ioapic_irq() was just a
> temporary measure until the code improved enough for us to do this.
>
> This changes APIC in a case when KVM_SET_GSI_ROUTING is called to set up pic
> and ioapic routes before KVM_CREATE_IRQCHIP.  Those rules would get overwritten
> by KVM_CREATE_IRQCHIP at best, so it is pointless to allow it.  Userspaces
> hopefully noticed that things don't work if they do that and don't do that.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: r-b Paolo
> ---
>  arch/x86/kvm/irq_comm.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
> index 1dfeb185a1e3..2639b8d3dce2 100644
> --- a/arch/x86/kvm/irq_comm.c
> +++ b/arch/x86/kvm/irq_comm.c
> @@ -41,15 +41,6 @@ static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
>  			   bool line_status)
>  {
>  	struct kvm_pic *pic = pic_irqchip(kvm);
> -
> -	/*
> -	 * XXX: rejecting pic routes when pic isn't in use would be better,
> -	 * but the default routing table is installed while kvm->arch.vpic is
> -	 * NULL and KVM_CREATE_IRQCHIP can race with KVM_IRQ_LINE.
> -	 */
> -	if (!pic)
> -		return -1;
> -
>  	return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
>  }
>
> @@ -58,10 +49,6 @@ static int kvm_set_ioapic_irq(struct kvm_kernel_irq_routing_entry *e,
>  			      bool line_status)
>  {
>  	struct kvm_ioapic *ioapic = kvm->arch.vioapic;
> -
> -	if (!ioapic)
> -		return -1;
> -
>  	return kvm_ioapic_set_irq(ioapic, e->irqchip.pin, irq_source_id, level,
>  				line_status);
>  }
> @@ -301,10 +288,16 @@ int kvm_set_routing_entry(struct kvm *kvm,
>  			delta = 8;
>  			/* fall through */
>  		case KVM_IRQCHIP_PIC_MASTER:
> +			if (!pic_in_kernel(kvm))
> +				goto out;
> +
>  			e->set = kvm_set_pic_irq;
>  			max_pin = PIC_NUM_PINS;
>  			break;
>  		case KVM_IRQCHIP_IOAPIC:
> +			if (!ioapic_in_kernel(kvm))
> +				goto out;
> +
>  			max_pin = KVM_IOAPIC_NUM_PINS;
>  			e->set = kvm_set_ioapic_irq;
>  			break;
>

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

David

Re: [PATCH v2 6/6] KVM: x86: simplify conditions with split/kernel irqchip

[David Hildenbrand]

Am 16.12.2016 um 16:10 schrieb Radim Krčmář:
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  v2: r-b Paolo, irqchip_kvm -> irqchip_kernel due to earlier changes

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

David

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

[Paolo Bonzini]

On 03/01/2017 14:04, David Hildenbrand wrote:
> 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.
>
> 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 :)

Yes, the previous patch referred to that when it said
"irqchip_in_kernel() tried to save a bit by reusing pic_irqchip()", and
what this commit message means by "not trteating kvm->arch.vpic
specially anymore".

Paolo

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

[David Hildenbrand]

Am 03.01.2017 um 17:12 schrieb Paolo Bonzini:
>
>
> On 03/01/2017 14:04, David Hildenbrand wrote:
>> 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.
>>
>> 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 :)
>
> Yes, the previous patch referred to that when it said
> "irqchip_in_kernel() tried to save a bit by reusing pic_irqchip()", and
> what this commit message means by "not trteating kvm->arch.vpic
> specially anymore".

Thanks for confirming!

Reviewed-by: David Hildenbrand <david@redhat.com>

>
> Paolo
>


-- 

David

Re: [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()

[Peter Xu]

On Tue, Jan 03, 2017 at 01:56:23PM +0100, David Hildenbrand wrote:
> Am 16.12.2016 um 16:10 schrieb Radim Krčmář:
> >irqchip_in_kernel() tried to save a bit by reusing pic_irqchip(), but it
> >just complicated the code.
> >Add a separate state for the irqchip mode.
> >
> >Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> >---
> > v2: change two bools into one enum and rename everything [Paolo]
> >---
> > arch/x86/include/asm/kvm_host.h |  8 +++++++-
> > arch/x86/kvm/irq.h              | 15 ++++++++-------
> > arch/x86/kvm/x86.c              |  5 +++--
> > 3 files changed, 18 insertions(+), 10 deletions(-)
> >
> >diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> >index 7892530cbacf..d3acd295446d 100644
> >--- a/arch/x86/include/asm/kvm_host.h
> >+++ b/arch/x86/include/asm/kvm_host.h
> >@@ -715,6 +715,12 @@ struct kvm_hv {
> > 	HV_REFERENCE_TSC_PAGE tsc_ref;
> > };
> >
> >+enum kvm_irqchip_mode {
> >+	KVM_IRQCHIP_NONE,
> >+	KVM_IRQCHIP_KERNEL,       /* created with KVM_CREATE_IRQCHIP */
> >+	KVM_IRQCHIP_SPLIT,        /* created with KVM_CAP_SPLIT_IRQCHIP */
> 
> Was wondering if FULL/SPLIT would be a better naming. However I also
> find irqchip_kernel() vs irqchip_in_kernel() slightly confusing.

Me too. Since we have kvm_irqchip_mode enum above, how about renaming
irqchip_{kernel|split}() into irqchip_mode_{kernel|split}()?

Sorry for such a late comment...

-- peterx

Re: [PATCH v2 5/6] KVM: x86: prevent setup of invalid routes

[Peter Xu]

On Fri, Dec 16, 2016 at 04:10:05PM +0100, Radim Krčmář wrote:
> The check in kvm_set_pic_irq() and kvm_set_ioapic_irq() was just a
> temporary measure until the code improved enough for us to do this.
> 
> This changes APIC in a case when KVM_SET_GSI_ROUTING is called to set up pic
> and ioapic routes before KVM_CREATE_IRQCHIP.  Those rules would get overwritten
> by KVM_CREATE_IRQCHIP at best, so it is pointless to allow it.  Userspaces
> hopefully noticed that things don't work if they do that and don't do that.
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>

Since we are at here, do we need to protect KVM_SET_GSI_ROUTING in
general as well to make sure kernel APIC is there? Like:

---------8<---------

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 482612b..31141a7 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -3057,6 +3057,10 @@ static long kvm_vm_ioctl(struct file *filp,
                struct kvm_irq_routing_entry *entries = NULL;

                r = -EFAULT;
+
+               if (!irqchip_in_kernel(kvm))
+                       goto out;
+
                if (copy_from_user(&routing, argp, sizeof(routing)))
                        goto out;
                r = -EINVAL;

--------->8---------

Thanks,

-- peterx

Re: [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()

[Paolo Bonzini]

On 10/01/2017 06:09, Peter Xu wrote:
>> Was wondering if FULL/SPLIT would be a better naming. However I also
>> find irqchip_kernel() vs irqchip_in_kernel() slightly confusing.
> Me too. Since we have kvm_irqchip_mode enum above, how about renaming
> irqchip_{kernel|split}() into irqchip_mode_{kernel|split}()?
> 
> Sorry for such a late comment...

No problem, it can be done on top.

Another thing to do is to make irqchip_in_kernel check mode != NONE.

Paolo

Re: [PATCH v2 5/6] KVM: x86: prevent setup of invalid routes

[Paolo Bonzini]

On 10/01/2017 06:26, Peter Xu wrote:
> On Fri, Dec 16, 2016 at 04:10:05PM +0100, Radim Krčmář wrote:
>> The check in kvm_set_pic_irq() and kvm_set_ioapic_irq() was just a
>> temporary measure until the code improved enough for us to do this.
>>
>> This changes APIC in a case when KVM_SET_GSI_ROUTING is called to set up pic
>> and ioapic routes before KVM_CREATE_IRQCHIP.  Those rules would get overwritten
>> by KVM_CREATE_IRQCHIP at best, so it is pointless to allow it.  Userspaces
>> hopefully noticed that things don't work if they do that and don't do that.
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> 
> Since we are at here, do we need to protect KVM_SET_GSI_ROUTING in
> general as well to make sure kernel APIC is there?

Skipping the check is harmless.  I agree that it should have been always
done like you suggest, but right now it may break something.

Paolo

> 
> ---------8<---------
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 482612b..31141a7 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3057,6 +3057,10 @@ static long kvm_vm_ioctl(struct file *filp,
>                 struct kvm_irq_routing_entry *entries = NULL;
> 
>                 r = -EFAULT;
> +
> +               if (!irqchip_in_kernel(kvm))
> +                       goto out;
> +
>                 if (copy_from_user(&routing, argp, sizeof(routing)))
>                         goto out;
>                 r = -EINVAL;
> 
> --------->8---------
> 
> Thanks,
> 
> -- peterx
>