Re: [Qemu-devel] [RFC PATCH v2] tests/vhost-user-bridge: add vhost-user bridge application

["Michael S. Tsirkin" <mst@redhat.com>]

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.