On Tue, Oct 27, 2015 at 03:29:21PM +0200, Victor Kaplansky wrote:
> On Tue, Oct 27, 2015 at 01:40:03PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 26, 2015 at 07:19:23PM +0200, Victor Kaplansky wrote:
> > > +
> > > +#define VHOST_USER_PROTOCOL_FEATURE_MASK ((1 <<
> > > VHOST_USER_PROTOCOL_F_MAX) - 1)
> > > +
> > > +typedef enum VhostUserRequest {
> > > + VHOST_USER_NONE = 0,
> > > + VHOST_USER_GET_FEATURES = 1,
> > > + VHOST_USER_SET_FEATURES = 2,
> > > + VHOST_USER_SET_OWNER = 3,
> > > + VHOST_USER_RESET_DEVICE = 4,
> > > + VHOST_USER_SET_MEM_TABLE = 5,
> > > + VHOST_USER_SET_LOG_BASE = 6,
> > > + VHOST_USER_SET_LOG_FD = 7,
> > > + VHOST_USER_SET_VRING_NUM = 8,
> > > + VHOST_USER_SET_VRING_ADDR = 9,
> > > + VHOST_USER_SET_VRING_BASE = 10,
> > > + VHOST_USER_GET_VRING_BASE = 11,
> > > + VHOST_USER_SET_VRING_KICK = 12,
> > > + VHOST_USER_SET_VRING_CALL = 13,
> > > + VHOST_USER_SET_VRING_ERR = 14,
> > > + VHOST_USER_GET_PROTOCOL_FEATURES = 15,
> > > + VHOST_USER_SET_PROTOCOL_FEATURES = 16,
> > > + VHOST_USER_GET_QUEUE_NUM = 17,
> > > + VHOST_USER_SET_VRING_ENABLE = 18,
> > > + VHOST_USER_SEND_RARP = 19,
> > > + VHOST_USER_MAX
> > > +} VhostUserRequest;
> >
> >
> > Maybe we need a common copy under tests/
>
> Definitely. Even better to have a common header with linux.
With QEMU? It's a matter of deciding whether it's a good idea.
If we keep a separate copy in tests, tests will break if we
change the ABI. Otherwise they will be rebuilt and pass
by mistake.
> Can be done as a separate patch?
>
> > > +static void
> > > +vubr_post_buffer(VhostDev *dev,
> > > + VirtQueue *vq,
> > > + uint8_t *buf,
> > > + int32_t len)
> > > +{
> > > + struct vring_desc *desc = vq->desc;
> > > + struct vring_avail *avail = vq->avail;
> > > + struct vring_used *used = vq->used;
> > > +
> > > + unsigned int size = vq->size;
> > > +
> > > + assert(vq->last_avail_index != avail->idx);
> >
> > Why? How do you know there's anything there?
>
> Right now the post_buffer() is unable to cope with full (or,
> rather empty) RX queue, thus the assumption that RX always have
> available descriptors. I'll add an explanation, and replace
> assert() by {fprintf(stderr, ...); exit(1)}
This doesn't matter - but what makes sure it's only invoked
if buffer is not empty?
> >
> > > + /* Prevent accessing descriptors, buffers, avail->ring and used
> > > before
> > > + * avail->idx */
> >
> > smp_rmb then? Can be fixed later ...
>
> 'used' has been accessed by stores, consequently, at least in theory, if you
> take conservative side, rmb() is not enough.
I don't think there's a race there. vhost in kernel has an rmb here
so if there's a problem I'd like to know what it is.
But let's make it correct first.
> IMO, the best here would be rmb()
> + compiler memory barrier to prevent compiler from hoisting stores beyond this
> point.
AFAIK it's implicit in the fact we have asm with no inputs or outputs.
IOW all barriers include the compiler barrier.
> > > + smp_mb();
> > > +
> > > + uint16_t a_index = vq->last_avail_index % size;
> > > + uint16_t u_index = vq->last_used_index % size;
> > > + uint16_t d_index = avail->ring[a_index];
> > > +
> > > + int i = d_index;
> > > +
> > > +
>
> > > +static int
> > > +vhost_user_none_exec(VhostDev *dev,
> > > + VhostUserMsg *vmsg)
> > > +{
> > > + printf("Function %s() not implemented yet.\n", __func__);
> > > + return 0;
> > > +}
> > > +
> > > +static int
> > > +vhost_user_get_features_exec(VhostDev *dev,
> > > + VhostUserMsg *vmsg)
> >
> > Please prefix everything with vubr_ consistently.
> > Same applies to types etc.
>
> I tried to change everything to use vhost_user_ prefix, but some
> functions have slipped my attention and still use vubr_ prefix.
> I'll fix this. Thanks for noticing this.
>
> -- Victor
Please don't use vhost_user_ prefix! That's used by vhost user code in
QEMU. Either vhost_user_bridge_ or vubr_ or whatever.
