[Xen-devel] [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling

2018-04-15 Thread Oleksandr Andrushchenko 
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

2018-04-16 Thread Juergen Gross 
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

2018-04-16 Thread kbuild test robot 
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

2018-04-17 Thread Oleksandr Andrushchenko 

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

2018-04-17 Thread Juergen Gross 
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

2018-04-17 Thread Oleksandr Andrushchenko 

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

2018-04-24 Thread Takashi Iwai 
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

2018-04-24 Thread Oleksandr Andrushchenko 

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

2018-04-24 Thread Takashi Iwai 
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

2018-04-24 Thread Oleksandr Andrushchenko 

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

2018-04-24 Thread Takashi Iwai 
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

2018-04-24 Thread Oleksandr Andrushchenko 

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

2018-04-25 Thread Oleksandr Andrushchenko 

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

2018-04-25 Thread Takashi Iwai 
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

2018-04-25 Thread Oleksandr Andrushchenko 

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