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
> -----Original Message-----
> From: zhenwei pi [mailto:[email protected]]
> Sent: Tuesday, March 1, 2022 6:26 PM
> To: Gonglei (Arei) <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; kernel test robot <[email protected]>
> Subject: PING: [PATCH v2 3/3] virtio-crypto: implement RSA algorithm
>
> 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.
>
I'm fine with separating them in different c files. Then should we rename virtio_crypto_algs.c
to virtio_crypto_skcipher_algo.c?
Regards,
-Gonglei
> 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.akciphe
> >> r);
> >> else
> >> crypto_register_skcipher(&virtio_crypto_skcipher_algs[i].algo.skciphe
> >> r);
> >> ...
> >> }
> >> ...
> >> }
> >>
> >> 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