From: Eric Biggers <ebiggers@kernel.org>
To: Gaurav Kashyap <quic_gaurkash@quicinc.com>
Cc: linux-scsi@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-mmc@vger.kernel.org, linux-block@vger.kernel.org,
linux-fscrypt@vger.kernel.org, thara.gopinath@linaro.org,
quic_neersoni@quicinc.com, dineshg@quicinc.com
Subject: Re: [PATCH 01/10] soc: qcom: new common library for ICE functionality
Date: Mon, 13 Dec 2021 16:20:46 -0800 [thread overview]
Message-ID: <YbfjXtQgLSLOFvBr@gmail.com> (raw)
In-Reply-To: <20211206225725.77512-2-quic_gaurkash@quicinc.com>
On Mon, Dec 06, 2021 at 02:57:16PM -0800, Gaurav Kashyap wrote:
> Add a new library which congregates all the ICE
> functionality so that all storage controllers containing
> ICE can utilize it.
In commit messages and comments, please spell out "Inline Crypto Engine (ICE)"
the first time it appears, so that people know what ICE means.
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 79b568f82a1c..a900f5ab6263 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -209,4 +209,11 @@ config QCOM_APR
> application processor and QDSP6. APR is
> used by audio driver to configure QDSP6
> ASM, ADM and AFE modules.
> +
> +config QTI_ICE_COMMON
> + tristate "QTI common ICE functionality"
Since this is a library, it shouldn't be user-selectable, but rather just be
selected by the other options that need it. Putting a string after "tristate"
makes it user-selectable; the string is the prompt string. The line should just
be "tristate", without a string afterwards.
> diff --git a/drivers/soc/qcom/qti-ice-common.c b/drivers/soc/qcom/qti-ice-common.c
> new file mode 100644
> index 000000000000..0c5b529201c5
> --- /dev/null
> +++ b/drivers/soc/qcom/qti-ice-common.c
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Common ICE library for storage encryption.
> + *
> + * Copyright (c) 2021, Qualcomm Innovation Center. All rights reserved.
> + */
> +
> +#include <linux/qti-ice-common.h>
> +#include <linux/module.h>
> +#include <linux/qcom_scm.h>
> +#include <linux/delay.h>
> +#include "qti-ice-regs.h"
> +
> +#define QTI_ICE_MAX_BIST_CHECK_COUNT 100
> +#define QTI_AES_256_XTS_KEY_RAW_SIZE 64
> +
> +static bool qti_ice_supported(const struct ice_mmio_data *mmio)
> +{
> + u32 regval = qti_ice_readl(mmio->ice_mmio, QTI_ICE_REGS_VERSION);
> + int major = regval >> 24;
> + int minor = (regval >> 16) & 0xFF;
> + int step = regval & 0xFFFF;
> +
> + /* For now this driver only supports ICE version 3 and higher. */
> + if (major < 3) {
> + pr_warn("Unsupported ICE version: v%d.%d.%d\n",
> + major, minor, step);
> + return false;
> + }
For log messages associated with a device, the dev_*() functions should be used
instead of pr_*(). How about including the relevant 'struct device *' in the
struct ice_mmio_data?
This comment applies to everywhere in qti-ice-common that is logging anything.
> +/**
> + * qti_ice_init() - Initialize ICE functionality
> + * @ice_mmio_data: contains ICE register mapping for i/o
> + *
> + * Initialize ICE by checking the version for ICE support and
> + * also checking the fuses blown.
> + *
> + * Return: 0 on success; -EINVAL on failure.
> + */
> +int qti_ice_init(const struct ice_mmio_data *mmio)
> +{
> + if (!qti_ice_supported(mmio))
> + return -EINVAL;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(qti_ice_init);
Be more specific about what "failure" means here. It means that the driver
doesn't support the ICE hardware, right? -ENODEV would be a more appropriate
error code for this. -EINVAL is a very generic error.
> +static void qti_ice_low_power_and_optimization_enable(
> + const struct ice_mmio_data *mmio)
> +{
> + u32 regval = qti_ice_readl(mmio->ice_mmio,
> + QTI_ICE_REGS_ADVANCED_CONTROL);
> +
> + /* Enable low power mode sequence
> + * [0]-0,[1]-0,[2]-0,[3]-7,[4]-0,[5]-0,[6]-0,[7]-0,
> + * Enable CONFIG_CLK_GATING, STREAM2_CLK_GATING and STREAM1_CLK_GATING
> + */
> + regval |= 0x7000;
> + /* Optimization enable sequence*/
> + regval |= 0xD807100;
> + qti_ice_writel(mmio->ice_mmio, regval, QTI_ICE_REGS_ADVANCED_CONTROL);
> + /* Memory barrier - to ensure write completion before next transaction */
> + wmb();
> +}
This part changed slightly from the original code in
drivers/scsi/ufs/ufs-qcom-ice.c and drivers/mmc/host/sdhci-msm.c. What is the
reason for these changes? To be fair, I can't properly explain this part of the
original code; I just did what some existing ICE code was doing. But if it
wasn't the correct or best way, it would be super useful to understand *why*
this different version is really the correct/best way.
Also note that the line "regval |= 0x7000;" is redundant.
> +static int qti_ice_wait_bist_status(const struct ice_mmio_data *mmio)
> +{
> + int count;
> + u32 regval;
> +
> + for (count = 0; count < QTI_ICE_MAX_BIST_CHECK_COUNT; count++) {
> + regval = qti_ice_readl(mmio->ice_mmio,
> + QTI_ICE_REGS_BIST_STATUS);
> + if (!(regval & QTI_ICE_BIST_STATUS_MASK))
> + break;
> + udelay(50);
> + }
> +
> + if (regval) {
> + pr_err("%s: wait bist status failed, reg %d\n",
> + __func__, regval);
> + return -ETIMEDOUT;
> + }
> +
> + return 0;
> +}
The copy of this in drivers/mmc/host/sdhci-msm.c is a bit nicer in that it uses
the readl_poll_timeout() helper function, and it has a longer comment explaining
it. So I think you should use that version rather than the UFS version.
> +/**
> + * qti_ice_keyslot_program() - Program a key to an ICE slot
> + * @ice_mmio_data: contains ICE register mapping for i/o
> + * @crypto_key: key to be program, this can be wrapped or raw
> + * @crypto_key_size: size of the key to be programmed
> + * @slot: the keyslot at which the key should be programmed.
> + * @data_unit_mask: mask for the dun which is part of the
> + * crypto configuration.
> + * @capid: capability index indicating the algorithm for the
> + * crypto configuration
> + *
> + * Program the passed in key to a slot in ICE.
> + * The key that is passed in can either be a raw key or wrapped.
> + * In both cases, due to access control of ICE for Qualcomm chipsets,
> + * a scm call is used to program the key into ICE from trustzone.
> + * Trustzone can differentiate between raw and wrapped keys.
How does TrustZone differentiate between raw and wrapped keys? I thought you
had mentioned that only one is supported at a time on a particular SoC.
> +int qti_ice_keyslot_program(const struct ice_mmio_data *mmio,
> + const u8 *crypto_key, unsigned int crypto_key_size,
> + unsigned int slot, u8 data_unit_mask, int capid)
> +{
> + int err = 0;
> + int i = 0;
> + union {
> + u8 bytes[QTI_AES_256_XTS_KEY_RAW_SIZE];
> + u32 words[QTI_AES_256_XTS_KEY_RAW_SIZE / sizeof(u32)];
> + } key;
> +
> + memcpy(key.bytes, crypto_key, crypto_key_size);
This is assuming that wrapped keys aren't longer than raw AES-256-XTS keys,
which isn't necessarily true.
> +#define qti_ice_writel(mmio, val, reg) \
> + writel_relaxed((val), mmio + (reg))
> +#define qti_ice_readl(mmio, reg) \
> + readl_relaxed(mmio + (reg))
Previously, writel() and readl() were used instead of writel_relaxed() and
readl_relaxed(). What is the reason for this change? My understanding is that
the *_relaxed() functions are harder to use correctly, so they shouldn't be used
unless it's been carefully thought through and the extra performance is needed.
- Eric
next prev parent reply other threads:[~2021-12-14 0:20 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-06 22:57 [PATCH 00/10] Add wrapped key support for Qualcomm ICE Gaurav Kashyap
2021-12-06 22:57 ` [PATCH 01/10] soc: qcom: new common library for ICE functionality Gaurav Kashyap
2021-12-07 0:24 ` Randy Dunlap
2021-12-14 0:20 ` Eric Biggers [this message]
2021-12-06 22:57 ` [PATCH 02/10] scsi: ufs: qcom: move ICE functionality to common library Gaurav Kashyap
2021-12-14 0:40 ` Eric Biggers
2021-12-06 22:57 ` [PATCH 03/10] qcom_scm: scm call for deriving a software secret Gaurav Kashyap
2021-12-14 0:53 ` Eric Biggers
2021-12-06 22:57 ` [PATCH 04/10] soc: qcom: add HWKM library for storage encryption Gaurav Kashyap
2021-12-14 1:08 ` Eric Biggers
2021-12-06 22:57 ` [PATCH 05/10] scsi: ufs: prepare to support wrapped keys Gaurav Kashyap
2021-12-14 1:26 ` Eric Biggers
2021-12-06 22:57 ` [PATCH 06/10] soc: qcom: add wrapped key support for ICE Gaurav Kashyap
2021-12-14 1:46 ` Eric Biggers
2021-12-06 22:57 ` [PATCH 07/10] qcom_scm: scm call for create, prepare and import keys Gaurav Kashyap
2021-12-14 1:50 ` Eric Biggers
2021-12-06 22:57 ` [PATCH 08/10] scsi: ufs: add support for generate, import and prepare keys Gaurav Kashyap
2021-12-14 1:53 ` Eric Biggers
2021-12-06 22:57 ` [PATCH 09/10] soc: qcom: support for generate, import and prepare key Gaurav Kashyap
2021-12-14 2:04 ` Eric Biggers
2021-12-06 22:57 ` [PATCH 10/10] arm64: dts: qcom: sm8350: add ice and hwkm mappings Gaurav Kashyap
2022-01-06 19:47 ` [PATCH 00/10] Add wrapped key support for Qualcomm ICE Eric Biggers
2022-01-06 21:14 ` Gaurav Kashyap
2022-01-27 0:51 ` Eric Biggers
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=YbfjXtQgLSLOFvBr@gmail.com \
--to=ebiggers@kernel.org \
--cc=dineshg@quicinc.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-fscrypt@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=quic_gaurkash@quicinc.com \
--cc=quic_neersoni@quicinc.com \
--cc=thara.gopinath@linaro.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).