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

[Farhan Ali]

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