Re: [Qemu-devel] [RFC v1 1/1] virtio-crypto: Allow disabling of cipher algorithms for virtio-crypto device

[Farhan Ali <alifm@linux.ibm.com>]

Hi Daniel

On 06/13/2018 05:37 AM, Daniel P. Berrangé wrote:
> On Tue, Jun 12, 2018 at 03:48:34PM -0400, Farhan Ali wrote:
>> The virtio-crypto driver currently propagates to the guest
>> all the cipher algorithms that the backend cryptodev can
>> support. But in certain cases where the guest has more
>> performant mechanism to handle some algorithms, it would be
>> useful to propagate only a subset of the algorithms.
> 
> I'm not really convinced by this.
> 
> The performance of crypto algorithms has many influencing
> factors, making it pretty hard to decide which is best
> without actively testing specific impls and comparing
> them in a manner which matches the application usage
> pattern. eg in theory the kernel crypto impl of an alg
> is faster than a userspace impl, if the kernel uses
> hardware accel and userspace does not. This, however,
> ignores the overhead of the kernel/userspace switch.
> The real world performance winner, thus depends on the
> amount of data being processed in each operation. Some
> times userspace can win & sometimes kernel space can
> win. This is even more relevant to virtio-crypto as
> it has more expensive context switches.

True. But what if the guest can perform some crypto algorithms without a 
incurring a VM exit? For example in s390 we have the cpacf instructions 
to perform crypto and this instruction is implemented for us by our 
hardware virtualization technology. In such a case it would be better 
not to use virtio-crypto's implementation of such a crypto algorithm.

At the same time we would like to take advantage of virtio-crypto's 
acceleration capabilities for certain crypto algorithms for which there 
is no hardware assistance.


> 
> IOW, when we expose a virtio-crypto dev to a guest,
> it is never reasonable for the guest to blindly assume
> that anything it does is faster than a pure software
> impl running in the guest. It will depend on the usage
> pattern. This is no different to bare metal where you
> should not assume kernel crypto is faster.
> 
> IMHO this is not a compelling reason to be able to turn
> off algorithms in virtio-crypto, as any decision will
> always be at best incomplete & inaccurate.

But shouldn't the user have the option to try and test by turning off 
certain algorithms? You are right the performance will depend on usage 
patterns, but I believe at least the user should have the option to test 
and see what works and does not work. It would be far easier to do so 
with the virtio-crypto dev than changing code in the kernel or userspace 
IMHO.


> 
>> @@ -853,6 +863,34 @@ static const VMStateDescription vmstate_virtio_crypto = {
>>   static Property virtio_crypto_properties[] = {
>>       DEFINE_PROP_LINK("cryptodev", VirtIOCrypto, conf.cryptodev,
>>                        TYPE_CRYPTODEV_BACKEND, CryptoDevBackend *),
>> +    DEFINE_PROP_BIT("no-cipher", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_ARC4, false),
>> +    DEFINE_PROP_BIT("cipher-arc4", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_ARC4, false),
>> +    DEFINE_PROP_BIT("cipher-aes-ecb", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_AES_ECB, false),
>> +    DEFINE_PROP_BIT("cipher-aes-cbc", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_AES_CBC, false),
>> +    DEFINE_PROP_BIT("cipher-aes-ctr", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_AES_CTR, false),
>> +    DEFINE_PROP_BIT("cipher-des-ecb", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_DES_ECB, false),
>> +    DEFINE_PROP_BIT("cipher-3des-ecb", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_3DES_ECB, false),
>> +    DEFINE_PROP_BIT("cipher-3des-cbc", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_3DES_CBC, false),
>> +    DEFINE_PROP_BIT("cipher-3des-ctr", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_3DES_CTR, false),
>> +    DEFINE_PROP_BIT("cipher-kasumi-f8", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_KASUMI_F8, false),
>> +    DEFINE_PROP_BIT("cipher-snow3g-uea2", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2, false),
>> +    DEFINE_PROP_BIT("cipher-aes-f8", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_AES_F8, false),
>> +    DEFINE_PROP_BIT("cipher-aes-xts", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_AES_XTS, false),
>> +    DEFINE_PROP_BIT("cipher-zuc-eea3", VirtIOCrypto, user_cipher_algo_l,
>> +                    VIRTIO_CRYPTO_CIPHER_ZUC_EEA3, false),
> 
> This does not scale as an approach IMHO which just reinforces to me
> that we shouldn't do this.

I am open suggestions on better implementation. I thought defining a 
property bit for the virtio-crypto dev would work, similar to cpu model 
approach.

Thanks for taking a look at the patch :)

Thanks
Farhan

> 
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>   
>> @@ -974,6 +1012,8 @@ static void virtio_crypto_instance_init(Object *obj)
>>        * Can be overriden with virtio_crypto_set_config_size.
>>        */
>>       vcrypto->config_size = sizeof(struct virtio_crypto_config);
>> +    vcrypto->user_cipher_algo_l = ~VIRTIO_CRYPTO_NO_CIPHER - 1;
>> +    vcrypto->user_cipher_algo_h = ~VIRTIO_CRYPTO_NO_CIPHER;
>>   }
>>   
>>   static const TypeInfo virtio_crypto_info = {
>> diff --git a/include/hw/virtio/virtio-crypto.h b/include/hw/virtio/virtio-crypto.h
>> index ca3a049..c5bb684 100644
>> --- a/include/hw/virtio/virtio-crypto.h
>> +++ b/include/hw/virtio/virtio-crypto.h
>> @@ -97,6 +97,9 @@ typedef struct VirtIOCrypto {
>>       uint32_t curr_queues;
>>       size_t config_size;
>>       uint8_t vhost_started;
>> +
>> +    uint32_t user_cipher_algo_l;
>> +    uint32_t user_cipher_algo_h;
>>   } VirtIOCrypto;
>>   
>>   #endif /* _QEMU_VIRTIO_CRYPTO_H */
>> -- 
>> 2.7.4
>>
>>
> 
> Regards,
> Daniel
>