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

[Denis Kirjanov <kda@xxxxxxxxxxxxxxxxx>]

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.

>
> 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;
>>>>    };