RE: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload

-----Original Message-----
From: dan.j.williams@gmail.com [mailto:dan.j.williams@gmail.com] On
Behalf Of Dan Williams
Sent: Sunday, September 02, 2012 4:12 PM
To: Liu Qiang-B32616
Cc: linux-crypto@vger.kernel.org; herbert@gondor.hengli.com.au;
davem@davemloft.net; linux-kernel@vger.kernel.org; linuxppc-
dev@lists.ozlabs.org; Li Yang-R58472; Phillips Kim-R1AAHA;
vinod.koul@intel.com; dan.j.williams@intel.com; arnd@arndb.de;
gregkh@linuxfoundation.org
Subject: Re: [PATCH v7 1/8] Talitos: Support for async_tx XOR offload
=20
On Thu, Aug 9, 2012 at 1:20 AM,  [off-list ref] wrote:

quoted

From: Qiang Liu <redacted>

Expose Talitos's XOR functionality to be used for RAID parity
calculation via the Async_tx layer.

Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Dipen Dudhat <redacted>
Signed-off-by: Maneesh Gupta <redacted>
Signed-off-by: Kim Phillips <redacted>
Signed-off-by: Vishnu Suresh <redacted>
Signed-off-by: Qiang Liu <redacted>
---
 drivers/crypto/Kconfig   |    9 +
 drivers/crypto/talitos.c |  413

++++++++++++++++++++++++++++++++++++++++++++++

quoted

 drivers/crypto/talitos.h |   53 ++++++
 3 files changed, 475 insertions(+), 0 deletions(-)

diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
index be6b2ba..f0a7c29 100644
--- a/drivers/crypto/Kconfig
+++ b/drivers/crypto/Kconfig

@@ -222,6 +222,15 @@ config CRYPTO_DEV_TALITOS
          To compile this driver as a module, choose M here: the module
          will be called talitos.

+config CRYPTO_DEV_TALITOS_RAIDXOR
+       bool "Talitos RAID5 XOR Calculation Offload"
+       default y

=20
No, default y.  The user should explicitly enable this.

Ok, I will change it to N.

=20

quoted

+       select DMA_ENGINE
+       depends on CRYPTO_DEV_TALITOS
+       help
+         Say 'Y' here to use the Freescale Security Engine (SEC) to
+         offload RAID XOR parity Calculation
+

=20
Is it faster than cpu xor?

Yes, I tested with IOZone, cpu load is also reduced.
I think one possible reason is the processor of p1022ds is not strong as ot=
hers, so the performance is improved obviously.

=20
Will this engine be coordinating with another to handle memory copies?
 The dma mapping code for async_tx/raid is broken when dma mapping
requests overlap or cross dma device boundaries [1].
=20
[1]: http://marc.info/?l=3Dlinux-arm-kernel&m=3D129407269402930&w=3D2

Yes, it needs fsl-dma to handle memcpy copies.
I read your link, the unmap address is stored in talitos hwdesc, the addres=
s will be unmapped when async_tx ack this descriptor, I know fsl-dma won't =
wait this ack flag in current kernel, so I fix it in fsl-dma patch 5/8. Do =
you mean that?

=20

quoted

+static void talitos_process_pending(struct talitos_xor_chan *xor_chan)
+{
+       struct talitos_xor_desc *desc, *_desc;
+       unsigned long flags;
+       int status;
+       struct talitos_private *priv;
+       int ch;
+
+       priv =3D dev_get_drvdata(xor_chan->dev);
+       ch =3D atomic_inc_return(&priv->last_chan) &
+                                 (priv->num_channels - 1);

=20
Maybe a comment about why this this is duplicated from
talitos_cra_init()?  It sticks out here and I had to go looking to
find out why this channel number increments on submit.

My fault, I will correct it as below,

atomic_read((&priv->last_chan) & (priv->num_channels - 1);

=20
=20

quoted

+       spin_lock_irqsave(&xor_chan->desc_lock, flags);
+
+       list_for_each_entry_safe(desc, _desc, &xor_chan->pending_q,

node) {

quoted

+               status =3D talitos_submit(xor_chan->dev, ch, &desc-
hwdesc,
+                                       talitos_release_xor, desc);
+               if (status !=3D -EINPROGRESS)
+                       break;
+
+               list_del(&desc->node);
+               list_add_tail(&desc->node, &xor_chan->in_progress_q);
+       }
+
+       spin_unlock_irqrestore(&xor_chan->desc_lock, flags);
+}
+
+static void talitos_xor_run_tx_complete_actions(struct

talitos_xor_desc *desc,

quoted

+               struct talitos_xor_chan *xor_chan)
+{
+       struct device *dev =3D xor_chan->dev;
+       dma_addr_t dest, addr;
+       unsigned int src_cnt =3D desc->unmap_src_cnt;
+       unsigned int len =3D desc->unmap_len;
+       enum dma_ctrl_flags flags =3D desc->async_tx.flags;
+       struct dma_async_tx_descriptor *tx =3D &desc->async_tx;
+
+       /* unmap dma addresses */
+       dest =3D desc->hwdesc.ptr[6].ptr;
+       if (likely(!(flags & DMA_COMPL_SKIP_DEST_UNMAP)))
+               dma_unmap_page(dev, dest, len, DMA_BIDIRECTIONAL);
+
+       desc->idx =3D 6 - src_cnt;
+       if (likely(!(flags & DMA_COMPL_SKIP_SRC_UNMAP))) {
+               while(desc->idx < 6) {

=20
Checkpatch says:
ERROR: space required before the open parenthesis '('

I will correct it next.

=20
=20

quoted

+                       addr =3D desc->hwdesc.ptr[desc->idx++].ptr;
+                       if (addr =3D=3D dest)
+                               continue;
+                       dma_unmap_page(dev, addr, len, DMA_TO_DEVICE);
+               }
+       }
+
+       /* run dependent operations */
+       dma_run_dependencies(tx);

=20
Here is where we run into problems if another engine accesses these
same buffers, especially on ARM v6+.

Do you mean in dma_run_dependencies() will occur to the async_tx descriptor=
 of fsl-dma will be processed? I'm not clearly.

=20

quoted

+}
+
+static void talitos_release_xor(struct device *dev, struct

talitos_desc *hwdesc,

quoted

+                               void *context, int error)
+{
+       struct talitos_xor_desc *desc =3D context;
+       struct talitos_xor_chan *xor_chan;
+       dma_async_tx_callback callback;
+       void *callback_param;
+
+       if (unlikely(error))
+               dev_err(dev, "xor operation: talitos error %d\n",

error);

quoted

+
+       xor_chan =3D container_of(desc->async_tx.chan, struct

talitos_xor_chan,

quoted

+                               common);
+       spin_lock_bh(&xor_chan->desc_lock);
+       if (xor_chan->completed_cookie < desc->async_tx.cookie)
+               xor_chan->completed_cookie =3D desc->async_tx.cookie;
+

=20
Use dma_cookie_complete().

Ok, I will correct it.

=20

quoted

+       callback =3D desc->async_tx.callback;
+       callback_param =3D desc->async_tx.callback_param;
+
+       if (callback) {
+               spin_unlock_bh(&xor_chan->desc_lock);
+               callback(callback_param);
+               spin_lock_bh(&xor_chan->desc_lock);

=20
As mentioned you'll either need to ensure that
talitos_process_pending() is only called from the tasklet, or upgrade
these locks to hardirq safe.

The point is flush_channel can be called both of interrupt and tasklet.
So the process should be redesigned that make sure talitos_process_pending(=
) is only called from tasklet. I will fix it in next series.
Thanks.

=20

quoted

+       }
+
+       talitos_xor_run_tx_complete_actions(desc, xor_chan);
+
+       list_del(&desc->node);
+       list_add_tail(&desc->node, &xor_chan->free_desc);
+       spin_unlock_bh(&xor_chan->desc_lock);
+       if (!list_empty(&xor_chan->pending_q))
+               talitos_process_pending(xor_chan);
+}
+
+/**
+ * talitos_issue_pending - move the descriptors in submit
+ * queue to pending queue and submit them for processing
+ * @chan: DMA channel
+ */
+static void talitos_issue_pending(struct dma_chan *chan)
+{
+       struct talitos_xor_chan *xor_chan;
+
+       xor_chan =3D container_of(chan, struct talitos_xor_chan, common=

);

quoted

+       spin_lock_bh(&xor_chan->desc_lock);
+       list_splice_tail_init(&xor_chan->submit_q,
+                                &xor_chan->pending_q);
+       spin_unlock_bh(&xor_chan->desc_lock);
+       talitos_process_pending(xor_chan);
+}
+
+static dma_cookie_t talitos_async_tx_submit(struct

dma_async_tx_descriptor *tx)

quoted

+{
+       struct talitos_xor_desc *desc;
+       struct talitos_xor_chan *xor_chan;
+       dma_cookie_t cookie;
+
+       desc =3D container_of(tx, struct talitos_xor_desc, async_tx);
+       xor_chan =3D container_of(tx->chan, struct talitos_xor_chan,

common);

quoted

+
+       spin_lock_bh(&xor_chan->desc_lock);
+
+       cookie =3D xor_chan->common.cookie + 1;
+       if (cookie < 0)
+               cookie =3D 1;

=20
Should use the new dma_cookie_assign() helper.

Ok.

=20

quoted

+
+       desc->async_tx.cookie =3D cookie;
+       xor_chan->common.cookie =3D desc->async_tx.cookie;
+
+       list_splice_tail_init(&desc->tx_list,
+                                &xor_chan->submit_q);
+
+       spin_unlock_bh(&xor_chan->desc_lock);
+
+       return cookie;
+}
+
+static struct talitos_xor_desc *talitos_xor_alloc_descriptor(
+                               struct talitos_xor_chan *xor_chan,

gfp_t flags)

quoted

+{
+       struct talitos_xor_desc *desc;
+
+       desc =3D kmalloc(sizeof(*desc), flags);
+       if (desc) {
+               xor_chan->total_desc++;
+               desc->async_tx.tx_submit =3D talitos_async_tx_submit;
+       }
+
+       return desc;
+}
+

[..]

I will remove this line.

quoted

+
+static struct dma_async_tx_descriptor *talitos_prep_dma_xor(
+                       struct dma_chan *chan, dma_addr_t dest,

dma_addr_t *src,

quoted

+                       unsigned int src_cnt, size_t len, unsigned long

flags)

quoted

+{
+       struct talitos_xor_chan *xor_chan;
+       struct talitos_xor_desc *new;
+       struct talitos_desc *desc;
+       int i, j;
+
+       BUG_ON(len > TALITOS_MAX_DATA_LEN);
+
+       xor_chan =3D container_of(chan, struct talitos_xor_chan, common=

);

quoted

+
+       spin_lock_bh(&xor_chan->desc_lock);
+       if (!list_empty(&xor_chan->free_desc)) {
+               new =3D container_of(xor_chan->free_desc.next,
+                                  struct talitos_xor_desc, node);
+               list_del(&new->node);
+       } else {
+                new =3D talitos_xor_alloc_descriptor(xor_chan,

GFP_KERNEL | GFP_DMA);
=20
You can't hold a spin_lock over a GFP_KERNEL allocation.

Ok, I will fix it in next.

=20

quoted

+       }
+       spin_unlock_bh(&xor_chan->desc_lock);
+
+       if (!new) {
+               dev_err(xor_chan->common.device->dev,
+                       "No free memory for XOR DMA descriptor\n");
+               return NULL;
+       }
+       dma_async_tx_descriptor_init(&new->async_tx, &xor_chan->common)=

;

quoted

+
+       INIT_LIST_HEAD(&new->node);
+       INIT_LIST_HEAD(&new->tx_list);
+
+       desc =3D &new->hwdesc;
+       /* Set destination: Last pointer pair */
+       to_talitos_ptr(&desc->ptr[6], dest);
+       desc->ptr[6].len =3D cpu_to_be16(len);
+       desc->ptr[6].j_extent =3D 0;
+       new->unmap_src_cnt =3D src_cnt;
+       new->unmap_len =3D len;
+
+       /* Set Sources: End loading from second-last pointer pair */
+       for (i =3D 5, j =3D 0; j < src_cnt && i >=3D 0; i--, j++) {
+               to_talitos_ptr(&desc->ptr[i], src[j]);
+               desc->ptr[i].len =3D cpu_to_be16(len);
+               desc->ptr[i].j_extent =3D 0;
+       }
+
+       /*
+        * documentation states first 0 ptr/len combo marks end of

sources

quoted

+        * yet device produces scatter boundary error unless all

subsequent

quoted

+        * sources are zeroed out
+        */
+       for (; i >=3D 0; i--) {
+               to_talitos_ptr(&desc->ptr[i], 0);
+               desc->ptr[i].len =3D 0;
+               desc->ptr[i].j_extent =3D 0;
+       }
+
+       desc->hdr =3D DESC_HDR_SEL0_AESU | DESC_HDR_MODE0_AESU_XOR |
+               DESC_HDR_TYPE_RAID_XOR;
+
+       new->async_tx.parent =3D NULL;
+       new->async_tx.next =3D NULL;
+       new->async_tx.cookie =3D 0;
+       async_tx_ack(&new->async_tx);
+
+       list_add_tail(&new->node, &new->tx_list);
+
+       new->async_tx.flags =3D flags;
+       new->async_tx.cookie =3D -EBUSY;
+
+       return &new->async_tx;
+}
+
+static void talitos_unregister_async_xor(struct device *dev)
+{
+       struct talitos_private *priv =3D dev_get_drvdata(dev);
+       struct talitos_xor_chan *xor_chan;
+       struct dma_chan *chan, *_chan;
+
+       if (priv->dma_dev_common.chancnt)
+               dma_async_device_unregister(&priv->dma_dev_common);
+
+       list_for_each_entry_safe(chan, _chan, &priv-
dma_dev_common.channels,
+                               device_node) {
+               xor_chan =3D container_of(chan, struct talitos_xor_chan=

,

quoted

+                                       common);
+               list_del(&chan->device_node);
+               priv->dma_dev_common.chancnt--;
+               kfree(xor_chan);
+       }
+}
+
+/**
+ * talitos_register_dma_async - Initialize the Freescale XOR ADMA

device

quoted

+ * It is registered as a DMA device with the capability to perform
+ * XOR operation with the Async_tx layer.
+ * The various queues and channel resources are also allocated.
+ */
+static int talitos_register_async_tx(struct device *dev, int

max_xor_srcs)

quoted

+{
+       struct talitos_private *priv =3D dev_get_drvdata(dev);
+       struct dma_device *dma_dev =3D &priv->dma_dev_common;
+       struct talitos_xor_chan *xor_chan;
+       int err;
+
+       xor_chan =3D kzalloc(sizeof(struct talitos_xor_chan), GFP_KERNE=

L);

quoted

+       if (!xor_chan) {
+               dev_err(dev, "unable to allocate xor channel\n");
+               return -ENOMEM;
+       }
+
+       dma_dev->dev =3D dev;
+       dma_dev->device_alloc_chan_resources =3D

talitos_alloc_chan_resources;

quoted

+       dma_dev->device_free_chan_resources =3D

talitos_free_chan_resources;

quoted

+       dma_dev->device_prep_dma_xor =3D talitos_prep_dma_xor;
+       dma_dev->max_xor =3D max_xor_srcs;
+       dma_dev->device_tx_status =3D talitos_is_tx_complete;
+       dma_dev->device_issue_pending =3D talitos_issue_pending;
+       INIT_LIST_HEAD(&dma_dev->channels);
+       dma_cap_set(DMA_XOR, dma_dev->cap_mask);
+
+       xor_chan->dev =3D dev;
+       xor_chan->common.device =3D dma_dev;
+       xor_chan->total_desc =3D 0;
+       INIT_LIST_HEAD(&xor_chan->submit_q);
+       INIT_LIST_HEAD(&xor_chan->pending_q);
+       INIT_LIST_HEAD(&xor_chan->in_progress_q);
+       INIT_LIST_HEAD(&xor_chan->free_desc);
+       spin_lock_init(&xor_chan->desc_lock);
+
+       list_add_tail(&xor_chan->common.device_node, &dma_dev-
channels);
+       dma_dev->chancnt++;
+
+       err =3D dma_async_device_register(dma_dev);
+       if (err) {
+               dev_err(dev, "Unable to register XOR with Async_tx\n");
+               goto err_out;
+       }
+
+       return err;
+
+err_out:
+       talitos_unregister_async_xor(dev);
+       return err;
+}
+#endif
+
 /*
  * crypto alg
  */

@@ -2891,6 +3284,26 @@ static int talitos_probe(struct platform_device

*ofdev)

quoted

                        dev_info(dev, "hwrng\n");
        }

+#ifdef CONFIG_CRYPTO_DEV_TALITOS_RAIDXOR

=20
Kill these ifdefs in the C file.  Do something like: if
(xor_enabled()) { } around the code sections that are optional so you
always get compile coverage.  Where xor_enabled() is a shorter form of
IS_ENABLED(CONFIG_CRYPTO_DEV_TALITOS_RAIDXOR).

I'm confused how to implement this interface,
First, this feature should be added in Kconfig (default N)
Second, the only judge condition is SEC version, but some time we don't wan=
t involve function XOR even though the SEC (version is not higher than 3.0,=
 SEC 4.0 is CAAM module) support this feature.
Last, there is not any hardware support.
Can you give me an example? Thanks.