On Fri, 2005-01-14 at 17:40 +0100, Michal Ludvig wrote: 
> > Is there any connection to Evgeniy Polyakov's acrypto work? It appears,
> > that there are two project for one objective. Would be nice to see both
> > parties pulling on one string.
> 
> These projects do not compete at all. Evgeniy's work is a complete 
> replacement for current cryptoapi and brings the asynchronous 
> operations at the first place. My patches are simple and straightforward 
> extensions to current cryptoapi that enable offloading the chaining to 
> hardware where possible.

Fine, I just saw in Evgeniy's reply, that he took your padlock
implementation. I thought both of you have been working on different
implementations. 

But actually both aim for the same goal. Hardware crypto-offloading.
With padlock the need for a async interface isn't that big, because it's
not "off-loading" as it's done on the same chip and in the same thread. 

However, developing two different APIs isn't particular efficient. I
know, at the moment there isn't much choice, as J.Morris hasn't commited
to acrypto in anyway. But I think it would be good to replace the
synchronized CryptoAPI implementation altogether, put the missing
internals of CryptoAPI into acrypto, and back the interfaces of
CryptoAPI with small stubs, that do like

somereturnvalue synchronized_interface(..) {
	acrypto_kick_some_operation(acrypto_struct);
	wait_for_completion(acrypto_struct);
	return fetch_result(acrypto_struct);
}

The other way round, a asynchron interface using a synchronized
interface doesn't seem natural to me.
(That doesn't mean I oppose your patches, merely that we should start to
think in different directions)

> > > +	void (*cia_ecb)(void *ctx, u8 *dst, const u8 *src, u8 *iv,
> > > +			size_t nbytes, int encdec, int inplace);
> > > +	void (*cia_cbc)(void *ctx, u8 *dst, const u8 *src, u8 *iv,
> > > +			size_t nbytes, int encdec, int inplace);
> > > +	void (*cia_cfb)(void *ctx, u8 *dst, const u8 *src, u8 *iv,
> > > +			size_t nbytes, int encdec, int inplace);
> > > +	void (*cia_ofb)(void *ctx, u8 *dst, const u8 *src, u8 *iv,
> > > +			size_t nbytes, int encdec, int inplace);
> > > +	void (*cia_ctr)(void *ctx, u8 *dst, const u8 *src, u8 *iv,
> > > +			size_t nbytes, int encdec, int inplace);
> > 
> > What's the use of adding mode specific functions to the tfm struct? And
> > why do they all have the same function type? For instance, the "iv" or
> > "inplace" argument is meaningless for ECB.
> 
> The prototypes must be the same in my implementation, because in crypt() 
> only a pointer to the appropriate mode function is taken and further used 
> as "(func*)(arg, arg, ...)".
> 
> BTW these functions are not added to "struct crypto_tfm", but to "struct 
> crypto_alg" which describes what a particular module supports (i.e. along 
> with the block size, algorithm name, etc). In this case it can say that 
> e.g. padlock.ko supports encryption in CBC mode in addition to a common 
> single-block processing.

Err, right. I overlooked that it's cia and not cit. However, I don't
like the idea of extending structs when there is a new cipher mode. I
think the API should not have to be extended for every addition, but
should be designed for such extension right from the start.

What about a "selector" function, which returns the appropriate
encryption function for a mode?

typedef void (procfn_t)(struct crypto_tfm *, u8 *,
                        u8*, cryptfn_t, int enc, void *, int);

put 
	procfn_t (*cia_modesel)(u32 function, int iface, int encdec);
into struct crypto_alg;

then in crypto_init_cipher_ops, instead of

	switch (tfm->crt_cipher.cit_mode) {
..
	case CRYPTO_TFM_MODE_CFB:
        	ops->cit_encrypt = cfb_encrypt;
		ops->cit_decrypt = cfb_decrypt;
..
}
we do,
	struct cipher_alg *cia = &tfm->__crt_alg->cra_cipher;
	
	switch (tfm->crt_cipher.cit_mode) {
..
		case CRYPTO_TFM_MODE_CFB:
			ops->cit_encrypt = cia->cia_modesel(cit_mode, 0, IFACE_ECB);
			ops->cit_decrypt = cia->cia_modesel(cit_mode, 1, IFACE_ECB);
			ops->cit_encrypt_iv = cia->cia_modesel(cit_mode, 0, IFACE_IV);
			ops->cit_decrypt_iv = cia->cia_modesel(cit_mode, 1, IFACE_IV);
..

Alternatively, we could also add a lookup table. But I like this better,
since this is much easier to read for people, and tfm's aren't alloced
that often.

Probably, we can add a wrapper for cia_modesel, that when cia_modesel is
NULL, it falls back to the old behaviour. This way, we don't have to
patch all algorithm implementations to include cia_modesel.

How you like that idea?

-- 
Fruhwirth Clemens <clemens@endorphin.org>  http://clemens.endorphin.org