[v5,5/7] Add vhost-user calls implementation

Each ioctl request of vhost-kernel has a vhost-user message equivalent,
which is sent it over the control socket.

The general approach is to copy the data from the supplied argument
pointer to a designated field in the message. If a file descriptor is
to be passed it should be placed also in the fds array for inclusion in
the sendmsd control header.

VHOST_SET_MEM_TABLE ignores the supplied vhost_memory structure and scans
the global ram_list for ram blocks wiht a valid fd field set. This would
be set when the -mem-path option with shared=on property is used.

Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
---
 hw/virtio/vhost-backend.c | 134 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 132 insertions(+), 2 deletions(-)

On Thu, Jan 09, 2014 at 03:59:59PM +0100, Antonios Motakis wrote:
> Each ioctl request of vhost-kernel has a vhost-user message equivalent,
> which is sent it over the control socket.
> 
> The general approach is to copy the data from the supplied argument
> pointer to a designated field in the message. If a file descriptor is
> to be passed it should be placed also in the fds array for inclusion in
> the sendmsd control header.

But why put it in the data part then? It seems useless to leak
local fd numbers.

> 
> VHOST_SET_MEM_TABLE ignores the supplied vhost_memory structure and scans
> the global ram_list for ram blocks wiht

typo

> a valid fd field set. This would
> be set when the -mem-path option with shared=on property is used.
> 
> Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> ---
>  hw/virtio/vhost-backend.c | 134 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 132 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index b33d35f..50ea307 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -81,8 +81,41 @@ typedef struct VhostUserMsg {
>  /* The version of the protocol we support */
>  #define VHOST_USER_VERSION    (0x1)
>  
> +static unsigned long int ioctl_to_vhost_user_request[VHOST_USER_MAX] = {
> +    -1, /* VHOST_USER_NONE */
> +    VHOST_GET_FEATURES, /* VHOST_USER_GET_FEATURES */
> +    VHOST_SET_FEATURES, /* VHOST_USER_SET_FEATURES */
> +    VHOST_SET_OWNER, /* VHOST_USER_SET_OWNER */
> +    VHOST_RESET_OWNER, /* VHOST_USER_RESET_OWNER */
> +    VHOST_SET_MEM_TABLE, /* VHOST_USER_SET_MEM_TABLE */
> +    VHOST_SET_LOG_BASE, /* VHOST_USER_SET_LOG_BASE */
> +    VHOST_SET_LOG_FD, /* VHOST_USER_SET_LOG_FD */
> +    VHOST_SET_VRING_NUM, /* VHOST_USER_SET_VRING_NUM */
> +    VHOST_SET_VRING_ADDR, /* VHOST_USER_SET_VRING_ADDR */
> +    VHOST_SET_VRING_BASE, /* VHOST_USER_SET_VRING_BASE */
> +    VHOST_GET_VRING_BASE, /* VHOST_USER_GET_VRING_BASE */
> +    VHOST_SET_VRING_KICK, /* VHOST_USER_SET_VRING_KICK */
> +    VHOST_SET_VRING_CALL, /* VHOST_USER_SET_VRING_CALL */
> +    VHOST_SET_VRING_ERR, /* VHOST_USER_SET_VRING_ERR */
> +    VHOST_NET_SET_BACKEND, /* VHOST_USER_NET_SET_BACKEND */
> +    -1 /* VHOST_USER_ECHO */
> +};
> +
>  static int vhost_user_cleanup(struct vhost_dev *dev);
>  
> +static VhostUserRequest vhost_user_request_translate(unsigned long int request)
> +{
> +    VhostUserRequest idx;
> +
> +    for (idx = 0; idx < VHOST_USER_MAX; idx++) {
> +        if (ioctl_to_vhost_user_request[idx] == request) {
> +            break;
> +        }
> +    }
> +
> +    return (idx == VHOST_USER_MAX) ? VHOST_USER_NONE : idx;
> +}
> +
>  static int vhost_user_recv(int fd, VhostUserMsg *msg)
>  {
>      ssize_t r;
> @@ -197,7 +230,8 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>  {
>      int fd = dev->control;
>      VhostUserMsg msg;
> -    int result = 0;
> +    RAMBlock *block = 0;
> +    int result = 0, need_reply = 0;
>      int fds[VHOST_MEMORY_MAX_NREGIONS];
>      size_t fd_num = 0;
>  
> @@ -207,11 +241,78 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>          return -1;
>      }
>  
> -    msg.request = VHOST_USER_NONE;
> +    msg.request = vhost_user_request_translate(request);
>      msg.flags = VHOST_USER_VERSION;
>      msg.size = 0;
>  
>      switch (request) {
> +    case VHOST_GET_FEATURES:
> +    case VHOST_GET_VRING_BASE:
> +        need_reply = 1;
> +        break;
> +
> +    case VHOST_SET_FEATURES:
> +    case VHOST_SET_LOG_BASE:
> +        msg.u64 = *((__u64 *) arg);
> +        msg.size = MEMB_SIZE(VhostUserMsg, u64);
> +        break;
> +
> +    case VHOST_SET_OWNER:
> +    case VHOST_RESET_OWNER:
> +        break;
> +
> +    case VHOST_SET_MEM_TABLE:
> +        QTAILQ_FOREACH(block, &ram_list.blocks, next)
> +        {
> +            if (block->fd > 0) {
> +                msg.memory.regions[fd_num].userspace_addr = (__u64) block->host;
> +                msg.memory.regions[fd_num].memory_size = block->length;
> +                msg.memory.regions[fd_num].guest_phys_addr = block->offset;
> +                fds[fd_num++] = block->fd;
> +            }
> +        }
> +
> +        msg.memory.nregions = fd_num;
> +
> +        if (!fd_num) {
> +            error_report("Failed initializing vhost-user memory map\n"
> +                    "consider using -mem-path option\n");
> +            return -1;
> +        }
> +
> +        msg.size = MEMB_SIZE(VhostUserMemory, nregions);
> +        msg.size += MEMB_SIZE(VhostUserMemory, padding);
> +        msg.size += fd_num*sizeof(VhostUserMemoryRegion);

need space around *
> +
> +        break;
> +
> +    case VHOST_SET_LOG_FD:
> +        msg.fd = *((int *) arg);
> +        msg.size = MEMB_SIZE(VhostUserMsg, fd);
> +        break;
> +
> +    case VHOST_SET_VRING_NUM:
> +    case VHOST_SET_VRING_BASE:
> +        memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> +        msg.size = MEMB_SIZE(VhostUserMsg, state);
> +        break;
> +
> +    case VHOST_SET_VRING_ADDR:
> +        memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
> +        msg.size = MEMB_SIZE(VhostUserMsg, addr);
> +        break;
> +
> +    case VHOST_SET_VRING_KICK:
> +    case VHOST_SET_VRING_CALL:
> +    case VHOST_SET_VRING_ERR:
> +    case VHOST_NET_SET_BACKEND:
> +        memcpy(&msg.file, arg, sizeof(struct vhost_vring_file));
> +        msg.size = MEMB_SIZE(VhostUserMsg, file);
> +        if (msg.file.fd > 0) {
> +            fds[0] = msg.file.fd;
> +            fd_num = 1;
> +        }
> +        break;
>      default:
>          error_report("vhost-user trying to send unhandled ioctl\n");
>          return -1;
> @@ -220,6 +321,35 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
>  
>      result = vhost_user_send_fds(fd, &msg, fds, fd_num);
>  
> +    if (!result && need_reply) {
> +        result = vhost_user_recv(fd, &msg);

shouldn't ypou check result here before using msg?


> +
> +        if ((msg.flags & VHOST_USER_REPLY_MASK) == 0 ||
> +            (msg.flags & VHOST_USER_VERSION_MASK) != VHOST_USER_VERSION) {
> +            error_report("Received bad msg.\n");
> +            return -1;
> +        }
> +
> +        if (!result) {
> +            switch (request) {
> +            case VHOST_GET_FEATURES:
> +                if (msg.size != MEMB_SIZE(VhostUserMsg, u64)) {
> +                    error_report("Received bad msg.\n");
> +                    return -1;
> +                }
> +                *((__u64 *) arg) = msg.u64;
> +                break;
> +            case VHOST_GET_VRING_BASE:
> +                if (msg.size != MEMB_SIZE(VhostUserMsg, state)) {
> +                    error_report("Received bad msg.\n");
> +                    return -1;
> +                }
> +                memcpy(arg, &msg.state, sizeof(struct vhost_vring_state));
> +                break;
> +            }
> +        }
> +    }
> +
>      return result;
>  }
>  
> -- 
> 1.8.3.2
>

On Thu, Jan 9, 2014 at 4:47 PM, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Thu, Jan 09, 2014 at 03:59:59PM +0100, Antonios Motakis wrote:
> > Each ioctl request of vhost-kernel has a vhost-user message equivalent,
> > which is sent it over the control socket.
> >
> > The general approach is to copy the data from the supplied argument
> > pointer to a designated field in the message. If a file descriptor is
> > to be passed it should be placed also in the fds array for inclusion in
> > the sendmsd control header.
>
> But why put it in the data part then? It seems useless to leak
> local fd numbers.
>
> >
> > VHOST_SET_MEM_TABLE ignores the supplied vhost_memory structure and scans
> > the global ram_list for ram blocks wiht
>
> typo
>
> > a valid fd field set. This would
> > be set when the -mem-path option with shared=on property is used.
> >
> > Signed-off-by: Antonios Motakis <a.motakis@virtualopensystems.com>
> > Signed-off-by: Nikolay Nikolaev <n.nikolaev@virtualopensystems.com>
> > ---
> >  hw/virtio/vhost-backend.c | 134
> +++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 132 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > index b33d35f..50ea307 100644
> > --- a/hw/virtio/vhost-backend.c
> > +++ b/hw/virtio/vhost-backend.c
> > @@ -81,8 +81,41 @@ typedef struct VhostUserMsg {
> >  /* The version of the protocol we support */
> >  #define VHOST_USER_VERSION    (0x1)
> >
> > +static unsigned long int ioctl_to_vhost_user_request[VHOST_USER_MAX] = {
> > +    -1, /* VHOST_USER_NONE */
> > +    VHOST_GET_FEATURES, /* VHOST_USER_GET_FEATURES */
> > +    VHOST_SET_FEATURES, /* VHOST_USER_SET_FEATURES */
> > +    VHOST_SET_OWNER, /* VHOST_USER_SET_OWNER */
> > +    VHOST_RESET_OWNER, /* VHOST_USER_RESET_OWNER */
> > +    VHOST_SET_MEM_TABLE, /* VHOST_USER_SET_MEM_TABLE */
> > +    VHOST_SET_LOG_BASE, /* VHOST_USER_SET_LOG_BASE */
> > +    VHOST_SET_LOG_FD, /* VHOST_USER_SET_LOG_FD */
> > +    VHOST_SET_VRING_NUM, /* VHOST_USER_SET_VRING_NUM */
> > +    VHOST_SET_VRING_ADDR, /* VHOST_USER_SET_VRING_ADDR */
> > +    VHOST_SET_VRING_BASE, /* VHOST_USER_SET_VRING_BASE */
> > +    VHOST_GET_VRING_BASE, /* VHOST_USER_GET_VRING_BASE */
> > +    VHOST_SET_VRING_KICK, /* VHOST_USER_SET_VRING_KICK */
> > +    VHOST_SET_VRING_CALL, /* VHOST_USER_SET_VRING_CALL */
> > +    VHOST_SET_VRING_ERR, /* VHOST_USER_SET_VRING_ERR */
> > +    VHOST_NET_SET_BACKEND, /* VHOST_USER_NET_SET_BACKEND */
> > +    -1 /* VHOST_USER_ECHO */
> > +};
> > +
> >  static int vhost_user_cleanup(struct vhost_dev *dev);
> >
> > +static VhostUserRequest vhost_user_request_translate(unsigned long int
> request)
> > +{
> > +    VhostUserRequest idx;
> > +
> > +    for (idx = 0; idx < VHOST_USER_MAX; idx++) {
> > +        if (ioctl_to_vhost_user_request[idx] == request) {
> > +            break;
> > +        }
> > +    }
> > +
> > +    return (idx == VHOST_USER_MAX) ? VHOST_USER_NONE : idx;
> > +}
> > +
> >  static int vhost_user_recv(int fd, VhostUserMsg *msg)
> >  {
> >      ssize_t r;
> > @@ -197,7 +230,8 @@ static int vhost_user_call(struct vhost_dev *dev,
> unsigned long int request,
> >  {
> >      int fd = dev->control;
> >      VhostUserMsg msg;
> > -    int result = 0;
> > +    RAMBlock *block = 0;
> > +    int result = 0, need_reply = 0;
> >      int fds[VHOST_MEMORY_MAX_NREGIONS];
> >      size_t fd_num = 0;
> >
> > @@ -207,11 +241,78 @@ static int vhost_user_call(struct vhost_dev *dev,
> unsigned long int request,
> >          return -1;
> >      }
> >
> > -    msg.request = VHOST_USER_NONE;
> > +    msg.request = vhost_user_request_translate(request);
> >      msg.flags = VHOST_USER_VERSION;
> >      msg.size = 0;
> >
> >      switch (request) {
> > +    case VHOST_GET_FEATURES:
> > +    case VHOST_GET_VRING_BASE:
> > +        need_reply = 1;
> > +        break;
> > +
> > +    case VHOST_SET_FEATURES:
> > +    case VHOST_SET_LOG_BASE:
> > +        msg.u64 = *((__u64 *) arg);
> > +        msg.size = MEMB_SIZE(VhostUserMsg, u64);
> > +        break;
> > +
> > +    case VHOST_SET_OWNER:
> > +    case VHOST_RESET_OWNER:
> > +        break;
> > +
> > +    case VHOST_SET_MEM_TABLE:
> > +        QTAILQ_FOREACH(block, &ram_list.blocks, next)
> > +        {
> > +            if (block->fd > 0) {
> > +                msg.memory.regions[fd_num].userspace_addr = (__u64)
> block->host;
> > +                msg.memory.regions[fd_num].memory_size = block->length;
> > +                msg.memory.regions[fd_num].guest_phys_addr =
> block->offset;
> > +                fds[fd_num++] = block->fd;
> > +            }
> > +        }
> > +
> > +        msg.memory.nregions = fd_num;
> > +
> > +        if (!fd_num) {
> > +            error_report("Failed initializing vhost-user memory map\n"
> > +                    "consider using -mem-path option\n");
> > +            return -1;
> > +        }
> > +
> > +        msg.size = MEMB_SIZE(VhostUserMemory, nregions);
> > +        msg.size += MEMB_SIZE(VhostUserMemory, padding);
> > +        msg.size += fd_num*sizeof(VhostUserMemoryRegion);
>
> need space around *
> > +
> > +        break;
> > +
> > +    case VHOST_SET_LOG_FD:
> > +        msg.fd = *((int *) arg);
> > +        msg.size = MEMB_SIZE(VhostUserMsg, fd);
> > +        break;
> > +
> > +    case VHOST_SET_VRING_NUM:
> > +    case VHOST_SET_VRING_BASE:
> > +        memcpy(&msg.state, arg, sizeof(struct vhost_vring_state));
> > +        msg.size = MEMB_SIZE(VhostUserMsg, state);
> > +        break;
> > +
> > +    case VHOST_SET_VRING_ADDR:
> > +        memcpy(&msg.addr, arg, sizeof(struct vhost_vring_addr));
> > +        msg.size = MEMB_SIZE(VhostUserMsg, addr);
> > +        break;
> > +
> > +    case VHOST_SET_VRING_KICK:
> > +    case VHOST_SET_VRING_CALL:
> > +    case VHOST_SET_VRING_ERR:
> > +    case VHOST_NET_SET_BACKEND:
> > +        memcpy(&msg.file, arg, sizeof(struct vhost_vring_file));
> > +        msg.size = MEMB_SIZE(VhostUserMsg, file);
> > +        if (msg.file.fd > 0) {
> > +            fds[0] = msg.file.fd;
> > +            fd_num = 1;
> > +        }
> > +        break;
> >      default:
> >          error_report("vhost-user trying to send unhandled ioctl\n");
> >          return -1;
> > @@ -220,6 +321,35 @@ static int vhost_user_call(struct vhost_dev *dev,
> unsigned long int request,
> >
> >      result = vhost_user_send_fds(fd, &msg, fds, fd_num);
> >
> > +    if (!result && need_reply) {
> > +        result = vhost_user_recv(fd, &msg);
>
> shouldn't ypou check result here before using msg?
>
>
> > +
> > +        if ((msg.flags & VHOST_USER_REPLY_MASK) == 0 ||
> > +            (msg.flags & VHOST_USER_VERSION_MASK) !=
> VHOST_USER_VERSION) {
> > +            error_report("Received bad msg.\n");
> > +            return -1;
> > +        }
> > +
> > +        if (!result) {
> > +            switch (request) {
> > +            case VHOST_GET_FEATURES:
> > +                if (msg.size != MEMB_SIZE(VhostUserMsg, u64)) {
> > +                    error_report("Received bad msg.\n");
> > +                    return -1;
> > +                }
> > +                *((__u64 *) arg) = msg.u64;
> > +                break;
> > +            case VHOST_GET_VRING_BASE:
> > +                if (msg.size != MEMB_SIZE(VhostUserMsg, state)) {
> > +                    error_report("Received bad msg.\n");
> > +                    return -1;
> > +                }
> > +                memcpy(arg, &msg.state, sizeof(struct
> vhost_vring_state));
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> >      return result;
> >  }
> >
> > --
> > 1.8.3.2
> >
>

Ack.