Re: [Qemu-devel] Question about add AF_ALG backend for virtio-crypto

["Longpeng (Mike)" <longpeng2@huawei.com>]

Hi Daniel,

On 2017/2/8 18:53, Daniel P. Berrange wrote:

> On Wed, Feb 08, 2017 at 06:46:04PM +0800, Longpeng (Mike) wrote:
>> Hi Daniel,
>>
>> I was writing AF_ALG-backed for QEMU crypto these days, I think there're more
>> than two ways to implements it.
>>
>> The first one look likes below:
>> [ cipher.c ]
>> qcrypto_cipher_new(...)
>> {
>> 	if (...) { /* use AF_ALG */
>> 		cipher = afalg_cipher_new(...)
>> 		if (cipher) {
>> 			return cipher;
>> 		}
>> 	}
>> 	
>> 	/* disabled AF_ALG or AF_ALG failed, then back to
>> 	 * using 'builtin'(gcrypt/nettle/...)
>> 	 */
>> 	cipher = __qcrypto_cipher_new(...)
>> }
>>
>> [ cipher-afalg.c ]
>> afalg_cipher_new(...) {....}
>> afalg_cipher_encrypt(...) {...}
>> ......
>>
>> [ cipher-gcrypt.c ]
>> __qcrypto_cipher_new(...) {...}
>> __qcrypto_cipher_encrypt(...) {...}
>> ......
>>
>> [ cipher-nettle.c ]
>> __qcrypto_cipher_new(...) {...}
>> __qcrypto_cipher_encrypt(...) {...}
>> ......
>>
>> In this way, I think I need to rename most functions in
>> cipher-gcrypt.c/cipher-nettle.c with a prefixion(such as '__')
>>


>> I'm confusing about which way you'd prefer, or do you have any better
>> suggestion?
> 
> Yeah, both approaches have some reasonably significant downsides. Approach
> 1 is sort of like providing a virtual driver table, except it is hardcoded
> to switch between 2 impls only.
> 
> A variant on approach 1 is to actually setup a proper driver-table dispatch
> layer. eg define a struct that contains callbacks for each public api
> operation. The qcrypto_cipher_new() method will then either setup callbacks
> for AF_ALG, or for the library impl.
> 
> This is the design we took in crypto/{ivgen.c,ivgenpriv.h}
> 


So...you prefer approach 1 with a driver-table dispatch layer, right?
And this implies that we must either rename some public methods in
cipher-gcrypt.c/cipher-nettle.c, or change them to 'static'.


I also have some other ideas:

1) *using bitmap to improve performance*

As you suggested before:
"if we had AF_ALG in QEMU, we would have to have a stacked impl, where
we try AF_ALG and then fallback to the current code when QEMU runs on a
kernel lacking the feature needed."

I think it would impact the performance if we "try AF_ALG and then fallback to
library" each time, so we can use a bitmap to indicate whether the @alg is
supported by AF_ALG.


2) *maybe we need a heuristic policy*

I added some speed test in test-crypto-cipher/hash and found that for big
packets AF_ALG is much faster than library-impl while library-impl is better
when the packets is small:

packet(bytes)	AF_ALG(MB/sec, intel QAT)	Library-impl(MB/sec)
512		53.68				127.82
1024		98.39				133.21
2048		167.56				134.62
4096		276.21				135.10
8192		410.80				135.82
16384		545.08				136.01
32768		654.49				136.30
65536		723.00				136.29

If a @alg is both supported by AF_ALG and library-impl, I think we should decide
to use which one dynamically.

> 
> Regards,
> Daniel


-- 
Regards,
Longpeng(Mike)