RE: [PATCH v5 04/15] soc: qcom: ice: add hwkm support in ice

["Gaurav Kashyap (QUIC)" <quic_gaurkash@quicinc.com>]

Hello Neil,

On 06/18/2024 12:14 AM PDT, Neil Armstrong wrote:
> On 17/06/2024 02:50, 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.
> >
> > When HWKM software support is not fully available (from Trustzone),
> > there can be a scenario where the ICE hardware supports HWKM, but it
> > cannot be used for wrapped keys. In this case, standard keys have to
> > be used without using HWKM. Hence, providing a toggle controlled by a
> > devicetree entry to use HWKM or not.
> >
> > Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> > Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> > ---
> >   drivers/soc/qcom/ice.c | 153
> +++++++++++++++++++++++++++++++++++++++--
> >   include/soc/qcom/ice.h |   1 +
> >   2 files changed, 150 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/ice.c b/drivers/soc/qcom/ice.c index
> > 6f941d32fffb..d5e74cf2946b 100644
> > --- a/drivers/soc/qcom/ice.c
> > +++ b/drivers/soc/qcom/ice.c
> > @@ -26,6 +26,40 @@
> 
> <snip>
> 
> > +
> >   static struct qcom_ice *qcom_ice_create(struct device *dev,
> >                                       void __iomem *base)
> >   {
> > @@ -239,6 +382,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");
> 
> Please drop this property and instead add an scm function calling:
> 
> __qcom_scm_is_call_available(QCOM_SCM_SVC_ES,
> QCOM_SCM_ES_DERIVE_SW_SECRET)
> 
> like
> 
> bool qcom_scm_derive_sw_secret_available(void)
> {
>         if (!__qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_ES,
>                                           QCOM_SCM_ES_DERIVE_SW_SECRET))
>                 return false;
> 
>         return true;
> }
> 
> You may perhaps only call qcom_scm_derive_sw_secret_available() for
> some ICE versions.
> 
> Neil

The issue here is that for the same ICE version, based on the chipset,
there might be different configurations.

Is it acceptable to use the addressable size from DTSI instead?
Meaning, if it 0x8000, it would take the legacy route, and only when it has been
updated to 0x10000, we would use HWKM and wrapped keys.

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

Regards,
Gaurav