linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: AnilKumar Chimata <anilc@codeaurora.org>
To: Randy Dunlap <rdunlap@infradead.org>
Cc: andy.gross@linaro.org, david.brown@linaro.org,
	robh+dt@kernel.org, mark.rutland@arm.com,
	herbert@gondor.apana.org.au, davem@davemloft.net,
	linux-soc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/3] crypto: qce: ice: Add support for Inline Crypto Engine
Date: Wed, 24 Oct 2018 20:13:41 +0530	[thread overview]
Message-ID: <fe8cd0a3-30d9-34ee-d0b9-2f1ddb655d70@codeaurora.org> (raw)
In-Reply-To: <48c7da0d-4e57-bc36-a683-94088b3b8319@infradead.org>

Hi Randy,

Thanks for your comments,

On 2018-10-17 23:09, Randy Dunlap wrote:
> On 10/17/18 8:17 AM, AnilKumar Chimata wrote:
>> This patch adds support for Inline Crypto Engine (ICE), which
>> is embedded into storage device/controller such as UFS/eMMC.
>> ICE is intended for high throughput cryptographic encryption
>> or decryption of storage data.
>> 
>> Signed-off-by: AnilKumar Chimata <anilc@codeaurora.org>
>> ---
>>  Documentation/crypto/msm/ice.txt |  235 ++++++
>>  drivers/crypto/Kconfig           |   10 +
>>  drivers/crypto/qce/Makefile      |    1 +
>>  drivers/crypto/qce/ice.c         | 1613 
>> ++++++++++++++++++++++++++++++++++++++
>>  drivers/crypto/qce/iceregs.h     |  159 ++++
>>  include/crypto/ice.h             |   80 ++
>>  6 files changed, 2098 insertions(+)
>>  create mode 100644 Documentation/crypto/msm/ice.txt
>>  create mode 100644 drivers/crypto/qce/ice.c
>>  create mode 100644 drivers/crypto/qce/iceregs.h
>>  create mode 100644 include/crypto/ice.h
>> 
>> diff --git a/Documentation/crypto/msm/ice.txt 
>> b/Documentation/crypto/msm/ice.txt
>> new file mode 100644
>> index 0000000..58f7081
>> --- /dev/null
>> +++ b/Documentation/crypto/msm/ice.txt
>> @@ -0,0 +1,235 @@
>> +Introduction:
>> +=============
>> +Storage encryption has been one of the most required feature from 
>> security
> 
>                                                         features

changed.

> 
>> +point of view. QTI based storage encryption solution uses general 
>> purpose
>> +crypto engine. While this kind of solution provide a decent amount of
> 
>                                               provides

changed.

> 
>> +performance, it falls short as storage speed is improving 
>> significantly
>> +continuously. To overcome performance degradation, newer chips are 
>> going to
>> +have Inline Crypto Engine (ICE) embedded into storage device. ICE is 
>> supposed
>> +to meet the line speed of storage devices.
>> +
>> +Hardware Description
>> +====================
>> +ICE is a HW block that is embedded into storage device such as 
>> UFS/eMMC. By
> 
> s/HW/hardware/                                     devices

changed.

> 
>> +default, ICE works in bypass mode i.e. ICE HW does not perform any 
>> crypto
>> +operation on data to be processed by storage device. If required, ICE 
>> can be
>> +configured to perform crypto operation in one direction (i.e. either 
>> encryption
>> +or decryption) or in both direction(both encryption & decryption).
> 
>                              directions (both ...

changed.

> 
>> +
>> +When a switch between the operation modes(plain to crypto or crypto 
>> to plain)
> 
>                                        modes (plain ...

changed.

> 
>> +is desired for a particular partition, SW must complete all 
>> transactions for
> 
> s/SW/software/

changed.

> 
>> +that particular partition before switching the crypto mode i.e. no 
>> crypto, one
> 
>                                                   crypto mode;

changed.

> 
>> +direction crypto or both direction crypto operation. Requests for 
>> other
> 
>                             directions

changed.

> 
>> +partitions are not impacted due to crypto mode switch.
>> +
>> +ICE HW currently supports AES128/256 bit ECB & XTS mode encryption 
>> algorithms.
>> +
>> +Keys for crypto operations are loaded from SW. Keys are stored in a 
>> lookup
>> +table(LUT) located inside ICE HW. Maximum of 32 keys can be loaded in 
>> ICE key
>> +LUT. A Key inside the LUT can be referred using a key index.
> 
>                                     referred to using

changed.

> 
>> +
>> +SW Description
>> +==============
>> +ICE HW has categorized ICE registers in 2 groups: those which can be 
>> accessed by
>> +only secure side i.e. TZ and those which can be accessed by 
>> non-secure side such
> 
> at least tell the reader what TZ and HLOS mean...

changed.

> 
>> +as HLOS as well. This requires that ICE driver to be split in two 
>> pieces: one
> 
>                                   that the ICE driver be split into
> two pieces: one

changed.

> 
>> +running from TZ space and another from HLOS space.
>> +
>> +ICE driver from TZ would configure keys as requested by HLOS side.
>> +
>> +ICE driver on HLOS side is responsible for initialization of ICE HW.
>> +
>> +SW Architecture Diagram
>> +=======================
>> +Following are all the components involved in the ICE driver for 
>> control path:
>> +
>> ++++++++++++++++++++++++++++++++++++++++++
>> ++               App layer               +
>> ++++++++++++++++++++++++++++++++++++++++++
>> ++             System layer              +
>> ++   ++++++++         +++++++            +
>> ++   + VOLD +         + PFM +            +
>> ++   ++++++++         +++++++            +
>> ++         ||         ||                 +
>> ++         ||         ||                 +
>> ++         \/         \/                 +
>> ++     +++++++++++++++++++++++++++       +
>> ++     + LibQSEECom / cryptfs_hw +       +
>> ++     +++++++++++++++++++++++++++       +
>> ++++++++++++++++++++++++++++++++++++++++++
>> ++             Kernel                    +       +++++++++++++++++
>> ++                                       +       +     KMS       +
>> ++  +++++++  +++++++++++  +++++++++++    +       +++++++++++++++++
>> ++  + ICE +  + Storage +  + QSEECom +    +       +   ICE Driver  +
>> ++++++++++++++++++++++++++++++++++++++++++ <===> +++++++++++++++++
>> +               ||                                    ||
>> +               ||                                    ||
>> +               \/                                    \/
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ++                      Storage Device                           +
>> ++                      ++++++++++++++                           +
>> ++                      +   ICE HW   +                           +
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> +
>> +Use Cases:
>> +----------
>> +a) Device bootup
>> +ICE HW is detected during bootup time and corresponding probe 
>> function is
>> +called. ICE driver parses its data from device tree node. ICE HW and 
>> storage
>> +HW are tightly coupled. Storage device probing is dependent upon ICE 
>> device
>> +probing. ICE driver configures all the required registers to put the 
>> ICE HW
>> +in bypass mode.
>> +
>> +b) Configuring keys
>> +Currently, there are couple of use cases to configure the keys.
>> +
>> +1) Full Disk Encryption(FDE)
>> +System layer(VOLD) at invocation of apps layer would call libqseecom 
>> to create
>> +the encryption key. Libqseecom calls qseecom driver to communicate 
>> with KMS
>> +module on the secure side i.e. TZ. KMS would call ICE driver on the 
>> TZ side to
>> +create and set the keys in ICE HW. At the end of transaction, VOLD 
>> would have
>> +key index of key LUT where encryption key is present.
>> +
>> +2) Per File Encryption (PFE)
>> +Per File Manager(PFM) calls QSEECom api to create the key. PFM has a 
>> peer comp-
> 
> Preferably don't split (hyphenate) "component."  But if you must, it
> should be com-
> ponent.

Taken into account, will follow on future patches. For this patch changed.

> 
>> +onent(PFT) at kernel layer which gets the corresponding key index 
>> from PFM.
>> +
>> +Following are all the components involved in the ICE driver for data 
>> path:
>> +
>> +       +++++++++++++++++++++++++++++++++++++++++
>> +       +               App layer               +
>> +       +++++++++++++++++++++++++++++++++++++++++
>> +       +              VFS                      +
>> +       +---------------------------------------+
>> +       +         File System (EXT4)            +
>> +       +---------------------------------------+
>> +       +             Block Layer               +
>> +       + --------------------------------------+
>> +       +                              +++++++  +
>> +       +              dm-req-crypt => + PFT +  +
>> +       +                              +++++++  +
>> +       +                                       +
>> +       +---------------------------------------+
>> +       +    +++++++++++           +++++++      +
>> +       +    + Storage +           + ICE +      +
>> +       +++++++++++++++++++++++++++++++++++++++++
>> +       +                  ||                   +
>> +       +                  || (Storage Req with +
>> +       +                  \/  ICE parameters ) +
>> +       +++++++++++++++++++++++++++++++++++++++++
>> +       +          Storage Device               +
>> +       +          ++++++++++++++               +
>> +       +          +   ICE HW   +               +
>> +       +++++++++++++++++++++++++++++++++++++++++
>> +
>> +c) Data transaction
>> +Once the crypto key has been configured, VOLD/PFM creates device 
>> mapping for
>> +data partition. As part of device mapping VOLD passes key index, 
>> crypto
>> +algorithm, mode and key length to dm layer. In case of PFE, keys are 
>> provided
>> +by PFT as and when request is processed by dm-req-crypt. When any 
>> application
>> +needs to read/write data, it would go through DM layer which would 
>> add crypto
>> +information, provided by VOLD/PFT, to Request. For each Request, 
>> Storage driver
>> +would ask ICE driver to configure crypto part of request. ICE driver 
>> extracts
>> +crypto data from Request structure and provide it to storage driver 
>> which would
> 
>                                           provides

changed.

> 
>> +finally dispatch request to storage device.
>> +
>> +d) Error Handling
>> +Due to issue # 1 mentioned in "Known Issues", ICE driver does not 
>> register for
>> +any interrupt. However, it enables sources of interrupt for ICE HW. 
>> After each
>> +data transaction, Storage driver receives transaction completion 
>> event. As part
>> +of event handling, storage driver calls  ICE driver to check if any 
>> of ICE
>> +interrupt status is set. If yes, storage driver returns error to 
>> upper layer.
>> +
>> +Error handling would be changed in future chips.
>> +
>> +Interfaces
>> +==========
>> +ICE driver exposes interfaces for storage driver to :
> 
>                                                     to:

changed.

> 
>> +1. Get the global instance of ICE driver
>> +2. Get the implemented interfaces of the particular ice instance
>> +3. Initialize the ICE HW
>> +4. Reset the ICE HW
>> +5. Resume/Suspend the ICE HW
>> +6. Get the Crypto configuration for the data request for storage
>> +7. Check if current data transaction has generated any interrupt
>> +
>> +Driver Parameters
>> +=================
>> +This driver is built and statically linked into the kernel; 
>> therefore,
>> +there are no module parameters supported by this driver.
>> +
>> +There are no kernel command line parameters supported by this driver.
>> +
>> +Power Management
>> +================
>> +ICE driver does not do power management on its own as it is part of 
>> storage
>> +hardware. Whenever storage driver receives request for power 
>> collapse/suspend
>> +resume, it would call ICE driver which exposes APIs for Storage HW. 
>> ICE HW
>> +during power collapse or reset, wipes crypto configuration data. When 
>> ICE
> 
>                                  ^no comma

changed.

> 
>> +driver receives request to resume, it would ask ICE driver on TZ side 
>> to
>> +restore the configuration. ICE driver does not do anything as part of 
>> power
>> +collapse or suspend event.
>> +
>> +Interface:
>> +==========
>> +ICE driver exposes following APIs for storage driver to use:
>> +
>> +int (*init)(struct platform_device *, void *, ice_success_cb, 
>> ice_error_cb);
>> +	-- This function is invoked by storage controller during 
>> initialization of
>> +	storage controller. Storage controller would provide success and 
>> error call
>> +	backs which would be invoked asynchronously once ICE HW init is 
>> done.
>> +
>> +int (*reset)(struct platform_device *);
>> +	-- ICE HW reset as part of storage controller reset. When storage 
>> controller
>> +	received reset command, it would call reset on ICE HW. As of now, 
>> ICE HW
>> +	does not need to do anything as part of reset.
>> +
>> +int (*resume)(struct platform_device *);
>> +	-- ICE HW while going to reset, wipes all crypto keys and other data 
>> from ICE
> 
>                                       ^no comma

changed.

> 
>> +	HW. ICE driver would reconfigure those data as part of resume 
>> operation.
>> +
>> +int (*suspend)(struct platform_device *);
>> +	-- This API would be called by storage driver when storage device is 
>> going to
>> +	suspend mode. As of today, ICE driver does not do anything to handle 
>> suspend.
>> +
>> +int (*config)(struct platform_device *, struct request* , struct 
>> ice_data_setting*);
>> +	-- Storage driver would call this interface to get all crypto data 
>> required to
>> +	perform crypto operation.
>> +
>> +int (*status)(struct platform_device *);
>> +	-- Storage driver would call this interface to check if previous 
>> data transfer
>> +	generated any error.
>> +
>> +Config options
>> +==============
>> +This driver is enabled by the kernel config option 
>> CONFIG_CRYPTO_DEV_MSM_ICE.
>> +
>> +Dependencies
>> +============
>> +ICE driver depends upon corresponding ICE driver on TZ side to 
>> function
>> +appropriately.
>> +
>> +Known Issues
>> +============
>> +1. ICE HW emits 0s even if it has generated an interrupt
> 
> That statement is unclear.  Emits where?  in a data stream?  in
> interrupt status?

Its interrupt status line unchanged, currently its handled on storage driver

> 
>> +This issue has significant impact on how ICE interrupts are handled. 
>> Currently,
>> +ICE driver does not register for any of the ICE interrupts but 
>> enables the
>> +sources of interrupt. Once storage driver asks to check the status of 
>> interrupt,
>> +it reads and clears the clear status and provide read status to 
>> storage driver.
> 
>                "clears the clear status"???         s/provide/provides/

changed.

> 
>> +This mechanism though not optimal but prevents file-system 
>> corruption.
> 
> mangled sentence above.

changed.

> 
>> +This issue has been fixed in newer chips.
>> +
>> +2. ICE HW wipes all crypto data during power collapse
>> +This issue necessitate that ICE driver on TZ side store the crypto 
>> material
> 
>               necessitates

changed.

> 
>> +which is not required in the case of general purpose crypto engine.
>> +This issue has been fixed in newer chips.
>> +
>> +Further Improvements
>> +====================
>> +Currently, Due to PFE use case, ICE driver is dependent upon 
>> dm-req-crypt to
> 
>               due

changed.

> 
>> +provide the keys as part of request structure. This couples ICE 
>> driver with
>> +dm-req-crypt based solution. It is under discussion to expose an 
>> IOCTL based
> 
>                                                        to expose IOCTL 
> based

changed.

> 
>> +and registration based interface APIs from ICE driver. ICE driver 
>> would use
>> +these two interfaces to find out if any key exists for current 
>> request. If
>> +yes, choose the right key index received from IOCTL or registration 
>> based
>> +APIs. If not, dont set any crypto parameter in the request.
> 
>                  don't

changed.

> 
>> diff --git a/drivers/crypto/Kconfig b/drivers/crypto/Kconfig
>> index a8c4ce0..e40750e 100644
>> --- a/drivers/crypto/Kconfig
>> +++ b/drivers/crypto/Kconfig
>> @@ -596,6 +596,16 @@ config CRYPTO_DEV_QCOM_RNG
>>  	  To compile this driver as a module, choose M here. The
>>            module will be called qcom-rng. If unsure, say N.
>> 
>> +config CRYPTO_DEV_QTI_ICE
>> +	tristate "Qualcomm inline crypto engine accelerator"
>> +	default n
> 
> Please drop the redundant "default n" since n is already the default.

Agree, will drop this change.

> 
>> +	depends on (ARCH_QCOM || COMPILE_TEST) && BLK_DEV_DM
>> +	help
>> +	  This driver supports Inline Crypto Engine for QTI chipsets, 
>> MSM8994
>> +	  and later, to accelerate crypto operations for storage needs.
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called ice.
>> +
>>  config CRYPTO_DEV_VMX
>>  	bool "Support for VMX cryptographic acceleration instructions"
>>  	depends on PPC64 && VSX

  reply	other threads:[~2018-10-24 14:43 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-17 15:17 [PATCH 0/3] Add Inline Crypto Engine (ICE) driver AnilKumar Chimata
2018-10-17 15:17 ` [PATCH 1/3] firmware: qcom: scm: Update qcom_scm_call signature AnilKumar Chimata
2018-10-17 15:17 ` [PATCH 2/3] dt-bindings: Add ICE device specific parameters AnilKumar Chimata
2018-10-25 18:15   ` Rob Herring
2018-10-29 13:30     ` AnilKumar Chimata
2018-10-17 15:17 ` [PATCH 3/3] crypto: qce: ice: Add support for Inline Crypto Engine AnilKumar Chimata
2018-10-17 17:04   ` Theodore Y. Ts'o
2018-10-24 12:04     ` AnilKumar Chimata
2018-10-17 17:39   ` Randy Dunlap
2018-10-24 14:43     ` AnilKumar Chimata [this message]
2018-10-18 11:43   ` kbuild test robot
2018-10-24 11:14     ` anilc
2018-10-25 14:58       ` Rob Herring
2018-10-29 13:31         ` AnilKumar Chimata
2018-10-25 14:55   ` Rob Herring
2018-10-25 15:28     ` Theodore Y. Ts'o
2018-10-25 15:45       ` Rob Herring
2018-10-29 13:47       ` AnilKumar Chimata

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fe8cd0a3-30d9-34ee-d0b9-2f1ddb655d70@codeaurora.org \
    --to=anilc@codeaurora.org \
    --cc=andy.gross@linaro.org \
    --cc=davem@davemloft.net \
    --cc=david.brown@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=herbert@gondor.apana.org.au \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=rdunlap@infradead.org \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH 3/3] crypto: qce: ice: Add support for Inline Crypto Engine' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).