> -----Original Message----- > From: Andrew Cooper [mailto:am...@hermes.cam.ac.uk] On Behalf Of > Andrew Cooper > Sent: 23 December 2015 11:45 > To: Paul Durrant; xen-de...@lists.xenproject.org > Cc: Keir (Xen.org); Ian Campbell; Tim (Xen.org); Ian Jackson; Jan Beulich > Subject: Re: [Xen-devel] [PATCH 2/3] public/io/netif.h: document control > ring and toeplitz hashing > > On 23/12/2015 10:06, Paul Durrant wrote: > > +#define NETIF_CTRL_RING_SIZE 1024 > > + > > +struct netif_ctrl_ring { > > + RING_IDX req_cons; > > + RING_IDX req_prod; > > + RING_IDX rsp_cons; > > + RING_IDX rsp_prod; > > + uint8_t req[NETIF_CTRL_RING_SIZE]; > > + uint8_t rsp[NETIF_CTRL_RING_SIZE]; > > To avoid making the same mistake as the xenstore ring, this at the very > minimum needs a defined reset protocol. It should also at least have a > version number (currently expected to be zero) which is used to > delineate the use of the remaining space in the page.
It doesn't need a reset protocol any more than the rx or tx rings do. xenstore is a special case, because you can't use xenstore to handle (re)connection (as you can in this case) ;-) Given the boolean feature flag in xenstore then I agree a version number could be useful... or the xenstore flag could be changed into a version number. > > > +}; > > + > > +struct xen_netif_ctrl_msg_hdr { > > + uint16_t type; > > + uint16_t len; > > These don't match your documentation above. uint32_t's ? Yikes, you're right. They should be uint32_ts. > > > +}; > > + > > +#define NETIF_CTRL_MSG_ACK 1 > > +#define NETIF_CTRL_MSG_GET_TOEPLITZ_FLAGS 2 > > +#define NETIF_CTRL_MSG_SET_TOEPLITZ_FLAGS 3 > > +#define NETIF_CTRL_MSG_SET_TOEPLITZ_KEY 4 > > +#define NETIF_CTRL_MSG_SET_TOEPLITZ_MAPPING 5 > > What about 0? Again learning from the xenstore case, can we define 0 as > explicitly an invalid value, so a page of zeroes doesn't appear to be a > valid sequence of messages. I thought 0 being invalid was kind of obvious from the fact I started at 1, but I'll make it explicit. > > > + > > +/* Control messages: */ > > + > > +/* > > + * NETIF_CTRL_MSG_ACK: > > + * > > + * This is the only valid type of message sent by the backend to the > > + * frontend. It carries a payload of the following format: > > + * > > + * 0 1 2 3 4 5 6 7 octet > > + * +-----+-----+-----+-----+-----+-----+-----+-----+ > > Can I recommend that all ack packets contain the control type they are > responding to. In the normal case, it indeed shouldn't be needed, but > if the front and back ever get out of sync, it will make debugging far > easier. Yep, that's a good idea. Paul > > ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel