Re: [dpdk-dev] [dpdk-dev, v1, 2/5] eventdev: add crypto adapter implementation

[Akhil Goyal <akhil.goyal@nxp.com>]

Hi Abhinandan/ Jerin,
On 4/18/2018 11:51 AM, Gujjar, Abhinandan S wrote:
> Hi Akhil,
>
> Please find the comments inline.
>
>> -----Original Message-----
>> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
>> Sent: Tuesday, April 17, 2018 7:48 PM
>> To: Gujjar, Abhinandan S <abhinandan.gujjar@intel.com>;
>> jerin.jacob@caviumnetworks.com; hemant.agrawal@nxp.com; dev@dpdk.org
>> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Doherty, Declan
>> <declan.doherty@intel.com>; Vangati, Narender
>> <narender.vangati@intel.com>; Rao, Nikhil <nikhil.rao@intel.com>; Eads, Gage
>> <gage.eads@intel.com>
>> Subject: Re: [dpdk-dev, v1, 2/5] eventdev: add crypto adapter implementation
>>
>> Hi Abhinandan,
>>
>> I have not reviewed the patch completely. But I have below query for further
>> review.
>> On 4/4/2018 12:26 PM, Abhinandan Gujjar wrote:
>>> Signed-off-by: Abhinandan Gujjar <abhinandan.gujjar@intel.com>
>>> Signed-off-by: Nikhil Rao <nikhil.rao@intel.com>
>>> Signed-off-by: Gage Eads <gage.eads@intel.com>
>>> ---
>>
>> [..snip..]
>>> +
>>> +int __rte_experimental
>>> +rte_event_crypto_adapter_queue_pair_add(uint8_t id,
>>> +					uint8_t cdev_id,
>>> +					int32_t queue_pair_id)
>>> +{
>>> +	struct rte_event_crypto_adapter *adapter;
>>> +	struct rte_eventdev *dev;
>>> +	struct crypto_device_info *dev_info;
>>> +	uint32_t cap;
>>> +	int ret;
>>> +
>>> +	RTE_EVENT_CRYPTO_ADAPTER_ID_VALID_OR_ERR_RET(id, -EINVAL);
>>> +
>>> +	if (!rte_cryptodev_pmd_is_valid_dev(cdev_id)) {
>>> +		RTE_EDEV_LOG_ERR("Invalid dev_id=%" PRIu8, cdev_id);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	adapter = eca_id_to_adapter(id);
>>> +	if (adapter == NULL)
>>> +		return -EINVAL;
>>> +
>>> +	dev = &rte_eventdevs[adapter->eventdev_id];
>>> +	ret = rte_event_crypto_adapter_caps_get(adapter->eventdev_id,
>>> +						cdev_id,
>>> +						&cap);
>>> +	if (ret) {
>>> +		RTE_EDEV_LOG_ERR("Failed to get adapter caps dev %" PRIu8
>>> +			"cdev %" PRIu8, id, cdev_id);
>>> +		return ret;
>>> +	}
>>> +
>>> +	dev_info = &adapter->cdevs[cdev_id];
>>> +
>>> +	if (queue_pair_id != -1 &&
>>> +	    (uint16_t)queue_pair_id >= dev_info->dev->data->nb_queue_pairs) {
>>> +		RTE_EDEV_LOG_ERR("Invalid queue_pair_id %" PRIu16,
>>> +				 (uint16_t)queue_pair_id);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (cap & RTE_EVENT_CRYPTO_ADAPTER_CAP_INTERNAL_PORT) {
>>> +		RTE_FUNC_PTR_OR_ERR_RET(
>>> +			*dev->dev_ops->crypto_adapter_queue_pair_add,
>>> +			-ENOTSUP);
>>> +		if (dev_info->qpairs == NULL) {
>>> +			dev_info->qpairs =
>>> +			    rte_zmalloc_socket(adapter->mem_name,
>>> +					dev_info->dev->data->nb_queue_pairs
>> *
>>> +					sizeof(struct crypto_queue_pair_info),
>>> +					0, adapter->socket_id);
>>> +			if (dev_info->qpairs == NULL)
>>> +				return -ENOMEM;
>>> +		}
>>> +
>>> +		ret = (*dev->dev_ops->crypto_adapter_queue_pair_add)(dev,
>>> +				dev_info->dev,
>>> +				queue_pair_id);
>>
>> crypto_adapter_queue_pair_add is supposed to attach a queue
>> (queue_pair_id) of cryptodev(dev_info->dev) to event device (dev).
>> But how will the underlying implementation attach it to event device without
>> knowing the eventdev queue_id. This information was coming in the RFC
>> patches with the parameter (rte_event_crypto_queue_pair_conf).
>> Why is this removed and if removed how will the driver attach the queue.
>> I can see that rte_event is passed in the session private data but how can we
>> attach the crypto queue with event dev queue?
>
> Yes, this was present in the first version of the RFC which is similar to eth rx adapter.
> After couple of discussions, thread http://dpdk.org/dev/patchwork/patch/31752/),
> it was changed. In eth rx adapter, eth queues are mapped to eventdev, whereas in crypto
> adapter the sessions are mapped to eventdev. Since event info is present along with the
> session, the get API has to be called in respective API to get the event information and
> then map to eventdev.
>

I think the intent of that discussion was misunderstood from our end.
But this is not going to work for hardware devices.

Because in case of hardware implementation, the scheduling is done in 
hardware and hardware cannot call the get API to get the event 
information then map to event device. Actually the scheduling has 
happened before the crypto_op is dequeued from the event port. So there 
is no point of set/get private data in our case.

We need to map the crypto queues to the event queue_ids at the time of 
queue_pair add API. In hardware scheduler, we map n(may be 1-8) crypto 
queues to m event queues(<= n). We can assign multiple sessions to any 
crypto queue pair, and after the crypto op is received by event queue, 
they are appropriately scheduled by hardware to event ports.

Session based mapping to event queue cannot be supported. Our design is 
same as that of eth rx adapter.

Akhil