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

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

[Farhan Ali]

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.

This patch adds support for disabling the cipher
algorithms of the backend cryptodev.

eg:
 -object cryptodev-backend-builtin,id=cryptodev0
 -device virtio-crypto-ccw,id=crypto0,cryptodev=cryptodev0,cipher-aes-cbc=off

Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
---

Please note this patch is not complete, and there are TODOs to handle
for other types of algorithms such Hash, AEAD and MAC algorithms.

This is mainly intended to get some feedback on the design approach
from the community.


 hw/virtio/virtio-crypto.c         | 46 ++++++++++++++++++++++++++++++++++++---
 include/hw/virtio/virtio-crypto.h |  3 +++
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 9a9fa49..4aed9ca 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -754,12 +754,22 @@ static void virtio_crypto_reset(VirtIODevice *vdev)
 static void virtio_crypto_init_config(VirtIODevice *vdev)
 {
     VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
+    uint32_t user_crypto_services = (1u << VIRTIO_CRYPTO_SERVICE_CIPHER) |
+                                    (1u << VIRTIO_CRYPTO_SERVICE_HASH) |
+                                    (1u << VIRTIO_CRYPTO_SERVICE_AEAD) |
+                                    (1u << VIRTIO_CRYPTO_SERVICE_MAC);
+
+    if (vcrypto->user_cipher_algo_l & (1u << VIRTIO_CRYPTO_NO_CIPHER)) {
+        vcrypto->user_cipher_algo_l = 1u << VIRTIO_CRYPTO_NO_CIPHER;
+        vcrypto->user_cipher_algo_h = 0;
+        user_crypto_services &= ~(1u << VIRTIO_CRYPTO_SERVICE_CIPHER);
+    }
 
-    vcrypto->conf.crypto_services =
+    vcrypto->conf.crypto_services = user_crypto_services &
                      vcrypto->conf.cryptodev->conf.crypto_services;
-    vcrypto->conf.cipher_algo_l =
+    vcrypto->conf.cipher_algo_l = vcrypto->user_cipher_algo_l &
                      vcrypto->conf.cryptodev->conf.cipher_algo_l;
-    vcrypto->conf.cipher_algo_h =
+    vcrypto->conf.cipher_algo_h = vcrypto->user_cipher_algo_h &
                      vcrypto->conf.cryptodev->conf.cipher_algo_h;
     vcrypto->conf.hash_algo = vcrypto->conf.cryptodev->conf.hash_algo;
     vcrypto->conf.mac_algo_l = vcrypto->conf.cryptodev->conf.mac_algo_l;
@@ -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),
     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

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

[Gonglei (Arei)]

> -----Original Message-----
> From: Farhan Ali [mailto:alifm@linux.ibm.com]
> Sent: Wednesday, June 13, 2018 3:49 AM
> To: qemu-devel@nongnu.org
> Cc: mst@redhat.com; Gonglei (Arei) <arei.gonglei@huawei.com>; longpeng
> <longpeng2@huawei.com>; pasic@linux.ibm.com; borntraeger@de.ibm.com;
> frankja@linux.ibm.com; alifm@linux.ibm.com
> Subject: [RFC v1 1/1] virtio-crypto: Allow disabling of cipher algorithms for
> virtio-crypto device
> 
> 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.
> 

It makes sense to me. E.g. current Intel CPU has the AES-NI instruction for accelerating
AES algo. We don't need to propagate AES algos.

> This patch adds support for disabling the cipher
> algorithms of the backend cryptodev.
> 
> eg:
>  -object cryptodev-backend-builtin,id=cryptodev0
>  -device virtio-crypto-ccw,id=crypto0,cryptodev=cryptodev0,cipher-aes-cbc=off
> 
> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> ---
> 
> Please note this patch is not complete, and there are TODOs to handle
> for other types of algorithms such Hash, AEAD and MAC algorithms.
> 
> This is mainly intended to get some feedback on the design approach
> from the community.
> 
> 
>  hw/virtio/virtio-crypto.c         | 46
> ++++++++++++++++++++++++++++++++++++---
>  include/hw/virtio/virtio-crypto.h |  3 +++
>  2 files changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
> index 9a9fa49..4aed9ca 100644
> --- a/hw/virtio/virtio-crypto.c
> +++ b/hw/virtio/virtio-crypto.c
> @@ -754,12 +754,22 @@ static void virtio_crypto_reset(VirtIODevice *vdev)
>  static void virtio_crypto_init_config(VirtIODevice *vdev)
>  {
>      VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
> +    uint32_t user_crypto_services = (1u <<
> VIRTIO_CRYPTO_SERVICE_CIPHER) |
> +                                    (1u <<
> VIRTIO_CRYPTO_SERVICE_HASH) |
> +                                    (1u <<
> VIRTIO_CRYPTO_SERVICE_AEAD) |
> +                                    (1u <<
> VIRTIO_CRYPTO_SERVICE_MAC);
> +
> +    if (vcrypto->user_cipher_algo_l & (1u << VIRTIO_CRYPTO_NO_CIPHER)) {
> +        vcrypto->user_cipher_algo_l = 1u << VIRTIO_CRYPTO_NO_CIPHER;
> +        vcrypto->user_cipher_algo_h = 0;
> +        user_crypto_services &= ~(1u <<
> VIRTIO_CRYPTO_SERVICE_CIPHER);
> +    }
> 
> -    vcrypto->conf.crypto_services =
> +    vcrypto->conf.crypto_services = user_crypto_services &
>                       vcrypto->conf.cryptodev->conf.crypto_services;
> -    vcrypto->conf.cipher_algo_l =
> +    vcrypto->conf.cipher_algo_l = vcrypto->user_cipher_algo_l &
>                       vcrypto->conf.cryptodev->conf.cipher_algo_l;
> -    vcrypto->conf.cipher_algo_h =
> +    vcrypto->conf.cipher_algo_h = vcrypto->user_cipher_algo_h &
>                       vcrypto->conf.cryptodev->conf.cipher_algo_h;
>      vcrypto->conf.hash_algo = vcrypto->conf.cryptodev->conf.hash_algo;
>      vcrypto->conf.mac_algo_l = vcrypto->conf.cryptodev->conf.mac_algo_l;
> @@ -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),

s/ VIRTIO_CRYPTO_CIPHER_ARC4/VIRTIO_CRYPTO_NO_CIPHER/

> +    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),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> 
We'd better keep all algorithms enabled by default. So pls s/false/true/g.

Thanks,
-Gonglei 

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

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

[Daniel P. Berrangé]

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.

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.

> @@ -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.

>      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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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
> 

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

[Daniel P. Berrangé]

On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
> 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.

IIUC, the kernel's crypto layer can support multiple implementations of
any algorithm. Providers can report a priority against implementations
which influences which impl is used in practice. So if there's a native
instruction for a partiuclar algorithm I would expect the impl registered
for that to be designated higher priority than other impls, so that it is
used in preference to other impls.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Halil Pasic]

On 06/13/2018 05:05 PM, Daniel P. Berrangé wrote:
> On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
>> 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.
> 
> IIUC, the kernel's crypto layer can support multiple implementations of
> any algorithm. Providers can report a priority against implementations
> which influences which impl is used in practice. So if there's a native
> instruction for a partiuclar algorithm I would expect the impl registered
> for that to be designated higher priority than other impls, so that it is
> used in preference to other impls.
> 

AFAIR the problem here is that in (the guest) kernel the virtio-crypto
driver has to register it's crypto algo implementations with a priority
(single number), which dictates if it's going to be the preferred (used)
implementation of the algorithm or not. The virtio-crypto driver does this
without having information about the (comparative or absolute) performance
of it's implementation (which depends on the backend among others). I don't think
any dynamic re-prioritization of the algorithms takes place (e.g. based on how these
perform in for the given configuration).

I think the strategy of the virtio-crypto is to rather overstate, than
understate the performance of it's implementation. If we were to 'be
conservative' and say, 'hey we don't know nothing about the performance,
let's make it lowest priority implementation' the implementations provided
by virtio-crypto would end up being used only if there is no other
implementation. And that does not sound like a good idea either.

So the idea is to give the user the power to effectively not provide
an algorithm via virtio-crypto. That is, if the user observes a performance
degradation because of virtio-crypto, he can turn off the bad algorithms
at the device. That way overstatement becomes a much smaller problem.
The user can turn off the bad algorithms for reasons other than performance
too.

Of course there are other ways to deal with the problem of virtio-crypto
driver not knowing how good it's implementation of a given algo is. We
could make the in kernel crypto priorities dynamically adjustable in general
or we could provide the user with means to specify the priorities (e.g.
as module parameter) with which the virtio-crypto driver registers each algo.
Both of these would be knobs in the guest. It's hard to tell if these first
one would be useful in scenarios not involving virtualization. Same goes
for some kind of dynamic priority management for crypto algorithm implementations
in the Linux kernel. I assume the people involved with the respective
subsystem do not see the necessity for something like that.

I hope, this clarifies the idea behind this patch.

Regards,
Halil

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

[Farhan Ali]

Hi Arei

On 06/12/2018 08:57 PM, Gonglei (Arei) wrote:
> 
>> -----Original Message-----
>> From: Farhan Ali [mailto:alifm@linux.ibm.com]
>> Sent: Wednesday, June 13, 2018 3:49 AM
>> To: qemu-devel@nongnu.org
>> Cc: mst@redhat.com; Gonglei (Arei) <arei.gonglei@huawei.com>; longpeng
>> <longpeng2@huawei.com>; pasic@linux.ibm.com; borntraeger@de.ibm.com;
>> frankja@linux.ibm.com; alifm@linux.ibm.com
>> Subject: [RFC v1 1/1] virtio-crypto: Allow disabling of cipher algorithms for
>> virtio-crypto device
>>
>> 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.
>>
> 
> It makes sense to me. E.g. current Intel CPU has the AES-NI instruction for accelerating
> AES algo. We don't need to propagate AES algos.
> 
>> This patch adds support for disabling the cipher
>> algorithms of the backend cryptodev.
>>
>> eg:
>>   -object cryptodev-backend-builtin,id=cryptodev0
>>   -device virtio-crypto-ccw,id=crypto0,cryptodev=cryptodev0,cipher-aes-cbc=off
>>
>> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
>> ---
>>
>> Please note this patch is not complete, and there are TODOs to handle
>> for other types of algorithms such Hash, AEAD and MAC algorithms.
>>
>> This is mainly intended to get some feedback on the design approach
>> from the community.
>>
>>
>>   hw/virtio/virtio-crypto.c         | 46
>> ++++++++++++++++++++++++++++++++++++---
>>   include/hw/virtio/virtio-crypto.h |  3 +++
>>   2 files changed, 46 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
>> index 9a9fa49..4aed9ca 100644
>> --- a/hw/virtio/virtio-crypto.c
>> +++ b/hw/virtio/virtio-crypto.c
>> @@ -754,12 +754,22 @@ static void virtio_crypto_reset(VirtIODevice *vdev)
>>   static void virtio_crypto_init_config(VirtIODevice *vdev)
>>   {
>>       VirtIOCrypto *vcrypto = VIRTIO_CRYPTO(vdev);
>> +    uint32_t user_crypto_services = (1u <<
>> VIRTIO_CRYPTO_SERVICE_CIPHER) |
>> +                                    (1u <<
>> VIRTIO_CRYPTO_SERVICE_HASH) |
>> +                                    (1u <<
>> VIRTIO_CRYPTO_SERVICE_AEAD) |
>> +                                    (1u <<
>> VIRTIO_CRYPTO_SERVICE_MAC);
>> +
>> +    if (vcrypto->user_cipher_algo_l & (1u << VIRTIO_CRYPTO_NO_CIPHER)) {
>> +        vcrypto->user_cipher_algo_l = 1u << VIRTIO_CRYPTO_NO_CIPHER;
>> +        vcrypto->user_cipher_algo_h = 0;
>> +        user_crypto_services &= ~(1u <<
>> VIRTIO_CRYPTO_SERVICE_CIPHER);
>> +    }
>>
>> -    vcrypto->conf.crypto_services =
>> +    vcrypto->conf.crypto_services = user_crypto_services &
>>                        vcrypto->conf.cryptodev->conf.crypto_services;
>> -    vcrypto->conf.cipher_algo_l =
>> +    vcrypto->conf.cipher_algo_l = vcrypto->user_cipher_algo_l &
>>                        vcrypto->conf.cryptodev->conf.cipher_algo_l;
>> -    vcrypto->conf.cipher_algo_h =
>> +    vcrypto->conf.cipher_algo_h = vcrypto->user_cipher_algo_h &
>>                        vcrypto->conf.cryptodev->conf.cipher_algo_h;
>>       vcrypto->conf.hash_algo = vcrypto->conf.cryptodev->conf.hash_algo;
>>       vcrypto->conf.mac_algo_l = vcrypto->conf.cryptodev->conf.mac_algo_l;
>> @@ -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),
> 
> s/ VIRTIO_CRYPTO_CIPHER_ARC4/VIRTIO_CRYPTO_NO_CIPHER/

Thanks for catching that. I will change.

> 
>> +    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),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
>>
> We'd better keep all algorithms enabled by default. So pls s/false/true/g.
> 

You are right. I will change it.

> Thanks,
> -Gonglei
> 

Thanks for taking a look.

Thanks
Farhan

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

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

[Daniel P. Berrangé]

On Wed, Jun 13, 2018 at 07:28:08PM +0200, Halil Pasic wrote:
> 
> 
> On 06/13/2018 05:05 PM, Daniel P. Berrangé wrote:
> > On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
> > > 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.
> > 
> > IIUC, the kernel's crypto layer can support multiple implementations of
> > any algorithm. Providers can report a priority against implementations
> > which influences which impl is used in practice. So if there's a native
> > instruction for a partiuclar algorithm I would expect the impl registered
> > for that to be designated higher priority than other impls, so that it is
> > used in preference to other impls.
> > 
> 
> AFAIR the problem here is that in (the guest) kernel the virtio-crypto
> driver has to register it's crypto algo implementations with a priority
> (single number), which dictates if it's going to be the preferred (used)
> implementation of the algorithm or not. The virtio-crypto driver does this
> without having information about the (comparative or absolute) performance
> of it's implementation (which depends on the backend among others). I don't think
> any dynamic re-prioritization of the algorithms takes place (e.g. based on how these
> perform in for the given configuration).
> 
> I think the strategy of the virtio-crypto is to rather overstate, than
> understate the performance of it's implementation. If we were to 'be
> conservative' and say, 'hey we don't know nothing about the performance,
> let's make it lowest priority implementation' the implementations provided
> by virtio-crypto would end up being used only if there is no other
> implementation. And that does not sound like a good idea either.


This problem you describe, however, is something that applies to *any*
kerenl code that is registering a crypto algo impl for accelerator
hardware. A non-virtualized crypto cards in bare metal likewise cannot
assume that its AES impl is better then the host CPU's  aes-ni instruction.

> So the idea is to give the user the power to effectively not provide
> an algorithm via virtio-crypto. That is, if the user observes a performance
> degradation because of virtio-crypto, he can turn off the bad algorithms
> at the device. That way overstatement becomes a much smaller problem.
> The user can turn off the bad algorithms for reasons other than performance
> too.
> 
> Of course there are other ways to deal with the problem of virtio-crypto
> driver not knowing how good it's implementation of a given algo is. We
> could make the in kernel crypto priorities dynamically adjustable in general
> or we could provide the user with means to specify the priorities (e.g.
> as module parameter) with which the virtio-crypto driver registers each algo.
> Both of these would be knobs in the guest. It's hard to tell if these first
> one would be useful in scenarios not involving virtualization. Same goes
> for some kind of dynamic priority management for crypto algorithm implementations
> in the Linux kernel. I assume the people involved with the respective
> subsystem do not see the necessity for something like that.

It still feels like this is a problem for the guest OS to solve. If you
put a physical crypto accelerator in a bare metal machine, that has the
same problem you describe here, so the kernel surely already needs to find
a viable solution for this problem. 


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Farhan Ali]

On 06/14/2018 04:21 AM, Daniel P. Berrangé wrote:
> On Wed, Jun 13, 2018 at 07:28:08PM +0200, Halil Pasic wrote:
>>
>>
>> On 06/13/2018 05:05 PM, Daniel P. Berrangé wrote:
>>> On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
>>>> 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.
>>>
>>> IIUC, the kernel's crypto layer can support multiple implementations of
>>> any algorithm. Providers can report a priority against implementations
>>> which influences which impl is used in practice. So if there's a native
>>> instruction for a partiuclar algorithm I would expect the impl registered
>>> for that to be designated higher priority than other impls, so that it is
>>> used in preference to other impls.
>>>
>>
>> AFAIR the problem here is that in (the guest) kernel the virtio-crypto
>> driver has to register it's crypto algo implementations with a priority
>> (single number), which dictates if it's going to be the preferred (used)
>> implementation of the algorithm or not. The virtio-crypto driver does this
>> without having information about the (comparative or absolute) performance
>> of it's implementation (which depends on the backend among others). I don't think
>> any dynamic re-prioritization of the algorithms takes place (e.g. based on how these
>> perform in for the given configuration).
>>
>> I think the strategy of the virtio-crypto is to rather overstate, than
>> understate the performance of it's implementation. If we were to 'be
>> conservative' and say, 'hey we don't know nothing about the performance,
>> let's make it lowest priority implementation' the implementations provided
>> by virtio-crypto would end up being used only if there is no other
>> implementation. And that does not sound like a good idea either.
> 
> 
> This problem you describe, however, is something that applies to *any*
> kerenl code that is registering a crypto algo impl for accelerator
> hardware. A non-virtualized crypto cards in bare metal likewise cannot
> assume that its AES impl is better then the host CPU's  aes-ni instruction.
> 
>> So the idea is to give the user the power to effectively not provide
>> an algorithm via virtio-crypto. That is, if the user observes a performance
>> degradation because of virtio-crypto, he can turn off the bad algorithms
>> at the device. That way overstatement becomes a much smaller problem.
>> The user can turn off the bad algorithms for reasons other than performance
>> too.
>>
>> Of course there are other ways to deal with the problem of virtio-crypto
>> driver not knowing how good it's implementation of a given algo is. We
>> could make the in kernel crypto priorities dynamically adjustable in general
>> or we could provide the user with means to specify the priorities (e.g.
>> as module parameter) with which the virtio-crypto driver registers each algo.
>> Both of these would be knobs in the guest. It's hard to tell if these first
>> one would be useful in scenarios not involving virtualization. Same goes
>> for some kind of dynamic priority management for crypto algorithm implementations
>> in the Linux kernel. I assume the people involved with the respective
>> subsystem do not see the necessity for something like that.
> 
> It still feels like this is a problem for the guest OS to solve. If you
> put a physical crypto accelerator in a bare metal machine, that has the
> same problem you describe here, so the kernel surely already needs to find
> a viable solution for this problem.
> 

How would the guest OS know which algo is better? As you mentioned it 
does depend on few factors and the best the kernel can do is use some 
sort of heuristics. Such a solution might not be very dynamic and might 
not work for all the cases for a user.

Shouldn't we use virtualization to give us the flexibility that we don't 
have with physical crypto accelerator? The crypto accelerator might not 
know if it's implementation is any better, but the user can experiment 
and see what works better.

Thanks
Farhan

> 
> Regards,
> Daniel
> 

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

[Daniel P. Berrangé]

On Thu, Jun 14, 2018 at 10:50:40AM -0400, Farhan Ali wrote:
> 
> 
> On 06/14/2018 04:21 AM, Daniel P. Berrangé wrote:
> > On Wed, Jun 13, 2018 at 07:28:08PM +0200, Halil Pasic wrote:
> > > 
> > > 
> > > On 06/13/2018 05:05 PM, Daniel P. Berrangé wrote:
> > > > On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
> > > > > 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.
> > > > 
> > > > IIUC, the kernel's crypto layer can support multiple implementations of
> > > > any algorithm. Providers can report a priority against implementations
> > > > which influences which impl is used in practice. So if there's a native
> > > > instruction for a partiuclar algorithm I would expect the impl registered
> > > > for that to be designated higher priority than other impls, so that it is
> > > > used in preference to other impls.
> > > > 
> > > 
> > > AFAIR the problem here is that in (the guest) kernel the virtio-crypto
> > > driver has to register it's crypto algo implementations with a priority
> > > (single number), which dictates if it's going to be the preferred (used)
> > > implementation of the algorithm or not. The virtio-crypto driver does this
> > > without having information about the (comparative or absolute) performance
> > > of it's implementation (which depends on the backend among others). I don't think
> > > any dynamic re-prioritization of the algorithms takes place (e.g. based on how these
> > > perform in for the given configuration).
> > > 
> > > I think the strategy of the virtio-crypto is to rather overstate, than
> > > understate the performance of it's implementation. If we were to 'be
> > > conservative' and say, 'hey we don't know nothing about the performance,
> > > let's make it lowest priority implementation' the implementations provided
> > > by virtio-crypto would end up being used only if there is no other
> > > implementation. And that does not sound like a good idea either.
> > 
> > 
> > This problem you describe, however, is something that applies to *any*
> > kerenl code that is registering a crypto algo impl for accelerator
> > hardware. A non-virtualized crypto cards in bare metal likewise cannot
> > assume that its AES impl is better then the host CPU's  aes-ni instruction.
> > 
> > > So the idea is to give the user the power to effectively not provide
> > > an algorithm via virtio-crypto. That is, if the user observes a performance
> > > degradation because of virtio-crypto, he can turn off the bad algorithms
> > > at the device. That way overstatement becomes a much smaller problem.
> > > The user can turn off the bad algorithms for reasons other than performance
> > > too.
> > > 
> > > Of course there are other ways to deal with the problem of virtio-crypto
> > > driver not knowing how good it's implementation of a given algo is. We
> > > could make the in kernel crypto priorities dynamically adjustable in general
> > > or we could provide the user with means to specify the priorities (e.g.
> > > as module parameter) with which the virtio-crypto driver registers each algo.
> > > Both of these would be knobs in the guest. It's hard to tell if these first
> > > one would be useful in scenarios not involving virtualization. Same goes
> > > for some kind of dynamic priority management for crypto algorithm implementations
> > > in the Linux kernel. I assume the people involved with the respective
> > > subsystem do not see the necessity for something like that.
> > 
> > It still feels like this is a problem for the guest OS to solve. If you
> > put a physical crypto accelerator in a bare metal machine, that has the
> > same problem you describe here, so the kernel surely already needs to find
> > a viable solution for this problem.
> > 
> 
> How would the guest OS know which algo is better? As you mentioned it does
> depend on few factors and the best the kernel can do is use some sort of
> heuristics. Such a solution might not be very dynamic and might not work for
> all the cases for a user.

Which is better will likely depend on the application using it. One might
be better for use by the kernel, while another is better for use by a
userspace application, or two userspace apps might have different preferences.

> Shouldn't we use virtualization to give us the flexibility that we don't
> have with physical crypto accelerator? The crypto accelerator might not know
> if it's implementation is any better, but the user can experiment and see
> what works better.

It is better to provide it all to the guest and let the guest decide which
is best to use.  If nothing else the virtio-crypto kernel module itself
can have module parameters to control the priority it gives to each
algorithm, or can avoid registering certain algorithms.  Doing it guest
side is more flexible, because realistically many virt host deployments
will never give the guest admin ability to control this from the host
side, so a guest kernel config ability will be the only thing available.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Farhan Ali]

On 06/14/2018 11:10 AM, Daniel P. Berrangé wrote:
> On Thu, Jun 14, 2018 at 10:50:40AM -0400, Farhan Ali wrote:
>>
>>
>> On 06/14/2018 04:21 AM, Daniel P. Berrangé wrote:
>>> On Wed, Jun 13, 2018 at 07:28:08PM +0200, Halil Pasic wrote:
>>>>
>>>>
>>>> On 06/13/2018 05:05 PM, Daniel P. Berrangé wrote:
>>>>> On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
>>>>>> 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.
>>>>>
>>>>> IIUC, the kernel's crypto layer can support multiple implementations of
>>>>> any algorithm. Providers can report a priority against implementations
>>>>> which influences which impl is used in practice. So if there's a native
>>>>> instruction for a partiuclar algorithm I would expect the impl registered
>>>>> for that to be designated higher priority than other impls, so that it is
>>>>> used in preference to other impls.
>>>>>
>>>>
>>>> AFAIR the problem here is that in (the guest) kernel the virtio-crypto
>>>> driver has to register it's crypto algo implementations with a priority
>>>> (single number), which dictates if it's going to be the preferred (used)
>>>> implementation of the algorithm or not. The virtio-crypto driver does this
>>>> without having information about the (comparative or absolute) performance
>>>> of it's implementation (which depends on the backend among others). I don't think
>>>> any dynamic re-prioritization of the algorithms takes place (e.g. based on how these
>>>> perform in for the given configuration).
>>>>
>>>> I think the strategy of the virtio-crypto is to rather overstate, than
>>>> understate the performance of it's implementation. If we were to 'be
>>>> conservative' and say, 'hey we don't know nothing about the performance,
>>>> let's make it lowest priority implementation' the implementations provided
>>>> by virtio-crypto would end up being used only if there is no other
>>>> implementation. And that does not sound like a good idea either.
>>>
>>>
>>> This problem you describe, however, is something that applies to *any*
>>> kerenl code that is registering a crypto algo impl for accelerator
>>> hardware. A non-virtualized crypto cards in bare metal likewise cannot
>>> assume that its AES impl is better then the host CPU's  aes-ni instruction.
>>>
>>>> So the idea is to give the user the power to effectively not provide
>>>> an algorithm via virtio-crypto. That is, if the user observes a performance
>>>> degradation because of virtio-crypto, he can turn off the bad algorithms
>>>> at the device. That way overstatement becomes a much smaller problem.
>>>> The user can turn off the bad algorithms for reasons other than performance
>>>> too.
>>>>
>>>> Of course there are other ways to deal with the problem of virtio-crypto
>>>> driver not knowing how good it's implementation of a given algo is. We
>>>> could make the in kernel crypto priorities dynamically adjustable in general
>>>> or we could provide the user with means to specify the priorities (e.g.
>>>> as module parameter) with which the virtio-crypto driver registers each algo.
>>>> Both of these would be knobs in the guest. It's hard to tell if these first
>>>> one would be useful in scenarios not involving virtualization. Same goes
>>>> for some kind of dynamic priority management for crypto algorithm implementations
>>>> in the Linux kernel. I assume the people involved with the respective
>>>> subsystem do not see the necessity for something like that.
>>>
>>> It still feels like this is a problem for the guest OS to solve. If you
>>> put a physical crypto accelerator in a bare metal machine, that has the
>>> same problem you describe here, so the kernel surely already needs to find
>>> a viable solution for this problem.
>>>
>>
>> How would the guest OS know which algo is better? As you mentioned it does
>> depend on few factors and the best the kernel can do is use some sort of
>> heuristics. Such a solution might not be very dynamic and might not work for
>> all the cases for a user.
> 
> Which is better will likely depend on the application using it. One might
> be better for use by the kernel, while another is better for use by a
> userspace application, or two userspace apps might have different preferences.
> 
>> Shouldn't we use virtualization to give us the flexibility that we don't
>> have with physical crypto accelerator? The crypto accelerator might not know
>> if it's implementation is any better, but the user can experiment and see
>> what works better.
> 
> It is better to provide it all to the guest and let the guest decide which
> is best to use.  If nothing else the virtio-crypto kernel module itself
> can have module parameters to control the priority it gives to each
> algorithm, or can avoid registering certain algorithms.  Doing it guest
> side is more flexible, because realistically many virt host deployments
> will never give the guest admin ability to control this from the host
> side, so a guest kernel config ability will be the only thing available.
> 

I am not sure if putting all that complexity on the guest OS is the 
right approach. I thought it would be better to let the user decide 
through device definition what algorithms should be available to the 
guest. But I am open to other approaches and suggestion :)

I would like to know if Arei or Longpeng(Mike) has any suggestion 
regarding this?

Thanks
Farhan

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

[Daniel P. Berrangé]

On Thu, Jun 14, 2018 at 12:12:23PM -0400, Farhan Ali wrote:
> 
> 
> On 06/14/2018 11:10 AM, Daniel P. Berrangé wrote:
> > On Thu, Jun 14, 2018 at 10:50:40AM -0400, Farhan Ali wrote:
> > > 
> > > 
> > > On 06/14/2018 04:21 AM, Daniel P. Berrangé wrote:
> > > > On Wed, Jun 13, 2018 at 07:28:08PM +0200, Halil Pasic wrote:
> > > > > 
> > > > > 
> > > > > On 06/13/2018 05:05 PM, Daniel P. Berrangé wrote:
> > > > > > On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
> > > > > > > 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.
> > > > > > 
> > > > > > IIUC, the kernel's crypto layer can support multiple implementations of
> > > > > > any algorithm. Providers can report a priority against implementations
> > > > > > which influences which impl is used in practice. So if there's a native
> > > > > > instruction for a partiuclar algorithm I would expect the impl registered
> > > > > > for that to be designated higher priority than other impls, so that it is
> > > > > > used in preference to other impls.
> > > > > > 
> > > > > 
> > > > > AFAIR the problem here is that in (the guest) kernel the virtio-crypto
> > > > > driver has to register it's crypto algo implementations with a priority
> > > > > (single number), which dictates if it's going to be the preferred (used)
> > > > > implementation of the algorithm or not. The virtio-crypto driver does this
> > > > > without having information about the (comparative or absolute) performance
> > > > > of it's implementation (which depends on the backend among others). I don't think
> > > > > any dynamic re-prioritization of the algorithms takes place (e.g. based on how these
> > > > > perform in for the given configuration).
> > > > > 
> > > > > I think the strategy of the virtio-crypto is to rather overstate, than
> > > > > understate the performance of it's implementation. If we were to 'be
> > > > > conservative' and say, 'hey we don't know nothing about the performance,
> > > > > let's make it lowest priority implementation' the implementations provided
> > > > > by virtio-crypto would end up being used only if there is no other
> > > > > implementation. And that does not sound like a good idea either.
> > > > 
> > > > 
> > > > This problem you describe, however, is something that applies to *any*
> > > > kerenl code that is registering a crypto algo impl for accelerator
> > > > hardware. A non-virtualized crypto cards in bare metal likewise cannot
> > > > assume that its AES impl is better then the host CPU's  aes-ni instruction.
> > > > 
> > > > > So the idea is to give the user the power to effectively not provide
> > > > > an algorithm via virtio-crypto. That is, if the user observes a performance
> > > > > degradation because of virtio-crypto, he can turn off the bad algorithms
> > > > > at the device. That way overstatement becomes a much smaller problem.
> > > > > The user can turn off the bad algorithms for reasons other than performance
> > > > > too.
> > > > > 
> > > > > Of course there are other ways to deal with the problem of virtio-crypto
> > > > > driver not knowing how good it's implementation of a given algo is. We
> > > > > could make the in kernel crypto priorities dynamically adjustable in general
> > > > > or we could provide the user with means to specify the priorities (e.g.
> > > > > as module parameter) with which the virtio-crypto driver registers each algo.
> > > > > Both of these would be knobs in the guest. It's hard to tell if these first
> > > > > one would be useful in scenarios not involving virtualization. Same goes
> > > > > for some kind of dynamic priority management for crypto algorithm implementations
> > > > > in the Linux kernel. I assume the people involved with the respective
> > > > > subsystem do not see the necessity for something like that.
> > > > 
> > > > It still feels like this is a problem for the guest OS to solve. If you
> > > > put a physical crypto accelerator in a bare metal machine, that has the
> > > > same problem you describe here, so the kernel surely already needs to find
> > > > a viable solution for this problem.
> > > > 
> > > 
> > > How would the guest OS know which algo is better? As you mentioned it does
> > > depend on few factors and the best the kernel can do is use some sort of
> > > heuristics. Such a solution might not be very dynamic and might not work for
> > > all the cases for a user.
> > 
> > Which is better will likely depend on the application using it. One might
> > be better for use by the kernel, while another is better for use by a
> > userspace application, or two userspace apps might have different preferences.
> > 
> > > Shouldn't we use virtualization to give us the flexibility that we don't
> > > have with physical crypto accelerator? The crypto accelerator might not know
> > > if it's implementation is any better, but the user can experiment and see
> > > what works better.
> > 
> > It is better to provide it all to the guest and let the guest decide which
> > is best to use.  If nothing else the virtio-crypto kernel module itself
> > can have module parameters to control the priority it gives to each
> > algorithm, or can avoid registering certain algorithms.  Doing it guest
> > side is more flexible, because realistically many virt host deployments
> > will never give the guest admin ability to control this from the host
> > side, so a guest kernel config ability will be the only thing available.
> > 
> 
> I am not sure if putting all that complexity on the guest OS is the right
> approach. I thought it would be better to let the user decide through device
> definition what algorithms should be available to the guest. But I am open
> to other approaches and suggestion :)

Well the guest admin always has 100% control over what happens inside their
guest OS. They often have no control at all over how QEMU is configured on
the host. eg apps like oVirt, OpenStack, etc may let them create a guest
with a virtio-crypto dev, but not provide any way to configure these kind
of fine details of it. So in many cases, configuring it in the guest is
going to be the only option you have.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Gonglei (Arei)]

> -----Original Message-----
> From: Daniel P. Berrangé [mailto:berrange@redhat.com]
> Sent: Thursday, June 14, 2018 11:11 PM
> To: Farhan Ali <alifm@linux.ibm.com>
> Cc: Halil Pasic <pasic@linux.ibm.com>; qemu-devel@nongnu.org;
> frankja@linux.ibm.com; mst@redhat.com; borntraeger@de.ibm.com; Gonglei
> (Arei) <arei.gonglei@huawei.com>; longpeng <longpeng2@huawei.com>;
> Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>;
> mjrosato@linux.vnet.ibm.com
> Subject: Re: [Qemu-devel] [RFC v1 1/1] virtio-crypto: Allow disabling of cipher
> algorithms for virtio-crypto device
> 
> On Thu, Jun 14, 2018 at 10:50:40AM -0400, Farhan Ali wrote:
> >
> >
> > On 06/14/2018 04:21 AM, Daniel P. Berrangé wrote:
> > > On Wed, Jun 13, 2018 at 07:28:08PM +0200, Halil Pasic wrote:
> > > >
> > > >
> > > > On 06/13/2018 05:05 PM, Daniel P. Berrangé wrote:
> > > > > On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
> > > > > > 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.
> > > > >
> > > > > IIUC, the kernel's crypto layer can support multiple implementations of
> > > > > any algorithm. Providers can report a priority against implementations
> > > > > which influences which impl is used in practice. So if there's a native
> > > > > instruction for a partiuclar algorithm I would expect the impl registered
> > > > > for that to be designated higher priority than other impls, so that it is
> > > > > used in preference to other impls.
> > > > >
> > > >
> > > > AFAIR the problem here is that in (the guest) kernel the virtio-crypto
> > > > driver has to register it's crypto algo implementations with a priority
> > > > (single number), which dictates if it's going to be the preferred (used)
> > > > implementation of the algorithm or not. The virtio-crypto driver does this
> > > > without having information about the (comparative or absolute)
> performance
> > > > of it's implementation (which depends on the backend among others). I
> don't think
> > > > any dynamic re-prioritization of the algorithms takes place (e.g. based on
> how these
> > > > perform in for the given configuration).
> > > >
> > > > I think the strategy of the virtio-crypto is to rather overstate, than
> > > > understate the performance of it's implementation. If we were to 'be
> > > > conservative' and say, 'hey we don't know nothing about the performance,
> > > > let's make it lowest priority implementation' the implementations
> provided
> > > > by virtio-crypto would end up being used only if there is no other
> > > > implementation. And that does not sound like a good idea either.
> > >
> > >
> > > This problem you describe, however, is something that applies to *any*
> > > kerenl code that is registering a crypto algo impl for accelerator
> > > hardware. A non-virtualized crypto cards in bare metal likewise cannot
> > > assume that its AES impl is better then the host CPU's  aes-ni instruction.
> > >
> > > > So the idea is to give the user the power to effectively not provide
> > > > an algorithm via virtio-crypto. That is, if the user observes a performance
> > > > degradation because of virtio-crypto, he can turn off the bad algorithms
> > > > at the device. That way overstatement becomes a much smaller problem.
> > > > The user can turn off the bad algorithms for reasons other than
> performance
> > > > too.
> > > >
> > > > Of course there are other ways to deal with the problem of virtio-crypto
> > > > driver not knowing how good it's implementation of a given algo is. We
> > > > could make the in kernel crypto priorities dynamically adjustable in
> general
> > > > or we could provide the user with means to specify the priorities (e.g.
> > > > as module parameter) with which the virtio-crypto driver registers each
> algo.
> > > > Both of these would be knobs in the guest. It's hard to tell if these first
> > > > one would be useful in scenarios not involving virtualization. Same goes
> > > > for some kind of dynamic priority management for crypto algorithm
> implementations
> > > > in the Linux kernel. I assume the people involved with the respective
> > > > subsystem do not see the necessity for something like that.
> > >
> > > It still feels like this is a problem for the guest OS to solve. If you
> > > put a physical crypto accelerator in a bare metal machine, that has the
> > > same problem you describe here, so the kernel surely already needs to find
> > > a viable solution for this problem.
> > >
> >
> > How would the guest OS know which algo is better? As you mentioned it does
> > depend on few factors and the best the kernel can do is use some sort of
> > heuristics. Such a solution might not be very dynamic and might not work for
> > all the cases for a user.
> 
> Which is better will likely depend on the application using it. One might
> be better for use by the kernel, while another is better for use by a
> userspace application, or two userspace apps might have different
> preferences.
> 
> > Shouldn't we use virtualization to give us the flexibility that we don't
> > have with physical crypto accelerator? The crypto accelerator might not know
> > if it's implementation is any better, but the user can experiment and see
> > what works better.
> 
> It is better to provide it all to the guest and let the guest decide which
> is best to use.  If nothing else the virtio-crypto kernel module itself
> can have module parameters to control the priority it gives to each
> algorithm, or can avoid registering certain algorithms.  Doing it guest
> side is more flexible, because realistically many virt host deployments
> will never give the guest admin ability to control this from the host
> side, so a guest kernel config ability will be the only thing available.
> 

From the perspective of your communication and production deployment, 
I tend to agree with Daniel’s point of view. AFAICT DPDK cryptodev scheduler
PMD driver did this thing:

" Added a packet-size based distribution mode, which distributes the enqueued
Crypto operations among two slaves, based on their data lengths."

Which it means that the guest's driver makes the decision.

Currently the Linux crypto framework uses the static priority of one algo to decide 
to choose what is simply. Maybe it should add a scheduler layer too?

Regards,
-Gonglei

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

[Daniel P. Berrangé]

On Fri, Jun 15, 2018 at 12:52:05AM +0000, Gonglei (Arei) wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel P. Berrangé [mailto:berrange@redhat.com]
> > Sent: Thursday, June 14, 2018 11:11 PM
> > To: Farhan Ali <alifm@linux.ibm.com>
> > Cc: Halil Pasic <pasic@linux.ibm.com>; qemu-devel@nongnu.org;
> > frankja@linux.ibm.com; mst@redhat.com; borntraeger@de.ibm.com; Gonglei
> > (Arei) <arei.gonglei@huawei.com>; longpeng <longpeng2@huawei.com>;
> > Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>;
> > mjrosato@linux.vnet.ibm.com
> > Subject: Re: [Qemu-devel] [RFC v1 1/1] virtio-crypto: Allow disabling of cipher
> > algorithms for virtio-crypto device
> > 
> > On Thu, Jun 14, 2018 at 10:50:40AM -0400, Farhan Ali wrote:
> > >
> > >
> > > On 06/14/2018 04:21 AM, Daniel P. Berrangé wrote:
> > > > On Wed, Jun 13, 2018 at 07:28:08PM +0200, Halil Pasic wrote:
> > > > >
> > > > >
> > > > > On 06/13/2018 05:05 PM, Daniel P. Berrangé wrote:
> > > > > > On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
> > > > > > > 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.
> > > > > >
> > > > > > IIUC, the kernel's crypto layer can support multiple implementations of
> > > > > > any algorithm. Providers can report a priority against implementations
> > > > > > which influences which impl is used in practice. So if there's a native
> > > > > > instruction for a partiuclar algorithm I would expect the impl registered
> > > > > > for that to be designated higher priority than other impls, so that it is
> > > > > > used in preference to other impls.
> > > > > >
> > > > >
> > > > > AFAIR the problem here is that in (the guest) kernel the virtio-crypto
> > > > > driver has to register it's crypto algo implementations with a priority
> > > > > (single number), which dictates if it's going to be the preferred (used)
> > > > > implementation of the algorithm or not. The virtio-crypto driver does this
> > > > > without having information about the (comparative or absolute)
> > performance
> > > > > of it's implementation (which depends on the backend among others). I
> > don't think
> > > > > any dynamic re-prioritization of the algorithms takes place (e.g. based on
> > how these
> > > > > perform in for the given configuration).
> > > > >
> > > > > I think the strategy of the virtio-crypto is to rather overstate, than
> > > > > understate the performance of it's implementation. If we were to 'be
> > > > > conservative' and say, 'hey we don't know nothing about the performance,
> > > > > let's make it lowest priority implementation' the implementations
> > provided
> > > > > by virtio-crypto would end up being used only if there is no other
> > > > > implementation. And that does not sound like a good idea either.
> > > >
> > > >
> > > > This problem you describe, however, is something that applies to *any*
> > > > kerenl code that is registering a crypto algo impl for accelerator
> > > > hardware. A non-virtualized crypto cards in bare metal likewise cannot
> > > > assume that its AES impl is better then the host CPU's  aes-ni instruction.
> > > >
> > > > > So the idea is to give the user the power to effectively not provide
> > > > > an algorithm via virtio-crypto. That is, if the user observes a performance
> > > > > degradation because of virtio-crypto, he can turn off the bad algorithms
> > > > > at the device. That way overstatement becomes a much smaller problem.
> > > > > The user can turn off the bad algorithms for reasons other than
> > performance
> > > > > too.
> > > > >
> > > > > Of course there are other ways to deal with the problem of virtio-crypto
> > > > > driver not knowing how good it's implementation of a given algo is. We
> > > > > could make the in kernel crypto priorities dynamically adjustable in
> > general
> > > > > or we could provide the user with means to specify the priorities (e.g.
> > > > > as module parameter) with which the virtio-crypto driver registers each
> > algo.
> > > > > Both of these would be knobs in the guest. It's hard to tell if these first
> > > > > one would be useful in scenarios not involving virtualization. Same goes
> > > > > for some kind of dynamic priority management for crypto algorithm
> > implementations
> > > > > in the Linux kernel. I assume the people involved with the respective
> > > > > subsystem do not see the necessity for something like that.
> > > >
> > > > It still feels like this is a problem for the guest OS to solve. If you
> > > > put a physical crypto accelerator in a bare metal machine, that has the
> > > > same problem you describe here, so the kernel surely already needs to find
> > > > a viable solution for this problem.
> > > >
> > >
> > > How would the guest OS know which algo is better? As you mentioned it does
> > > depend on few factors and the best the kernel can do is use some sort of
> > > heuristics. Such a solution might not be very dynamic and might not work for
> > > all the cases for a user.
> > 
> > Which is better will likely depend on the application using it. One might
> > be better for use by the kernel, while another is better for use by a
> > userspace application, or two userspace apps might have different
> > preferences.
> > 
> > > Shouldn't we use virtualization to give us the flexibility that we don't
> > > have with physical crypto accelerator? The crypto accelerator might not know
> > > if it's implementation is any better, but the user can experiment and see
> > > what works better.
> > 
> > It is better to provide it all to the guest and let the guest decide which
> > is best to use.  If nothing else the virtio-crypto kernel module itself
> > can have module parameters to control the priority it gives to each
> > algorithm, or can avoid registering certain algorithms.  Doing it guest
> > side is more flexible, because realistically many virt host deployments
> > will never give the guest admin ability to control this from the host
> > side, so a guest kernel config ability will be the only thing available.
> > 
> 
> From the perspective of your communication and production deployment, 
> I tend to agree with Daniel’s point of view. AFAICT DPDK cryptodev scheduler
> PMD driver did this thing:
> 
> " Added a packet-size based distribution mode, which distributes the enqueued
> Crypto operations among two slaves, based on their data lengths."
> 
> Which it means that the guest's driver makes the decision.
> 
> Currently the Linux crypto framework uses the static priority of one algo to decide 
> to choose what is simply. Maybe it should add a scheduler layer too?

The simplest option would probably be either a kernel module parameter,
or even better, sysfs tunables, to allow the priorities to be set
dynamically

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Christian Borntraeger]

On 06/13/2018 05:05 PM, Daniel P. Berrangé wrote:
> On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
>> 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.
> 
> IIUC, the kernel's crypto layer can support multiple implementations of
> any algorithm. Providers can report a priority against implementations
> which influences which impl is used in practice. So if there's a native
> instruction for a partiuclar algorithm I would expect the impl registered
> for that to be designated higher priority than other impls, so that it is
> used in preference to other impls.

Yes. This is what the kernel currently does. Back then I asked Gonglei to reduce
the default priority of virtio-crypto  (it was 501 and thus higher than anybody
else). Right now virtio-crypto has lower priority than the s390 cpacf code.

What does make sense though, is to actually have a mean to synchronize between
guest driver and host provider about what are actually supported operations.

Right now you only have aes cbc. What happens now, when a new driver also supports
lets say sha3, but the host does not. Having some kind of handshaking certainly 
makes sense. Do we need this configurable? This is another question. 

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

[Viktor VM Mihajlovski]

On 14.06.2018 18:12, Farhan Ali wrote:
> 
> 
> On 06/14/2018 11:10 AM, Daniel P. Berrangé wrote:
>> On Thu, Jun 14, 2018 at 10:50:40AM -0400, Farhan Ali wrote:
>>>
>>>
>>> On 06/14/2018 04:21 AM, Daniel P. Berrangé wrote:
>>>> On Wed, Jun 13, 2018 at 07:28:08PM +0200, Halil Pasic wrote:
>>>>>
>>>>>
>>>>> On 06/13/2018 05:05 PM, Daniel P. Berrangé wrote:
>>>>>> On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
>>>>>>> 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.
>>>>>>
>>>>>> IIUC, the kernel's crypto layer can support multiple
>>>>>> implementations of
>>>>>> any algorithm. Providers can report a priority against
>>>>>> implementations
>>>>>> which influences which impl is used in practice. So if there's a
>>>>>> native
>>>>>> instruction for a partiuclar algorithm I would expect the impl
>>>>>> registered
>>>>>> for that to be designated higher priority than other impls, so
>>>>>> that it is
>>>>>> used in preference to other impls.
>>>>>>
>>>>>
>>>>> AFAIR the problem here is that in (the guest) kernel the virtio-crypto
>>>>> driver has to register it's crypto algo implementations with a
>>>>> priority
>>>>> (single number), which dictates if it's going to be the preferred
>>>>> (used)
>>>>> implementation of the algorithm or not. The virtio-crypto driver
>>>>> does this
>>>>> without having information about the (comparative or absolute)
>>>>> performance
>>>>> of it's implementation (which depends on the backend among others).
>>>>> I don't think
>>>>> any dynamic re-prioritization of the algorithms takes place (e.g.
>>>>> based on how these
>>>>> perform in for the given configuration).
>>>>>
>>>>> I think the strategy of the virtio-crypto is to rather overstate, than
>>>>> understate the performance of it's implementation. If we were to 'be
>>>>> conservative' and say, 'hey we don't know nothing about the
>>>>> performance,
>>>>> let's make it lowest priority implementation' the implementations
>>>>> provided
>>>>> by virtio-crypto would end up being used only if there is no other
>>>>> implementation. And that does not sound like a good idea either.
>>>>
>>>>
>>>> This problem you describe, however, is something that applies to *any*
>>>> kerenl code that is registering a crypto algo impl for accelerator
>>>> hardware. A non-virtualized crypto cards in bare metal likewise cannot
>>>> assume that its AES impl is better then the host CPU's  aes-ni
>>>> instruction.
>>>>
>>>>> So the idea is to give the user the power to effectively not provide
>>>>> an algorithm via virtio-crypto. That is, if the user observes a
>>>>> performance
>>>>> degradation because of virtio-crypto, he can turn off the bad
>>>>> algorithms
>>>>> at the device. That way overstatement becomes a much smaller problem.
>>>>> The user can turn off the bad algorithms for reasons other than
>>>>> performance
>>>>> too.
>>>>>
>>>>> Of course there are other ways to deal with the problem of
>>>>> virtio-crypto
>>>>> driver not knowing how good it's implementation of a given algo is. We
>>>>> could make the in kernel crypto priorities dynamically adjustable
>>>>> in general
>>>>> or we could provide the user with means to specify the priorities
>>>>> (e.g.
>>>>> as module parameter) with which the virtio-crypto driver registers
>>>>> each algo.
>>>>> Both of these would be knobs in the guest. It's hard to tell if
>>>>> these first
>>>>> one would be useful in scenarios not involving virtualization. Same
>>>>> goes
>>>>> for some kind of dynamic priority management for crypto algorithm
>>>>> implementations
>>>>> in the Linux kernel. I assume the people involved with the respective
>>>>> subsystem do not see the necessity for something like that.
>>>>
>>>> It still feels like this is a problem for the guest OS to solve. If you
>>>> put a physical crypto accelerator in a bare metal machine, that has the
>>>> same problem you describe here, so the kernel surely already needs
>>>> to find
>>>> a viable solution for this problem.
>>>>
>>>
>>> How would the guest OS know which algo is better? As you mentioned it
>>> does
>>> depend on few factors and the best the kernel can do is use some sort of
>>> heuristics. Such a solution might not be very dynamic and might not
>>> work for
>>> all the cases for a user.
>>
>> Which is better will likely depend on the application using it. One might
>> be better for use by the kernel, while another is better for use by a
>> userspace application, or two userspace apps might have different
>> preferences.
>>
>>> Shouldn't we use virtualization to give us the flexibility that we don't
>>> have with physical crypto accelerator? The crypto accelerator might
>>> not know
>>> if it's implementation is any better, but the user can experiment and
>>> see
>>> what works better.
>>
>> It is better to provide it all to the guest and let the guest decide
>> which
>> is best to use.  If nothing else the virtio-crypto kernel module itself
>> can have module parameters to control the priority it gives to each
>> algorithm, or can avoid registering certain algorithms.  Doing it guest
>> side is more flexible, because realistically many virt host deployments
>> will never give the guest admin ability to control this from the host
>> side, so a guest kernel config ability will be the only thing available.
>>
> 
> I am not sure if putting all that complexity on the guest OS is the
> right approach. I thought it would be better to let the user decide
> through device definition what algorithms should be available to the
> guest. But I am open to other approaches and suggestion :)
> 
> I would like to know if Arei or Longpeng(Mike) has any suggestion
> regarding this?
> 
> Thanks
> Farhan
> 

With the current virtio-crypto backend functionality offered (CBC AES
only) it may seem a bit over-engineered to offer a configuration option
to remove the only supported algorithm...

What I could imagine to be useful though, would be to allow the backend
to advertise its capabilities to the guest virtio-crypto device, so that
the guest driver can register the algorithms supported dynamically.
Currently, the algorithms are hard-coded on both sides which makes it a
bit hard to extend the backends to support new algorithms (or write new
backends if so desired).

Whether the backend itself was configurable would be of less importance
then (but still could make sense).

-- 
Regards,
  Viktor Mihajlovski

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

[Farhan Ali]

On 06/15/2018 09:17 AM, Viktor VM Mihajlovski wrote:
> On 14.06.2018 18:12, Farhan Ali wrote:
>>
>>
>> On 06/14/2018 11:10 AM, Daniel P. Berrangé wrote:
>>> On Thu, Jun 14, 2018 at 10:50:40AM -0400, Farhan Ali wrote:
>>>>
>>>>
>>>> On 06/14/2018 04:21 AM, Daniel P. Berrangé wrote:
>>>>> On Wed, Jun 13, 2018 at 07:28:08PM +0200, Halil Pasic wrote:
>>>>>>
>>>>>>
>>>>>> On 06/13/2018 05:05 PM, Daniel P. Berrangé wrote:
>>>>>>> On Wed, Jun 13, 2018 at 11:01:05AM -0400, Farhan Ali wrote:
>>>>>>>> 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.
>>>>>>>
>>>>>>> IIUC, the kernel's crypto layer can support multiple
>>>>>>> implementations of
>>>>>>> any algorithm. Providers can report a priority against
>>>>>>> implementations
>>>>>>> which influences which impl is used in practice. So if there's a
>>>>>>> native
>>>>>>> instruction for a partiuclar algorithm I would expect the impl
>>>>>>> registered
>>>>>>> for that to be designated higher priority than other impls, so
>>>>>>> that it is
>>>>>>> used in preference to other impls.
>>>>>>>
>>>>>>
>>>>>> AFAIR the problem here is that in (the guest) kernel the virtio-crypto
>>>>>> driver has to register it's crypto algo implementations with a
>>>>>> priority
>>>>>> (single number), which dictates if it's going to be the preferred
>>>>>> (used)
>>>>>> implementation of the algorithm or not. The virtio-crypto driver
>>>>>> does this
>>>>>> without having information about the (comparative or absolute)
>>>>>> performance
>>>>>> of it's implementation (which depends on the backend among others).
>>>>>> I don't think
>>>>>> any dynamic re-prioritization of the algorithms takes place (e.g.
>>>>>> based on how these
>>>>>> perform in for the given configuration).
>>>>>>
>>>>>> I think the strategy of the virtio-crypto is to rather overstate, than
>>>>>> understate the performance of it's implementation. If we were to 'be
>>>>>> conservative' and say, 'hey we don't know nothing about the
>>>>>> performance,
>>>>>> let's make it lowest priority implementation' the implementations
>>>>>> provided
>>>>>> by virtio-crypto would end up being used only if there is no other
>>>>>> implementation. And that does not sound like a good idea either.
>>>>>
>>>>>
>>>>> This problem you describe, however, is something that applies to *any*
>>>>> kerenl code that is registering a crypto algo impl for accelerator
>>>>> hardware. A non-virtualized crypto cards in bare metal likewise cannot
>>>>> assume that its AES impl is better then the host CPU's  aes-ni
>>>>> instruction.
>>>>>
>>>>>> So the idea is to give the user the power to effectively not provide
>>>>>> an algorithm via virtio-crypto. That is, if the user observes a
>>>>>> performance
>>>>>> degradation because of virtio-crypto, he can turn off the bad
>>>>>> algorithms
>>>>>> at the device. That way overstatement becomes a much smaller problem.
>>>>>> The user can turn off the bad algorithms for reasons other than
>>>>>> performance
>>>>>> too.
>>>>>>
>>>>>> Of course there are other ways to deal with the problem of
>>>>>> virtio-crypto
>>>>>> driver not knowing how good it's implementation of a given algo is. We
>>>>>> could make the in kernel crypto priorities dynamically adjustable
>>>>>> in general
>>>>>> or we could provide the user with means to specify the priorities
>>>>>> (e.g.
>>>>>> as module parameter) with which the virtio-crypto driver registers
>>>>>> each algo.
>>>>>> Both of these would be knobs in the guest. It's hard to tell if
>>>>>> these first
>>>>>> one would be useful in scenarios not involving virtualization. Same
>>>>>> goes
>>>>>> for some kind of dynamic priority management for crypto algorithm
>>>>>> implementations
>>>>>> in the Linux kernel. I assume the people involved with the respective
>>>>>> subsystem do not see the necessity for something like that.
>>>>>
>>>>> It still feels like this is a problem for the guest OS to solve. If you
>>>>> put a physical crypto accelerator in a bare metal machine, that has the
>>>>> same problem you describe here, so the kernel surely already needs
>>>>> to find
>>>>> a viable solution for this problem.
>>>>>
>>>>
>>>> How would the guest OS know which algo is better? As you mentioned it
>>>> does
>>>> depend on few factors and the best the kernel can do is use some sort of
>>>> heuristics. Such a solution might not be very dynamic and might not
>>>> work for
>>>> all the cases for a user.
>>>
>>> Which is better will likely depend on the application using it. One might
>>> be better for use by the kernel, while another is better for use by a
>>> userspace application, or two userspace apps might have different
>>> preferences.
>>>
>>>> Shouldn't we use virtualization to give us the flexibility that we don't
>>>> have with physical crypto accelerator? The crypto accelerator might
>>>> not know
>>>> if it's implementation is any better, but the user can experiment and
>>>> see
>>>> what works better.
>>>
>>> It is better to provide it all to the guest and let the guest decide
>>> which
>>> is best to use.  If nothing else the virtio-crypto kernel module itself
>>> can have module parameters to control the priority it gives to each
>>> algorithm, or can avoid registering certain algorithms.  Doing it guest
>>> side is more flexible, because realistically many virt host deployments
>>> will never give the guest admin ability to control this from the host
>>> side, so a guest kernel config ability will be the only thing available.
>>>
>>
>> I am not sure if putting all that complexity on the guest OS is the
>> right approach. I thought it would be better to let the user decide
>> through device definition what algorithms should be available to the
>> guest. But I am open to other approaches and suggestion :)
>>
>> I would like to know if Arei or Longpeng(Mike) has any suggestion
>> regarding this?
>>
>> Thanks
>> Farhan
>>
> 
> With the current virtio-crypto backend functionality offered (CBC AES
> only) it may seem a bit over-engineered to offer a configuration option
> to remove the only supported algorithm...
> 
> What I could imagine to be useful though, would be to allow the backend
> to advertise its capabilities to the guest virtio-crypto device, so that
> the guest driver can register the algorithms supported dynamically.
> Currently, the algorithms are hard-coded on both sides which makes it a
> bit hard to extend the backends to support new algorithms (or write new
> backends if so desired).

I posted some kernel patches 
(https://www.spinics.net/lists/kvm/msg170332.html), that takes care of 
registering algorithms based on what the backend advertises.


> 
> Whether the backend itself was configurable would be of less importance
> then (but still could make sense).
> 

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

[Viktor VM Mihajlovski]

On 15.06.2018 17:10, Farhan Ali wrote:
> 
> 
> On 06/15/2018 09:17 AM, Viktor VM Mihajlovski wrote:
[...]
>>
>> With the current virtio-crypto backend functionality offered (CBC AES
>> only) it may seem a bit over-engineered to offer a configuration option
>> to remove the only supported algorithm...
>>
>> What I could imagine to be useful though, would be to allow the backend
>> to advertise its capabilities to the guest virtio-crypto device, so that
>> the guest driver can register the algorithms supported dynamically.
>> Currently, the algorithms are hard-coded on both sides which makes it a
>> bit hard to extend the backends to support new algorithms (or write new
>> backends if so desired).
> 
> I posted some kernel patches
> (https://www.spinics.net/lists/kvm/msg170332.html), that takes care of
> registering algorithms based on what the backend advertises.
> 
Sorry, I missed that. Sounds as if the principal mechanism to configure
guest virtio-crypto based on the host capabilities would be established
with the patches.
> 
>>
>> Whether the backend itself was configurable would be of less importance
>> then (but still could make sense).
>>
> 
> 


-- 
Regards,
  Viktor Mihajlovski