[PATCH v4] public/io/netif.h: add a new extra type for XDP

[PATCH v4] public/io/netif.h: add a new extra type for XDP

[Denis Kirjanov]

The patch adds a new extra type to be able to diffirentiate
between RX responses on xen-netfront side with the adjusted offset
required for XDP processing.

The offset value from a guest is passed via xenstore.

Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
---
v4:
- updated the commit and documenation

v3:
- updated the commit message

v2:
- added documentation
- fixed padding for netif_extra_info
---
---
 xen/include/public/io/netif.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index 9fcf91a..a92bf04 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -161,6 +161,17 @@
  */
 
 /*
+ * "xdp-headroom" is used to request that extra space is added
+ * for XDP processing.  The value is measured in bytes and passed by
+ * the frontend to be consistent between both ends.
+ * If the value is greater than zero that means that
+ * an RX response is going to be passed to an XDP program for processing.
+ *
+ * "feature-xdp-headroom" is set to "1" by the netback side like other features
+ * so a guest can check if an XDP program can be processed.
+ */
+
+/*
  * Control ring
  * ============
  *
@@ -985,7 +996,8 @@ typedef struct netif_tx_request netif_tx_request_t;
 #define XEN_NETIF_EXTRA_TYPE_MCAST_ADD (2)  /* u.mcast */
 #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3)  /* u.mcast */
 #define XEN_NETIF_EXTRA_TYPE_HASH      (4)  /* u.hash */
-#define XEN_NETIF_EXTRA_TYPE_MAX       (5)
+#define XEN_NETIF_EXTRA_TYPE_XDP       (5)  /* u.xdp */
+#define XEN_NETIF_EXTRA_TYPE_MAX       (6)
 
 /* netif_extra_info_t flags. */
 #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
@@ -1018,6 +1030,10 @@ struct netif_extra_info {
             uint8_t algorithm;
             uint8_t value[4];
         } hash;
+        struct {
+            uint16_t headroom;
+            uint16_t pad[2]
+        } xdp;
         uint16_t pad[3];
     } u;
 };
-- 
1.8.3.1

Re: [PATCH v4] public/io/netif.h: add a new extra type for XDP

[Oleksandr Andrushchenko]

On 5/18/20 6:04 PM, Denis Kirjanov wrote:
> The patch adds a new extra type to be able to diffirentiate
> between RX responses on xen-netfront side with the adjusted offset
> required for XDP processing.
>
> The offset value from a guest is passed via xenstore.
>
> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
> ---
> v4:
> - updated the commit and documenation
>
> v3:
> - updated the commit message
>
> v2:
> - added documentation
> - fixed padding for netif_extra_info
> ---
> ---
>   xen/include/public/io/netif.h | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> index 9fcf91a..a92bf04 100644
> --- a/xen/include/public/io/netif.h
> +++ b/xen/include/public/io/netif.h
> @@ -161,6 +161,17 @@
>    */
>   
>   /*
> + * "xdp-headroom" is used to request that extra space is added
> + * for XDP processing.  The value is measured in bytes and passed by

not sure that we should use word "bytes" here as the rest of the 
protocol (mostly)

talks about octets. It is somewhat mixed here, no strong opinion

> + * the frontend to be consistent between both ends.
> + * If the value is greater than zero that means that
> + * an RX response is going to be passed to an XDP program for processing.
> + *
> + * "feature-xdp-headroom" is set to "1" by the netback side like other features
> + * so a guest can check if an XDP program can be processed.
> + */
> +
> +/*
>    * Control ring
>    * ============
>    *
> @@ -985,7 +996,8 @@ typedef struct netif_tx_request netif_tx_request_t;
>   #define XEN_NETIF_EXTRA_TYPE_MCAST_ADD (2)  /* u.mcast */
>   #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3)  /* u.mcast */
>   #define XEN_NETIF_EXTRA_TYPE_HASH      (4)  /* u.hash */
> -#define XEN_NETIF_EXTRA_TYPE_MAX       (5)
> +#define XEN_NETIF_EXTRA_TYPE_XDP       (5)  /* u.xdp */
> +#define XEN_NETIF_EXTRA_TYPE_MAX       (6)
>   
>   /* netif_extra_info_t flags. */
>   #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
> @@ -1018,6 +1030,10 @@ struct netif_extra_info {
>               uint8_t algorithm;
>               uint8_t value[4];
>           } hash;
> +        struct {
> +            uint16_t headroom;
why do you need "pad" field here?
> +            uint16_t pad[2]
> +        } xdp;
>           uint16_t pad[3];
>       } u;
>   };

Re: [PATCH v4] public/io/netif.h: add a new extra type for XDP

[Denis Kirjanov]

On 5/22/20, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> wrote:
> On 5/18/20 6:04 PM, Denis Kirjanov wrote:
>> The patch adds a new extra type to be able to diffirentiate
>> between RX responses on xen-netfront side with the adjusted offset
>> required for XDP processing.
>>
>> The offset value from a guest is passed via xenstore.
>>
>> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
>> ---
>> v4:
>> - updated the commit and documenation
>>
>> v3:
>> - updated the commit message
>>
>> v2:
>> - added documentation
>> - fixed padding for netif_extra_info
>> ---
>> ---
>>   xen/include/public/io/netif.h | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/include/public/io/netif.h
>> b/xen/include/public/io/netif.h
>> index 9fcf91a..a92bf04 100644
>> --- a/xen/include/public/io/netif.h
>> +++ b/xen/include/public/io/netif.h
>> @@ -161,6 +161,17 @@
>>    */
>>
>>   /*
>> + * "xdp-headroom" is used to request that extra space is added
>> + * for XDP processing.  The value is measured in bytes and passed by
>
> not sure that we should use word "bytes" here as the rest of the
> protocol (mostly)
>
> talks about octets. It is somewhat mixed here, no strong opinion

sure, but since the public header mixes it I've decided to use that word.


>
>> + * the frontend to be consistent between both ends.
>> + * If the value is greater than zero that means that
>> + * an RX response is going to be passed to an XDP program for
>> processing.
>> + *
>> + * "feature-xdp-headroom" is set to "1" by the netback side like other
>> features
>> + * so a guest can check if an XDP program can be processed.
>> + */
>> +
>> +/*
>>    * Control ring
>>    * ============
>>    *
>> @@ -985,7 +996,8 @@ typedef struct netif_tx_request netif_tx_request_t;
>>   #define XEN_NETIF_EXTRA_TYPE_MCAST_ADD (2)  /* u.mcast */
>>   #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3)  /* u.mcast */
>>   #define XEN_NETIF_EXTRA_TYPE_HASH      (4)  /* u.hash */
>> -#define XEN_NETIF_EXTRA_TYPE_MAX       (5)
>> +#define XEN_NETIF_EXTRA_TYPE_XDP       (5)  /* u.xdp */
>> +#define XEN_NETIF_EXTRA_TYPE_MAX       (6)
>>
>>   /* netif_extra_info_t flags. */
>>   #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
>> @@ -1018,6 +1030,10 @@ struct netif_extra_info {
>>               uint8_t algorithm;
>>               uint8_t value[4];
>>           } hash;
>> +        struct {
>> +            uint16_t headroom;
> why do you need "pad" field here?

To state that we have a fixed size available.

>> +            uint16_t pad[2]
>> +        } xdp;
>>           uint16_t pad[3];
>>       } u;
>>   };

Re: [PATCH v4] public/io/netif.h: add a new extra type for XDP

[Denis Kirjanov]

On 5/18/20, Denis Kirjanov <kda@linux-powerpc.org> wrote:
> The patch adds a new extra type to be able to diffirentiate
> between RX responses on xen-netfront side with the adjusted offset
> required for XDP processing.
>
> The offset value from a guest is passed via xenstore.

I'm going to send a new version for Linux with the above changes applied.

>
> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
> ---
> v4:
> - updated the commit and documenation
>
> v3:
> - updated the commit message
>
> v2:
> - added documentation
> - fixed padding for netif_extra_info
> ---
> ---
>  xen/include/public/io/netif.h | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> index 9fcf91a..a92bf04 100644
> --- a/xen/include/public/io/netif.h
> +++ b/xen/include/public/io/netif.h
> @@ -161,6 +161,17 @@
>   */
>
>  /*
> + * "xdp-headroom" is used to request that extra space is added
> + * for XDP processing.  The value is measured in bytes and passed by
> + * the frontend to be consistent between both ends.
> + * If the value is greater than zero that means that
> + * an RX response is going to be passed to an XDP program for processing.
> + *
> + * "feature-xdp-headroom" is set to "1" by the netback side like other
> features
> + * so a guest can check if an XDP program can be processed.
> + */
> +
> +/*
>   * Control ring
>   * ============
>   *
> @@ -985,7 +996,8 @@ typedef struct netif_tx_request netif_tx_request_t;
>  #define XEN_NETIF_EXTRA_TYPE_MCAST_ADD (2)  /* u.mcast */
>  #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3)  /* u.mcast */
>  #define XEN_NETIF_EXTRA_TYPE_HASH      (4)  /* u.hash */
> -#define XEN_NETIF_EXTRA_TYPE_MAX       (5)
> +#define XEN_NETIF_EXTRA_TYPE_XDP       (5)  /* u.xdp */
> +#define XEN_NETIF_EXTRA_TYPE_MAX       (6)
>
>  /* netif_extra_info_t flags. */
>  #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
> @@ -1018,6 +1030,10 @@ struct netif_extra_info {
>              uint8_t algorithm;
>              uint8_t value[4];
>          } hash;
> +        struct {
> +            uint16_t headroom;
> +            uint16_t pad[2]
> +        } xdp;
>          uint16_t pad[3];
>      } u;
>  };
> --
> 1.8.3.1
>
>

Re: [PATCH v4] public/io/netif.h: add a new extra type for XDP

[Oleksandr Andrushchenko]

On 5/22/20 12:17 PM, Denis Kirjanov wrote:
> On 5/22/20, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> wrote:
>> On 5/18/20 6:04 PM, Denis Kirjanov wrote:
>>> The patch adds a new extra type to be able to diffirentiate
>>> between RX responses on xen-netfront side with the adjusted offset
>>> required for XDP processing.
>>>
>>> The offset value from a guest is passed via xenstore.
>>>
>>> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
>>> ---
>>> v4:
>>> - updated the commit and documenation
>>>
>>> v3:
>>> - updated the commit message
>>>
>>> v2:
>>> - added documentation
>>> - fixed padding for netif_extra_info
>>> ---
>>> ---
>>>    xen/include/public/io/netif.h | 18 +++++++++++++++++-
>>>    1 file changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/include/public/io/netif.h
>>> b/xen/include/public/io/netif.h
>>> index 9fcf91a..a92bf04 100644
>>> --- a/xen/include/public/io/netif.h
>>> +++ b/xen/include/public/io/netif.h
>>> @@ -161,6 +161,17 @@
>>>     */
>>>
>>>    /*
>>> + * "xdp-headroom" is used to request that extra space is added
>>> + * for XDP processing.  The value is measured in bytes and passed by
>> not sure that we should use word "bytes" here as the rest of the
>> protocol (mostly)
>>
>> talks about octets. It is somewhat mixed here, no strong opinion
> sure, but since the public header mixes it I've decided to use that word.
>
>
>>> + * the frontend to be consistent between both ends.
>>> + * If the value is greater than zero that means that
>>> + * an RX response is going to be passed to an XDP program for
>>> processing.
>>> + *
>>> + * "feature-xdp-headroom" is set to "1" by the netback side like other
>>> features
>>> + * so a guest can check if an XDP program can be processed.
>>> + */
>>> +
>>> +/*
>>>     * Control ring
>>>     * ============
>>>     *
>>> @@ -985,7 +996,8 @@ typedef struct netif_tx_request netif_tx_request_t;
>>>    #define XEN_NETIF_EXTRA_TYPE_MCAST_ADD (2)  /* u.mcast */
>>>    #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3)  /* u.mcast */
>>>    #define XEN_NETIF_EXTRA_TYPE_HASH      (4)  /* u.hash */
>>> -#define XEN_NETIF_EXTRA_TYPE_MAX       (5)
>>> +#define XEN_NETIF_EXTRA_TYPE_XDP       (5)  /* u.xdp */
>>> +#define XEN_NETIF_EXTRA_TYPE_MAX       (6)
>>>
>>>    /* netif_extra_info_t flags. */
>>>    #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
>>> @@ -1018,6 +1030,10 @@ struct netif_extra_info {
>>>                uint8_t algorithm;
>>>                uint8_t value[4];
>>>            } hash;
>>> +        struct {
>>> +            uint16_t headroom;
>> why do you need "pad" field here?
> To state that we have a fixed size available.

Well, I would expect "reserved" or something in that case and "pad" in case

there are other fields following (see gso above).

But here I think "pad" is not required, just like mcast doesn't add any

>
>>> +            uint16_t pad[2]
>>> +        } xdp;
>>>            uint16_t pad[3];
>>>        } u;
>>>    };

Re: [PATCH v4] public/io/netif.h: add a new extra type for XDP

[Denis Kirjanov]

On 5/22/20, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> wrote:
> On 5/22/20 12:17 PM, Denis Kirjanov wrote:
>> On 5/22/20, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
>> wrote:
>>> On 5/18/20 6:04 PM, Denis Kirjanov wrote:
>>>> The patch adds a new extra type to be able to diffirentiate
>>>> between RX responses on xen-netfront side with the adjusted offset
>>>> required for XDP processing.
>>>>
>>>> The offset value from a guest is passed via xenstore.
>>>>
>>>> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
>>>> ---
>>>> v4:
>>>> - updated the commit and documenation
>>>>
>>>> v3:
>>>> - updated the commit message
>>>>
>>>> v2:
>>>> - added documentation
>>>> - fixed padding for netif_extra_info
>>>> ---
>>>> ---
>>>>    xen/include/public/io/netif.h | 18 +++++++++++++++++-
>>>>    1 file changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/include/public/io/netif.h
>>>> b/xen/include/public/io/netif.h
>>>> index 9fcf91a..a92bf04 100644
>>>> --- a/xen/include/public/io/netif.h
>>>> +++ b/xen/include/public/io/netif.h
>>>> @@ -161,6 +161,17 @@
>>>>     */
>>>>
>>>>    /*
>>>> + * "xdp-headroom" is used to request that extra space is added
>>>> + * for XDP processing.  The value is measured in bytes and passed by
>>> not sure that we should use word "bytes" here as the rest of the
>>> protocol (mostly)
>>>
>>> talks about octets. It is somewhat mixed here, no strong opinion
>> sure, but since the public header mixes it I've decided to use that word.
>>
>>
>>>> + * the frontend to be consistent between both ends.
>>>> + * If the value is greater than zero that means that
>>>> + * an RX response is going to be passed to an XDP program for
>>>> processing.
>>>> + *
>>>> + * "feature-xdp-headroom" is set to "1" by the netback side like other
>>>> features
>>>> + * so a guest can check if an XDP program can be processed.
>>>> + */
>>>> +
>>>> +/*
>>>>     * Control ring
>>>>     * ============
>>>>     *
>>>> @@ -985,7 +996,8 @@ typedef struct netif_tx_request netif_tx_request_t;
>>>>    #define XEN_NETIF_EXTRA_TYPE_MCAST_ADD (2)  /* u.mcast */
>>>>    #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3)  /* u.mcast */
>>>>    #define XEN_NETIF_EXTRA_TYPE_HASH      (4)  /* u.hash */
>>>> -#define XEN_NETIF_EXTRA_TYPE_MAX       (5)
>>>> +#define XEN_NETIF_EXTRA_TYPE_XDP       (5)  /* u.xdp */
>>>> +#define XEN_NETIF_EXTRA_TYPE_MAX       (6)
>>>>
>>>>    /* netif_extra_info_t flags. */
>>>>    #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
>>>> @@ -1018,6 +1030,10 @@ struct netif_extra_info {
>>>>                uint8_t algorithm;
>>>>                uint8_t value[4];
>>>>            } hash;
>>>> +        struct {
>>>> +            uint16_t headroom;
>>> why do you need "pad" field here?
>> To state that we have a fixed size available.
>
> Well, I would expect "reserved" or something in that case and "pad" in case
>
> there are other fields following (see gso above).

it can be consistent with other names like pad at then end of the structure.

If it really matters I can change it, no problem.

>
> But here I think "pad" is not required, just like mcast doesn't add any
because it's already 6-bytes long

>
>>
>>>> +            uint16_t pad[2]
>>>> +        } xdp;
>>>>            uint16_t pad[3];
>>>>        } u;
>>>>    };

Re: [PATCH v4] public/io/netif.h: add a new extra type for XDP

[Oleksandr Andrushchenko]

On 5/22/20 12:33 PM, Denis Kirjanov wrote:
> On 5/22/20, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com> wrote:
>> On 5/22/20 12:17 PM, Denis Kirjanov wrote:
>>> On 5/22/20, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>
>>> wrote:
>>>> On 5/18/20 6:04 PM, Denis Kirjanov wrote:
>>>>> The patch adds a new extra type to be able to diffirentiate
>>>>> between RX responses on xen-netfront side with the adjusted offset
>>>>> required for XDP processing.
>>>>>
>>>>> The offset value from a guest is passed via xenstore.
>>>>>
>>>>> Signed-off-by: Denis Kirjanov <denis.kirjanov@suse.com>
>>>>> ---
>>>>> v4:
>>>>> - updated the commit and documenation
>>>>>
>>>>> v3:
>>>>> - updated the commit message
>>>>>
>>>>> v2:
>>>>> - added documentation
>>>>> - fixed padding for netif_extra_info
>>>>> ---
>>>>> ---
>>>>>     xen/include/public/io/netif.h | 18 +++++++++++++++++-
>>>>>     1 file changed, 17 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/xen/include/public/io/netif.h
>>>>> b/xen/include/public/io/netif.h
>>>>> index 9fcf91a..a92bf04 100644
>>>>> --- a/xen/include/public/io/netif.h
>>>>> +++ b/xen/include/public/io/netif.h
>>>>> @@ -161,6 +161,17 @@
>>>>>      */
>>>>>
>>>>>     /*
>>>>> + * "xdp-headroom" is used to request that extra space is added
>>>>> + * for XDP processing.  The value is measured in bytes and passed by
>>>> not sure that we should use word "bytes" here as the rest of the
>>>> protocol (mostly)
>>>>
>>>> talks about octets. It is somewhat mixed here, no strong opinion
>>> sure, but since the public header mixes it I've decided to use that word.
>>>
>>>
>>>>> + * the frontend to be consistent between both ends.
>>>>> + * If the value is greater than zero that means that
>>>>> + * an RX response is going to be passed to an XDP program for
>>>>> processing.
>>>>> + *
>>>>> + * "feature-xdp-headroom" is set to "1" by the netback side like other
>>>>> features
>>>>> + * so a guest can check if an XDP program can be processed.
>>>>> + */
>>>>> +
>>>>> +/*
>>>>>      * Control ring
>>>>>      * ============
>>>>>      *
>>>>> @@ -985,7 +996,8 @@ typedef struct netif_tx_request netif_tx_request_t;
>>>>>     #define XEN_NETIF_EXTRA_TYPE_MCAST_ADD (2)  /* u.mcast */
>>>>>     #define XEN_NETIF_EXTRA_TYPE_MCAST_DEL (3)  /* u.mcast */
>>>>>     #define XEN_NETIF_EXTRA_TYPE_HASH      (4)  /* u.hash */
>>>>> -#define XEN_NETIF_EXTRA_TYPE_MAX       (5)
>>>>> +#define XEN_NETIF_EXTRA_TYPE_XDP       (5)  /* u.xdp */
>>>>> +#define XEN_NETIF_EXTRA_TYPE_MAX       (6)
>>>>>
>>>>>     /* netif_extra_info_t flags. */
>>>>>     #define _XEN_NETIF_EXTRA_FLAG_MORE (0)
>>>>> @@ -1018,6 +1030,10 @@ struct netif_extra_info {
>>>>>                 uint8_t algorithm;
>>>>>                 uint8_t value[4];
>>>>>             } hash;
>>>>> +        struct {
>>>>> +            uint16_t headroom;
>>>> why do you need "pad" field here?
>>> To state that we have a fixed size available.
>> Well, I would expect "reserved" or something in that case and "pad" in case
>>
>> there are other fields following (see gso above).
> it can be consistent with other names like pad at then end of the structure.
>
> If it really matters I can change it, no problem.
My point is that IMO it is not required at all, but this is up to 
maintainers to decide
>
>> But here I think "pad" is not required, just like mcast doesn't add any
> because it's already 6-bytes long
you are right
>
>>>>> +            uint16_t pad[2]
>>>>> +        } xdp;
>>>>>             uint16_t pad[3];
>>>>>         } u;
>>>>>     };