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