diff mbox series

[v5,13/15] dt-bindings: crypto: ice: document the hwkm property

Message ID 20240617005825.1443206-14-quic_gaurkash@quicinc.com
State New
Headers show
Series Hardware wrapped key support for qcom ice and ufs | expand

Commit Message

Gaurav Kashyap (QUIC) June 17, 2024, 12:51 a.m. UTC 
When Qualcomm's Inline Crypto Engine (ICE) contains Hardware
Key Manager (HWKM), and the 'HWKM' mode is enabled, it
supports wrapped keys. However, this also requires firmware
support in Trustzone to work correctly, which may not be available
on all chipsets. In the above scenario, ICE needs to support standard
keys even though HWKM is integrated from a hardware perspective.

Introducing this property so that Hardware wrapped key support
can be enabled/disabled from software based on chipset firmware,
and not just based on hardware version.

Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
---
 .../bindings/crypto/qcom,inline-crypto-engine.yaml     | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Krzysztof Kozlowski June 17, 2024, 7:16 a.m. UTC | #1 
On 17/06/2024 02:51, Gaurav Kashyap wrote:
> +  qcom,ice-use-hwkm:
> +    type: boolean
> +    description:
> +      Use the supported Hardware Key Manager (HWKM) in Qualcomm ICE
> +      to support wrapped keys. Having this entry helps scenarios where
> +      the ICE hardware supports HWKM, but the Trustzone firmware does
> +      not have the full capability to use this HWKM and support wrapped
> +      keys. Not having this entry enabled would make ICE function in
> +      non-HWKM mode supporting standard keys.

No changelog, previous comments and discussion ignored.

NAK

Best regards,
Krzysztof
Konrad Dybcio June 17, 2024, 5:39 p.m. UTC | #2 
On 6/17/24 02:51, Gaurav Kashyap wrote:
> When Qualcomm's Inline Crypto Engine (ICE) contains Hardware
> Key Manager (HWKM), and the 'HWKM' mode is enabled, it
> supports wrapped keys. However, this also requires firmware
> support in Trustzone to work correctly, which may not be available
> on all chipsets. In the above scenario, ICE needs to support standard
> keys even though HWKM is integrated from a hardware perspective.
> 
> Introducing this property so that Hardware wrapped key support
> can be enabled/disabled from software based on chipset firmware,
> and not just based on hardware version.
> 
> Tested-by: Neil Armstrong <neil.armstrong@linaro.org>
> Signed-off-by: Gaurav Kashyap <quic_gaurkash@quicinc.com>
> ---
>   .../bindings/crypto/qcom,inline-crypto-engine.yaml     | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> index 0304f074cf08..0bb4d008f961 100644
> --- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> +++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
> @@ -27,6 +27,16 @@ properties:
>     clocks:
>       maxItems: 1
>   
> +  qcom,ice-use-hwkm:
> +    type: boolean
> +    description:
> +      Use the supported Hardware Key Manager (HWKM) in Qualcomm ICE
> +      to support wrapped keys. Having this entry helps scenarios where
> +      the ICE hardware supports HWKM, but the Trustzone firmware does
> +      not have the full capability to use this HWKM and support wrapped
> +      keys. Not having this entry enabled would make ICE function in
> +      non-HWKM mode supporting standard keys.

Just check if qcom_scm_derive_sw_secret is available instead

Konrad
Gaurav Kashyap (QUIC) June 18, 2024, 12:35 a.m. UTC | #3 
Hello Krzysztof

On   06/17/2024 12:17 AM PDT, Krzysztof Kozlowski wrote:
> On 17/06/2024 02:51, Gaurav Kashyap wrote:
> > +  qcom,ice-use-hwkm:
> > +    type: boolean
> > +    description:
> > +      Use the supported Hardware Key Manager (HWKM) in Qualcomm ICE
> > +      to support wrapped keys. Having this entry helps scenarios where
> > +      the ICE hardware supports HWKM, but the Trustzone firmware does
> > +      not have the full capability to use this HWKM and support wrapped
> > +      keys. Not having this entry enabled would make ICE function in
> > +      non-HWKM mode supporting standard keys.
> 
> No changelog, previous comments and discussion ignored.
> 
> NAK

Apologies for not addressing the previous comments.
https://lore.kernel.org/all/9892c541ba4e4b5d975faaa4b49c92ba@quicinc.com/

Maybe we can continue our discussion here;
" SM8450 and SM8350 QCOM ICE both support HWKM in their ICE hardware.
However, wrapped keys can not be enabled on those targets due to certain
missing trustzone support. If we solely rely on hardware version to decide
if ICE has to use wrapped keys for data encryption, then it becomes untestable
on those chipsets. 

So, we want another way to distinguish this scenario, and hence I chose a DT vendor property
to explicitly mention if we have to use the supported HWKM.
If there is another way, I am open to exploring that as well."

> 
> Best regards,
> Krzysztof

Regards, 
Gaurav
Krzysztof Kozlowski June 18, 2024, 6:30 a.m. UTC | #4 
On 18/06/2024 02:35, Gaurav Kashyap (QUIC) wrote:
> Hello Krzysztof
> 
> On   06/17/2024 12:17 AM PDT, Krzysztof Kozlowski wrote:
>> On 17/06/2024 02:51, Gaurav Kashyap wrote:
>>> +  qcom,ice-use-hwkm:
>>> +    type: boolean
>>> +    description:
>>> +      Use the supported Hardware Key Manager (HWKM) in Qualcomm ICE
>>> +      to support wrapped keys. Having this entry helps scenarios where
>>> +      the ICE hardware supports HWKM, but the Trustzone firmware does
>>> +      not have the full capability to use this HWKM and support wrapped
>>> +      keys. Not having this entry enabled would make ICE function in
>>> +      non-HWKM mode supporting standard keys.
>>
>> No changelog, previous comments and discussion ignored.
>>
>> NAK
> 
> Apologies for not addressing the previous comments.
> https://lore.kernel.org/all/9892c541ba4e4b5d975faaa4b49c92ba@quicinc.com/
> 
> Maybe we can continue our discussion here;
> " SM8450 and SM8350 QCOM ICE both support HWKM in their ICE hardware.
> However, wrapped keys can not be enabled on those targets due to certain
> missing trustzone support. If we solely rely on hardware version to decide
> if ICE has to use wrapped keys for data encryption, then it becomes untestable
> on those chipsets. 

That does not make any sense to me. You enable it for SM8550 and SM8650
not SM8450 and SM8350.

> 
> So, we want another way to distinguish this scenario, and hence I chose a DT vendor property

What scenario? Show it in your patches.

> to explicitly mention if we have to use the supported HWKM.
> If there is another way, I am open to exploring that as well."

That property is just entirely redundant. If you claim otherwise, show
it through patches.

To be clear, so you will not resend the same ignoring comments: NAK.

Best regards,
Krzysztof
Gaurav Kashyap (QUIC) June 19, 2024, 10:07 p.m. UTC | #5 
On 06/17/2024 11:31 PM PDT, Krzysztof Kozlowski wrote:
> On 18/06/2024 02:35, Gaurav Kashyap (QUIC) wrote:
> > Hello Krzysztof
> >
> > On   06/17/2024 12:17 AM PDT, Krzysztof Kozlowski wrote:
> >> On 17/06/2024 02:51, Gaurav Kashyap wrote:
> >>> +  qcom,ice-use-hwkm:
> >>> +    type: boolean
> >>> +    description:
> >>> +      Use the supported Hardware Key Manager (HWKM) in Qualcomm
> ICE
> >>> +      to support wrapped keys. Having this entry helps scenarios where
> >>> +      the ICE hardware supports HWKM, but the Trustzone firmware does
> >>> +      not have the full capability to use this HWKM and support wrapped
> >>> +      keys. Not having this entry enabled would make ICE function in
> >>> +      non-HWKM mode supporting standard keys.
> >>
> >> No changelog, previous comments and discussion ignored.
> >>
> >> NAK
> >
> > Apologies for not addressing the previous comments.
> > https://lore.kernel.org/all/9892c541ba4e4b5d975faaa4b49c92ba@quicinc.c
> > om/
> >
> > Maybe we can continue our discussion here; " SM8450 and SM8350 QCOM
> > ICE both support HWKM in their ICE hardware.
> > However, wrapped keys can not be enabled on those targets due to
> > certain missing trustzone support. If we solely rely on hardware
> > version to decide if ICE has to use wrapped keys for data encryption,
> > then it becomes untestable on those chipsets.
> 
> That does not make any sense to me. You enable it for SM8550 and SM8650
> not SM8450 and SM8350.
> 
> >
> > So, we want another way to distinguish this scenario, and hence I
> > chose a DT vendor property
> 
> What scenario? Show it in your patches.
> 
> > to explicitly mention if we have to use the supported HWKM.
> > If there is another way, I am open to exploring that as well."
> 
> That property is just entirely redundant. If you claim otherwise, show it
> through patches.
> 
> To be clear, so you will not resend the same ignoring comments: NAK.
> 

Ack, next set of patches will have the property removed.

> Best regards,
> Krzysztof

Regards,
Gaurav
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
index 0304f074cf08..0bb4d008f961 100644
--- a/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
+++ b/Documentation/devicetree/bindings/crypto/qcom,inline-crypto-engine.yaml
@@ -27,6 +27,16 @@  properties:
   clocks:
     maxItems: 1
 
+  qcom,ice-use-hwkm:
+    type: boolean
+    description:
+      Use the supported Hardware Key Manager (HWKM) in Qualcomm ICE
+      to support wrapped keys. Having this entry helps scenarios where
+      the ICE hardware supports HWKM, but the Trustzone firmware does
+      not have the full capability to use this HWKM and support wrapped
+      keys. Not having this entry enabled would make ICE function in
+      non-HWKM mode supporting standard keys.
+
 required:
   - compatible
   - reg