Comments below.
> -----Original Message-----
> From: lng-odp [mailto:lng-odp-boun...@lists.linaro.org] On Behalf Of Bala
> Manoharan
> Sent: Wednesday, November 16, 2016 11:16 AM
> To: Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia-bell-labs.com>
> Cc: lng-odp-forward <lng-odp@lists.linaro.org>
> Subject: Re: [lng-odp] [API-NEXT DRAFT] api: ipsec: added IPSEC API
>
> Regards,
> Bala
>
>
> On 15 November 2016 at 20:17, Savolainen, Petri (Nokia - FI/Espoo)
> <petri.savolai...@nokia-bell-labs.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Bala Manoharan [mailto:bala.manoha...@linaro.org]
> >> Sent: Tuesday, November 15, 2016 4:15 PM
> >> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia-bell-
> >> labs.com>
> >> Cc: lng-odp-forward <lng-odp@lists.linaro.org>
> >> Subject: Re: [lng-odp] [API-NEXT DRAFT] api: ipsec: added IPSEC API
> >>
> >> Regards,
> >> Bala
> >>
> >>
> >> On 15 November 2016 at 14:17, Petri Savolainen
> >> <petri.savolai...@nokia.com> wrote:
> >> > Added definitions for a look-a-side IPSEC offload API. In addition to
> >> > IPSEC packet transformations, it also supports:
> >> > * inbound SA look up
> >> > * outbound IP fragmentation
> >> >
> >> > Signed-off-by: Petri Savolainen <petri.savolai...@nokia.com>
> >> > ---
> >> > include/odp/api/spec/event.h | 2 +-
> >> > include/odp/api/spec/ipsec.h | 720
> >> +++++++++++++++++++++
> >> > include/odp_api.h | 1 +
> >> > platform/Makefile.inc | 1 +
> >> > platform/linux-generic/Makefile.am | 2 +
> >> > platform/linux-generic/include/odp/api/ipsec.h | 36 ++
> >> > .../include/odp/api/plat/event_types.h | 1 +
> >> > .../include/odp/api/plat/ipsec_types.h | 39 ++
> >> > 8 files changed, 801 insertions(+), 1 deletion(-)
> >> > create mode 100644 include/odp/api/spec/ipsec.h
> >> > create mode 100644 platform/linux-generic/include/odp/api/ipsec.h
> >> > create mode 100644 platform/linux-
> >> generic/include/odp/api/plat/ipsec_types.h
> >> >
> >> > diff --git a/include/odp/api/spec/event.h b/include/odp/api/spec/event.h
> >> > index fdfa52d..75c0bbc 100644
> >> > --- a/include/odp/api/spec/event.h
> >> > +++ b/include/odp/api/spec/event.h
> >> > @@ -39,7 +39,7 @@ extern "C" {
> >> > * @typedef odp_event_type_t
> >> > * ODP event types:
> >> > * ODP_EVENT_BUFFER, ODP_EVENT_PACKET, ODP_EVENT_TIMEOUT,
> >> > - * ODP_EVENT_CRYPTO_COMPL
> >> > + * ODP_EVENT_CRYPTO_COMPL, ODP_EVENT_IPSEC_RESULT
> >> > */
> >> >
> >
> >
> >> > +
> >> > + /** Destination queue for IPSEC events
> >> > + *
> >> > + * Operations in asynchronous mode enqueue resulting events
> >> into
> >> > + * this queue.
> >> > + */
> >> > + odp_queue_t dest_queue;
> >> > +
> >> > + /** User defined SA context pointer
> >> > + *
> >> > + * User defined context pointer associated with the SA.
> >> > + * The implementation may prefetch the context data. Default
> >> value
> >> > + * of the pointer is NULL.
> >> > + */
> >> > + void *context;
> >> > +
> >> > + /** Context data length
> >> > + *
> >> > + * User defined context data length in bytes for prefetching.
> >> > + * The implementation may use this value as a hint for the
> >> number of
> >> > + * context data bytes to prefetch. Default value is zero (no
> >> hint).
> >> > + */
> >> > + uint32_t context_len;
> >> > +
> >> > +} odp_ipsec_sa_param_t;
> >>
> >> What will be the total number of SA in the system? Will it be around 100K?
> >> If that is the case IMO it might be better to add a "context_packet"
> >> which could be copied to the output packet instead of having 100K
> >> different queues?
> >
> > Yes, number of SAs maybe large.
> >
> > Are you asking why dest_queue or context ptr is on SA level ?
> >
> > Some speculative answers:
> > * crypto API has a queue per session (== IPSEC SA if application is IPSEC)
> > * Application may choose to use one ODP queue for all results, or one per
> > SA, or anything in
> between
> > * In sync mode there would not be queues and no queue context, so only
> > place for application's
> SA context pointer is this struct
> > * Packet context ptr is specified to be copied from input packets to output
> > packets.
> Application could use it for SA context. We could remove SA specific context
> pointer if
> application can always use packet context pointer for SA (and does not need
> it for anything
> else)
>
> Packet context ptr will not work in the case in which the packet is
> directly received from the interface by the offload engine.
> Is the above case valid or all the packet comes only through the application?
In this API draft the packets come only through the application,
but I think SA context is still quite useful in that case too.
I think from application point of view an SA context is always
needed, regardless of whether it is explicit in the API or buried
in the packet context by the application.
> In-case the above scenario is valid, my suggestion is to have a
> context_ptr per SA so that all packets belonging to a single SA will
> have the same context_ptr (implementation will update the ctx_ptr
> field of the output packet with the "ctx_packet_ptr" given in the SA)
> so it will be easier for application to maintain context per SA and
> there will not be a need to create one queue per SA.
There is a user defined SA context pointer in the SA in this API
draft and I think the intention was to provide it back to the
application as part of odp_ipsec_packet_result_t. But maybe there
is an error there as there does not seem to be any way to actually
get the context pointer after SA creation.
Now the packet result struct has the SA handle, which alone is
not very useful to the application. I think the API draft
should either include the user defined SA context pointer in
odp_ipsec_packet_result_t (and maybe also remove the plain SA
handle), or provide a way to get the SA context through an
SA handle. I would prefer the former.
>
> >
> >
> >>
> >> > +
> >> > +/**
> >> > + * Initialize IPSEC SA parameters
> >> > + *
> >> > + * Initialize an odp_ipsec_sa_param_t to its default values for all
> >> fields.
> >> > + *
> >> > + * @param param Pointer to the parameter structure
> >> > + */
> >> > +void odp_ipsec_sa_param_init(odp_ipsec_sa_param_t *param);
> >> > +
> >> > +/**
> >> > + * Create IPSEC SA
> >> > + *
> >> > + * Create a new IPSEC SA according to the parameters.
> >> > + *
> >> > + * @param param IPSEC SA parameters
> >> > + *
> >> > + * @return IPSEC SA handle
> >> > + * @retval ODP_IPSEC_SA_INVALID on failure
> >> > + */
> >> > +odp_ipsec_sa_t odp_ipsec_sa_create(odp_ipsec_sa_param_t *param);
> >>
> >> How do you update the SA associated once the policy is expired? Or is
> >> it not in the scope of this RFC?
> >
> > Create another SA, run old and new SA for a while in parallel, destroy old
> > one when sure that
> it's not needed any more.
>
> When you say run in parallel do you mean there will be two SA attached
> to a single Policy?
> So who destroys the old SA? IMO the implementation should destroy the
> old SA when hard limit expires and send notification to the
> application.
There are no concept of policies in this API draft, just SAs. The
application decides how it keeps track of SAs and how it maps them
to policies.
Anyway, the application is supposed to destroy the old SA, but if it
does not do so before the hard limit expires, then all IPsec operations
that attempt to use the old SA would just fail.
Janne
