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@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;
> > + 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
> >
> >
