[PATCH 0/2] Update netif public header

[PATCH 0/2] Update netif public header

[Wei Liu]

These constants were added when fixing XSA-39 regression. Keep master copy in
Xen up to date with this series.


Thanks
Wei.

[PATCH 1/2] netif: define XEN_NETIF_MAX_TX_SIZE in public header

[Wei Liu]

This is the maximum supportted size of a packet.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/public/io/netif.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index b184e49..ccde53b 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -65,6 +65,7 @@
 #define _NETTXF_extra_info     (3)
 #define  NETTXF_extra_info     (1U<<_NETTXF_extra_info)
 
+#define XEN_NETIF_MAX_TX_SIZE 0xFFFF
 struct netif_tx_request {
     grant_ref_t gref;      /* Reference to buffer page */
     uint16_t offset;       /* Offset within buffer page */
-- 
1.7.10.4

[PATCH 2/2] netif: define XEN_NETIF_NR_SLOTS_MIN in public header

[Wei Liu]

Xen network protocol has implicit dependency on MAX_SKB_FRAGS. In order to
remove dependency on MAX_SKB_FRAGS, we derive a constant from historical
MAX_SKB_FRAGS for future reference.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/public/io/netif.h |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index ccde53b..9664998 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -31,6 +31,24 @@
 #include "../grant_table.h"
 
 /*
+ * Older implementation of Xen network frontend / backend has an
+ * implicit dependency on the MAX_SKB_FRAGS as the maximum number of
+ * ring slots a skb can use. Netfront / netback may not work as
+ * expected when frontend and backend have different MAX_SKB_FRAGS.
+ *
+ * A better approach is to add mechanism for netfront / netback to
+ * negotiate this value. However we cannot fix all possible
+ * frontends, so we need to define a value which states the minimum
+ * slots backend must support.
+ *
+ * The minimum value derives from older Linux kernel's MAX_SKB_FRAGS
+ * (18), which is proved to work with most frontends. Any new backend
+ * which doesn't negotiate with frontend should expect frontend to
+ * send a valid packet using slots up to this value.
+ */
+#define XEN_NETIF_NR_SLOTS_MIN 18
+
+/*
  * Notifications after enqueuing any type of message should be conditional on
  * the appropriate req_event or rsp_event field in the shared ring.
  * If the client sends notification for rx requests then it should specify
-- 
1.7.10.4

Re: [PATCH 1/2] netif: define XEN_NETIF_MAX_TX_SIZE in public header

[Ian Campbell]

On Tue, 2013-04-30 at 18:06 +0100, Wei Liu wrote:
> This is the maximum supportted size of a packet.

Too many t's in supported.

Worth mentioning that this limit comes from the size of
netif_tx_request.size.

> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/include/public/io/netif.h |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> index b184e49..ccde53b 100644
> --- a/xen/include/public/io/netif.h
> +++ b/xen/include/public/io/netif.h
> @@ -65,6 +65,7 @@
>  #define _NETTXF_extra_info     (3)
>  #define  NETTXF_extra_info     (1U<<_NETTXF_extra_info)
>  
> +#define XEN_NETIF_MAX_TX_SIZE 0xFFFF
>  struct netif_tx_request {
>      grant_ref_t gref;      /* Reference to buffer page */
>      uint16_t offset;       /* Offset within buffer page */

Re: [PATCH 2/2] netif: define XEN_NETIF_NR_SLOTS_MIN in public header

[Ian Campbell]

On Tue, 2013-04-30 at 18:06 +0100, Wei Liu wrote:
> Xen network protocol has implicit dependency on MAX_SKB_FRAGS. In order to
> remove dependency on MAX_SKB_FRAGS, we derive a constant from historical
> MAX_SKB_FRAGS for future reference.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  xen/include/public/io/netif.h |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> index ccde53b..9664998 100644
> --- a/xen/include/public/io/netif.h
> +++ b/xen/include/public/io/netif.h
> @@ -31,6 +31,24 @@
>  #include "../grant_table.h"
>  
>  /*
> + * Older implementation of Xen network frontend / backend has an
> + * implicit dependency on the MAX_SKB_FRAGS as the maximum number of
> + * ring slots a skb can use. Netfront / netback may not work as
> + * expected when frontend and backend have different MAX_SKB_FRAGS.
> + *
> + * A better approach is to add mechanism for netfront / netback to
> + * negotiate this value. However we cannot fix all possible
> + * frontends, so we need to define a value which states the minimum
> + * slots backend must support.
> + *
> + * The minimum value derives from older Linux kernel's MAX_SKB_FRAGS
> + * (18), which is proved to work with most frontends. Any new backend
> + * which doesn't negotiate with frontend should expect frontend to
> + * send a valid packet using slots up to this value.
> + */
> +#define XEN_NETIF_NR_SLOTS_MIN 18
> +
> +/*
>   * Notifications after enqueuing any type of message should be conditional on
>   * the appropriate req_event or rsp_event field in the shared ring.
>   * If the client sends notification for rx requests then it should specify

Re: [PATCH 1/2] netif: define XEN_NETIF_MAX_TX_SIZE in public header

[Wei Liu]

On Wed, May 01, 2013 at 11:17:28AM +0100, Ian Campbell wrote:
> On Tue, 2013-04-30 at 18:06 +0100, Wei Liu wrote:
> > This is the maximum supportted size of a packet.
> 
> Too many t's in supported.
> 
> Worth mentioning that this limit comes from the size of
> netif_tx_request.size.
> 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> > ---

-------8<----
>From b52999bc45cf1fcac369346c14b03bcf844db410 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Tue, 30 Apr 2013 17:58:48 +0100
Subject: [PATCH] netif: define XEN_NETIF_MAX_TX_SIZE in public header

This is the maximum supported size of a packet. It comes from the size of
netif_tx_request.size.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/include/public/io/netif.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
index b184e49..ccde53b 100644
--- a/xen/include/public/io/netif.h
+++ b/xen/include/public/io/netif.h
@@ -65,6 +65,7 @@
 #define _NETTXF_extra_info     (3)
 #define  NETTXF_extra_info     (1U<<_NETTXF_extra_info)
 
+#define XEN_NETIF_MAX_TX_SIZE 0xFFFF
 struct netif_tx_request {
     grant_ref_t gref;      /* Reference to buffer page */
     uint16_t offset;       /* Offset within buffer page */
-- 
1.7.10.4

interface header maintainership (was Re: [PATCH 2/2] netif: define XEN_NETIF_NR_SLOTS_MIN in public header)

[Jan Beulich]

>>> On 01.05.13 at 12:19, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2013-04-30 at 18:06 +0100, Wei Liu wrote:
>> Xen network protocol has implicit dependency on MAX_SKB_FRAGS. In order to
>> remove dependency on MAX_SKB_FRAGS, we derive a constant from historical
>> MAX_SKB_FRAGS for future reference.
>> 
>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

In order to not require Keir to ack any trivial change like this,
wouldn't it make sense to list you as the maintainer for
xen/include/public/io/netif.h, and Konrad similarly for blkif.h,
matching the Linux side maintainership?

Which reminds me that blkif.h too is lagging behind what's being
done in Linux (wrt indirect requests)...

Jan

Re: interface header maintainership (was Re: [PATCH 2/2] netif: define XEN_NETIF_NR_SLOTS_MIN in public header)

[Ian Campbell]

On Thu, 2013-05-02 at 10:30 +0100, Jan Beulich wrote:
> >>> On 01.05.13 at 12:19, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2013-04-30 at 18:06 +0100, Wei Liu wrote:
> >> Xen network protocol has implicit dependency on MAX_SKB_FRAGS. In order to
> >> remove dependency on MAX_SKB_FRAGS, we derive a constant from historical
> >> MAX_SKB_FRAGS for future reference.
> >> 
> >> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> In order to not require Keir to ack any trivial change like this,
> wouldn't it make sense to list you as the maintainer for
> xen/include/public/io/netif.h, and Konrad similarly for blkif.h,
> matching the Linux side maintainership?

My only concern is that the I/O interfaces should be independent from
the Linux implementation and by having the Linux maintainer be the *if.h
maintainer risks unintentionally baking (more) Linux'isms into our
interfaces or at least failing to fully consider the other OSes.

So I think any decision regarding maintainership of io/*.h should be
made independently of the Linux maintainership, even if the result is
that the "best" person happens to also be the Linux maintainer.

With that said I'm happy to try and wear both hats at the appropriate
times.

Ian.

Re: [PATCH 1/2] netif: define XEN_NETIF_MAX_TX_SIZE in public header

[Keir Fraser]

On 30/04/2013 18:06, "Wei Liu" <wei.liu2@citrix.com> wrote:

> This is the maximum supportted size of a packet.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Keir Fraser <keir@xen.org>

> ---
>  xen/include/public/io/netif.h |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> index b184e49..ccde53b 100644
> --- a/xen/include/public/io/netif.h
> +++ b/xen/include/public/io/netif.h
> @@ -65,6 +65,7 @@
>  #define _NETTXF_extra_info     (3)
>  #define  NETTXF_extra_info     (1U<<_NETTXF_extra_info)
>  
> +#define XEN_NETIF_MAX_TX_SIZE 0xFFFF
>  struct netif_tx_request {
>      grant_ref_t gref;      /* Reference to buffer page */
>      uint16_t offset;       /* Offset within buffer page */

Re: [PATCH 2/2] netif: define XEN_NETIF_NR_SLOTS_MIN in public header

[Keir Fraser]

On 30/04/2013 18:06, "Wei Liu" <wei.liu2@citrix.com> wrote:

> Xen network protocol has implicit dependency on MAX_SKB_FRAGS. In order to
> remove dependency on MAX_SKB_FRAGS, we derive a constant from historical
> MAX_SKB_FRAGS for future reference.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Keir Fraser <keir@xen.org>

> ---
>  xen/include/public/io/netif.h |   18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/xen/include/public/io/netif.h b/xen/include/public/io/netif.h
> index ccde53b..9664998 100644
> --- a/xen/include/public/io/netif.h
> +++ b/xen/include/public/io/netif.h
> @@ -31,6 +31,24 @@
>  #include "../grant_table.h"
>  
>  /*
> + * Older implementation of Xen network frontend / backend has an
> + * implicit dependency on the MAX_SKB_FRAGS as the maximum number of
> + * ring slots a skb can use. Netfront / netback may not work as
> + * expected when frontend and backend have different MAX_SKB_FRAGS.
> + *
> + * A better approach is to add mechanism for netfront / netback to
> + * negotiate this value. However we cannot fix all possible
> + * frontends, so we need to define a value which states the minimum
> + * slots backend must support.
> + *
> + * The minimum value derives from older Linux kernel's MAX_SKB_FRAGS
> + * (18), which is proved to work with most frontends. Any new backend
> + * which doesn't negotiate with frontend should expect frontend to
> + * send a valid packet using slots up to this value.
> + */
> +#define XEN_NETIF_NR_SLOTS_MIN 18
> +
> +/*
>   * Notifications after enqueuing any type of message should be conditional on
>   * the appropriate req_event or rsp_event field in the shared ring.
>   * If the client sends notification for rx requests then it should specify