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
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ář <[email protected]>
---
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
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ář <[email protected]>
---
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
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ář <[email protected]>
---
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
Signed-off-by: Radim Krčmář <[email protected]>
---
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
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ář <[email protected]>
---
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
Signed-off-by: Radim Krčmář <[email protected]>
---
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
> 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ář <[email protected]>
> ---
> 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
>
>
----- Original Message -----
> From: "Radim Krčmář" <[email protected]>
> To: [email protected], [email protected]
> Cc: "Paolo Bonzini" <[email protected]>
> 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 <[email protected]>
> 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
>
>
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;
----- Original Message -----
> From: "Radim Krčmář" <[email protected]>
> To: "Paolo Bonzini" <[email protected]>
> Cc: [email protected], [email protected]
> 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;
>
2016-11-25 03:59-0500, Paolo Bonzini:
> ----- Original Message -----
>> From: "Radim Krčmář" <[email protected]>
>> To: "Paolo Bonzini" <[email protected]>
>> Cc: [email protected], [email protected]
>> 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.
On 25/11/2016 18:11, Radim Krčmář wrote:
> 2016-11-25 03:59-0500, Paolo Bonzini:
>> ----- Original Message -----
>>> From: "Radim Krčmář" <[email protected]>
>>> To: "Paolo Bonzini" <[email protected]>
>>> Cc: [email protected], [email protected]
>>> 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.
>