[v3,00/12] Hardware wrapped key support for qcom ice and ufs

These are the third iteration of patches that add support to Qualcomm ICE (Inline Crypto Engine) for hardware wrapped keys using Qualcomm Hardware Key Manager (HWKM)

They patches do the following:
- Address comments from v2 (Found here: https://lore.kernel.org/all/20230719170423.220033-1-quic_gaurkash@quicinc.com/)
- Rebased and tested on top of Eric's latest patchset: https://lore.kernel.org/all/20231104211259.17448-1-ebiggers@kernel.org/
- Rebased and tested on top of SM8650 patches from Linaro: https://lore.kernel.org/all/?q=sm8650

Information about patches copied over from v2:

"
Explanation and use of hardware-wrapped-keys can be found here:
Documentation/block/inline-encryption.rst

This patch is organized as follows:

Patch 1 - Prepares ICE and storage layers (UFS and EMMC) to pass around wrapped keys.
Patch 2 - Adds a new SCM api to support deriving software secret when wrapped keys are used
Patch 3-4 - Adds support for wrapped keys in the ICE driver. This includes adding HWKM support
Patch 5-6 - Adds support for wrapped keys in UFS
Patch 7-10 - Supports generate, prepare and import functionality in ICE and UFS

NOTE: MMC will have similar changes to UFS and will be uploaded in a different patchset
      Patch 3, 4, 8, 10 will have MMC equivalents.
"

Testing: 
Test platform: SM8650 MTP

The changes were tested by mounting initramfs and running the fscryptctl
tool (Ref: https://github.com/ebiggers/fscryptctl/tree/wip-wrapped-keys) to
generate and prepare keys, as well as to set policies on folders, which
consequently invokes disk encryption flows through UFS.

Tested both standard and wrapped keys (Removing qcom,ice-use-hwkm from dtsi will support using standard keys)

Steps to test:

The following configs were enabled:
CONFIG_BLK_INLINE_ENCRYPTION=y
CONFIG_QCOM_INLINE_CRYPTO_ENGINE=m
CONFIG_FS_ENCRYPTION_INLINE_CRYPT=y
CONFIG_SCSI_UFS_CRYPTO=y

Flash boot image, boot to shell and run the following commands

Creating and preparing keys
- mkfs.ext4 -F -O encrypt,stable_inodes /dev/disk/by-partlabel/userdata
- mount /dev/disk/by-partlabel/userdata -o inlinecrypt /mnt
- ./fscryptctl generate_hw_wrapped_key /dev/disk/by-partlabel/userdata > /mnt/key.longterm 
Note: import_hw_wrapped_key currently has a big which just got fixed, so it will be functional in the next SM8650 release
(It might already be available by the time the boards are available to public)
- ./fscryptctl prepare_hw_wrapped_key /dev/disk/by-partlabel/userdata < /mnt/key.longterm > /tmp/key.ephemeral
- ./fscryptctl add_key --hw-wrapped-key < /tmp/key.ephemeral /mnt

Create a folder and associate created keys with the folder
- rm -rf /mnt/dir
- mkdir /mnt/dir
- ./fscryptctl set_policy --hw-wrapped-key --iv-ino-lblk-64 "$keyid" /mnt/dir
- dmesg > /mnt/dir/test.txt
- sync

- Reboot
- mount /dev/disk/by-partlabel/userdata -o inlinecrypt /mnt
- ls /mnt/dir (You should see an encrypted file)
- ./fscryptctl prepare_hw_wrapped_key /dev/disk/by-partlabel/userdata < /mnt/key.longterm > /tmp/key.ephemeral
- ./fscryptctl add_key --hw-wrapped-key < /tmp/key.ephemeral /mnt
- cat /mnt/dir/test.txt

Gaurav Kashyap (12):
  ice, ufs, mmc: use blk_crypto_key for program_key
  qcom_scm: scm call for deriving a software secret
  soc: qcom: ice: add hwkm support in ice
  soc: qcom: ice: support for hardware wrapped keys
  ufs: core: support wrapped keys in ufs core
  ufs: host: wrapped keys support in ufs qcom
  qcom_scm: scm call for create, prepare and import keys
  ufs: core: add support for generate, import and prepare keys
  soc: qcom: support for generate, import and prepare key
  ufs: host: support for generate, import and prepare key
  arm64: dts: qcom: sm8650: add hwkm support to ufs ice
  dt-bindings: crypto: ice: document the hwkm property

 .../crypto/qcom,inline-crypto-engine.yaml     |   7 +
 arch/arm64/boot/dts/qcom/sm8650.dtsi          |   3 +-
 drivers/firmware/qcom/qcom_scm.c              | 276 +++++++++++++++
 drivers/firmware/qcom/qcom_scm.h              |   4 +
 drivers/mmc/host/cqhci-crypto.c               |   7 +-
 drivers/mmc/host/cqhci.h                      |   2 +
 drivers/mmc/host/sdhci-msm.c                  |   6 +-
 drivers/soc/qcom/ice.c                        | 321 +++++++++++++++++-
 drivers/ufs/core/ufshcd-crypto.c              |  87 ++++-
 drivers/ufs/host/ufs-qcom.c                   |  61 +++-
 include/linux/firmware/qcom/qcom_scm.h        |   7 +
 include/soc/qcom/ice.h                        |  18 +-
 include/ufs/ufshcd.h                          |  22 ++
 13 files changed, 784 insertions(+), 37 deletions(-)

On Tue, Nov 21, 2023 at 09:38:13PM -0800, Gaurav Kashyap wrote:
> This patch contains two changes in UFS for wrapped keys.

The code in this patch isn't two different changes, it just add the new
variant_ops and tie these to the blk crypto ops. That is one logical
change.

> 1. Implements the blk_crypto_profile ops for generate, import
>    and prepare key apis.
> 2. Defines UFS vops for generate, import and prepare keys so
>    that vendors can hook into them.

Please describe why this code is needed. Something like "Key management
is vendor specific, so add new variant ops and tie these to the block
crypto ops"...

Thanks,
Bjorn

On Tue, Nov 21, 2023 at 09:38:16PM -0800, Gaurav Kashyap wrote:
> The Inline Crypto Engine (ICE) for UFS supports the
> Hardware Key Manager (hwkm) to securely manage storage
> keys. Enable using this hardware on sm8650.

I'm unable to understand why the size of the reg changes based on this
motivation.

Regards,
Bjorn

> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sm8650.dtsi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index bbebe15437aa..b61066210e09 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -763,7 +763,8 @@ rng: rng@10c3000 {
>  		ice: crypto@1d88000 {
>  			compatible = "qcom,sm8650-inline-crypto-engine",
>  				     "qcom,inline-crypto-engine";
> -			reg = <0 0x01d88000 0 0x8000>;
> +			reg = <0 0x01d88000 0 0x10000>;
> +			qcom,ice-use-hwkm;
>  
>  			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
>  		};
> -- 
> 2.25.1
> 
>

On Tue, Nov 21, 2023 at 09:38:08PM -0800, Gaurav Kashyap wrote:
> Qualcomm's ICE (Inline Crypto Engine) contains a proprietary
> key management hardware called Hardware Key Manager (HWKM).
> This patch integrates HWKM support in ICE when it is
> available. HWKM primarily provides hardware wrapped key support
> where the ICE (storage) keys are not available in software and
> protected in hardware.
> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> ---
>  drivers/soc/qcom/ice.c | 133 ++++++++++++++++++++++++++++++++++++++++-
>  include/soc/qcom/ice.h |   1 +
>  2 files changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index 6f941d32fffb..adf9cab848fa 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -26,6 +26,19 @@
>  #define QCOM_ICE_REG_FUSE_SETTING		0x0010
>  #define QCOM_ICE_REG_BIST_STATUS		0x0070
>  #define QCOM_ICE_REG_ADVANCED_CONTROL		0x1000
> +#define QCOM_ICE_REG_CONTROL			0x0
> +/* QCOM ICE HWKM registers */
> +#define QCOM_ICE_REG_HWKM_TZ_KM_CTL			0x1000
> +#define QCOM_ICE_REG_HWKM_TZ_KM_STATUS			0x1004
> +#define QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS	0x2008
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_0			0x5000
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_1			0x5004
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_2			0x5008
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_3			0x500C
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_4			0x5010
> +
> +#define QCOM_ICE_HWKM_BIST_DONE_V1_VAL		0x11
> +#define QCOM_ICE_HWKM_BIST_DONE_V2_VAL		0x287
>  
>  /* BIST ("built-in self-test") status flags */
>  #define QCOM_ICE_BIST_STATUS_MASK		GENMASK(31, 28)
> @@ -34,6 +47,9 @@
>  #define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK	0x2
>  #define QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK	0x4
>  
> +#define QCOM_ICE_HWKM_REG_OFFSET	0x8000
> +#define HWKM_OFFSET(reg)		((reg) + QCOM_ICE_HWKM_REG_OFFSET)
> +
>  #define qcom_ice_writel(engine, val, reg)	\
>  	writel((val), (engine)->base + (reg))
>  
> @@ -46,6 +62,9 @@ struct qcom_ice {
>  	struct device_link *link;
>  
>  	struct clk *core_clk;
> +	u8 hwkm_version;
> +	bool use_hwkm;
> +	bool hwkm_init_complete;
>  };
>  
>  static bool qcom_ice_check_supported(struct qcom_ice *ice)
> @@ -63,8 +82,26 @@ static bool qcom_ice_check_supported(struct qcom_ice *ice)
>  		return false;
>  	}
>  
> +	if (major >= 4 || (major == 3 && minor == 2 && step >= 1))
> +		ice->hwkm_version = 2;
> +	else if (major == 3 && minor == 2)
> +		ice->hwkm_version = 1;
> +	else
> +		ice->hwkm_version = 0;
> +
> +	if (ice->hwkm_version == 0)
> +		ice->use_hwkm = false;
> +
>  	dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n",
>  		 major, minor, step);
> +	if (!ice->hwkm_version)
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not supported");

So for a version < 3.2.0, we will dev_info() three times, one stating
the version found, one stating that HWKM is not supported, and then
below one saying that HWKM is not used.

> +	else
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) version = %d",
> +			 ice->hwkm_version);

And for version >= 3.2.0 we will dev_info() two times.


To the vast majority of readers of the kernel log none of these
info-prints are useful - it's just spam.

I'd prefer that it was turned into dev_dbg(), which those who want to
know (e.g. during bringup) can enable. But that's a separate change,
please start by consolidating your information into a single line
printed in the log.

> +
> +	if (!ice->use_hwkm)
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not used");
>  
>  	/* If fuses are blown, ICE might not work in the standard way. */
>  	regval = qcom_ice_readl(ice, QCOM_ICE_REG_FUSE_SETTING);
> @@ -113,10 +150,14 @@ static void qcom_ice_optimization_enable(struct qcom_ice *ice)
>   * fails, so we needn't do it in software too, and (c) properly testing
>   * storage encryption requires testing the full storage stack anyway,
>   * and not relying on hardware-level self-tests.
> + *
> + * However, we still care about if HWKM BIST failed (when supported) as
> + * important functionality would fail later, so disable hwkm on failure.
>   */
>  static int qcom_ice_wait_bist_status(struct qcom_ice *ice)
>  {
>  	u32 regval;
> +	u32 bist_done_val;

The "val" suffix indicates that this would be a "value", but it's
actually a register offset. "bist_done_reg" would be better.

>  	int err;
>  
>  	err = readl_poll_timeout(ice->base + QCOM_ICE_REG_BIST_STATUS,
> @@ -125,15 +166,95 @@ static int qcom_ice_wait_bist_status(struct qcom_ice *ice)
>  	if (err)
>  		dev_err(ice->dev, "Timed out waiting for ICE self-test to complete\n");
>  
> +	if (ice->use_hwkm) {
> +		bist_done_val = (ice->hwkm_version == 1) ?
> +				 QCOM_ICE_HWKM_BIST_DONE_V1_VAL :
> +				 QCOM_ICE_HWKM_BIST_DONE_V2_VAL;
> +		if (qcom_ice_readl(ice,
> +				   HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_STATUS)) !=
> +				   bist_done_val) {
> +			dev_warn(ice->dev, "HWKM BIST error\n");

Sounds like a error to me, wouldn't dev_err() be suitable?

> +			ice->use_hwkm = false;
> +		}
> +	}
>  	return err;
>  }
>  
> +static void qcom_ice_enable_standard_mode(struct qcom_ice *ice)
> +{
> +	u32 val = 0;
> +
> +	if (!ice->use_hwkm)
> +		return;
> +
> +	/*
> +	 * When ICE is in standard (hwkm) mode, it supports HW wrapped
> +	 * keys, and when it is in legacy mode, it only supports standard
> +	 * (non HW wrapped) keys.
> +	 *
> +	 * Put ICE in standard mode, ICE defaults to legacy mode.
> +	 * Legacy mode - ICE HWKM slave not supported.
> +	 * Standard mode - ICE HWKM slave supported.
> +	 *
> +	 * Depending on the version of HWKM, it is controlled by different
> +	 * registers in ICE.
> +	 */
> +	if (ice->hwkm_version >= 2) {
> +		val = qcom_ice_readl(ice, QCOM_ICE_REG_CONTROL);
> +		val = val & 0xFFFFFFFE;
> +		qcom_ice_writel(ice, val, QCOM_ICE_REG_CONTROL);
> +	} else {
> +		qcom_ice_writel(ice, 0x7,
> +				HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> +	}
> +}
> +
> +static void qcom_ice_hwkm_init(struct qcom_ice *ice)
> +{
> +	if (!ice->use_hwkm)
> +		return;
> +
> +	/* Disable CRC checks. This HWKM feature is not used. */
> +	qcom_ice_writel(ice, 0x6,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> +
> +	/*
> +	 * Give register bank of the HWKM slave access to read and modify
> +	 * the keyslots in ICE HWKM slave. Without this, trustzone will not
> +	 * be able to program keys into ICE.
> +	 */
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_0));

This line is 86 characters long if left unwrapped. You're allowed to go
over 80 characters if it makes the code more readable, so please do so
for these and below.

> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_1));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_2));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_3));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_4));
> +
> +	/* Clear HWKM response FIFO before doing anything */
> +	qcom_ice_writel(ice, 0x8,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS));
> +}
> +
>  int qcom_ice_enable(struct qcom_ice *ice)
>  {
> +	int err;
> +
>  	qcom_ice_low_power_mode_enable(ice);
>  	qcom_ice_optimization_enable(ice);
>  
> -	return qcom_ice_wait_bist_status(ice);
> +	qcom_ice_enable_standard_mode(ice);
> +
> +	err = qcom_ice_wait_bist_status(ice);
> +	if (err)
> +		return err;
> +
> +	qcom_ice_hwkm_init(ice);
> +
> +	return err;
>  }
>  EXPORT_SYMBOL_GPL(qcom_ice_enable);
>  
> @@ -149,6 +270,8 @@ int qcom_ice_resume(struct qcom_ice *ice)
>  		return err;
>  	}
>  
> +	qcom_ice_enable_standard_mode(ice);
> +	qcom_ice_hwkm_init(ice);
>  	return qcom_ice_wait_bist_status(ice);
>  }
>  EXPORT_SYMBOL_GPL(qcom_ice_resume);
> @@ -205,6 +328,12 @@ int qcom_ice_evict_key(struct qcom_ice *ice, int slot)
>  }
>  EXPORT_SYMBOL_GPL(qcom_ice_evict_key);
>  
> +bool qcom_ice_hwkm_supported(struct qcom_ice *ice)
> +{
> +	return ice->use_hwkm;
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported);
> +
>  static struct qcom_ice *qcom_ice_create(struct device *dev,
>  					void __iomem *base)
>  {
> @@ -239,6 +368,8 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>  		engine->core_clk = devm_clk_get_enabled(dev, NULL);
>  	if (IS_ERR(engine->core_clk))
>  		return ERR_CAST(engine->core_clk);
> +	engine->use_hwkm = of_property_read_bool(dev->of_node,
> +						 "qcom,ice-use-hwkm");

Under what circumstances would we, with version >= 3.2, not specify this
flag?

Thanks,
Bjorn

>  
>  	if (!qcom_ice_check_supported(engine))
>  		return ERR_PTR(-EOPNOTSUPP);
> diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h
> index 9dd835dba2a7..1f52e82e3e1c 100644
> --- a/include/soc/qcom/ice.h
> +++ b/include/soc/qcom/ice.h
> @@ -34,5 +34,6 @@ int qcom_ice_program_key(struct qcom_ice *ice,
>  			 const struct blk_crypto_key *bkey,
>  			 u8 data_unit_size, int slot);
>  int qcom_ice_evict_key(struct qcom_ice *ice, int slot);
> +bool qcom_ice_hwkm_supported(struct qcom_ice *ice);
>  struct qcom_ice *of_qcom_ice_get(struct device *dev);
>  #endif /* __QCOM_ICE_H__ */
> -- 
> 2.25.1
> 
>

On Tue, Nov 21, 2023 at 09:38:17PM -0800, Gaurav Kashyap wrote:
> Add documentation for the ice-use-hwkm property in
> qcom ice.

Please describe the problem that you're solving by adding this property
to the binding (the description of hardware/firmware).

> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> ---
>  .../bindings/crypto/qcom,inline-crypto-engine.yaml         | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> index ca4f7d1cefaa..93e017dddc9d 100644
> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> @@ -24,6 +24,13 @@ properties:
>    clocks:
>      maxItems: 1
>  
> +  qcom,ice-use-hwkm:
> +    type: boolean
> +    description:
> +      Use the supported Hardware Key Manager (HWKM) in Qualcomm
> +      ICE to support wrapped keys. This dictates if wrapped keys
> +      have to be used by ICE.

As described it sounds like software configuration, rather than a
description of the capabilities of the hardware/firmware.

In what cases would we, and would we not, specify this property?

Regards,
Bjorn

> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.25.1
> 
>

On 11/22/2023 11:08 AM, Gaurav Kashyap wrote:
> Qualcomm's ICE (Inline Crypto Engine) contains a proprietary
> key management hardware called Hardware Key Manager (HWKM).
> This patch integrates HWKM support in ICE when it is
> available. HWKM primarily provides hardware wrapped key support
> where the ICE (storage) keys are not available in software and
> protected in hardware.
> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> ---
>   drivers/soc/qcom/ice.c | 133 ++++++++++++++++++++++++++++++++++++++++-
>   include/soc/qcom/ice.h |   1 +
>   2 files changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index 6f941d32fffb..adf9cab848fa 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -26,6 +26,19 @@
>   #define QCOM_ICE_REG_FUSE_SETTING		0x0010
>   #define QCOM_ICE_REG_BIST_STATUS		0x0070
>   #define QCOM_ICE_REG_ADVANCED_CONTROL		0x1000
> +#define QCOM_ICE_REG_CONTROL			0x0
> +/* QCOM ICE HWKM registers */
> +#define QCOM_ICE_REG_HWKM_TZ_KM_CTL			0x1000
> +#define QCOM_ICE_REG_HWKM_TZ_KM_STATUS			0x1004
> +#define QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS	0x2008
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_0			0x5000
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_1			0x5004
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_2			0x5008
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_3			0x500C
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_4			0x5010
> +
> +#define QCOM_ICE_HWKM_BIST_DONE_V1_VAL		0x11
> +#define QCOM_ICE_HWKM_BIST_DONE_V2_VAL		0x287
>   
>   /* BIST ("built-in self-test") status flags */
>   #define QCOM_ICE_BIST_STATUS_MASK		GENMASK(31, 28)
> @@ -34,6 +47,9 @@
>   #define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK	0x2
>   #define QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK	0x4
>   
> +#define QCOM_ICE_HWKM_REG_OFFSET	0x8000
> +#define HWKM_OFFSET(reg)		((reg) + QCOM_ICE_HWKM_REG_OFFSET)
> +
>   #define qcom_ice_writel(engine, val, reg)	\
>   	writel((val), (engine)->base + (reg))
>   
> @@ -46,6 +62,9 @@ struct qcom_ice {
>   	struct device_link *link;
>   
>   	struct clk *core_clk;
> +	u8 hwkm_version;
> +	bool use_hwkm;

we can rely on hwkm_version alone to determine if hwkm support is 
available or not.
if hwkm_version = 0 (default) consider hwkm is not enabled

> +	bool hwkm_init_complete;
>   };
>   
>   static bool qcom_ice_check_supported(struct qcom_ice *ice)
> @@ -63,8 +82,26 @@ static bool qcom_ice_check_supported(struct qcom_ice *ice)
>   		return false;
>   	}
>   
> +	if (major >= 4 || (major == 3 && minor == 2 && step >= 1))
> +		ice->hwkm_version = 2;
> +	else if (major == 3 && minor == 2)
> +		ice->hwkm_version = 1;
> +	else
> +		ice->hwkm_version = 0;
> +
> +	if (ice->hwkm_version == 0)
> +		ice->use_hwkm = false;
> +
hwkm version should pass from device-tree property instead of current 
complex logic. This will be helpful in support future hwkm version also.
ice->hwkm_version == 0, condition can be use for determining if hwkm is 
enabled or not.

>   	dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n",
>   		 major, minor, step);
> +	if (!ice->hwkm_version)
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not supported");
> +	else
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) version = %d",
> +			 ice->hwkm_version);
> +
> +	if (!ice->use_hwkm)
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not used");
>   
>   	/* If fuses are blown, ICE might not work in the standard way. */
>   	regval = qcom_ice_readl(ice, QCOM_ICE_REG_FUSE_SETTING);
> @@ -113,10 +150,14 @@ static void qcom_ice_optimization_enable(struct qcom_ice *ice)
>    * fails, so we needn't do it in software too, and (c) properly testing
>    * storage encryption requires testing the full storage stack anyway,
>    * and not relying on hardware-level self-tests.
> + *
> + * However, we still care about if HWKM BIST failed (when supported) as
> + * important functionality would fail later, so disable hwkm on failure.
>    */
>   static int qcom_ice_wait_bist_status(struct qcom_ice *ice)
>   {
>   	u32 regval;
> +	u32 bist_done_val;
>   	int err;
>   
>   	err = readl_poll_timeout(ice->base + QCOM_ICE_REG_BIST_STATUS,
> @@ -125,15 +166,95 @@ static int qcom_ice_wait_bist_status(struct qcom_ice *ice)
>   	if (err)
>   		dev_err(ice->dev, "Timed out waiting for ICE self-test to complete\n");
>   
> +	if (ice->use_hwkm) {
> +		bist_done_val = (ice->hwkm_version == 1) ?
> +				 QCOM_ICE_HWKM_BIST_DONE_V1_VAL :
> +				 QCOM_ICE_HWKM_BIST_DONE_V2_VAL;
> +		if (qcom_ice_readl(ice,
> +				   HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_STATUS)) !=
> +				   bist_done_val) {
> +			dev_warn(ice->dev, "HWKM BIST error\n");
> +			ice->use_hwkm = false;
error is not passed to caller. if HWKM is enabled, and BIST failed, ICE 
init also should be considered has failure.

Any reason considering it as warning only ?
> +		}
> +	}
>   	return err;
>   }
>   
> +static void qcom_ice_enable_standard_mode(struct qcom_ice *ice)
> +{
> +	u32 val = 0;
> +
> +	if (!ice->use_hwkm)
> +		return;
> +
> +	/*
> +	 * When ICE is in standard (hwkm) mode, it supports HW wrapped
> +	 * keys, and when it is in legacy mode, it only supports standard
> +	 * (non HW wrapped) keys.
> +	 *
> +	 * Put ICE in standard mode, ICE defaults to legacy mode.
> +	 * Legacy mode - ICE HWKM slave not supported.
> +	 * Standard mode - ICE HWKM slave supported.
> +	 *
> +	 * Depending on the version of HWKM, it is controlled by different
> +	 * registers in ICE.
> +	 */
> +	if (ice->hwkm_version >= 2) {

hwkm_version version >= 2 is not exist. This programming instruction may 
create problem in future.

> +		val = qcom_ice_readl(ice, QCOM_ICE_REG_CONTROL);
> +		val = val & 0xFFFFFFFE;
> +		qcom_ice_writel(ice, val, QCOM_ICE_REG_CONTROL);
> +	} else {
> +		qcom_ice_writel(ice, 0x7,
void using constant value like "0x7" in code use #define with meaningful 
name that can indicate what operation is being performed.
This comment is applicable for many places.
> +				HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> +	}
> +}
> +
> +static void qcom_ice_hwkm_init(struct qcom_ice *ice)
> +{
> +	if (!ice->use_hwkm)
> +		return;
> +
> +	/* Disable CRC checks. This HWKM feature is not used. */
> +	qcom_ice_writel(ice, 0x6,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> +
> +	/*
> +	 * Give register bank of the HWKM slave access to read and modify
> +	 * the keyslots in ICE HWKM slave. Without this, trustzone will not
> +	 * be able to program keys into ICE.
> +	 */
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_0));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_1));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_2));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_3));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_4));
> +
> +	/* Clear HWKM response FIFO before doing anything */
> +	qcom_ice_writel(ice, 0x8,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS));
> +}
> +
>   int qcom_ice_enable(struct qcom_ice *ice)
>   {
> +	int err;
> +
>   	qcom_ice_low_power_mode_enable(ice);
>   	qcom_ice_optimization_enable(ice);
>   
> -	return qcom_ice_wait_bist_status(ice);
> +	qcom_ice_enable_standard_mode(ice);
> +
> +	err = qcom_ice_wait_bist_status(ice);
> +	if (err)
> +		return err;
> +
> +	qcom_ice_hwkm_init(ice);
new code added in this section is only application when hwkm is enabled.
if (!ice->hwkm_version) {
	qcom_ice_enable_standard_mode(ice);
	qcom_ice_hwkm_init(ice);
}
> +
> +	return err;
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_enable);
>   
> @@ -149,6 +270,8 @@ int qcom_ice_resume(struct qcom_ice *ice)
>   		return err;
>   	}
>   
> +	qcom_ice_enable_standard_mode(ice);
> +	qcom_ice_hwkm_init(ice);
>   	return qcom_ice_wait_bist_status(ice);
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_resume);
> @@ -205,6 +328,12 @@ int qcom_ice_evict_key(struct qcom_ice *ice, int slot)
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_evict_key);
>   
> +bool qcom_ice_hwkm_supported(struct qcom_ice *ice)
> +{
> +	return ice->use_hwkm;
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported);
> +
>   static struct qcom_ice *qcom_ice_create(struct device *dev,
>   					void __iomem *base)
>   {
> @@ -239,6 +368,8 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>   		engine->core_clk = devm_clk_get_enabled(dev, NULL);
>   	if (IS_ERR(engine->core_clk))
>   		return ERR_CAST(engine->core_clk);
> +	engine->use_hwkm = of_property_read_bool(dev->of_node,
> +						 "qcom,ice-use-hwkm");
>   
>   	if (!qcom_ice_check_supported(engine))
>   		return ERR_PTR(-EOPNOTSUPP);
> diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h
> index 9dd835dba2a7..1f52e82e3e1c 100644
> --- a/include/soc/qcom/ice.h
> +++ b/include/soc/qcom/ice.h
> @@ -34,5 +34,6 @@ int qcom_ice_program_key(struct qcom_ice *ice,
>   			 const struct blk_crypto_key *bkey,
>   			 u8 data_unit_size, int slot);
>   int qcom_ice_evict_key(struct qcom_ice *ice, int slot);
> +bool qcom_ice_hwkm_supported(struct qcom_ice *ice);
>   struct qcom_ice *of_qcom_ice_get(struct device *dev);
>   #endif /* __QCOM_ICE_H__ */

On 11/22/2023 11:08 AM, Gaurav Kashyap wrote:
> Qualcomm's ICE (Inline Crypto Engine) contains a proprietary
> key management hardware called Hardware Key Manager (HWKM).
> This patch integrates HWKM support in ICE when it is
> available. HWKM primarily provides hardware wrapped key support
> where the ICE (storage) keys are not available in software and
> protected in hardware.
> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> ---
>   drivers/soc/qcom/ice.c | 133 ++++++++++++++++++++++++++++++++++++++++-
>   include/soc/qcom/ice.h |   1 +
>   2 files changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index 6f941d32fffb..adf9cab848fa 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -26,6 +26,19 @@
>   #define QCOM_ICE_REG_FUSE_SETTING		0x0010
>   #define QCOM_ICE_REG_BIST_STATUS		0x0070
>   #define QCOM_ICE_REG_ADVANCED_CONTROL		0x1000
> +#define QCOM_ICE_REG_CONTROL			0x0
> +/* QCOM ICE HWKM registers */
> +#define QCOM_ICE_REG_HWKM_TZ_KM_CTL			0x1000
> +#define QCOM_ICE_REG_HWKM_TZ_KM_STATUS			0x1004
> +#define QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS	0x2008
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_0			0x5000
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_1			0x5004
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_2			0x5008
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_3			0x500C
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_4			0x5010
> +
> +#define QCOM_ICE_HWKM_BIST_DONE_V1_VAL		0x11
> +#define QCOM_ICE_HWKM_BIST_DONE_V2_VAL		0x287
>   
>   /* BIST ("built-in self-test") status flags */
>   #define QCOM_ICE_BIST_STATUS_MASK		GENMASK(31, 28)
> @@ -34,6 +47,9 @@
>   #define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK	0x2
>   #define QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK	0x4
>   
> +#define QCOM_ICE_HWKM_REG_OFFSET	0x8000
> +#define HWKM_OFFSET(reg)		((reg) + QCOM_ICE_HWKM_REG_OFFSET)
> +
>   #define qcom_ice_writel(engine, val, reg)	\
>   	writel((val), (engine)->base + (reg))
>   
> @@ -46,6 +62,9 @@ struct qcom_ice {
>   	struct device_link *link;
>   
>   	struct clk *core_clk;
> +	u8 hwkm_version;
> +	bool use_hwkm;

we can rely on hwkm_version alone to determine if hwkm support is 
available or not.
if hwkm_version = 0 (default) consider hwkm is not enabled

> +	bool hwkm_init_complete;
>   };
>   
>   static bool qcom_ice_check_supported(struct qcom_ice *ice)
> @@ -63,8 +82,26 @@ static bool qcom_ice_check_supported(struct qcom_ice *ice)
>   		return false;
>   	}
>   
> +	if (major >= 4 || (major == 3 && minor == 2 && step >= 1))
> +		ice->hwkm_version = 2;
> +	else if (major == 3 && minor == 2)
> +		ice->hwkm_version = 1;
> +	else
> +		ice->hwkm_version = 0;
> +
> +	if (ice->hwkm_version == 0)
> +		ice->use_hwkm = false;
> +
hwkm version should pass from device-tree property instead of current 
complex logic. This will be helpful in support future hwkm version also.
ice->hwkm_version == 0, condition can be use for determining if hwkm is 
enabled or not.

>   	dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n",
>   		 major, minor, step);
> +	if (!ice->hwkm_version)
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not supported");
> +	else
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) version = %d",
> +			 ice->hwkm_version);
> +
> +	if (!ice->use_hwkm)
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not used");
>   
>   	/* If fuses are blown, ICE might not work in the standard way. */
>   	regval = qcom_ice_readl(ice, QCOM_ICE_REG_FUSE_SETTING);
> @@ -113,10 +150,14 @@ static void qcom_ice_optimization_enable(struct qcom_ice *ice)
>    * fails, so we needn't do it in software too, and (c) properly testing
>    * storage encryption requires testing the full storage stack anyway,
>    * and not relying on hardware-level self-tests.
> + *
> + * However, we still care about if HWKM BIST failed (when supported) as
> + * important functionality would fail later, so disable hwkm on failure.
>    */
>   static int qcom_ice_wait_bist_status(struct qcom_ice *ice)
>   {
>   	u32 regval;
> +	u32 bist_done_val;
>   	int err;
>   
>   	err = readl_poll_timeout(ice->base + QCOM_ICE_REG_BIST_STATUS,
> @@ -125,15 +166,95 @@ static int qcom_ice_wait_bist_status(struct qcom_ice *ice)
>   	if (err)
>   		dev_err(ice->dev, "Timed out waiting for ICE self-test to complete\n");
>   
> +	if (ice->use_hwkm) {
> +		bist_done_val = (ice->hwkm_version == 1) ?
> +				 QCOM_ICE_HWKM_BIST_DONE_V1_VAL :
> +				 QCOM_ICE_HWKM_BIST_DONE_V2_VAL;
> +		if (qcom_ice_readl(ice,
> +				   HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_STATUS)) !=
> +				   bist_done_val) {
> +			dev_warn(ice->dev, "HWKM BIST error\n");
> +			ice->use_hwkm = false;
error is not passed to caller. if HWKM is enabled, and BIST failed, ICE 
init also should be considered has failure.

Any reason considering it as warning only ?
> +		}
> +	}
>   	return err;
>   }
>   
> +static void qcom_ice_enable_standard_mode(struct qcom_ice *ice)
> +{
> +	u32 val = 0;
> +
> +	if (!ice->use_hwkm)
> +		return;
> +
> +	/*
> +	 * When ICE is in standard (hwkm) mode, it supports HW wrapped
> +	 * keys, and when it is in legacy mode, it only supports standard
> +	 * (non HW wrapped) keys.
> +	 *
> +	 * Put ICE in standard mode, ICE defaults to legacy mode.
> +	 * Legacy mode - ICE HWKM slave not supported.
> +	 * Standard mode - ICE HWKM slave supported.
> +	 *
> +	 * Depending on the version of HWKM, it is controlled by different
> +	 * registers in ICE.
> +	 */
> +	if (ice->hwkm_version >= 2) {

hwkm_version version >= 2 is not exist. This programming instruction may 
create problem in future.

> +		val = qcom_ice_readl(ice, QCOM_ICE_REG_CONTROL);
> +		val = val & 0xFFFFFFFE;
> +		qcom_ice_writel(ice, val, QCOM_ICE_REG_CONTROL);
> +	} else {
> +		qcom_ice_writel(ice, 0x7,
void using constant value like "0x7" in code use #define with meaningful 
name that can indicate what operation is being performed.
This comment is applicable for many places.
> +				HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> +	}
> +}
> +
> +static void qcom_ice_hwkm_init(struct qcom_ice *ice)
> +{
> +	if (!ice->use_hwkm)
> +		return;
> +
> +	/* Disable CRC checks. This HWKM feature is not used. */
> +	qcom_ice_writel(ice, 0x6,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> +
> +	/*
> +	 * Give register bank of the HWKM slave access to read and modify
> +	 * the keyslots in ICE HWKM slave. Without this, trustzone will not
> +	 * be able to program keys into ICE.
> +	 */
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_0));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_1));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_2));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_3));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_4));
> +
> +	/* Clear HWKM response FIFO before doing anything */
> +	qcom_ice_writel(ice, 0x8,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS));
> +}
> +
>   int qcom_ice_enable(struct qcom_ice *ice)
>   {
> +	int err;
> +
>   	qcom_ice_low_power_mode_enable(ice);
>   	qcom_ice_optimization_enable(ice);
>   
> -	return qcom_ice_wait_bist_status(ice);
> +	qcom_ice_enable_standard_mode(ice);
> +
> +	err = qcom_ice_wait_bist_status(ice);
> +	if (err)
> +		return err;
> +
> +	qcom_ice_hwkm_init(ice);
new code added in this section is only application when hwkm is enabled.
if (!ice->hwkm_version) {
	qcom_ice_enable_standard_mode(ice);
	qcom_ice_hwkm_init(ice);
}
> +
> +	return err;
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_enable);
>   
> @@ -149,6 +270,8 @@ int qcom_ice_resume(struct qcom_ice *ice)
>   		return err;
>   	}
>   
> +	qcom_ice_enable_standard_mode(ice);
> +	qcom_ice_hwkm_init(ice);
>   	return qcom_ice_wait_bist_status(ice);
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_resume);
> @@ -205,6 +328,12 @@ int qcom_ice_evict_key(struct qcom_ice *ice, int slot)
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_evict_key);
>   
> +bool qcom_ice_hwkm_supported(struct qcom_ice *ice)
> +{
> +	return ice->use_hwkm;
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported);
> +
>   static struct qcom_ice *qcom_ice_create(struct device *dev,
>   					void __iomem *base)
>   {
> @@ -239,6 +368,8 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>   		engine->core_clk = devm_clk_get_enabled(dev, NULL);
>   	if (IS_ERR(engine->core_clk))
>   		return ERR_CAST(engine->core_clk);
> +	engine->use_hwkm = of_property_read_bool(dev->of_node,
> +						 "qcom,ice-use-hwkm");
>   
>   	if (!qcom_ice_check_supported(engine))
>   		return ERR_PTR(-EOPNOTSUPP);
> diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h
> index 9dd835dba2a7..1f52e82e3e1c 100644
> --- a/include/soc/qcom/ice.h
> +++ b/include/soc/qcom/ice.h
> @@ -34,5 +34,6 @@ int qcom_ice_program_key(struct qcom_ice *ice,
>   			 const struct blk_crypto_key *bkey,
>   			 u8 data_unit_size, int slot);
>   int qcom_ice_evict_key(struct qcom_ice *ice, int slot);
> +bool qcom_ice_hwkm_supported(struct qcom_ice *ice);
>   struct qcom_ice *of_qcom_ice_get(struct device *dev);
>   #endif /* __QCOM_ICE_H__ */

On 11/22/2023 11:08 AM, Gaurav Kashyap wrote:
> Qualcomm's ICE (Inline Crypto Engine) contains a proprietary
> key management hardware called Hardware Key Manager (HWKM).
> This patch integrates HWKM support in ICE when it is
> available. HWKM primarily provides hardware wrapped key support
> where the ICE (storage) keys are not available in software and
> protected in hardware.
> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> ---
>   drivers/soc/qcom/ice.c | 133 ++++++++++++++++++++++++++++++++++++++++-
>   include/soc/qcom/ice.h |   1 +
>   2 files changed, 133 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index 6f941d32fffb..adf9cab848fa 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -26,6 +26,19 @@
>   #define QCOM_ICE_REG_FUSE_SETTING		0x0010
>   #define QCOM_ICE_REG_BIST_STATUS		0x0070
>   #define QCOM_ICE_REG_ADVANCED_CONTROL		0x1000
> +#define QCOM_ICE_REG_CONTROL			0x0
> +/* QCOM ICE HWKM registers */
> +#define QCOM_ICE_REG_HWKM_TZ_KM_CTL			0x1000
> +#define QCOM_ICE_REG_HWKM_TZ_KM_STATUS			0x1004
> +#define QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS	0x2008
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_0			0x5000
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_1			0x5004
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_2			0x5008
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_3			0x500C
> +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_4			0x5010
> +
> +#define QCOM_ICE_HWKM_BIST_DONE_V1_VAL		0x11
> +#define QCOM_ICE_HWKM_BIST_DONE_V2_VAL		0x287
>   
>   /* BIST ("built-in self-test") status flags */
>   #define QCOM_ICE_BIST_STATUS_MASK		GENMASK(31, 28)
> @@ -34,6 +47,9 @@
>   #define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK	0x2
>   #define QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK	0x4
>   
> +#define QCOM_ICE_HWKM_REG_OFFSET	0x8000
> +#define HWKM_OFFSET(reg)		((reg) + QCOM_ICE_HWKM_REG_OFFSET)
> +
>   #define qcom_ice_writel(engine, val, reg)	\
>   	writel((val), (engine)->base + (reg))
>   
> @@ -46,6 +62,9 @@ struct qcom_ice {
>   	struct device_link *link;
>   
>   	struct clk *core_clk;
> +	u8 hwkm_version;
> +	bool use_hwkm;

we can rely on hwkm_version alone to determine if hwkm support is 
available or not.
if hwkm_version = 0 (default) consider hwkm is not enabled

> +	bool hwkm_init_complete;
>   };
>   
>   static bool qcom_ice_check_supported(struct qcom_ice *ice)
> @@ -63,8 +82,26 @@ static bool qcom_ice_check_supported(struct qcom_ice *ice)
>   		return false;
>   	}
>   
> +	if (major >= 4 || (major == 3 && minor == 2 && step >= 1))
> +		ice->hwkm_version = 2;
> +	else if (major == 3 && minor == 2)
> +		ice->hwkm_version = 1;
> +	else
> +		ice->hwkm_version = 0;
> +
> +	if (ice->hwkm_version == 0)
> +		ice->use_hwkm = false;
> +
hwkm version should pass from device-tree property instead of current 
complex logic. This will be helpful in support future hwkm version also.
ice->hwkm_version == 0, condition can be use for determining if hwkm is 
enabled or not.

>   	dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n",
>   		 major, minor, step);
> +	if (!ice->hwkm_version)
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not supported");
> +	else
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) version = %d",
> +			 ice->hwkm_version);
> +
> +	if (!ice->use_hwkm)
> +		dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not used");
>   
>   	/* If fuses are blown, ICE might not work in the standard way. */
>   	regval = qcom_ice_readl(ice, QCOM_ICE_REG_FUSE_SETTING);
> @@ -113,10 +150,14 @@ static void qcom_ice_optimization_enable(struct qcom_ice *ice)
>    * fails, so we needn't do it in software too, and (c) properly testing
>    * storage encryption requires testing the full storage stack anyway,
>    * and not relying on hardware-level self-tests.
> + *
> + * However, we still care about if HWKM BIST failed (when supported) as
> + * important functionality would fail later, so disable hwkm on failure.
>    */
>   static int qcom_ice_wait_bist_status(struct qcom_ice *ice)
>   {
>   	u32 regval;
> +	u32 bist_done_val;
>   	int err;
>   
>   	err = readl_poll_timeout(ice->base + QCOM_ICE_REG_BIST_STATUS,
> @@ -125,15 +166,95 @@ static int qcom_ice_wait_bist_status(struct qcom_ice *ice)
>   	if (err)
>   		dev_err(ice->dev, "Timed out waiting for ICE self-test to complete\n");
>   
> +	if (ice->use_hwkm) {
> +		bist_done_val = (ice->hwkm_version == 1) ?
> +				 QCOM_ICE_HWKM_BIST_DONE_V1_VAL :
> +				 QCOM_ICE_HWKM_BIST_DONE_V2_VAL;
> +		if (qcom_ice_readl(ice,
> +				   HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_STATUS)) !=
> +				   bist_done_val) {
> +			dev_warn(ice->dev, "HWKM BIST error\n");
> +			ice->use_hwkm = false;
error is not passed to caller. if HWKM is enabled, and BIST failed, ICE 
init also should be considered has failure.

Any reason considering it as warning only ?
> +		}
> +	}
>   	return err;
>   }
>   
> +static void qcom_ice_enable_standard_mode(struct qcom_ice *ice)
> +{
> +	u32 val = 0;
> +
> +	if (!ice->use_hwkm)
> +		return;
> +
> +	/*
> +	 * When ICE is in standard (hwkm) mode, it supports HW wrapped
> +	 * keys, and when it is in legacy mode, it only supports standard
> +	 * (non HW wrapped) keys.
> +	 *
> +	 * Put ICE in standard mode, ICE defaults to legacy mode.
> +	 * Legacy mode - ICE HWKM slave not supported.
> +	 * Standard mode - ICE HWKM slave supported.
> +	 *
> +	 * Depending on the version of HWKM, it is controlled by different
> +	 * registers in ICE.
> +	 */
> +	if (ice->hwkm_version >= 2) {
This programming instruction may create problem fn future of hwkm 
version is not compatible. Better way is:
if (ice->hwkm_version == 2 && ice->hwkm_version == 3)

> +		val = qcom_ice_readl(ice, QCOM_ICE_REG_CONTROL);
> +		val = val & 0xFFFFFFFE;
> +		qcom_ice_writel(ice, val, QCOM_ICE_REG_CONTROL);
> +	} else {
> +		qcom_ice_writel(ice, 0x7,
void using constant value like "0x7" in code use #define with meaningful 
name that can indicate what operation is being performed.
This comment is applicable for many places.
> +				HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> +	}
> +}
> +
> +static void qcom_ice_hwkm_init(struct qcom_ice *ice)
> +{
> +	if (!ice->use_hwkm)
> +		return;
> +
> +	/* Disable CRC checks. This HWKM feature is not used. */
> +	qcom_ice_writel(ice, 0x6,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> +
> +	/*
> +	 * Give register bank of the HWKM slave access to read and modify
> +	 * the keyslots in ICE HWKM slave. Without this, trustzone will not
> +	 * be able to program keys into ICE.
> +	 */
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_0));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_1));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_2));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_3));
> +	qcom_ice_writel(ice, 0xFFFFFFFF,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_4));
> +
> +	/* Clear HWKM response FIFO before doing anything */
> +	qcom_ice_writel(ice, 0x8,
> +			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS));
> +}
> +
>   int qcom_ice_enable(struct qcom_ice *ice)
>   {
> +	int err;
> +
>   	qcom_ice_low_power_mode_enable(ice);
>   	qcom_ice_optimization_enable(ice);
>   
> -	return qcom_ice_wait_bist_status(ice);
> +	qcom_ice_enable_standard_mode(ice);
> +
> +	err = qcom_ice_wait_bist_status(ice);
> +	if (err)
> +		return err;
> +
> +	qcom_ice_hwkm_init(ice);
new code added in this section is only application when hwkm is enabled.
if (!ice->hwkm_version) {
	qcom_ice_enable_standard_mode(ice);
	qcom_ice_hwkm_init(ice);
}
> +
> +	return err;
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_enable);
>   
> @@ -149,6 +270,8 @@ int qcom_ice_resume(struct qcom_ice *ice)
>   		return err;
>   	}
>   
> +	qcom_ice_enable_standard_mode(ice);
> +	qcom_ice_hwkm_init(ice);
>   	return qcom_ice_wait_bist_status(ice);
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_resume);
> @@ -205,6 +328,12 @@ int qcom_ice_evict_key(struct qcom_ice *ice, int slot)
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_evict_key);
>   
> +bool qcom_ice_hwkm_supported(struct qcom_ice *ice)
> +{
> +	return ice->use_hwkm;
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported);
> +
>   static struct qcom_ice *qcom_ice_create(struct device *dev,
>   					void __iomem *base)
>   {
> @@ -239,6 +368,8 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
>   		engine->core_clk = devm_clk_get_enabled(dev, NULL);
>   	if (IS_ERR(engine->core_clk))
>   		return ERR_CAST(engine->core_clk);
> +	engine->use_hwkm = of_property_read_bool(dev->of_node,
> +						 "qcom,ice-use-hwkm");
>   
>   	if (!qcom_ice_check_supported(engine))
>   		return ERR_PTR(-EOPNOTSUPP);
> diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h
> index 9dd835dba2a7..1f52e82e3e1c 100644
> --- a/include/soc/qcom/ice.h
> +++ b/include/soc/qcom/ice.h
> @@ -34,5 +34,6 @@ int qcom_ice_program_key(struct qcom_ice *ice,
>   			 const struct blk_crypto_key *bkey,
>   			 u8 data_unit_size, int slot);
>   int qcom_ice_evict_key(struct qcom_ice *ice, int slot);
> +bool qcom_ice_hwkm_supported(struct qcom_ice *ice);
>   struct qcom_ice *of_qcom_ice_get(struct device *dev);
>   #endif /* __QCOM_ICE_H__ */

On 11/22/2023 11:08 AM, Gaurav Kashyap wrote:
> Inline storage encryption requires deriving a sw secret from
> the hardware wrapped keys. For non-wrapped keys, this can be
> directly done as keys are in the clear.
> 
> However, when keys are hardware wrapped, it can be unwrapped
> by HWKM (Hardware Key Manager) which is accessible only from Qualcomm
> Trustzone. Hence, it also makes sense that the software secret is also
> derived there and returned to the linux kernel . This can be invoked by
> using the crypto profile APIs provided by the block layer.
> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> ---
>   drivers/firmware/qcom/qcom_scm.c       | 71 ++++++++++++++++++++++++++
>   drivers/firmware/qcom/qcom_scm.h       |  1 +
>   include/linux/firmware/qcom/qcom_scm.h |  2 +
>   3 files changed, 74 insertions(+)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 520de9b5633a..6dfb913f3e33 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -1214,6 +1214,77 @@ int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
>   }
>   EXPORT_SYMBOL_GPL(qcom_scm_ice_set_key);
>   
> +/**
> + * qcom_scm_derive_sw_secret() - Derive software secret from wrapped key
> + * @wkey: the hardware wrapped key inaccessible to software
> + * @wkey_size: size of the wrapped key
> + * @sw_secret: the secret to be derived which is exactly the secret size
> + * @sw_secret_size: size of the sw_secret
> + *
> + * Derive a software secret from a hardware wrapped key for software crypto
> + * operations.
> + * For wrapped keys, the key needs to be unwrapped, in order to derive a
> + * software secret, which can be done in the hardware from a secure execution
> + * environment.
> + *
> + * For more information on sw secret, please refer to "Hardware-wrapped keys"
> + * section of Documentation/block/inline-encryption.rst.
> + *
> + * Return: 0 on success; -errno on failure.
> + */
> +int qcom_scm_derive_sw_secret(const u8 *wkey, size_t wkey_size,
> +			      u8 *sw_secret, size_t sw_secret_size)
> +{
> +	struct qcom_scm_desc desc = {
> +		.svc = QCOM_SCM_SVC_ES,
> +		.cmd =  QCOM_SCM_ES_DERIVE_SW_SECRET,
> +		.arginfo = QCOM_SCM_ARGS(4, QCOM_SCM_RW,
> +					 QCOM_SCM_VAL, QCOM_SCM_RW,
> +					 QCOM_SCM_VAL),
> +		.args[1] = wkey_size,
> +		.args[3] = sw_secret_size,
> +		.owner = ARM_SMCCC_OWNER_SIP,
> +	};
> +
> +	void *wkey_buf, *secret_buf;
> +	dma_addr_t wkey_phys, secret_phys;
> +	int ret;
> +
> +	/*
> +	 * Like qcom_scm_ice_set_key(), we use dma_alloc_coherent() to properly
> +	 * get a physical address, while guaranteeing that we can zeroize the
> +	 * key material later using memzero_explicit().
> +	 */
> +	wkey_buf = dma_alloc_coherent(__scm->dev, wkey_size, &wkey_phys, GFP_KERNEL);
> +	if (!wkey_buf)
> +		return -ENOMEM;
> +	secret_buf = dma_alloc_coherent(__scm->dev, sw_secret_size, &secret_phys, GFP_KERNEL);
> +	if (!secret_buf) {
> +		ret = -ENOMEM;
> +		goto err_free_wrapped;
> +	}
> +
> +	memcpy(wkey_buf, wkey, wkey_size);
> +	desc.args[0] = wkey_phys;
> +	desc.args[2] = secret_phys;
> +
> +	ret = qcom_scm_call(__scm->dev, &desc, NULL);
> +	if (!ret)
> +		memcpy(sw_secret, secret_buf, sw_secret_size);
> +
> +	memzero_explicit(secret_buf, sw_secret_size);
> +
> +	dma_free_coherent(__scm->dev, sw_secret_size, secret_buf, secret_phys);
> +
> +err_free_wrapped:
> +	memzero_explicit(wkey_buf, wkey_size);
In error handling case the operation is being performed on unallocated 
memory.
> +
> +	dma_free_coherent(__scm->dev, wkey_size, wkey_buf, wkey_phys);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(qcom_scm_derive_sw_secret);
> +
>   /**
>    * qcom_scm_hdcp_available() - Check if secure environment supports HDCP.
>    *
> diff --git a/drivers/firmware/qcom/qcom_scm.h b/drivers/firmware/qcom/qcom_scm.h
> index 4532907e8489..c75456aa6ac5 100644
> --- a/drivers/firmware/qcom/qcom_scm.h
> +++ b/drivers/firmware/qcom/qcom_scm.h
> @@ -121,6 +121,7 @@ int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc,
>   #define QCOM_SCM_SVC_ES			0x10	/* Enterprise Security */
>   #define QCOM_SCM_ES_INVALIDATE_ICE_KEY	0x03
>   #define QCOM_SCM_ES_CONFIG_SET_ICE_KEY	0x04
> +#define QCOM_SCM_ES_DERIVE_SW_SECRET	0x07
>   
>   #define QCOM_SCM_SVC_HDCP		0x11
>   #define QCOM_SCM_HDCP_INVOKE		0x01
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index ccaf28846054..c65f2d61492d 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -103,6 +103,8 @@ bool qcom_scm_ice_available(void);
>   int qcom_scm_ice_invalidate_key(u32 index);
>   int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
>   			 enum qcom_scm_ice_cipher cipher, u32 data_unit_size);
> +int qcom_scm_derive_sw_secret(const u8 *wkey, size_t wkey_size,
> +			      u8 *sw_secret, size_t sw_secret_size);
>   
>   bool qcom_scm_hdcp_available(void);
>   int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, u32 *resp);

On 11/22/2023 11:08 AM, Gaurav Kashyap wrote:
> Now that HWKM support is added to ICE, extend the ICE
> driver to support hardware wrapped keys programming coming
> in from the storage controllers (ufs and emmc). The patches that follow
> will add ufs and emmc support.
> 
> Derive software secret support is also added by forwarding the
> call the corresponding scm api.
> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> ---
>   drivers/soc/qcom/ice.c | 114 +++++++++++++++++++++++++++++++++++++----
>   include/soc/qcom/ice.h |   4 ++
>   2 files changed, 107 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index adf9cab848fa..ee7c0beef3d2 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -27,6 +27,8 @@
>   #define QCOM_ICE_REG_BIST_STATUS		0x0070
>   #define QCOM_ICE_REG_ADVANCED_CONTROL		0x1000
>   #define QCOM_ICE_REG_CONTROL			0x0
> +#define QCOM_ICE_LUT_KEYS_CRYPTOCFG_R16		0x4040
> +
>   /* QCOM ICE HWKM registers */
>   #define QCOM_ICE_REG_HWKM_TZ_KM_CTL			0x1000
>   #define QCOM_ICE_REG_HWKM_TZ_KM_STATUS			0x1004
> @@ -37,6 +39,7 @@
>   #define QCOM_ICE_REG_HWKM_BANK0_BBAC_3			0x500C
>   #define QCOM_ICE_REG_HWKM_BANK0_BBAC_4			0x5010
>   
> +/* QCOM ICE HWKM BIST vals */
>   #define QCOM_ICE_HWKM_BIST_DONE_V1_VAL		0x11
>   #define QCOM_ICE_HWKM_BIST_DONE_V2_VAL		0x287
>   
> @@ -47,6 +50,8 @@
>   #define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK	0x2
>   #define QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK	0x4
>   
> +#define QCOM_ICE_LUT_KEYS_CRYPTOCFG_OFFSET	0x80
> +
>   #define QCOM_ICE_HWKM_REG_OFFSET	0x8000
>   #define HWKM_OFFSET(reg)		((reg) + QCOM_ICE_HWKM_REG_OFFSET)
>   
> @@ -67,6 +72,16 @@ struct qcom_ice {
>   	bool hwkm_init_complete;
>   };
>   
> +union crypto_cfg {
> +	__le32 regval;
> +	struct {
> +		u8 dusize;
> +		u8 capidx;
> +		u8 reserved;
> +		u8 cfge;
> +	};
> +};
> +
>   static bool qcom_ice_check_supported(struct qcom_ice *ice)
>   {
>   	u32 regval = qcom_ice_readl(ice, QCOM_ICE_REG_VERSION);
> @@ -237,6 +252,8 @@ static void qcom_ice_hwkm_init(struct qcom_ice *ice)
>   	/* Clear HWKM response FIFO before doing anything */
>   	qcom_ice_writel(ice, 0x8,
>   			HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS));
> +
> +	ice->hwkm_init_complete = true;
This this change should go with previous patch 3/12.
>   }
>   
>   int qcom_ice_enable(struct qcom_ice *ice)
> @@ -284,6 +301,51 @@ int qcom_ice_suspend(struct qcom_ice *ice)
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_suspend);
>   
> +/*
> + * HW dictates the internal mapping between the ICE and HWKM slots,
> + * which are different for different versions, make the translation
> + * here. For v1 however, the translation is done in trustzone.
> + */
> +static int translate_hwkm_slot(struct qcom_ice *ice, int slot)
> +{
> +	return (ice->hwkm_version == 1) ? slot : (slot * 2);
> +}
> +
> +static int qcom_ice_program_wrapped_key(struct qcom_ice *ice,
> +					const struct blk_crypto_key *key,
> +					u8 data_unit_size, int slot)
> +{
> +	int hwkm_slot;
> +	int err;
> +	union crypto_cfg cfg;
> +
> +	hwkm_slot = translate_hwkm_slot(ice, slot);
> +
> +	memset(&cfg, 0, sizeof(cfg));
> +	cfg.dusize = data_unit_size;
> +	cfg.capidx = QCOM_SCM_ICE_CIPHER_AES_256_XTS;
> +	cfg.cfge = 0x80;
use macro for constant value "0x80"
> +
> +	/* Clear CFGE */
> +	qcom_ice_writel(ice, 0x0, QCOM_ICE_LUT_KEYS_CRYPTOCFG_R16 +
> +				  QCOM_ICE_LUT_KEYS_CRYPTOCFG_OFFSET * slot);
> +
> +	/* Call trustzone to program the wrapped key using hwkm */
> +	err = qcom_scm_ice_set_key(hwkm_slot, key->raw, key->size,
> +				   QCOM_SCM_ICE_CIPHER_AES_256_XTS, data_unit_size);
> +	if (err) {
> +		pr_err("%s:SCM call Error: 0x%x slot %d\n", __func__, err,
> +		       slot);
> +		return err;
> +	}
> +
> +	/* Enable CFGE after programming key */
> +	qcom_ice_writel(ice, cfg.regval, QCOM_ICE_LUT_KEYS_CRYPTOCFG_R16 +
> +					 QCOM_ICE_LUT_KEYS_CRYPTOCFG_OFFSET * slot);
> +
> +	return err;
> +}
> +
>   int qcom_ice_program_key(struct qcom_ice *ice,
>   			 u8 algorithm_id, u8 key_size,
>   			 const struct blk_crypto_key *bkey,
> @@ -299,24 +361,31 @@ int qcom_ice_program_key(struct qcom_ice *ice,
>   
>   	/* Only AES-256-XTS has been tested so far. */
>   	if (algorithm_id != QCOM_ICE_CRYPTO_ALG_AES_XTS ||
> -	    key_size != QCOM_ICE_CRYPTO_KEY_SIZE_256) {
> +	    (key_size != QCOM_ICE_CRYPTO_KEY_SIZE_256 &&
> +	    key_size != QCOM_ICE_CRYPTO_KEY_SIZE_WRAPPED)) {
Can you please check the logic with && operation. the condition will 
always be false.
>   		dev_err_ratelimited(dev,
>   				    "Unhandled crypto capability; algorithm_id=%d, key_size=%d\n",
>   				    algorithm_id, key_size);
>   		return -EINVAL;
>   	}
>   
> -	memcpy(key.bytes, bkey->raw, AES_256_XTS_KEY_SIZE);
> -
> -	/* The SCM call requires that the key words are encoded in big endian */
> -	for (i = 0; i < ARRAY_SIZE(key.words); i++)
> -		__cpu_to_be32s(&key.words[i]);
> +	if (bkey->crypto_cfg.key_type == BLK_CRYPTO_KEY_TYPE_HW_WRAPPED) {
> +		if (!ice->use_hwkm)
> +			return -EINVAL;
having error log in failure case would help in debugging.
> +		err = qcom_ice_program_wrapped_key(ice, bkey, data_unit_size,
> +						   slot);
> +	} else {
> +		memcpy(key.bytes, bkey->raw, AES_256_XTS_KEY_SIZE);
>   
> -	err = qcom_scm_ice_set_key(slot, key.bytes, AES_256_XTS_KEY_SIZE,
> -				   QCOM_SCM_ICE_CIPHER_AES_256_XTS,
> -				   data_unit_size);
> +		/* The SCM call requires that the key words are encoded in big endian */
> +		for (i = 0; i < ARRAY_SIZE(key.words); i++)
> +			__cpu_to_be32s(&key.words[i]);
>   
> -	memzero_explicit(&key, sizeof(key));
> +		err = qcom_scm_ice_set_key(slot, key.bytes, AES_256_XTS_KEY_SIZE,
> +					   QCOM_SCM_ICE_CIPHER_AES_256_XTS,
> +					   data_unit_size);
> +		memzero_explicit(&key, sizeof(key));
> +	}
>   
>   	return err;
>   }
> @@ -324,7 +393,21 @@ EXPORT_SYMBOL_GPL(qcom_ice_program_key);
>   
>   int qcom_ice_evict_key(struct qcom_ice *ice, int slot)
>   {
> -	return qcom_scm_ice_invalidate_key(slot);
> +	int hwkm_slot = slot;
> +
> +	if (ice->use_hwkm) {
> +		hwkm_slot = translate_hwkm_slot(ice, slot);
> +	/*
> +	 * Ignore calls to evict key when HWKM is supported and hwkm init
> +	 * is not yet done. This is to avoid the clearing all slots call
> +	 * during a storage reset when ICE is still in legacy mode. HWKM slave
> +	 * in ICE takes care of zeroing out the keytable on reset.
> +	 */
> +		if (!ice->hwkm_init_complete)
> +			return 0;
> +	}
> +
> +	return qcom_scm_ice_invalidate_key(hwkm_slot);
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_evict_key);
>   
> @@ -334,6 +417,15 @@ bool qcom_ice_hwkm_supported(struct qcom_ice *ice)
>   }
>   EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported);
>   
> +int qcom_ice_derive_sw_secret(struct qcom_ice *ice, const u8 wkey[],
> +			      unsigned int wkey_size,
> +			      u8 sw_secret[BLK_CRYPTO_SW_SECRET_SIZE])
> +{
> +	return qcom_scm_derive_sw_secret(wkey, wkey_size,
> +					 sw_secret, BLK_CRYPTO_SW_SECRET_SIZE);
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_derive_sw_secret);
> +
>   static struct qcom_ice *qcom_ice_create(struct device *dev,
>   					void __iomem *base)
>   {
> diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h
> index 1f52e82e3e1c..dabe0d3a1fd0 100644
> --- a/include/soc/qcom/ice.h
> +++ b/include/soc/qcom/ice.h
> @@ -17,6 +17,7 @@ enum qcom_ice_crypto_key_size {
>   	QCOM_ICE_CRYPTO_KEY_SIZE_192		= 0x2,
>   	QCOM_ICE_CRYPTO_KEY_SIZE_256		= 0x3,
>   	QCOM_ICE_CRYPTO_KEY_SIZE_512		= 0x4,
> +	QCOM_ICE_CRYPTO_KEY_SIZE_WRAPPED	= 0x5,
>   };
>   
>   enum qcom_ice_crypto_alg {
> @@ -35,5 +36,8 @@ int qcom_ice_program_key(struct qcom_ice *ice,
>   			 u8 data_unit_size, int slot);
>   int qcom_ice_evict_key(struct qcom_ice *ice, int slot);
>   bool qcom_ice_hwkm_supported(struct qcom_ice *ice);
> +int qcom_ice_derive_sw_secret(struct qcom_ice *ice, const u8 wkey[],
> +			      unsigned int wkey_size,
> +			      u8 sw_secret[BLK_CRYPTO_SW_SECRET_SIZE]);
>   struct qcom_ice *of_qcom_ice_get(struct device *dev);
>   #endif /* __QCOM_ICE_H__ */

On 11/22/2023 11:08 AM, Gaurav Kashyap wrote:
> This patch contains two changes in UFS for wrapped keys.
> 1. Implements the blk_crypto_profile ops for generate, import
>     and prepare key apis.
> 2. Defines UFS vops for generate, import and prepare keys so
>     that vendors can hook into them.

re-write commit message as it is single change.
using numbering in commit message is indication of making multiple 
independent changes in single patch, which should be avoided.

> 
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> ---
>   drivers/ufs/core/ufshcd-crypto.c | 41 ++++++++++++++++++++++++++++++++
>   include/ufs/ufshcd.h             | 11 +++++++++
>   2 files changed, 52 insertions(+)
> 
> diff --git a/drivers/ufs/core/ufshcd-crypto.c b/drivers/ufs/core/ufshcd-crypto.c
> index 3edbca87c322..cf34f4a9cda8 100644
> --- a/drivers/ufs/core/ufshcd-crypto.c
> +++ b/drivers/ufs/core/ufshcd-crypto.c
> @@ -143,10 +143,51 @@ static int ufshcd_crypto_derive_sw_secret(struct blk_crypto_profile *profile,
>   	return -EOPNOTSUPP;
>   }
>   
> +static int ufshcd_crypto_generate_key(struct blk_crypto_profile *profile,
> +				      u8 lt_key[BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE])
> +{
> +	struct ufs_hba *hba =
> +		container_of(profile, struct ufs_hba, crypto_profile);
> +
> +	if (hba->vops && hba->vops->generate_key)
> +		return  hba->vops->generate_key(hba, lt_key);
Please fix double space.
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int ufshcd_crypto_prepare_key(struct blk_crypto_profile *profile,
> +				     const u8 *lt_key, size_t lt_key_size,
> +				     u8 eph_key[BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE])
> +{
> +	struct ufs_hba *hba =
> +		container_of(profile, struct ufs_hba, crypto_profile);
> +
> +	if (hba->vops && hba->vops->prepare_key)
> +		return  hba->vops->prepare_key(hba, lt_key, lt_key_size, eph_key);
Please fix double space.
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static int ufshcd_crypto_import_key(struct blk_crypto_profile *profile,
> +				    const u8 *imp_key, size_t imp_key_size,
> +				    u8 lt_key[BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE])
> +{
> +	struct ufs_hba *hba =
> +		container_of(profile, struct ufs_hba, crypto_profile);
> +
> +	if (hba->vops && hba->vops->import_key)
> +		return  hba->vops->import_key(hba, imp_key, imp_key_size, lt_key);
Please fix double space.
> +
> +	return -EOPNOTSUPP;
> +}
> +
>   static const struct blk_crypto_ll_ops ufshcd_crypto_ops = {
>   	.keyslot_program	= ufshcd_crypto_keyslot_program,
>   	.keyslot_evict		= ufshcd_crypto_keyslot_evict,
>   	.derive_sw_secret	= ufshcd_crypto_derive_sw_secret,
> +	.generate_key		= ufshcd_crypto_generate_key,
> +	.prepare_key		= ufshcd_crypto_prepare_key,
> +	.import_key		= ufshcd_crypto_import_key,
>   };
>   
>   static enum blk_crypto_mode_num
> diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
> index 86677788b5bd..49657a5d1e34 100644
> --- a/include/ufs/ufshcd.h
> +++ b/include/ufs/ufshcd.h
> @@ -321,6 +321,9 @@ struct ufs_pwr_mode_info {
>    * @config_scaling_param: called to configure clock scaling parameters
>    * @program_key: program or evict an inline encryption key
>    * @derive_sw_secret: derive sw secret from a wrapped key
> + * @generate_key: generate a storage key and return longterm wrapped key
> + * @prepare_key: unwrap longterm key and return ephemeral wrapped key
> + * @import_key: import sw storage key and return longterm wrapped key
>    * @event_notify: called to notify important events
>    * @reinit_notify: called to notify reinit of UFSHCD during max gear switch
>    * @mcq_config_resource: called to configure MCQ platform resources
> @@ -368,6 +371,14 @@ struct ufs_hba_variant_ops {
>   	int	(*derive_sw_secret)(struct ufs_hba *hba, const u8 wkey[],
>   				    unsigned int wkey_size,
>   				    u8 sw_secret[BLK_CRYPTO_SW_SECRET_SIZE]);
> +	int	(*generate_key)(struct ufs_hba *hba,
> +				u8 lt_key[BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE]);
> +	int	(*prepare_key)(struct ufs_hba *hba,
> +			       const u8 *lt_key, size_t lt_key_size,
> +			       u8 eph_key[BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE]);
> +	int	(*import_key)(struct ufs_hba *hba,
> +			      const u8 *imp_key, size_t imp_key_size,
> +			      u8 lt_key[BLK_CRYPTO_MAX_HW_WRAPPED_KEY_SIZE]);
>   	void	(*event_notify)(struct ufs_hba *hba,
>   				enum ufs_event_type evt, void *data);
>   	void	(*reinit_notify)(struct ufs_hba *);

On 12/8/2023 9:21 AM, Bjorn Andersson wrote:
> On Tue, Nov 21, 2023 at 09:38:16PM -0800, Gaurav Kashyap wrote:
>> The Inline Crypto Engine (ICE) for UFS supports the
>> Hardware Key Manager (hwkm) to securely manage storage
>> keys. Enable using this hardware on sm8650.
> 
> I'm unable to understand why the size of the reg changes based on this
> motivation.
> 
> Regards,
> Bjorn
>
Actual size of register space size is 0x18000 (as per IPCAT). But all 
registers space are not used.

HWKM register offset is start at 0x8000. Hence needed to expand reg map 
size.


>>
>> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sm8650.dtsi | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> index bbebe15437aa..b61066210e09 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
>> @@ -763,7 +763,8 @@ rng: rng@10c3000 {
>>   		ice: crypto@1d88000 {
>>   			compatible = "qcom,sm8650-inline-crypto-engine",
>>   				     "qcom,inline-crypto-engine";
>> -			reg = <0 0x01d88000 0 0x8000>;
>> +			reg = <0 0x01d88000 0 0x10000>;
>> +			qcom,ice-use-hwkm;
>>   
>>   			clocks = <&gcc GCC_UFS_PHY_ICE_CORE_CLK>;
>>   		};
>> -- 
>> 2.25.1
>>
>>
>

On 11/22/2023 11:08 AM, Gaurav Kashyap wrote:
> The Inline Crypto Engine (ICE) for UFS supports the Hardware Key Manager 
> (hwkm) to securely manage storage keys. Enable using this hardware on 
> sm8650. Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com> ---
Reviewed-by: Om Prakash Singh <quic_omprsing@quicinc.com>

Hello Bjorn,

On 12/07/2023, Bjorn Andersson wrote:
> On Tue, Nov 21, 2023 at 09:38:08PM -0800, Gaurav Kashyap wrote:
> > Qualcomm's ICE (Inline Crypto Engine) contains a proprietary key
> > management hardware called Hardware Key Manager (HWKM).
> > This patch integrates HWKM support in ICE when it is available. HWKM
> > primarily provides hardware wrapped key support where the ICE
> > (storage) keys are not available in software and protected in
> > hardware.
> >
> > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> > ---
> >  drivers/soc/qcom/ice.c | 133
> ++++++++++++++++++++++++++++++++++++++++-
> >  include/soc/qcom/ice.h |   1 +
> >  2 files changed, 133 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c index
> > 6f941d32fffb..adf9cab848fa 100644
> > --- a/drivers/soc/qcom/ice.c
> > +++ b/drivers/soc/qcom/ice.c
> > @@ -26,6 +26,19 @@
> >  #define QCOM_ICE_REG_FUSE_SETTING            0x0010
> >  #define QCOM_ICE_REG_BIST_STATUS             0x0070
> >  #define QCOM_ICE_REG_ADVANCED_CONTROL                0x1000
> > +#define QCOM_ICE_REG_CONTROL                 0x0
> > +/* QCOM ICE HWKM registers */
> > +#define QCOM_ICE_REG_HWKM_TZ_KM_CTL                  0x1000
> > +#define QCOM_ICE_REG_HWKM_TZ_KM_STATUS                       0x1004
> > +#define QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS     0x2008
> > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_0                       0x5000
> > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_1                       0x5004
> > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_2                       0x5008
> > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_3                       0x500C
> > +#define QCOM_ICE_REG_HWKM_BANK0_BBAC_4                       0x5010
> > +
> > +#define QCOM_ICE_HWKM_BIST_DONE_V1_VAL               0x11
> > +#define QCOM_ICE_HWKM_BIST_DONE_V2_VAL               0x287
> >
> >  /* BIST ("built-in self-test") status flags */
> >  #define QCOM_ICE_BIST_STATUS_MASK            GENMASK(31, 28)
> > @@ -34,6 +47,9 @@
> >  #define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK  0x2  #define
> > QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK  0x4
> >
> > +#define QCOM_ICE_HWKM_REG_OFFSET     0x8000
> > +#define HWKM_OFFSET(reg)             ((reg) +
> QCOM_ICE_HWKM_REG_OFFSET)
> > +
> >  #define qcom_ice_writel(engine, val, reg)    \
> >       writel((val), (engine)->base + (reg))
> >
> > @@ -46,6 +62,9 @@ struct qcom_ice {
> >       struct device_link *link;
> >
> >       struct clk *core_clk;
> > +     u8 hwkm_version;
> > +     bool use_hwkm;
> > +     bool hwkm_init_complete;
> >  };
> >
> >  static bool qcom_ice_check_supported(struct qcom_ice *ice) @@ -63,8
> > +82,26 @@ static bool qcom_ice_check_supported(struct qcom_ice *ice)
> >               return false;
> >       }
> >
> > +     if (major >= 4 || (major == 3 && minor == 2 && step >= 1))
> > +             ice->hwkm_version = 2;
> > +     else if (major == 3 && minor == 2)
> > +             ice->hwkm_version = 1;
> > +     else
> > +             ice->hwkm_version = 0;
> > +
> > +     if (ice->hwkm_version == 0)
> > +             ice->use_hwkm = false;
> > +
> >       dev_info(dev, "Found QC Inline Crypto Engine (ICE) v%d.%d.%d\n",
> >                major, minor, step);
> > +     if (!ice->hwkm_version)
> > +             dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not
> > + supported");
> 
> So for a version < 3.2.0, we will dev_info() three times, one stating the
> version found, one stating that HWKM is not supported, and then below one
> saying that HWKM is not used.
> 
> > +     else
> > +             dev_info(dev, "QC ICE HWKM (Hardware Key Manager) version =
> %d",
> > +                      ice->hwkm_version);
> 
> And for version >= 3.2.0 we will dev_info() two times.
> 
> 
> To the vast majority of readers of the kernel log none of these info-prints are
> useful - it's just spam.
> 
> I'd prefer that it was turned into dev_dbg(), which those who want to know
> (e.g. during bringup) can enable. But that's a separate change, please start by
> consolidating your information into a single line printed in the log.

Noted for next patch.

> 
> > +
> > +     if (!ice->use_hwkm)
> > +             dev_info(dev, "QC ICE HWKM (Hardware Key Manager) not
> > + used");
> >
> >       /* If fuses are blown, ICE might not work in the standard way. */
> >       regval = qcom_ice_readl(ice, QCOM_ICE_REG_FUSE_SETTING); @@
> > -113,10 +150,14 @@ static void qcom_ice_optimization_enable(struct
> qcom_ice *ice)
> >   * fails, so we needn't do it in software too, and (c) properly testing
> >   * storage encryption requires testing the full storage stack anyway,
> >   * and not relying on hardware-level self-tests.
> > + *
> > + * However, we still care about if HWKM BIST failed (when supported)
> > + as
> > + * important functionality would fail later, so disable hwkm on failure.
> >   */
> >  static int qcom_ice_wait_bist_status(struct qcom_ice *ice)  {
> >       u32 regval;
> > +     u32 bist_done_val;
> 
> The "val" suffix indicates that this would be a "value", but it's actually a
> register offset. "bist_done_reg" would be better.
> 

Noted for next patch.

> >       int err;
> >
> >       err = readl_poll_timeout(ice->base + QCOM_ICE_REG_BIST_STATUS,
> > @@ -125,15 +166,95 @@ static int qcom_ice_wait_bist_status(struct
> qcom_ice *ice)
> >       if (err)
> >               dev_err(ice->dev, "Timed out waiting for ICE self-test
> > to complete\n");
> >
> > +     if (ice->use_hwkm) {
> > +             bist_done_val = (ice->hwkm_version == 1) ?
> > +                              QCOM_ICE_HWKM_BIST_DONE_V1_VAL :
> > +                              QCOM_ICE_HWKM_BIST_DONE_V2_VAL;
> > +             if (qcom_ice_readl(ice,
> > +
> HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_STATUS)) !=
> > +                                bist_done_val) {
> > +                     dev_warn(ice->dev, "HWKM BIST error\n");
> 
> Sounds like a error to me, wouldn't dev_err() be suitable?
> 

Yes, noted for next patch.

> > +                     ice->use_hwkm = false;
> > +             }
> > +     }
> >       return err;
> >  }
> >
> > +static void qcom_ice_enable_standard_mode(struct qcom_ice *ice) {
> > +     u32 val = 0;
> > +
> > +     if (!ice->use_hwkm)
> > +             return;
> > +
> > +     /*
> > +      * When ICE is in standard (hwkm) mode, it supports HW wrapped
> > +      * keys, and when it is in legacy mode, it only supports standard
> > +      * (non HW wrapped) keys.
> > +      *
> > +      * Put ICE in standard mode, ICE defaults to legacy mode.
> > +      * Legacy mode - ICE HWKM slave not supported.
> > +      * Standard mode - ICE HWKM slave supported.
> > +      *
> > +      * Depending on the version of HWKM, it is controlled by different
> > +      * registers in ICE.
> > +      */
> > +     if (ice->hwkm_version >= 2) {
> > +             val = qcom_ice_readl(ice, QCOM_ICE_REG_CONTROL);
> > +             val = val & 0xFFFFFFFE;
> > +             qcom_ice_writel(ice, val, QCOM_ICE_REG_CONTROL);
> > +     } else {
> > +             qcom_ice_writel(ice, 0x7,
> > +                             HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> > +     }
> > +}
> > +
> > +static void qcom_ice_hwkm_init(struct qcom_ice *ice) {
> > +     if (!ice->use_hwkm)
> > +             return;
> > +
> > +     /* Disable CRC checks. This HWKM feature is not used. */
> > +     qcom_ice_writel(ice, 0x6,
> > +                     HWKM_OFFSET(QCOM_ICE_REG_HWKM_TZ_KM_CTL));
> > +
> > +     /*
> > +      * Give register bank of the HWKM slave access to read and modify
> > +      * the keyslots in ICE HWKM slave. Without this, trustzone will not
> > +      * be able to program keys into ICE.
> > +      */
> > +     qcom_ice_writel(ice, 0xFFFFFFFF,
> > +                     HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_0));
> 
> This line is 86 characters long if left unwrapped. You're allowed to go over 80
> characters if it makes the code more readable, so please do so for these and
> below.
> 

Noted for next patch.

> > +     qcom_ice_writel(ice, 0xFFFFFFFF,
> > +                     HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_1));
> > +     qcom_ice_writel(ice, 0xFFFFFFFF,
> > +                     HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_2));
> > +     qcom_ice_writel(ice, 0xFFFFFFFF,
> > +                     HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_3));
> > +     qcom_ice_writel(ice, 0xFFFFFFFF,
> > +                     HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BBAC_4));
> > +
> > +     /* Clear HWKM response FIFO before doing anything */
> > +     qcom_ice_writel(ice, 0x8,
> > +
> >
> +HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STATUS));
> > +}
> > +
> >  int qcom_ice_enable(struct qcom_ice *ice)  {
> > +     int err;
> > +
> >       qcom_ice_low_power_mode_enable(ice);
> >       qcom_ice_optimization_enable(ice);
> >
> > -     return qcom_ice_wait_bist_status(ice);
> > +     qcom_ice_enable_standard_mode(ice);
> > +
> > +     err = qcom_ice_wait_bist_status(ice);
> > +     if (err)
> > +             return err;
> > +
> > +     qcom_ice_hwkm_init(ice);
> > +
> > +     return err;
> >  }
> >  EXPORT_SYMBOL_GPL(qcom_ice_enable);
> >
> > @@ -149,6 +270,8 @@ int qcom_ice_resume(struct qcom_ice *ice)
> >               return err;
> >       }
> >
> > +     qcom_ice_enable_standard_mode(ice);
> > +     qcom_ice_hwkm_init(ice);
> >       return qcom_ice_wait_bist_status(ice);  }
> > EXPORT_SYMBOL_GPL(qcom_ice_resume);
> > @@ -205,6 +328,12 @@ int qcom_ice_evict_key(struct qcom_ice *ice, int
> > slot)  }  EXPORT_SYMBOL_GPL(qcom_ice_evict_key);
> >
> > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice) {
> > +     return ice->use_hwkm;
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported);
> > +
> >  static struct qcom_ice *qcom_ice_create(struct device *dev,
> >                                       void __iomem *base)  { @@ -239,6
> > +368,8 @@ static struct qcom_ice *qcom_ice_create(struct device *dev,
> >               engine->core_clk = devm_clk_get_enabled(dev, NULL);
> >       if (IS_ERR(engine->core_clk))
> >               return ERR_CAST(engine->core_clk);
> > +     engine->use_hwkm = of_property_read_bool(dev->of_node,
> > +                                              "qcom,ice-use-hwkm");
> 
> Under what circumstances would we, with version >= 3.2, not specify this
> flag?
> 
> Thanks,
> Bjorn
> 

For 3.2.0 versions and above where all the Trustzone support is not present for wrapped keys, 
using Qualcomm ICE means using standard (non-wrapped) keys. This cannot work in conjunction
with "HWKM mode" being enabled, and ICE needs to be in "Legacy Mode".  HWKM mode is
basically a bunch of register initializations.

Ideally, there should not be any SoC supporting HWKM which does not have all the support, with
a pure hardware version based decision. But unfortunately, we need an explicit switch to 
support the above scenario.

> >
> >       if (!qcom_ice_check_supported(engine))
> >               return ERR_PTR(-EOPNOTSUPP); diff --git
> > a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h index
> > 9dd835dba2a7..1f52e82e3e1c 100644
> > --- a/include/soc/qcom/ice.h
> > +++ b/include/soc/qcom/ice.h
> > @@ -34,5 +34,6 @@ int qcom_ice_program_key(struct qcom_ice *ice,
> >                        const struct blk_crypto_key *bkey,
> >                        u8 data_unit_size, int slot);  int
> > qcom_ice_evict_key(struct qcom_ice *ice, int slot);
> > +bool qcom_ice_hwkm_supported(struct qcom_ice *ice);
> >  struct qcom_ice *of_qcom_ice_get(struct device *dev);  #endif /*
> > __QCOM_ICE_H__ */
> > --
> > 2.25.1
> >
> >

Hello Om

On 12/07/2023, Om Prakash Singh wrote:
> On 11/22/2023 11:08 AM, Gaurav Kashyap wrote:
> > Now that HWKM support is added to ICE, extend the ICE driver to
> > support hardware wrapped keys programming coming in from the storage
> > controllers (ufs and emmc). The patches that follow will add ufs and
> > emmc support.
> >
> > Derive software secret support is also added by forwarding the call
> > the corresponding scm api.
> >
> > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> > ---
> >   drivers/soc/qcom/ice.c | 114
> +++++++++++++++++++++++++++++++++++++----
> >   include/soc/qcom/ice.h |   4 ++
> >   2 files changed, 107 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c index
> > adf9cab848fa..ee7c0beef3d2 100644
> > --- a/drivers/soc/qcom/ice.c
> > +++ b/drivers/soc/qcom/ice.c
> > @@ -27,6 +27,8 @@
> >   #define QCOM_ICE_REG_BIST_STATUS		0x0070
> >   #define QCOM_ICE_REG_ADVANCED_CONTROL		0x1000
> >   #define QCOM_ICE_REG_CONTROL			0x0
> > +#define QCOM_ICE_LUT_KEYS_CRYPTOCFG_R16		0x4040
> > +
> >   /* QCOM ICE HWKM registers */
> >   #define QCOM_ICE_REG_HWKM_TZ_KM_CTL
> 	0x1000
> >   #define QCOM_ICE_REG_HWKM_TZ_KM_STATUS
> 	0x1004
> > @@ -37,6 +39,7 @@
> >   #define QCOM_ICE_REG_HWKM_BANK0_BBAC_3
> 	0x500C
> >   #define QCOM_ICE_REG_HWKM_BANK0_BBAC_4
> 	0x5010
> >
> > +/* QCOM ICE HWKM BIST vals */
> >   #define QCOM_ICE_HWKM_BIST_DONE_V1_VAL		0x11
> >   #define QCOM_ICE_HWKM_BIST_DONE_V2_VAL		0x287
> >
> > @@ -47,6 +50,8 @@
> >   #define QCOM_ICE_FORCE_HW_KEY0_SETTING_MASK	0x2
> >   #define QCOM_ICE_FORCE_HW_KEY1_SETTING_MASK	0x4
> >
> > +#define QCOM_ICE_LUT_KEYS_CRYPTOCFG_OFFSET	0x80
> > +
> >   #define QCOM_ICE_HWKM_REG_OFFSET	0x8000
> >   #define HWKM_OFFSET(reg)		((reg) +
> QCOM_ICE_HWKM_REG_OFFSET)
> >
> > @@ -67,6 +72,16 @@ struct qcom_ice {
> >   	bool hwkm_init_complete;
> >   };
> >
> > +union crypto_cfg {
> > +	__le32 regval;
> > +	struct {
> > +		u8 dusize;
> > +		u8 capidx;
> > +		u8 reserved;
> > +		u8 cfge;
> > +	};
> > +};
> > +
> >   static bool qcom_ice_check_supported(struct qcom_ice *ice)
> >   {
> >   	u32 regval = qcom_ice_readl(ice, QCOM_ICE_REG_VERSION); @@ -
> 237,6
> > +252,8 @@ static void qcom_ice_hwkm_init(struct qcom_ice *ice)
> >   	/* Clear HWKM response FIFO before doing anything */
> >   	qcom_ice_writel(ice, 0x8,
> >
> 	HWKM_OFFSET(QCOM_ICE_REG_HWKM_BANK0_BANKN_IRQ_STA
> TUS));
> > +
> > +	ice->hwkm_init_complete = true;
> This this change should go with previous patch 3/12.

Okay, makes sense.

> >   }
> >
> >   int qcom_ice_enable(struct qcom_ice *ice) @@ -284,6 +301,51 @@ int
> > qcom_ice_suspend(struct qcom_ice *ice)
> >   }
> >   EXPORT_SYMBOL_GPL(qcom_ice_suspend);
> >
> > +/*
> > + * HW dictates the internal mapping between the ICE and HWKM slots,
> > + * which are different for different versions, make the translation
> > + * here. For v1 however, the translation is done in trustzone.
> > + */
> > +static int translate_hwkm_slot(struct qcom_ice *ice, int slot) {
> > +	return (ice->hwkm_version == 1) ? slot : (slot * 2); }
> > +
> > +static int qcom_ice_program_wrapped_key(struct qcom_ice *ice,
> > +					const struct blk_crypto_key *key,
> > +					u8 data_unit_size, int slot)
> > +{
> > +	int hwkm_slot;
> > +	int err;
> > +	union crypto_cfg cfg;
> > +
> > +	hwkm_slot = translate_hwkm_slot(ice, slot);
> > +
> > +	memset(&cfg, 0, sizeof(cfg));
> > +	cfg.dusize = data_unit_size;
> > +	cfg.capidx = QCOM_SCM_ICE_CIPHER_AES_256_XTS;
> > +	cfg.cfge = 0x80;
> use macro for constant value "0x80"
> > +
> > +	/* Clear CFGE */
> > +	qcom_ice_writel(ice, 0x0, QCOM_ICE_LUT_KEYS_CRYPTOCFG_R16 +
> > +				  QCOM_ICE_LUT_KEYS_CRYPTOCFG_OFFSET
> * slot);
> > +
> > +	/* Call trustzone to program the wrapped key using hwkm */
> > +	err = qcom_scm_ice_set_key(hwkm_slot, key->raw, key->size,
> > +				   QCOM_SCM_ICE_CIPHER_AES_256_XTS,
> data_unit_size);
> > +	if (err) {
> > +		pr_err("%s:SCM call Error: 0x%x slot %d\n", __func__, err,
> > +		       slot);
> > +		return err;
> > +	}
> > +
> > +	/* Enable CFGE after programming key */
> > +	qcom_ice_writel(ice, cfg.regval,
> QCOM_ICE_LUT_KEYS_CRYPTOCFG_R16 +
> > +
> QCOM_ICE_LUT_KEYS_CRYPTOCFG_OFFSET * slot);
> > +
> > +	return err;
> > +}
> > +
> >   int qcom_ice_program_key(struct qcom_ice *ice,
> >   			 u8 algorithm_id, u8 key_size,
> >   			 const struct blk_crypto_key *bkey, @@ -299,24
> +361,31 @@ int
> > qcom_ice_program_key(struct qcom_ice *ice,
> >
> >   	/* Only AES-256-XTS has been tested so far. */
> >   	if (algorithm_id != QCOM_ICE_CRYPTO_ALG_AES_XTS ||
> > -	    key_size != QCOM_ICE_CRYPTO_KEY_SIZE_256) {
> > +	    (key_size != QCOM_ICE_CRYPTO_KEY_SIZE_256 &&
> > +	    key_size != QCOM_ICE_CRYPTO_KEY_SIZE_WRAPPED)) {
> Can you please check the logic with && operation. the condition will always
> be false.

No, the condition won't always be false, my v2 patches were wrong which was pointed out by Neil
https://lore.kernel.org/all/fcbf6dee-aa6a-4af8-9ff1-495adbcb5a57@linaro.org/

The condition would check if the size passed is either 256 or wrapped.

> >   		dev_err_ratelimited(dev,
> >   				    "Unhandled crypto capability;
> algorithm_id=%d, key_size=%d\n",
> >   				    algorithm_id, key_size);
> >   		return -EINVAL;
> >   	}
> >
> > -	memcpy(key.bytes, bkey->raw, AES_256_XTS_KEY_SIZE);
> > -
> > -	/* The SCM call requires that the key words are encoded in big
> endian */
> > -	for (i = 0; i < ARRAY_SIZE(key.words); i++)
> > -		__cpu_to_be32s(&key.words[i]);
> > +	if (bkey->crypto_cfg.key_type ==
> BLK_CRYPTO_KEY_TYPE_HW_WRAPPED) {
> > +		if (!ice->use_hwkm)
> > +			return -EINVAL;
> having error log in failure case would help in debugging.
> > +		err = qcom_ice_program_wrapped_key(ice, bkey,
> data_unit_size,
> > +						   slot);
> > +	} else {
> > +		memcpy(key.bytes, bkey->raw, AES_256_XTS_KEY_SIZE);
> >
> > -	err = qcom_scm_ice_set_key(slot, key.bytes,
> AES_256_XTS_KEY_SIZE,
> > -				   QCOM_SCM_ICE_CIPHER_AES_256_XTS,
> > -				   data_unit_size);
> > +		/* The SCM call requires that the key words are encoded in
> big endian */
> > +		for (i = 0; i < ARRAY_SIZE(key.words); i++)
> > +			__cpu_to_be32s(&key.words[i]);
> >
> > -	memzero_explicit(&key, sizeof(key));
> > +		err = qcom_scm_ice_set_key(slot, key.bytes,
> AES_256_XTS_KEY_SIZE,
> > +
> QCOM_SCM_ICE_CIPHER_AES_256_XTS,
> > +					   data_unit_size);
> > +		memzero_explicit(&key, sizeof(key));
> > +	}
> >
> >   	return err;
> >   }
> > @@ -324,7 +393,21 @@ EXPORT_SYMBOL_GPL(qcom_ice_program_key);
> >
> >   int qcom_ice_evict_key(struct qcom_ice *ice, int slot)
> >   {
> > -	return qcom_scm_ice_invalidate_key(slot);
> > +	int hwkm_slot = slot;
> > +
> > +	if (ice->use_hwkm) {
> > +		hwkm_slot = translate_hwkm_slot(ice, slot);
> > +	/*
> > +	 * Ignore calls to evict key when HWKM is supported and hwkm init
> > +	 * is not yet done. This is to avoid the clearing all slots call
> > +	 * during a storage reset when ICE is still in legacy mode. HWKM slave
> > +	 * in ICE takes care of zeroing out the keytable on reset.
> > +	 */
> > +		if (!ice->hwkm_init_complete)
> > +			return 0;
> > +	}
> > +
> > +	return qcom_scm_ice_invalidate_key(hwkm_slot);
> >   }
> >   EXPORT_SYMBOL_GPL(qcom_ice_evict_key);
> >
> > @@ -334,6 +417,15 @@ bool qcom_ice_hwkm_supported(struct qcom_ice
> *ice)
> >   }
> >   EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported);
> >
> > +int qcom_ice_derive_sw_secret(struct qcom_ice *ice, const u8 wkey[],
> > +			      unsigned int wkey_size,
> > +			      u8 sw_secret[BLK_CRYPTO_SW_SECRET_SIZE])
> > +{
> > +	return qcom_scm_derive_sw_secret(wkey, wkey_size,
> > +					 sw_secret,
> BLK_CRYPTO_SW_SECRET_SIZE); }
> > +EXPORT_SYMBOL_GPL(qcom_ice_derive_sw_secret);
> > +
> >   static struct qcom_ice *qcom_ice_create(struct device *dev,
> >   					void __iomem *base)
> >   {
> > diff --git a/include/soc/qcom/ice.h b/include/soc/qcom/ice.h index
> > 1f52e82e3e1c..dabe0d3a1fd0 100644
> > --- a/include/soc/qcom/ice.h
> > +++ b/include/soc/qcom/ice.h
> > @@ -17,6 +17,7 @@ enum qcom_ice_crypto_key_size {
> >   	QCOM_ICE_CRYPTO_KEY_SIZE_192		= 0x2,
> >   	QCOM_ICE_CRYPTO_KEY_SIZE_256		= 0x3,
> >   	QCOM_ICE_CRYPTO_KEY_SIZE_512		= 0x4,
> > +	QCOM_ICE_CRYPTO_KEY_SIZE_WRAPPED	= 0x5,
> >   };
> >
> >   enum qcom_ice_crypto_alg {
> > @@ -35,5 +36,8 @@ int qcom_ice_program_key(struct qcom_ice *ice,
> >   			 u8 data_unit_size, int slot);
> >   int qcom_ice_evict_key(struct qcom_ice *ice, int slot);
> >   bool qcom_ice_hwkm_supported(struct qcom_ice *ice);
> > +int qcom_ice_derive_sw_secret(struct qcom_ice *ice, const u8 wkey[],
> > +			      unsigned int wkey_size,
> > +			      u8 sw_secret[BLK_CRYPTO_SW_SECRET_SIZE]);
> >   struct qcom_ice *of_qcom_ice_get(struct device *dev);
> >   #endif /* __QCOM_ICE_H__ */