| Message ID | 20200109134713.14755-1-cohuck@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: check if kernel irqchip is actually enabled | expand |
On Thu, 9 Jan 2020 14:47:13 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On s390, we only allow userspace to create an in-kernel irqchip > if it has first enabled the KVM_CAP_S390_IRQCHIP vm capability. > Let's assume that a userspace that enabled that capability has > created an irqchip as well. > > Fixes: 84223598778b ("KVM: s390: irq routing for adapter interrupts.") > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > > A more precise check would be to add a field in kvm_arch that tracks > whether an irqchip has actually been created; not sure if that is > really needed. > > Found while trying to hunt down QEMU crashes with kvm-irqchip=off; s/kvm-irqchip/kernel_irqchip/ > this is not sufficient, though. I *think* everything but irqfds > should work without kvm-irqchip as well, but have not found the problem > yet. > > --- > arch/s390/kvm/irq.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/s390/kvm/irq.h b/arch/s390/kvm/irq.h > index 484608c71dd0..30e13d031379 100644 > --- a/arch/s390/kvm/irq.h > +++ b/arch/s390/kvm/irq.h > @@ -13,7 +13,7 @@ > > static inline int irqchip_in_kernel(struct kvm *kvm) > { > - return 1; > + return !!kvm->arch.use_irqchip; > } > > #endif
On 09.01.20 14:47, Cornelia Huck wrote: > On s390, we only allow userspace to create an in-kernel irqchip > if it has first enabled the KVM_CAP_S390_IRQCHIP vm capability. > Let's assume that a userspace that enabled that capability has > created an irqchip as well. > > Fixes: 84223598778b ("KVM: s390: irq routing for adapter interrupts.") > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > > A more precise check would be to add a field in kvm_arch that tracks > whether an irqchip has actually been created; not sure if that is > really needed. > > Found while trying to hunt down QEMU crashes with kvm-irqchip=off; > this is not sufficient, though. I *think* everything but irqfds > should work without kvm-irqchip as well, but have not found the problem > yet. > > --- > arch/s390/kvm/irq.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/s390/kvm/irq.h b/arch/s390/kvm/irq.h > index 484608c71dd0..30e13d031379 100644 > --- a/arch/s390/kvm/irq.h > +++ b/arch/s390/kvm/irq.h > @@ -13,7 +13,7 @@ > > static inline int irqchip_in_kernel(struct kvm *kvm) > { > - return 1; > + return !!kvm->arch.use_irqchip; > } > > #endif > This function is used on s390x/common code only in virt/kvm/irqchip.c:kvm_send_userspace_msi() That function is only used with CONFIG_HAVE_KVM_MSI. That is not selected for s390x. What does this patch fix?
On 09.01.20 14:47, Cornelia Huck wrote: > On s390, we only allow userspace to create an in-kernel irqchip > if it has first enabled the KVM_CAP_S390_IRQCHIP vm capability. > Let's assume that a userspace that enabled that capability has > created an irqchip as well. > > Fixes: 84223598778b ("KVM: s390: irq routing for adapter interrupts.") > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > > A more precise check would be to add a field in kvm_arch that tracks > whether an irqchip has actually been created; not sure if that is > really needed. I think this is semantically wrong. We always have in-kernel irq handling. It is actually not possible to not use it. So I understand where you are coming from but this feels kind of wrong. > > Found while trying to hunt down QEMU crashes with kvm-irqchip=off; > this is not sufficient, though. I *think* everything but irqfds > should work without kvm-irqchip as well, but have not found the problem > yet. > > --- > arch/s390/kvm/irq.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/s390/kvm/irq.h b/arch/s390/kvm/irq.h > index 484608c71dd0..30e13d031379 100644 > --- a/arch/s390/kvm/irq.h > +++ b/arch/s390/kvm/irq.h > @@ -13,7 +13,7 @@ > > static inline int irqchip_in_kernel(struct kvm *kvm) > { > - return 1; > + return !!kvm->arch.use_irqchip; > } > > #endif >
On Thu, 9 Jan 2020 14:57:43 +0100 David Hildenbrand <david@redhat.com> wrote: > On 09.01.20 14:47, Cornelia Huck wrote: > > On s390, we only allow userspace to create an in-kernel irqchip > > if it has first enabled the KVM_CAP_S390_IRQCHIP vm capability. > > Let's assume that a userspace that enabled that capability has > > created an irqchip as well. > > > > Fixes: 84223598778b ("KVM: s390: irq routing for adapter interrupts.") > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > --- > > > > A more precise check would be to add a field in kvm_arch that tracks > > whether an irqchip has actually been created; not sure if that is > > really needed. > > > > Found while trying to hunt down QEMU crashes with kvm-irqchip=off; > > this is not sufficient, though. I *think* everything but irqfds > > should work without kvm-irqchip as well, but have not found the problem > > yet. > > > > --- > > arch/s390/kvm/irq.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/s390/kvm/irq.h b/arch/s390/kvm/irq.h > > index 484608c71dd0..30e13d031379 100644 > > --- a/arch/s390/kvm/irq.h > > +++ b/arch/s390/kvm/irq.h > > @@ -13,7 +13,7 @@ > > > > static inline int irqchip_in_kernel(struct kvm *kvm) > > { > > - return 1; > > + return !!kvm->arch.use_irqchip; > > } > > > > #endif > > > > This function is used on s390x/common code only in > > virt/kvm/irqchip.c:kvm_send_userspace_msi() > > That function is only used with CONFIG_HAVE_KVM_MSI. That is not > selected for s390x. > > What does this patch fix? Currently, only future callers... but maybe this should be an optional function instead that is only defined by the architectures that can actually do something useful here? TBH, I'm not completely sure what this function is actually supposed to indicate...
On Thu, 9 Jan 2020 15:06:22 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 09.01.20 14:47, Cornelia Huck wrote: > > On s390, we only allow userspace to create an in-kernel irqchip > > if it has first enabled the KVM_CAP_S390_IRQCHIP vm capability. > > Let's assume that a userspace that enabled that capability has > > created an irqchip as well. > > > > Fixes: 84223598778b ("KVM: s390: irq routing for adapter interrupts.") > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > --- > > > > A more precise check would be to add a field in kvm_arch that tracks > > whether an irqchip has actually been created; not sure if that is > > really needed. > > I think this is semantically wrong. We always have in-kernel irq handling. > It is actually not possible to not use it. So I understand where you are coming > from but this feels kind of wrong. You cannot actually call create_irqchip if not enabled, though... as I said in my other reply, the intended semantics here are a bit unclear. > > > > > Found while trying to hunt down QEMU crashes with kvm-irqchip=off; > > this is not sufficient, though. I *think* everything but irqfds > > should work without kvm-irqchip as well, but have not found the problem > > yet. > > > > --- > > arch/s390/kvm/irq.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/s390/kvm/irq.h b/arch/s390/kvm/irq.h > > index 484608c71dd0..30e13d031379 100644 > > --- a/arch/s390/kvm/irq.h > > +++ b/arch/s390/kvm/irq.h > > @@ -13,7 +13,7 @@ > > > > static inline int irqchip_in_kernel(struct kvm *kvm) > > { > > - return 1; > > + return !!kvm->arch.use_irqchip; > > } > > > > #endif > > >
diff --git a/arch/s390/kvm/irq.h b/arch/s390/kvm/irq.h index 484608c71dd0..30e13d031379 100644 --- a/arch/s390/kvm/irq.h +++ b/arch/s390/kvm/irq.h @@ -13,7 +13,7 @@ static inline int irqchip_in_kernel(struct kvm *kvm) { - return 1; + return !!kvm->arch.use_irqchip; } #endif
On s390, we only allow userspace to create an in-kernel irqchip if it has first enabled the KVM_CAP_S390_IRQCHIP vm capability. Let's assume that a userspace that enabled that capability has created an irqchip as well. Fixes: 84223598778b ("KVM: s390: irq routing for adapter interrupts.") Signed-off-by: Cornelia Huck <cohuck@redhat.com> --- A more precise check would be to add a field in kvm_arch that tracks whether an irqchip has actually been created; not sure if that is really needed. Found while trying to hunt down QEMU crashes with kvm-irqchip=off; this is not sufficient, though. I *think* everything but irqfds should work without kvm-irqchip as well, but have not found the problem yet. --- arch/s390/kvm/irq.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)