Re: [PATCH v2 3/3] virtio-crypto: implement RSA algorithm

[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