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

[Longpeng (Mike)]

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)