* RE: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
2019-10-16 11:49 [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware Ben Dooks (Codethink)
@ 2019-10-17 17:14 ` Pascal Van Leeuwen
2019-10-17 19:46 ` Pascal Van Leeuwen
2019-10-21 10:23 ` Pascal Van Leeuwen
2 siblings, 0 replies; 4+ messages in thread
From: Pascal Van Leeuwen @ 2019-10-17 17:14 UTC (permalink / raw)
To: Ben Dooks (Codethink), linux-kernel
Cc: Antoine Tenart, Herbert Xu, David S. Miller, linux-crypto, linux-kernel
> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Ben
> Dooks (Codethink)
> Sent: Wednesday, October 16, 2019 1:50 PM
> To: linux-kernel@lists.codethink.co.uk
> Cc: Ben Dooks (Codethink) <ben.dooks@codethink.co.uk>; Antoine Tenart
> <antoine.tenart@bootlin.com>; Herbert Xu <herbert@gondor.apana.org.au>; David S. Miller
> <davem@davemloft.net>; linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
>
> In eip197_write_firmware() the firmware buffer is sent using
> writel(be32_to_cpu(),,,) this produces a number of warnings.
>
> Note, should this really be cpu_to_be32() ?
>
No, it should certainly not be cpu_to_be32() since the HW itself is most
definitely little endian, so that would not make sense to me.
Actually, I don't think either solution would be correct on a big-endian
CPU. But I don't have any big-endian CPU available to test that theory.
What I believe must happen is that the bytes must *always* be swapped
here, regardless of the endianness of the CPU. And with a little-endian
CPU, be32_to_cpu() coincidentally always does that.
Basically, what we need here is: read a dword (32 bits) from the memory
subsystem and write it back to the memory subsystem with bytes reversed.
Does the kernel have any dedicated function for just always swapping?
Anyway: NACK on this patch for now due to this.
> drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/crypto/inside-secure/safexcel.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c
> index 223d1bfdc7e6..dd33f6dda295 100644
> --- a/drivers/crypto/inside-secure/safexcel.c
> +++ b/drivers/crypto/inside-secure/safexcel.c
> @@ -298,13 +298,13 @@ static void eip197_init_firmware(struct safexcel_crypto_priv *priv)
> static int eip197_write_firmware(struct safexcel_crypto_priv *priv,
> const struct firmware *fw)
> {
> - const u32 *data = (const u32 *)fw->data;
> + const __be32 *data = (const __be32 *)fw->data;
> int i;
>
> /* Write the firmware */
> - for (i = 0; i < fw->size / sizeof(u32); i++)
> + for (i = 0; i < fw->size / sizeof(__be32); i++)
> writel(be32_to_cpu(data[i]),
> - priv->base + EIP197_CLASSIFICATION_RAMS + i * sizeof(u32));
> + priv->base + EIP197_CLASSIFICATION_RAMS + i * sizeof(__be32));
>
> /* Exclude final 2 NOPs from size */
> return i - EIP197_FW_TERMINAL_NOPS;
> --
> 2.23.0
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
^ permalink raw reply [flat|nested] 4+ messages in thread* RE: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
2019-10-16 11:49 [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware Ben Dooks (Codethink)
2019-10-17 17:14 ` Pascal Van Leeuwen
@ 2019-10-17 19:46 ` Pascal Van Leeuwen
2019-10-21 10:23 ` Pascal Van Leeuwen
2 siblings, 0 replies; 4+ messages in thread
From: Pascal Van Leeuwen @ 2019-10-17 19:46 UTC (permalink / raw)
To: Ben Dooks (Codethink), linux-kernel
Cc: Antoine Tenart, Herbert Xu, David S. Miller, linux-crypto, linux-kernel
> -----Original Message-----
> From: Pascal Van Leeuwen
> Sent: Thursday, October 17, 2019 7:14 PM
> To: 'Ben Dooks (Codethink)' <ben.dooks@codethink.co.uk>; linux-
> kernel@lists.codethink.co.uk
> Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Herbert Xu
> <herbert@gondor.apana.org.au>; David S. Miller <davem@davemloft.net>; linux-
> crypto@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
>
> > -----Original Message-----
> > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf
> Of Ben
> > Dooks (Codethink)
> > Sent: Wednesday, October 16, 2019 1:50 PM
> > To: linux-kernel@lists.codethink.co.uk
> > Cc: Ben Dooks (Codethink) <ben.dooks@codethink.co.uk>; Antoine Tenart
> > <antoine.tenart@bootlin.com>; Herbert Xu <herbert@gondor.apana.org.au>; David S. Miller
> > <davem@davemloft.net>; linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
> >
> > In eip197_write_firmware() the firmware buffer is sent using
> > writel(be32_to_cpu(),,,) this produces a number of warnings.
> >
> > Note, should this really be cpu_to_be32() ?
> >
> No, it should certainly not be cpu_to_be32() since the HW itself is most
> definitely little endian, so that would not make sense to me.
>
> Actually, I don't think either solution would be correct on a big-endian
> CPU. But I don't have any big-endian CPU available to test that theory.
>
> What I believe must happen is that the bytes must *always* be swapped
> here, regardless of the endianness of the CPU. And with a little-endian
> CPU, be32_to_cpu() coincidentally always does that.
>
> Basically, what we need here is: read a dword (32 bits) from the memory
> subsystem and write it back to the memory subsystem with bytes reversed.
>
> Does the kernel have any dedicated function for just always swapping?
>
After some more thought on the train home:
I think the correct construct would be cpu_to_le32(be32_to_cpu(data[i]))
This would correctly reflect that the data is read from big-endian
memory and subsequently written to little-endian "memory" (aka the EIP).
It also fits in nicely with your other changes. Could you work that into
a patch v2? Then I would ack it (after testing).
> Anyway: NACK on this patch for now due to this.
>
> > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> >
> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > ---
> > Cc: Antoine Tenart <antoine.tenart@bootlin.com>
> > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: linux-crypto@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > drivers/crypto/inside-secure/safexcel.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-
> secure/safexcel.c
> > index 223d1bfdc7e6..dd33f6dda295 100644
> > --- a/drivers/crypto/inside-secure/safexcel.c
> > +++ b/drivers/crypto/inside-secure/safexcel.c
> > @@ -298,13 +298,13 @@ static void eip197_init_firmware(struct safexcel_crypto_priv
> *priv)
> > static int eip197_write_firmware(struct safexcel_crypto_priv *priv,
> > const struct firmware *fw)
> > {
> > - const u32 *data = (const u32 *)fw->data;
> > + const __be32 *data = (const __be32 *)fw->data;
> > int i;
> >
> > /* Write the firmware */
> > - for (i = 0; i < fw->size / sizeof(u32); i++)
> > + for (i = 0; i < fw->size / sizeof(__be32); i++)
> > writel(be32_to_cpu(data[i]),
> > - priv->base + EIP197_CLASSIFICATION_RAMS + i * sizeof(u32));
> > + priv->base + EIP197_CLASSIFICATION_RAMS + i * sizeof(__be32));
> >
> > /* Exclude final 2 NOPs from size */
> > return i - EIP197_FW_TERMINAL_NOPS;
> > --
> > 2.23.0
>
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> www.insidesecure.com
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
^ permalink raw reply [flat|nested] 4+ messages in thread* RE: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
2019-10-16 11:49 [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware Ben Dooks (Codethink)
2019-10-17 17:14 ` Pascal Van Leeuwen
2019-10-17 19:46 ` Pascal Van Leeuwen
@ 2019-10-21 10:23 ` Pascal Van Leeuwen
2 siblings, 0 replies; 4+ messages in thread
From: Pascal Van Leeuwen @ 2019-10-21 10:23 UTC (permalink / raw)
To: Ben Dooks (Codethink), linux-kernel
Cc: Antoine Tenart, Herbert Xu, David S. Miller, linux-crypto, linux-kernel
> -----Original Message-----
> From: Pascal Van Leeuwen
> Sent: Thursday, October 17, 2019 9:46 PM
> To: 'Ben Dooks (Codethink)' <ben.dooks@codethink.co.uk>; 'linux-kernel@lists.codethink.co.uk'
> <linux-kernel@lists.codethink.co.uk>
> Cc: 'Antoine Tenart' <antoine.tenart@bootlin.com>; 'Herbert Xu' <herbert@gondor.apana.org.au>;
> 'David S. Miller' <davem@davemloft.net>; 'linux-crypto@vger.kernel.org' <linux-
> crypto@vger.kernel.org>; 'linux-kernel@vger.kernel.org' <linux-kernel@vger.kernel.org>
> Subject: RE: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
>
> > -----Original Message-----
> > From: Pascal Van Leeuwen
> > Sent: Thursday, October 17, 2019 7:14 PM
> > To: 'Ben Dooks (Codethink)' <ben.dooks@codethink.co.uk>; linux-
> > kernel@lists.codethink.co.uk
> > Cc: Antoine Tenart <antoine.tenart@bootlin.com>; Herbert Xu
> > <herbert@gondor.apana.org.au>; David S. Miller <davem@davemloft.net>; linux-
> > crypto@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: RE: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
> >
> > > -----Original Message-----
> > > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf
> > Of Ben
> > > Dooks (Codethink)
> > > Sent: Wednesday, October 16, 2019 1:50 PM
> > > To: linux-kernel@lists.codethink.co.uk
> > > Cc: Ben Dooks (Codethink) <ben.dooks@codethink.co.uk>; Antoine Tenart
> > > <antoine.tenart@bootlin.com>; Herbert Xu <herbert@gondor.apana.org.au>; David S. Miller
> > > <davem@davemloft.net>; linux-crypto@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: [PATCH] crypto: inside-secure - fix type of buffer in eip197_write_firmware
> > >
> > > In eip197_write_firmware() the firmware buffer is sent using
> > > writel(be32_to_cpu(),,,) this produces a number of warnings.
> > >
> > > Note, should this really be cpu_to_be32() ?
> > >
> > No, it should certainly not be cpu_to_be32() since the HW itself is most
> > definitely little endian, so that would not make sense to me.
>
Never mind that. While looking into more endianness related sparse issues,
I realised that the HW register access is actually configured to match the
CPU endianness. So the CPU will not need to swap bytes when accessing the
hardware registers.
(Technically, the hardware _is_ little endian but our slave interface
contains byte swapping logic that is enabled for big-endian CPU's ...)
> >
> > Actually, I don't think either solution would be correct on a big-endian
> > CPU. But I don't have any big-endian CPU available to test that theory.
> >
> > What I believe must happen is that the bytes must *always* be swapped
> > here, regardless of the endianness of the CPU. And with a little-endian
> > CPU, be32_to_cpu() coincidentally always does that.
> >
> > Basically, what we need here is: read a dword (32 bits) from the memory
> > subsystem and write it back to the memory subsystem with bytes reversed.
> >
> > Does the kernel have any dedicated function for just always swapping?
> >
>
> After some more thought on the train home:
>
> I think the correct construct would be cpu_to_le32(be32_to_cpu(data[i]))
> This would correctly reflect that the data is read from big-endian
> memory and subsequently written to little-endian "memory" (aka the EIP).
> It also fits in nicely with your other changes. Could you work that into
> a patch v2? Then I would ack it (after testing).
>
Since the HW slave ifc is configured to match the CPU endianness, the
cpu_to_le32() I suggested should NOT be performed and the code is fine as
it was.
> > Anyway: NACK on this patch for now due to this.
> >
Apologies for the mistake and inconvenience. Correction:
Acked-by: Pascal van Leeuwen <pvanleeuwen@verimatrix.com>
> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > > drivers/crypto/inside-secure/safexcel.c:306:17: warning: cast to restricted __be32
> > >
> > > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > > ---
> > > Cc: Antoine Tenart <antoine.tenart@bootlin.com>
> > > Cc: Herbert Xu <herbert@gondor.apana.org.au>
> > > Cc: "David S. Miller" <davem@davemloft.net>
> > > Cc: linux-crypto@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > ---
> > > drivers/crypto/inside-secure/safexcel.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-
> > secure/safexcel.c
> > > index 223d1bfdc7e6..dd33f6dda295 100644
> > > --- a/drivers/crypto/inside-secure/safexcel.c
> > > +++ b/drivers/crypto/inside-secure/safexcel.c
> > > @@ -298,13 +298,13 @@ static void eip197_init_firmware(struct safexcel_crypto_priv
> > *priv)
> > > static int eip197_write_firmware(struct safexcel_crypto_priv *priv,
> > > const struct firmware *fw)
> > > {
> > > - const u32 *data = (const u32 *)fw->data;
> > > + const __be32 *data = (const __be32 *)fw->data;
> > > int i;
> > >
> > > /* Write the firmware */
> > > - for (i = 0; i < fw->size / sizeof(u32); i++)
> > > + for (i = 0; i < fw->size / sizeof(__be32); i++)
> > > writel(be32_to_cpu(data[i]),
> > > - priv->base + EIP197_CLASSIFICATION_RAMS + i * sizeof(u32));
> > > + priv->base + EIP197_CLASSIFICATION_RAMS + i * sizeof(__be32));
> > >
> > > /* Exclude final 2 NOPs from size */
> > > return i - EIP197_FW_TERMINAL_NOPS;
> > > --
> > > 2.23.0
> >
> > Regards,
> > Pascal van Leeuwen
> > Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> > www.insidesecure.com
>
> Regards,
> Pascal van Leeuwen
> Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
> www.insidesecure.com
Regards,
Pascal van Leeuwen
Silicon IP Architect, Multi-Protocol Engines @ Verimatrix
www.insidesecure.com
^ permalink raw reply [flat|nested] 4+ messages in thread