From: Farhan Ali <alifm@linux.ibm.com>
To: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: qemu-devel@nongnu.org, frankja@linux.ibm.com, mst@redhat.com,
pasic@linux.ibm.com, borntraeger@de.ibm.com,
arei.gonglei@huawei.com, 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
Date: Wed, 13 Jun 2018 11:01:05 -0400 [thread overview]
Message-ID: <7b51465a-b7c1-58ec-1ef6-9fe791e96bbf@linux.ibm.com> (raw)
In-Reply-To: <20180613093700.GG27901@redhat.com>
Hi Daniel
On 06/13/2018 05:37 AM, Daniel P. Berrangé wrote:
> On Tue, Jun 12, 2018 at 03:48:34PM -0400, Farhan Ali wrote:
>> The virtio-crypto driver currently propagates to the guest
>> all the cipher algorithms that the backend cryptodev can
>> support. But in certain cases where the guest has more
>> performant mechanism to handle some algorithms, it would be
>> useful to propagate only a subset of the algorithms.
>
> I'm not really convinced by this.
>
> The performance of crypto algorithms has many influencing
> factors, making it pretty hard to decide which is best
> without actively testing specific impls and comparing
> them in a manner which matches the application usage
> pattern. eg in theory the kernel crypto impl of an alg
> is faster than a userspace impl, if the kernel uses
> hardware accel and userspace does not. This, however,
> ignores the overhead of the kernel/userspace switch.
> The real world performance winner, thus depends on the
> amount of data being processed in each operation. Some
> times userspace can win & sometimes kernel space can
> win. This is even more relevant to virtio-crypto as
> it has more expensive context switches.
True. But what if the guest can perform some crypto algorithms without a
incurring a VM exit? For example in s390 we have the cpacf instructions
to perform crypto and this instruction is implemented for us by our
hardware virtualization technology. In such a case it would be better
not to use virtio-crypto's implementation of such a crypto algorithm.
At the same time we would like to take advantage of virtio-crypto's
acceleration capabilities for certain crypto algorithms for which there
is no hardware assistance.
>
> IOW, when we expose a virtio-crypto dev to a guest,
> it is never reasonable for the guest to blindly assume
> that anything it does is faster than a pure software
> impl running in the guest. It will depend on the usage
> pattern. This is no different to bare metal where you
> should not assume kernel crypto is faster.
>
> IMHO this is not a compelling reason to be able to turn
> off algorithms in virtio-crypto, as any decision will
> always be at best incomplete & inaccurate.
But shouldn't the user have the option to try and test by turning off
certain algorithms? You are right the performance will depend on usage
patterns, but I believe at least the user should have the option to test
and see what works and does not work. It would be far easier to do so
with the virtio-crypto dev than changing code in the kernel or userspace
IMHO.
>
>> @@ -853,6 +863,34 @@ static const VMStateDescription vmstate_virtio_crypto = {
>> static Property virtio_crypto_properties[] = {
>> DEFINE_PROP_LINK("cryptodev", VirtIOCrypto, conf.cryptodev,
>> TYPE_CRYPTODEV_BACKEND, CryptoDevBackend *),
>> + DEFINE_PROP_BIT("no-cipher", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_ARC4, false),
>> + DEFINE_PROP_BIT("cipher-arc4", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_ARC4, false),
>> + DEFINE_PROP_BIT("cipher-aes-ecb", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_AES_ECB, false),
>> + DEFINE_PROP_BIT("cipher-aes-cbc", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_AES_CBC, false),
>> + DEFINE_PROP_BIT("cipher-aes-ctr", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_AES_CTR, false),
>> + DEFINE_PROP_BIT("cipher-des-ecb", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_DES_ECB, false),
>> + DEFINE_PROP_BIT("cipher-3des-ecb", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_3DES_ECB, false),
>> + DEFINE_PROP_BIT("cipher-3des-cbc", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_3DES_CBC, false),
>> + DEFINE_PROP_BIT("cipher-3des-ctr", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_3DES_CTR, false),
>> + DEFINE_PROP_BIT("cipher-kasumi-f8", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_KASUMI_F8, false),
>> + DEFINE_PROP_BIT("cipher-snow3g-uea2", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_SNOW3G_UEA2, false),
>> + DEFINE_PROP_BIT("cipher-aes-f8", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_AES_F8, false),
>> + DEFINE_PROP_BIT("cipher-aes-xts", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_AES_XTS, false),
>> + DEFINE_PROP_BIT("cipher-zuc-eea3", VirtIOCrypto, user_cipher_algo_l,
>> + VIRTIO_CRYPTO_CIPHER_ZUC_EEA3, false),
>
> This does not scale as an approach IMHO which just reinforces to me
> that we shouldn't do this.
I am open suggestions on better implementation. I thought defining a
property bit for the virtio-crypto dev would work, similar to cpu model
approach.
Thanks for taking a look at the patch :)
Thanks
Farhan
>
>> DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> @@ -974,6 +1012,8 @@ static void virtio_crypto_instance_init(Object *obj)
>> * Can be overriden with virtio_crypto_set_config_size.
>> */
>> vcrypto->config_size = sizeof(struct virtio_crypto_config);
>> + vcrypto->user_cipher_algo_l = ~VIRTIO_CRYPTO_NO_CIPHER - 1;
>> + vcrypto->user_cipher_algo_h = ~VIRTIO_CRYPTO_NO_CIPHER;
>> }
>>
>> static const TypeInfo virtio_crypto_info = {
>> diff --git a/include/hw/virtio/virtio-crypto.h b/include/hw/virtio/virtio-crypto.h
>> index ca3a049..c5bb684 100644
>> --- a/include/hw/virtio/virtio-crypto.h
>> +++ b/include/hw/virtio/virtio-crypto.h
>> @@ -97,6 +97,9 @@ typedef struct VirtIOCrypto {
>> uint32_t curr_queues;
>> size_t config_size;
>> uint8_t vhost_started;
>> +
>> + uint32_t user_cipher_algo_l;
>> + uint32_t user_cipher_algo_h;
>> } VirtIOCrypto;
>>
>> #endif /* _QEMU_VIRTIO_CRYPTO_H */
>> --
>> 2.7.4
>>
>>
>
> Regards,
> Daniel
>
next prev parent reply other threads:[~2018-06-13 15:01 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <cover.1528832686.git.alifm@linux.ibm.com>
2018-06-12 19:48 ` [Qemu-devel] [RFC v1 1/1] virtio-crypto: Allow disabling of cipher algorithms for virtio-crypto device Farhan Ali
2018-06-13 0:57 ` Gonglei (Arei)
2018-06-13 20:14 ` Farhan Ali
2018-06-13 9:37 ` Daniel P. Berrangé
2018-06-13 15:01 ` Farhan Ali [this message]
2018-06-13 15:05 ` Daniel P. Berrangé
2018-06-13 17:28 ` Halil Pasic
2018-06-14 8:21 ` Daniel P. Berrangé
2018-06-14 14:50 ` Farhan Ali
2018-06-14 15:10 ` Daniel P. Berrangé
2018-06-14 16:12 ` Farhan Ali
2018-06-14 16:15 ` Daniel P. Berrangé
2018-06-15 13:17 ` Viktor VM Mihajlovski
2018-06-15 15:10 ` Farhan Ali
2018-06-18 10:27 ` Viktor VM Mihajlovski
2018-06-15 0:52 ` Gonglei (Arei)
2018-06-15 9:26 ` Daniel P. Berrangé
2018-06-15 13:07 ` Christian Borntraeger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7b51465a-b7c1-58ec-1ef6-9fe791e96bbf@linux.ibm.com \
--to=alifm@linux.ibm.com \
--cc=arei.gonglei@huawei.com \
--cc=berrange@redhat.com \
--cc=borntraeger@de.ibm.com \
--cc=frankja@linux.ibm.com \
--cc=longpeng2@huawei.com \
--cc=mihajlov@linux.vnet.ibm.com \
--cc=mjrosato@linux.vnet.ibm.com \
--cc=mst@redhat.com \
--cc=pasic@linux.ibm.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.