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

[Gujjar, Abhinandan S]

Hi Akhil,

Thanks for pointing out the requirement from your side.
I will update you on this.

Regard
Abhinandan

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> Sent: Friday, April 20, 2018 5:05 PM
> To: Gujjar, Abhinandan S <abhinandan.gujjar at intel.com>;
> jerin.jacob at caviumnetworks.com; hemant.agrawal at nxp.com; dev at dpdk.org
> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; Doherty, Declan
> <declan.doherty at intel.com>; Vangati, Narender
> <narender.vangati at intel.com>; Rao, Nikhil <nikhil.rao at intel.com>; Eads, Gage
> <gage.eads at intel.com>
> Subject: Re: [dpdk-dev, v1, 2/5] eventdev: add crypto adapter implementation
> 
> 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 at nxp.com]
> >> Sent: Tuesday, April 17, 2018 7:48 PM
> >> To: Gujjar, Abhinandan S <abhinandan.gujjar at intel.com>;
> >> jerin.jacob at caviumnetworks.com; hemant.agrawal at nxp.com;
> dev at dpdk.org
> >> Cc: De Lara Guarch, Pablo <pablo.de.lara.guarch at intel.com>; Doherty,
> >> Declan <declan.doherty at intel.com>; Vangati, Narender
> >> <narender.vangati at intel.com>; Rao, Nikhil <nikhil.rao at intel.com>;
> >> Eads, Gage <gage.eads at 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 at intel.com>
> >>> Signed-off-by: Nikhil Rao <nikhil.rao at intel.com>
> >>> Signed-off-by: Gage Eads <gage.eads at 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