| Message ID | dd7e90c5b3de6f98218908c7f57d9d0286089ad5.1528832686.git.alifm@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----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
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
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 >
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
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
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 > >
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
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 >
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
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
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
> -----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
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
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.
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).
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). >
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). >> > >
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 */
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(-)