Re: [PATCH net-next] vhost: switch to use new message format
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
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
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
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; > +
[PATCH net-next] vhost: switch to use new message format
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; + goto done; + } + + iov_iter_advance(from, offset); + ret = copy_from_iter(, sizeof(msg), from); + if (ret != sizeof(msg)) + goto done; + if