* Re: [PATCH] irqbypass: fix error handle when irq_bypass_register_producer() return fails
[not found] <1601470479-26848-1-git-send-email-guomin_chen@sina.com>
@ 2020-09-30 14:09 ` Alex Williamson
2020-10-09 4:30 ` gchen chen
0 siblings, 1 reply; 6+ messages in thread
From: Alex Williamson @ 2020-09-30 14:09 UTC (permalink / raw)
To: guomin_chen
Cc: Cornelia Huck, Jiang Yi, Marc Zyngier, Peter Xu, Eric Auger,
Michael S. Tsirkin, Jason Wang, guomin chen, kvm, linux-kernel
Please version your postings so we know which one to consider as the
current proposal.
On Wed, 30 Sep 2020 20:54:39 +0800
guomin_chen@sina.com wrote:
> From: guomin chen <guomin_chen@sina.com>
>
> When the producer object registration fails,In the future, due to
> incorrect matching when unregistering, list_del(&producer->node)
> may still be called, then trigger a BUG:
>
> vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> ...
> list_del corruption, ffff8f7fb8ba0828->next is LIST_POISON1 (dead000000000100)
> ------------[ cut here ]------------
> kernel BUG at lib/list_debug.c:47!
> invalid opcode: 0000 [#1] SMP NOPTI
> CPU: 29 PID: 3914 Comm: qemu-kvm Kdump: loaded Tainted: G E
> -------- - -4.18.0-193.6.3.el8.x86_64 #1
> Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> BIOS -[IVE636Z-2.13]- 07/18/2019
> RIP: 0010:__list_del_entry_valid.cold.1+0x12/0x4c
> Code: ce ff 0f 0b 48 89 c1 4c 89 c6 48 c7 c7 40 85 4d 88 e8 8c bc
> ce ff 0f 0b 48 89 fe 48 89 c2 48 c7 c7 d0 85 4d 88 e8 78 bc
> ce ff <0f> 0b 48 c7 c7 80 86 4d 88 e8 6a bc ce ff 0f 0b 48
> 89 f2 48 89 fe
> RSP: 0018:ffffaa9d60197d20 EFLAGS: 00010246
> RAX: 000000000000004e RBX: ffff8f7fb8ba0828 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff8f7fbf4d6a08 RDI: ffff8f7fbf4d6a08
> RBP: 0000000000000000 R08: 000000000000084b R09: 000000000000005d
> R10: 0000000000000000 R11: ffffaa9d60197bd0 R12: ffff8f4fbe863000
> R13: 00000000000000c2 R14: 0000000000000000 R15: 0000000000000000
> FS: 00007f7cb97fa700(0000) GS:ffff8f7fbf4c0000(0000)
> knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fcf31da4000 CR3: 0000005f6d404001 CR4: 00000000007626e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
> Call Trace:
> irq_bypass_unregister_producer+0x9b/0xf0 [irqbypass]
> vfio_msi_set_vector_signal+0x8c/0x290 [vfio_pci]
> ? load_fixmap_gdt+0x22/0x30
> vfio_msi_set_block+0x6e/0xd0 [vfio_pci]
> vfio_pci_ioctl+0x218/0xbe0 [vfio_pci]
> ? kvm_vcpu_ioctl+0xf2/0x5f0 [kvm]
> do_vfs_ioctl+0xa4/0x630
> ? syscall_trace_enter+0x1d3/0x2c0
> ksys_ioctl+0x60/0x90
> __x64_sys_ioctl+0x16/0x20
> do_syscall_64+0x5b/0x1a0
> entry_SYSCALL_64_after_hwframe+0x65/0xca
>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Cc: Jiang Yi <giangyi@amazon.com>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: kvm@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: guomin chen <guomin_chen@sina.com>
> ---
> drivers/vfio/pci/vfio_pci_intrs.c | 13 +++++++++++--
> drivers/vhost/vdpa.c | 7 +++++++
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 1d9fb25..c371943 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -352,12 +352,21 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
> vdev->ctx[vector].producer.token = trigger;
> vdev->ctx[vector].producer.irq = irq;
> ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> - if (unlikely(ret))
> + if (unlikely(ret)) {
> dev_info(&pdev->dev,
> "irq bypass producer (token %p) registration fails: %d\n",
> vdev->ctx[vector].producer.token, ret);
>
> - vdev->ctx[vector].trigger = trigger;
> + kfree(vdev->ctx[vector].name);
> + eventfd_ctx_put(trigger);
> +
> + cmd = vfio_pci_memory_lock_and_enable(vdev);
> + free_irq(irq, trigger);
> + vfio_pci_memory_unlock_and_restore(vdev, cmd);
> +
> + vdev->ctx[vector].trigger = NULL;
> + } else
> + vdev->ctx[vector].trigger = trigger;
>
> return 0;
> }
Once again, the irq bypass registration cannot cause the vector setup
to fail, either by returning an error code or failing to configure the
vector while returning success. It's my assertion that we simply need
to set the producer.token to NULL on failure such that unregistering
the producer will not generate a match, as you've done below. The
vector still works even if this registration fails.
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 796fe97..4e082b8 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -107,6 +107,13 @@ static void vhost_vdpa_setup_vq_irq(struct vhost_vdpa *v, u16 qid)
> vq->call_ctx.producer.token = vq->call_ctx.ctx;
> vq->call_ctx.producer.irq = irq;
> ret = irq_bypass_register_producer(&vq->call_ctx.producer);
> + if (unlikely(ret)) {
> + /*
> + * If registration failed,
> + * there is no need to unregister later.
> + */
> + vq->call_ctx.producer.token = NULL;
> + }
> spin_unlock(&vq->call_ctx.ctx_lock);
> }
>
'ret' is otherwise unused in this function, so we could simply remove
ret and change this to
if (irq_bypass_register_producer(&vq->call_ctx.producer)) {
/* avoid generating bogus match on unregister */
vq->call_ctx.producer.token = NULL;
}
Also please submit vfio and vdpa as separate patches so that the
respective maintainer for each area can handle them. Thanks,
Alex
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] irqbypass: fix error handle when irq_bypass_register_producer() return fails
2020-09-30 14:09 ` [PATCH] irqbypass: fix error handle when irq_bypass_register_producer() return fails Alex Williamson
@ 2020-10-09 4:30 ` gchen chen
2020-10-09 18:44 ` Alex Williamson
0 siblings, 1 reply; 6+ messages in thread
From: gchen chen @ 2020-10-09 4:30 UTC (permalink / raw)
To: Alex Williamson
Cc: guomin_chen, Cornelia Huck, Jiang Yi, Marc Zyngier, Peter Xu,
Eric Auger, Michael S. Tsirkin, Jason Wang, kvm, linux-kernel
Alex Williamson <alex.williamson@redhat.com> 于2020年9月30日周三 下午10:09写道：
>
>
> Please version your postings so we know which one to consider as the
> current proposal.
>
> On Wed, 30 Sep 2020 20:54:39 +0800
> guomin_chen@sina.com wrote:
>
> > From: guomin chen <guomin_chen@sina.com>
> >
> > When the producer object registration fails,In the future, due to
> > incorrect matching when unregistering, list_del(&producer->node)
> > may still be called, then trigger a BUG:
> >
> > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > ...
> > list_del corruption, ffff8f7fb8ba0828->next is LIST_POISON1 (dead000000000100)
> > ------------[ cut here ]------------
> > kernel BUG at lib/list_debug.c:47!
> > invalid opcode: 0000 [#1] SMP NOPTI
> > CPU: 29 PID: 3914 Comm: qemu-kvm Kdump: loaded Tainted: G E
> > -------- - -4.18.0-193.6.3.el8.x86_64 #1
> > Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> > BIOS -[IVE636Z-2.13]- 07/18/2019
> > RIP: 0010:__list_del_entry_valid.cold.1+0x12/0x4c
> > Code: ce ff 0f 0b 48 89 c1 4c 89 c6 48 c7 c7 40 85 4d 88 e8 8c bc
> > ce ff 0f 0b 48 89 fe 48 89 c2 48 c7 c7 d0 85 4d 88 e8 78 bc
> > ce ff <0f> 0b 48 c7 c7 80 86 4d 88 e8 6a bc ce ff 0f 0b 48
> > 89 f2 48 89 fe
> > RSP: 0018:ffffaa9d60197d20 EFLAGS: 00010246
> > RAX: 000000000000004e RBX: ffff8f7fb8ba0828 RCX: 0000000000000000
> > RDX: 0000000000000000 RSI: ffff8f7fbf4d6a08 RDI: ffff8f7fbf4d6a08
> > RBP: 0000000000000000 R08: 000000000000084b R09: 000000000000005d
> > R10: 0000000000000000 R11: ffffaa9d60197bd0 R12: ffff8f4fbe863000
> > R13: 00000000000000c2 R14: 0000000000000000 R15: 0000000000000000
> > FS: 00007f7cb97fa700(0000) GS:ffff8f7fbf4c0000(0000)
> > knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007fcf31da4000 CR3: 0000005f6d404001 CR4: 00000000007626e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > PKRU: 55555554
> > Call Trace:
> > irq_bypass_unregister_producer+0x9b/0xf0 [irqbypass]
> > vfio_msi_set_vector_signal+0x8c/0x290 [vfio_pci]
> > ? load_fixmap_gdt+0x22/0x30
> > vfio_msi_set_block+0x6e/0xd0 [vfio_pci]
> > vfio_pci_ioctl+0x218/0xbe0 [vfio_pci]
> > ? kvm_vcpu_ioctl+0xf2/0x5f0 [kvm]
> > do_vfs_ioctl+0xa4/0x630
> > ? syscall_trace_enter+0x1d3/0x2c0
> > ksys_ioctl+0x60/0x90
> > __x64_sys_ioctl+0x16/0x20
> > do_syscall_64+0x5b/0x1a0
> > entry_SYSCALL_64_after_hwframe+0x65/0xca
> >
> > Cc: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Cornelia Huck <cohuck@redhat.com>
> > Cc: Jiang Yi <giangyi@amazon.com>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Peter Xu <peterx@redhat.com>
> > Cc: Eric Auger <eric.auger@redhat.com>
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Cc: kvm@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: guomin chen <guomin_chen@sina.com>
> > ---
> > drivers/vfio/pci/vfio_pci_intrs.c | 13 +++++++++++--
> > drivers/vhost/vdpa.c | 7 +++++++
> > 2 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> > index 1d9fb25..c371943 100644
> > --- a/drivers/vfio/pci/vfio_pci_intrs.c
> > +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> > @@ -352,12 +352,21 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
> > vdev->ctx[vector].producer.token = trigger;
> > vdev->ctx[vector].producer.irq = irq;
> > ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> > - if (unlikely(ret))
> > + if (unlikely(ret)) {
> > dev_info(&pdev->dev,
> > "irq bypass producer (token %p) registration fails: %d\n",
> > vdev->ctx[vector].producer.token, ret);
> >
> > - vdev->ctx[vector].trigger = trigger;
> > + kfree(vdev->ctx[vector].name);
> > + eventfd_ctx_put(trigger);
> > +
> > + cmd = vfio_pci_memory_lock_and_enable(vdev);
> > + free_irq(irq, trigger);
> > + vfio_pci_memory_unlock_and_restore(vdev, cmd);
> > +
> > + vdev->ctx[vector].trigger = NULL;
> > + } else
> > + vdev->ctx[vector].trigger = trigger;
> >
> > return 0;
> > }
>
> Once again, the irq bypass registration cannot cause the vector setup
> to fail, either by returning an error code or failing to configure the
> vector while returning success. It's my assertion that we simply need
> to set the producer.token to NULL on failure such that unregistering
> the producer will not generate a match, as you've done below. The
> vector still works even if this registration fails.
>
Yes, the irq bypass registration cannot cause the vector setup to fail.
But if I simply set producer.token to NULL when fails, instead of
cleaning up vector, it will trigger the following BUG:
vfio_ecap_init: 0000:db:00.0 hiding ecap 0x1e@0x310
vfio-pci 0000:db:00.0: irq bypass producer (token 000000004409229f)
registration fails: -16
------------[ cut here ]------------
kernel BUG at drivers/pci/msi.c:352!
invalid opcode: 0000 [#1] SMP NOPTI
CPU: 55 PID: 9389 Comm: qemu-kvm Kdump: loaded Tainted: G
E --------- - - 4.18.0-193.irqb.r1.el8.x86_64 #1
Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
BIOS -[IVE636Z-2.13]- 07/18/2019
RIP: 0010:free_msi_irqs+0x180/0x1b0
Code: 14 85 c0 0f 84 d5 fe ff ff 31 ed eb 0f 83 c5 01 39 6b 14 0f 86
c5 fe ff ff 8b 7b 10 01 ef e8 d7 4a c9 ff 48 83 78 70 00 74 e3
<0f> 0b 49 8d b5 b0 00 00 00 e8 e2 e3 c9 ff e9 c7 fe ff ff 48
8b 7b
RSP: 0018:ffffaeca4f4bfcd8 EFLAGS: 00010286
RAX: ffff8bec77441600 RBX: ffff8bbcdb637e40 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 00000000000001ab RDI: ffffffff8ea5b2a0
RBP: 0000000000000000 R08: ffff8bec7e746828 R09: ffff8bec7e7466a8
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8bbcde921308
R13: ffff8bbcde921000 R14: 000000000000000b R15: 0000000000000021
FS: 00007fd18d7fa700(0000) GS:ffff8bec7f6c0000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f83650024a0 CR3: 000000476e70c001 CR4: 00000000007626e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
pci_disable_msix+0xf3/0x120
pci_free_irq_vectors+0xe/0x20
vfio_msi_disable+0x89/0xd0 [vfio_pci]
vfio_pci_set_msi_trigger+0x229/0x2d0 [vfio_pci]
vfio_pci_ioctl+0x24f/0xdb0 [vfio_pci]
? pollwake+0x74/0x90
? wake_up_q+0x70/0x70
do_vfs_ioctl+0xa4/0x630
? __alloc_fd+0x33/0x140
? syscall_trace_enter+0x1d3/0x2c0
ksys_ioctl+0x60/0x90
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x5b/0x1a0
entry_SYSCALL_64_after_hwframe+0x65/0xca
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index 796fe97..4e082b8 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -107,6 +107,13 @@ static void vhost_vdpa_setup_vq_irq(struct vhost_vdpa *v, u16 qid)
> > vq->call_ctx.producer.token = vq->call_ctx.ctx;
> > vq->call_ctx.producer.irq = irq;
> > ret = irq_bypass_register_producer(&vq->call_ctx.producer);
> > + if (unlikely(ret)) {
> > + /*
> > + * If registration failed,
> > + * there is no need to unregister later.
> > + */
> > + vq->call_ctx.producer.token = NULL;
> > + }
> > spin_unlock(&vq->call_ctx.ctx_lock);
> > }
> >
>
> 'ret' is otherwise unused in this function, so we could simply remove
> ret and change this to
>
> if (irq_bypass_register_producer(&vq->call_ctx.producer)) {
> /* avoid generating bogus match on unregister */
> vq->call_ctx.producer.token = NULL;
> }
>
> Also please submit vfio and vdpa as separate patches so that the
> respective maintainer for each area can handle them. Thanks,
>
> Alex
>
OK, thank you, I'll submit it again in two separate patches.
Thanks Alex.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] irqbypass: fix error handle when irq_bypass_register_producer() return fails
2020-10-09 4:30 ` gchen chen
@ 2020-10-09 18:44 ` Alex Williamson
2020-10-10 11:01 ` gchen chen
0 siblings, 1 reply; 6+ messages in thread
From: Alex Williamson @ 2020-10-09 18:44 UTC (permalink / raw)
To: gchen chen
Cc: guomin_chen, Cornelia Huck, Jiang Yi, Marc Zyngier, Peter Xu,
Eric Auger, Michael S. Tsirkin, Jason Wang, kvm, linux-kernel
On Fri, 9 Oct 2020 12:30:04 +0800
gchen chen <gchen.guomin@gmail.com> wrote:
> Alex Williamson <alex.williamson@redhat.com> 于2020年9月30日周三 下午10:09写道：
> >
> >
> > Please version your postings so we know which one to consider as the
> > current proposal.
> >
> > On Wed, 30 Sep 2020 20:54:39 +0800
> > guomin_chen@sina.com wrote:
> >
> > > From: guomin chen <guomin_chen@sina.com>
> > >
> > > When the producer object registration fails,In the future, due to
> > > incorrect matching when unregistering, list_del(&producer->node)
> > > may still be called, then trigger a BUG:
> > >
> > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > ...
> > > list_del corruption, ffff8f7fb8ba0828->next is LIST_POISON1 (dead000000000100)
> > > ------------[ cut here ]------------
> > > kernel BUG at lib/list_debug.c:47!
> > > invalid opcode: 0000 [#1] SMP NOPTI
> > > CPU: 29 PID: 3914 Comm: qemu-kvm Kdump: loaded Tainted: G E
> > > -------- - -4.18.0-193.6.3.el8.x86_64 #1
> > > Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> > > BIOS -[IVE636Z-2.13]- 07/18/2019
> > > RIP: 0010:__list_del_entry_valid.cold.1+0x12/0x4c
> > > Code: ce ff 0f 0b 48 89 c1 4c 89 c6 48 c7 c7 40 85 4d 88 e8 8c bc
> > > ce ff 0f 0b 48 89 fe 48 89 c2 48 c7 c7 d0 85 4d 88 e8 78 bc
> > > ce ff <0f> 0b 48 c7 c7 80 86 4d 88 e8 6a bc ce ff 0f 0b 48
> > > 89 f2 48 89 fe
> > > RSP: 0018:ffffaa9d60197d20 EFLAGS: 00010246
> > > RAX: 000000000000004e RBX: ffff8f7fb8ba0828 RCX: 0000000000000000
> > > RDX: 0000000000000000 RSI: ffff8f7fbf4d6a08 RDI: ffff8f7fbf4d6a08
> > > RBP: 0000000000000000 R08: 000000000000084b R09: 000000000000005d
> > > R10: 0000000000000000 R11: ffffaa9d60197bd0 R12: ffff8f4fbe863000
> > > R13: 00000000000000c2 R14: 0000000000000000 R15: 0000000000000000
> > > FS: 00007f7cb97fa700(0000) GS:ffff8f7fbf4c0000(0000)
> > > knlGS:0000000000000000
> > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 00007fcf31da4000 CR3: 0000005f6d404001 CR4: 00000000007626e0
> > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > PKRU: 55555554
> > > Call Trace:
> > > irq_bypass_unregister_producer+0x9b/0xf0 [irqbypass]
> > > vfio_msi_set_vector_signal+0x8c/0x290 [vfio_pci]
> > > ? load_fixmap_gdt+0x22/0x30
> > > vfio_msi_set_block+0x6e/0xd0 [vfio_pci]
> > > vfio_pci_ioctl+0x218/0xbe0 [vfio_pci]
> > > ? kvm_vcpu_ioctl+0xf2/0x5f0 [kvm]
> > > do_vfs_ioctl+0xa4/0x630
> > > ? syscall_trace_enter+0x1d3/0x2c0
> > > ksys_ioctl+0x60/0x90
> > > __x64_sys_ioctl+0x16/0x20
> > > do_syscall_64+0x5b/0x1a0
> > > entry_SYSCALL_64_after_hwframe+0x65/0xca
> > >
> > > Cc: Alex Williamson <alex.williamson@redhat.com>
> > > Cc: Cornelia Huck <cohuck@redhat.com>
> > > Cc: Jiang Yi <giangyi@amazon.com>
> > > Cc: Marc Zyngier <maz@kernel.org>
> > > Cc: Peter Xu <peterx@redhat.com>
> > > Cc: Eric Auger <eric.auger@redhat.com>
> > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > Cc: Jason Wang <jasowang@redhat.com>
> > > Cc: kvm@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Signed-off-by: guomin chen <guomin_chen@sina.com>
> > > ---
> > > drivers/vfio/pci/vfio_pci_intrs.c | 13 +++++++++++--
> > > drivers/vhost/vdpa.c | 7 +++++++
> > > 2 files changed, 18 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> > > index 1d9fb25..c371943 100644
> > > --- a/drivers/vfio/pci/vfio_pci_intrs.c
> > > +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> > > @@ -352,12 +352,21 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
> > > vdev->ctx[vector].producer.token = trigger;
> > > vdev->ctx[vector].producer.irq = irq;
> > > ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> > > - if (unlikely(ret))
> > > + if (unlikely(ret)) {
> > > dev_info(&pdev->dev,
> > > "irq bypass producer (token %p) registration fails: %d\n",
> > > vdev->ctx[vector].producer.token, ret);
> > >
> > > - vdev->ctx[vector].trigger = trigger;
> > > + kfree(vdev->ctx[vector].name);
> > > + eventfd_ctx_put(trigger);
> > > +
> > > + cmd = vfio_pci_memory_lock_and_enable(vdev);
> > > + free_irq(irq, trigger);
> > > + vfio_pci_memory_unlock_and_restore(vdev, cmd);
> > > +
> > > + vdev->ctx[vector].trigger = NULL;
> > > + } else
> > > + vdev->ctx[vector].trigger = trigger;
> > >
> > > return 0;
> > > }
> >
> > Once again, the irq bypass registration cannot cause the vector setup
> > to fail, either by returning an error code or failing to configure the
> > vector while returning success. It's my assertion that we simply need
> > to set the producer.token to NULL on failure such that unregistering
> > the producer will not generate a match, as you've done below. The
> > vector still works even if this registration fails.
> >
> Yes, the irq bypass registration cannot cause the vector setup to fail.
> But if I simply set producer.token to NULL when fails, instead of
> cleaning up vector, it will trigger the following BUG:
>
> vfio_ecap_init: 0000:db:00.0 hiding ecap 0x1e@0x310
> vfio-pci 0000:db:00.0: irq bypass producer (token 000000004409229f)
> registration fails: -16
> ------------[ cut here ]------------
> kernel BUG at drivers/pci/msi.c:352!
> invalid opcode: 0000 [#1] SMP NOPTI
> CPU: 55 PID: 9389 Comm: qemu-kvm Kdump: loaded Tainted: G
> E --------- - - 4.18.0-193.irqb.r1.el8.x86_64 #1
> Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> BIOS -[IVE636Z-2.13]- 07/18/2019
> RIP: 0010:free_msi_irqs+0x180/0x1b0
> Code: 14 85 c0 0f 84 d5 fe ff ff 31 ed eb 0f 83 c5 01 39 6b 14 0f 86
> c5 fe ff ff 8b 7b 10 01 ef e8 d7 4a c9 ff 48 83 78 70 00 74 e3
> <0f> 0b 49 8d b5 b0 00 00 00 e8 e2 e3 c9 ff e9 c7 fe ff ff 48
> 8b 7b
> RSP: 0018:ffffaeca4f4bfcd8 EFLAGS: 00010286
> RAX: ffff8bec77441600 RBX: ffff8bbcdb637e40 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: 00000000000001ab RDI: ffffffff8ea5b2a0
> RBP: 0000000000000000 R08: ffff8bec7e746828 R09: ffff8bec7e7466a8
> R10: 0000000000000000 R11: 0000000000000000 R12: ffff8bbcde921308
> R13: ffff8bbcde921000 R14: 000000000000000b R15: 0000000000000021
> FS: 00007fd18d7fa700(0000) GS:ffff8bec7f6c0000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f83650024a0 CR3: 000000476e70c001 CR4: 00000000007626e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
> Call Trace:
> pci_disable_msix+0xf3/0x120
> pci_free_irq_vectors+0xe/0x20
> vfio_msi_disable+0x89/0xd0 [vfio_pci]
> vfio_pci_set_msi_trigger+0x229/0x2d0 [vfio_pci]
> vfio_pci_ioctl+0x24f/0xdb0 [vfio_pci]
> ? pollwake+0x74/0x90
> ? wake_up_q+0x70/0x70
> do_vfs_ioctl+0xa4/0x630
> ? __alloc_fd+0x33/0x140
> ? syscall_trace_enter+0x1d3/0x2c0
> ksys_ioctl+0x60/0x90
> __x64_sys_ioctl+0x16/0x20
> do_syscall_64+0x5b/0x1a0
> entry_SYSCALL_64_after_hwframe+0x65/0xca
Please post the patch that triggers this, I'm not yet convinced we're
speaking of the same solution. The user ioctl cannot fail due to the
failure to setup a bypass accelerator, nor can the ioctl return success
without configuring all of the user requested vectors, which is what I
understand the v2 patch above to do. We simply want to configure the
failed producer such that when we unregister it at user request, we
avoid creating a bogus match. It's not apparent to me why doing that
would cause any changes to the setup or teardown of the MSI vector in
PCI code. Thanks,
Alex
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] irqbypass: fix error handle when irq_bypass_register_producer() return fails
2020-10-09 18:44 ` Alex Williamson
@ 2020-10-10 11:01 ` gchen chen
2020-10-10 14:26 ` Alex Williamson
0 siblings, 1 reply; 6+ messages in thread
From: gchen chen @ 2020-10-10 11:01 UTC (permalink / raw)
To: Alex Williamson
Cc: guomin_chen, Cornelia Huck, Jiang Yi, Marc Zyngier, Peter Xu,
Eric Auger, Michael S. Tsirkin, Jason Wang, kvm, linux-kernel
Alex Williamson <alex.williamson@redhat.com> 于2020年10月10日周六 上午2:44写道：
>
> On Fri, 9 Oct 2020 12:30:04 +0800
> gchen chen <gchen.guomin@gmail.com> wrote:
>
> > Alex Williamson <alex.williamson@redhat.com> 于2020年9月30日周三 下午10:09写道：
> > >
> > >
> > > Please version your postings so we know which one to consider as the
> > > current proposal.
> > >
> > > On Wed, 30 Sep 2020 20:54:39 +0800
> > > guomin_chen@sina.com wrote:
> > >
> > > > From: guomin chen <guomin_chen@sina.com>
> > > >
> > > > When the producer object registration fails,In the future, due to
> > > > incorrect matching when unregistering, list_del(&producer->node)
> > > > may still be called, then trigger a BUG:
> > > >
> > > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > > ...
> > > > list_del corruption, ffff8f7fb8ba0828->next is LIST_POISON1 (dead000000000100)
> > > > ------------[ cut here ]------------
> > > > kernel BUG at lib/list_debug.c:47!
> > > > invalid opcode: 0000 [#1] SMP NOPTI
> > > > CPU: 29 PID: 3914 Comm: qemu-kvm Kdump: loaded Tainted: G E
> > > > -------- - -4.18.0-193.6.3.el8.x86_64 #1
> > > > Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> > > > BIOS -[IVE636Z-2.13]- 07/18/2019
> > > > RIP: 0010:__list_del_entry_valid.cold.1+0x12/0x4c
> > > > Code: ce ff 0f 0b 48 89 c1 4c 89 c6 48 c7 c7 40 85 4d 88 e8 8c bc
> > > > ce ff 0f 0b 48 89 fe 48 89 c2 48 c7 c7 d0 85 4d 88 e8 78 bc
> > > > ce ff <0f> 0b 48 c7 c7 80 86 4d 88 e8 6a bc ce ff 0f 0b 48
> > > > 89 f2 48 89 fe
> > > > RSP: 0018:ffffaa9d60197d20 EFLAGS: 00010246
> > > > RAX: 000000000000004e RBX: ffff8f7fb8ba0828 RCX: 0000000000000000
> > > > RDX: 0000000000000000 RSI: ffff8f7fbf4d6a08 RDI: ffff8f7fbf4d6a08
> > > > RBP: 0000000000000000 R08: 000000000000084b R09: 000000000000005d
> > > > R10: 0000000000000000 R11: ffffaa9d60197bd0 R12: ffff8f4fbe863000
> > > > R13: 00000000000000c2 R14: 0000000000000000 R15: 0000000000000000
> > > > FS: 00007f7cb97fa700(0000) GS:ffff8f7fbf4c0000(0000)
> > > > knlGS:0000000000000000
> > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: 00007fcf31da4000 CR3: 0000005f6d404001 CR4: 00000000007626e0
> > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > PKRU: 55555554
> > > > Call Trace:
> > > > irq_bypass_unregister_producer+0x9b/0xf0 [irqbypass]
> > > > vfio_msi_set_vector_signal+0x8c/0x290 [vfio_pci]
> > > > ? load_fixmap_gdt+0x22/0x30
> > > > vfio_msi_set_block+0x6e/0xd0 [vfio_pci]
> > > > vfio_pci_ioctl+0x218/0xbe0 [vfio_pci]
> > > > ? kvm_vcpu_ioctl+0xf2/0x5f0 [kvm]
> > > > do_vfs_ioctl+0xa4/0x630
> > > > ? syscall_trace_enter+0x1d3/0x2c0
> > > > ksys_ioctl+0x60/0x90
> > > > __x64_sys_ioctl+0x16/0x20
> > > > do_syscall_64+0x5b/0x1a0
> > > > entry_SYSCALL_64_after_hwframe+0x65/0xca
> > > >
> > > > Cc: Alex Williamson <alex.williamson@redhat.com>
> > > > Cc: Cornelia Huck <cohuck@redhat.com>
> > > > Cc: Jiang Yi <giangyi@amazon.com>
> > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > Cc: Peter Xu <peterx@redhat.com>
> > > > Cc: Eric Auger <eric.auger@redhat.com>
> > > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > Cc: kvm@vger.kernel.org
> > > > Cc: linux-kernel@vger.kernel.org
> > > > Signed-off-by: guomin chen <guomin_chen@sina.com>
> > > > ---
> > > > drivers/vfio/pci/vfio_pci_intrs.c | 13 +++++++++++--
> > > > drivers/vhost/vdpa.c | 7 +++++++
> > > > 2 files changed, 18 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> > > > index 1d9fb25..c371943 100644
> > > > --- a/drivers/vfio/pci/vfio_pci_intrs.c
> > > > +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> > > > @@ -352,12 +352,21 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
> > > > vdev->ctx[vector].producer.token = trigger;
> > > > vdev->ctx[vector].producer.irq = irq;
> > > > ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> > > > - if (unlikely(ret))
> > > > + if (unlikely(ret)) {
> > > > dev_info(&pdev->dev,
> > > > "irq bypass producer (token %p) registration fails: %d\n",
> > > > vdev->ctx[vector].producer.token, ret);
> > > >
> > > > - vdev->ctx[vector].trigger = trigger;
> > > > + kfree(vdev->ctx[vector].name);
> > > > + eventfd_ctx_put(trigger);
> > > > +
> > > > + cmd = vfio_pci_memory_lock_and_enable(vdev);
> > > > + free_irq(irq, trigger);
> > > > + vfio_pci_memory_unlock_and_restore(vdev, cmd);
> > > > +
> > > > + vdev->ctx[vector].trigger = NULL;
> > > > + } else
> > > > + vdev->ctx[vector].trigger = trigger;
> > > >
> > > > return 0;
> > > > }
> > >
> > > Once again, the irq bypass registration cannot cause the vector setup
> > > to fail, either by returning an error code or failing to configure the
> > > vector while returning success. It's my assertion that we simply need
> > > to set the producer.token to NULL on failure such that unregistering
> > > the producer will not generate a match, as you've done below. The
> > > vector still works even if this registration fails.
> > >
> > Yes, the irq bypass registration cannot cause the vector setup to fail.
> > But if I simply set producer.token to NULL when fails, instead of
> > cleaning up vector, it will trigger the following BUG:
> >
> > vfio_ecap_init: 0000:db:00.0 hiding ecap 0x1e@0x310
> > vfio-pci 0000:db:00.0: irq bypass producer (token 000000004409229f)
> > registration fails: -16
> > ------------[ cut here ]------------
> > kernel BUG at drivers/pci/msi.c:352!
> > invalid opcode: 0000 [#1] SMP NOPTI
> > CPU: 55 PID: 9389 Comm: qemu-kvm Kdump: loaded Tainted: G
> > E --------- - - 4.18.0-193.irqb.r1.el8.x86_64 #1
> > Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> > BIOS -[IVE636Z-2.13]- 07/18/2019
> > RIP: 0010:free_msi_irqs+0x180/0x1b0
> > Code: 14 85 c0 0f 84 d5 fe ff ff 31 ed eb 0f 83 c5 01 39 6b 14 0f 86
> > c5 fe ff ff 8b 7b 10 01 ef e8 d7 4a c9 ff 48 83 78 70 00 74 e3
> > <0f> 0b 49 8d b5 b0 00 00 00 e8 e2 e3 c9 ff e9 c7 fe ff ff 48
> > 8b 7b
> > RSP: 0018:ffffaeca4f4bfcd8 EFLAGS: 00010286
> > RAX: ffff8bec77441600 RBX: ffff8bbcdb637e40 RCX: 0000000000000000
> > RDX: 0000000000000000 RSI: 00000000000001ab RDI: ffffffff8ea5b2a0
> > RBP: 0000000000000000 R08: ffff8bec7e746828 R09: ffff8bec7e7466a8
> > R10: 0000000000000000 R11: 0000000000000000 R12: ffff8bbcde921308
> > R13: ffff8bbcde921000 R14: 000000000000000b R15: 0000000000000021
> > FS: 00007fd18d7fa700(0000) GS:ffff8bec7f6c0000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007f83650024a0 CR3: 000000476e70c001 CR4: 00000000007626e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > PKRU: 55555554
> > Call Trace:
> > pci_disable_msix+0xf3/0x120
> > pci_free_irq_vectors+0xe/0x20
> > vfio_msi_disable+0x89/0xd0 [vfio_pci]
> > vfio_pci_set_msi_trigger+0x229/0x2d0 [vfio_pci]
> > vfio_pci_ioctl+0x24f/0xdb0 [vfio_pci]
> > ? pollwake+0x74/0x90
> > ? wake_up_q+0x70/0x70
> > do_vfs_ioctl+0xa4/0x630
> > ? __alloc_fd+0x33/0x140
> > ? syscall_trace_enter+0x1d3/0x2c0
> > ksys_ioctl+0x60/0x90
> > __x64_sys_ioctl+0x16/0x20
> > do_syscall_64+0x5b/0x1a0
> > entry_SYSCALL_64_after_hwframe+0x65/0xca
>
> Please post the patch that triggers this, I'm not yet convinced we're
> speaking of the same solution. The user ioctl cannot fail due to the
> failure to setup a bypass accelerator, nor can the ioctl return success
> without configuring all of the user requested vectors, which is what I
> understand the v2 patch above to do. We simply want to configure the
> failed producer such that when we unregister it at user request, we
> avoid creating a bogus match. It's not apparent to me why doing that
> would cause any changes to the setup or teardown of the MSI vector in
> PCI code. Thanks,
>
> Alex
>
Hi Alex, as you said before, I only need to set the producer.token
to NULL on failure such that unregistering the producer will not
generate a match.
So I wrote a patch (As you said patch v2), as follows:
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c
b/drivers/vfio/pci/vfio_pci_intrs.c
index 1d9fb25..1969cd0 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -352,12 +352,15 @@ static int vfio_msi_set_vector_signal(struct
vfio_pci_device *vdev,
vdev->ctx[vector].producer.token = trigger;
vdev->ctx[vector].producer.irq = irq;
ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
- if (unlikely(ret))
+ if (unlikely(ret)) {
dev_info(&pdev->dev,
"irq bypass producer (token %p) registration fails: %d\n",
vdev->ctx[vector].producer.token, ret);
- vdev->ctx[vector].trigger = trigger;
+ eventfd_ctx_put(trigger);
+ vdev->ctx[vector].trigger = NULL;
+ } else
+ vdev->ctx[vector].trigger = trigger;
return 0;
}
--
However, when I use this patch to testing, the following bugs are
triggered when vfio_msi_disable() called because the msi vector
is not cleaned up:
vfio_ecap_init: 0000:db:00.0 hiding ecap 0x1e@0x310
vfio-pci 0000:db:00.0: irq bypass producer (token 000000004409229f)
registration fails: -16
------------[ cut here ]------------
kernel BUG at drivers/pci/msi.c:352!
invalid opcode: 0000 [#1] SMP NOPTI
CPU: 55 PID: 9389 Comm: qemu-kvm Kdump: loaded Tainted: G
E --------- - - 4.18.0-193.irqb.r1.el8.x86_64 #1
Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
BIOS -[IVE636Z-2.13]- 07/18/2019
RIP: 0010:free_msi_irqs+0x180/0x1b0
Code: 14 85 c0 0f 84 d5 fe ff ff 31 ed eb 0f 83 c5 01 39 6b 14 0f 86
c5 fe ff ff 8b 7b 10 01 ef e8 d7 4a c9 ff 48 83 78 70 00 74 e3
<0f> 0b 49 8d b5 b0 00 00 00 e8 e2 e3 c9 ff e9 c7 fe ff ff 48
8b 7b
RSP: 0018:ffffaeca4f4bfcd8 EFLAGS: 00010286
RAX: ffff8bec77441600 RBX: ffff8bbcdb637e40 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 00000000000001ab RDI: ffffffff8ea5b2a0
RBP: 0000000000000000 R08: ffff8bec7e746828 R09: ffff8bec7e7466a8
R10: 0000000000000000 R11: 0000000000000000 R12: ffff8bbcde921308
R13: ffff8bbcde921000 R14: 000000000000000b R15: 0000000000000021
FS: 00007fd18d7fa700(0000) GS:ffff8bec7f6c0000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f83650024a0 CR3: 000000476e70c001 CR4: 00000000007626e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
pci_disable_msix+0xf3/0x120
pci_free_irq_vectors+0xe/0x20
vfio_msi_disable+0x89/0xd0 [vfio_pci]
vfio_pci_set_msi_trigger+0x229/0x2d0 [vfio_pci]
vfio_pci_ioctl+0x24f/0xdb0 [vfio_pci]
? pollwake+0x74/0x90
? wake_up_q+0x70/0x70
do_vfs_ioctl+0xa4/0x630
? __alloc_fd+0x33/0x140
? syscall_trace_enter+0x1d3/0x2c0
ksys_ioctl+0x60/0x90
__x64_sys_ioctl+0x16/0x20
do_syscall_64+0x5b/0x1a0
entry_SYSCALL_64_after_hwframe+0x65/0xca
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] irqbypass: fix error handle when irq_bypass_register_producer() return fails
2020-10-10 11:01 ` gchen chen
@ 2020-10-10 14:26 ` Alex Williamson
2020-10-16 13:38 ` gchen chen
0 siblings, 1 reply; 6+ messages in thread
From: Alex Williamson @ 2020-10-10 14:26 UTC (permalink / raw)
To: gchen chen
Cc: guomin_chen, Cornelia Huck, Jiang Yi, Marc Zyngier, Peter Xu,
Eric Auger, Michael S. Tsirkin, Jason Wang, kvm, linux-kernel
On Sat, 10 Oct 2020 19:01:30 +0800
gchen chen <gchen.guomin@gmail.com> wrote:
> Alex Williamson <alex.williamson@redhat.com> 于2020年10月10日周六 上午2:44写道：
> >
> > On Fri, 9 Oct 2020 12:30:04 +0800
> > gchen chen <gchen.guomin@gmail.com> wrote:
> >
> > > Alex Williamson <alex.williamson@redhat.com> 于2020年9月30日周三 下午10:09写道：
> > > >
> > > >
> > > > Please version your postings so we know which one to consider as the
> > > > current proposal.
> > > >
> > > > On Wed, 30 Sep 2020 20:54:39 +0800
> > > > guomin_chen@sina.com wrote:
> > > >
> > > > > From: guomin chen <guomin_chen@sina.com>
> > > > >
> > > > > When the producer object registration fails,In the future, due to
> > > > > incorrect matching when unregistering, list_del(&producer->node)
> > > > > may still be called, then trigger a BUG:
> > > > >
> > > > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > > > ...
> > > > > list_del corruption, ffff8f7fb8ba0828->next is LIST_POISON1 (dead000000000100)
> > > > > ------------[ cut here ]------------
> > > > > kernel BUG at lib/list_debug.c:47!
> > > > > invalid opcode: 0000 [#1] SMP NOPTI
> > > > > CPU: 29 PID: 3914 Comm: qemu-kvm Kdump: loaded Tainted: G E
> > > > > -------- - -4.18.0-193.6.3.el8.x86_64 #1
> > > > > Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> > > > > BIOS -[IVE636Z-2.13]- 07/18/2019
> > > > > RIP: 0010:__list_del_entry_valid.cold.1+0x12/0x4c
> > > > > Code: ce ff 0f 0b 48 89 c1 4c 89 c6 48 c7 c7 40 85 4d 88 e8 8c bc
> > > > > ce ff 0f 0b 48 89 fe 48 89 c2 48 c7 c7 d0 85 4d 88 e8 78 bc
> > > > > ce ff <0f> 0b 48 c7 c7 80 86 4d 88 e8 6a bc ce ff 0f 0b 48
> > > > > 89 f2 48 89 fe
> > > > > RSP: 0018:ffffaa9d60197d20 EFLAGS: 00010246
> > > > > RAX: 000000000000004e RBX: ffff8f7fb8ba0828 RCX: 0000000000000000
> > > > > RDX: 0000000000000000 RSI: ffff8f7fbf4d6a08 RDI: ffff8f7fbf4d6a08
> > > > > RBP: 0000000000000000 R08: 000000000000084b R09: 000000000000005d
> > > > > R10: 0000000000000000 R11: ffffaa9d60197bd0 R12: ffff8f4fbe863000
> > > > > R13: 00000000000000c2 R14: 0000000000000000 R15: 0000000000000000
> > > > > FS: 00007f7cb97fa700(0000) GS:ffff8f7fbf4c0000(0000)
> > > > > knlGS:0000000000000000
> > > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > CR2: 00007fcf31da4000 CR3: 0000005f6d404001 CR4: 00000000007626e0
> > > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > > PKRU: 55555554
> > > > > Call Trace:
> > > > > irq_bypass_unregister_producer+0x9b/0xf0 [irqbypass]
> > > > > vfio_msi_set_vector_signal+0x8c/0x290 [vfio_pci]
> > > > > ? load_fixmap_gdt+0x22/0x30
> > > > > vfio_msi_set_block+0x6e/0xd0 [vfio_pci]
> > > > > vfio_pci_ioctl+0x218/0xbe0 [vfio_pci]
> > > > > ? kvm_vcpu_ioctl+0xf2/0x5f0 [kvm]
> > > > > do_vfs_ioctl+0xa4/0x630
> > > > > ? syscall_trace_enter+0x1d3/0x2c0
> > > > > ksys_ioctl+0x60/0x90
> > > > > __x64_sys_ioctl+0x16/0x20
> > > > > do_syscall_64+0x5b/0x1a0
> > > > > entry_SYSCALL_64_after_hwframe+0x65/0xca
> > > > >
> > > > > Cc: Alex Williamson <alex.williamson@redhat.com>
> > > > > Cc: Cornelia Huck <cohuck@redhat.com>
> > > > > Cc: Jiang Yi <giangyi@amazon.com>
> > > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > > Cc: Peter Xu <peterx@redhat.com>
> > > > > Cc: Eric Auger <eric.auger@redhat.com>
> > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > > Cc: kvm@vger.kernel.org
> > > > > Cc: linux-kernel@vger.kernel.org
> > > > > Signed-off-by: guomin chen <guomin_chen@sina.com>
> > > > > ---
> > > > > drivers/vfio/pci/vfio_pci_intrs.c | 13 +++++++++++--
> > > > > drivers/vhost/vdpa.c | 7 +++++++
> > > > > 2 files changed, 18 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> > > > > index 1d9fb25..c371943 100644
> > > > > --- a/drivers/vfio/pci/vfio_pci_intrs.c
> > > > > +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> > > > > @@ -352,12 +352,21 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
> > > > > vdev->ctx[vector].producer.token = trigger;
> > > > > vdev->ctx[vector].producer.irq = irq;
> > > > > ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> > > > > - if (unlikely(ret))
> > > > > + if (unlikely(ret)) {
> > > > > dev_info(&pdev->dev,
> > > > > "irq bypass producer (token %p) registration fails: %d\n",
> > > > > vdev->ctx[vector].producer.token, ret);
> > > > >
> > > > > - vdev->ctx[vector].trigger = trigger;
> > > > > + kfree(vdev->ctx[vector].name);
> > > > > + eventfd_ctx_put(trigger);
> > > > > +
> > > > > + cmd = vfio_pci_memory_lock_and_enable(vdev);
> > > > > + free_irq(irq, trigger);
> > > > > + vfio_pci_memory_unlock_and_restore(vdev, cmd);
> > > > > +
> > > > > + vdev->ctx[vector].trigger = NULL;
> > > > > + } else
> > > > > + vdev->ctx[vector].trigger = trigger;
> > > > >
> > > > > return 0;
> > > > > }
> > > >
> > > > Once again, the irq bypass registration cannot cause the vector setup
> > > > to fail, either by returning an error code or failing to configure the
> > > > vector while returning success. It's my assertion that we simply need
> > > > to set the producer.token to NULL on failure such that unregistering
> > > > the producer will not generate a match, as you've done below. The
> > > > vector still works even if this registration fails.
> > > >
> > > Yes, the irq bypass registration cannot cause the vector setup to fail.
> > > But if I simply set producer.token to NULL when fails, instead of
> > > cleaning up vector, it will trigger the following BUG:
> > >
> > > vfio_ecap_init: 0000:db:00.0 hiding ecap 0x1e@0x310
> > > vfio-pci 0000:db:00.0: irq bypass producer (token 000000004409229f)
> > > registration fails: -16
> > > ------------[ cut here ]------------
> > > kernel BUG at drivers/pci/msi.c:352!
> > > invalid opcode: 0000 [#1] SMP NOPTI
> > > CPU: 55 PID: 9389 Comm: qemu-kvm Kdump: loaded Tainted: G
> > > E --------- - - 4.18.0-193.irqb.r1.el8.x86_64 #1
> > > Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> > > BIOS -[IVE636Z-2.13]- 07/18/2019
> > > RIP: 0010:free_msi_irqs+0x180/0x1b0
> > > Code: 14 85 c0 0f 84 d5 fe ff ff 31 ed eb 0f 83 c5 01 39 6b 14 0f 86
> > > c5 fe ff ff 8b 7b 10 01 ef e8 d7 4a c9 ff 48 83 78 70 00 74 e3
> > > <0f> 0b 49 8d b5 b0 00 00 00 e8 e2 e3 c9 ff e9 c7 fe ff ff 48
> > > 8b 7b
> > > RSP: 0018:ffffaeca4f4bfcd8 EFLAGS: 00010286
> > > RAX: ffff8bec77441600 RBX: ffff8bbcdb637e40 RCX: 0000000000000000
> > > RDX: 0000000000000000 RSI: 00000000000001ab RDI: ffffffff8ea5b2a0
> > > RBP: 0000000000000000 R08: ffff8bec7e746828 R09: ffff8bec7e7466a8
> > > R10: 0000000000000000 R11: 0000000000000000 R12: ffff8bbcde921308
> > > R13: ffff8bbcde921000 R14: 000000000000000b R15: 0000000000000021
> > > FS: 00007fd18d7fa700(0000) GS:ffff8bec7f6c0000(0000) knlGS:0000000000000000
> > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 00007f83650024a0 CR3: 000000476e70c001 CR4: 00000000007626e0
> > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > PKRU: 55555554
> > > Call Trace:
> > > pci_disable_msix+0xf3/0x120
> > > pci_free_irq_vectors+0xe/0x20
> > > vfio_msi_disable+0x89/0xd0 [vfio_pci]
> > > vfio_pci_set_msi_trigger+0x229/0x2d0 [vfio_pci]
> > > vfio_pci_ioctl+0x24f/0xdb0 [vfio_pci]
> > > ? pollwake+0x74/0x90
> > > ? wake_up_q+0x70/0x70
> > > do_vfs_ioctl+0xa4/0x630
> > > ? __alloc_fd+0x33/0x140
> > > ? syscall_trace_enter+0x1d3/0x2c0
> > > ksys_ioctl+0x60/0x90
> > > __x64_sys_ioctl+0x16/0x20
> > > do_syscall_64+0x5b/0x1a0
> > > entry_SYSCALL_64_after_hwframe+0x65/0xca
> >
> > Please post the patch that triggers this, I'm not yet convinced we're
> > speaking of the same solution. The user ioctl cannot fail due to the
> > failure to setup a bypass accelerator, nor can the ioctl return success
> > without configuring all of the user requested vectors, which is what I
> > understand the v2 patch above to do. We simply want to configure the
> > failed producer such that when we unregister it at user request, we
> > avoid creating a bogus match. It's not apparent to me why doing that
> > would cause any changes to the setup or teardown of the MSI vector in
> > PCI code. Thanks,
> >
> > Alex
> >
> Hi Alex, as you said before, I only need to set the producer.token
> to NULL on failure such that unregistering the producer will not
> generate a match.
>
> So I wrote a patch (As you said patch v2), as follows:
>
> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c
> b/drivers/vfio/pci/vfio_pci_intrs.c
> index 1d9fb25..1969cd0 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -352,12 +352,15 @@ static int vfio_msi_set_vector_signal(struct
> vfio_pci_device *vdev,
> vdev->ctx[vector].producer.token = trigger;
> vdev->ctx[vector].producer.irq = irq;
> ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> - if (unlikely(ret))
> + if (unlikely(ret)) {
> dev_info(&pdev->dev,
> "irq bypass producer (token %p) registration fails: %d\n",
> vdev->ctx[vector].producer.token, ret);
>
> - vdev->ctx[vector].trigger = trigger;
> + eventfd_ctx_put(trigger);
> + vdev->ctx[vector].trigger = NULL;
> + } else
> + vdev->ctx[vector].trigger = trigger;
>
> return 0;
> }
How does this remotely match "only need to set the producer.token to
NULL on failure"? What I'm suggesting is:
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -352,10 +352,12 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
vdev->ctx[vector].producer.token = trigger;
vdev->ctx[vector].producer.irq = irq;
ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
- if (unlikely(ret))
+ if (unlikely(ret)) {
dev_info(&pdev->dev,
"irq bypass producer (token %p) registration fails: %d\n",
vdev->ctx[vector].producer.token, ret);
+ vdev->ctx[vector].producer.token = NULL;
+ }
vdev->ctx[vector].trigger = trigger;
This is exactly what you proposed for vhost/vdpa.c, so I don't see why
you're playing with the trigger context, which will clearly cause
problems. Thanks,
Alex
> --
>
> However, when I use this patch to testing, the following bugs are
> triggered when vfio_msi_disable() called because the msi vector
> is not cleaned up:
>
> vfio_ecap_init: 0000:db:00.0 hiding ecap 0x1e@0x310
> vfio-pci 0000:db:00.0: irq bypass producer (token 000000004409229f)
> registration fails: -16
> ------------[ cut here ]------------
> kernel BUG at drivers/pci/msi.c:352!
> invalid opcode: 0000 [#1] SMP NOPTI
> CPU: 55 PID: 9389 Comm: qemu-kvm Kdump: loaded Tainted: G
> E --------- - - 4.18.0-193.irqb.r1.el8.x86_64 #1
> Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> BIOS -[IVE636Z-2.13]- 07/18/2019
> RIP: 0010:free_msi_irqs+0x180/0x1b0
> Code: 14 85 c0 0f 84 d5 fe ff ff 31 ed eb 0f 83 c5 01 39 6b 14 0f 86
> c5 fe ff ff 8b 7b 10 01 ef e8 d7 4a c9 ff 48 83 78 70 00 74 e3
> <0f> 0b 49 8d b5 b0 00 00 00 e8 e2 e3 c9 ff e9 c7 fe ff ff 48
> 8b 7b
> RSP: 0018:ffffaeca4f4bfcd8 EFLAGS: 00010286
> RAX: ffff8bec77441600 RBX: ffff8bbcdb637e40 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: 00000000000001ab RDI: ffffffff8ea5b2a0
> RBP: 0000000000000000 R08: ffff8bec7e746828 R09: ffff8bec7e7466a8
> R10: 0000000000000000 R11: 0000000000000000 R12: ffff8bbcde921308
> R13: ffff8bbcde921000 R14: 000000000000000b R15: 0000000000000021
> FS: 00007fd18d7fa700(0000) GS:ffff8bec7f6c0000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f83650024a0 CR3: 000000476e70c001 CR4: 00000000007626e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
> Call Trace:
> pci_disable_msix+0xf3/0x120
> pci_free_irq_vectors+0xe/0x20
> vfio_msi_disable+0x89/0xd0 [vfio_pci]
> vfio_pci_set_msi_trigger+0x229/0x2d0 [vfio_pci]
> vfio_pci_ioctl+0x24f/0xdb0 [vfio_pci]
> ? pollwake+0x74/0x90
> ? wake_up_q+0x70/0x70
> do_vfs_ioctl+0xa4/0x630
> ? __alloc_fd+0x33/0x140
> ? syscall_trace_enter+0x1d3/0x2c0
> ksys_ioctl+0x60/0x90
> __x64_sys_ioctl+0x16/0x20
> do_syscall_64+0x5b/0x1a0
> entry_SYSCALL_64_after_hwframe+0x65/0xca
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] irqbypass: fix error handle when irq_bypass_register_producer() return fails
2020-10-10 14:26 ` Alex Williamson
@ 2020-10-16 13:38 ` gchen chen
0 siblings, 0 replies; 6+ messages in thread
From: gchen chen @ 2020-10-16 13:38 UTC (permalink / raw)
To: Alex Williamson
Cc: guomin_chen, Cornelia Huck, Jiang Yi, Marc Zyngier, Peter Xu,
Eric Auger, Michael S. Tsirkin, Jason Wang, kvm, linux-kernel
Hi Alex
According to your suggestion, the final patch is as follows:
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c
b/drivers/vfio/pci/vfio_pci_intrs.c
index 1d9fb25..869dce5 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -352,11 +352,13 @@ static int vfio_msi_set_vector_signal(struct
vfio_pci_device *vdev,
vdev->ctx[vector].producer.token = trigger;
vdev->ctx[vector].producer.irq = irq;
ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
- if (unlikely(ret))
+ if (unlikely(ret)) {
dev_info(&pdev->dev,
"irq bypass producer (token %p) registration fails: %d\n",
vdev->ctx[vector].producer.token, ret);
+ vdev->ctx[vector].producer.token = NULL;
+ }
vdev->ctx[vector].trigger = trigger;
return 0;
--
I applied this patch, after several days of testing, it is now normal
and the BUG no longer occurs.
Now the kernel will only print the log of registration failure,
Thanks Alex.
Alex Williamson <alex.williamson@redhat.com> 于2020年10月10日周六 下午10:26写道：
>
> On Sat, 10 Oct 2020 19:01:30 +0800
> gchen chen <gchen.guomin@gmail.com> wrote:
>
> > Alex Williamson <alex.williamson@redhat.com> 于2020年10月10日周六 上午2:44写道：
> > >
> > > On Fri, 9 Oct 2020 12:30:04 +0800
> > > gchen chen <gchen.guomin@gmail.com> wrote:
> > >
> > > > Alex Williamson <alex.williamson@redhat.com> 于2020年9月30日周三 下午10:09写道：
> > > > >
> > > > >
> > > > > Please version your postings so we know which one to consider as the
> > > > > current proposal.
> > > > >
> > > > > On Wed, 30 Sep 2020 20:54:39 +0800
> > > > > guomin_chen@sina.com wrote:
> > > > >
> > > > > > From: guomin chen <guomin_chen@sina.com>
> > > > > >
> > > > > > When the producer object registration fails,In the future, due to
> > > > > > incorrect matching when unregistering, list_del(&producer->node)
> > > > > > may still be called, then trigger a BUG:
> > > > > >
> > > > > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > > > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > > > > vfio-pci 0000:db:00.0: irq bypass producer (token 0000000060c8cda5) registration fails: -16
> > > > > > ...
> > > > > > list_del corruption, ffff8f7fb8ba0828->next is LIST_POISON1 (dead000000000100)
> > > > > > ------------[ cut here ]------------
> > > > > > kernel BUG at lib/list_debug.c:47!
> > > > > > invalid opcode: 0000 [#1] SMP NOPTI
> > > > > > CPU: 29 PID: 3914 Comm: qemu-kvm Kdump: loaded Tainted: G E
> > > > > > -------- - -4.18.0-193.6.3.el8.x86_64 #1
> > > > > > Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> > > > > > BIOS -[IVE636Z-2.13]- 07/18/2019
> > > > > > RIP: 0010:__list_del_entry_valid.cold.1+0x12/0x4c
> > > > > > Code: ce ff 0f 0b 48 89 c1 4c 89 c6 48 c7 c7 40 85 4d 88 e8 8c bc
> > > > > > ce ff 0f 0b 48 89 fe 48 89 c2 48 c7 c7 d0 85 4d 88 e8 78 bc
> > > > > > ce ff <0f> 0b 48 c7 c7 80 86 4d 88 e8 6a bc ce ff 0f 0b 48
> > > > > > 89 f2 48 89 fe
> > > > > > RSP: 0018:ffffaa9d60197d20 EFLAGS: 00010246
> > > > > > RAX: 000000000000004e RBX: ffff8f7fb8ba0828 RCX: 0000000000000000
> > > > > > RDX: 0000000000000000 RSI: ffff8f7fbf4d6a08 RDI: ffff8f7fbf4d6a08
> > > > > > RBP: 0000000000000000 R08: 000000000000084b R09: 000000000000005d
> > > > > > R10: 0000000000000000 R11: ffffaa9d60197bd0 R12: ffff8f4fbe863000
> > > > > > R13: 00000000000000c2 R14: 0000000000000000 R15: 0000000000000000
> > > > > > FS: 00007f7cb97fa700(0000) GS:ffff8f7fbf4c0000(0000)
> > > > > > knlGS:0000000000000000
> > > > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > > CR2: 00007fcf31da4000 CR3: 0000005f6d404001 CR4: 00000000007626e0
> > > > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > > > PKRU: 55555554
> > > > > > Call Trace:
> > > > > > irq_bypass_unregister_producer+0x9b/0xf0 [irqbypass]
> > > > > > vfio_msi_set_vector_signal+0x8c/0x290 [vfio_pci]
> > > > > > ? load_fixmap_gdt+0x22/0x30
> > > > > > vfio_msi_set_block+0x6e/0xd0 [vfio_pci]
> > > > > > vfio_pci_ioctl+0x218/0xbe0 [vfio_pci]
> > > > > > ? kvm_vcpu_ioctl+0xf2/0x5f0 [kvm]
> > > > > > do_vfs_ioctl+0xa4/0x630
> > > > > > ? syscall_trace_enter+0x1d3/0x2c0
> > > > > > ksys_ioctl+0x60/0x90
> > > > > > __x64_sys_ioctl+0x16/0x20
> > > > > > do_syscall_64+0x5b/0x1a0
> > > > > > entry_SYSCALL_64_after_hwframe+0x65/0xca
> > > > > >
> > > > > > Cc: Alex Williamson <alex.williamson@redhat.com>
> > > > > > Cc: Cornelia Huck <cohuck@redhat.com>
> > > > > > Cc: Jiang Yi <giangyi@amazon.com>
> > > > > > Cc: Marc Zyngier <maz@kernel.org>
> > > > > > Cc: Peter Xu <peterx@redhat.com>
> > > > > > Cc: Eric Auger <eric.auger@redhat.com>
> > > > > > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > > > > > Cc: Jason Wang <jasowang@redhat.com>
> > > > > > Cc: kvm@vger.kernel.org
> > > > > > Cc: linux-kernel@vger.kernel.org
> > > > > > Signed-off-by: guomin chen <guomin_chen@sina.com>
> > > > > > ---
> > > > > > drivers/vfio/pci/vfio_pci_intrs.c | 13 +++++++++++--
> > > > > > drivers/vhost/vdpa.c | 7 +++++++
> > > > > > 2 files changed, 18 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> > > > > > index 1d9fb25..c371943 100644
> > > > > > --- a/drivers/vfio/pci/vfio_pci_intrs.c
> > > > > > +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> > > > > > @@ -352,12 +352,21 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
> > > > > > vdev->ctx[vector].producer.token = trigger;
> > > > > > vdev->ctx[vector].producer.irq = irq;
> > > > > > ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> > > > > > - if (unlikely(ret))
> > > > > > + if (unlikely(ret)) {
> > > > > > dev_info(&pdev->dev,
> > > > > > "irq bypass producer (token %p) registration fails: %d\n",
> > > > > > vdev->ctx[vector].producer.token, ret);
> > > > > >
> > > > > > - vdev->ctx[vector].trigger = trigger;
> > > > > > + kfree(vdev->ctx[vector].name);
> > > > > > + eventfd_ctx_put(trigger);
> > > > > > +
> > > > > > + cmd = vfio_pci_memory_lock_and_enable(vdev);
> > > > > > + free_irq(irq, trigger);
> > > > > > + vfio_pci_memory_unlock_and_restore(vdev, cmd);
> > > > > > +
> > > > > > + vdev->ctx[vector].trigger = NULL;
> > > > > > + } else
> > > > > > + vdev->ctx[vector].trigger = trigger;
> > > > > >
> > > > > > return 0;
> > > > > > }
> > > > >
> > > > > Once again, the irq bypass registration cannot cause the vector setup
> > > > > to fail, either by returning an error code or failing to configure the
> > > > > vector while returning success. It's my assertion that we simply need
> > > > > to set the producer.token to NULL on failure such that unregistering
> > > > > the producer will not generate a match, as you've done below. The
> > > > > vector still works even if this registration fails.
> > > > >
> > > > Yes, the irq bypass registration cannot cause the vector setup to fail.
> > > > But if I simply set producer.token to NULL when fails, instead of
> > > > cleaning up vector, it will trigger the following BUG:
> > > >
> > > > vfio_ecap_init: 0000:db:00.0 hiding ecap 0x1e@0x310
> > > > vfio-pci 0000:db:00.0: irq bypass producer (token 000000004409229f)
> > > > registration fails: -16
> > > > ------------[ cut here ]------------
> > > > kernel BUG at drivers/pci/msi.c:352!
> > > > invalid opcode: 0000 [#1] SMP NOPTI
> > > > CPU: 55 PID: 9389 Comm: qemu-kvm Kdump: loaded Tainted: G
> > > > E --------- - - 4.18.0-193.irqb.r1.el8.x86_64 #1
> > > > Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> > > > BIOS -[IVE636Z-2.13]- 07/18/2019
> > > > RIP: 0010:free_msi_irqs+0x180/0x1b0
> > > > Code: 14 85 c0 0f 84 d5 fe ff ff 31 ed eb 0f 83 c5 01 39 6b 14 0f 86
> > > > c5 fe ff ff 8b 7b 10 01 ef e8 d7 4a c9 ff 48 83 78 70 00 74 e3
> > > > <0f> 0b 49 8d b5 b0 00 00 00 e8 e2 e3 c9 ff e9 c7 fe ff ff 48
> > > > 8b 7b
> > > > RSP: 0018:ffffaeca4f4bfcd8 EFLAGS: 00010286
> > > > RAX: ffff8bec77441600 RBX: ffff8bbcdb637e40 RCX: 0000000000000000
> > > > RDX: 0000000000000000 RSI: 00000000000001ab RDI: ffffffff8ea5b2a0
> > > > RBP: 0000000000000000 R08: ffff8bec7e746828 R09: ffff8bec7e7466a8
> > > > R10: 0000000000000000 R11: 0000000000000000 R12: ffff8bbcde921308
> > > > R13: ffff8bbcde921000 R14: 000000000000000b R15: 0000000000000021
> > > > FS: 00007fd18d7fa700(0000) GS:ffff8bec7f6c0000(0000) knlGS:0000000000000000
> > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: 00007f83650024a0 CR3: 000000476e70c001 CR4: 00000000007626e0
> > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > PKRU: 55555554
> > > > Call Trace:
> > > > pci_disable_msix+0xf3/0x120
> > > > pci_free_irq_vectors+0xe/0x20
> > > > vfio_msi_disable+0x89/0xd0 [vfio_pci]
> > > > vfio_pci_set_msi_trigger+0x229/0x2d0 [vfio_pci]
> > > > vfio_pci_ioctl+0x24f/0xdb0 [vfio_pci]
> > > > ? pollwake+0x74/0x90
> > > > ? wake_up_q+0x70/0x70
> > > > do_vfs_ioctl+0xa4/0x630
> > > > ? __alloc_fd+0x33/0x140
> > > > ? syscall_trace_enter+0x1d3/0x2c0
> > > > ksys_ioctl+0x60/0x90
> > > > __x64_sys_ioctl+0x16/0x20
> > > > do_syscall_64+0x5b/0x1a0
> > > > entry_SYSCALL_64_after_hwframe+0x65/0xca
> > >
> > > Please post the patch that triggers this, I'm not yet convinced we're
> > > speaking of the same solution. The user ioctl cannot fail due to the
> > > failure to setup a bypass accelerator, nor can the ioctl return success
> > > without configuring all of the user requested vectors, which is what I
> > > understand the v2 patch above to do. We simply want to configure the
> > > failed producer such that when we unregister it at user request, we
> > > avoid creating a bogus match. It's not apparent to me why doing that
> > > would cause any changes to the setup or teardown of the MSI vector in
> > > PCI code. Thanks,
> > >
> > > Alex
> > >
> > Hi Alex, as you said before, I only need to set the producer.token
> > to NULL on failure such that unregistering the producer will not
> > generate a match.
> >
> > So I wrote a patch (As you said patch v2), as follows:
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_intrs.c
> > b/drivers/vfio/pci/vfio_pci_intrs.c
> > index 1d9fb25..1969cd0 100644
> > --- a/drivers/vfio/pci/vfio_pci_intrs.c
> > +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> > @@ -352,12 +352,15 @@ static int vfio_msi_set_vector_signal(struct
> > vfio_pci_device *vdev,
> > vdev->ctx[vector].producer.token = trigger;
> > vdev->ctx[vector].producer.irq = irq;
> > ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> > - if (unlikely(ret))
> > + if (unlikely(ret)) {
> > dev_info(&pdev->dev,
> > "irq bypass producer (token %p) registration fails: %d\n",
> > vdev->ctx[vector].producer.token, ret);
> >
> > - vdev->ctx[vector].trigger = trigger;
> > + eventfd_ctx_put(trigger);
> > + vdev->ctx[vector].trigger = NULL;
> > + } else
> > + vdev->ctx[vector].trigger = trigger;
> >
> > return 0;
> > }
>
> How does this remotely match "only need to set the producer.token to
> NULL on failure"? What I'm suggesting is:
>
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c
> @@ -352,10 +352,12 @@ static int vfio_msi_set_vector_signal(struct vfio_pci_device *vdev,
> vdev->ctx[vector].producer.token = trigger;
> vdev->ctx[vector].producer.irq = irq;
> ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
> - if (unlikely(ret))
> + if (unlikely(ret)) {
> dev_info(&pdev->dev,
> "irq bypass producer (token %p) registration fails: %d\n",
> vdev->ctx[vector].producer.token, ret);
> + vdev->ctx[vector].producer.token = NULL;
> + }
>
> vdev->ctx[vector].trigger = trigger;
>
> This is exactly what you proposed for vhost/vdpa.c, so I don't see why
> you're playing with the trigger context, which will clearly cause
> problems. Thanks,
>
> Alex
>
> > --
> >
> > However, when I use this patch to testing, the following bugs are
> > triggered when vfio_msi_disable() called because the msi vector
> > is not cleaned up:
> >
> > vfio_ecap_init: 0000:db:00.0 hiding ecap 0x1e@0x310
> > vfio-pci 0000:db:00.0: irq bypass producer (token 000000004409229f)
> > registration fails: -16
> > ------------[ cut here ]------------
> > kernel BUG at drivers/pci/msi.c:352!
> > invalid opcode: 0000 [#1] SMP NOPTI
> > CPU: 55 PID: 9389 Comm: qemu-kvm Kdump: loaded Tainted: G
> > E --------- - - 4.18.0-193.irqb.r1.el8.x86_64 #1
> > Hardware name: Lenovo ThinkSystem SR650 -[7X06CTO1WW]-/-[7X06CTO1WW]-,
> > BIOS -[IVE636Z-2.13]- 07/18/2019
> > RIP: 0010:free_msi_irqs+0x180/0x1b0
> > Code: 14 85 c0 0f 84 d5 fe ff ff 31 ed eb 0f 83 c5 01 39 6b 14 0f 86
> > c5 fe ff ff 8b 7b 10 01 ef e8 d7 4a c9 ff 48 83 78 70 00 74 e3
> > <0f> 0b 49 8d b5 b0 00 00 00 e8 e2 e3 c9 ff e9 c7 fe ff ff 48
> > 8b 7b
> > RSP: 0018:ffffaeca4f4bfcd8 EFLAGS: 00010286
> > RAX: ffff8bec77441600 RBX: ffff8bbcdb637e40 RCX: 0000000000000000
> > RDX: 0000000000000000 RSI: 00000000000001ab RDI: ffffffff8ea5b2a0
> > RBP: 0000000000000000 R08: ffff8bec7e746828 R09: ffff8bec7e7466a8
> > R10: 0000000000000000 R11: 0000000000000000 R12: ffff8bbcde921308
> > R13: ffff8bbcde921000 R14: 000000000000000b R15: 0000000000000021
> > FS: 00007fd18d7fa700(0000) GS:ffff8bec7f6c0000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007f83650024a0 CR3: 000000476e70c001 CR4: 00000000007626e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > PKRU: 55555554
> > Call Trace:
> > pci_disable_msix+0xf3/0x120
> > pci_free_irq_vectors+0xe/0x20
> > vfio_msi_disable+0x89/0xd0 [vfio_pci]
> > vfio_pci_set_msi_trigger+0x229/0x2d0 [vfio_pci]
> > vfio_pci_ioctl+0x24f/0xdb0 [vfio_pci]
> > ? pollwake+0x74/0x90
> > ? wake_up_q+0x70/0x70
> > do_vfs_ioctl+0xa4/0x630
> > ? __alloc_fd+0x33/0x140
> > ? syscall_trace_enter+0x1d3/0x2c0
> > ksys_ioctl+0x60/0x90
> > __x64_sys_ioctl+0x16/0x20
> > do_syscall_64+0x5b/0x1a0
> > entry_SYSCALL_64_after_hwframe+0x65/0xca
> >
>
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-16 13:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1601470479-26848-1-git-send-email-guomin_chen@sina.com>
2020-09-30 14:09 ` [PATCH] irqbypass: fix error handle when irq_bypass_register_producer() return fails Alex Williamson
2020-10-09 4:30 ` gchen chen
2020-10-09 18:44 ` Alex Williamson
2020-10-10 11:01 ` gchen chen
2020-10-10 14:26 ` Alex Williamson
2020-10-16 13:38 ` gchen chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.