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

[Victor Kaplansky]

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.
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)}

> 
> > +    /* 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. IMO, the best here would be rmb()
+ compiler memory barrier to prevent compiler from hoisting stores beyond this
point.

> > +    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