Re: [PATCH net-next] vhost: switch to use new message format

2018-08-05 Thread Jason Wang



On 2018年08月03日 15:59, Michael S. Tsirkin wrote:

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index a502f1a..6f6c42d 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -315,6 +315,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
vq->log_addr = -1ull;
vq->private_data = NULL;
vq->acked_features = 0;
+   vq->acked_backend_features = 0;
vq->log_base = NULL;
vq->error_ctx = NULL;
vq->kick = NULL;
@@ -1027,28 +1028,40 @@ static int vhost_process_iotlb_msg(struct vhost_dev 
*dev,
  ssize_t vhost_chr_write_iter(struct vhost_dev *dev,
 struct iov_iter *from)
  {
-   struct vhost_msg_node node;
-   unsigned size = sizeof(struct vhost_msg);
-   size_t ret;
-   int err;
+   struct vhost_iotlb_msg msg;
+   size_t offset;
+   int type, ret;
  
-	if (iov_iter_count(from) < size)

-   return 0;
-   ret = copy_from_iter(, size, from);
-   if (ret != size)
+   ret = copy_from_iter(, sizeof(type), from);
+   if (ret != sizeof(type))
goto done;
  
-	switch (node.msg.type) {

+   switch (type) {
case VHOST_IOTLB_MSG:
-   err = vhost_process_iotlb_msg(dev, );
-   if (err)
-   ret = err;
+   /* There maybe a hole after type for V1 message type,
+* so skip it here.
+*/
+   offset = offsetof(struct vhost_msg, iotlb) - sizeof(int);
+   break;
+   case VHOST_IOTLB_MSG_V2:
+   offset = sizeof(__u32);
break;
default:
ret = -EINVAL;
-   break;
+   goto done;
+   }
+
+   iov_iter_advance(from, offset);
+   ret = copy_from_iter(, sizeof(msg), from);
+   if (ret != sizeof(msg))
+   goto done;
+   if (vhost_process_iotlb_msg(dev, )) {
+   ret = -EFAULT;
+   goto done;
}
  
+	ret = (type == VHOST_IOTLB_MSG) ? sizeof(struct vhost_msg) :

+ sizeof(struct vhost_msg_v2);
  done:
return ret;
  }

We can actually fix 32 bit apps too, checking the mode for v1.
But that can wait for another patch.



Yes, let me do it on top.

Thanks

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next] vhost: switch to use new message format

2018-08-05 Thread Jason Wang



On 2018年08月05日 04:21, David Miller wrote:

From: Jason Wang 
Date: Fri,  3 Aug 2018 15:04:51 +0800


So fixing this by introducing a new message type with an explicit
32bit reserved field after type like:

struct vhost_msg_v2 {
int type;
__u32 reserved;

Please use fixed sized types consistently.  Use 's32' instead of 'int'
here.

Thanks!


Ok, V2 will be posted soon.

And it looks to me u32 is sufficient.

Thanks
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next] vhost: switch to use new message format

2018-08-04 Thread David Miller
From: Jason Wang 
Date: Fri,  3 Aug 2018 15:04:51 +0800

> So fixing this by introducing a new message type with an explicit
> 32bit reserved field after type like:
> 
> struct vhost_msg_v2 {
>   int type;
>   __u32 reserved;

Please use fixed sized types consistently.  Use 's32' instead of 'int'
here.

Thanks!
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [PATCH net-next] vhost: switch to use new message format

2018-08-03 Thread Michael S. Tsirkin
On Fri, Aug 03, 2018 at 03:04:51PM +0800, Jason Wang wrote:
> We use to have message like:
> 
> struct vhost_msg {
>   int type;
>   union {
>   struct vhost_iotlb_msg iotlb;
>   __u8 padding[64];
>   };
> };
> 
> Unfortunately, there will be a hole of 32bit in 64bit machine because
> of the alignment. This leads a different formats between 32bit API and
> 64bit API. What's more it will break 32bit program running on 64bit
> machine.
> 
> So fixing this by introducing a new message type with an explicit
> 32bit reserved field after type like:
> 
> struct vhost_msg_v2 {
>   int type;
>   __u32 reserved;
>   union {
>   struct vhost_iotlb_msg iotlb;
>   __u8 padding[64];
>   };
> };
> 
> We will have a consistent ABI after switching to use this. To enable
> this capability, introduce a new ioctl (VHOST_SET_BAKCEND_FEATURE) for
> userspace to enable this feature (VHOST_BACKEND_F_IOTLB_V2).
> 
> Fixes: 6b1e6cc7855b ("vhost: new device IOTLB API")
> Signed-off-by: Jason Wang 
> ---
>  drivers/vhost/net.c| 30 
>  drivers/vhost/vhost.c  | 71 
> ++
>  drivers/vhost/vhost.h  | 11 ++-
>  include/uapi/linux/vhost.h | 18 
>  4 files changed, 111 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 367d802..4e656f8 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -78,6 +78,10 @@ enum {
>  };
>  
>  enum {
> + VHOST_NET_BACKEND_FEATURES = (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2)
> +};
> +
> +enum {
>   VHOST_NET_VQ_RX = 0,
>   VHOST_NET_VQ_TX = 1,
>   VHOST_NET_VQ_MAX = 2,
> @@ -1399,6 +1403,21 @@ static long vhost_net_reset_owner(struct vhost_net *n)
>   return err;
>  }
>  
> +static int vhost_net_set_backend_features(struct vhost_net *n, u64 features)
> +{
> + int i;
> +
> + mutex_lock(>dev.mutex);
> + for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
> + mutex_lock(>vqs[i].vq.mutex);
> + n->vqs[i].vq.acked_backend_features = features;
> + mutex_unlock(>vqs[i].vq.mutex);
> + }
> + mutex_unlock(>dev.mutex);
> +
> + return 0;
> +}
> +
>  static int vhost_net_set_features(struct vhost_net *n, u64 features)
>  {
>   size_t vhost_hlen, sock_hlen, hdr_len;
> @@ -1489,6 +1508,17 @@ static long vhost_net_ioctl(struct file *f, unsigned 
> int ioctl,
>   if (features & ~VHOST_NET_FEATURES)
>   return -EOPNOTSUPP;
>   return vhost_net_set_features(n, features);
> + case VHOST_GET_BACKEND_FEATURES:
> + features = VHOST_NET_BACKEND_FEATURES;
> + if (copy_to_user(featurep, , sizeof(features)))
> + return -EFAULT;
> + return 0;
> + case VHOST_SET_BACKEND_FEATURES:
> + if (copy_from_user(, featurep, sizeof(features)))
> + return -EFAULT;
> + if (features & ~VHOST_NET_BACKEND_FEATURES)
> + return -EOPNOTSUPP;
> + return vhost_net_set_backend_features(n, features);
>   case VHOST_RESET_OWNER:
>   return vhost_net_reset_owner(n);
>   case VHOST_SET_OWNER:
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index a502f1a..6f6c42d 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -315,6 +315,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
>   vq->log_addr = -1ull;
>   vq->private_data = NULL;
>   vq->acked_features = 0;
> + vq->acked_backend_features = 0;
>   vq->log_base = NULL;
>   vq->error_ctx = NULL;
>   vq->kick = NULL;
> @@ -1027,28 +1028,40 @@ static int vhost_process_iotlb_msg(struct vhost_dev 
> *dev,
>  ssize_t vhost_chr_write_iter(struct vhost_dev *dev,
>struct iov_iter *from)
>  {
> - struct vhost_msg_node node;
> - unsigned size = sizeof(struct vhost_msg);
> - size_t ret;
> - int err;
> + struct vhost_iotlb_msg msg;
> + size_t offset;
> + int type, ret;
>  
> - if (iov_iter_count(from) < size)
> - return 0;
> - ret = copy_from_iter(, size, from);
> - if (ret != size)
> + ret = copy_from_iter(, sizeof(type), from);
> + if (ret != sizeof(type))
>   goto done;
>  
> - switch (node.msg.type) {
> + switch (type) {
>   case VHOST_IOTLB_MSG:
> - err = vhost_process_iotlb_msg(dev, );
> - if (err)
> - ret = err;
> + /* There maybe a hole after type for V1 message type,
> +  * so skip it here.
> +  */
> + offset = offsetof(struct vhost_msg, iotlb) - sizeof(int);
> + break;
> + case VHOST_IOTLB_MSG_V2:
> + offset = sizeof(__u32);
>   break;
>   default:
>   ret = -EINVAL;
> - break;
> +