2014-10-21 16:46, Jijiang Liu: > There are "some" destination UDP port numbers that have unque meaning. > In terms of VxLAN, "IANA has assigned the value 4789 for the VXLAN UDP port, > and this value > SHOULD be used by default as the destination UDP port. Some early > implementations of VXLAN > have used other values for the destination port. To enable interoperability > with these > implementations, the destination port SHOULD be configurable." > > Add two APIs in librte_ether for supporting UDP tunneling port configuration > on i40e. > Currently, only VxLAN is implemented in this patch set.
Actually, there are 2 different things in this patch - new tunnelling API - VXLAN macros Please split in 2 patches. > int > +rte_eth_dev_udp_tunnel_add(uint8_t port_id, > + struct rte_eth_udp_tunnel *udp_tunnel, > + uint8_t count) > +{ > + uint8_t i; > + struct rte_eth_dev *dev; > + struct rte_eth_udp_tunnel *tunnel; > + > + if (port_id >= nb_ports) { > + PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id); > + return -ENODEV; > + } > + > + if (udp_tunnel == NULL) { > + PMD_DEBUG_TRACE("Invalid udp_tunnel parameter\n"); > + return -EINVAL; > + } > + tunnel = udp_tunnel; > + > + for (i = 0; i < count; i++, tunnel++) { > + if (tunnel->prot_type >= RTE_TUNNEL_TYPE_MAX) { > + PMD_DEBUG_TRACE("Invalid tunnel type\n"); > + return -EINVAL; > + } > + } I'm not sure it's a good idea to provide a count parameter to iterate in a loop. It's probably something that the application should do by itself. > + > + dev = &rte_eth_devices[port_id]; > + FUNC_PTR_OR_ERR_RET(*dev->dev_ops->udp_tunnel_add, -ENOTSUP); > + return (*dev->dev_ops->udp_tunnel_add)(dev, udp_tunnel, count); > +} [...] > +/** > + * Tunneled type. > + */ > +enum rte_eth_tunnel_type { > + RTE_TUNNEL_TYPE_NONE = 0, > + RTE_TUNNEL_TYPE_VXLAN, > + RTE_TUNNEL_TYPE_GENEVE, > + RTE_TUNNEL_TYPE_TEREDO, > + RTE_TUNNEL_TYPE_NVGRE, > + RTE_TUNNEL_TYPE_MAX, > +}; This is moved later from rte_ethdev.h to rte_eth_ctrl.h. Please choose where is the right location in this patch. By the way, I think ethdev is the right location because it's not directly related to ctrl API, right? > struct rte_eth_conf { > + enum rte_eth_tunnel_type tunnel_type; > uint16_t link_speed; > /**< ETH_LINK_SPEED_10[0|00|000], or 0 for autonegotation */ > uint16_t link_duplex; Please don't add this field as the first. It's more logical to start port configuration with link speed, duplex, etc. Then please add a comment to explain how it should be used. But I doubt we should configure a tunnel type for a whole port. There's something weird here. > +/* VXLAN protocol header */ This comment should be doxygen'ed. > +struct vxlan_hdr { > + uint32_t vx_flags; /**< VxLAN flag. */ > + uint32_t vx_vni; /**< VxLAN ID. */ > +} __attribute__((__packed__)); > + [...] > #define ETHER_TYPE_VLAN 0x8100 /**< IEEE 802.1Q VLAN tagging. */ > #define ETHER_TYPE_1588 0x88F7 /**< IEEE 802.1AS 1588 Precise Time Protocol. > */ > > +#define ETHER_VXLAN_HLEN (sizeof(struct udp_hdr) + sizeof(struct vxlan_hdr)) Please add a doxygen comment. -- Thomas