| Message ID | 72609f999d9451c13240acb8e4a4e54f8c62f542.camel@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hvc/xen: fix event channel handling for secondary consoles | expand |
On 18.10.23 01:46, David Woodhouse wrote: > From: David Woodhouse <dwmw@amazon.co.uk> > > The xencons_connect_backend() function allocates a local interdomain > event channel with xenbus_alloc_evtchn(), then calls > bind_interdomain_evtchn_to_irq_lateeoi() to bind to that port# on the > *remote* domain. > > That doesn't work very well: > > (qemu) device_add xen-console,id=con1,chardev=pty0 > [ 44.323872] xenconsole console-1: 2 xenbus_dev_probe on device/console/1 > [ 44.323995] xenconsole: probe of console-1 failed with error -2 > > Fix it to use bind_evtchn_to_irq_lateeoi(), which does the right thing > by just binding that *local* event channel to an irq. The backend will > do the interdomain binding. > > This didn't affect the primary console because the setup for that is > special — the toolstack allocates the guest event channel and the guest > discovers it with HVMOP_get_param. > > Once that's fixed, there's also a warning on hot-unplug because > xencons_disconnect_backend() unconditionally calls free_irq() via > unbind_from_irqhandler(): > > (qemu) device_del con1 > [ 32.050919] ------------[ cut here ]------------ > [ 32.050942] Trying to free already-free IRQ 33 > [ 32.050990] WARNING: CPU: 0 PID: 51 at kernel/irq/manage.c:1895 __free_irq+0x1d4/0x330 > > Fix that by calling notifier_del_irq() first, which only calls > free_irq() if the irq was requested in the first place. Then use I don't think the "if the irq was requested in the first place" is the correct reasoning. I think the problem is that notifier_del_irq() will be called another time through the .notifier_del hook. Two calls of notifier_del_irq() are fine, but one call of it and another call of free_irq() via unbind_from_irqhandler() is a problem. > evtchn_put() to release the irq and event channel. Avoid calling > xenbus_free_evtchn() in the normal case, as evtchn_put() will do that > too. The only time xenbus_free_evtchn() needs to be called is for the > cleanup when bind_evtchn_to_irq_lateeoi() fails. > > Finally, fix the error path in xen_hvc_init() when there's no primary > console. It should still register the frontend driver, as there may be > secondary consoles. (Qemu can always add secondary consoles, but only > the toolstack can add the primary because it's special.) > > Fixes: fe415186b4 ("xen/console: harden hvc_xen against event channel storms") > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > Cc: stable@vger.kernel.org With above fixed in the commit message: Reviewed-by: Juergen Gross <jgross@suse.com> Juergen
On Fri, 2023-10-20 at 10:51 +0200, Juergen Gross wrote: > > > (qemu) device_del con1 > > [ 32.050919] ------------[ cut here ]------------ > > [ 32.050942] Trying to free already-free IRQ 33 > > [ 32.050990] WARNING: CPU: 0 PID: 51 at kernel/irq/manage.c:1895 __free_irq+0x1d4/0x330 > > > > Fix that by calling notifier_del_irq() first, which only calls > > free_irq() if the irq was requested in the first place. Then use > > I don't think the "if the irq was requested in the first place" is the correct > reasoning. > > I think the problem is that notifier_del_irq() will be called another time > through the .notifier_del hook. Two calls of notifier_del_irq() are fine, but > one call of it and another call of free_irq() via unbind_from_irqhandler() is > a problem. Er... yes, the HVC tty device still exists, can still be open and in use by userspace during the time that xencons_disconnect_backend() is running. Why does xencons_disconnect_backend() do all the evtchn and gnttab teardown *first* and then only call hvc_remove() at the end. That seems weird. So if I do 'dd if=/dev/zero of=/dev/hvc1' while I remove the device from qemu... yep, that seems to have filled the ring after the evtchn was torn down, and it's waiting for ever in domU_write_console(). In fact, that isn't *even* because of the race within xencons_disconnect_backend(). In xencons_backend_changed() when the backend goes into state XenbusStateClos{ing,ed} for the disconnect, we just set the frontend state directly to XenbusStateClosed without even *calling* xencons_disconnect_backend(). So we were *told* of the impending unplug. We fail to actually stop using the device, but we tell the backend that it's OK to go away. Oops :) The incremental patch below makes it work if I unplug a console while writing /dev/zero to it. But I suspect instead of calling xencons_disconnect_backend() when the backend changes to XenbusStateClos{ing,ed}, it should actually *just* do the hvc_close() part? The evtchn and gnttab cleanup should wait until the backend has actually finished closing? And what locking is there around xencons_disconnect_backend() anyway? Do we rely on the fact that it can all only happen from the xenstore watch thread? diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c index f0376612b267..0806078835f6 100644 --- a/drivers/tty/hvc/hvc_xen.c +++ b/drivers/tty/hvc/hvc_xen.c @@ -377,8 +377,10 @@ void xen_console_resume(void) #ifdef CONFIG_HVC_XEN_FRONTEND static void xencons_disconnect_backend(struct xencons_info *info) { + if (info->hvc != NULL) + hvc_remove(info->hvc); + info->hvc = NULL; if (info->irq > 0) { - notifier_del_irq(info->hvc, info->irq); evtchn_put(info->evtchn); info->irq = 0; info->evtchn = 0; @@ -390,9 +392,6 @@ static void xencons_disconnect_backend(struct xencons_info *info) if (info->gntref > 0) gnttab_free_grant_references(info->gntref); info->gntref = 0; - if (info->hvc != NULL) - hvc_remove(info->hvc); - info->hvc = NULL; } static void xencons_free(struct xencons_info *info) @@ -558,6 +557,7 @@ static void xencons_backend_changed(struct xenbus_device *dev, break; fallthrough; /* Missed the backend's CLOSING state */ case XenbusStateClosing: + xencons_disconnect_backend(dev_get_drvdata(&dev->dev)); xenbus_frontend_closed(dev); break; }
diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c index 98764e740c07..f0376612b267 100644 --- a/drivers/tty/hvc/hvc_xen.c +++ b/drivers/tty/hvc/hvc_xen.c @@ -377,9 +377,13 @@ void xen_console_resume(void) #ifdef CONFIG_HVC_XEN_FRONTEND static void xencons_disconnect_backend(struct xencons_info *info) { - if (info->irq > 0) - unbind_from_irqhandler(info->irq, NULL); - info->irq = 0; + if (info->irq > 0) { + notifier_del_irq(info->hvc, info->irq); + evtchn_put(info->evtchn); + info->irq = 0; + info->evtchn = 0; + } + /* evtchn_put() will also close it so this is only an error path */ if (info->evtchn > 0) xenbus_free_evtchn(info->xbdev, info->evtchn); info->evtchn = 0; @@ -433,7 +437,7 @@ static int xencons_connect_backend(struct xenbus_device *dev, if (ret) return ret; info->evtchn = evtchn; - irq = bind_interdomain_evtchn_to_irq_lateeoi(dev, evtchn); + irq = bind_evtchn_to_irq_lateeoi(evtchn); if (irq < 0) return irq; info->irq = irq; @@ -597,7 +601,7 @@ static int __init xen_hvc_init(void) else r = xen_pv_console_init(); if (r < 0) - return r; + goto register_fe; info = vtermno_to_xencons(HVC_COOKIE); info->irq = bind_evtchn_to_irq_lateeoi(info->evtchn); @@ -616,12 +620,13 @@ static int __init xen_hvc_init(void) list_del(&info->list); spin_unlock_irqrestore(&xencons_lock, flags); if (info->irq) - unbind_from_irqhandler(info->irq, NULL); + evtchn_put(info->evtchn); kfree(info); return r; } r = 0; + register_fe: #ifdef CONFIG_HVC_XEN_FRONTEND r = xenbus_register_frontend(&xencons_driver); #endif