* [PATCH v2 1/6] KVM: x86: don't allow kernel irqchip with split irqchip
2016-12-16 15:10 [PATCH v2 0/6] KVM: x86: minor irqchip improvements (API change) Radim Krčmář
@ 2016-12-16 15:10 ` Radim Krčmář
2017-01-03 12:15 ` David Hildenbrand
2016-12-16 15:10 ` [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip() Radim Krčmář
` (4 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Radim Krčmář @ 2016-12-16 15:10 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/6] KVM: x86: don't allow kernel irqchip with split irqchip
2016-12-16 15:10 ` [PATCH v2 1/6] KVM: x86: don't allow kernel irqchip with split irqchip Radim Krčmář
@ 2017-01-03 12:15 ` David Hildenbrand
0 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2017-01-03 12:15 UTC (permalink / raw)
To: Radim Krčmář, linux-kernel, kvm; +Cc: Paolo Bonzini
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()
2016-12-16 15:10 [PATCH v2 0/6] KVM: x86: minor irqchip improvements (API change) Radim Krčmář
2016-12-16 15:10 ` [PATCH v2 1/6] KVM: x86: don't allow kernel irqchip with split irqchip Radim Krčmář
@ 2016-12-16 15:10 ` Radim Krčmář
2016-12-16 15:25 ` Paolo Bonzini
2017-01-03 12:56 ` David Hildenbrand
2016-12-16 15:10 ` [PATCH v2 3/6] KVM: x86: make pic setup code look like ioapic setup Radim Krčmář
` (3 subsequent siblings)
5 siblings, 2 replies; 21+ messages in thread
From: Radim Krčmář @ 2016-12-16 15:10 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()
2016-12-16 15:10 ` [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip() Radim Krčmář
@ 2016-12-16 15:25 ` Paolo Bonzini
2016-12-16 15:44 ` Radim Krčmář
2017-01-03 12:56 ` David Hildenbrand
1 sibling, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2016-12-16 15:25 UTC (permalink / raw)
To: Radim Krčmář, linux-kernel, kvm
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();
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()
2016-12-16 15:25 ` Paolo Bonzini
@ 2016-12-16 15:44 ` Radim Krčmář
0 siblings, 0 replies; 21+ messages in thread
From: Radim Krčmář @ 2016-12-16 15:44 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()
2016-12-16 15:10 ` [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip() Radim Krčmář
2016-12-16 15:25 ` Paolo Bonzini
@ 2017-01-03 12:56 ` David Hildenbrand
2017-01-10 5:09 ` Peter Xu
1 sibling, 1 reply; 21+ messages in thread
From: David Hildenbrand @ 2017-01-03 12:56 UTC (permalink / raw)
To: Radim Krčmář, linux-kernel, kvm; +Cc: Paolo Bonzini
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()
2017-01-03 12:56 ` David Hildenbrand
@ 2017-01-10 5:09 ` Peter Xu
2017-01-10 9:39 ` Paolo Bonzini
0 siblings, 1 reply; 21+ messages in thread
From: Peter Xu @ 2017-01-10 5:09 UTC (permalink / raw)
To: David Hildenbrand
Cc: Radim Krčmář, linux-kernel, kvm, Paolo Bonzini
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip()
2017-01-10 5:09 ` Peter Xu
@ 2017-01-10 9:39 ` Paolo Bonzini
0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2017-01-10 9:39 UTC (permalink / raw)
To: Peter Xu, David Hildenbrand
Cc: Radim Krčmář, linux-kernel, kvm
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 3/6] KVM: x86: make pic setup code look like ioapic setup
2016-12-16 15:10 [PATCH v2 0/6] KVM: x86: minor irqchip improvements (API change) Radim Krčmář
2016-12-16 15:10 ` [PATCH v2 1/6] KVM: x86: don't allow kernel irqchip with split irqchip Radim Krčmář
2016-12-16 15:10 ` [PATCH v2 2/6] KVM: x86: decouple irqchip_in_kernel() and pic_irqchip() Radim Krčmář
@ 2016-12-16 15:10 ` Radim Krčmář
2017-01-03 13:04 ` David Hildenbrand
2016-12-16 15:10 ` [PATCH v2 4/6] KVM: x86: refactor pic setup in kvm_set_routing_entry Radim Krčmář
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Radim Krčmář @ 2016-12-16 15:10 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/6] KVM: x86: make pic setup code look like ioapic setup
2016-12-16 15:10 ` [PATCH v2 3/6] KVM: x86: make pic setup code look like ioapic setup Radim Krčmář
@ 2017-01-03 13:04 ` David Hildenbrand
2017-01-03 16:12 ` Paolo Bonzini
0 siblings, 1 reply; 21+ messages in thread
From: David Hildenbrand @ 2017-01-03 13:04 UTC (permalink / raw)
To: Radim Krčmář, linux-kernel, kvm; +Cc: Paolo Bonzini
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/6] KVM: x86: make pic setup code look like ioapic setup
2017-01-03 13:04 ` David Hildenbrand
@ 2017-01-03 16:12 ` Paolo Bonzini
2017-01-04 9:10 ` David Hildenbrand
0 siblings, 1 reply; 21+ messages in thread
From: Paolo Bonzini @ 2017-01-03 16:12 UTC (permalink / raw)
To: David Hildenbrand, Radim Krčmář, linux-kernel, kvm
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/6] KVM: x86: make pic setup code look like ioapic setup
2017-01-03 16:12 ` Paolo Bonzini
@ 2017-01-04 9:10 ` David Hildenbrand
0 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2017-01-04 9:10 UTC (permalink / raw)
To: Paolo Bonzini, Radim Krčmář, linux-kernel, kvm
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 4/6] KVM: x86: refactor pic setup in kvm_set_routing_entry
2016-12-16 15:10 [PATCH v2 0/6] KVM: x86: minor irqchip improvements (API change) Radim Krčmář
` (2 preceding siblings ...)
2016-12-16 15:10 ` [PATCH v2 3/6] KVM: x86: make pic setup code look like ioapic setup Radim Krčmář
@ 2016-12-16 15:10 ` Radim Krčmář
2017-01-03 13:05 ` David Hildenbrand
2016-12-16 15:10 ` [PATCH v2 5/6] KVM: x86: prevent setup of invalid routes Radim Krčmář
2016-12-16 15:10 ` [PATCH v2 6/6] KVM: x86: simplify conditions with split/kernel irqchip Radim Krčmář
5 siblings, 1 reply; 21+ messages in thread
From: Radim Krčmář @ 2016-12-16 15:10 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 4/6] KVM: x86: refactor pic setup in kvm_set_routing_entry
2016-12-16 15:10 ` [PATCH v2 4/6] KVM: x86: refactor pic setup in kvm_set_routing_entry Radim Krčmář
@ 2017-01-03 13:05 ` David Hildenbrand
0 siblings, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2017-01-03 13:05 UTC (permalink / raw)
To: Radim Krčmář, linux-kernel, kvm; +Cc: Paolo Bonzini
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 5/6] KVM: x86: prevent setup of invalid routes
2016-12-16 15:10 [PATCH v2 0/6] KVM: x86: minor irqchip improvements (API change) Radim Krčmář
` (3 preceding siblings ...)
2016-12-16 15:10 ` [PATCH v2 4/6] KVM: x86: refactor pic setup in kvm_set_routing_entry Radim Krčmář
@ 2016-12-16 15:10 ` Radim Krčmář
2017-01-03 13:06 ` David Hildenbrand
2017-01-10 5:26 ` Peter Xu
2016-12-16 15:10 ` [PATCH v2 6/6] KVM: x86: simplify conditions with split/kernel irqchip Radim Krčmář
5 siblings, 2 replies; 21+ messages in thread
From: Radim Krčmář @ 2016-12-16 15:10 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 5/6] KVM: x86: prevent setup of invalid routes
2016-12-16 15:10 ` [PATCH v2 5/6] KVM: x86: prevent setup of invalid routes Radim Krčmář
@ 2017-01-03 13:06 ` David Hildenbrand
2017-01-10 5:26 ` Peter Xu
1 sibling, 0 replies; 21+ messages in thread
From: David Hildenbrand @ 2017-01-03 13:06 UTC (permalink / raw)
To: Radim Krčmář, linux-kernel, kvm; +Cc: Paolo Bonzini
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 5/6] KVM: x86: prevent setup of invalid routes
2016-12-16 15:10 ` [PATCH v2 5/6] KVM: x86: prevent setup of invalid routes Radim Krčmář
2017-01-03 13:06 ` David Hildenbrand
@ 2017-01-10 5:26 ` Peter Xu
2017-01-10 9:43 ` Paolo Bonzini
1 sibling, 1 reply; 21+ messages in thread
From: Peter Xu @ 2017-01-10 5:26 UTC (permalink / raw)
To: Radim Krčmář; +Cc: linux-kernel, kvm, Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 5/6] KVM: x86: prevent setup of invalid routes
2017-01-10 5:26 ` Peter Xu
@ 2017-01-10 9:43 ` Paolo Bonzini
0 siblings, 0 replies; 21+ messages in thread
From: Paolo Bonzini @ 2017-01-10 9:43 UTC (permalink / raw)
To: Peter Xu, Radim Krčmář; +Cc: linux-kernel, kvm
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
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 6/6] KVM: x86: simplify conditions with split/kernel irqchip
2016-12-16 15:10 [PATCH v2 0/6] KVM: x86: minor irqchip improvements (API change) Radim Krčmář
` (4 preceding siblings ...)
2016-12-16 15:10 ` [PATCH v2 5/6] KVM: x86: prevent setup of invalid routes Radim Krčmář
@ 2016-12-16 15:10 ` Radim Krčmář
2017-01-03 13:20 ` David Hildenbrand
5 siblings, 1 reply; 21+ messages in thread
From: Radim Krčmář @ 2016-12-16 15:10 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: Paolo Bonzini
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
^ permalink raw reply related [flat|nested] 21+ messages in thread