Re: [PATCH v2 03/10] soc: qcom: ice: add hwkm support in ice

[Neil Armstrong <neil.armstrong@linaro.org>]

Hi,

On 19/07/2023 19:04, 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 | 112 ++++++++++++++++++++++++++++++++++++++++-
>   include/soc/qcom/ice.h |   1 +
>   2 files changed, 112 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c
> index d19f674bb1b6..242306d13049 100644
> --- a/drivers/soc/qcom/ice.c
> +++ b/drivers/soc/qcom/ice.c
> @@ -24,6 +24,18 @@
>   #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_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)
> @@ -32,6 +44,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)

The current upstream ICE memory region is only 0x8000, so you should
probably check the memory region size before enabling HWKM otherwise
you'll access unmapped memory for non-updated DT.

Neil

> +
>   #define qcom_ice_writel(engine, val, reg)	\
>   	writel((val), (engine)->base + (reg))
>   
> @@ -44,6 +59,7 @@ struct qcom_ice {
>   	struct device_link *link;
>   
>   	struct clk *core_clk;
> +	u8 hwkm_version;
>   };
>   
>   static bool qcom_ice_check_supported(struct qcom_ice *ice)
> @@ -61,8 +77,20 @@ 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;
> +
>   	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 fuses are blown, ICE might not work in the standard way. */
>   	regval = qcom_ice_readl(ice, QCOM_ICE_REG_FUSE_SETTING);
> @@ -111,10 +139,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,
> @@ -123,15 +155,87 @@ 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->hwkm_version) {
> +		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->hwkm_version = 0;
> +		}
> +	}
>   	return err;
>   }
>   
> +static void qcom_ice_enable_standard_mode(struct qcom_ice *ice)
> +{
> +	u32 val = 0;
> +
> +	if (!ice->hwkm_version)
> +		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->hwkm_version)
> +		return;
> +
> +	/*
> +	 * 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));
> +}
> +
>   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);
>   
> @@ -203,6 +307,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->hwkm_version > 0);
> +}
> +EXPORT_SYMBOL_GPL(qcom_ice_hwkm_supported);
> +
>   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 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__ */