Re: [Xen-devel] [RFC PATCH V3 13/16] netback: stub for multi receive protocol support.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

> > > existing code as protocol 0.
> > 
> > Why not 1?
> > 
> 
> We have some existing xenolinux code which has not been upstreamed calls
> this protocol 0, just try to be compatible.

Ah. Please do mention that in the description.

> 
> > Why do we need a new rework without anything using it besides
> > the existing framework? OR if you are, you should say which
> > patch is doing it...
> > 
> 
> It is not in use at the moment, and will be in use in the future.

Ok, should it be part of the "in the future" patchset then?

> 
> > > 
> > > Now the file layout becomes:
> > > 
> > >  - interface.c: xenvif interfaces
> > >  - xenbus.c: xenbus related functions
> > >  - netback.c: common functions for various protocols
> > > 
> > > For different protocols:
> > > 
> > >  - xenvif_rx_protocolX.h: header file for the protocol, including
> > >                           protocol structures and functions
> > >  - xenvif_rx_protocolX.c: implementations
> > > 
> > > To add a new protocol:
> > > 
> > >  - include protocol header in common.h
> > >  - modify XENVIF_MAX_RX_PROTOCOL in common.h
> > >  - add protocol structure in xenvif.rx union
> > >  - stub in xenbus.c
> > >  - modify Makefile
> > > 
> > > A protocol should define five functions:
> > > 
> > >  - setup: setup frontend / backend ring connections
> > >  - teardown: teardown frontend / backend ring connections
> > >  - start_xmit: host start xmit (i.e. guest need to do rx)
> > >  - event: rx completion event
> > >  - action: prepare host side data for guest rx
> > > 
> > .. snip..
> > 
> > > -
> > > -	return resp;
> > > -}
> > > -
> > >  static inline int rx_work_todo(struct xenvif *vif)
> > >  {
> > >  	return !skb_queue_empty(&vif->rx_queue);
> > > @@ -1507,8 +999,8 @@ int xenvif_kthread(void *data)
> > >  		if (kthread_should_stop())
> > >  			break;
> > >  
> > > -		if (rx_work_todo(vif))
> > > -			xenvif_rx_action(vif);
> > > +		if (rx_work_todo(vif) && vif->action)
> > > +			vif->action(vif);
> > >  	}
> > >  
> > >  	return 0;
> > > diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> > > index 79499fc..4067286 100644
> > > --- a/drivers/net/xen-netback/xenbus.c
> > > +++ b/drivers/net/xen-netback/xenbus.c
> > > @@ -415,6 +415,7 @@ static int connect_rings(struct backend_info *be)
> > >  	unsigned long rx_ring_ref[NETBK_MAX_RING_PAGES];
> > >  	unsigned int  tx_ring_order;
> > >  	unsigned int  rx_ring_order;
> > > +	unsigned int  rx_protocol;
> > >  
> > >  	err = xenbus_gather(XBT_NIL, dev->otherend,
> > >  			    "event-channel", "%u", &evtchn, NULL);
> > > @@ -510,6 +511,11 @@ static int connect_rings(struct backend_info *be)
> > >  		}
> > >  	}
> > >  
> > > +	err = xenbus_scanf(XBT_NIL, dev->otherend, "rx-protocol",
> > 
> > feature-rx-protocol?
> > 
> 
> This is not a feature switch. Does it make sense to add "feature-"

Good point.
> prefix?

It is negotiating a new protocol. Hm, perhaps 'protocol-rx-version' instead?
Or just 'protocol-version'?