Re: [PATCH v25 2/4] crypto: spacc - Add SPAcc ahash support

[Pavitrakumar Managutte]

Hi Herbert,
   My comments are embedded below.

Also while bringing up an async ahash SPAcc driver against 7.3-rc1,
cmac(aes) and xcbc(aes) fail their self-tests at tfm allocation, and
I'd like to understand whether the underlying cause is intentional
before I work around it in the driver.

All tests pass for every hash and HMAC we register, except cmac(aes)
and xcbc(aes). For cmac(aes)/xcbc(aes) it fails.

crypto_ahash_init_tfm() allocates the fallback with:

    crypto_alloc_ahash(name, CRYPTO_ALG_REQ_VIRT,
                       CRYPTO_ALG_ASYNC | CRYPTO_ALG_REQ_VIRT |
                       CRYPTO_AHASH_ALG_NO_EXPORT_CORE);

On our tree no cmac(aes)/xcbc(aes) provider satisfies that: with both
CONFIG_CRYPTO_CMAC/XCBC=y and CONFIG_CRYPTO_LIB_AES_CBC_MACS=y,
neither the crypto/cmac.c template nor the new AES-CMAC library shash
implements export_core/import_core, so both report NO_EXPORT_CORE. The
allocation finds no candidate and returns -EINVAL.

alg: hash: failed to allocate transform for spacc-cmac(aes): -22

The library shash matches ASYNC=0 and REQ_VIRT but is rejected on
NO_EXPORT_CORE. No candidate remains. CONFIG_CRYPTO_SELFTESTS_FULL=y,
so this is hit deterministically.

This looks like the case flagged in "crypto: sha256 - Implement
export_core() and import_core()" (Eric, Sep 2025), which notes that
since
commit 9d7a0ab1c753 export_core/import_core "effectively became
mandatory... since legacy drivers that need a fallback depend on
them."
sha256/sha512/md5/hmac gained export_core, but cmac(aes)/xcbc(aes) did not.

Is the absence of export_core/import_core on cmac(aes)/xcbc(aes)
intentional -- i.e. async offload drivers must not rely on the core
software fallback for these MACs -- or is it an oversight that should
be addressed the way sha256 was?

Warm regards,
Pavitrakumar
Vayavya Labs Pvt. Ltd.

On Fri, Sep 18, 2026 at 2:10 PM Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Sep 07, 2026 at 09:29:14PM +0530, Pavitrakumar Managutte wrote:
> >
> > +static int spacc_hash_init(struct ahash_request *req)
> > +{
> > +     int rc;
> > +     struct crypto_ahash *tfm        = crypto_ahash_reqtfm(req);
> > +     struct spacc_crypto_ctx *tctx   = crypto_ahash_ctx(tfm);
> > +     struct spacc_crypto_reqctx *ctx = ahash_request_ctx(req);
> > +
> > +     memset(ctx->state_buffer, 0, sizeof(ctx->state_buffer));
> > +
> > +     if (tctx->shash_fb) {
> > +             SHASH_DESC_ON_STACK(desc, tctx->shash_fb);
> > +
> > +             desc->tfm = tctx->shash_fb;
> > +             rc = crypto_shash_init(desc);
> > +             if (!rc)
> > +                     rc = crypto_shash_export(desc, ctx->state_buffer);
> > +             shash_desc_zero(desc);
> > +     } else {
> > +             HASH_FBREQ_ON_STACK(fbreq, req);
> > +
> > +             rc = crypto_ahash_init(fbreq);
> > +             if (!rc)
> > +                     rc = crypto_ahash_export(fbreq, ctx->state_buffer);
> > +             HASH_REQUEST_ZERO(fbreq);
> > +     }
>
> Why do we need two fallback paths? It would appear that you can
> just use the else clause in all cases.
PK: Will fix that. We just need the ahash else path.

>
> > +static const struct ahash_engine_alg spacc_hash_template = {
> > +     .base = {
> > +             .init     = spacc_hash_init,
> > +             .update   = spacc_hash_update,
> > +             .final    = spacc_hash_final,
> > +             .finup    = spacc_hash_finup,
> > +             .digest   = spacc_hash_digest,
> > +             .setkey   = spacc_hash_setkey,
> > +             .export   = spacc_hash_export,
> > +             .import   = spacc_hash_import,
> > +             .init_tfm = spacc_hash_init_tfm,
> > +             .exit_tfm = spacc_hash_exit_tfm,
> > +
> > +             .halg.base = {
> > +                     .cra_priority   = 300,
> > +                     .cra_module     = THIS_MODULE,
> > +                     .cra_ctxsize    = sizeof(struct spacc_crypto_ctx),
> > +                     .cra_reqsize    = sizeof(struct spacc_crypto_reqctx),
> > +                     .cra_destroy    = spacc_hash_alg_destroy,
> > +                     .cra_flags      = CRYPTO_ALG_TYPE_AHASH    |
> > +                                       CRYPTO_ALG_ASYNC
>
> Please use BLOCK_ONLY for all new drivers.  See aspeed for an
> example on how it works.
PK: Is this needed for SPAcc? Let me give some context here.
The SPAcc engine computes MACs in a single shot only; it cannot export
or import intermediate MAC state. So, following the current model, the
driver registers async ahash algorithms and relies on the
core-provided software fallback for the incremental
(init/update/final) path, as per your suggestion. So is the BLOCK_ONLY
flag still needed?

>
> > +     /*
> > +      * CMAC/XCBC-AES have a dedicated shash fallback (allocated in
> > +      * init_tfm).  They must not advertise CRYPTO_ALG_OPTIONAL_KEY or
> > +      * the crypto API may try to use the generic ahash fallback for them,
> > +      * which would recurse back into this driver.
> > +      */
>
> How can it recurse back into the driver? The fallback allocated
> by the API is always synchronous so unless your driver is sync
> (which it is not) it cannot recurse.
PK: Agreed, will remove that.

>
> > +             salg->alg.hash.base.halg.base.cra_flags =
> > +                     CRYPTO_ALG_TYPE_AHASH | CRYPTO_ALG_ASYNC |
> > +                     CRYPTO_ALG_NEED_FALLBACK;
>
> There is no need to set NEED_FALLBACK as the API now allocates
> fallbacks for all async drivers.
PK: Agreed, I will fix that.

>
> Cheers,
> --
> Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt