RE: [PATCH v3 03/12] soc: qcom: ice: add hwkm support in ice

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Om

On 12/07/2023, Om Prakash Singh wrote:
> 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@xxxxxxxxxxx>
> > ---
> >   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
> 

The reason this is being added is to support standard keys on chipsets which contain a HWKM version, but
does not have the capability in Trustzone (for example) to completely use wrapped keys.
Using a HWKM DTSI entry will let you explicitly enable/disable wrapped key (or hwkm) support.

In general too, I think it is good to allow support for both standard and wrapped keys.

> > +	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.
> 

Shouldn't the hardware version be read from the hardware?

> >   	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 ?

Noted for the next patch.

> > +		}
> > +	}
> >   	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.
> 

We currently use HWKM version 2 on SM8650.
If you are specifying only versions greater than 2,
At best, we won't need any changes to support let's say a version 3
At worst, changes need to be made to support version specific changes,
which we would have had to make anyway.

> > +		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.

I agree, noted for the next patch.

> > +
> 	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_STA
> TUS));
> > +}
> > +
> >   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);
> }

It is already gated within the new APIs

> > +
> > +	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__ */




[Index of Archives]     [Linux RAID]     [Linux SCSI]     [Linux ATA RAID]     [IDE]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Device Mapper]

  Powered by Linux