Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: PING: [PATCH v2 3/3] virtio-crypto: implement RSA algorithm
From:   zhenwei pi <pizhenwei@bytedance.com>
To:     "Gonglei (Arei)" <arei.gonglei@huawei.com>
Cc:     "jasowang@redhat.com" <jasowang@redhat.com>,
        "mst@redhat.com" <mst@redhat.com>,
        "virtualization@lists.linux-foundation.org" 
        <virtualization@lists.linux-foundation.org>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "helei.sig11@bytedance.com" <helei.sig11@bytedance.com>,
        "herbert@gondor.apana.org.au" <herbert@gondor.apana.org.au>,
        kernel test robot <lkp@intel.com>
References: <20220211084108.1254218-1-pizhenwei@bytedance.com>
 <20220211084108.1254218-4-pizhenwei@bytedance.com>
 <c9144b0d82e34566a960f210ddc32696@huawei.com>
 <8ef2f660-bd84-de70-1539-402c73795dfe@bytedance.com>
 <bc2bbc3b-8a34-2f09-41f5-60f1568a6bef@bytedance.com>
Message-ID: <0c148ada-9f32-3272-8a89-591299ab098d@bytedance.com>
Date:   Tue, 1 Mar 2022 18:25:36 +0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
MIME-Version: 1.0
In-Reply-To: <bc2bbc3b-8a34-2f09-41f5-60f1568a6bef@bytedance.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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