Re: [dpdk-dev] [PATCH v2] ip_pipeline: add QinQ PPPoE encapsulation action

["Dumitrescu, Cristian" <cristian.dumitrescu@intel.com>]

Hi Nemanja,

> -----Original Message-----
> From: Marjanovic, Nemanja
> Sent: Monday, January 14, 2019 10:23 AM
> To: dev@dpdk.org
> Cc: Singh, Jasvinder <jasvinder.singh@intel.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; Marjanovic, Nemanja
> <nemanja.marjanovic@intel.com>
> Subject: [PATCH v2] ip_pipeline: add QinQ PPPoE encapsulation action
> 
> Add implementation of QinQ PPPoE packet encapsulation action.
> 
> Signed-off-by: Nemanja Marjanovic <nemanja.marjanovic@intel.com>
> ---
> v2:Fix changes from check patch
> ---
>  examples/ip_pipeline/cli.c             | 43 +++++++++++++++++-
>  lib/librte_net/rte_ether.h             |  1 +
>  lib/librte_pipeline/rte_table_action.c | 61 ++++++++++++++++++++++++++
>  lib/librte_pipeline/rte_table_action.h | 16 +++++++
>  4 files changed, 119 insertions(+), 2 deletions(-)
> 
> diff --git a/examples/ip_pipeline/cli.c b/examples/ip_pipeline/cli.c
> index 910386282..dcc98ee65 100644
> --- a/examples/ip_pipeline/cli.c
> +++ b/examples/ip_pipeline/cli.c

We need to add support for this new packet encapsulation in the Soft NIC CLI, we don't want it to get out of sync with IP pipeline.
Basically, add essentially the same CLI code in drivers/net/softnic/rte_eth_softnic_cli.c, can you please do this?

> @@ -1290,11 +1290,13 @@ cmd_table_action_profile(char **tokens,
> 
>  			p.encap.encap_mask = 1LLU <<
> RTE_TABLE_ACTION_ENCAP_VXLAN;
>  			n_extra_tokens = 5;
> -		} else {
> +		} else if (strcmp(tokens[t0 + 1], "qinq_pppoe") == 0)
> +			p.encap.encap_mask =
> +				1LLU <<
> RTE_TABLE_ACTION_ENCAP_QINQ_PPPOE;
> +		else {
>  			snprintf(out, out_size, MSG_ARG_MISMATCH,
> "encap");
>  			return;
>  		}
> -

Please put back this blank line.

>  		p.action_mask |= 1LLU << RTE_TABLE_ACTION_ENCAP;
>  		t0 += 2 + n_extra_tokens;
>  	} /* encap */
> @@ -3391,6 +3393,43 @@ parse_table_action_encap(char **tokens,
>  		return 1 + 9;
>  	}
> 
> +	/* qinq_pppoe */
> +		if (n_tokens && (strcmp(tokens[0], "qinq_pppoe") == 0)) {
> +			uint32_t svlan_pcp, svlan_dei, svlan_vid;
> +			uint32_t cvlan_pcp, cvlan_dei, cvlan_vid;
> +
> +		if ((n_tokens < 10) ||
> +			parse_mac_addr(tokens[1],
> +				&a->encap.qinq_pppoe.ether.da) ||
> +			parse_mac_addr(tokens[2],
> +				&a->encap.qinq_pppoe.ether.sa) ||
> +			parser_read_uint32(&svlan_pcp, tokens[3]) ||
> +			(svlan_pcp > 0x7) ||
> +			parser_read_uint32(&svlan_dei, tokens[4]) ||
> +			(svlan_dei > 0x1) ||
> +			parser_read_uint32(&svlan_vid, tokens[5]) ||
> +			(svlan_vid > 0xFFF) ||
> +			parser_read_uint32(&cvlan_pcp, tokens[6]) ||
> +			(cvlan_pcp > 0x7) ||
> +			parser_read_uint32(&cvlan_dei, tokens[7]) ||
> +			(cvlan_dei > 0x1) ||
> +			parser_read_uint32(&cvlan_vid, tokens[8]) ||
> +			(cvlan_vid > 0xFFF) ||
> +			parser_read_uint16(&a-
> >encap.qinq_pppoe.pppoe.session_id,
> +				tokens[9]))
> +			return 0;
> +
> +		a->encap.qinq_pppoe.svlan.pcp = svlan_pcp & 0x7;
> +		a->encap.qinq_pppoe.svlan.dei = svlan_dei & 0x1;
> +		a->encap.qinq_pppoe.svlan.vid = svlan_vid & 0xFFF;
> +		a->encap.qinq_pppoe.cvlan.pcp = cvlan_pcp & 0x7;
> +		a->encap.qinq_pppoe.cvlan.dei = cvlan_dei & 0x1;
> +		a->encap.qinq_pppoe.cvlan.vid = cvlan_vid & 0xFFF;
> +		a->encap.type = RTE_TABLE_ACTION_ENCAP_QINQ_PPPOE;
> +		a->action_mask |= 1 << RTE_TABLE_ACTION_ENCAP;
> +		return 1 + 10;
> +		}
> +

Please add this new encap in the cmd_table_action_profile_help string, as well in the comment describing the table action options (search for "table_action ::= ") in this file.

>  	/* mpls */
>  	if (n_tokens && (strcmp(tokens[0], "mpls") == 0)) {
>  		uint32_t label, tc, ttl;
> diff --git a/lib/librte_net/rte_ether.h b/lib/librte_net/rte_ether.h
> index c2c5e249f..212ead64a 100644
> --- a/lib/librte_net/rte_ether.h
> +++ b/lib/librte_net/rte_ether.h
> @@ -300,6 +300,7 @@ struct vxlan_hdr {
>  #define ETHER_TYPE_ARP  0x0806 /**< Arp Protocol. */
>  #define ETHER_TYPE_RARP 0x8035 /**< Reverse Arp Protocol. */
>  #define ETHER_TYPE_VLAN 0x8100 /**< IEEE 802.1Q VLAN tagging. */
> +#define ETHER_TYPE_PPPoE 0x8864 /**< PPPoE Session Stage */

Please add both PPPoE session and PPPoE discovery ethertypes, and call them accordingly. Please keep the existing naming convention, i.e. keep names all upper case for the purpose of these macros: PPPOE_DISCOVERY and PPPOE_SESSION (as opposed to lover case "o" in PPPoE).

>  #define ETHER_TYPE_QINQ 0x88A8 /**< IEEE 802.1ad QinQ tagging. */
>  #define ETHER_TYPE_ETAG 0x893F /**< IEEE 802.1BR E-Tag. */
>  #define ETHER_TYPE_1588 0x88F7 /**< IEEE 802.1AS 1588 Precise Time
> Protocol. */
> diff --git a/lib/librte_pipeline/rte_table_action.c
> b/lib/librte_pipeline/rte_table_action.c
> index 7c7c8dd82..78cc7a7a2 100644
> --- a/lib/librte_pipeline/rte_table_action.c
> +++ b/lib/librte_pipeline/rte_table_action.c
> @@ -432,6 +432,7 @@ encap_valid(enum rte_table_action_encap_type
> encap)
>  	case RTE_TABLE_ACTION_ENCAP_MPLS:
>  	case RTE_TABLE_ACTION_ENCAP_PPPOE:
>  	case RTE_TABLE_ACTION_ENCAP_VXLAN:
> +	case RTE_TABLE_ACTION_ENCAP_QINQ_PPPOE:
>  		return 1;
>  	default:
>  		return 0;
> @@ -532,6 +533,13 @@ struct encap_vxlan_ipv6_vlan_data {
>  	struct vxlan_hdr vxlan;
>  } __attribute__((__packed__));
> 
> +struct encap_qinq_pppoe_data {
> +	struct ether_hdr ether;
> +	struct vlan_hdr svlan;
> +	struct vlan_hdr cvlan;
> +	struct pppoe_ppp_hdr pppoe_ppp;
> +} __attribute__((__packed__));
> +
>  static size_t
>  encap_data_size(struct rte_table_action_encap_config *encap)
>  {
> @@ -563,6 +571,9 @@ encap_data_size(struct
> rte_table_action_encap_config *encap)
>  			else
>  				return sizeof(struct encap_vxlan_ipv6_data);
> 
> +	case 1LLU << RTE_TABLE_ACTION_ENCAP_QINQ_PPPOE:
> +			return sizeof(struct encap_qinq_pppoe_data);
> +
>  	default:
>  		return 0;
>  	}
> @@ -599,6 +610,9 @@ encap_apply_check(struct
> rte_table_action_encap_params *p,
>  	case RTE_TABLE_ACTION_ENCAP_VXLAN:
>  		return 0;
> 
> +	case RTE_TABLE_ACTION_ENCAP_QINQ_PPPOE:
> +		return 0;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -676,6 +690,38 @@ encap_qinq_apply(void *data,
>  	return 0;
>  }
> 
> +static int
> +encap_qinq_pppoe_apply(void *data,
> +	struct rte_table_action_encap_params *p)
> +{
> +	struct encap_qinq_pppoe_data *d = data;
> +
> +	/* Ethernet */
> +	ether_addr_copy(&p->qinq.ether.da, &d->ether.d_addr);
> +	ether_addr_copy(&p->qinq.ether.sa, &d->ether.s_addr);
> +	d->ether.ether_type = rte_htons(ETHER_TYPE_VLAN);
> +
> +	/* SVLAN */
> +	d->svlan.vlan_tci = rte_htons(VLAN(p->qinq.svlan.pcp,
> +		p->qinq.svlan.dei,
> +		p->qinq.svlan.vid));
> +	d->svlan.eth_proto = rte_htons(ETHER_TYPE_VLAN);
> +
> +	/* CVLAN */
> +	d->cvlan.vlan_tci = rte_htons(VLAN(p->qinq.cvlan.pcp,
> +		p->qinq.cvlan.dei,
> +		p->qinq.cvlan.vid));
> +	d->cvlan.eth_proto = rte_htons(ETHER_TYPE_PPPoE);
> +
> +	/* PPPoE and PPP*/
> +	d->pppoe_ppp.ver_type_code = rte_htons(0x1100);
> +	d->pppoe_ppp.session_id = rte_htons(p-
> >qinq_pppoe.pppoe.session_id);
> +	d->pppoe_ppp.length = 0; /* not pre-computed */
> +	d->pppoe_ppp.protocol = rte_htons(PPP_PROTOCOL_IP);
> +
> +	return 0;
> +}
> +
>  static int
>  encap_mpls_apply(void *data,
>  	struct rte_table_action_encap_params *p)
> @@ -921,6 +967,9 @@ encap_apply(void *data,
>  	case RTE_TABLE_ACTION_ENCAP_VXLAN:
>  		return encap_vxlan_apply(data, p, cfg);
> 
> +	case RTE_TABLE_ACTION_ENCAP_QINQ_PPPOE:
> +		return encap_qinq_pppoe_apply(data, p);
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1119,6 +1168,18 @@ pkt_work_encap(struct rte_mbuf *mbuf,
>  		break;
>  	}
> 
> +	case 1LLU << RTE_TABLE_ACTION_ENCAP_QINQ_PPPOE:
> +	{
> +		struct encap_qinq_pppoe_data *qinq_pppoe =
> +			encap(ip, data, sizeof(struct
> encap_qinq_pppoe_data));
> +		qinq_pppoe->pppoe_ppp.length = rte_htons(total_length +
> 2);
> +		mbuf->data_off = ip_offset - (sizeof(struct rte_mbuf) +
> +			sizeof(struct encap_qinq_pppoe_data));
> +		mbuf->pkt_len = mbuf->data_len = total_length +
> +			sizeof(struct encap_qinq_pppoe_data);
> +		break;
> +	}
> +
>  	case 1LLU << RTE_TABLE_ACTION_ENCAP_VXLAN:
>  	{
>  		if (cfg->vxlan.ip_version)
> diff --git a/lib/librte_pipeline/rte_table_action.h
> b/lib/librte_pipeline/rte_table_action.h
> index c96061291..53d16af8a 100644
> --- a/lib/librte_pipeline/rte_table_action.h
> +++ b/lib/librte_pipeline/rte_table_action.h
> @@ -380,6 +380,9 @@ enum rte_table_action_encap_type {
>  	 * Ether -> { Ether | VLAN | IP | UDP | VXLAN | Ether }
>  	 */
>  	RTE_TABLE_ACTION_ENCAP_VXLAN,
> +
> +	/** IP -> { Ether | S-VLAN | C-VLAN | PPPoE | PPP | IP } */
> +	RTE_TABLE_ACTION_ENCAP_QINQ_PPPOE,
>  };
> 
>  /** Pre-computed Ethernet header fields for encapsulation action. */
> @@ -529,6 +532,16 @@ struct rte_table_action_encap_config {
>  	};
>  };
> 
> +/** QinQ_PPPoE encap paramaeters. */
> +struct rte_table_encap_ether_qinq_pppoe {
> +
> +	/** Only valid when *type* is set to QinQ. */
> +	struct rte_table_action_ether_hdr ether;
> +	struct rte_table_action_vlan_hdr svlan; /**< Service VLAN header.
> */
> +	struct rte_table_action_vlan_hdr cvlan; /**< Customer VLAN
> header. */
> +	struct rte_table_action_pppoe_hdr pppoe; /**< PPPoE/PPP
> headers. */
> +};
> +
>  /** Encap action parameters (per table rule). */
>  struct rte_table_action_encap_params {
>  	/** Encapsulation type. */
> @@ -553,6 +566,9 @@ struct rte_table_action_encap_params {
> 
>  		/** Only valid when *type* is set to VXLAN. */
>  		struct rte_table_action_encap_vxlan_params vxlan;
> +
> +		/** Only valid when *type* is set to QinQ_PPPoE. */
> +		struct rte_table_encap_ether_qinq_pppoe qinq_pppoe;
>  	};
>  };
> 
> --
> 2.17.1

Thanks,
Cristian