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

[Gonglei (Arei)]

> -----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