On Fri, Jun 9, 2017 at 7:32 AM, Savolainen, Petri (Nokia - FI/Espoo)
<[email protected]> wrote:
>> > + * - ODP_EVENT_PACKET_IPSEC
>> > + * - Packet event (odp_packet_t) genereted as a result of an IPsec
>>
>> generated
>
> Ok
>
>
>> > /**
>> > diff --git a/include/odp/api/spec/ipsec.h b/include/odp/api/spec/ipsec.h
>> > index 65f0b066..0da2b42c 100644
>> > --- a/include/odp/api/spec/ipsec.h
>> > +++ b/include/odp/api/spec/ipsec.h
>> > @@ -552,16 +552,18 @@ typedef enum odp_ipsec_frag_mode_t {
>> >
>> > /**
>> > * Packet lookup mode
>> > + *
>> > + * Lookup mode controls how an SA participates in SA lookup offload.
>> > + * Inbound operations perform SA lookup if application does not provide
>> a SA as
>> > + * a parameter. In inline mode, a lookup miss directs the packet back
>> to normal
>> > + * packet input interface processing. SA lookup failure status
>> (error.sa_lookup)
>> > + * is reported through odp_ipsec_packet_result_t.
>> > */
>> > typedef enum odp_ipsec_lookup_mode_t {
>> > - /** Inbound SA lookup is disabled. */
>> > + /** Inbound SA lookup is disabled for the SA. */
>> > ODP_IPSEC_LOOKUP_DISABLED = 0,
>> >
>> > - /** Inbound SA lookup is enabled. Lookup matches only SPI
>> value.
>> > - * In inline mode, a lookup miss directs the packet back to
>> normal
>> > - * packet input interface processing. In other modes, the SA
>> lookup
>> > - * failure status (error.sa_lookup) is reported through
>> > - * odp_ipsec_packet_result_t. */
>> > + /** Inbound SA lookup is enabled. Lookup matches only SPI
>> value. */
>> > ODP_IPSEC_LOOKUP_SPI,
>> >
>> > /** Inbound SA lookup is enabled. Lookup matches both SPI value
>> and
>>
>> This can go to separate patch.
>
>
> We updated the lookup offload text in odp_ipsec_in_param_t (0 == no SA, but
> lookup), which is a new type. This text pair with that and is merely moved
> from ODP_IPSEC_LOOKUP_SPI mode to up, since it's true for both lookup modes.
> So, patch is more complete with the move, than without it.
>
>
>
>>
>> > @@ -850,17 +852,6 @@ int odp_ipsec_sa_destroy(odp_ipsec_sa_t sa);
>> > */
>> > uint64_t odp_ipsec_sa_to_u64(odp_ipsec_sa_t sa);
>> >
>> > -/**
>> > - * IPSEC operation level options
>> > - *
>> > - * These may be used to override some SA level options
>> > - */
>> > -typedef struct odp_ipsec_op_opt_t {
>> > - /** Fragmentation mode */
>> > - odp_ipsec_frag_mode_t mode;
>> > -
>> > -} odp_ipsec_op_opt_t;
>> > -
>> > /** IPSEC operation status has no errors */
>> > #define ODP_IPSEC_OK 0
>> >
>> > @@ -870,7 +861,8 @@ typedef struct odp_ipsec_op_status_t {
>> > union {
>> > /** Error flags */
>> > struct {
>> > - /** Protocol error. Not a valid ESP or AH
>> packet. */
>> > + /** Protocol error. Not a valid ESP or AH
>> packet,
>> > + * packet data length error, etc. */
>> > uint32_t proto : 1;
>> >
>> > /** SA lookup failed */
>> > @@ -934,41 +926,70 @@ typedef struct odp_ipsec_op_status_t {
>> > } odp_ipsec_op_status_t;
>> >
>> > /**
>> > - * IPSEC operation input parameters
>> > + * IPSEC outbound operation options
>> > + *
>> > + * These may be used to override some SA level options
>> > */
>> > -typedef struct odp_ipsec_op_param_t {
>> > - /** Number of packets to be processed */
>> > - int num_pkt;
>> > +typedef struct odp_ipsec_out_opt_t {
>> > + /** Fragmentation mode */
>> > + odp_ipsec_frag_mode_t mode;
>> > +
>> > +} odp_ipsec_out_opt_t;
>>
>> Maybe we can just inline this into out_param_t ?
>> With this in/out split, it should be quite straightforward change from
>> the API point of view.
>
>
> The thing is that it's a pointer to an odp_ipsec_out_opt_t array, so
> application needs the type above. Otherwise, we'd need to define maximum of
> opts in the API, which would be the same as maximum number of packets. API
> doesn't need to limit the burst size.
>
>
>>
>> >
>> > +/**
>> > + * IPSEC outbound operation parameters
>> > + */
>> > +typedef struct odp_ipsec_out_param_t {
>> > /** Number of SAs
>> > *
>> > + * Outbound IPSEC operation needs SA from application. Use
>> either
>> > + * single SA for all packets, or a SA per packet.
>> > + *
>> > * Valid values are:
>> > - * * 0: No SAs (default)
>> > - * * 1: Single SA for all packets
>> > - * * num_pkt: SA per packet
>> > + * * 1: Single SA for all packets
>> > + * * N: A SA per packet. N must match the number of packets.
>> > */
>> > int num_sa;
>> >
>> > - /** Number of operation options
>> > + /** Number of outbound operation options
>> > *
>> > * Valid values are:
>> > - * * 0: No options (default)
>> > - * * 1: Single option for all packets
>> > - * * num_pkt: An option per packet
>> > + * * 0: No options
>> > + * * 1: Single option for all packets
>> > + * * N: An option per packet. N must match the number of
>> packets.
>> > */
>> > int num_opt;
>> >
>> > - /** Pointer to an array of packets
>> > + /** Pointer to an array of IPSEC SAs */
>> > + odp_ipsec_sa_t *sa;
>> > +
>> > + /** Pointer to an array of outbound operation options
>> > + *
>> > + * May be NULL when num_opt is zero.
>> > + */
>> > + odp_ipsec_out_opt_t *opt;
>> > +
>> > +} odp_ipsec_out_param_t;
>> > +
>> > +/**
>> > + * IPSEC inbound operation parameters
>> > + */
>> > +typedef struct odp_ipsec_in_param_t {
>> > + /** Number of SAs
>> > *
>> > - * Each packet must have a valid value for these metadata:
>> > - * * L3 offset: Offset to the first byte of the (outmost) IP
>> header
>> > - * * L4 offset: For inbound direction, when udp_encap is
>> enabled -
>> > - * offset to the first byte of the encapsulating
>> UDP
>> > - * header
>> > + * Inbound IPSEC operation processes a packet using the SA
>> provided by
>> > + * the application. If the application does not provide an SA,
>> the
>> > + * operation searches for the SA by matching the input packet
>> with all
>> > + * inbound SAs according to the lookup mode
>> (odp_ipsec_lookup_mode_t)
>> > + * configured in each SA. When passing SAs, use either single
>> SA for
>> > + * all packets, or a SA per packet.
>> > *
>> > - * @see odp_packet_l3_offset(), odp_packet_l4_offset()
>> > + * Valid values are:
>> > + * * 0: No SAs. SA lookup is done for all packets.
>> > + * * 1: Single SA for all packets
>> > + * * N: A SA per packet. N must match the number of packets.
>> > */
>> > - odp_packet_t *pkt;
>> > + int num_sa;
>> >
>> > /** Pointer to an array of IPSEC SAs
>> > *
>> > @@ -976,18 +997,12 @@ typedef struct odp_ipsec_op_param_t {
>> > */
>> > odp_ipsec_sa_t *sa;
>> >
>> > - /** Pointer to an array of operation options
>> > - *
>> > - * May be NULL when num_opt is zero.
>> > - */
>> > - odp_ipsec_op_opt_t *opt;
>> > -
>> > -} odp_ipsec_op_param_t;
>> > +} odp_ipsec_in_param_t;
>>
>> I like this idea, it will simplify code for both implementations and
>> applications. However can we move this op_param -> in_param+out_param to
>> a separate patch?
>
>
> We cannot since it's part of the function prototypes under.
>
> int odp_ipsec_in(const odp_packet_t pkt_in[], int num_in,
> odp_packet_t pkt_out[], int *num_out,
> const odp_ipsec_in_param_t *param);
>
>
>
>
>
>>
>> > /**
>> > * Outbound inline IPSEC operation parameters
>> > */
>> > -typedef struct odp_ipsec_inline_op_param_t {
>> > +typedef struct odp_ipsec_out_inline_param_t {
>> > /** Packet output interface for inline output operation
>> > *
>> > * Outbound inline IPSEC operation uses this packet IO
>> interface to
>> > @@ -1011,7 +1026,7 @@ typedef struct odp_ipsec_inline_op_param_t {
>> > uint32_t len;
>> > } outer_hdr;
>> >
>> > -} odp_ipsec_inline_op_param_t;
>> > +} odp_ipsec_out_inline_param_t;
>> >
>> > /**
>> > * IPSEC operation result for a packet
>> > @@ -1020,16 +1035,6 @@ typedef struct odp_ipsec_packet_result_t {
>> > /** IPSEC operation status */
>> > odp_ipsec_op_status_t status;
>> >
>> > - /** Number of output packets created from the corresponding
>> input packet
>> > - *
>> > - * Without fragmentation offload this is always one. However,
>> if the
>> > - * input packet was fragmented during the operation this is
>> larger than
>> > - * one for the first returned fragment and zero for the rest
>> of the
>> > - * fragments. All the fragments (of the same source packet)
>> are stored
>> > - * consecutively in the 'pkt' array.
>> > - */
>> > - int num_out;
>> > -
>> > /** IPSEC SA that was used to create the packet
>> > *
>> > * Operation updates this SA handle value, when SA look up is
>> performed
>> > @@ -1040,7 +1045,8 @@ typedef struct odp_ipsec_packet_result_t {
>> > odp_ipsec_sa_t sa;
>> >
>> > /** Packet outer header status before inbound inline
>> processing.
>> > - * This is valid only when status.flag.inline_mode is set.
>> > + * This is valid only when outer headers are retained
>> > + * (see odp_ipsec_inbound_config_t) and
>> status.flag.inline_mode is set.
>>
>> Related question: would we like to lift this restriction and enable
>> headers retaining for all kinds of inline processing?
>
>
> What means "all kind of inline processing"?
>
> The limitation here is that *non-inline* IPSEC operations does not need to
> support retaining, since application sees all those headers before calling
> IPSEC. Anyway, this can be discussed/expanded later.
>
>> > /**
>> > - * Get IPSEC results from an ODP_EVENT_IPSEC_RESULT event
>> > + * Convert IPSEC processed packet handle to event
>> > *
>> > - * Copies IPSEC operation results from an event. The event must be of
>> > - * type ODP_EVENT_IPSEC_RESULT. It must be freed before the application
>> passes
>> > - * any resulting packet handles to other ODP calls.
>> > + * The packet handle must be an output of an IPSEC operation.
>> > *
>> > - * @param[out] result Pointer to operation result for output. Maybe
>> NULL, if
>> > - * application is interested only on the number
>> of
>> > - * packets.
>> > - * @param event An ODP_EVENT_IPSEC_RESULT event
>> > + * @param pkt Packet handle from IPSEC operation
>> > *
>> > - * @return Number of packets in the event. If this is larger than
>> > - * 'result.num_pkt', all packets did not fit into result struct
>> and
>> > - * application must call the function again with a larger
>> result struct.
>> > + * @return Event handle
>> > + */
>> > +odp_event_t odp_ipsec_packet_to_event(odp_packet_t pkt);
>>
>> Shouldn't this be an internal API?
>
>
> No. This is the way an application can forward resulting packets as events
> (EVENT_PACKET_IPSEC) through queues to e.g. a next stage of application
> processing.
>
>
>> > /**
>> > * Get IPSEC status information from an ODP_EVENT_IPSEC_STATUS event
>> > diff --git a/include/odp/arch/default/api/abi/event.h
>> b/include/odp/arch/default/api/abi/event.h
>> > index 87220d63..d807425b 100644
>> > --- a/include/odp/arch/default/api/abi/event.h
>> > +++ b/include/odp/arch/default/api/abi/event.h
>> > @@ -27,9 +27,9 @@ typedef _odp_abi_event_t *odp_event_t;
>> > typedef enum odp_event_type_t {
>> > ODP_EVENT_BUFFER = 1,
>> > ODP_EVENT_PACKET = 2,
>> > - ODP_EVENT_TIMEOUT = 3,
>> > - ODP_EVENT_CRYPTO_COMPL = 4,
>> > - ODP_EVENT_IPSEC_RESULT = 5,
>> > + ODP_EVENT_PACKET_IPSEC = 3,
>> > + ODP_EVENT_TIMEOUT = 4,
>> > + ODP_EVENT_CRYPTO_COMPL = 5,
>> > ODP_EVENT_IPSEC_STATUS = 6
>> > } odp_event_type_t;
>>
>> Any reason for rearrangement of existing values?
>
> Yes, it's now
>
> ODP_EVENT_BUFFER = 1,
> ODP_EVENT_PACKET = 2,
> ODP_EVENT_PACKET_IPSEC = 3,
>
> which may turn in couple of weeks into
>
> ODP_EVENT_BUFFER = 1,
> ODP_EVENT_PACKET = 2,
> ODP_EVENT_PACKET_IPSEC = 3,
> ODP_EVENT_PACKET_CRYPTO = 4,
> ODP_EVENT_PACKET_COMP = 5,
>
> ... normal packet and special ones.
The problem with trying to to this as an ever-growing list of event
types, is that every time we introduce a new one every application
needs to change. ODP_EVENT_PACKET_IPSEC is simply an ODP_EVENT_PACKET
with some additional metadata that may or may not be of interest to a
given application processing stage. So the basic architecture should
reflect this fact.
The typical event loop for a worker thread would look like:
while (1) {
odp_event_t ev = odp_schedule(&queue, ODP_SCHED_WAIT);
switch (odp_event_type(ev)) {
case ODP_EVENT_BUFFER:
do_buffer_processing(odp_buffer_from_event(ev), queue);
break;
case ODP_EVENT_TIMEOUT:
do_timeout_processing(odp_timeout_from_event(ev), queue);
break;
case ODP_EVENT_PACKET:
do_packet_processing(odp_packet_from_event(ev), queue);
break;
...
default:
// Unknown event type, log and discard
}
}
So now that processing loop needs to be updated even if this worker
thread doesn't care about that optional IPsec metadata.
A more flexible approach would be to define a new odp_packet_type_t
and associated odp_packet_type() API that identifies these subtypes.
That way the worker packet processing routine can be written as:
void do_packet_processing(odp_packet_t pkt, odp_queue_t queue)
{
....
switch (odp_packet_type(pkt)) {
case ODP_PACKET_IPSEC:
// do processing of interest with associated
IPsec metadata
break;
case ODP_PACKET_COMP:
// do processing of interest with associated
COMP metadata
break;
default:
}
... process the basic packet itself
}
Note that this means that anytime we add a new packet subtype nothing
has to change at the application layer. It only needs to add code to
process that optional metadata in stages that specifically want to do
something with it. Otherwise they are treated as regular packets. So
the fact that this packet was IPsec decrypted before it got to this
stage is something the stage need not be aware of or care about unless
it wants to take special action because the packet arrived via that
offload path.
As we add more pipeline capabilities this sort of structure will
become increasingly important as the whole idea behind pipelining is
that worker threads don't need to be aware of where the packets came
from or their offload history in order to do what that stage needs to
do. Some stages will want to inspect that history and do something
different based on it, but most will not.
This becomes even more important when we consider pipeline chains. For
example, what happens when we support full offloading of packet that
is IPsec decrypted, then decompressed, and then handed to a worker?
Trying to encode these composites in a single event type quickly
becomes completely unmanageable. However, we can easily extend
odp_packet_type() to be:
odp_packet_type_t odp_packet_type(odp_packet_t pkt, int level);
So that odp_packet_type(pkt, 0) gives the current packet subtype,
odp_packet_type(pkt, 1) gives the previous, etc. That way processing
stages that need this sort of "forensic" information can get it if
needed, but other stages can ignore it. We may want to add the API:
int odp_packet_history(odp_packet_t pkt);
to return the number of such levels available, but this sort of
extension may be premature for now.
>
> -Petri
>
>
