Thanks Taras, all good feedback (and good eye catching the odp_buffer_t
versus odp_queue_t).  I've incorporated the changes, and changed the
TRUE/FALSE to:

#ifndef TRUE
#define TRUE  1
#endif
#ifndef FALSE
#define FALSE 0
#endif

-----Original Message-----
From: Taras Kondratiuk [mailto:[email protected]] 
Sent: Friday, August 22, 2014 4:52 AM
To: Robbie King (robking); [email protected]
Subject: Re: [lng-odp] [PATCHv2 4/4] Add IPsec example app to build environment

On 08/21/2014 10:10 PM, Robbie King wrote:
> diff --git a/example/ipsec/odp_ipsec.c b/example/ipsec/odp_ipsec.c
> new file mode 100644
> index 0000000..6cad92a
> --- /dev/null
> +++ b/example/ipsec/odp_ipsec.c

[...]

> +/**
> + * odp_schedule replacement to poll queues versus using ODP scheduler
> + */
> +static
> +odp_buffer_t polled_odp_schedule(odp_queue_t *from, uint64_t wait)
> +{
> +     uint64_t start_cycle, cycle, diff;
> +
> +     start_cycle = 0;
> +
> +     while (1) {
> +             int idx;
> +
> +             for (idx = 0; idx < num_polled_queues; idx++) {
> +                     odp_buffer_t queue = poll_queues[idx];

s/odp_buffer_t/odp_queue_t/

> +                     odp_buffer_t buf;
> +
> +                     buf = odp_queue_deq(queue);
> +
> +                     if (ODP_BUFFER_INVALID != buf) {
> +                             *from = queue;
> +                             return buf;
> +                     }
> +             }

[...]

> +/**
> + * IPsec post argument processing intialization
> + *
> + * Resolve SP DB with SA DB and create corresponding IPsec cache entries
> + *
> + * @param api_mode  Mode to use when invoking per packet crypto API
> + */
> +static
> +void ipsec_init_post(crypto_api_mode_e api_mode)
> +{
> +     sp_db_entry_t *entry;
> +
> +     /* Attempt to find appropriate SA for each SP */
> +     for (entry = sp_db->list; NULL != entry; entry = entry->next) {
> +             sa_db_entry_t *cipher_sa = NULL;
> +             sa_db_entry_t *auth_sa = NULL;
> +
> +             if (entry->esp)
> +                     cipher_sa = find_sa_db_entry(&entry->src_subnet,
> +                                                  &entry->dst_subnet,
> +                                                  1);
> +             if (entry->ah)
> +                     auth_sa = find_sa_db_entry(&entry->src_subnet,
> +                                                &entry->dst_subnet,
> +                                                0);
> +
> +             if (cipher_sa || auth_sa)
> +                     create_ipsec_cache_entry(cipher_sa,
> +                                              auth_sa,
> +                                              api_mode,
> +                                              entry->input,
> +                                              completionq,
> +                                              out_pool);

create_ipsec_cache_entry() may fail if session creation failed,
so entry won't be found during lookup. It worth to print some warning
here or exit application.

> +             else {
> +                     printf(" WARNING: SA not found for SP\n");
> +                     dump_sp_db_entry(entry);
> +             }
> +     }
> +}

[...]

> +/**
> + * Packet Processing - Input IPsec packet processing cleanup
> + *
> + * @param pkt  Packet to handle
> + * @param ctx  Packet process context
> + *
> + * @return PKT_CONTINUE if successful else PKT_DROP
> + */
> +static
> +pkt_disposition_e do_ipsec_in_finish(odp_packet_t pkt,
> +                                  pkt_ctx_t *ctx)
> +{
> +     odp_buffer_t event;
> +     odp_crypto_compl_status_t cipher_rc, auth_rc;
> +     odp_ipv4hdr_t *ip = (odp_ipv4hdr_t *)odp_packet_l3(pkt);

pkt is a 'completion event' but not yet a packet here. I'll try to
address it in my RFC, but for now I'm postprocessing 'completion event'
to convert it into packet in
odp_crypto_get_operation_compl_status(...). Could you please invoke

ip = (odp_ipv4hdr_t *)odp_packet_l3(pkt);

after odp_crypto_get_operation_compl_status(...). Just a few lines
below.

> +     int       hdr_len = ctx->ipsec.hdr_len;
> +     int       trl_len = 0;
> +
> +     /* Check crypto result */
> +     event = odp_buffer_from_packet(pkt);
> +     odp_crypto_get_operation_compl_status(event, &cipher_rc, &auth_rc);
> +     if (!is_crypto_compl_status_ok(&cipher_rc))
> +             return PKT_DROP;
> +     if (!is_crypto_compl_status_ok(&auth_rc))
> +             return PKT_DROP;

[...]

> +/**
> + * Packet Processing - Output IPsec packet processing cleanup
> + *
> + * @param pkt  Packet to handle
> + * @param ctx  Packet process context
> + *
> + * @return PKT_CONTINUE if successful else PKT_DROP
> + */
> +static
> +pkt_disposition_e do_ipsec_out_finish(odp_packet_t pkt,
> +                                   pkt_ctx_t *ctx)
> +{
> +     odp_buffer_t event;
> +     odp_crypto_compl_status_t cipher_rc, auth_rc;
> +     odp_ipv4hdr_t *ip = (odp_ipv4hdr_t *)odp_packet_l3(pkt);

The same request here. Move odp_packet_l3(pkt) after
odp_crypto_get_operation_compl_status().

> +
> +     /* Check crypto result */
> +     event = odp_buffer_from_packet(pkt);
> +     odp_crypto_get_operation_compl_status(event, &cipher_rc, &auth_rc);
> +     if (!is_crypto_compl_status_ok(&cipher_rc))
> +             return PKT_DROP;
> +     if (!is_crypto_compl_status_ok(&auth_rc))
> +             return PKT_DROP;

[...]

> diff --git a/example/ipsec/odp_ipsec_misc.h b/example/ipsec/odp_ipsec_misc.h
> new file mode 100644
> index 0000000..e5db950
> --- /dev/null
> +++ b/example/ipsec/odp_ipsec_misc.h
> @@ -0,0 +1,321 @@
> +/* Copyright (c) 2014, Linaro Limited
> + * All rights reserved.
> + *
> + * SPDX-License-Identifier:     BSD-3-Clause
> + */
> +
> +#ifndef ODP_IPSEC_MISC_H_
> +#define ODP_IPSEC_MISC_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <odp.h>
> +#include <helper/odp_ip.h>
> +#include <helper/odp_ipsec.h>
> +
> +#define TRUE  1
> +#define FALSE 0

My SDK also defines TRUE/FALSE and it confilicts here.

Could you wrap it with
#ifndef TRUE
#define TRUE  1
#define FALSE 0
#endif

Or should we define a ODP_TRUE/ODP_FALSE in some header?
_______________________________________________
lng-odp mailing list
[email protected]
http://lists.linaro.org/mailman/listinfo/lng-odp

Reply via email to