[PATCH 1/2] crypto: caam: Change the i.MX8MQ check support all i.MX8M variants

[PATCH 1/2] crypto: caam: Change the i.MX8MQ check support all i.MX8M variants

[Adam Ford]

The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
the driver is restricting the check to just the i.MX8MQ.

This patch lets the driver support all i.MX8M Variants if enabled.

Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index db22777d59b4..1ce03f8961b6 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -527,7 +527,7 @@ static const struct soc_device_attribute caam_imx_soc_table[] = {
 	{ .soc_id = "i.MX6UL", .data = &caam_imx6ul_data },
 	{ .soc_id = "i.MX6*",  .data = &caam_imx6_data },
 	{ .soc_id = "i.MX7*",  .data = &caam_imx7_data },
-	{ .soc_id = "i.MX8MQ", .data = &caam_imx7_data },
+	{ .soc_id = "i.MX8M*", .data = &caam_imx7_data },
 	{ .family = "Freescale i.MX" },
 	{ /* sentinel */ }
 };
-- 
2.20.1

[PATCH 2/2] arm64: dts: imx8mm: Add Crypto CAAM support

[Adam Ford]

The i.MX8M Mini supports the same crypto engine as what is in
the i.MX8MQ, but it is not currently present in the device tree,
because it may be resricted by security features.

This patch places in into the device tree and marks it as disabled,
but anyone not restricting the CAAM with secure mode functions
can mark it as enabled.

Signed-off-by: Adam Ford <aford173@gmail.com>

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index 2ed1a3537f05..68c7c1adeb60 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -723,6 +723,37 @@
 				status = "disabled";
 			};
 
+			crypto: crypto@30900000 {
+				compatible = "fsl,sec-v4.0";
+				#address-cells = <1>;
+				#size-cells = <1>;
+				reg = <0x30900000 0x40000>;
+				ranges = <0 0x30900000 0x40000>;
+				interrupts = <GIC_SPI 91 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clk IMX8MM_CLK_AHB>,
+					 <&clk IMX8MM_CLK_IPG_ROOT>;
+				clock-names = "aclk", "ipg";
+				status = "disabled";
+
+				sec_jr0: jr@1000 {
+					compatible = "fsl,sec-v4.0-job-ring";
+					reg = <0x1000 0x1000>;
+					interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL_HIGH>;
+				};
+
+				sec_jr1: jr@2000 {
+					compatible = "fsl,sec-v4.0-job-ring";
+					reg = <0x2000 0x1000>;
+					interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>;
+				};
+
+				sec_jr2: jr@3000 {
+					compatible = "fsl,sec-v4.0-job-ring";
+					reg = <0x3000 0x1000>;
+					interrupts = <GIC_SPI 114 IRQ_TYPE_LEVEL_HIGH>;
+				};
+			};
+
 			i2c1: i2c@30a20000 {
 				compatible = "fsl,imx8mm-i2c", "fsl,imx21-i2c";
 				#address-cells = <1>;
-- 
2.20.1

Re: [PATCH 1/2] crypto: caam: Change the i.MX8MQ check support all i.MX8M variants

[Schrempf Frieder]

Hi Adam,

On 30.11.19 23:51, Adam Ford wrote:
> The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
> the driver is restricting the check to just the i.MX8MQ.
> 
> This patch lets the driver support all i.MX8M Variants if enabled.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>

What about the following lines in run_descriptor_deco0()? Does this 
condition also apply to i.MX8MM?

drivers/crypto/caam/ctrl.c:

	if (ctrlpriv->virt_en == 1 ||
	    /*
	     * Apparently on i.MX8MQ it doesn't matter if virt_en == 1
	     * and the following steps should be performed regardless
	     */
	    of_machine_is_compatible("fsl,imx8mq")) {
		clrsetbits_32(&ctrl->deco_rsr, 0, DECORSR_JR0);

		while (!(rd_reg32(&ctrl->deco_rsr) & DECORSR_VALID) &&
		       --timeout)
			cpu_relax();

		timeout = 100000;
	}

Regards,
Frieder

> 
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index db22777d59b4..1ce03f8961b6 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -527,7 +527,7 @@ static const struct soc_device_attribute caam_imx_soc_table[] = {
>   	{ .soc_id = "i.MX6UL", .data = &caam_imx6ul_data },
>   	{ .soc_id = "i.MX6*",  .data = &caam_imx6_data },
>   	{ .soc_id = "i.MX7*",  .data = &caam_imx7_data },
> -	{ .soc_id = "i.MX8MQ", .data = &caam_imx7_data },
> +	{ .soc_id = "i.MX8M*", .data = &caam_imx7_data },
>   	{ .family = "Freescale i.MX" },
>   	{ /* sentinel */ }
>   };
> 

Re: [PATCH 1/2] crypto: caam: Change the i.MX8MQ check support all i.MX8M variants

[Adam Ford]

On Wed, Dec 4, 2019 at 5:38 AM Schrempf Frieder
<frieder.schrempf@kontron.de> wrote:
>
> Hi Adam,
>
> On 30.11.19 23:51, Adam Ford wrote:
> > The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
> > the driver is restricting the check to just the i.MX8MQ.
> >
> > This patch lets the driver support all i.MX8M Variants if enabled.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
>
> What about the following lines in run_descriptor_deco0()? Does this
> condition also apply to i.MX8MM?

I think that's a question for NXP.  I am not seeing that in the NXP
Linux Release, and I don't have an 8MQ to compare.

I was able to get the driver working on the i.MXMM with the patch.

NXP  Team,

Do you have any opinions on this?

adam
>
> drivers/crypto/caam/ctrl.c:
>
>         if (ctrlpriv->virt_en == 1 ||
>             /*
>              * Apparently on i.MX8MQ it doesn't matter if virt_en == 1
>              * and the following steps should be performed regardless
>              */
>             of_machine_is_compatible("fsl,imx8mq")) {
>                 clrsetbits_32(&ctrl->deco_rsr, 0, DECORSR_JR0);
>
>                 while (!(rd_reg32(&ctrl->deco_rsr) & DECORSR_VALID) &&
>                        --timeout)
>                         cpu_relax();
>
>                 timeout = 100000;
>         }
>
> Regards,
> Frieder
>
> >
> > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> > index db22777d59b4..1ce03f8961b6 100644
> > --- a/drivers/crypto/caam/ctrl.c
> > +++ b/drivers/crypto/caam/ctrl.c
> > @@ -527,7 +527,7 @@ static const struct soc_device_attribute caam_imx_soc_table[] = {
> >       { .soc_id = "i.MX6UL", .data = &caam_imx6ul_data },
> >       { .soc_id = "i.MX6*",  .data = &caam_imx6_data },
> >       { .soc_id = "i.MX7*",  .data = &caam_imx7_data },
> > -     { .soc_id = "i.MX8MQ", .data = &caam_imx7_data },
> > +     { .soc_id = "i.MX8M*", .data = &caam_imx7_data },
> >       { .family = "Freescale i.MX" },
> >       { /* sentinel */ }
> >   };
> >

Re: [PATCH 2/2] arm64: dts: imx8mm: Add Crypto CAAM support

[Horia Geanta]

On 12/1/2019 12:52 AM, Adam Ford wrote:
> The i.MX8M Mini supports the same crypto engine as what is in
> the i.MX8MQ, but it is not currently present in the device tree,
> because it may be resricted by security features.
> 
What exactly are you referring to?

> This patch places in into the device tree and marks it as disabled,
> but anyone not restricting the CAAM with secure mode functions
> can mark it as enabled.
> 
Even if - due to export control regulations - CAAM is "trimmed down",
it loses only the encryption capabilities (hashing etc. still working).

Again, please clarify what you mean by "secure mode functions",
"security features" etc.

Horia

Re: [PATCH 2/2] arm64: dts: imx8mm: Add Crypto CAAM support

[Adam Ford]

On Mon, Dec 9, 2019 at 10:23 AM Horia Geanta <horia.geanta@nxp.com> wrote:
>
> On 12/1/2019 12:52 AM, Adam Ford wrote:
> > The i.MX8M Mini supports the same crypto engine as what is in
> > the i.MX8MQ, but it is not currently present in the device tree,
> > because it may be resricted by security features.
> >
> What exactly are you referring to?

I don't know this hardware very well, but on a different platform, we
needed to make the crypto engines as disabled if they were being
accessed through secure operations which made it unavailable to Linux
without using some special barriers. I didn't have the special
hardware on the other platform that required it that way, so I can't
really explain it well.  I know on those special cases, because some
people were accessing these registers through other means, the devices
had to be marked as 'disabled' so to avoid breaking something.  Since
I wasn't sure if this was left out of the i.MX8M Mini on purpose, I
let this disabled just in case this hardware platform was also
affected in a similar and people wanting to use it could mark it as
'okay'

adam

>
> > This patch places in into the device tree and marks it as disabled,
> > but anyone not restricting the CAAM with secure mode functions
> > can mark it as enabled.
> >
> Even if - due to export control regulations - CAAM is "trimmed down",
> it loses only the encryption capabilities (hashing etc. still working).
>
> Again, please clarify what you mean by "secure mode functions",
> "security features" etc.
>
> Horia

Re: [PATCH 1/2] crypto: caam: Change the i.MX8MQ check support all i.MX8M variants

[Horia Geanta]

On 12/6/2019 9:55 PM, Adam Ford wrote:
> On Wed, Dec 4, 2019 at 5:38 AM Schrempf Frieder
> <frieder.schrempf@kontron.de> wrote:
>>
>> Hi Adam,
>>
>> On 30.11.19 23:51, Adam Ford wrote:
>>> The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
>>> the driver is restricting the check to just the i.MX8MQ.
>>>
>>> This patch lets the driver support all i.MX8M Variants if enabled.
>>>
>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>
>> What about the following lines in run_descriptor_deco0()? Does this
>> condition also apply to i.MX8MM?
> 
> I think that's a question for NXP.  I am not seeing that in the NXP
> Linux Release, and I don't have an 8MQ to compare.
> 
IIRC the i.MX BSP releases use the JRI for initializing the RNG,
and not the DECO register interface.

> I was able to get the driver working on the i.MXMM with the patch.
> 
You are probably using a newer U-boot, which includes
commit dfaec76029f2 ("crypto/fsl: instantiate all rng state handles")

> NXP  Team,
> 
> Do you have any opinions on this?
> 
Since current U-boot initializes both RNG state handles, practically
instantiate_rng() is a no-op.

A simple experiment is to "lie" about the state_handle_mask, to exercise
the DECO acquire code (or, as mentioned above, to run with an older U-boot):

@@ -268,12 +272,19 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
        struct caam_ctrl __iomem *ctrl;
        u32 *desc, status = 0, rdsta_val;
        int ret = 0, sh_idx;
+       static int force_init = 1;

        ctrl = (struct caam_ctrl __iomem *)ctrlpriv->ctrl;
        desc = kmalloc(CAAM_CMD_SZ * 7, GFP_KERNEL);
        if (!desc)
                return -ENOMEM;

+       if (force_init && (state_handle_mask == 0x3)) {
+               dev_err(ctrldev, "Forcing reinit of RNG state handle 0!\n");
+               force_init = 0;
+               state_handle_mask = 0x2;
+       }
+
        for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
                /*
                 * If the corresponding bit is set, this state handle

In this case boot log confirms the DECO cannot be acquired:
[    2.137101] caam 30900000.crypto: Forcing reinit of RNG state handle 0!
[    2.172293] caam 30900000.crypto: failed to acquire DECO 0
[    2.177786] caam 30900000.crypto: failed to instantiate RNG

To sum up, writing to DECORSR is mandatory.

Horia

Re: [PATCH 1/2] crypto: caam: Change the i.MX8MQ check support all i.MX8M variants

[Schrempf Frieder]

On 10.12.19 08:56, Horia Geanta wrote:
> On 12/6/2019 9:55 PM, Adam Ford wrote:
>> On Wed, Dec 4, 2019 at 5:38 AM Schrempf Frieder
>> <frieder.schrempf@kontron.de> wrote:
>>>
>>> Hi Adam,
>>>
>>> On 30.11.19 23:51, Adam Ford wrote:
>>>> The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
>>>> the driver is restricting the check to just the i.MX8MQ.
>>>>
>>>> This patch lets the driver support all i.MX8M Variants if enabled.
>>>>
>>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>>
>>> What about the following lines in run_descriptor_deco0()? Does this
>>> condition also apply to i.MX8MM?
>>
>> I think that's a question for NXP.  I am not seeing that in the NXP
>> Linux Release, and I don't have an 8MQ to compare.
>>
> IIRC the i.MX BSP releases use the JRI for initializing the RNG,
> and not the DECO register interface.
> 
>> I was able to get the driver working on the i.MXMM with the patch.
>>
> You are probably using a newer U-boot, which includes
> commit dfaec76029f2 ("crypto/fsl: instantiate all rng state handles")
> 
>> NXP  Team,
>>
>> Do you have any opinions on this?
>>
> Since current U-boot initializes both RNG state handles, practically
> instantiate_rng() is a no-op.
> 
> A simple experiment is to "lie" about the state_handle_mask, to exercise
> the DECO acquire code (or, as mentioned above, to run with an older U-boot):
> 
> @@ -268,12 +272,19 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
>          struct caam_ctrl __iomem *ctrl;
>          u32 *desc, status = 0, rdsta_val;
>          int ret = 0, sh_idx;
> +       static int force_init = 1;
> 
>          ctrl = (struct caam_ctrl __iomem *)ctrlpriv->ctrl;
>          desc = kmalloc(CAAM_CMD_SZ * 7, GFP_KERNEL);
>          if (!desc)
>                  return -ENOMEM;
> 
> +       if (force_init && (state_handle_mask == 0x3)) {
> +               dev_err(ctrldev, "Forcing reinit of RNG state handle 0!\n");
> +               force_init = 0;
> +               state_handle_mask = 0x2;
> +       }
> +
>          for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
>                  /*
>                   * If the corresponding bit is set, this state handle
> 
> In this case boot log confirms the DECO cannot be acquired:
> [    2.137101] caam 30900000.crypto: Forcing reinit of RNG state handle 0!
> [    2.172293] caam 30900000.crypto: failed to acquire DECO 0
> [    2.177786] caam 30900000.crypto: failed to instantiate RNG
> 
> To sum up, writing to DECORSR is mandatory.

Thanks Horia for providing the details.

Adam, can you update your patch to enable the code in 
run_descriptor_deco0() for i.MX8MM?

If I understand this correctly, this is necessary to have the RNG 
initialize correctly no matter what version of U-Boot is used.

Thanks,
Frieder

Re: [PATCH 1/2] crypto: caam: Change the i.MX8MQ check support all i.MX8M variants

[Adam Ford]

On Wed, Dec 11, 2019 at 8:23 AM Schrempf Frieder
<frieder.schrempf@kontron.de> wrote:
>
> On 10.12.19 08:56, Horia Geanta wrote:
> > On 12/6/2019 9:55 PM, Adam Ford wrote:
> >> On Wed, Dec 4, 2019 at 5:38 AM Schrempf Frieder
> >> <frieder.schrempf@kontron.de> wrote:
> >>>
> >>> Hi Adam,
> >>>
> >>> On 30.11.19 23:51, Adam Ford wrote:
> >>>> The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
> >>>> the driver is restricting the check to just the i.MX8MQ.
> >>>>
> >>>> This patch lets the driver support all i.MX8M Variants if enabled.
> >>>>
> >>>> Signed-off-by: Adam Ford <aford173@gmail.com>
> >>>
> >>> What about the following lines in run_descriptor_deco0()? Does this
> >>> condition also apply to i.MX8MM?
> >>
> >> I think that's a question for NXP.  I am not seeing that in the NXP
> >> Linux Release, and I don't have an 8MQ to compare.
> >>
> > IIRC the i.MX BSP releases use the JRI for initializing the RNG,
> > and not the DECO register interface.
> >
> >> I was able to get the driver working on the i.MXMM with the patch.
> >>
> > You are probably using a newer U-boot, which includes
> > commit dfaec76029f2 ("crypto/fsl: instantiate all rng state handles")
> >
> >> NXP  Team,
> >>
> >> Do you have any opinions on this?
> >>
> > Since current U-boot initializes both RNG state handles, practically
> > instantiate_rng() is a no-op.
> >
> > A simple experiment is to "lie" about the state_handle_mask, to exercise
> > the DECO acquire code (or, as mentioned above, to run with an older U-boot):
> >
> > @@ -268,12 +272,19 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
> >          struct caam_ctrl __iomem *ctrl;
> >          u32 *desc, status = 0, rdsta_val;
> >          int ret = 0, sh_idx;
> > +       static int force_init = 1;
> >
> >          ctrl = (struct caam_ctrl __iomem *)ctrlpriv->ctrl;
> >          desc = kmalloc(CAAM_CMD_SZ * 7, GFP_KERNEL);
> >          if (!desc)
> >                  return -ENOMEM;
> >
> > +       if (force_init && (state_handle_mask == 0x3)) {
> > +               dev_err(ctrldev, "Forcing reinit of RNG state handle 0!\n");
> > +               force_init = 0;
> > +               state_handle_mask = 0x2;
> > +       }
> > +
> >          for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
> >                  /*
> >                   * If the corresponding bit is set, this state handle
> >
> > In this case boot log confirms the DECO cannot be acquired:
> > [    2.137101] caam 30900000.crypto: Forcing reinit of RNG state handle 0!
> > [    2.172293] caam 30900000.crypto: failed to acquire DECO 0
> > [    2.177786] caam 30900000.crypto: failed to instantiate RNG
> >
> > To sum up, writing to DECORSR is mandatory.
>
> Thanks Horia for providing the details.

I appreciate it too.

>
> Adam, can you update your patch to enable the code in
> run_descriptor_deco0() for i.MX8MM?

I will work on that.  I have been trying to get the mainline U-Boot to
start correctly, because I wanted to see if/how this interacted. I'll
try to get a V2 pushed today.

>
> If I understand this correctly, this is necessary to have the RNG
> initialize correctly no matter what version of U-Boot is used.

That makes sense based on Horia's feedback.  With the holidays this
month, my spare time and weekends have been full.

adam
>
> Thanks,
> Frieder

Re: [PATCH 2/2] arm64: dts: imx8mm: Add Crypto CAAM support

[Schrempf Frieder]

Hi Adam,

On 09.12.19 17:47, Adam Ford wrote:
> On Mon, Dec 9, 2019 at 10:23 AM Horia Geanta <horia.geanta@nxp.com> wrote:
>>
>> On 12/1/2019 12:52 AM, Adam Ford wrote:
>>> The i.MX8M Mini supports the same crypto engine as what is in
>>> the i.MX8MQ, but it is not currently present in the device tree,
>>> because it may be resricted by security features.
>>>
>> What exactly are you referring to?
> 
> I don't know this hardware very well, but on a different platform, we
> needed to make the crypto engines as disabled if they were being
> accessed through secure operations which made it unavailable to Linux
> without using some special barriers. I didn't have the special
> hardware on the other platform that required it that way, so I can't
> really explain it well.  I know on those special cases, because some
> people were accessing these registers through other means, the devices
> had to be marked as 'disabled' so to avoid breaking something.  Since
> I wasn't sure if this was left out of the i.MX8M Mini on purpose, I
> let this disabled just in case this hardware platform was also
> affected in a similar and people wanting to use it could mark it as
> 'okay'

I don't know enough about this to understand the problem you're 
describing. It seems like most SoCs have the CAAM enabled by default in 
the devicetree. On first glance I could only find fsl-lx2160a.dtsi that 
has it disabled.

> 
> adam
> 
>>
>>> This patch places in into the device tree and marks it as disabled,
>>> but anyone not restricting the CAAM with secure mode functions
>>> can mark it as enabled.
>>>
>> Even if - due to export control regulations - CAAM is "trimmed down",
>> it loses only the encryption capabilities (hashing etc. still working).

I don't know much about this, but as Horia said the CAAM might have 
limited capabilities in some cases but would still work.

Therefore I think the CAAM should be enabled by default as it already is 
done for most other SoCs.

Regards,
Frieder

>>
>> Again, please clarify what you mean by "secure mode functions",
>> "security features" etc.
>>
>> Horia
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>