Re: [Xen-devel] [PATCH net-next v2 1/5] xen-netback: add IPv6 checksum offload to guest

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@xxxxxxxxxx]
> Sent: 08 October 2013 14:10
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; Wei Liu; David Vrabel;
> Ian Campbell
> Subject: Re: [PATCH net-next v2 1/5] xen-netback: add IPv6 checksum
> offload to guest
> 
> On Tue, Oct 08, 2013 at 11:58:12AM +0100, Paul Durrant wrote:
> > Check xenstore flag feature-ipv6-csum-offload to determine if a
> > guest is happy to accept IPv6 packets with only partial checksum.
> > Also check analogous feature-ip-csum-offload to determone if a
> 
> determone -> determine
> 

Ok.

> > guest is happy to accept IPv4 packets with only partial checksum
> > as a replacement for a negated feature-no-csum-offload value and
> > add a comment to deprecate use of feature-no-csum-offload.
> >
> [...]
> > @@ -306,7 +308,7 @@ struct xenvif *xenvif_alloc(struct device *parent,
> domid_t domid,
> >     vif->domid  = domid;
> >     vif->handle = handle;
> >     vif->can_sg = 1;
> > -   vif->csum = 1;
> > +   vif->ip_csum = 1;
> 
> Not setting a default value for vif->ipv6_csum?
> 

The default is 0 so no need.

> >     vif->dev = dev;
> >
> [...]
> > +   /* Before feature-ipv6-csum-offload was introduced, checksum
> offload
> > +    * was turned on by a zero value in (or lack of)
> > +    * feature-no-csum-offload. Therefore, for compatibility, assume
> > +    * that if feature-ip-csum-offload is missing that IP (v4) offload
> > +    * is turned on. If this is not the case then the flag will be
> > +    * cleared when we subsequently sample feature-no-csum-offload.
> > +    */
> > +   if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-ip-csum-
> offload",
> > +                    "%d", &val) < 0)
> > +           val = 1;
> > +   vif->ip_csum = !!val;
> > +
> > +   if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-ipv6-csum-
> offload",
> > +                    "%d", &val) < 0)
> > +           val = 0;
> > +   vif->ipv6_csum = !!val;
> > +
> > +   /* This is deprecated. Frontends should set IP v4 and v6 checksum
> > +    * offload using feature-ip-csum-offload and
> > +    * feature-ipv6-csum-offload respectively.
> > +    */
> 
> These comments on feature flags should also be synchronized to master
> netif.h in Xen and Linux's header. We've been trying to document thing
> along the way for quite some time and the long term benifit is big.
> 

Ok. That sounds like a good idea. I'll add a patch to do that.

> >     if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-no-csum-
> offload",
> >                      "%d", &val) < 0)
> >             val = 0;
> > -   vif->csum = !val;
> > +   if (val)
> > +           vif->ip_csum = 0;
> 
> Do you also need to set vif->ipv6_csum to 0? I don't think a frontend
> which cannot deal with V4 csum offload has the ability to deal with V6
> csum offload.
> 

I was just trying to preserve the existing semantic of feature-no-csum-offload, 
which only affects v4. Since it's deprecated, should we add a new semantic?

  Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel