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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Denis Kirjanov <kda@xxxxxxxxxxxxxxxxx>
> Sent: 18 May 2020 14:49
> To: paul@xxxxxxx
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; jgross@xxxxxxxx
> Subject: Re: [PATCH v3] public/io/netif.h: add a new extra type for XDP
> 
> On 5/18/20, Paul Durrant <xadimgnik@xxxxxxxxx> wrote:
> >> -----Original Message-----
> >> From: Denis Kirjanov <kda@xxxxxxxxxxxxxxxxx>
> >> Sent: 18 May 2020 14:26
> >> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> >> Cc: paul@xxxxxxx; jgross@xxxxxxxx
> >> Subject: [PATCH v3] public/io/netif.h: add a new extra type for XDP
> >>
> >> 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>
> >> ---
> >> v3:
> >> - updated the commit message
> >>
> >> v2:
> >> - added documentation
> >> - fixed padding for netif_extra_info
> >> ---
> >>  xen/include/public/io/netif.h | 14 +++++++++++++-
> >>  1 file changed, 13 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/xen/include/public/io/netif.h
> >> b/xen/include/public/io/netif.h
> >> index 9fcf91a..ec56a15 100644
> >> --- a/xen/include/public/io/netif.h
> >> +++ b/xen/include/public/io/netif.h
> >> @@ -161,6 +161,13 @@
> >>   */
> >>
> >>  /*
> >> + * "netfront-xdp-headroom" is used to add an extra space before packet
> >> data
> >> + * for XDP processing. The value is 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.
> >> + */
> >
> > I think 'used to add extra space' is probably the wrong phrase. How about
> > 'is used to request that extra space is added'
> > It also does not state what unit the value is specified in so you need
> > something to clarify that.
> Ok.
> 
> I also don't understand what "The
> > value is passed by the frontend to be consistent between both ends" means.
> > What happens if the backend is older and does not know
> > what this key means?
> 
> Looks like it has also be stated here since I've added another value
> "feature-xdp-headroom" which is set by  the netback side.
> 

Yeah, that needs to be mentioned. I also think you should drop the 'netfront' 
out of the name. I think simply 'xdp-headroom' would be fine.

  Paul

> >
> >   Paul
> >
> >> +
> >> +/*
> >>   * Control ring
> >>   * ============
> >>   *
> >> @@ -985,7 +992,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 +1026,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
> >
> >
> >