Petri Savolainen(psavol) replied on github web page: include/odp/api/spec/packet.h @@ -1140,6 +1179,82 @@ int odp_packet_move_data(odp_packet_t pkt, uint32_t dst_offset, */ /** + * Packet parse parameters + */ +typedef struct odp_packet_parse_param_t { + /** Protocol header at parse starting point. Valid values for this + * field are: ODP_PROTO_ETH, ODP_PROTO_IPV4, ODP_PROTO_IPV6. */ + odp_proto_t proto; + + /** Continue parsing until this layer. Must be the same or higher + * layer than the layer of 'proto'. */ + odp_proto_layer_t layer; + + /** Flags to control payload data checks up to the selected parse + * layer. Checksum checking status can be queried for each packet with + * odp_packet_l3_chksum_status() and odp_packet_l4_chksum_status(). + */ + union { + struct { + /** Check IPv4 header checksum */ + uint32_t ipv4_chksum : 1; + + /** Check UDP checksum */ + uint32_t udp_chksum : 1; + + /** Check TCP checksum */ + uint32_t tcp_chksum : 1; + + } check; + + /** All check bits. This can be used to set/clear all flags. */ + uint32_t all_check; + }; + +} odp_packet_parse_param_t; + +/** + * Parse packet + * + * Parse protocol headers in packet data. Parsing starts at 'offset', which + * is the first header byte of protocol 'param.proto'. Parameter 'param.layer' + * defines the last layer application is interested about. + * Use ODP_PROTO_LAYER_ALL for all layers. The operation sets or resets packet + * metadata for all layers from the layer of 'param.proto' to the application + * defined last layer. Metadata of other layers have undefined values. + * When operation fails, metadata of all protocol layers have undefined values.
Comment: Fail means an operation error (bad param, internal error), not e.g. error in packet header formats. > Petri Savolainen(psavol) wrote: > Application calls parse because metadata is not valid to begin with. There > would not be much benefit for application to require that implementation > saves old metadata values before the operation and then restores those after > if e.g. an internal error occurred. Implementation performance would be hurt > by making a copy of the old metadata. > > Since it's just (L2/L3/L4) metadata (not data) that may have changed, > application can continue processing the packet data also after a failed > parse. >> Dmitry Eremin-Solenikov(lumag) wrote: >> @psavol ok, agreed >>> Petri Savolainen(psavol) wrote: >>> This function is different from e.g. enqueue multi, where the destination >>> queue may be close to full and that's why only part of events were >>> enqueued. Here operation fails due to bad params or internal error. >>> >>> Anyway, I'll change it to return num processed. It's more flexible. >>>> bogdanPricope wrote >>>> What about: "When operation fails, metadata of checked layers indicates >>>> the error condition."? >>>>> bogdanPricope wrote >>>>> What about: "Metadata of other layers remains unmodified."? >>>>>> Petri Savolainen(psavol) wrote: >>>>>> Layer is not enough information for the parser. It needs to know the >>>>>> first protocol (e.g. MPLS vs IP) to be able to start the parsing. >>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>> @psavol `_param_init` seems like an overkill in this case. From my >>>>>>> point of view, `param_init` are good for object creation params (where >>>>>>> we might not know platform-optimized defaults) but are an overkill for >>>>>>> operation functions (we do not have `param_init` for ipsec, crypto, etc >>>>>>> operational params). >>>>>>>> Petri Savolainen(psavol) wrote: >>>>>>>> No, other APIs order checksum checking the same way: per protocol. >>>>>>>> Application may be e.g. terminating only UDP and forwarding all others >>>>>>>> - so it would be waste to check e.g. TCP checksum. >>>>>>>> >>>>>>>> typedef union odp_pktin_config_opt_t { >>>>>>>> /** Option flags */ >>>>>>>> struct { >>>>>>>> /** Timestamp all packets on packet input */ >>>>>>>> uint64_t ts_all : 1; >>>>>>>> >>>>>>>> /** Timestamp (at least) IEEE1588 / PTP packets >>>>>>>> * on packet input */ >>>>>>>> uint64_t ts_ptp : 1; >>>>>>>> >>>>>>>> /** Check IPv4 header checksum on packet input */ >>>>>>>> uint64_t ipv4_chksum : 1; >>>>>>>> >>>>>>>> /** Check UDP checksum on packet input */ >>>>>>>> uint64_t udp_chksum : 1; >>>>>>>> >>>>>>>> /** Check TCP checksum on packet input */ >>>>>>>> uint64_t tcp_chksum : 1; >>>>>>>>> Petri Savolainen(psavol) wrote: >>>>>>>>> Param init on fast path is a bit overkill. However, application could >>>>>>>>> call it once and store the result. So, it could avoid extra function >>>>>>>>> call for every packet. So, I'll add it. >>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>> `return i;` if we adopt the RC == number of successful packets >>>>>>>>>> parsed convention. >>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>> For consistency with other `odp_xxx_multi()` APIs, the RC should >>>>>>>>>>> indicate the number of input packets that were successfully >>>>>>>>>>> processed. >>>>>>>>>>>> muvarov wrote >>>>>>>>>>>> it looks like 2 enums are not needed here. odp_proto_layet_t >>>>>>>>>>>> layer_start; and odp_proto_layet_t layer_end; and one enum for >>>>>>>>>>>> layers. >>>>>>>>>>>>> muvarov wrote >>>>>>>>>>>>> maybe return number of correctly parsed packets? And say that >>>>>>>>>>>>> parsing always starts from be beginning of array. >>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>>>>> +1 since other checksum APIs refer to L3 and L4 rather than >>>>>>>>>>>>>> specific protocols. >>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>> What about just `l3_chksum`, and `l4_chksum`? >>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>>>>>>> Now that we have a `param` struct we should have an >>>>>>>>>>>>>>>> `odp_packet_parse_param_init()` API as well for completeness. >>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>> @Bill-Fischofer-Linaro Because it is a parsing error, if >>>>>>>>>>>>>>>>> lower layer provides us IP version that does not correspond >>>>>>>>>>>>>>>>> to the in-packet version. We should detect that, rather than >>>>>>>>>>>>>>>>> silently parsing this header. >>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>>>>>>>>> I'm not sure why an API named `odp_packet_parse()` needs >>>>>>>>>>>>>>>>>> help here. It's purpose, after all, is to parse packets and >>>>>>>>>>>>>>>>>> determining IPv4 vs IPv6 is part of that activity. Moreover, >>>>>>>>>>>>>>>>>> the only way an application can inspect the IP header is to >>>>>>>>>>>>>>>>>> access it via other ODP API calls, so I don't see how asking >>>>>>>>>>>>>>>>>> the application to do this is any better than having the >>>>>>>>>>>>>>>>>> `odp_packet_parse()` implementation do this itself. What's >>>>>>>>>>>>>>>>>> the purpose of having a parse API in that case since clearly >>>>>>>>>>>>>>>>>> the application could parse the entire packet "by hand" as >>>>>>>>>>>>>>>>>> well. >>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>> Failure needs to be defined in a more precise way (and >>>>>>>>>>>>>>>>>>> maybe for a single packet case). I assume that it means >>>>>>>>>>>>>>>>>>> internal ODP error, rather than just packet with wrong >>>>>>>>>>>>>>>>>>> headers. What happens in multi-packet case if failure >>>>>>>>>>>>>>>>>>> occurs in the middle of parsing? >>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>> @psavol Also for multi-packet parsing, we can change >>>>>>>>>>>>>>>>>>>> `proto` to be an array, allowing applications to easily >>>>>>>>>>>>>>>>>>>> intermix IPv4 and IPv6 packets. >>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>>> @psavol yes. I just wanted to focus on cases, when >>>>>>>>>>>>>>>>>>>>> passing packet with wrong protocol is an error. E.g. IPv6 >>>>>>>>>>>>>>>>>>>>> packet inside IPsec packet with NH = 4. So it is not a >>>>>>>>>>>>>>>>>>>>> question of selecting proper L3 parser, but rather a >>>>>>>>>>>>>>>>>>>>> question of nailing down error/malicious packets. >>>>>>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote: >>>>>>>>>>>>>>>>>>>>>> It's there to enable application to call parsing in >>>>>>>>>>>>>>>>>>>>>> parts - e.g. first up to IP and then continue from L4. >>>>>>>>>>>>>>>>>>>>>> But since IP and transport protocols are tied together >>>>>>>>>>>>>>>>>>>>>> with pseudo headers, it's cleaner to remove L4 as a >>>>>>>>>>>>>>>>>>>>>> starting point. >>>>>>>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote: >>>>>>>>>>>>>>>>>>>>>>> First bits of an IP header marks the version. So, it >>>>>>>>>>>>>>>>>>>>>>> would be trivial for both app and implementation to >>>>>>>>>>>>>>>>>>>>>>> read the version from the data. Common IP define is >>>>>>>>>>>>>>>>>>>>>>> easier for application when a burst of packets may >>>>>>>>>>>>>>>>>>>>>>> contain both v4 and v6 mixed. Application does not need >>>>>>>>>>>>>>>>>>>>>>> to sort packets into two arrays (one for v4 and other >>>>>>>>>>>>>>>>>>>>>>> for v6) but just pass the entire array for parsing. >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>> There are three ways to define the enumeration: IP, >>>>>>>>>>>>>>>>>>>>>>> IPv4+IPv6, IP+IPv4+IPv6. I'm OK with any of those. >>>>>>>>>>>>>>>>>>>>>>> IPv4+IPv6 would be a bit more robust since version >>>>>>>>>>>>>>>>>>>>>>> information comes from two sources. >>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>>>>>> I felt easier to reparse both L3 and L4 headers in >>>>>>>>>>>>>>>>>>>>>>>> IPsec case, especially since transport mode ESP can >>>>>>>>>>>>>>>>>>>>>>>> en/decrypt some of L3 headers in IPv6 case. >>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>>>>>>> @Bill-Fischofer-Linaro In IPsec case Next Header >>>>>>>>>>>>>>>>>>>>>>>>> field will contain 4 for IPv4 and 41 for IPv6. >>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>> That might be overly complicated since until a >>>>>>>>>>>>>>>>>>>>>>>>>> decrypted tunnel mode IPsec packet is parsed you >>>>>>>>>>>>>>>>>>>>>>>>>> don't know whether it's IPv4 or IPv6. It's parsing >>>>>>>>>>>>>>>>>>>>>>>>>> that makes that determination. >>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>> @lumag IPsec operating in transport mode is, I'd >>>>>>>>>>>>>>>>>>>>>>>>>>> imagine, the main use case here. >>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>> What is the usecase for parsing a packet starting >>>>>>>>>>>>>>>>>>>>>>>>>>>> from L4 header? Also there are several (lots) of >>>>>>>>>>>>>>>>>>>>>>>>>>>> other L4 protocols. Do we want to support them all >>>>>>>>>>>>>>>>>>>>>>>>>>>> here? >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Dmitry Eremin-Solenikov(lumag) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>> Maybe it would be better to split this into >>>>>>>>>>>>>>>>>>>>>>>>>>>>> separate IPv4 and IPv6 packets. It would be an >>>>>>>>>>>>>>>>>>>>>>>>>>>>> error to pass IPv6 packet with ethtype (or IP >>>>>>>>>>>>>>>>>>>>>>>>>>>>> tunnel type) being set to IPv4. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> A vector of packets is CPU vector instructions >>>>>>>>>>>>>>>>>>>>>>>>>>>>>> friendly. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Petri Savolainen(psavol) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> The use case is mentioned in log message: parse >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> after decrypt/IP reassembly. Application has >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> recreated an inner packet and needs to parse it >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> before continue. This is inherently SW parse >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> which may be accelerated with CPU vector >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> instructions, etc. >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> What's the use case for a multi() form of this >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> API? Might VPP use it? Perhaps Sachin can >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> comment? >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> We had considered an `odp_packet_parse()` >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> function some time back but it was rejected >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> as something that would not fit well with >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> hardware parsers. What's changed? https://github.com/Linaro/odp/pull/273#discussion_r149916975 updated_at 2017-11-09 12:49:44