> -----Original Message----- > From: zhenwei pi [mailto:pizhenwei@xxxxxxxxxxxxx] > Sent: Tuesday, March 1, 2022 6:26 PM > To: Gonglei (Arei) <arei.gonglei@xxxxxxxxxx> > Cc: jasowang@xxxxxxxxxx; mst@xxxxxxxxxx; > virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx; linux-crypto@xxxxxxxxxxxxxxx; > linux-kernel@xxxxxxxxxxxxxxx; helei.sig11@xxxxxxxxxxxxx; > herbert@xxxxxxxxxxxxxxxxxxx; kernel test robot <lkp@xxxxxxxxx> > 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
