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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 5/22/20 12:33 PM, Denis Kirjanov wrote:
> On 5/22/20, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx> wrote:
>> On 5/22/20 12:17 PM, Denis Kirjanov wrote:
>>> On 5/22/20, Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>
>>> 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@xxxxxxxx>
>>>>> ---
>>>>> 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;
>>>>>     };