Technically, the EIP97 is supported by this driver. The EIP-197 hasn't been tested.

You also need ofw_bus_if.h here as it's included from ofw_bus.h and ofw_bus_subr.h

When is this used? I don't see it in the Linux driver or bindings documentation.

I would perhaps invert this condition so it's 'If this specific case, use EBADMSG, else use EIO for generic errors'.

Also, what happens if you have multiple errors for a single crp? Right now you use the last one. That is probably ok since it probably never happens anyway?

This seems somewhat curious as you haven't added any children devices so bus_generic_attach(dev) should be a no-op? I would normally only use this in bus drivers and just return (0) in a leaf driver.

Likewise. (Plus, normally you want to detach children devices first so they can veto the attach early)

This is true of existing OCF consumers and it's ok to rely on that for now (your existing validation of csp_ivlen in your probesession hook will protect you if this changes). I may eventually add a new ioctl to /dev/crypto that permits setting the IV length for a given session. The only use case currently is to permit running all of the NIST KAT vectors for CCM and GCM. Some of the test vectors have IV lengths and tag lengths other than 12 and 16, respectively.

This doesn't appear to handle the recently added CSP_F_SEPARATE_AAD. For that, you wound want to append the equivalent of sglist_append(sg, crp->crp_aad, crp->crp_aad_length) when crp->crp_aad != NULL. IPsec will eventually use this for AES-GCM when using ESN.

To support CSP_F_SEPARATE_OUTPUT I think you would just need to adjust this to use segs from the output buffer and start at crp_payload_output_start when CRYPTO_HAS_OUTPUT_BUFFER() is true.

This can use ahash = crypto_auth_hash(csp) if you pass the csp to this routine.

You could perhaps use hmac_init_ipad and hmac_init_opad here. To do that, you would change setkey_hmac_digest to only copy the bits out of the ctx. If you combine that with passing in the csp from the caller this function becomes:

{
    union authctx ctx;

    ahash = crypto_auth_hash(csp);
    hmac_init_ipad(ahash, key, klen, &ctx);
    safexcel_setkey_hmac_digest(ahash, &ctx, ses->hmac_ipad);
    hmac_init_opad(ahash, key, klen, &ctx);
    safexcel_setkey_hmac_digest(ahash, &ctx, ses->hmac_opad);
    explicit_bzero(&ctx, sizeof(ctx));
}

FWIW, I prefer to use bus_write_4((_sc)->sc_res, _off, _val) for new drivers and avoid the use of bus_space tags and handles directly when possible.

I changed it so that we set EBADMSG only if AUTH_FAILED is set, which I should have been doing anyway. That error flag is set if digest verification fails, so it is the only flag we expect to see. Other error flags can be set as a result of programming errors in the driver (I triggered that during development), but I don't expect to see them otherwise.

Indeed, we can just return 0 here and below.

I was surprised that OCF isn't more flexible here. It shouldn't be a problem to generalize this code once the framework supports multiple IV lengths.

Hmm, how is this supposed to work with bus_dmamap_load_crp()? Don't I need a function that will load crp->crp_obuf?

Thanks, that is much nicer. It is a bit unfortunate though that we compute the same hash twice that way.

We get this one already via sys/rman.h.

It seems odd to me, but we don't get machine/bus.h via sys/bus.h.

Is the OCF klen in XTS context actually correct, or is half?

AES key scheduling and encryption in software is generally pretty slow. Is it slower to chain this operation through the device?

/ sizeof(uint64_t)?

Don't we guarantee the session is pre-zeroed? Or is that some other object. I might misremember.

It does eliminate concurrency if the same session is used from many threads. I think GELI does this. Perhaps it shouldn't.

Note that sess->klen is half of the klen given by OCF (see the XTS case in safexcel_setkey()). So here, sess->klen is the block cipher key length.

In principle it is possible to offload this operation to the device, but typically this is going to be done as part of session initialization, so the cost will be amortized over all operations using the session. For IPSec the cost should be negligible.

Thanks.

You are right, crypto_newsession() allocates a zeroed session softc structure.

Each GELI worker thread gets a unique session, from my reading of the code.

Should this be a KASSERT rather than a plain panic()?

If you're going to keep this delay can you leave a comment as to why it's 100?

Also if you want to keep it can you #define it to something so it's not a magic number?

Please give that magic number a #define'd name.

I tend to favor parens for this type of equation, even when not strictly necessary.

0x2 is...? Name it please.

More magic numbers.

Please name this even if it is 0.

Well, prior to the refactor the IV length was hardcoded. The refactor added csp_ivlen which makes it possible to support this. However, existing in-tree uses of GCM and CCM (IPsec, TLS) all use an IV length of 12. As I said, I need to extend /dev/crypto's interface to support setting the value of csp_ivlen. The current ioctl doesn't include an IV length so it always uses 12.

There is now a bus_dmamap_load_crp_buf(). bus_dmamap_load_crp(crp, ...) is now just bus_dmamap_load_crp_buf(&crp->crp_buf, ...). When you have an output buffer you would use bus_dmamap_load_crp_buf() with crp->crp_obuf, but you would need to deal with the fact that you have two sglists.

While this is true and ccr(4) takes the same approach, it does occur to me that it would be nice to have a little wrapper around AESNI or the like that one could use for quick AES bits. It could fallback to the existing C software on platforms without some kind of MD software AES. Probably not very urgent though.

Similar to the ghash case above, this isn't typically a hot path. I could provide a hmac_init_pads() perhaps that would take in two contexts and to avoid some of the duplication, but you really only duplicate work in the case of a key longer than the digest size (which OCF didn't even support consistently prior to the refactor, so isn't common anyway). The Init method of these transforms is pretty simple, and you always have to do the hash on the xored key twice.

It does, but it's also true that right now there is nothing in OCF that implicitly says sessions are single-threaded or can only have a single request in flight. For co-processors you probably want to support multiple sessions in flight (and in theory IPsec should be able to trigger that since multiple flows can share a single SA). Most drivers permit multiple in-flight operations for a single session (ccp(4) is the one driver I know that isn't safe to do this, and it is also missing the checks to defer requests to avoid doing the wrong thing in this scenario). I believe most drivers also permit concurrent operations though they may use a lock to single thread them.

Ok, I can update the driver and test once you're able to update OCF and cryptotest to handle alternate IV lengths.

I see, thanks. This still seems pretty awkward. To support both separate AAD and separate output buffers I need to use up to three dmamaps per request. As far as I can see, to load both crp->crp_buf and crp->crp_obuf I need two back-to-back calls to bus_dmamap_load_crp_buffer(), and I don't think they can share a dmamap_t. Similarly I need a third dmamap_load for separate AAD. I have it implemented and working, but it feels more complicated than it should be.

Right, the only reason I hadn't used hmac_init_ipad/opad() was that I didn't know about them.

There is nothing here preventing having multiple in-flight operations on a single session, the mutex simply serializes setup of the request. The session just seemed like a natural serialization point, which is why I went with this scheme. I doubt it will matter much in practice since the driver is going to be used on SOCs with low core counts anyway, but I could revisit this if IPSec ends up triggering a lot of ring lock contention. The only other option I can see is to have a global or per-session round-robin iterator which gets incremented for each request.

Ok, thanks for confirming that I'm not missing something. I will commit the required support as a follow-up change, since I'm working on porting this driver to stable/12 and having support for separate buffers will just complicate things a bit at this point.