[Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
From: Oleksandr Andrushchenko Handle Xen event channels: - create for all configured streams and publish corresponding ring references and event channels in Xen store, so backend can connect - implement event channels interrupt handlers - create and destroy event channels with respect to Xen bus state Signed-off-by: Oleksandr Andrushchenko --- sound/xen/Makefile| 3 +- sound/xen/xen_snd_front.c | 10 +- sound/xen/xen_snd_front.h | 7 + sound/xen/xen_snd_front_evtchnl.c | 474 ++ sound/xen/xen_snd_front_evtchnl.h | 92 5 files changed, 584 insertions(+), 2 deletions(-) create mode 100644 sound/xen/xen_snd_front_evtchnl.c create mode 100644 sound/xen/xen_snd_front_evtchnl.h diff --git a/sound/xen/Makefile b/sound/xen/Makefile index 06705bef61fa..03c669984000 100644 --- a/sound/xen/Makefile +++ b/sound/xen/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 OR MIT snd_xen_front-objs := xen_snd_front.o \ - xen_snd_front_cfg.o + xen_snd_front_cfg.o \ + xen_snd_front_evtchnl.o obj-$(CONFIG_SND_XEN_FRONTEND) += snd_xen_front.o diff --git a/sound/xen/xen_snd_front.c b/sound/xen/xen_snd_front.c index 65d2494a9d14..eb46bf4070f9 100644 --- a/sound/xen/xen_snd_front.c +++ b/sound/xen/xen_snd_front.c @@ -18,9 +18,11 @@ #include #include "xen_snd_front.h" +#include "xen_snd_front_evtchnl.h" static void xen_snd_drv_fini(struct xen_snd_front_info *front_info) { + xen_snd_front_evtchnl_free_all(front_info); } static int sndback_initwait(struct xen_snd_front_info *front_info) @@ -32,7 +34,12 @@ static int sndback_initwait(struct xen_snd_front_info *front_info) if (ret < 0) return ret; - return 0; + /* create event channels for all streams and publish */ + ret = xen_snd_front_evtchnl_create_all(front_info, num_streams); + if (ret < 0) + return ret; + + return xen_snd_front_evtchnl_publish_all(front_info); } static int sndback_connect(struct xen_snd_front_info *front_info) @@ -122,6 +129,7 @@ static int xen_drv_probe(struct xenbus_device *xb_dev, return -ENOMEM; front_info->xb_dev = xb_dev; + spin_lock_init(&front_info->io_lock); dev_set_drvdata(&xb_dev->dev, front_info); return xenbus_switch_state(xb_dev, XenbusStateInitialising); diff --git a/sound/xen/xen_snd_front.h b/sound/xen/xen_snd_front.h index b52226cb30bc..9c2ffbb4e4b8 100644 --- a/sound/xen/xen_snd_front.h +++ b/sound/xen/xen_snd_front.h @@ -13,9 +13,16 @@ #include "xen_snd_front_cfg.h" +struct xen_snd_front_evtchnl_pair; + struct xen_snd_front_info { struct xenbus_device *xb_dev; + /* serializer for backend IO: request/response */ + spinlock_t io_lock; + int num_evt_pairs; + struct xen_snd_front_evtchnl_pair *evt_pairs; + struct xen_front_cfg_card cfg; }; diff --git a/sound/xen/xen_snd_front_evtchnl.c b/sound/xen/xen_snd_front_evtchnl.c new file mode 100644 index ..9ece39f938f8 --- /dev/null +++ b/sound/xen/xen_snd_front_evtchnl.c @@ -0,0 +1,474 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT + +/* + * Xen para-virtual sound device + * + * Copyright (C) 2016-2018 EPAM Systems Inc. + * + * Author: Oleksandr Andrushchenko + */ + +#include +#include +#include +#include + +#include "xen_snd_front.h" +#include "xen_snd_front_cfg.h" +#include "xen_snd_front_evtchnl.h" + +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) +{ + struct xen_snd_front_evtchnl *channel = dev_id; + struct xen_snd_front_info *front_info = channel->front_info; + struct xensnd_resp *resp; + RING_IDX i, rp; + unsigned long flags; + + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) + return IRQ_HANDLED; + + spin_lock_irqsave(&front_info->io_lock, flags); + +again: + rp = channel->u.req.ring.sring->rsp_prod; + /* ensure we see queued responses up to rp */ + rmb(); + + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { + resp = RING_GET_RESPONSE(&channel->u.req.ring, i); + if (resp->id != channel->evt_id) + continue; + switch (resp->operation) { + case XENSND_OP_OPEN: + /* fall through */ + case XENSND_OP_CLOSE: + /* fall through */ + case XENSND_OP_READ: + /* fall through */ + case XENSND_OP_WRITE: + /* fall through */ + case XENSND_OP_TRIGGER: + channel->u.req.resp_status = resp->status; + complete(&channel->u.req.completion); + break; + case XENSND_OP_HW_PARAM_QUERY: + channel->u.req.resp_s
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On 16/04/18 08:24, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko > > Handle Xen event channels: > - create for all configured streams and publish > corresponding ring references and event channels in Xen store, > so backend can connect > - implement event channels interrupt handlers > - create and destroy event channels with respect to Xen bus state > > Signed-off-by: Oleksandr Andrushchenko > --- > sound/xen/Makefile| 3 +- > sound/xen/xen_snd_front.c | 10 +- > sound/xen/xen_snd_front.h | 7 + > sound/xen/xen_snd_front_evtchnl.c | 474 > ++ > sound/xen/xen_snd_front_evtchnl.h | 92 > 5 files changed, 584 insertions(+), 2 deletions(-) > create mode 100644 sound/xen/xen_snd_front_evtchnl.c > create mode 100644 sound/xen/xen_snd_front_evtchnl.h > > diff --git a/sound/xen/Makefile b/sound/xen/Makefile > index 06705bef61fa..03c669984000 100644 > --- a/sound/xen/Makefile > +++ b/sound/xen/Makefile > @@ -1,6 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0 OR MIT > > snd_xen_front-objs := xen_snd_front.o \ > - xen_snd_front_cfg.o > + xen_snd_front_cfg.o \ > + xen_snd_front_evtchnl.o > > obj-$(CONFIG_SND_XEN_FRONTEND) += snd_xen_front.o > diff --git a/sound/xen/xen_snd_front.c b/sound/xen/xen_snd_front.c > index 65d2494a9d14..eb46bf4070f9 100644 > --- a/sound/xen/xen_snd_front.c > +++ b/sound/xen/xen_snd_front.c > @@ -18,9 +18,11 @@ > #include > > #include "xen_snd_front.h" > +#include "xen_snd_front_evtchnl.h" Does it really make sense to have multiple driver-private headers? I think those can be merged. > > static void xen_snd_drv_fini(struct xen_snd_front_info *front_info) > { > + xen_snd_front_evtchnl_free_all(front_info); > } > > static int sndback_initwait(struct xen_snd_front_info *front_info) > @@ -32,7 +34,12 @@ static int sndback_initwait(struct xen_snd_front_info > *front_info) > if (ret < 0) > return ret; > > - return 0; > + /* create event channels for all streams and publish */ > + ret = xen_snd_front_evtchnl_create_all(front_info, num_streams); > + if (ret < 0) > + return ret; > + > + return xen_snd_front_evtchnl_publish_all(front_info); > } > > static int sndback_connect(struct xen_snd_front_info *front_info) > @@ -122,6 +129,7 @@ static int xen_drv_probe(struct xenbus_device *xb_dev, > return -ENOMEM; > > front_info->xb_dev = xb_dev; > + spin_lock_init(&front_info->io_lock); > dev_set_drvdata(&xb_dev->dev, front_info); > > return xenbus_switch_state(xb_dev, XenbusStateInitialising); > diff --git a/sound/xen/xen_snd_front.h b/sound/xen/xen_snd_front.h > index b52226cb30bc..9c2ffbb4e4b8 100644 > --- a/sound/xen/xen_snd_front.h > +++ b/sound/xen/xen_snd_front.h > @@ -13,9 +13,16 @@ > > #include "xen_snd_front_cfg.h" > > +struct xen_snd_front_evtchnl_pair; > + > struct xen_snd_front_info { > struct xenbus_device *xb_dev; > > + /* serializer for backend IO: request/response */ > + spinlock_t io_lock; > + int num_evt_pairs; > + struct xen_snd_front_evtchnl_pair *evt_pairs; > + > struct xen_front_cfg_card cfg; > }; > > diff --git a/sound/xen/xen_snd_front_evtchnl.c > b/sound/xen/xen_snd_front_evtchnl.c > new file mode 100644 > index ..9ece39f938f8 > --- /dev/null > +++ b/sound/xen/xen_snd_front_evtchnl.c > @@ -0,0 +1,474 @@ > +// SPDX-License-Identifier: GPL-2.0 OR MIT > + > +/* > + * Xen para-virtual sound device > + * > + * Copyright (C) 2016-2018 EPAM Systems Inc. > + * > + * Author: Oleksandr Andrushchenko > + */ > + > +#include > +#include > +#include > +#include > + > +#include "xen_snd_front.h" > +#include "xen_snd_front_cfg.h" > +#include "xen_snd_front_evtchnl.h" > + > +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) > +{ > + struct xen_snd_front_evtchnl *channel = dev_id; > + struct xen_snd_front_info *front_info = channel->front_info; > + struct xensnd_resp *resp; > + RING_IDX i, rp; > + unsigned long flags; > + > + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) > + return IRQ_HANDLED; > + > + spin_lock_irqsave(&front_info->io_lock, flags); > + > +again: > + rp = channel->u.req.ring.sring->rsp_prod; > + /* ensure we see queued responses up to rp */ > + rmb(); > + > + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { > + resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > + if (resp->id != channel->evt_id) > + continue; > + switch (resp->operation) { > + case XENSND_OP_OPEN: > + /* fall through */ > + case XENSND_OP_CLOSE: > + /* fall through */ > + case XENSND_OP_READ: > + /* fall through */ > +
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
Hi Oleksandr, Thank you for the patch! Yet something to improve: [auto build test ERROR on sound/for-next] [also build test ERROR on v4.17-rc1 next-20180416] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Oleksandr-Andrushchenko/ALSA-xen-front-Add-Xen-para-virtualized-frontend-driver/20180416-143123 base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next config: x86_64-allmodconfig (attached as .config) compiler: gcc-7 (Debian 7.3.0-1) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): In file included from sound/xen/xen_snd_front.c:21:0: >> sound/xen/xen_snd_front_evtchnl.h:62:34: error: field 'hw_param' has >> incomplete type struct xensnd_query_hw_param hw_param; ^~~~ -- sound/xen/xen_snd_front_evtchnl.c:51:22: sparse: undefined identifier 'XENSND_OP_TRIGGER' sound/xen/xen_snd_front_evtchnl.c:55:22: sparse: undefined identifier 'XENSND_OP_HW_PARAM_QUERY' sound/xen/xen_snd_front_evtchnl.c:58:45: sparse: no member 'resp' in struct xensnd_resp sound/xen/xen_snd_front_evtchnl.c:51:22: sparse: incompatible types for 'case' statement sound/xen/xen_snd_front_evtchnl.c:55:22: sparse: incompatible types for 'case' statement sound/xen/xen_snd_front_evtchnl.c:99:20: sparse: using member 'in_prod' in incomplete struct xensnd_event_page sound/xen/xen_snd_front_evtchnl.c:102:25: sparse: using member 'in_cons' in incomplete struct xensnd_event_page sound/xen/xen_snd_front_evtchnl.c:105:25: sparse: using member 'in_cons' in incomplete struct xensnd_event_page sound/xen/xen_snd_front_evtchnl.c:108:26: sparse: undefined identifier 'XENSND_IN_RING_REF' sound/xen/xen_snd_front_evtchnl.c:109:21: sparse: using member 'id' in incomplete struct xensnd_evt sound/xen/xen_snd_front_evtchnl.c:112:30: sparse: using member 'type' in incomplete struct xensnd_evt sound/xen/xen_snd_front_evtchnl.c:113:22: sparse: undefined identifier 'XENSND_EVT_CUR_POS' sound/xen/xen_snd_front_evtchnl.c:113:22: sparse: incompatible types for 'case' statement sound/xen/xen_snd_front_evtchnl.c:119:13: sparse: using member 'in_cons' in incomplete struct xensnd_event_page sound/xen/xen_snd_front_evtchnl.c:213:34: sparse: undefined identifier 'XENSND_FIELD_EVT_RING_REF' sound/xen/xen_snd_front_evtchnl.c:412:47: sparse: undefined identifier 'XENSND_FIELD_EVT_RING_REF' sound/xen/xen_snd_front_evtchnl.c:432:47: sparse: undefined identifier 'XENSND_FIELD_EVT_RING_REF' sound/xen/xen_snd_front_evtchnl.c:51:22: sparse: Expected constant expression in case statement sound/xen/xen_snd_front_evtchnl.c:55:22: sparse: Expected constant expression in case statement sound/xen/xen_snd_front_evtchnl.c:113:22: sparse: Expected constant expression in case statement In file included from sound/xen/xen_snd_front_evtchnl.c:18:0: >> sound/xen/xen_snd_front_evtchnl.h:62:34: error: field 'hw_param' has >> incomplete type struct xensnd_query_hw_param hw_param; ^~~~ sound/xen/xen_snd_front_evtchnl.c: In function 'evtchnl_interrupt_req': >> sound/xen/xen_snd_front_evtchnl.c:51:8: error: 'XENSND_OP_TRIGGER' >> undeclared (first use in this function); did you mean 'XENSND_OP_WRITE'? case XENSND_OP_TRIGGER: ^ XENSND_OP_WRITE sound/xen/xen_snd_front_evtchnl.c:51:8: note: each undeclared identifier is reported only once for each function it appears in >> sound/xen/xen_snd_front_evtchnl.c:55:8: error: 'XENSND_OP_HW_PARAM_QUERY' >> undeclared (first use in this function); did you mean 'XENSND_OP_TRIGGER'? case XENSND_OP_HW_PARAM_QUERY: ^~~~ XENSND_OP_TRIGGER >> sound/xen/xen_snd_front_evtchnl.c:58:10: error: 'struct xensnd_resp' has no >> member named 'resp' resp->resp.hw_param; ^~ sound/xen/xen_snd_front_evtchnl.c: In function 'evtchnl_interrupt_evt': >> sound/xen/xen_snd_front_evtchnl.c:99:13: error: dereferencing pointer to >> incomplete type 'struct xensnd_event_page' prod = page->in_prod; ^~ >> sound/xen/xen_snd_front_evtchnl.c:108:12: error: implicit declaration of >> function 'XENSND_IN_RING_REF'; did you mean 'XENSND_FIELD_RING_REF'? >> [-Werror=implicit-function-declaration] event = &XENSND_IN_RING_REF(page, cons); ^~ XENSND_FIELD_RING_REF >> sound/xen/xen_snd_front_evtchnl.c:108:11: error: lvalue required as unary >> '&' operand event = &XENSND_IN_RING_REF(page, cons); ^ In file included from include/linux/kernel.h:10:0, from include/linux/interrupt.h:6, from include/xen/events.h:5, from sound/xen/xen
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On 04/16/2018 04:12 PM, Juergen Gross wrote: On 16/04/18 08:24, Oleksandr Andrushchenko wrote: From: Oleksandr Andrushchenko Handle Xen event channels: - create for all configured streams and publish corresponding ring references and event channels in Xen store, so backend can connect - implement event channels interrupt handlers - create and destroy event channels with respect to Xen bus state Signed-off-by: Oleksandr Andrushchenko --- sound/xen/Makefile| 3 +- sound/xen/xen_snd_front.c | 10 +- sound/xen/xen_snd_front.h | 7 + sound/xen/xen_snd_front_evtchnl.c | 474 ++ sound/xen/xen_snd_front_evtchnl.h | 92 5 files changed, 584 insertions(+), 2 deletions(-) create mode 100644 sound/xen/xen_snd_front_evtchnl.c create mode 100644 sound/xen/xen_snd_front_evtchnl.h diff --git a/sound/xen/Makefile b/sound/xen/Makefile index 06705bef61fa..03c669984000 100644 --- a/sound/xen/Makefile +++ b/sound/xen/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 OR MIT snd_xen_front-objs := xen_snd_front.o \ - xen_snd_front_cfg.o + xen_snd_front_cfg.o \ + xen_snd_front_evtchnl.o obj-$(CONFIG_SND_XEN_FRONTEND) += snd_xen_front.o diff --git a/sound/xen/xen_snd_front.c b/sound/xen/xen_snd_front.c index 65d2494a9d14..eb46bf4070f9 100644 --- a/sound/xen/xen_snd_front.c +++ b/sound/xen/xen_snd_front.c @@ -18,9 +18,11 @@ #include #include "xen_snd_front.h" +#include "xen_snd_front_evtchnl.h" Does it really make sense to have multiple driver-private headers? I think those can be merged. I would really like to keep it separate as it clearly shows which stuff belongs to which modules. At least for me it is easier to maintain it this way. static void xen_snd_drv_fini(struct xen_snd_front_info *front_info) { + xen_snd_front_evtchnl_free_all(front_info); } static int sndback_initwait(struct xen_snd_front_info *front_info) @@ -32,7 +34,12 @@ static int sndback_initwait(struct xen_snd_front_info *front_info) if (ret < 0) return ret; - return 0; + /* create event channels for all streams and publish */ + ret = xen_snd_front_evtchnl_create_all(front_info, num_streams); + if (ret < 0) + return ret; + + return xen_snd_front_evtchnl_publish_all(front_info); } static int sndback_connect(struct xen_snd_front_info *front_info) @@ -122,6 +129,7 @@ static int xen_drv_probe(struct xenbus_device *xb_dev, return -ENOMEM; front_info->xb_dev = xb_dev; + spin_lock_init(&front_info->io_lock); dev_set_drvdata(&xb_dev->dev, front_info); return xenbus_switch_state(xb_dev, XenbusStateInitialising); diff --git a/sound/xen/xen_snd_front.h b/sound/xen/xen_snd_front.h index b52226cb30bc..9c2ffbb4e4b8 100644 --- a/sound/xen/xen_snd_front.h +++ b/sound/xen/xen_snd_front.h @@ -13,9 +13,16 @@ #include "xen_snd_front_cfg.h" +struct xen_snd_front_evtchnl_pair; + struct xen_snd_front_info { struct xenbus_device *xb_dev; + /* serializer for backend IO: request/response */ + spinlock_t io_lock; + int num_evt_pairs; + struct xen_snd_front_evtchnl_pair *evt_pairs; + struct xen_front_cfg_card cfg; }; diff --git a/sound/xen/xen_snd_front_evtchnl.c b/sound/xen/xen_snd_front_evtchnl.c new file mode 100644 index ..9ece39f938f8 --- /dev/null +++ b/sound/xen/xen_snd_front_evtchnl.c @@ -0,0 +1,474 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT + +/* + * Xen para-virtual sound device + * + * Copyright (C) 2016-2018 EPAM Systems Inc. + * + * Author: Oleksandr Andrushchenko + */ + +#include +#include +#include +#include + +#include "xen_snd_front.h" +#include "xen_snd_front_cfg.h" +#include "xen_snd_front_evtchnl.h" + +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) +{ + struct xen_snd_front_evtchnl *channel = dev_id; + struct xen_snd_front_info *front_info = channel->front_info; + struct xensnd_resp *resp; + RING_IDX i, rp; + unsigned long flags; + + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) + return IRQ_HANDLED; + + spin_lock_irqsave(&front_info->io_lock, flags); + +again: + rp = channel->u.req.ring.sring->rsp_prod; + /* ensure we see queued responses up to rp */ + rmb(); + + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { + resp = RING_GET_RESPONSE(&channel->u.req.ring, i); + if (resp->id != channel->evt_id) + continue; + switch (resp->operation) { + case XENSND_OP_OPEN: + /* fall through */ + case XENSND_OP_CLOSE: + /* fall through */ + case XENSND_OP_READ: + /* fall thro
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On 17/04/18 10:58, Oleksandr Andrushchenko wrote: > On 04/16/2018 04:12 PM, Juergen Gross wrote: >> On 16/04/18 08:24, Oleksandr Andrushchenko wrote: >>> From: Oleksandr Andrushchenko >>> >>> Handle Xen event channels: >>> - create for all configured streams and publish >>> corresponding ring references and event channels in Xen store, >>> so backend can connect >>> - implement event channels interrupt handlers >>> - create and destroy event channels with respect to Xen bus state >>> >>> Signed-off-by: Oleksandr Andrushchenko >>> >>> --- >>> sound/xen/Makefile | 3 +- >>> sound/xen/xen_snd_front.c | 10 +- >>> sound/xen/xen_snd_front.h | 7 + >>> sound/xen/xen_snd_front_evtchnl.c | 474 >>> ++ >>> sound/xen/xen_snd_front_evtchnl.h | 92 >>> 5 files changed, 584 insertions(+), 2 deletions(-) >>> create mode 100644 sound/xen/xen_snd_front_evtchnl.c >>> create mode 100644 sound/xen/xen_snd_front_evtchnl.h >>> >>> diff --git a/sound/xen/Makefile b/sound/xen/Makefile >>> index 06705bef61fa..03c669984000 100644 >>> --- a/sound/xen/Makefile >>> +++ b/sound/xen/Makefile >>> @@ -1,6 +1,7 @@ >>> # SPDX-License-Identifier: GPL-2.0 OR MIT >>> snd_xen_front-objs := xen_snd_front.o \ >>> - xen_snd_front_cfg.o >>> + xen_snd_front_cfg.o \ >>> + xen_snd_front_evtchnl.o >>> obj-$(CONFIG_SND_XEN_FRONTEND) += snd_xen_front.o >>> diff --git a/sound/xen/xen_snd_front.c b/sound/xen/xen_snd_front.c >>> index 65d2494a9d14..eb46bf4070f9 100644 >>> --- a/sound/xen/xen_snd_front.c >>> +++ b/sound/xen/xen_snd_front.c >>> @@ -18,9 +18,11 @@ >>> #include >>> #include "xen_snd_front.h" >>> +#include "xen_snd_front_evtchnl.h" >> Does it really make sense to have multiple driver-private headers? >> >> I think those can be merged. > I would really like to keep it separate as it clearly > shows which stuff belongs to which modules. > At least for me it is easier to maintain it this way. >> >>> static void xen_snd_drv_fini(struct xen_snd_front_info *front_info) >>> { >>> + xen_snd_front_evtchnl_free_all(front_info); >>> } >>> static int sndback_initwait(struct xen_snd_front_info *front_info) >>> @@ -32,7 +34,12 @@ static int sndback_initwait(struct >>> xen_snd_front_info *front_info) >>> if (ret < 0) >>> return ret; >>> - return 0; >>> + /* create event channels for all streams and publish */ >>> + ret = xen_snd_front_evtchnl_create_all(front_info, num_streams); >>> + if (ret < 0) >>> + return ret; >>> + >>> + return xen_snd_front_evtchnl_publish_all(front_info); >>> } >>> static int sndback_connect(struct xen_snd_front_info *front_info) >>> @@ -122,6 +129,7 @@ static int xen_drv_probe(struct xenbus_device >>> *xb_dev, >>> return -ENOMEM; >>> front_info->xb_dev = xb_dev; >>> + spin_lock_init(&front_info->io_lock); >>> dev_set_drvdata(&xb_dev->dev, front_info); >>> return xenbus_switch_state(xb_dev, XenbusStateInitialising); >>> diff --git a/sound/xen/xen_snd_front.h b/sound/xen/xen_snd_front.h >>> index b52226cb30bc..9c2ffbb4e4b8 100644 >>> --- a/sound/xen/xen_snd_front.h >>> +++ b/sound/xen/xen_snd_front.h >>> @@ -13,9 +13,16 @@ >>> #include "xen_snd_front_cfg.h" >>> +struct xen_snd_front_evtchnl_pair; >>> + >>> struct xen_snd_front_info { >>> struct xenbus_device *xb_dev; >>> + /* serializer for backend IO: request/response */ >>> + spinlock_t io_lock; >>> + int num_evt_pairs; >>> + struct xen_snd_front_evtchnl_pair *evt_pairs; >>> + >>> struct xen_front_cfg_card cfg; >>> }; >>> diff --git a/sound/xen/xen_snd_front_evtchnl.c >>> b/sound/xen/xen_snd_front_evtchnl.c >>> new file mode 100644 >>> index ..9ece39f938f8 >>> --- /dev/null >>> +++ b/sound/xen/xen_snd_front_evtchnl.c >>> @@ -0,0 +1,474 @@ >>> +// SPDX-License-Identifier: GPL-2.0 OR MIT >>> + >>> +/* >>> + * Xen para-virtual sound device >>> + * >>> + * Copyright (C) 2016-2018 EPAM Systems Inc. >>> + * >>> + * Author: Oleksandr Andrushchenko >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include "xen_snd_front.h" >>> +#include "xen_snd_front_cfg.h" >>> +#include "xen_snd_front_evtchnl.h" >>> + >>> +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) >>> +{ >>> + struct xen_snd_front_evtchnl *channel = dev_id; >>> + struct xen_snd_front_info *front_info = channel->front_info; >>> + struct xensnd_resp *resp; >>> + RING_IDX i, rp; >>> + unsigned long flags; >>> + >>> + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) >>> + return IRQ_HANDLED; >>> + >>> + spin_lock_irqsave(&front_info->io_lock, flags); >>> + >>> +again: >>> + rp = channel->u.req.ring.sring->rsp_prod; >>> + /* ensure we see queued responses up to rp */ >>> + rmb(); >>> + >>> + for (i = channel->u.req.ring.rsp_cons
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On 04/17/2018 02:14 PM, Juergen Gross wrote: On 17/04/18 10:58, Oleksandr Andrushchenko wrote: On 04/16/2018 04:12 PM, Juergen Gross wrote: On 16/04/18 08:24, Oleksandr Andrushchenko wrote: From: Oleksandr Andrushchenko Handle Xen event channels: - create for all configured streams and publish corresponding ring references and event channels in Xen store, so backend can connect - implement event channels interrupt handlers - create and destroy event channels with respect to Xen bus state Signed-off-by: Oleksandr Andrushchenko --- sound/xen/Makefile | 3 +- sound/xen/xen_snd_front.c | 10 +- sound/xen/xen_snd_front.h | 7 + sound/xen/xen_snd_front_evtchnl.c | 474 ++ sound/xen/xen_snd_front_evtchnl.h | 92 5 files changed, 584 insertions(+), 2 deletions(-) create mode 100644 sound/xen/xen_snd_front_evtchnl.c create mode 100644 sound/xen/xen_snd_front_evtchnl.h diff --git a/sound/xen/Makefile b/sound/xen/Makefile index 06705bef61fa..03c669984000 100644 --- a/sound/xen/Makefile +++ b/sound/xen/Makefile @@ -1,6 +1,7 @@ # SPDX-License-Identifier: GPL-2.0 OR MIT snd_xen_front-objs := xen_snd_front.o \ - xen_snd_front_cfg.o + xen_snd_front_cfg.o \ + xen_snd_front_evtchnl.o obj-$(CONFIG_SND_XEN_FRONTEND) += snd_xen_front.o diff --git a/sound/xen/xen_snd_front.c b/sound/xen/xen_snd_front.c index 65d2494a9d14..eb46bf4070f9 100644 --- a/sound/xen/xen_snd_front.c +++ b/sound/xen/xen_snd_front.c @@ -18,9 +18,11 @@ #include #include "xen_snd_front.h" +#include "xen_snd_front_evtchnl.h" Does it really make sense to have multiple driver-private headers? I think those can be merged. I would really like to keep it separate as it clearly shows which stuff belongs to which modules. At least for me it is easier to maintain it this way. static void xen_snd_drv_fini(struct xen_snd_front_info *front_info) { + xen_snd_front_evtchnl_free_all(front_info); } static int sndback_initwait(struct xen_snd_front_info *front_info) @@ -32,7 +34,12 @@ static int sndback_initwait(struct xen_snd_front_info *front_info) if (ret < 0) return ret; - return 0; + /* create event channels for all streams and publish */ + ret = xen_snd_front_evtchnl_create_all(front_info, num_streams); + if (ret < 0) + return ret; + + return xen_snd_front_evtchnl_publish_all(front_info); } static int sndback_connect(struct xen_snd_front_info *front_info) @@ -122,6 +129,7 @@ static int xen_drv_probe(struct xenbus_device *xb_dev, return -ENOMEM; front_info->xb_dev = xb_dev; + spin_lock_init(&front_info->io_lock); dev_set_drvdata(&xb_dev->dev, front_info); return xenbus_switch_state(xb_dev, XenbusStateInitialising); diff --git a/sound/xen/xen_snd_front.h b/sound/xen/xen_snd_front.h index b52226cb30bc..9c2ffbb4e4b8 100644 --- a/sound/xen/xen_snd_front.h +++ b/sound/xen/xen_snd_front.h @@ -13,9 +13,16 @@ #include "xen_snd_front_cfg.h" +struct xen_snd_front_evtchnl_pair; + struct xen_snd_front_info { struct xenbus_device *xb_dev; + /* serializer for backend IO: request/response */ + spinlock_t io_lock; + int num_evt_pairs; + struct xen_snd_front_evtchnl_pair *evt_pairs; + struct xen_front_cfg_card cfg; }; diff --git a/sound/xen/xen_snd_front_evtchnl.c b/sound/xen/xen_snd_front_evtchnl.c new file mode 100644 index ..9ece39f938f8 --- /dev/null +++ b/sound/xen/xen_snd_front_evtchnl.c @@ -0,0 +1,474 @@ +// SPDX-License-Identifier: GPL-2.0 OR MIT + +/* + * Xen para-virtual sound device + * + * Copyright (C) 2016-2018 EPAM Systems Inc. + * + * Author: Oleksandr Andrushchenko + */ + +#include +#include +#include +#include + +#include "xen_snd_front.h" +#include "xen_snd_front_cfg.h" +#include "xen_snd_front_evtchnl.h" + +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) +{ + struct xen_snd_front_evtchnl *channel = dev_id; + struct xen_snd_front_info *front_info = channel->front_info; + struct xensnd_resp *resp; + RING_IDX i, rp; + unsigned long flags; + + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) + return IRQ_HANDLED; + + spin_lock_irqsave(&front_info->io_lock, flags); + +again: + rp = channel->u.req.ring.sring->rsp_prod; + /* ensure we see queued responses up to rp */ + rmb(); + + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { + resp = RING_GET_RESPONSE(&channel->u.req.ring, i); + if (resp->id != channel->evt_id) + continue; + switch (resp->operation) { + case XENSND_OP_OPEN: + /* fall through */ + case XENSND_OP_CLOSE: + /* fall through */ + case XENSND_OP_READ: + /* fall through */ + case XENSND_OP_WRITE: + /* fall through */ + c
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On Mon, 16 Apr 2018 08:24:51 +0200, Oleksandr Andrushchenko wrote: > +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) > +{ > + struct xen_snd_front_evtchnl *channel = dev_id; > + struct xen_snd_front_info *front_info = channel->front_info; > + struct xensnd_resp *resp; > + RING_IDX i, rp; > + unsigned long flags; > + > + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) > + return IRQ_HANDLED; > + > + spin_lock_irqsave(&front_info->io_lock, flags); > + > +again: > + rp = channel->u.req.ring.sring->rsp_prod; > + /* ensure we see queued responses up to rp */ > + rmb(); > + > + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { I'm not familiar with Xen stuff in general, but through a quick glance, this kind of code worries me a bit. If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a very long loop, no? Better to have a sanity check of the ring buffer size. > +static irqreturn_t evtchnl_interrupt_evt(int irq, void *dev_id) > +{ > + struct xen_snd_front_evtchnl *channel = dev_id; > + struct xen_snd_front_info *front_info = channel->front_info; > + struct xensnd_event_page *page = channel->u.evt.page; > + u32 cons, prod; > + unsigned long flags; > + > + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) > + return IRQ_HANDLED; > + > + spin_lock_irqsave(&front_info->io_lock, flags); > + > + prod = page->in_prod; > + /* ensure we see ring contents up to prod */ > + virt_rmb(); > + if (prod == page->in_cons) > + goto out; > + > + for (cons = page->in_cons; cons != prod; cons++) { Ditto. thanks, Takashi ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On 04/24/2018 05:20 PM, Takashi Iwai wrote: On Mon, 16 Apr 2018 08:24:51 +0200, Oleksandr Andrushchenko wrote: +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) +{ + struct xen_snd_front_evtchnl *channel = dev_id; + struct xen_snd_front_info *front_info = channel->front_info; + struct xensnd_resp *resp; + RING_IDX i, rp; + unsigned long flags; + + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) + return IRQ_HANDLED; + + spin_lock_irqsave(&front_info->io_lock, flags); + +again: + rp = channel->u.req.ring.sring->rsp_prod; + /* ensure we see queued responses up to rp */ + rmb(); + + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { I'm not familiar with Xen stuff in general, but through a quick glance, this kind of code worries me a bit. If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a very long loop, no? Better to have a sanity check of the ring buffer size. In this loop I have: resp = RING_GET_RESPONSE(&channel->u.req.ring, i); and the RING_GET_RESPONSE macro is designed in the way that it wraps around when *i* in the question gets bigger than the ring size: #define RING_GET_REQUEST(_r, _idx) \ (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) So, even if the counter has a bogus number it will not last long +static irqreturn_t evtchnl_interrupt_evt(int irq, void *dev_id) +{ + struct xen_snd_front_evtchnl *channel = dev_id; + struct xen_snd_front_info *front_info = channel->front_info; + struct xensnd_event_page *page = channel->u.evt.page; + u32 cons, prod; + unsigned long flags; + + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) + return IRQ_HANDLED; + + spin_lock_irqsave(&front_info->io_lock, flags); + + prod = page->in_prod; + /* ensure we see ring contents up to prod */ + virt_rmb(); + if (prod == page->in_cons) + goto out; + + for (cons = page->in_cons; cons != prod; cons++) { Ditto. Same as above thanks, Takashi Thank you, Oleksandr ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On Tue, 24 Apr 2018 16:29:15 +0200, Oleksandr Andrushchenko wrote: > > On 04/24/2018 05:20 PM, Takashi Iwai wrote: > > On Mon, 16 Apr 2018 08:24:51 +0200, > > Oleksandr Andrushchenko wrote: > >> +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) > >> +{ > >> + struct xen_snd_front_evtchnl *channel = dev_id; > >> + struct xen_snd_front_info *front_info = channel->front_info; > >> + struct xensnd_resp *resp; > >> + RING_IDX i, rp; > >> + unsigned long flags; > >> + > >> + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) > >> + return IRQ_HANDLED; > >> + > >> + spin_lock_irqsave(&front_info->io_lock, flags); > >> + > >> +again: > >> + rp = channel->u.req.ring.sring->rsp_prod; > >> + /* ensure we see queued responses up to rp */ > >> + rmb(); > >> + > >> + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { > > I'm not familiar with Xen stuff in general, but through a quick > > glance, this kind of code worries me a bit. > > > > If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a > > very long loop, no? Better to have a sanity check of the ring buffer > > size. > In this loop I have: > resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > and the RING_GET_RESPONSE macro is designed in the way that > it wraps around when *i* in the question gets bigger than > the ring size: > > #define RING_GET_REQUEST(_r, _idx) \ > (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) > > So, even if the counter has a bogus number it will not last long Hm, this prevents from accessing outside the ring buffer, but does it change the loop behavior? Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below would still consume the whole 32bit counts, no? for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { resp = RING_GET_RESPONSE(&channel->u.req.ring, i); ... } Takashi ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On 04/24/2018 05:35 PM, Takashi Iwai wrote: On Tue, 24 Apr 2018 16:29:15 +0200, Oleksandr Andrushchenko wrote: On 04/24/2018 05:20 PM, Takashi Iwai wrote: On Mon, 16 Apr 2018 08:24:51 +0200, Oleksandr Andrushchenko wrote: +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) +{ + struct xen_snd_front_evtchnl *channel = dev_id; + struct xen_snd_front_info *front_info = channel->front_info; + struct xensnd_resp *resp; + RING_IDX i, rp; + unsigned long flags; + + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) + return IRQ_HANDLED; + + spin_lock_irqsave(&front_info->io_lock, flags); + +again: + rp = channel->u.req.ring.sring->rsp_prod; + /* ensure we see queued responses up to rp */ + rmb(); + + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { I'm not familiar with Xen stuff in general, but through a quick glance, this kind of code worries me a bit. If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a very long loop, no? Better to have a sanity check of the ring buffer size. In this loop I have: resp = RING_GET_RESPONSE(&channel->u.req.ring, i); and the RING_GET_RESPONSE macro is designed in the way that it wraps around when *i* in the question gets bigger than the ring size: #define RING_GET_REQUEST(_r, _idx) \ (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) So, even if the counter has a bogus number it will not last long Hm, this prevents from accessing outside the ring buffer, but does it change the loop behavior? no, it doesn't Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below would still consume the whole 32bit counts, no? for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { resp = RING_GET_RESPONSE(&channel->u.req.ring, i); ... } You are right here and the comment is totally valid. I'll put an additional check like here [1] and here [2] Will this address your comment? Takashi Thank you, Oleksandr [1] https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1127 [2] https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1135 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On Tue, 24 Apr 2018 16:58:43 +0200, Oleksandr Andrushchenko wrote: > > On 04/24/2018 05:35 PM, Takashi Iwai wrote: > > On Tue, 24 Apr 2018 16:29:15 +0200, > > Oleksandr Andrushchenko wrote: > >> On 04/24/2018 05:20 PM, Takashi Iwai wrote: > >>> On Mon, 16 Apr 2018 08:24:51 +0200, > >>> Oleksandr Andrushchenko wrote: > +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) > +{ > +struct xen_snd_front_evtchnl *channel = dev_id; > +struct xen_snd_front_info *front_info = channel->front_info; > +struct xensnd_resp *resp; > +RING_IDX i, rp; > +unsigned long flags; > + > +if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) > +return IRQ_HANDLED; > + > +spin_lock_irqsave(&front_info->io_lock, flags); > + > +again: > +rp = channel->u.req.ring.sring->rsp_prod; > +/* ensure we see queued responses up to rp */ > +rmb(); > + > +for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { > >>> I'm not familiar with Xen stuff in general, but through a quick > >>> glance, this kind of code worries me a bit. > >>> > >>> If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a > >>> very long loop, no? Better to have a sanity check of the ring buffer > >>> size. > >> In this loop I have: > >> resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > >> and the RING_GET_RESPONSE macro is designed in the way that > >> it wraps around when *i* in the question gets bigger than > >> the ring size: > >> > >> #define RING_GET_REQUEST(_r, _idx) \ > >> (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) > >> > >> So, even if the counter has a bogus number it will not last long > > Hm, this prevents from accessing outside the ring buffer, but does it > > change the loop behavior? > no, it doesn't > > Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below > > would still consume the whole 32bit counts, no? > > > > for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { > > resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > > ... > > } > You are right here and the comment is totally valid. > I'll put an additional check like here [1] and here [2] > Will this address your comment? Yep, this kind of sanity checks should work. thanks, Takashi > > > > Takashi > Thank you, > Oleksandr > > [1] > https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1127 > [2] > https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1135 > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On 04/24/2018 06:02 PM, Takashi Iwai wrote: On Tue, 24 Apr 2018 16:58:43 +0200, Oleksandr Andrushchenko wrote: On 04/24/2018 05:35 PM, Takashi Iwai wrote: On Tue, 24 Apr 2018 16:29:15 +0200, Oleksandr Andrushchenko wrote: On 04/24/2018 05:20 PM, Takashi Iwai wrote: On Mon, 16 Apr 2018 08:24:51 +0200, Oleksandr Andrushchenko wrote: +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) +{ + struct xen_snd_front_evtchnl *channel = dev_id; + struct xen_snd_front_info *front_info = channel->front_info; + struct xensnd_resp *resp; + RING_IDX i, rp; + unsigned long flags; + + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) + return IRQ_HANDLED; + + spin_lock_irqsave(&front_info->io_lock, flags); + +again: + rp = channel->u.req.ring.sring->rsp_prod; + /* ensure we see queued responses up to rp */ + rmb(); + + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { I'm not familiar with Xen stuff in general, but through a quick glance, this kind of code worries me a bit. If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a very long loop, no? Better to have a sanity check of the ring buffer size. In this loop I have: resp = RING_GET_RESPONSE(&channel->u.req.ring, i); and the RING_GET_RESPONSE macro is designed in the way that it wraps around when *i* in the question gets bigger than the ring size: #define RING_GET_REQUEST(_r, _idx) \ (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) So, even if the counter has a bogus number it will not last long Hm, this prevents from accessing outside the ring buffer, but does it change the loop behavior? no, it doesn't Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below would still consume the whole 32bit counts, no? for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { resp = RING_GET_RESPONSE(&channel->u.req.ring, i); ... } You are right here and the comment is totally valid. I'll put an additional check like here [1] and here [2] Will this address your comment? Yep, this kind of sanity checks should work. Great, will implement the checks this way then thanks, Takashi Thank you, Oleksandr Takashi Thank you, Oleksandr [1] https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1127 [2] https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1135 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On 04/24/2018 07:23 PM, Oleksandr Andrushchenko wrote: On 04/24/2018 06:02 PM, Takashi Iwai wrote: On Tue, 24 Apr 2018 16:58:43 +0200, Oleksandr Andrushchenko wrote: On 04/24/2018 05:35 PM, Takashi Iwai wrote: On Tue, 24 Apr 2018 16:29:15 +0200, Oleksandr Andrushchenko wrote: On 04/24/2018 05:20 PM, Takashi Iwai wrote: On Mon, 16 Apr 2018 08:24:51 +0200, Oleksandr Andrushchenko wrote: +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) +{ + struct xen_snd_front_evtchnl *channel = dev_id; + struct xen_snd_front_info *front_info = channel->front_info; + struct xensnd_resp *resp; + RING_IDX i, rp; + unsigned long flags; + + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) + return IRQ_HANDLED; + + spin_lock_irqsave(&front_info->io_lock, flags); + +again: + rp = channel->u.req.ring.sring->rsp_prod; + /* ensure we see queued responses up to rp */ + rmb(); + + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { I'm not familiar with Xen stuff in general, but through a quick glance, this kind of code worries me a bit. If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a very long loop, no? Better to have a sanity check of the ring buffer size. In this loop I have: resp = RING_GET_RESPONSE(&channel->u.req.ring, i); and the RING_GET_RESPONSE macro is designed in the way that it wraps around when *i* in the question gets bigger than the ring size: #define RING_GET_REQUEST(_r, _idx) \ (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) So, even if the counter has a bogus number it will not last long Hm, this prevents from accessing outside the ring buffer, but does it change the loop behavior? no, it doesn't Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below would still consume the whole 32bit counts, no? for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { resp = RING_GET_RESPONSE(&channel->u.req.ring, i); ... } You are right here and the comment is totally valid. I'll put an additional check like here [1] and here [2] Will this address your comment? Yep, this kind of sanity checks should work. Great, will implement the checks this way then Well, after thinking a bit more on that and chatting on #xendevel IRC with Juergen (he is on CC list), it seems that the way the code is now it is all fine without the checks: the assumption here is that the backend is trusted to always write sane values to the ring counters, thus no overflow checks on frontend side are required. Even if I implement the checks then I have no means to recover, but just print an error message and bail out not handling any responses. This is probably why the checks [1] and [2] are only implemented for the backend side and there are no such macros for the frontend side. Takashi, please let me know if the above sounds reasonable and addresses your comments. thanks, Takashi Thank you, Oleksandr Takashi Thank you, Oleksandr [1] https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1127 [2] https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1135 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On Wed, 25 Apr 2018 10:26:34 +0200, Oleksandr Andrushchenko wrote: > > On 04/24/2018 07:23 PM, Oleksandr Andrushchenko wrote: > > On 04/24/2018 06:02 PM, Takashi Iwai wrote: > >> On Tue, 24 Apr 2018 16:58:43 +0200, > >> Oleksandr Andrushchenko wrote: > >>> On 04/24/2018 05:35 PM, Takashi Iwai wrote: > On Tue, 24 Apr 2018 16:29:15 +0200, > Oleksandr Andrushchenko wrote: > > On 04/24/2018 05:20 PM, Takashi Iwai wrote: > >> On Mon, 16 Apr 2018 08:24:51 +0200, > >> Oleksandr Andrushchenko wrote: > >>> +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) > >>> +{ > >>> + struct xen_snd_front_evtchnl *channel = dev_id; > >>> + struct xen_snd_front_info *front_info = channel->front_info; > >>> + struct xensnd_resp *resp; > >>> + RING_IDX i, rp; > >>> + unsigned long flags; > >>> + > >>> + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) > >>> + return IRQ_HANDLED; > >>> + > >>> + spin_lock_irqsave(&front_info->io_lock, flags); > >>> + > >>> +again: > >>> + rp = channel->u.req.ring.sring->rsp_prod; > >>> + /* ensure we see queued responses up to rp */ > >>> + rmb(); > >>> + > >>> + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { > >> I'm not familiar with Xen stuff in general, but through a quick > >> glance, this kind of code worries me a bit. > >> > >> If channel->u.req.ring.rsp_cons has a bogus number, this may > >> lead to a > >> very long loop, no? Better to have a sanity check of the ring > >> buffer > >> size. > > In this loop I have: > > resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > > and the RING_GET_RESPONSE macro is designed in the way that > > it wraps around when *i* in the question gets bigger than > > the ring size: > > > > #define RING_GET_REQUEST(_r, _idx) \ > > (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) > > > > So, even if the counter has a bogus number it will not last long > Hm, this prevents from accessing outside the ring buffer, but does it > change the loop behavior? > >>> no, it doesn't > Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below > would still consume the whole 32bit counts, no? > > for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { > resp = RING_GET_RESPONSE(&channel->u.req.ring, i); > ... > } > >>> You are right here and the comment is totally valid. > >>> I'll put an additional check like here [1] and here [2] > >>> Will this address your comment? > >> Yep, this kind of sanity checks should work. > >> > > Great, will implement the checks this way then > Well, after thinking a bit more on that and chatting on #xendevel IRC > with Juergen (he is on CC list), it seems that the way the code is now > it is all fine without the checks: the assumption here is that > the backend is trusted to always write sane values to the ring counters, > thus no overflow checks on frontend side are required. > Even if I implement the checks then I have no means to recover, but > just print > an error message and bail out not handling any responses. > This is probably why the checks [1] and [2] are only implemented for the > backend side and there are no such macros for the frontend side. > > Takashi, please let me know if the above sounds reasonable and > addresses your comments. If it's guaranteed to work, that's OK. But maybe it's worth to comment for readers. thanks, Takashi ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling
On 04/25/2018 12:02 PM, Takashi Iwai wrote: On Wed, 25 Apr 2018 10:26:34 +0200, Oleksandr Andrushchenko wrote: On 04/24/2018 07:23 PM, Oleksandr Andrushchenko wrote: On 04/24/2018 06:02 PM, Takashi Iwai wrote: On Tue, 24 Apr 2018 16:58:43 +0200, Oleksandr Andrushchenko wrote: On 04/24/2018 05:35 PM, Takashi Iwai wrote: On Tue, 24 Apr 2018 16:29:15 +0200, Oleksandr Andrushchenko wrote: On 04/24/2018 05:20 PM, Takashi Iwai wrote: On Mon, 16 Apr 2018 08:24:51 +0200, Oleksandr Andrushchenko wrote: +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id) +{ + struct xen_snd_front_evtchnl *channel = dev_id; + struct xen_snd_front_info *front_info = channel->front_info; + struct xensnd_resp *resp; + RING_IDX i, rp; + unsigned long flags; + + if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED)) + return IRQ_HANDLED; + + spin_lock_irqsave(&front_info->io_lock, flags); + +again: + rp = channel->u.req.ring.sring->rsp_prod; + /* ensure we see queued responses up to rp */ + rmb(); + + for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { I'm not familiar with Xen stuff in general, but through a quick glance, this kind of code worries me a bit. If channel->u.req.ring.rsp_cons has a bogus number, this may lead to a very long loop, no? Better to have a sanity check of the ring buffer size. In this loop I have: resp = RING_GET_RESPONSE(&channel->u.req.ring, i); and the RING_GET_RESPONSE macro is designed in the way that it wraps around when *i* in the question gets bigger than the ring size: #define RING_GET_REQUEST(_r, _idx) \ (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req)) So, even if the counter has a bogus number it will not last long Hm, this prevents from accessing outside the ring buffer, but does it change the loop behavior? no, it doesn't Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below would still consume the whole 32bit counts, no? for (i = channel->u.req.ring.rsp_cons; i != rp; i++) { resp = RING_GET_RESPONSE(&channel->u.req.ring, i); ... } You are right here and the comment is totally valid. I'll put an additional check like here [1] and here [2] Will this address your comment? Yep, this kind of sanity checks should work. Great, will implement the checks this way then Well, after thinking a bit more on that and chatting on #xendevel IRC with Juergen (he is on CC list), it seems that the way the code is now it is all fine without the checks: the assumption here is that the backend is trusted to always write sane values to the ring counters, thus no overflow checks on frontend side are required. Even if I implement the checks then I have no means to recover, but just print an error message and bail out not handling any responses. This is probably why the checks [1] and [2] are only implemented for the backend side and there are no such macros for the frontend side. Takashi, please let me know if the above sounds reasonable and addresses your comments. If it's guaranteed to work, that's OK. But maybe it's worth to comment for readers. ok, will put a comment on that thanks, Takashi Thank you, Oleksandr ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel