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

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

[Radim Krčmář]

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: do allow kvm 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/kvm irqchip

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/i8259.c            | 16 +++++++++++-----
 arch/x86/kvm/irq.h              | 17 +++++++++--------
 arch/x86/kvm/irq_comm.c         | 29 ++++++++++-------------------
 arch/x86/kvm/x86.c              | 39 ++++++++++++++++++++-------------------
 5 files changed, 51 insertions(+), 51 deletions(-)

-- 
2.10.2

[PATCH 1/6] KVM: x86: do allow kvm irqchip with split irqchip

[Radim Krčmář]

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

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 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 6f9c9ad13f88..dbed51045c37 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3901,7 +3901,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.10.2

[PATCH 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 kvm->arch.irqchip_kvm that matches kvm->arch.irqchip_split.
(Turning them into an exclusive type would be nicer.)

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/irq.h              | 13 +++++++------
 arch/x86/kvm/x86.c              |  3 ++-
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index bdde80731f49..929228ec2839 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -778,6 +778,7 @@ struct kvm_arch {
 
 	u64 disabled_quirks;
 
+	bool irqchip_kvm;
 	bool irqchip_split;
 	u8 nr_reserved_ioapic_pins;
 
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index 035731eb3897..8536be85d529 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -96,15 +96,16 @@ static inline int irqchip_split(struct kvm *kvm)
 	return kvm->arch.irqchip_split;
 }
 
+static inline int irqchip_kvm(struct kvm *kvm)
+{
+	return kvm->arch.irqchip_kvm;
+}
+
 static inline int irqchip_in_kernel(struct kvm *kvm)
 {
-	struct kvm_pic *vpic = pic_irqchip(kvm);
-	bool ret;
+	bool ret = irqchip_kvm(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 dbed51045c37..dd8431a7e18b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3928,8 +3928,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_kvm = true;
 		kvm->arch.vpic = vpic;
 	create_irqchip_unlock:
 		mutex_unlock(&kvm->lock);
-- 
2.10.2

[PATCH 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.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 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 8536be85d529..a80515e38645 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 dd8431a7e18b..40d4c782a752 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3897,33 +3897,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;
@@ -3931,7 +3932,6 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		/* Write kvm->irq_routing before enabling irqchip_in_kernel. */
 		smp_wmb();
 		kvm->arch.irqchip_kvm = true;
-		kvm->arch.vpic = vpic;
 	create_irqchip_unlock:
 		mutex_unlock(&kvm->lock);
 		break;
-- 
2.10.2

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

[Radim Krčmář]

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 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 ddd63b8b176e..913e054a68e9 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -289,15 +289,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.10.2

[PATCH 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.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 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 913e054a68e9..2838c0c37279 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);
 }
@@ -293,10 +280,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.10.2

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

[Radim Krčmář]

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 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 2838c0c37279..8ce1c3e52dbd 100644
--- a/arch/x86/kvm/irq_comm.c
+++ b/arch/x86/kvm/irq_comm.c
@@ -392,7 +392,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 40d4c782a752..4c364a13b17c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3967,7 +3967,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		}
 
 		r = -ENXIO;
-		if (!irqchip_in_kernel(kvm) || irqchip_split(kvm))
+		if (!irqchip_kvm(kvm))
 			goto get_irqchip_out;
 		r = kvm_vm_ioctl_get_irqchip(kvm, chip);
 		if (r)
@@ -3991,7 +3991,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		}
 
 		r = -ENXIO;
-		if (!irqchip_in_kernel(kvm) || irqchip_split(kvm))
+		if (!irqchip_kvm(kvm))
 			goto set_irqchip_out;
 		r = kvm_vm_ioctl_set_irqchip(kvm, chip);
 		if (r)
-- 
2.10.2

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

[Paolo Bonzini]

> irqchip_in_kernel() tried to save a bit by reusing pic_irqchip(), but it
> just complicated the code.
> Add kvm->arch.irqchip_kvm that matches kvm->arch.irqchip_split.
> (Turning them into an exclusive type would be nicer.)

Then do it. ;)

Paolo

> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/irq.h              | 13 +++++++------
>  arch/x86/kvm/x86.c              |  3 ++-
>  3 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h
> index bdde80731f49..929228ec2839 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -778,6 +778,7 @@ struct kvm_arch {
>  
>  	u64 disabled_quirks;
>  
> +	bool irqchip_kvm;
>  	bool irqchip_split;
>  	u8 nr_reserved_ioapic_pins;
>  
> diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
> index 035731eb3897..8536be85d529 100644
> --- a/arch/x86/kvm/irq.h
> +++ b/arch/x86/kvm/irq.h
> @@ -96,15 +96,16 @@ static inline int irqchip_split(struct kvm *kvm)
>  	return kvm->arch.irqchip_split;
>  }
>  
> +static inline int irqchip_kvm(struct kvm *kvm)
> +{
> +	return kvm->arch.irqchip_kvm;
> +}
> +
>  static inline int irqchip_in_kernel(struct kvm *kvm)
>  {
> -	struct kvm_pic *vpic = pic_irqchip(kvm);
> -	bool ret;
> +	bool ret = irqchip_kvm(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 dbed51045c37..dd8431a7e18b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3928,8 +3928,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_kvm = true;
>  		kvm->arch.vpic = vpic;
>  	create_irqchip_unlock:
>  		mutex_unlock(&kvm->lock);
> --
> 2.10.2
> 
> 

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

[Paolo Bonzini]

----- Original Message -----
> From: "Radim Krčmář" <rkrcmar@redhat.com>
> To: linux-kernel@vger.kernel.org, kvm@vger.kernel.org
> Cc: "Paolo Bonzini" <pbonzini@redhat.com>
> Sent: Thursday, November 24, 2016 5:31:28 PM
> Subject: [PATCH 0/6] KVM: x86: minor irqchip improvements (API change)
> 
> 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.

Looks good---but they don't apply directly on top of kvm/next so we have
to delay them until -rc2 or a second 4.11 pull request.

Anyway,

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> Radim Krčmář (6):
>   KVM: x86: do allow kvm 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/kvm irqchip
> 
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/i8259.c            | 16 +++++++++++-----
>  arch/x86/kvm/irq.h              | 17 +++++++++--------
>  arch/x86/kvm/irq_comm.c         | 29 ++++++++++-------------------
>  arch/x86/kvm/x86.c              | 39 ++++++++++++++++++++-------------------
>  5 files changed, 51 insertions(+), 51 deletions(-)
> 
> --
> 2.10.2
> 
> 

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

[Radim Krčmář]

2016-11-24 11:55-0500, Paolo Bonzini:
>> irqchip_in_kernel() tried to save a bit by reusing pic_irqchip(), but it
>> just complicated the code.
>> Add kvm->arch.irqchip_kvm that matches kvm->arch.irqchip_split.
>> (Turning them into an exclusive type would be nicer.)
> 
> Then do it. ;)

It is hard to name! :)

I would squash something like this if the names were ok:

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 929228ec2839..726235f0e3f3 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -706,6 +706,12 @@ struct kvm_hv {
 	HV_REFERENCE_TSC_PAGE tsc_ref;
 };
 
+enum kvm_kernel_irqchip {
+	KVM_IRQCHIP_NONE,
+	KVM_IRQCHIP_KVM,          /* created in KVM_CREATE_IRQCHIP */
+	KVM_IRQCHIP_SPLIT,        /* created in KVM_CAP_SPLIT_IRQCHIP */
+};
+
 struct kvm_arch {
 	unsigned int n_used_mmu_pages;
 	unsigned int n_requested_mmu_pages;
@@ -778,8 +784,7 @@ struct kvm_arch {
 
 	u64 disabled_quirks;
 
-	bool irqchip_kvm;
-	bool irqchip_split;
+	enum kvm_kernel_irqchip irqchip;
 	u8 nr_reserved_ioapic_pins;
 
 	bool disabled_lapic_found;
diff --git a/arch/x86/kvm/irq.h b/arch/x86/kvm/irq.h
index a80515e38645..f90ca9e0affc 100644
--- a/arch/x86/kvm/irq.h
+++ b/arch/x86/kvm/irq.h
@@ -93,12 +93,12 @@ 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 == KVM_IRQCHIP_SPLIT;
 }
 
 static inline int irqchip_kvm(struct kvm *kvm)
 {
-	return kvm->arch.irqchip_kvm;
+	return kvm->arch.irqchip == KVM_IRQCHIP_KVM;
 }
 
 static inline int irqchip_in_kernel(struct kvm *kvm)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4c364a13b17c..99c63b73dd35 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3834,7 +3834,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 = KVM_IRQCHIP_SPLIT;
 		kvm->arch.nr_reserved_ioapic_pins = cap->args[0];
 		r = 0;
 split_irqchip_unlock:
@@ -3931,7 +3931,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		}
 		/* Write kvm->irq_routing before enabling irqchip_in_kernel. */
 		smp_wmb();
-		kvm->arch.irqchip_kvm = true;
+		kvm->arch.irqchip = KVM_IRQCHIP_KVM;
 	create_irqchip_unlock:
 		mutex_unlock(&kvm->lock);
 		break;

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

[Paolo Bonzini]

----- Original Message -----
> From: "Radim Krčmář" <rkrcmar@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org
> Sent: Thursday, November 24, 2016 6:21:04 PM
> Subject: Re: [PATCH 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()
> 
> 2016-11-24 11:55-0500, Paolo Bonzini:
> >> irqchip_in_kernel() tried to save a bit by reusing pic_irqchip(), but it
> >> just complicated the code.
> >> Add kvm->arch.irqchip_kvm that matches kvm->arch.irqchip_split.
> >> (Turning them into an exclusive type would be nicer.)
> > 
> > Then do it. ;)
> 
> It is hard to name! :)
> 
> I would squash something like this if the names were ok:
> 
> diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h
> index 929228ec2839..726235f0e3f3 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -706,6 +706,12 @@ struct kvm_hv {
>  	HV_REFERENCE_TSC_PAGE tsc_ref;
>  };
>  
> +enum kvm_kernel_irqchip {

kvm_kernel_irqchip_mode?

> +	KVM_IRQCHIP_NONE,
> +	KVM_IRQCHIP_KVM,          /* created in KVM_CREATE_IRQCHIP */
> +	KVM_IRQCHIP_SPLIT,        /* created in KVM_CAP_SPLIT_IRQCHIP */

Since you pretty much asked to nitpick, KVM_IRQCHIP_KERNEL would
match irqchip_in_kernel better.  Also, s/in/with/? :)

> +};
> +
>  struct kvm_arch {
>  	unsigned int n_used_mmu_pages;
>  	unsigned int n_requested_mmu_pages;
> @@ -778,8 +784,7 @@ struct kvm_arch {
>  
>  	u64 disabled_quirks;
>  
> -	bool irqchip_kvm;
> -	bool irqchip_split;
> +	enum kvm_kernel_irqchip irqchip;

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 a80515e38645..f90ca9e0affc 100644
> --- a/arch/x86/kvm/irq.h
> +++ b/arch/x86/kvm/irq.h
> @@ -93,12 +93,12 @@ 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 == KVM_IRQCHIP_SPLIT;
>  }
>  
>  static inline int irqchip_kvm(struct kvm *kvm)
>  {
> -	return kvm->arch.irqchip_kvm;
> +	return kvm->arch.irqchip == KVM_IRQCHIP_KVM;
>  }
>  
>  static inline int irqchip_in_kernel(struct kvm *kvm)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4c364a13b17c..99c63b73dd35 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3834,7 +3834,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 = KVM_IRQCHIP_SPLIT;
>  		kvm->arch.nr_reserved_ioapic_pins = cap->args[0];
>  		r = 0;
>  split_irqchip_unlock:
> @@ -3931,7 +3931,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  		}
>  		/* Write kvm->irq_routing before enabling irqchip_in_kernel. */
>  		smp_wmb();
> -		kvm->arch.irqchip_kvm = true;
> +		kvm->arch.irqchip = KVM_IRQCHIP_KVM;
>  	create_irqchip_unlock:
>  		mutex_unlock(&kvm->lock);
>  		break;
> 

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

[Radim Krčmář]

2016-11-25 03:59-0500, Paolo Bonzini:
> ----- Original Message -----
>> From: "Radim Krčmář" <rkrcmar@redhat.com>
>> To: "Paolo Bonzini" <pbonzini@redhat.com>
>> Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org
>> Sent: Thursday, November 24, 2016 6:21:04 PM
>> Subject: Re: [PATCH 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()
>> 
>> 2016-11-24 11:55-0500, Paolo Bonzini:
>> >> irqchip_in_kernel() tried to save a bit by reusing pic_irqchip(), but it
>> >> just complicated the code.
>> >> Add kvm->arch.irqchip_kvm that matches kvm->arch.irqchip_split.
>> >> (Turning them into an exclusive type would be nicer.)
>> > 
>> > Then do it. ;)
>> 
>> It is hard to name! :)
>> 
>> I would squash something like this if the names were ok:
>> 
>> diff --git a/arch/x86/include/asm/kvm_host.h
>> b/arch/x86/include/asm/kvm_host.h
>> index 929228ec2839..726235f0e3f3 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -706,6 +706,12 @@ struct kvm_hv {
>>  	HV_REFERENCE_TSC_PAGE tsc_ref;
>>  };
>>  
>> +enum kvm_kernel_irqchip {
> 
> kvm_kernel_irqchip_mode?

If we append the mode, what about just "kvm_irqchip_mode"?

irqchip_in_kernel() tells which one is in the kernel.

>> +	KVM_IRQCHIP_NONE,
>> +	KVM_IRQCHIP_KVM,          /* created in KVM_CREATE_IRQCHIP */
>> +	KVM_IRQCHIP_SPLIT,        /* created in KVM_CAP_SPLIT_IRQCHIP */
> 
> Since you pretty much asked to nitpick,

I am always interested in nitpicks!

>                                         KVM_IRQCHIP_KERNEL would
> match irqchip_in_kernel better.

Matching the enum name prefix would be nice, so I'd rename it to
enum kvm_irqchip_kernel_mode then.  I'd keep them this way if we go with
enum kvm_irqchip_mode.

>                                  Also, s/in/with/? :)

Ok.

>> +};
>> +
>>  struct kvm_arch {
>>  	unsigned int n_used_mmu_pages;
>>  	unsigned int n_requested_mmu_pages;
>> @@ -778,8 +784,7 @@ struct kvm_arch {
>>  
>>  	u64 disabled_quirks;
>>  
>> -	bool irqchip_kvm;
>> -	bool irqchip_split;
>> +	enum kvm_kernel_irqchip irqchip;
> 
> irqchip_mode?

Yes, thanks.

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

[Paolo Bonzini]

On 25/11/2016 18:11, Radim Krčmář wrote:
> 2016-11-25 03:59-0500, Paolo Bonzini:
>> ----- Original Message -----
>>> From: "Radim Krčmář" <rkrcmar@redhat.com>
>>> To: "Paolo Bonzini" <pbonzini@redhat.com>
>>> Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org
>>> Sent: Thursday, November 24, 2016 6:21:04 PM
>>> Subject: Re: [PATCH 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()
>>>
>>> 2016-11-24 11:55-0500, Paolo Bonzini:
>>>>> irqchip_in_kernel() tried to save a bit by reusing pic_irqchip(), but it
>>>>> just complicated the code.
>>>>> Add kvm->arch.irqchip_kvm that matches kvm->arch.irqchip_split.
>>>>> (Turning them into an exclusive type would be nicer.)
>>>>
>>>> Then do it. ;)
>>>
>>> It is hard to name! :)
>>>
>>> I would squash something like this if the names were ok:
>>>
>>> diff --git a/arch/x86/include/asm/kvm_host.h
>>> b/arch/x86/include/asm/kvm_host.h
>>> index 929228ec2839..726235f0e3f3 100644
>>> --- a/arch/x86/include/asm/kvm_host.h
>>> +++ b/arch/x86/include/asm/kvm_host.h
>>> @@ -706,6 +706,12 @@ struct kvm_hv {
>>>  	HV_REFERENCE_TSC_PAGE tsc_ref;
>>>  };
>>>  
>>> +enum kvm_kernel_irqchip {
>>
>> kvm_kernel_irqchip_mode?
> 
> If we append the mode, what about just "kvm_irqchip_mode"?
> 
> irqchip_in_kernel() tells which one is in the kernel.
> 
>>> +	KVM_IRQCHIP_NONE,
>>> +	KVM_IRQCHIP_KVM,          /* created in KVM_CREATE_IRQCHIP */
>>> +	KVM_IRQCHIP_SPLIT,        /* created in KVM_CAP_SPLIT_IRQCHIP */
>>
>> Since you pretty much asked to nitpick,
> 
> I am always interested in nitpicks!
> 
>>                                         KVM_IRQCHIP_KERNEL would
>> match irqchip_in_kernel better.
> 
> Matching the enum name prefix would be nice, so I'd rename it to
> enum kvm_irqchip_kernel_mode then.  I'd keep them this way if we go with
> enum kvm_irqchip_mode.

kvm_irqchip_mode is best I think (NONE/KERNEL/SPLIT).

Paolo

>>                                  Also, s/in/with/? :)
> 
> Ok.
> 
>>> +};
>>> +
>>>  struct kvm_arch {
>>>  	unsigned int n_used_mmu_pages;
>>>  	unsigned int n_requested_mmu_pages;
>>> @@ -778,8 +784,7 @@ struct kvm_arch {
>>>  
>>>  	u64 disabled_quirks;
>>>  
>>> -	bool irqchip_kvm;
>>> -	bool irqchip_split;
>>> +	enum kvm_kernel_irqchip irqchip;
>>
>> irqchip_mode?
> 
> Yes, thanks.
>