mbox series

[v3,0/6] Add support for MCRC64 engine to calculate 64-bit CRC in Full-CPU mode

Message ID 20240524-mcrc64-upstream-v3-0-24b94d8e8578@ti.com
Headers show
Series Add support for MCRC64 engine to calculate 64-bit CRC in Full-CPU mode | expand

Message

Kamlesh Gurudasani May 30, 2024, 12:24 p.m. UTC
From: Kamlesh Gurudasani <kamlesh@ti.com>

MCRC64 engine calculates 64-bit cyclic redundancy checks (CRC)
according to the ISO 3309 standard.

The ISO 3309 64-bit CRC model parameters are as follows:
    Generator Polynomial: x^64 + x^4 + x^3 + x + 1
    Polynomial Value: 0x000000000000001B
    Initial value: 0x0000000000000000
    Reflected Input: False
    Reflected Output: False
    Xor Final: 0x0000000000000000

ISO 3309 which came in 1991 define this 64-bit CRC, ISO 3309 which
came in 1993 doesn't define 64-bit CRC algorithm at all, so ISO-3309
naming is unique enough for this CRC64 generator polynomial.

Tested with
CONFIG_CRYPTO_MANAGER_DISABLE_TESTS is not set
CONFIG_CRYPTO_MANAGER_EXTRA_TESTS=y

and tcrypt,
sudo modprobe tcrypt mode=329 sec=1

User space application implemented using algif_hash [0].

I understand that this is a old algorithm but,
most of TI's K3 based J7* and AM62* SOCs have MCRC64
which we want to use from User Space.

As per suggestions, we did the calculations and it turns out to be we
are saving good number of cpu cycles with HW offload[1].

Also, there are some automotive customers who have a safety
requirement to offload any parameters that are in Linux to ensure FFI.

[0] https://gist.github.com/ti-kamlesh/73abfcc1a33318bb3b199d36b6209e59
[1] https://patchwork.kernel.org/comment/25492483/

Signed-off-by: Kamlesh Gurudasani <kamlesh@ti.com>
---
Changes in v3:
- Changed the name for algorithm from crc64-iso to crc64-iso3309
- Removed unused code from Patch 2/6.
- Updated dt-bindings according to review comments Patch 3/6
- Added option to select SW implementation of algorithm Patch
  4/6
- Added depends ORed with COMPILE_TEST Patch 4/6
- Patch 6/6, added specific devices that will benefit out of this
- Marking patch 5,6/6 as DONOTMERGE as they will go in via subsystem
  Maintainer tree

Changes in v2:
- Add generic implementation of crc64-iso
- Fixes according to review comments
- Link to v1: https://lore.kernel.org/r/20230719-mcrc-upstream-v1-0-dc8798a24c47@ti.com

---
Kamlesh Gurudasani (6):
      lib: add ISO 3309 model crc64
      crypto: crc64 - add crc64-iso3309 framework
      dt-bindings: crypto: Add Texas Instruments MCRC64
      crypto: ti - add driver for MCRC64 engine
      arm64: dts: ti: k3-am62: Add dt node, cbass_main ranges for MCRC64
      arm64: defconfig: enable TI MCRC64 module

 Documentation/devicetree/bindings/crypto/ti,mcrc64.yaml |  48 +++++++
 MAINTAINERS                                             |   7 ++
 arch/arm64/boot/dts/ti/k3-am62-main.dtsi                |   7 ++
 arch/arm64/boot/dts/ti/k3-am62.dtsi                     |   1 +
 arch/arm64/configs/defconfig                            |   2 +
 crypto/Kconfig                                          |  11 ++
 crypto/Makefile                                         |   1 +
 crypto/crc64_iso3309_generic.c                          | 119 ++++++++++++++++++
 crypto/tcrypt.c                                         |   6 +
 crypto/testmgr.c                                        |   7 ++
 crypto/testmgr.h                                        | 404 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/crypto/Kconfig                                  |   1 +
 drivers/crypto/Makefile                                 |   1 +
 drivers/crypto/ti/Kconfig                               |  11 ++
 drivers/crypto/ti/Makefile                              |   2 +
 drivers/crypto/ti/mcrc64.c                              | 442 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/crc64.h                                   |   2 +
 lib/crc64.c                                             |  27 ++++
 lib/gen_crc64table.c                                    |   6 +
 19 files changed, 1105 insertions(+)
---
base-commit: 13909a0c88972c5ef5d13f44d1a8bf065a31bdf4
change-id: 20240524-mcrc64-upstream-e0ce4aad64ad

Best regards,

Comments

Kamlesh Gurudasani June 10, 2024, 2:33 p.m. UTC | #1
<kamlesh@ti.com> writes:

> From: Kamlesh Gurudasani <kamlesh@ti.com>
>
> MCRC64 engine calculates 64-bit cyclic redundancy checks (CRC)
> according to the ISO 3309 standard.
>

Hi Herbert,

Could you please review this and let me know if any changes are needed
to get it merged.

Thanks,
Kamlesh
Herbert Xu June 11, 2024, 2:31 a.m. UTC | #2
On Mon, Jun 10, 2024 at 08:03:44PM +0530, Kamlesh Gurudasani wrote:
> <kamlesh@ti.com> writes:
> 
> > From: Kamlesh Gurudasani <kamlesh@ti.com>
> >
> > MCRC64 engine calculates 64-bit cyclic redundancy checks (CRC)
> > according to the ISO 3309 standard.
> 
> Could you please review this and let me know if any changes are needed
> to get it merged.

Eric Biggers had concerns about adding this to the kernel.  I'd
like know if he's OK with this or not.

Thanks,
Eric Biggers June 11, 2024, 3:13 a.m. UTC | #3
On Tue, Jun 11, 2024 at 10:31:45AM +0800, Herbert Xu wrote:
> On Mon, Jun 10, 2024 at 08:03:44PM +0530, Kamlesh Gurudasani wrote:
> > <kamlesh@ti.com> writes:
> > 
> > > From: Kamlesh Gurudasani <kamlesh@ti.com>
> > >
> > > MCRC64 engine calculates 64-bit cyclic redundancy checks (CRC)
> > > according to the ISO 3309 standard.
> > 
> > Could you please review this and let me know if any changes are needed
> > to get it merged.
> 
> Eric Biggers had concerns about adding this to the kernel.  I'd
> like know if he's OK with this or not.
> 

I thought the rule is that there needs to be an in-kernel user to add algorithms
to the crypto API?  Is there any precedent for adding new algorithms purely so
that accelerators that implement them can be accessed from userspace via AF_ALG?

Even if acceptable, the motivation for this one does seem weak, given that a
userspace software implementation would actually be faster.  It could be
marginally useful for freeing up the CPU for other tasks if the inputs being
processed are very large (probably at least several megabytes), though.

- Eric
Herbert Xu June 11, 2024, 3:17 a.m. UTC | #4
On Mon, Jun 10, 2024 at 08:13:14PM -0700, Eric Biggers wrote:
>
> I thought the rule is that there needs to be an in-kernel user to add algorithms
> to the crypto API?  Is there any precedent for adding new algorithms purely so
> that accelerators that implement them can be accessed from userspace via AF_ALG?

I agree.  Perhaps this driver could instead be added as a simple
char device that is then used directly by the intended user without
going through the Crypto API at all.

That would make it a lot simpler.

Cheers,
Kamlesh Gurudasani Sept. 6, 2024, 11:14 a.m. UTC | #5
Herbert Xu <herbert@gondor.apana.org.au> writes:

> On Mon, Jun 10, 2024 at 08:13:14PM -0700, Eric Biggers wrote:
>>
>> I thought the rule is that there needs to be an in-kernel user to add algorithms
>> to the crypto API?  Is there any precedent for adding new algorithms purely so
>> that accelerators that implement them can be accessed from userspace via AF_ALG?
>
> I agree.  Perhaps this driver could instead be added as a simple
> char device that is then used directly by the intended user without
> going through the Crypto API at all.
>
> That would make it a lot simpler.
Thanks for all the support Herbert, Eric.

Just wanted to confirm, if this is being rejected primarily because
1. there is no in-kernel user for crc64-iso3309
2. or poor performance benefit of using it from userspace

The context for asking is that we have another superset IP known as MCRC
(this one is MCRC64), which supports crc8/16/32/64(iso-3309).

That IP has working DMA and will give good offloading numbers.

We are planning to send drivers for crc8/16/32 for MCRC
1.should I put efforts for crc64-iso3309 as well or
2.drop the crc64-iso3309 and send only for remaining
crc8/16/32(standard algorithms with already in-kernel user).

All our devices either have MCRC or MCRC64.

Thanks,
Kamlesh
Herbert Xu Sept. 13, 2024, 10:35 a.m. UTC | #6
On Fri, Sep 06, 2024 at 04:44:44PM +0530, Kamlesh Gurudasani wrote:
>
> Just wanted to confirm, if this is being rejected primarily because
> 1. there is no in-kernel user for crc64-iso3309
> 2. or poor performance benefit of using it from userspace

Essentially we don't want to add every random algorithm to the crypto
API because we may end up having to maintain them long after the users
have disappeared.

For a special-purpose algorithm like this, it's perfectly fine to have
a custom driver to be made so that your user-space app can access the
hardware.

> The context for asking is that we have another superset IP known as MCRC
> (this one is MCRC64), which supports crc8/16/32/64(iso-3309).
> 
> That IP has working DMA and will give good offloading numbers.
> 
> We are planning to send drivers for crc8/16/32 for MCRC
> 1.should I put efforts for crc64-iso3309 as well or
> 2.drop the crc64-iso3309 and send only for remaining
> crc8/16/32(standard algorithms with already in-kernel user).
> 
> All our devices either have MCRC or MCRC64.

Do any existing kernel users benefit sufficiently from these algorithms
being offloaded? If no then there is no need to bother.

Cheers,