muvarov replied on github web page:
include/odp/api/spec/packet.h
line 59
@@ -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;
Comment:
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_r149708399
updated_at 2017-11-08 15:48:42
