Re: [dpdk-dev] [PATCH 2/2] lib/security: add SA lifetime configuration

[Anoob Joseph <anoobj@marvell.com>]

Hi Konstantin,

> Subject: [EXT] RE: [PATCH 2/2] lib/security: add SA lifetime configuration
> 
> Hi Anoob,
> 
> > > > Now that we have an agreement on bitfields (hoping no one else has
> > > > an objection), I would like to discuss one more topic. It is more
> > > > related to
> > > checksum offload, but it's better that we discuss along with other
> > > similar items (like soft expiry).
> > > >
> > > > L3 & L4 checksum can be tristate (CSUM_OK, CSUM_ERROR,
> > > CSUM_UNKOWN)
> > > >
> > > > 1. Application didn't request. Nothing computed.
> > > > 2. Application requested. Checksum verification success.
> > > > 3. Application requested. Checksum verification failed.
> > > > 4. Application requested. Checksum could not be computed (PMD
> > > limitations etc).
> > > >
> > > > How would we indicate each case?
> > > >
> > > > My proposal would be, let's call the field that we called
> > > > "warning" as
> > > "aux_flags" (auxiliary or secondary information from the operation).
> > > >
> > > > Sequence in the application would be,
> > > >
> > > > if (op.status != SUCCESS) {
> > > >     /* handle errors */
> > > > }
> > > >
> > > > #define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED (1
> << 0)
> > > #define
> > > > RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD (1 << 1)
> > > >
> > > > if (op.aux_flags &
> > > RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED) {
> > > > 	if (op.aux_flags &
> > > RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD)
> > > > 		mbuf->l4_checksum_good = 1;
> > > > 	else
> > > > 		mbuf->l4_checksum_good = 0;
> > > > } else {
> > > > 	if (verify_l4_checksum(mbuf) == SUCCESS) {
> > > > 		mbuf->l4_checksum_good = 1;
> > > > 	else
> > > > 		mbuf->l4_checksum_good = 0;
> > > > }
> > > >
> > > > For an application not worried about aux_flags (ex: ipsec-secgw),
> > > > additional checks are not required. For applications not
> > > > interested in checksum, a blind check on op.aux_flags would be enough
> to bail out early.
> > > For applications interested in checksum, it can follow above
> > > sequence (kinds, for demonstration purpose only).
> > > >
> > > > Would something like above fine? Or if we want to restrict
> > > > additional fields for just warnings, (L4_CHECKSUM_ERROR), how
> > > > would application differentiate between checksum good & checksum
> > > > not computed? In that
> > > case, what should be PMDs treatment of "could not compute" v/s
> > > "computed and wrong".
> > >
> > > I am ok with what you suggest.
> > > My only thought - we already have CSUM flags in mbuf itself, so why
> > > not to use them instead to pass this information from crypto PMD to
> user?
> > > That way it would be compliant with ethdev CSUM approach and no need
> > > to spend
> > > 2 bits in 'aux_flags'.
> > > Konstantin
> >
> > [Anoob] You are right. We do have CSUM flags in mbuf and that would fully
> suite our requirement here.
> >
> > Our problem was, it's called PKT_RX_ and the description text refers to RX.
> >
> > /**
> >  * Mask of bits used to determine the status of RX IP checksum.
> >  * - PKT_RX_IP_CKSUM_UNKNOWN: no information about the RX IP
> checksum
> >  * - PKT_RX_IP_CKSUM_BAD: the IP checksum in the packet is wrong
> >  * - PKT_RX_IP_CKSUM_GOOD: the IP checksum in the packet is valid
> >  * - PKT_RX_IP_CKSUM_NONE: the IP checksum is not correct in the packet
> >  *   data, but the integrity of the IP header is verified.
> >  */
> >
> > But if we overlook that (& may be update documentation), it's a rather
> > great idea. We could use similar PKT_TX_* flags for requesting checksum
> generation with outbound operations (checksum generation for plain packet
> before IPsec processing).
> >
> > /**
> >  * Offload the IP checksum in the hardware. The flag PKT_TX_IPV4
> > should
> >  * also be set by the application, although a PMD will only check
> >  * PKT_TX_IP_CKSUM.
> >  *  - fill the mbuf offload information: l2_len, l3_len  */
> > #define PKT_TX_IP_CKSUM      (1ULL << 54)
> >
> > /**
> >  * Packet is IPv4. This flag must be set when using any offload
> > feature
> >  * (TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
> >  * packet. If the packet is a tunneled packet, this flag is related to
> >  * the inner headers.
> >  */
> > #define PKT_TX_IPV4          (1ULL << 55)
> >
> > Do you think above might require some modifications to document
> behavior with lookaside IPsec?
> >
> > Also, these flags are probably the best way for checksum for inner
> > packet with inline IPsec. So this looks like overall better idea. Do you agree?
> 
> Not sure I understand your proposal fully.
> Yes, right now inside mbuf we have different set of flags for checksum
> offloads: RX and TX.
> RX flags - indicate was checksum calculated/checked for incoming packet and
> what was the result, While TX flags define which CSUM calculations have to
> be done by HW.
> Yes, I suppose same flags can be reused by crypto-dev, if it capable to
> implement these HW offloads.
> Though not sure what changes do you think will be required inside mbuf?

[Anoob] My concern was regarding "RX" & "TX" in the comments, which are more applicable for ethdev than cryptodev. But then, I guess it's fine this way itself.

Will submit a new version for all the proposals with some unit tests so that we can discuss if there is any ambiguity in the usage.

Appreciate your feedback and suggestions.

Thanks,
Anoob