From: Paul Durrant <pdurrant@amazon.com> To: <xen-devel@lists.xenproject.org>, <qemu-block@nongnu.org>, <qemu-devel@nongnu.org> Cc: Paul Durrant <pdurrant@amazon.com>, Julien Grall <jgrall@amazon.com>, Stefano Stabellini <sstabellini@kernel.org>, Anthony Perard <anthony.perard@citrix.com>, Stefan Hajnoczi <stefanha@redhat.com>, "Kevin Wolf" <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com> Subject: [PATCH] xen-bus/block: explicitly assign event channels to an AioContext Date: Mon, 16 Dec 2019 14:34:51 +0000 [thread overview] Message-ID: <20191216143451.19024-1-pdurrant@amazon.com> (raw) It is not safe to close an event channel from the QEMU main thread when that channel's poller is running in IOThread context. This patch adds a new xen_device_set_event_channel_context() function to explicitly assign the channel AioContext, and modifies xen_device_bind_event_channel() to initially assign the channel's poller to the QEMU main thread context. The code in xen-block's dataplane is then modified to assign the channel to IOThread context during xen_block_dataplane_start() and de-assign it during in xen_block_dataplane_stop(), such that the channel is always assigned back to main thread context before it is closed. aio_set_fd_handler() already deals with all the necessary synchronization when moving an fd between AioContext-s so no extra code is needed to manage this. Reported-by: Julien Grall <jgrall@amazon.com> Signed-off-by: Paul Durrant <pdurrant@amazon.com> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Anthony Perard <anthony.perard@citrix.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Tested against an HVM debian guest with a QCOW2 image as system disk, and as a hot-plugged/unplgged secondary disk. --- hw/block/dataplane/xen-block.c | 20 ++++++++++++++++++-- hw/xen/xen-bus.c | 27 +++++++++++++++++++++++---- include/hw/xen/xen-bus.h | 5 ++++- 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c index 3b9caeb2fa..288a87a814 100644 --- a/hw/block/dataplane/xen-block.c +++ b/hw/block/dataplane/xen-block.c @@ -685,12 +685,24 @@ void xen_block_dataplane_stop(XenBlockDataPlane *dataplane) return; } + xendev = dataplane->xendev; + aio_context_acquire(dataplane->ctx); + if (dataplane->event_channel) { + /* Only reason for failure is a NULL channel */ + xen_device_set_event_channel_context(xendev, dataplane->event_channel, + qemu_get_aio_context(), + &error_abort); + } /* Xen doesn't have multiple users for nodes, so this can't fail */ blk_set_aio_context(dataplane->blk, qemu_get_aio_context(), &error_abort); aio_context_release(dataplane->ctx); - xendev = dataplane->xendev; + /* + * Now that the context has been moved onto the main thread, cancel + * further processing. + */ + qemu_bh_cancel(dataplane->bh); if (dataplane->event_channel) { Error *local_err = NULL; @@ -807,7 +819,7 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane, } dataplane->event_channel = - xen_device_bind_event_channel(xendev, dataplane->ctx, event_channel, + xen_device_bind_event_channel(xendev, event_channel, xen_block_dataplane_event, dataplane, &local_err); if (local_err) { @@ -818,7 +830,11 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane, aio_context_acquire(dataplane->ctx); /* If other users keep the BlockBackend in the iothread, that's ok */ blk_set_aio_context(dataplane->blk, dataplane->ctx, NULL); + /* Only reason for failure is a NULL channel */ + xen_device_set_event_channel_context(xendev, dataplane->event_channel, + dataplane->ctx, &error_abort); aio_context_release(dataplane->ctx); + return; stop: diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c index c2ad22a42d..349856b32b 100644 --- a/hw/xen/xen-bus.c +++ b/hw/xen/xen-bus.c @@ -1089,8 +1089,26 @@ static void xen_device_event(void *opaque) } } +void xen_device_set_event_channel_context(XenDevice *xendev, + XenEventChannel *channel, + AioContext *ctx, + Error **errp) +{ + if (!channel) { + error_setg(errp, "bad channel"); + return; + } + + if (channel->ctx) + aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), true, + NULL, NULL, NULL, NULL); + + channel->ctx = ctx; + aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), true, + xen_device_event, NULL, xen_device_poll, channel); +} + XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev, - AioContext *ctx, unsigned int port, XenEventHandler handler, void *opaque, Error **errp) @@ -1116,9 +1134,10 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev, channel->handler = handler; channel->opaque = opaque; - channel->ctx = ctx; - aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), true, - xen_device_event, NULL, xen_device_poll, channel); + /* Only reason for failure is a NULL channel */ + xen_device_set_event_channel_context(xendev, channel, + qemu_get_aio_context(), + &error_abort); QLIST_INSERT_HEAD(&xendev->event_channels, channel, list); diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h index 3d5532258d..c18c1372af 100644 --- a/include/hw/xen/xen-bus.h +++ b/include/hw/xen/xen-bus.h @@ -128,10 +128,13 @@ void xen_device_copy_grant_refs(XenDevice *xendev, bool to_domain, typedef bool (*XenEventHandler)(void *opaque); XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev, - AioContext *ctx, unsigned int port, XenEventHandler handler, void *opaque, Error **errp); +void xen_device_set_event_channel_context(XenDevice *xendev, + XenEventChannel *channel, + AioContext *ctx, + Error **errp); void xen_device_notify_event_channel(XenDevice *xendev, XenEventChannel *channel, Error **errp); -- 2.20.1
WARNING: multiple messages have this Message-ID (diff)
From: Paul Durrant <pdurrant@amazon.com> To: <xen-devel@lists.xenproject.org>, <qemu-block@nongnu.org>, <qemu-devel@nongnu.org> Cc: Kevin Wolf <kwolf@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, Paul Durrant <pdurrant@amazon.com>, Max Reitz <mreitz@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Anthony Perard <anthony.perard@citrix.com>, Julien Grall <jgrall@amazon.com> Subject: [Xen-devel] [PATCH] xen-bus/block: explicitly assign event channels to an AioContext Date: Mon, 16 Dec 2019 14:34:51 +0000 [thread overview] Message-ID: <20191216143451.19024-1-pdurrant@amazon.com> (raw) It is not safe to close an event channel from the QEMU main thread when that channel's poller is running in IOThread context. This patch adds a new xen_device_set_event_channel_context() function to explicitly assign the channel AioContext, and modifies xen_device_bind_event_channel() to initially assign the channel's poller to the QEMU main thread context. The code in xen-block's dataplane is then modified to assign the channel to IOThread context during xen_block_dataplane_start() and de-assign it during in xen_block_dataplane_stop(), such that the channel is always assigned back to main thread context before it is closed. aio_set_fd_handler() already deals with all the necessary synchronization when moving an fd between AioContext-s so no extra code is needed to manage this. Reported-by: Julien Grall <jgrall@amazon.com> Signed-off-by: Paul Durrant <pdurrant@amazon.com> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Anthony Perard <anthony.perard@citrix.com> Cc: Stefan Hajnoczi <stefanha@redhat.com> Cc: Kevin Wolf <kwolf@redhat.com> Cc: Max Reitz <mreitz@redhat.com> Tested against an HVM debian guest with a QCOW2 image as system disk, and as a hot-plugged/unplgged secondary disk. --- hw/block/dataplane/xen-block.c | 20 ++++++++++++++++++-- hw/xen/xen-bus.c | 27 +++++++++++++++++++++++---- include/hw/xen/xen-bus.h | 5 ++++- 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c index 3b9caeb2fa..288a87a814 100644 --- a/hw/block/dataplane/xen-block.c +++ b/hw/block/dataplane/xen-block.c @@ -685,12 +685,24 @@ void xen_block_dataplane_stop(XenBlockDataPlane *dataplane) return; } + xendev = dataplane->xendev; + aio_context_acquire(dataplane->ctx); + if (dataplane->event_channel) { + /* Only reason for failure is a NULL channel */ + xen_device_set_event_channel_context(xendev, dataplane->event_channel, + qemu_get_aio_context(), + &error_abort); + } /* Xen doesn't have multiple users for nodes, so this can't fail */ blk_set_aio_context(dataplane->blk, qemu_get_aio_context(), &error_abort); aio_context_release(dataplane->ctx); - xendev = dataplane->xendev; + /* + * Now that the context has been moved onto the main thread, cancel + * further processing. + */ + qemu_bh_cancel(dataplane->bh); if (dataplane->event_channel) { Error *local_err = NULL; @@ -807,7 +819,7 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane, } dataplane->event_channel = - xen_device_bind_event_channel(xendev, dataplane->ctx, event_channel, + xen_device_bind_event_channel(xendev, event_channel, xen_block_dataplane_event, dataplane, &local_err); if (local_err) { @@ -818,7 +830,11 @@ void xen_block_dataplane_start(XenBlockDataPlane *dataplane, aio_context_acquire(dataplane->ctx); /* If other users keep the BlockBackend in the iothread, that's ok */ blk_set_aio_context(dataplane->blk, dataplane->ctx, NULL); + /* Only reason for failure is a NULL channel */ + xen_device_set_event_channel_context(xendev, dataplane->event_channel, + dataplane->ctx, &error_abort); aio_context_release(dataplane->ctx); + return; stop: diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c index c2ad22a42d..349856b32b 100644 --- a/hw/xen/xen-bus.c +++ b/hw/xen/xen-bus.c @@ -1089,8 +1089,26 @@ static void xen_device_event(void *opaque) } } +void xen_device_set_event_channel_context(XenDevice *xendev, + XenEventChannel *channel, + AioContext *ctx, + Error **errp) +{ + if (!channel) { + error_setg(errp, "bad channel"); + return; + } + + if (channel->ctx) + aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), true, + NULL, NULL, NULL, NULL); + + channel->ctx = ctx; + aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), true, + xen_device_event, NULL, xen_device_poll, channel); +} + XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev, - AioContext *ctx, unsigned int port, XenEventHandler handler, void *opaque, Error **errp) @@ -1116,9 +1134,10 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev, channel->handler = handler; channel->opaque = opaque; - channel->ctx = ctx; - aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), true, - xen_device_event, NULL, xen_device_poll, channel); + /* Only reason for failure is a NULL channel */ + xen_device_set_event_channel_context(xendev, channel, + qemu_get_aio_context(), + &error_abort); QLIST_INSERT_HEAD(&xendev->event_channels, channel, list); diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h index 3d5532258d..c18c1372af 100644 --- a/include/hw/xen/xen-bus.h +++ b/include/hw/xen/xen-bus.h @@ -128,10 +128,13 @@ void xen_device_copy_grant_refs(XenDevice *xendev, bool to_domain, typedef bool (*XenEventHandler)(void *opaque); XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev, - AioContext *ctx, unsigned int port, XenEventHandler handler, void *opaque, Error **errp); +void xen_device_set_event_channel_context(XenDevice *xendev, + XenEventChannel *channel, + AioContext *ctx, + Error **errp); void xen_device_notify_event_channel(XenDevice *xendev, XenEventChannel *channel, Error **errp); -- 2.20.1 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
next reply other threads:[~2019-12-16 14:36 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-12-16 14:34 Paul Durrant [this message] 2019-12-16 14:34 ` [Xen-devel] [PATCH] xen-bus/block: explicitly assign event channels to an AioContext Paul Durrant 2019-12-19 17:11 ` Anthony PERARD 2019-12-19 17:11 ` [Xen-devel] " Anthony PERARD 2020-01-29 22:22 ` Julien Grall 2020-01-29 22:22 ` [Xen-devel] " Julien Grall 2020-01-31 11:12 ` Anthony PERARD 2020-01-31 11:12 ` [Xen-devel] " Anthony PERARD
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191216143451.19024-1-pdurrant@amazon.com \ --to=pdurrant@amazon.com \ --cc=anthony.perard@citrix.com \ --cc=jgrall@amazon.com \ --cc=kwolf@redhat.com \ --cc=mreitz@redhat.com \ --cc=qemu-block@nongnu.org \ --cc=qemu-devel@nongnu.org \ --cc=sstabellini@kernel.org \ --cc=stefanha@redhat.com \ --cc=xen-devel@lists.xenproject.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.