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

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Fri, 20 Apr 2018 17:04:36 +0530
> From: Akhil Goyal <akhil.goyal@nxp.com>
> To: "Gujjar, Abhinandan S" <abhinandan.gujjar@intel.com>,
>  "jerin.jacob@caviumnetworks.com" <jerin.jacob@caviumnetworks.com>,
>  "hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>, "dev@dpdk.org"
>  <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
> User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101
>  Thunderbird/45.8.0
> 
> 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.

Crypto queue pair to eventdev queue mapping should be supported. But
That's a limited set. meaning if an application needs millions of IPSec SA sessions
then we can not map it. So, IMO, If an HW/SW can not support session
based mapping then it needs to be exposed/abstracted through capabilities.

crypto qp to event queue mapping will be supported in all adapter
implementation.

Does that sounds OK?

> 
> Akhil
>