[PATCH v1 0/4] vduse: Reconnection support in vduse

[PATCH v1 0/4] vduse: Reconnection support in vduse

[Cindy Lu]

Here is the reconnect support in vduse, 

The kernel will allocate pages for reconnection
userspace needs to use ioctl VDUSE_GET_RECONNECT_INFO to 
get the mmap related infomation and then map these pages
to userspace. 
The kernel and userspace will use these pages to sync
the reconnect information

Tested in vduse + dpdk test-pmd

Cindy Lu (4):
  vduse: Add function to get/free the pages for reconnection
  vduse: Add file operation for mmap
  vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO
  vduse: update the vq_info in ioctl

 drivers/vdpa/vdpa_user/vduse_dev.c | 175 +++++++++++++++++++++++++++++
 include/uapi/linux/vduse.h         |  43 +++++++
 2 files changed, 218 insertions(+)

-- 
2.34.3

[PATCH v1 1/4] vduse: Add function to get/free the pages for reconnection

[Cindy Lu]

Add the function vduse_alloc_reconnnect_info_mem
and vduse_alloc_reconnnect_info_mem
In this 2 function, vduse will get/free (vq_num + 1)*page  
Page 0 will be used to save the reconnection information, The
Userspace App will maintain this. Page 1 ~ vq_num + 1 will save
the reconnection information for vqs.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 78 ++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 26b7e29cb900..565126a9ab01 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -30,6 +30,10 @@
 #include <uapi/linux/virtio_blk.h>
 #include <linux/mod_devicetable.h>
 
+#ifdef CONFIG_X86
+#include <asm/set_memory.h>
+#endif
+
 #include "iova_domain.h"
 
 #define DRV_AUTHOR   "Yongji Xie <xieyongji@bytedance.com>"
@@ -41,6 +45,19 @@
 #define VDUSE_IOVA_SIZE (128 * 1024 * 1024)
 #define VDUSE_MSG_DEFAULT_TIMEOUT 30
 
+/* struct vdpa_reconnect_info saved the alloc pages info
+ * these pages will mmaped to userspace for reconnection
+ */
+struct vdpa_reconnect_info {
+	/* Offset (within vm_file) in PAGE_SIZE
+	 */
+	u32 index;
+	/* virtual address for this page*/
+	unsigned long vaddr;
+	/* allocated memory size, */
+	phys_addr_t size;
+};
+
 struct vduse_virtqueue {
 	u16 index;
 	u16 num_max;
@@ -57,6 +74,7 @@ struct vduse_virtqueue {
 	struct vdpa_callback cb;
 	struct work_struct inject;
 	struct work_struct kick;
+	struct vdpa_reconnect_info reconnect_info;
 };
 
 struct vduse_dev;
@@ -106,6 +124,7 @@ struct vduse_dev {
 	u32 vq_align;
 	struct vduse_umem *umem;
 	struct mutex mem_lock;
+	struct vdpa_reconnect_info reconnect_status;
 };
 
 struct vduse_dev_msg {
@@ -1030,6 +1049,61 @@ static int vduse_dev_reg_umem(struct vduse_dev *dev,
 	return ret;
 }
 
+static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
+{
+	struct vdpa_reconnect_info *info;
+	struct vduse_virtqueue *vq;
+	void *addr;
+
+	/*page 0 is use to save status,dpdk will use this to save the information
+	 *needed in reconnection,kernel don't need to maintain this
+	 */
+	info = &dev->reconnect_status;
+	addr = (void *)get_zeroed_page(GFP_KERNEL);
+	if (!addr)
+		return -1;
+
+	info->vaddr = (unsigned long)(addr);
+	info->size = PAGE_SIZE;
+	/* index is vm Offset in PAGE_SIZE */
+	info->index = 0;
+
+	/*page 1~ vq_num + 1 save the reconnect info for vqs*/
+	for (int i = 0; i < dev->vq_num + 1; i++) {
+		vq = &dev->vqs[i];
+		info = &vq->reconnect_info;
+		addr = (void *)get_zeroed_page(GFP_KERNEL);
+		if (!addr)
+			return -1;
+
+		info->vaddr = (unsigned long)(addr);
+		info->size = PAGE_SIZE;
+		info->index = i + 1;
+	}
+
+	return 0;
+}
+
+static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
+{
+	struct vdpa_reconnect_info *info;
+	struct vduse_virtqueue *vq;
+
+	info = &dev->reconnect_status;
+	free_page(info->vaddr);
+	info->size = 0;
+	info->vaddr = 0;
+	for (int i = 0; i < dev->vq_num + 1; i++) {
+		vq = &dev->vqs[i];
+		info = &vq->reconnect_info;
+		free_page(info->vaddr);
+		info->size = 0;
+		info->vaddr = 0;
+	}
+
+	return 0;
+}
+
 static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg)
 {
@@ -1390,6 +1464,8 @@ static int vduse_destroy_dev(char *name)
 		mutex_unlock(&dev->lock);
 		return -EBUSY;
 	}
+	vduse_free_reconnnect_info_mem(dev);
+
 	dev->connected = true;
 	mutex_unlock(&dev->lock);
 
@@ -1542,6 +1618,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
 		ret = PTR_ERR(dev->dev);
 		goto err_dev;
 	}
+
+	vduse_alloc_reconnnect_info_mem(dev);
 	__module_get(THIS_MODULE);
 
 	return 0;
-- 
2.34.3

[PATCH v1 2/4] vduse: Add file operation for mmap

[Cindy Lu]

Add the operation for mmap, The user space APP will
use this function to map the pages to userspace

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 66 ++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 565126a9ab01..05e72d752fb6 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1380,6 +1380,70 @@ static struct vduse_dev *vduse_dev_get_from_minor(int minor)
 	return dev;
 }
 
+static vm_fault_t vduse_vm_fault(struct vm_fault *vmf)
+{
+	struct vduse_dev *dev = vmf->vma->vm_file->private_data;
+	struct vm_area_struct *vma = vmf->vma;
+	u16 index = vma->vm_pgoff;
+	struct vduse_virtqueue *vq;
+	struct vdpa_reconnect_info *info;
+
+	/* index 0  page reserved for vduse status*/
+	if (index == 0) {
+		info = &dev->reconnect_status;
+	} else {
+		/* index 1+vq_number page reserved for vduse vqs*/
+		vq = &dev->vqs[index - 1];
+		info = &vq->reconnect_info;
+	}
+	if (remap_pfn_range(vma, vmf->address & PAGE_MASK,
+			    PFN_DOWN(virt_to_phys((void *)info->vaddr)),
+			    PAGE_SIZE, vma->vm_page_prot))
+		return VM_FAULT_SIGBUS;
+	return VM_FAULT_NOPAGE;
+}
+
+static const struct vm_operations_struct vduse_vm_ops = {
+	.fault = vduse_vm_fault,
+};
+
+static int vduse_dev_mmap(struct file *file, struct vm_area_struct *vma)
+{
+	struct vduse_dev *dev = file->private_data;
+	struct vdpa_reconnect_info *info;
+	unsigned long index = vma->vm_pgoff;
+	struct vduse_virtqueue *vq;
+
+	if (vma->vm_end - vma->vm_start != PAGE_SIZE)
+		return -EINVAL;
+	if ((vma->vm_flags & VM_SHARED) == 0)
+		return -EINVAL;
+
+	if (index > dev->vq_num + 1)
+		return -EINVAL;
+
+	/* index 0  page reserved for vduse status*/
+	if (index == 0) {
+		info = &dev->reconnect_status;
+	} else {
+		/* index 1+vq_number page reserved for vduse vqs*/
+		vq = &dev->vqs[index - 1];
+		info = &vq->reconnect_info;
+	}
+	if (info->vaddr == 0)
+		return -ENOTSUPP;
+
+	if (virt_to_phys((void *)info->vaddr) & (PAGE_SIZE - 1))
+		return -EINVAL;
+	if (vma->vm_end - vma->vm_start != info->size)
+		return -EOPNOTSUPP;
+
+	vm_flags_set(vma, VM_IO | VM_DONTDUMP);
+	vma->vm_ops = &vduse_vm_ops;
+
+	return 0;
+}
+
 static int vduse_dev_open(struct inode *inode, struct file *file)
 {
 	int ret;
@@ -1412,6 +1476,8 @@ static const struct file_operations vduse_dev_fops = {
 	.unlocked_ioctl	= vduse_dev_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
 	.llseek		= noop_llseek,
+	.mmap		= vduse_dev_mmap,
+
 };
 
 static struct vduse_dev *vduse_dev_create(void)
-- 
2.34.3

[PATCH v1 3/4] vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO

[Cindy Lu]

In VDUSE_GET_RECONNECT_INFO, the Userspace App can get the map size
and The number of mapping memory pages from the kernel. The userspace
App can use this information to map the pages.

Add struct vhost_reconnect_data/vhost_reconnect_vring for sync the
information in reconnection

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 15 +++++++++++
 include/uapi/linux/vduse.h         | 43 ++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 05e72d752fb6..0f15e7ac716b 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1347,6 +1347,21 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 		ret = 0;
 		break;
 	}
+	case VDUSE_GET_RECONNECT_INFO: {
+		struct vduse_reconnect_mmap_info info;
+
+		ret = -EFAULT;
+		if (copy_from_user(&info, argp, sizeof(info)))
+			break;
+
+		info.size = PAGE_SIZE;
+		info.max_index = dev->vq_num + 1;
+
+		if (copy_to_user(argp, &info, sizeof(info)))
+			break;
+		ret = 0;
+		break;
+	}
 	default:
 		ret = -ENOIOCTLCMD;
 		break;
diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
index 11bd48c72c6c..5ccac535fba6 100644
--- a/include/uapi/linux/vduse.h
+++ b/include/uapi/linux/vduse.h
@@ -350,4 +350,47 @@ struct vduse_dev_response {
 	};
 };
 
+/**
+ * struct vhost_reconnect_data - saved the reconnect info for device
+ * @reconnect_time: reconnect time for this device. userspace APP needs to do ++
+ *                  while reconnecting
+ * @version; version for userspace APP
+ * @features; Device features negotiated in the last reconnect.
+ * @status; Device status in last reconnect
+ * @nr_vrings; number of vqs
+ */
+
+struct vhost_reconnect_data {
+	__u32 reconnect_time;
+	__u32 version;
+	__u64 features;
+	__u8 status;
+	__u32 nr_vrings;
+};
+
+/**
+ * struct vhost_reconnect_vring -saved the reconnect info for vqs
+ * @last_avail_idx: device available index
+ * @avail_wrap_counter: Driver ring wrap counter
+ */
+struct vhost_reconnect_vring {
+	__u16 last_avail_idx;
+	__u16 avail_wrap_counter;
+};
+
+/**
+ * struct vduse_reconnect_mmap_info
+ * @size: mapping memory size, here we use page_size
+ * @max_index: the number of pages allocated in kernel,just
+ * use for check
+ */
+
+struct vduse_reconnect_mmap_info {
+	__u32 size;
+	__u32 max_index;
+};
+
+#define VDUSE_GET_RECONNECT_INFO \
+	_IOWR(VDUSE_BASE, 0x1b, struct vduse_reconnect_mmap_info)
+
 #endif /* _UAPI_VDUSE_H_ */
-- 
2.34.3

[PATCH v1 4/4] vduse: update the vq_info in ioctl

[Cindy Lu]

In VDUSE_VQ_GET_INFO, the driver will sync the last_avail_idx
with reconnect info, After mapping the reconnect pages to userspace
The userspace App will update the reconnect_time in
struct vhost_reconnect_vring, If this is not 0 then it means this
vq is reconnected and will update the last_avail_idx

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 0f15e7ac716b..42e7a90ab74c 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1213,6 +1213,9 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 		struct vduse_vq_info vq_info;
 		struct vduse_virtqueue *vq;
 		u32 index;
+		struct vdpa_reconnect_info *area;
+		struct vhost_reconnect_vring *vq_reconnect;
+		struct vhost_reconnect_data *dev_reconnect;
 
 		ret = -EFAULT;
 		if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
@@ -1244,6 +1247,19 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 
 		vq_info.ready = vq->ready;
 
+		area = &dev->reconnect_status;
+		dev_reconnect = (struct vhost_reconnect_data *)area->vaddr;
+
+		area = &vq->reconnect_info;
+		vq_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
+		/*check if the vq is reconnect, if yes then update the last_avail_idx*/
+		if ((vq_reconnect->last_avail_idx !=
+		     vq_info.split.avail_index) &&
+		    (dev_reconnect->reconnect_time != 0)) {
+			vq_info.split.avail_index =
+				vq_reconnect->last_avail_idx;
+		}
+
 		ret = -EFAULT;
 		if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
 			break;
-- 
2.34.3

Re: [PATCH v1 0/4] vduse: Reconnection support in vduse

[Jason Wang]

On Wed, Oct 11, 2023 at 2:42 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Here is the reconnect support in vduse,
>
> The kernel will allocate pages for reconnection
> userspace needs to use ioctl VDUSE_GET_RECONNECT_INFO to
> get the mmap related infomation and then map these pages
> to userspace.
> The kernel and userspace will use these pages to sync
> the reconnect information
>
> Tested in vduse + dpdk test-pmd

I would like to see the DPDK part as a reference. Would you mind
giving me a pointer?

Thanks

>
> Cindy Lu (4):
>   vduse: Add function to get/free the pages for reconnection
>   vduse: Add file operation for mmap
>   vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO
>   vduse: update the vq_info in ioctl
>
>  drivers/vdpa/vdpa_user/vduse_dev.c | 175 +++++++++++++++++++++++++++++
>  include/uapi/linux/vduse.h         |  43 +++++++
>  2 files changed, 218 insertions(+)
>
> --
> 2.34.3
>

Re: [PATCH v1 0/4] vduse: Reconnection support in vduse

[Cindy Lu]

On Tue, Oct 17, 2023 at 10:44 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Oct 11, 2023 at 2:42 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Here is the reconnect support in vduse,
> >
> > The kernel will allocate pages for reconnection
> > userspace needs to use ioctl VDUSE_GET_RECONNECT_INFO to
> > get the mmap related infomation and then map these pages
> > to userspace.
> > The kernel and userspace will use these pages to sync
> > the reconnect information
> >
> > Tested in vduse + dpdk test-pmd
>
> I would like to see the DPDK part as a reference. Would you mind
> giving me a pointer?
>
> Thanks
>
sure, Will send the dpdk patch soon
thanks
cindy
> >
> > Cindy Lu (4):
> >   vduse: Add function to get/free the pages for reconnection
> >   vduse: Add file operation for mmap
> >   vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO
> >   vduse: update the vq_info in ioctl
> >
> >  drivers/vdpa/vdpa_user/vduse_dev.c | 175 +++++++++++++++++++++++++++++
> >  include/uapi/linux/vduse.h         |  43 +++++++
> >  2 files changed, 218 insertions(+)
> >
> > --
> > 2.34.3
> >
>

Re: [PATCH v1 1/4] vduse: Add function to get/free the pages for reconnection

[Jason Wang]

On Wed, Oct 11, 2023 at 2:42 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Add the function vduse_alloc_reconnnect_info_mem
> and vduse_alloc_reconnnect_info_mem
> In this 2 function, vduse will get/free (vq_num + 1)*page
> Page 0 will be used to save the reconnection information, The
> Userspace App will maintain this. Page 1 ~ vq_num + 1 will save
> the reconnection information for vqs.

I'd align this with the spec. E.g spec starts virtqueue index from 0.

>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 78 ++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 26b7e29cb900..565126a9ab01 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -30,6 +30,10 @@
>  #include <uapi/linux/virtio_blk.h>
>  #include <linux/mod_devicetable.h>
>
> +#ifdef CONFIG_X86
> +#include <asm/set_memory.h>
> +#endif

Why do we need special care for x86?

> +
>  #include "iova_domain.h"
>
>  #define DRV_AUTHOR   "Yongji Xie <xieyongji@bytedance.com>"
> @@ -41,6 +45,19 @@
>  #define VDUSE_IOVA_SIZE (128 * 1024 * 1024)
>  #define VDUSE_MSG_DEFAULT_TIMEOUT 30
>
> +/* struct vdpa_reconnect_info saved the alloc pages info
> + * these pages will mmaped to userspace for reconnection
> + */
> +struct vdpa_reconnect_info {
> +       /* Offset (within vm_file) in PAGE_SIZE
> +        */
> +       u32 index;
> +       /* virtual address for this page*/
> +       unsigned long vaddr;
> +       /* allocated memory size, */
> +       phys_addr_t size;

Anything wrong with using "unsigned long" here.

Btw, what is "allocated memory size" since I think we can use up to
PAGE_SIZE here?

> +};
> +
>  struct vduse_virtqueue {
>         u16 index;
>         u16 num_max;
> @@ -57,6 +74,7 @@ struct vduse_virtqueue {
>         struct vdpa_callback cb;
>         struct work_struct inject;
>         struct work_struct kick;
> +       struct vdpa_reconnect_info reconnect_info;
>  };
>
>  struct vduse_dev;
> @@ -106,6 +124,7 @@ struct vduse_dev {
>         u32 vq_align;
>         struct vduse_umem *umem;
>         struct mutex mem_lock;
> +       struct vdpa_reconnect_info reconnect_status;
>  };
>
>  struct vduse_dev_msg {
> @@ -1030,6 +1049,61 @@ static int vduse_dev_reg_umem(struct vduse_dev *dev,
>         return ret;
>  }
>
> +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> +{
> +       struct vdpa_reconnect_info *info;
> +       struct vduse_virtqueue *vq;
> +       void *addr;
> +
> +       /*page 0 is use to save status,dpdk will use this to save the information
> +        *needed in reconnection,kernel don't need to maintain this
> +        */

Let's avoid mentioning any specific userspace like DPDK in the kernel
source. A good uAPI should not be designed for a specific userspace.

> +       info = &dev->reconnect_status;
> +       addr = (void *)get_zeroed_page(GFP_KERNEL);
> +       if (!addr)
> +               return -1;

Let's don't use magic number here but something like -ENOMEM.

> +
> +       info->vaddr = (unsigned long)(addr);
> +       info->size = PAGE_SIZE;
> +       /* index is vm Offset in PAGE_SIZE */
> +       info->index = 0;
> +
> +       /*page 1~ vq_num + 1 save the reconnect info for vqs*/

There's no need to explain what the code does.

> +       for (int i = 0; i < dev->vq_num + 1; i++) {
> +               vq = &dev->vqs[i];
> +               info = &vq->reconnect_info;
> +               addr = (void *)get_zeroed_page(GFP_KERNEL);
> +               if (!addr)
> +                       return -1;
> +
> +               info->vaddr = (unsigned long)(addr);
> +               info->size = PAGE_SIZE;
> +               info->index = i + 1;

Btw, I don't see an obvious difference in the page for virtqueue and
device. Could we unify the logic there? So did the free.

> +       }
> +
> +       return 0;
> +}
> +
> +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
> +{
> +       struct vdpa_reconnect_info *info;
> +       struct vduse_virtqueue *vq;
> +
> +       info = &dev->reconnect_status;
> +       free_page(info->vaddr);
> +       info->size = 0;
> +       info->vaddr = 0;
> +       for (int i = 0; i < dev->vq_num + 1; i++) {
> +               vq = &dev->vqs[i];
> +               info = &vq->reconnect_info;
> +               free_page(info->vaddr);
> +               info->size = 0;
> +               info->vaddr = 0;
> +       }
> +
> +       return 0;
> +}
> +
>  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                             unsigned long arg)
>  {
> @@ -1390,6 +1464,8 @@ static int vduse_destroy_dev(char *name)
>                 mutex_unlock(&dev->lock);
>                 return -EBUSY;
>         }
> +       vduse_free_reconnnect_info_mem(dev);
> +
>         dev->connected = true;
>         mutex_unlock(&dev->lock);
>
> @@ -1542,6 +1618,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>                 ret = PTR_ERR(dev->dev);
>                 goto err_dev;
>         }
> +
> +       vduse_alloc_reconnnect_info_mem(dev);

The errors were ignored here.

Thanks

>         __module_get(THIS_MODULE);
>
>         return 0;
> --
> 2.34.3
>

Re: [PATCH v1 2/4] vduse: Add file operation for mmap

[Jason Wang]

On Wed, Oct 11, 2023 at 2:42 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Add the operation for mmap, The user space APP will
> use this function to map the pages to userspace
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 66 ++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 565126a9ab01..05e72d752fb6 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1380,6 +1380,70 @@ static struct vduse_dev *vduse_dev_get_from_minor(int minor)
>         return dev;
>  }
>
> +static vm_fault_t vduse_vm_fault(struct vm_fault *vmf)
> +{
> +       struct vduse_dev *dev = vmf->vma->vm_file->private_data;
> +       struct vm_area_struct *vma = vmf->vma;
> +       u16 index = vma->vm_pgoff;
> +       struct vduse_virtqueue *vq;
> +       struct vdpa_reconnect_info *info;
> +
> +       /* index 0  page reserved for vduse status*/
> +       if (index == 0) {
> +               info = &dev->reconnect_status;
> +       } else {
> +               /* index 1+vq_number page reserved for vduse vqs*/
> +               vq = &dev->vqs[index - 1];
> +               info = &vq->reconnect_info;
> +       }
> +       if (remap_pfn_range(vma, vmf->address & PAGE_MASK,
> +                           PFN_DOWN(virt_to_phys((void *)info->vaddr)),
> +                           PAGE_SIZE, vma->vm_page_prot))
> +               return VM_FAULT_SIGBUS;
> +       return VM_FAULT_NOPAGE;
> +}
> +
> +static const struct vm_operations_struct vduse_vm_ops = {
> +       .fault = vduse_vm_fault,
> +};
> +
> +static int vduse_dev_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +       struct vduse_dev *dev = file->private_data;
> +       struct vdpa_reconnect_info *info;
> +       unsigned long index = vma->vm_pgoff;
> +       struct vduse_virtqueue *vq;
> +
> +       if (vma->vm_end - vma->vm_start != PAGE_SIZE)
> +               return -EINVAL;
> +       if ((vma->vm_flags & VM_SHARED) == 0)
> +               return -EINVAL;
> +
> +       if (index > dev->vq_num + 1)
> +               return -EINVAL;
> +
> +       /* index 0  page reserved for vduse status*/
> +       if (index == 0) {
> +               info = &dev->reconnect_status;
> +       } else {
> +               /* index 1+vq_number page reserved for vduse vqs*/
> +               vq = &dev->vqs[index - 1];
> +               info = &vq->reconnect_info;
> +       }
> +       if (info->vaddr == 0)
> +               return -ENOTSUPP;

Under which condition could we meet this?

> +
> +       if (virt_to_phys((void *)info->vaddr) & (PAGE_SIZE - 1))
> +               return -EINVAL;

And this?

> +       if (vma->vm_end - vma->vm_start != info->size)
> +               return -EOPNOTSUPP;
> +
> +       vm_flags_set(vma, VM_IO | VM_DONTDUMP);

I still don't understand the flags here. Or at least it requires some
comment to explain why they are needed here.

Please refer to the kernel sources which explains their use cases.

Thanks

> +       vma->vm_ops = &vduse_vm_ops;
> +
> +       return 0;
> +}
> +
>  static int vduse_dev_open(struct inode *inode, struct file *file)
>  {
>         int ret;
> @@ -1412,6 +1476,8 @@ static const struct file_operations vduse_dev_fops = {
>         .unlocked_ioctl = vduse_dev_ioctl,
>         .compat_ioctl   = compat_ptr_ioctl,
>         .llseek         = noop_llseek,
> +       .mmap           = vduse_dev_mmap,
> +
>  };
>
>  static struct vduse_dev *vduse_dev_create(void)
> --
> 2.34.3
>

Re: [PATCH v1 3/4] vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO

[Jason Wang]

On Wed, Oct 11, 2023 at 2:42 PM Cindy Lu <lulu@redhat.com> wrote:
>
> In VDUSE_GET_RECONNECT_INFO, the Userspace App can get the map size

I'm not sure why this is needed, the structure that mmaped to
userspace should belong to uAPI then userspace can do versions there?

> and The number of mapping memory pages from the kernel.

Userspace knows how many vq at most, why is this still needed?

> The userspace
> App can use this information to map the pages.
>
> Add struct vhost_reconnect_data/vhost_reconnect_vring for sync the
> information in reconnection
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 15 +++++++++++
>  include/uapi/linux/vduse.h         | 43 ++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 05e72d752fb6..0f15e7ac716b 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1347,6 +1347,21 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                 ret = 0;
>                 break;
>         }
> +       case VDUSE_GET_RECONNECT_INFO: {
> +               struct vduse_reconnect_mmap_info info;
> +
> +               ret = -EFAULT;
> +               if (copy_from_user(&info, argp, sizeof(info)))
> +                       break;
> +
> +               info.size = PAGE_SIZE;
> +               info.max_index = dev->vq_num + 1;
> +
> +               if (copy_to_user(argp, &info, sizeof(info)))
> +                       break;
> +               ret = 0;
> +               break;
> +       }
>         default:
>                 ret = -ENOIOCTLCMD;
>                 break;
> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> index 11bd48c72c6c..5ccac535fba6 100644
> --- a/include/uapi/linux/vduse.h
> +++ b/include/uapi/linux/vduse.h
> @@ -350,4 +350,47 @@ struct vduse_dev_response {
>         };
>  };
>
> +/**
> + * struct vhost_reconnect_data - saved the reconnect info for device
> + * @reconnect_time: reconnect time for this device. userspace APP needs to do ++
> + *                  while reconnecting

This commnet needs tweaking as I don't think "++" is good English. And
you need to explain why we need this.

> + * @version; version for userspace APP

It's the version of uAPI not the userspace APP. And in order to be
able for a correct bootstrap, this needs to be the first field instead
of the second.

> + * @features; Device features negotiated in the last reconnect.
> + * @status; Device status in last reconnect

I wonder if we need just more than this, for example the number of
active queues, or this is what does @nr_vrings mean?

> + * @nr_vrings; number of vqs
> + */
> +
> +struct vhost_reconnect_data {
> +       __u32 reconnect_time;
> +       __u32 version;
> +       __u64 features;
> +       __u8 status;
> +       __u32 nr_vrings;
> +};
> +
> +/**
> + * struct vhost_reconnect_vring -saved the reconnect info for vqs
> + * @last_avail_idx: device available index
> + * @avail_wrap_counter: Driver ring wrap counter
> + */
> +struct vhost_reconnect_vring {
> +       __u16 last_avail_idx;
> +       __u16 avail_wrap_counter;
> +};

Do we need the last_used_idx and last_used_wrap_counter? If not,
please document why.

> +
> +/**
> + * struct vduse_reconnect_mmap_info
> + * @size: mapping memory size, here we use page_size
> + * @max_index: the number of pages allocated in kernel,just
> + * use for check

Did you mean "sanity check"?

Thanks

> + */
> +
> +struct vduse_reconnect_mmap_info {
> +       __u32 size;
> +       __u32 max_index;
> +};
> +
> +#define VDUSE_GET_RECONNECT_INFO \
> +       _IOWR(VDUSE_BASE, 0x1b, struct vduse_reconnect_mmap_info)
> +
>  #endif /* _UAPI_VDUSE_H_ */
> --
> 2.34.3
>

Re: [PATCH v1 3/4] vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO

[Jason Wang]

On Wed, Oct 18, 2023 at 11:23 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Oct 11, 2023 at 2:42 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > In VDUSE_GET_RECONNECT_INFO, the Userspace App can get the map size
>
> I'm not sure why this is needed, the structure that mmaped to
> userspace should belong to uAPI then userspace can do versions there?

Ok, so I think this makes sense, it can help to avoid duplicating
versions in each mmaped pages.

Thanks

Re: [PATCH v1 4/4] vduse: update the vq_info in ioctl

[Jason Wang]

On Wed, Oct 11, 2023 at 2:42 PM Cindy Lu <lulu@redhat.com> wrote:
>
> In VDUSE_VQ_GET_INFO, the driver will sync the last_avail_idx
> with reconnect info, After mapping the reconnect pages to userspace
> The userspace App will update the reconnect_time in
> struct vhost_reconnect_vring, If this is not 0 then it means this
> vq is reconnected and will update the last_avail_idx
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 0f15e7ac716b..42e7a90ab74c 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1213,6 +1213,9 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                 struct vduse_vq_info vq_info;
>                 struct vduse_virtqueue *vq;
>                 u32 index;
> +               struct vdpa_reconnect_info *area;
> +               struct vhost_reconnect_vring *vq_reconnect;
> +               struct vhost_reconnect_data *dev_reconnect;
>
>                 ret = -EFAULT;
>                 if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
> @@ -1244,6 +1247,19 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>
>                 vq_info.ready = vq->ready;
>
> +               area = &dev->reconnect_status;
> +               dev_reconnect = (struct vhost_reconnect_data *)area->vaddr;
> +
> +               area = &vq->reconnect_info;
> +               vq_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
> +               /*check if the vq is reconnect, if yes then update the last_avail_idx*/
> +               if ((vq_reconnect->last_avail_idx !=
> +                    vq_info.split.avail_index) &&
> +                   (dev_reconnect->reconnect_time != 0)) {
> +                       vq_info.split.avail_index =
> +                               vq_reconnect->last_avail_idx;
> +               }

This may work only for split, how about packed? And I wonder when such
synchronization is not done at set_vq_state() but get_vq_info()?

Thanks

> +
>                 ret = -EFAULT;
>                 if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
>                         break;
> --
> 2.34.3
>

Re: [PATCH v1 0/4] vduse: Reconnection support in vduse

[Jason Wang]

On Tue, Oct 17, 2023 at 10:52 AM Cindy Lu <lulu@redhat.com> wrote:
>
> On Tue, Oct 17, 2023 at 10:44 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Oct 11, 2023 at 2:42 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > Here is the reconnect support in vduse,
> > >
> > > The kernel will allocate pages for reconnection
> > > userspace needs to use ioctl VDUSE_GET_RECONNECT_INFO to
> > > get the mmap related infomation and then map these pages
> > > to userspace.
> > > The kernel and userspace will use these pages to sync
> > > the reconnect information
> > >
> > > Tested in vduse + dpdk test-pmd
> >
> > I would like to see the DPDK part as a reference. Would you mind
> > giving me a pointer?
> >
> > Thanks
> >
> sure, Will send the dpdk patch soon
> thanks
> cindy

Btw, I would expect to add the protocol into the VDUSE doc to describe
the exact step for doing reconnection.

Thanks

> > >
> > > Cindy Lu (4):
> > >   vduse: Add function to get/free the pages for reconnection
> > >   vduse: Add file operation for mmap
> > >   vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO
> > >   vduse: update the vq_info in ioctl
> > >
> > >  drivers/vdpa/vdpa_user/vduse_dev.c | 175 +++++++++++++++++++++++++++++
> > >  include/uapi/linux/vduse.h         |  43 +++++++
> > >  2 files changed, 218 insertions(+)
> > >
> > > --
> > > 2.34.3
> > >
> >
>

Re: [PATCH v1 1/4] vduse: Add function to get/free the pages for reconnection

[Cindy Lu]

On Wed, Oct 18, 2023 at 10:57 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Oct 11, 2023 at 2:42 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Add the function vduse_alloc_reconnnect_info_mem
> > and vduse_alloc_reconnnect_info_mem
> > In this 2 function, vduse will get/free (vq_num + 1)*page
> > Page 0 will be used to save the reconnection information, The
> > Userspace App will maintain this. Page 1 ~ vq_num + 1 will save
> > the reconnection information for vqs.
>
> I'd align this with the spec. E.g spec starts virtqueue index from 0.
>
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vdpa/vdpa_user/vduse_dev.c | 78 ++++++++++++++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index 26b7e29cb900..565126a9ab01 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -30,6 +30,10 @@
> >  #include <uapi/linux/virtio_blk.h>
> >  #include <linux/mod_devicetable.h>
> >
> > +#ifdef CONFIG_X86
> > +#include <asm/set_memory.h>
> > +#endif
>
> Why do we need special care for x86?
>
> > +
> >  #include "iova_domain.h"
> >
> >  #define DRV_AUTHOR   "Yongji Xie <xieyongji@bytedance.com>"
> > @@ -41,6 +45,19 @@
> >  #define VDUSE_IOVA_SIZE (128 * 1024 * 1024)
> >  #define VDUSE_MSG_DEFAULT_TIMEOUT 30
> >
> > +/* struct vdpa_reconnect_info saved the alloc pages info
> > + * these pages will mmaped to userspace for reconnection
> > + */
> > +struct vdpa_reconnect_info {
> > +       /* Offset (within vm_file) in PAGE_SIZE
> > +        */
> > +       u32 index;
> > +       /* virtual address for this page*/
> > +       unsigned long vaddr;
> > +       /* allocated memory size, */
> > +       phys_addr_t size;
>
> Anything wrong with using "unsigned long" here.
>
> Btw, what is "allocated memory size" since I think we can use up to
> PAGE_SIZE here?
>
sure， will remove this here
Thanks
Cindy
> > +};
> > +
> >  struct vduse_virtqueue {
> >         u16 index;
> >         u16 num_max;
> > @@ -57,6 +74,7 @@ struct vduse_virtqueue {
> >         struct vdpa_callback cb;
> >         struct work_struct inject;
> >         struct work_struct kick;
> > +       struct vdpa_reconnect_info reconnect_info;
> >  };
> >
> >  struct vduse_dev;
> > @@ -106,6 +124,7 @@ struct vduse_dev {
> >         u32 vq_align;
> >         struct vduse_umem *umem;
> >         struct mutex mem_lock;
> > +       struct vdpa_reconnect_info reconnect_status;
> >  };
> >
> >  struct vduse_dev_msg {
> > @@ -1030,6 +1049,61 @@ static int vduse_dev_reg_umem(struct vduse_dev *dev,
> >         return ret;
> >  }
> >
> > +static int vduse_alloc_reconnnect_info_mem(struct vduse_dev *dev)
> > +{
> > +       struct vdpa_reconnect_info *info;
> > +       struct vduse_virtqueue *vq;
> > +       void *addr;
> > +
> > +       /*page 0 is use to save status,dpdk will use this to save the information
> > +        *needed in reconnection,kernel don't need to maintain this
> > +        */
>
> Let's avoid mentioning any specific userspace like DPDK in the kernel
> source. A good uAPI should not be designed for a specific userspace.
>
will change this
> > +       info = &dev->reconnect_status;
> > +       addr = (void *)get_zeroed_page(GFP_KERNEL);
> > +       if (!addr)
> > +               return -1;
>
> Let's don't use magic number here but something like -ENOMEM.
>
> > +
> > +       info->vaddr = (unsigned long)(addr);
> > +       info->size = PAGE_SIZE;
> > +       /* index is vm Offset in PAGE_SIZE */
> > +       info->index = 0;
> > +
> > +       /*page 1~ vq_num + 1 save the reconnect info for vqs*/
>
> There's no need to explain what the code does.
>
will change this

> > +       for (int i = 0; i < dev->vq_num + 1; i++) {
> > +               vq = &dev->vqs[i];
> > +               info = &vq->reconnect_info;
> > +               addr = (void *)get_zeroed_page(GFP_KERNEL);
> > +               if (!addr)
> > +                       return -1;
> > +
> > +               info->vaddr = (unsigned long)(addr);
> > +               info->size = PAGE_SIZE;
> > +               info->index = i + 1;
>
> Btw, I don't see an obvious difference in the page for virtqueue and
> device. Could we unify the logic there? So did the free.
>
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int vduse_free_reconnnect_info_mem(struct vduse_dev *dev)
> > +{
> > +       struct vdpa_reconnect_info *info;
> > +       struct vduse_virtqueue *vq;
> > +
> > +       info = &dev->reconnect_status;
> > +       free_page(info->vaddr);
> > +       info->size = 0;
> > +       info->vaddr = 0;
> > +       for (int i = 0; i < dev->vq_num + 1; i++) {
> > +               vq = &dev->vqs[i];
> > +               info = &vq->reconnect_info;
> > +               free_page(info->vaddr);
> > +               info->size = 0;
> > +               info->vaddr = 0;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> >                             unsigned long arg)
> >  {
> > @@ -1390,6 +1464,8 @@ static int vduse_destroy_dev(char *name)
> >                 mutex_unlock(&dev->lock);
> >                 return -EBUSY;
> >         }
> > +       vduse_free_reconnnect_info_mem(dev);
> > +
> >         dev->connected = true;
> >         mutex_unlock(&dev->lock);
> >
> > @@ -1542,6 +1618,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> >                 ret = PTR_ERR(dev->dev);
> >                 goto err_dev;
> >         }
> > +
> > +       vduse_alloc_reconnnect_info_mem(dev);
>
> The errors were ignored here.
>
> Thanks
>
sure will add the check here

Thanks
cindy

> >         __module_get(THIS_MODULE);
> >
> >         return 0;
> > --
> > 2.34.3
> >
>