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
Reviewed-by: Paolo Bonzini <[email protected]>
Signed-off-by: Radim Krčmář <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Radim Krčmář <[email protected]>
---
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
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ář <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Radim Krčmář <[email protected]>
---
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
Reviewed-by: Paolo Bonzini <[email protected]>
Signed-off-by: Radim Krčmář <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Radim Krčmář <[email protected]>
---
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
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();
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.