Virtualization - Mar 2022 - PING: [PATCH v2 3/3] virtio-crypto: implement RSA algorithm

>> +void virtio_crypto_akcipher_algs_unregister(struct virtio_crypto
>> +*vcrypto) {
>> +	int i = 0;
>> +
>> +	mutex_lock(&algs_lock);
>> +
>> +	for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs); i++) {
>> +		uint32_t service = virtio_crypto_akcipher_algs[i].service;
>> +		uint32_t algonum = virtio_crypto_akcipher_algs[i].algonum;
>> +
>> +		if (virtio_crypto_akcipher_algs[i].active_devs == 0 ||
>> +		    !virtcrypto_algo_is_supported(vcrypto, service, algonum))
>> +			continue;
>> +
>> +		if (virtio_crypto_akcipher_algs[i].active_devs == 1)
>> +
>> 	crypto_unregister_akcipher(&virtio_crypto_akcipher_algs[i].algo);
>> +
>> +		virtio_crypto_akcipher_algs[i].active_devs--;
>> +	}
>> +
>> +	mutex_unlock(&algs_lock);
>> +}
> 
> Why don't you reuse the virtio_crypto_algs_register/unregister
functions?
> The current code is too repetitive. Maybe we don't need create the new
file virtio_crypto_akcipher_algo.c
> because we had virtio_crypto_algs.c which includes all algorithms.
> 
Yes, this looks similar to virtio_crypto_algs_register/unregister.

Let's look at the difference:
struct virtio_crypto_akcipher_algo {
         uint32_t algonum;
         uint32_t service;
         unsigned int active_devs;
         struct akcipher_alg algo;
};

struct virtio_crypto_algo {
         uint32_t algonum;
         uint32_t service; 

         unsigned int active_devs;
         struct skcipher_alg algo; /* akcipher_alg VS skcipher_alg */
};

If reusing virtio_crypto_algs_register/unregister, we need to modify the 
data structure like this:
struct virtio_crypto_akcipher_algo {
         uint32_t algonum;
         uint32_t service;	/* use service to distinguish 
akcipher/skcipher */
         unsigned int active_devs;
	union {
		struct skcipher_alg skcipher;
	        struct akcipher_alg akcipher;
	} alg;
};

int virtio_crypto_akcipher_algs_register(struct virtio_crypto *vcrypto)
{
	...
         for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs); i++) {
                 uint32_t service = virtio_crypto_akcipher_algs[i].service;
		...
		/* test service type then call 
crypto_register_akcipher/crypto_register_skcipher */
                 if (service == VIRTIO_CRYPTO_SERVICE_AKCIPHER)
			crypto_register_akcipher(&virtio_crypto_akcipher_algs[i].algo.akcipher);
		else
			crypto_register_skcipher(&virtio_crypto_skcipher_algs[i].algo.skcipher);
		...
         }
	...
}

Also test service type and call 
crypto_unregister_skcipher/crypto_unregister_akcipher.

This gets unclear from current v2 version.

On the other hand, the kernel side prefers to separate skcipher and 
akcipher(separated header files and implementations).

-- 
zhenwei pi

On 2/18/22 11:12 AM, zhenwei pi wrote:
>>> +void virtio_crypto_akcipher_algs_unregister(struct virtio_crypto
>>> +*vcrypto) {
>>> +??? int i = 0;
>>> +
>>> +??? mutex_lock(&algs_lock);
>>> +
>>> +??? for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs);
i++) {
>>> +??????? uint32_t service = virtio_crypto_akcipher_algs[i].service;
>>> +??????? uint32_t algonum = virtio_crypto_akcipher_algs[i].algonum;
>>> +
>>> +??????? if (virtio_crypto_akcipher_algs[i].active_devs == 0 ||
>>> +??????????? !virtcrypto_algo_is_supported(vcrypto, service,
algonum))
>>> +??????????? continue;
>>> +
>>> +??????? if (virtio_crypto_akcipher_algs[i].active_devs == 1)
>>> +
>>>
????crypto_unregister_akcipher(&virtio_crypto_akcipher_algs[i].algo);
>>> +
>>> +??????? virtio_crypto_akcipher_algs[i].active_devs--;
>>> +??? }
>>> +
>>> +??? mutex_unlock(&algs_lock);
>>> +}
>>
>> Why don't you reuse the virtio_crypto_algs_register/unregister
functions?
>> The current code is too repetitive. Maybe we don't need create the
new
>> file virtio_crypto_akcipher_algo.c
>> because we had virtio_crypto_algs.c which includes all algorithms.
>>
> 
> Yes, this looks similar to virtio_crypto_algs_register/unregister.
> 
> Let's look at the difference:
> struct virtio_crypto_akcipher_algo {
>  ??????? uint32_t algonum;
>  ??????? uint32_t service;
>  ??????? unsigned int active_devs;
>  ??????? struct akcipher_alg algo;
> };
> 
> struct virtio_crypto_algo {
>  ??????? uint32_t algonum;
>  ??????? uint32_t service;
>  ??????? unsigned int active_devs;
>  ??????? struct skcipher_alg algo; /* akcipher_alg VS skcipher_alg */
> };
> 
> If reusing virtio_crypto_algs_register/unregister, we need to modify the 
> data structure like this:
> struct virtio_crypto_akcipher_algo {
>  ??????? uint32_t algonum;
>  ??????? uint32_t service;??? /* use service to distinguish 
> akcipher/skcipher */
>  ??????? unsigned int active_devs;
>  ????union {
>  ??????? struct skcipher_alg skcipher;
>  ??????????? struct akcipher_alg akcipher;
>  ????} alg;
> };
> 
> int virtio_crypto_akcipher_algs_register(struct virtio_crypto *vcrypto)
> {
>  ????...
>  ??????? for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs); i++) {
>  ??????????????? uint32_t service = virtio_crypto_akcipher_algs[i].service;
>  ??????? ...
>  ??????? /* test service type then call 
> crypto_register_akcipher/crypto_register_skcipher */
>  ??????????????? if (service == VIRTIO_CRYPTO_SERVICE_AKCIPHER)
>              
>
crypto_register_akcipher(&virtio_crypto_akcipher_algs[i].algo.akcipher);
>  ??????? else
>              
>
crypto_register_skcipher(&virtio_crypto_skcipher_algs[i].algo.skcipher);
>  ??????? ...
>  ??????? }
>  ????...
> }
> 
> Also test service type and call 
> crypto_unregister_skcipher/crypto_unregister_akcipher.
> 
> This gets unclear from current v2 version.
> 
> On the other hand, the kernel side prefers to separate skcipher and 
> akcipher(separated header files and implementations).
> 
Hi, Lei
I also take a look at other crypto drivers at qat/ccp/hisilicon, they 
separate akcipher/skcipher algo. If you consider that reusing 
virtio_crypto_algs_register/unregister seems better, I will try to merge 
them into a single function.

-- 
zhenwei pi

PING!

Hi, Lei
I also take a look at other crypto drivers qat/ccp/hisilicon, they
separate akcipher/skcipher algo. If you consider that reusing
virtio_crypto_algs_register/unregister seems better, I will try to merge
them into a single function.

On 2/23/22 6:17 PM, zhenwei pi wrote:
> 
> On 2/18/22 11:12 AM, zhenwei pi wrote:
>>>> +void virtio_crypto_akcipher_algs_unregister(struct
virtio_crypto
>>>> +*vcrypto) {
>>>> +??? int i = 0;
>>>> +
>>>> +??? mutex_lock(&algs_lock);
>>>> +
>>>> +??? for (i = 0; i <
ARRAY_SIZE(virtio_crypto_akcipher_algs); i++) {
>>>> +??????? uint32_t service =
virtio_crypto_akcipher_algs[i].service;
>>>> +??????? uint32_t algonum =
virtio_crypto_akcipher_algs[i].algonum;
>>>> +
>>>> +??????? if (virtio_crypto_akcipher_algs[i].active_devs == 0 ||
>>>> +??????????? !virtcrypto_algo_is_supported(vcrypto, service,
algonum))
>>>> +??????????? continue;
>>>> +
>>>> +??????? if (virtio_crypto_akcipher_algs[i].active_devs == 1)
>>>> +
>>>>
????crypto_unregister_akcipher(&virtio_crypto_akcipher_algs[i].algo);
>>>> +
>>>> +??????? virtio_crypto_akcipher_algs[i].active_devs--;
>>>> +??? }
>>>> +
>>>> +??? mutex_unlock(&algs_lock);
>>>> +}
>>>
>>> Why don't you reuse the virtio_crypto_algs_register/unregister 
>>> functions?
>>> The current code is too repetitive. Maybe we don't need create
the
>>> new file virtio_crypto_akcipher_algo.c
>>> because we had virtio_crypto_algs.c which includes all algorithms.
>>>
>>
>> Yes, this looks similar to virtio_crypto_algs_register/unregister.
>>
>> Let's look at the difference:
>> struct virtio_crypto_akcipher_algo {
>> ???????? uint32_t algonum;
>> ???????? uint32_t service;
>> ???????? unsigned int active_devs;
>> ???????? struct akcipher_alg algo;
>> };
>>
>> struct virtio_crypto_algo {
>> ???????? uint32_t algonum;
>> ???????? uint32_t service;
>> ???????? unsigned int active_devs;
>> ???????? struct skcipher_alg algo; /* akcipher_alg VS skcipher_alg */
>> };
>>
>> If reusing virtio_crypto_algs_register/unregister, we need to modify 
>> the data structure like this:
>> struct virtio_crypto_akcipher_algo {
>> ???????? uint32_t algonum;
>> ???????? uint32_t service;??? /* use service to distinguish 
>> akcipher/skcipher */
>> ???????? unsigned int active_devs;
>> ?????union {
>> ???????? struct skcipher_alg skcipher;
>> ???????????? struct akcipher_alg akcipher;
>> ?????} alg;
>> };
>>
>> int virtio_crypto_akcipher_algs_register(struct virtio_crypto *vcrypto)
>> {
>> ?????...
>> ???????? for (i = 0; i < ARRAY_SIZE(virtio_crypto_akcipher_algs);
i++) {
>> ???????????????? uint32_t service = 
>> virtio_crypto_akcipher_algs[i].service;
>> ???????? ...
>> ???????? /* test service type then call 
>> crypto_register_akcipher/crypto_register_skcipher */
>> ???????????????? if (service == VIRTIO_CRYPTO_SERVICE_AKCIPHER)
>>
crypto_register_akcipher(&virtio_crypto_akcipher_algs[i].algo.akcipher);
>> ???????? else
>>
crypto_register_skcipher(&virtio_crypto_skcipher_algs[i].algo.skcipher);
>> ???????? ...
>> ???????? }
>> ?????...
>> }
>>
>> Also test service type and call 
>> crypto_unregister_skcipher/crypto_unregister_akcipher.
>>
>> This gets unclear from current v2 version.
>>
>> On the other hand, the kernel side prefers to separate skcipher and 
>> akcipher(separated header files and implementations).
>>
>
-- 
zhenwei pi