On Wed, Mar 9, 2022 at 12:36 AM Michael S. Tsirkin <[email protected]> wrote:
> On Tue, Mar 08, 2022 at 03:19:16PM +0000, Marc Zyngier wrote: > > On Tue, 19 Oct 2021 08:01:46 +0100, > > Jason Wang <[email protected]> wrote: > > > > > > We used to synchronize pending MSI-X irq handlers via > > > synchronize_irq(), this may not work for the untrusted device which > > > may keep sending interrupts after reset which may lead unexpected > > > results. Similarly, we should not enable MSI-X interrupt until the > > > device is ready. So this patch fixes those two issues by: > > > > > > 1) switching to use disable_irq() to prevent the virtio interrupt > > > handlers to be called after the device is reset. > > > 2) using IRQF_NO_AUTOEN and enable the MSI-X irq during .ready() > > > > > > This can make sure the virtio interrupt handler won't be called before > > > virtio_device_ready() and after reset. > > > > > > Cc: Thomas Gleixner <[email protected]> > > > Cc: Peter Zijlstra <[email protected]> > > > Cc: Paul E. McKenney <[email protected]> > > > Signed-off-by: Jason Wang <[email protected]> > > > --- > > > drivers/virtio/virtio_pci_common.c | 27 +++++++++++++++++++++------ > > > drivers/virtio/virtio_pci_common.h | 6 ++++-- > > > drivers/virtio/virtio_pci_legacy.c | 5 +++-- > > > drivers/virtio/virtio_pci_modern.c | 6 ++++-- > > > 4 files changed, 32 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/virtio/virtio_pci_common.c > b/drivers/virtio/virtio_pci_common.c > > > index b35bb2d57f62..8d8f83aca721 100644 > > > --- a/drivers/virtio/virtio_pci_common.c > > > +++ b/drivers/virtio/virtio_pci_common.c > > > @@ -24,8 +24,8 @@ MODULE_PARM_DESC(force_legacy, > > > "Force legacy mode for transitional virtio 1 devices"); > > > #endif > > > > > > -/* wait for pending irq handlers */ > > > -void vp_synchronize_vectors(struct virtio_device *vdev) > > > +/* disable irq handlers */ > > > +void vp_disable_cbs(struct virtio_device *vdev) > > > { > > > struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > > int i; > > > @@ -34,7 +34,20 @@ void vp_synchronize_vectors(struct virtio_device > *vdev) > > > synchronize_irq(vp_dev->pci_dev->irq); > > > > > > for (i = 0; i < vp_dev->msix_vectors; ++i) > > > - synchronize_irq(pci_irq_vector(vp_dev->pci_dev, i)); > > > + disable_irq(pci_irq_vector(vp_dev->pci_dev, i)); > > > +} > > > + > > > +/* enable irq handlers */ > > > +void vp_enable_cbs(struct virtio_device *vdev) > > > +{ > > > + struct virtio_pci_device *vp_dev = to_vp_device(vdev); > > > + int i; > > > + > > > + if (vp_dev->intx_enabled) > > > + return; > > > + > > > + for (i = 0; i < vp_dev->msix_vectors; ++i) > > > + enable_irq(pci_irq_vector(vp_dev->pci_dev, i)); > > > > This results in a splat at boot time if you set maxcpus=<whatever>, > > see below. Enabling interrupts that are affinity managed is *bad*. You > > don't even know whether the CPU which is supposed to handle this is > > online or not. > > > > The core kernel notices it, shouts and keeps the interrupt disabled, > > but this should be fixed. The whole point of managed interrupts is to > > let them be dealt with outside of the drivers, and tied into the CPUs > > being brought up and down. If virtio needs (for one reason or another) > > to manage interrupts on its own, so be it. But this patch isn't the > > way to do it, I'm afraid. > > > > M. > > Thanks for reporting this! > > What virtio is doing here isn't unique however. > > If device driver is to be hardened against device sending interrupts > while driver is initializing/cleaning up, it needs kernel to provide > capability to disable these. > > If we then declare that that is impossible for managed interrupts > then that will mean most devices can't use managed interrupts > because ideally we'd have all drivers hardened. > Or that probably means we need to use a driver specific mechanism as what we did for INTX instead of using NO_AUTO_EN. Thanks > > Thomas I think you were the one who suggested enabling/disabling > interrupts originally - thoughts? > > Feedback appreciated. > > > > > [ 3.434849] ------------[ cut here ]------------ > > [ 3.434850] WARNING: CPU: 0 PID: 93 at kernel/irq/chip.c:210 > irq_startup+0x10 > > e/0x120 > > [ 3.434861] Modules linked in: virtio_net(E+) net_failover(E) > failover(E) vir > > tio_blk(E+) bochs(E+) drm_vram_helper(E) drm_ttm_helper(E) ttm(E) > ahci(E+) libah > > ci(E) virtio_pci(E) virtio_pci_legacy_dev(E) virtio_pci_modern_dev(E) > virtio(E) > > drm_kms_helper(E) cec(E) libata(E) crct10dif_pclmul(E) > crct10dif_common(E) crc32 > > _pclmul(E) scsi_mod(E) i2c_i801(E) crc32c_intel(E) psmouse(E) > i2c_smbus(E) scsi_ > > common(E) lpc_ich(E) virtio_ring(E) drm(E) button(E) > > [ 3.434890] CPU: 0 PID: 93 Comm: systemd-udevd Tainted: G > E 5. > > 17.0-rc7-00020-gea4424be1688 #63 > > [ 3.434893] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS > 0.0.0 02 > > /06/2015 > > [ 3.434897] RIP: 0010:irq_startup+0x10e/0x120 > > [ 3.434904] Code: c0 75 2b 4c 89 e7 31 d2 4c 89 ee e8 dc c5 ff ff 48 > 89 ef e8 > > 94 fe ff ff 41 89 c4 e9 33 ff ff ff e8 e7 ca ff ff e9 50 ff ff ff <0f> > 0b eb ac > > 0f 0b eb a8 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 > > [ 3.434906] RSP: 0018:ffff972c402bbbf0 EFLAGS: 00010002 > > [ 3.434908] RAX: 0000000000000004 RBX: 0000000000000001 RCX: > 0000000000000040 > > [ 3.434912] RDX: 0000000000000000 RSI: ffffffffa768dee0 RDI: > ffff8bcf8ce34648 > > [ 3.434913] RBP: ffff8bcfb007a800 R08: 0000000000000004 R09: > ffffffffa74cb828 > > [ 3.434915] R10: 0000000000000000 R11: 0000000000000000 R12: > 0000000000000001 > > [ 3.434916] R13: ffff8bcf8ce34648 R14: ffff8bcf8d185c70 R15: > 0000000000000200 > > [ 3.434918] FS: 00007f5b3179f8c0(0000) GS:ffff8bcffbc00000(0000) > knlGS:00000 > > 00000000000 > > [ 3.434919] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > [ 3.434921] CR2: 000055ca47bab6b8 CR3: 000000017bc40003 CR4: > 0000000000170ef0 > > [ 3.434928] Call Trace: > > [ 3.434936] <TASK> > > [ 3.434938] enable_irq+0x48/0x90 > > [ 3.434943] vp_enable_cbs+0x36/0x70 [virtio_pci] > > [ 3.434948] virtblk_probe+0x457/0x7dc [virtio_blk] > > [ 3.434954] virtio_dev_probe+0x1ae/0x280 [virtio] > > [ 3.434959] really_probe+0x1f5/0x3d0 > > [ 3.434966] __driver_probe_device+0xfe/0x180 > > [ 3.434969] driver_probe_device+0x1e/0x90 > > [ 3.434971] __driver_attach+0xc0/0x1c0 > > [ 3.434974] ? __device_attach_driver+0xe0/0xe0 > > [ 3.434976] ? __device_attach_driver+0xe0/0xe0 > > [ 3.434978] bus_for_each_dev+0x78/0xc0 > > [ 3.434982] bus_add_driver+0x149/0x1e0 > > [ 3.434985] driver_register+0x8b/0xe0 > > [ 3.434987] ? 0xffffffffc01aa000 > > [ 3.434990] init+0x52/0x1000 [virtio_blk] > > [ 3.434994] do_one_initcall+0x44/0x200 > > [ 3.435001] ? kmem_cache_alloc_trace+0x300/0x400 > > [ 3.435006] do_init_module+0x4c/0x260 > > [ 3.435013] __do_sys_finit_module+0xb4/0x120 > > [ 3.435018] do_syscall_64+0x3b/0xc0 > > [ 3.435027] entry_SYSCALL_64_after_hwframe+0x44/0xae > > [ 3.435037] RIP: 0033:0x7f5b31c589b9 > > [ 3.435040] Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 > 48 89 f8 > > 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> > 3d 01 f0 > > ff ff 73 01 c3 48 8b 0d a7 54 0c 00 f7 d8 64 89 01 48 > > [ 3.435042] RSP: 002b:00007ffc608fc198 EFLAGS: 00000246 ORIG_RAX: > 00000000000 > > 00139 > > [ 3.435045] RAX: ffffffffffffffda RBX: 000055ca47ba8700 RCX: > 00007f5b31c589b9 > > [ 3.435046] RDX: 0000000000000000 RSI: 00007f5b31de3e2d RDI: > 0000000000000005 > > [ 3.435048] RBP: 0000000000020000 R08: 0000000000000000 R09: > 000055ca47ba9030 > > [ 3.435049] R10: 0000000000000005 R11: 0000000000000246 R12: > 00007f5b31de3e2d > > [ 3.435050] R13: 0000000000000000 R14: 000055ca47ba7060 R15: > 000055ca47ba8700 > > [ 3.435053] </TASK> > > [ 3.435059] ---[ end trace 0000000000000000 ]--- > > [ 3.440593] vda: vda1 vda2 vda3 > > [ 3.445283] scsi host0: Virtio SCSI HBA > > [ 3.450373] scsi 0:0:0:0: CD-ROM QEMU QEMU CD-ROM > 2.5+ PQ > > : 0 ANSI: 5 > > > > -- > > Without deviation from the norm, progress is not possible. > >
_______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
