From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb0-f176.google.com (mail-yb0-f176.google.com [209.85.213.176]) by dpdk.org (Postfix) with ESMTP id 7287B558D for ; Fri, 13 Jan 2017 08:41:39 +0100 (CET) Received: by mail-yb0-f176.google.com with SMTP id l23so12847214ybj.2 for ; Thu, 12 Jan 2017 23:41:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=pVnn7mWXvlmuLC3fDuQRYENpm5ICSYmWEOdj2mvd1cA=; b=ZOQK+jmLSOifJbR9gQjz0gOD4DSLaGJgVo8GDDkbKqlb8gDR6xotMYN1NNP7barBi5 Fbdrp4ZPYM1obBQrF0svepoWGAETVYQlMMZu19sJ4o7oLJemGyUCn1jhHAHrhJ+1qxR6 n+Zqnn+UdiaT49q4e88Ko94tQKEuBC3lXe82g= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=pVnn7mWXvlmuLC3fDuQRYENpm5ICSYmWEOdj2mvd1cA=; b=g8fCFUJrYDWKME3EW+zsMWQJDVZCe6eFdNg9JXcdKkTx1+Sx80XvSKhsDGrPyGLSJv 7Uy8MGpRg4ulFjwW2ENyi1XFjuupXw9FugInDCN1UE3RgoXmdpLl1f+I8zsbbGkEbPnS CzkgarqEaNhzrMgghOxmifwAIlaybQkf7D8SuGhOdS5uBP0bqwGjg+In6NF9o8gXk/FW 35NuNLgO84S7mPtA7NbW6t8bWAxsfhoZhqlhwf5k8w9IoOUIVCY/gwrMHpZ7bCHBwHcC nq4WIv2BY6e1nB1AePKRCFjkQ3FPy/ClHOpzEmdKeO8q5rlk+NJvPo4xJSoC1sJffOx7 xlYQ== X-Gm-Message-State: AIkVDXJ0RnOLQLG7fC5Wpcz+wWS4Hl/N2UooTXhLgYuZqfeKwH1RFJT/WgMMywHrxbsFFDdxmlEINRkQx1WvJy85 X-Received: by 10.37.163.33 with SMTP id d30mr10666014ybi.54.1484293298562; Thu, 12 Jan 2017 23:41:38 -0800 (PST) MIME-Version: 1.0 Received: by 10.37.221.6 with HTTP; Thu, 12 Jan 2017 23:41:38 -0800 (PST) In-Reply-To: <356b7ac5-0196-6181-bfc9-06e5d0d5a227@caviumnetworks.com> References: <1481077985-4224-2-git-send-email-zbigniew.bodek@caviumnetworks.com> <1483551207-18236-1-git-send-email-zbigniew.bodek@caviumnetworks.com> <1483551207-18236-4-git-send-email-zbigniew.bodek@caviumnetworks.com> <356b7ac5-0196-6181-bfc9-06e5d0d5a227@caviumnetworks.com> From: Jianbo Liu Date: Fri, 13 Jan 2017 15:41:38 +0800 Message-ID: To: Zbigniew Bodek Cc: dev@dpdk.org, pablo.de.lara.guarch@intel.com, Declan Doherty , Jerin Jacob Content-Type: text/plain; charset=UTF-8 Subject: Re: [dpdk-dev] [PATCH v3 3/8] crypto/armv8: add PMD optimized for ARMv8 processors X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Jan 2017 07:41:39 -0000 On 12 January 2017 at 21:12, Zbigniew Bodek wrote: > Hello Jianbo Liu, > > Thanks for the review. Please check my answers in-line. > > Kind regards > Zbigniew > > > On 06.01.2017 03:45, Jianbo Liu wrote: >> >> On 5 January 2017 at 01:33, wrote: >>> >>> From: Zbigniew Bodek >>> >>> This patch introduces crypto poll mode driver >>> using ARMv8 cryptographic extensions. >>> CPU compatibility with this driver is detected in >>> run-time and virtual crypto device will not be >>> created if CPU doesn't provide: >>> AES, SHA1, SHA2 and NEON. >>> >>> This PMD is optimized to provide performance boost >>> for chained crypto operations processing, >>> such as encryption + HMAC generation, >>> decryption + HMAC validation. In particular, >>> cipher only or hash only operations are >>> not provided. >>> >>> The driver currently supports AES-128-CBC >>> in combination with: SHA256 HMAC and SHA1 HMAC >>> and relies on the external armv8_crypto library: >>> https://github.com/caviumnetworks/armv8_crypto >>> >> >> It's standalone lib. I think you should change the following line in >> its Makefile, so not depend on DPDK. >> "include $(RTE_SDK)/mk/rte.lib.mk" >> >>> This patch adds driver's code only and does >>> not include it in the build system. >>> >>> Signed-off-by: Zbigniew Bodek >>> --- >>> drivers/crypto/armv8/Makefile | 73 ++ >>> drivers/crypto/armv8/rte_armv8_pmd.c | 926 >>> +++++++++++++++++++++++++ >>> drivers/crypto/armv8/rte_armv8_pmd_ops.c | 369 ++++++++++ >>> drivers/crypto/armv8/rte_armv8_pmd_private.h | 211 ++++++ >>> drivers/crypto/armv8/rte_armv8_pmd_version.map | 3 + >>> 5 files changed, 1582 insertions(+) >>> create mode 100644 drivers/crypto/armv8/Makefile >>> create mode 100644 drivers/crypto/armv8/rte_armv8_pmd.c >>> create mode 100644 drivers/crypto/armv8/rte_armv8_pmd_ops.c >>> create mode 100644 drivers/crypto/armv8/rte_armv8_pmd_private.h >>> create mode 100644 drivers/crypto/armv8/rte_armv8_pmd_version.map >>> ..... >>> + /* Select auth algo */ >>> + switch (auth_xform->auth.algo) { >>> + /* Cover supported hash algorithms */ >>> + case RTE_CRYPTO_AUTH_SHA256: >>> + aalg = auth_xform->auth.algo; >>> + sess->auth.mode = ARMV8_CRYPTO_AUTH_AS_AUTH; >>> + break; >>> + case RTE_CRYPTO_AUTH_SHA1_HMAC: >>> + case RTE_CRYPTO_AUTH_SHA256_HMAC: /* Fall through */ >>> + aalg = auth_xform->auth.algo; >>> + sess->auth.mode = ARMV8_CRYPTO_AUTH_AS_HMAC; >>> + break; >>> + default: >>> + return -EINVAL; >>> + } >>> + >>> + /* Verify supported key lengths and extract proper algorithm */ >>> + switch (cipher_xform->cipher.key.length << 3) { >>> + case 128: >>> + sess->crypto_func = >>> + CRYPTO_GET_ALGO(order, cop, calg, aalg, >>> 128); >>> + sess->cipher.key_sched = >>> + CRYPTO_GET_KEY_SCHED(cop, calg, 128); >>> + break; >>> + case 192: >>> + sess->crypto_func = >>> + CRYPTO_GET_ALGO(order, cop, calg, aalg, >>> 192); >>> + sess->cipher.key_sched = >>> + CRYPTO_GET_KEY_SCHED(cop, calg, 192); >>> + break; >>> + case 256: >>> + sess->crypto_func = >>> + CRYPTO_GET_ALGO(order, cop, calg, aalg, >>> 256); >>> + sess->cipher.key_sched = >>> + CRYPTO_GET_KEY_SCHED(cop, calg, 256); >>> + break; >>> + default: >>> + sess->crypto_func = NULL; >>> + sess->cipher.key_sched = NULL; >>> + return -EINVAL; >>> + } >>> + >>> + if (unlikely(sess->crypto_func == NULL)) { >>> + /* >>> + * If we got here that means that there must be a bug >> >> >> Since AES-128-CBC is only supported in your patch. It means that >> crypto_func could be NULL according to the switch above if >> cipher.key.length > 128? > > > Yes. Instead of checking for key lengths in a similar way that we check for > algorithms, etc. we just fail when we don't find appropriate function. Do > you suggest that this should be changed? > I mean to return error directly if length is not 128 in the above switch, so this "if" is no necessary. > >> >>> + * in the algorithms selection above. Nevertheless keep >>> + * it here to catch bug immediately and avoid NULL >>> pointer >>> + * dereference in OPs processing. >>> + */ >>> + ARMV8_CRYPTO_LOG_ERR( >>> + "No appropriate crypto function for given >>> parameters"); >>> + return -EINVAL; >>> + } >>> + >>> + /* Set up cipher session prerequisites */ >>> + if (cipher_set_prerequisites(sess, cipher_xform) != 0) >>> + return -EINVAL; >>> + >>> + /* Set up authentication session prerequisites */ >>> + if (auth_set_prerequisites(sess, auth_xform) != 0) >>> + return -EINVAL; >>> + >>> + return 0; >>> +} >>> + >> >> >> .... >> >>> diff --git a/drivers/crypto/armv8/rte_armv8_pmd_ops.c >>> b/drivers/crypto/armv8/rte_armv8_pmd_ops.c >>> new file mode 100644 >>> index 0000000..2bf6475 >>> --- /dev/null >>> +++ b/drivers/crypto/armv8/rte_armv8_pmd_ops.c >>> @@ -0,0 +1,369 @@ >>> +/* >>> + * BSD LICENSE >>> + * >>> + * Copyright (C) Cavium networks Ltd. 2017. >>> + * >>> + * Redistribution and use in source and binary forms, with or without >>> + * modification, are permitted provided that the following conditions >>> + * are met: >>> + * >>> + * * Redistributions of source code must retain the above copyright >>> + * notice, this list of conditions and the following disclaimer. >>> + * * Redistributions in binary form must reproduce the above >>> copyright >>> + * notice, this list of conditions and the following disclaimer in >>> + * the documentation and/or other materials provided with the >>> + * distribution. >>> + * * Neither the name of Cavium networks nor the names of its >>> + * contributors may be used to endorse or promote products derived >>> + * from this software without specific prior written permission. >>> + * >>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS >>> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT >>> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS >>> FOR >>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE >>> COPYRIGHT >>> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, >>> INCIDENTAL, >>> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT >>> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF >>> USE, >>> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON >>> ANY >>> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT >>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE >>> USE >>> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH >>> DAMAGE. >>> + */ >>> + >>> +#include >>> + >>> +#include >>> +#include >>> +#include >>> + >>> +#include "armv8_crypto_defs.h" >>> + >>> +#include "rte_armv8_pmd_private.h" >>> + >>> +static const struct rte_cryptodev_capabilities >>> + armv8_crypto_pmd_capabilities[] = { >>> + { /* SHA1 HMAC */ >>> + .op = RTE_CRYPTO_OP_TYPE_SYMMETRIC, >>> + {.sym = { >>> + .xform_type = RTE_CRYPTO_SYM_XFORM_AUTH, >>> + {.auth = { >>> + .algo = >>> RTE_CRYPTO_AUTH_SHA1_HMAC, >>> + .block_size = 64, >>> + .key_size = { >>> + .min = 16, >>> + .max = 128, >>> + .increment = 0 >>> + }, >>> + .digest_size = { >>> + .min = 20, >>> + .max = 20, >>> + .increment = 0 >>> + }, >>> + .aad_size = { 0 } >>> + }, } >>> + }, } >>> + }, >>> + { /* SHA256 HMAC */ >>> + .op = RTE_CRYPTO_OP_TYPE_SYMMETRIC, >>> + {.sym = { >>> + .xform_type = RTE_CRYPTO_SYM_XFORM_AUTH, >>> + {.auth = { >>> + .algo = >>> RTE_CRYPTO_AUTH_SHA256_HMAC, >>> + .block_size = 64, >>> + .key_size = { >>> + .min = 16, >>> + .max = 128, >>> + .increment = 0 >>> + }, >>> + .digest_size = { >>> + .min = 32, >>> + .max = 32, >>> + .increment = 0 >>> + }, >>> + .aad_size = { 0 } >>> + }, } >>> + }, } >>> + }, >>> + { /* AES CBC */ >>> + .op = RTE_CRYPTO_OP_TYPE_SYMMETRIC, >>> + {.sym = { >>> + .xform_type = >>> RTE_CRYPTO_SYM_XFORM_CIPHER, >>> + {.cipher = { >>> + .algo = >>> RTE_CRYPTO_CIPHER_AES_CBC, >>> + .block_size = 16, >>> + .key_size = { >>> + .min = 16, >>> + .max = 16, >>> + .increment = 0 >>> + }, >>> + .iv_size = { >>> + .min = 16, >>> + .max = 16, >>> + .increment = 0 >>> + } >>> + }, } >>> + }, } >>> + }, >>> + >> >> >> It's strange that you defined aes and hmac here, but not implemented >> them, though their combinations are implemented. >> Will you add later? > > > We may add standalone algorithms in the future but those ops here are not > for that purpose. I thought that since there is no chained operations > capability we should export what we can do even though that it will work > (mean not return error) only if the operations are chained. > Do you have some other suggestion? > Nothing special. Either implement them later, or add new chained ops (is that possible?) BTW, can you explain what optimization you have done, so I can better understand your asm code, thanks! > >> >>> + RTE_CRYPTODEV_END_OF_CAPABILITIES_LIST() >>> +}; >>> + >>> +