[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Farhan Ali
Subject: Re: [Qemu-devel] [RFC v1 1/1] virtio-crypto: Allow disabling of cipher algorithms for virtio-crypto device
Date: Wed, 13 Jun 2018 11:01:05 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

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 :)


@@ -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 
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 */


reply via email to

[Prev in Thread] Current Thread [Next in Thread]