[RFC 0/4] reconnect support in vduse

[RFC 0/4] reconnect support in vduse

[Cindy Lu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 653 bytes --]

Hi all

Here is the reconnect support in vduse, The steps
is map the pages from kernel to userland, usespace    
app will wirte the vq_info in the pages
And then sync this information in kernel/user-space
I just the split mode, So only use the last_avail_idx
here, Will send the patch for DPDK later

Tested in vduse + dpdk test-pmd

Cindy Lu (4):
  vduse: Add the struct to save the vq reconnect info
  vduse: Add file operation for mmap
  vduse: Add the function for get/free the mapp pages
  vduse: update the vq_info in ioctl

 drivers/vdpa/vdpa_user/vduse_dev.c | 107 +++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

-- 
2.34.3

[RFC 1/4] vduse: Add the struct to save the vq reconnect info

[Cindy Lu]

From: Your Name <you@example.com>

this struct is to save the reconnect info struct, in this
struct saved the page info that alloc to save the
reconnect info

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

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 26b7e29cb900..f845dc46b1db 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -72,6 +72,12 @@ struct vduse_umem {
 	struct page **pages;
 	struct mm_struct *mm;
 };
+struct vdpa_reconnect_info {
+	u32 index;
+	phys_addr_t addr;
+	unsigned long vaddr;
+	phys_addr_t size;
+};
 
 struct vduse_dev {
 	struct vduse_vdpa *vdev;
@@ -106,6 +112,7 @@ struct vduse_dev {
 	u32 vq_align;
 	struct vduse_umem *umem;
 	struct mutex mem_lock;
+	struct vdpa_reconnect_info reconnect_info[64];
 };
 
 struct vduse_dev_msg {
-- 
2.34.3

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

[Cindy Lu]

From: Your Name <you@example.com>

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 | 49 ++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index f845dc46b1db..1b833bf0ae37 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1313,6 +1313,54 @@ 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 vdpa_reconnect_info *info;
+	info = &dev->reconnect_info[index];
+
+	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	if (remap_pfn_range(vma, vmf->address & PAGE_MASK, PFN_DOWN(info->addr),
+			    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_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;
+
+	if (vma->vm_end - vma->vm_start != PAGE_SIZE)
+		return -EINVAL;
+	if ((vma->vm_flags & VM_SHARED) == 0)
+		return -EINVAL;
+
+	if (index > 65535)
+		return -EINVAL;
+
+	info = &dev->reconnect_info[index];
+	if (info->addr & (PAGE_SIZE - 1))
+		return -EINVAL;
+	if (vma->vm_end - vma->vm_start != info->size) {
+		return -ENOTSUPP;
+	}
+
+	vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
+	vma->vm_ops = &vduse_vm_ops;
+
+	return 0;
+}
+
 static int vduse_dev_open(struct inode *inode, struct file *file)
 {
 	int ret;
@@ -1345,6 +1393,7 @@ static const struct file_operations vduse_dev_fops = {
 	.unlocked_ioctl	= vduse_dev_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
 	.llseek		= noop_llseek,
+	.mmap		= vduse_mmap,
 };
 
 static struct vduse_dev *vduse_dev_create(void)
-- 
2.34.3

[RFC 3/4] vduse: Add the function for get/free the mapp pages

[Cindy Lu]

From: Your Name <you@example.com>

Add the function for get/free pages, ad this info
will saved in dev->reconnect_info

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

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 1b833bf0ae37..3df1256eccb4 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1313,6 +1313,35 @@ static struct vduse_dev *vduse_dev_get_from_minor(int minor)
 	return dev;
 }
 
+int vduse_get_vq_reconnnect(struct vduse_dev *dev, u16 idx)
+{
+	struct vdpa_reconnect_info *area;
+	void *addr = (void *)get_zeroed_page(GFP_KERNEL);
+
+	area = &dev->reconnect_info[idx];
+
+	area->addr = virt_to_phys(addr);
+	area->vaddr = (unsigned long)addr;
+	area->size = PAGE_SIZE;
+	area->index = idx;
+
+	return 0;
+}
+
+int vduse_free_vq_reconnnect(struct vduse_dev *dev, u16 idx)
+{
+	struct vdpa_reconnect_info *area;
+
+	area = &dev->reconnect_info[idx];
+	if ((area->size == PAGE_SIZE) && (area->addr != NULL)) {
+		free_page(area->vaddr);
+		area->size = 0;
+		area->addr = 0;
+		area->vaddr = 0;
+	}
+
+	return 0;
+}
 
 static vm_fault_t vduse_vm_fault(struct vm_fault *vmf)
 {
@@ -1446,6 +1475,10 @@ static int vduse_destroy_dev(char *name)
 		mutex_unlock(&dev->lock);
 		return -EBUSY;
 	}
+	for (int i = 0; i < dev->vq_num; i++) {
+
+		vduse_free_vq_reconnnect(dev, i);
+	}
 	dev->connected = true;
 	mutex_unlock(&dev->lock);
 
@@ -1583,6 +1616,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
 		INIT_WORK(&dev->vqs[i].kick, vduse_vq_kick_work);
 		spin_lock_init(&dev->vqs[i].kick_lock);
 		spin_lock_init(&dev->vqs[i].irq_lock);
+
+		vduse_get_vq_reconnnect(dev, i);
 	}
 
 	ret = idr_alloc(&vduse_idr, dev, 1, VDUSE_DEV_MAX, GFP_KERNEL);
-- 
2.34.3

[RFC 4/4] vduse: update the vq_info in ioctl

[Cindy Lu]

From: Your Name <you@example.com>

in VDUSE_VQ_GET_INFO, driver will sync the last_avail_idx
with reconnect info, I have olny test the split mode, so
only use this here, will add more information later

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 3df1256eccb4..b8e453eac0ce 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -141,6 +141,11 @@ static u32 allowed_device_id[] = {
 	VIRTIO_ID_NET,
 };
 
+struct vhost_reconnect_vring {
+	uint16_t last_avail_idx;
+	bool avail_wrap_counter;
+};
+
 static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
 {
 	struct vduse_vdpa *vdev = container_of(vdpa, struct vduse_vdpa, vdpa);
@@ -1176,6 +1181,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 				vq->state.split.avail_index;
 
 		vq_info.ready = vq->ready;
+		struct vdpa_reconnect_info *area;
+
+		area = &dev->reconnect_info[index];
+		struct vhost_reconnect_vring *log_reconnect;
+
+		log_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
+		if (log_reconnect->last_avail_idx !=
+		    vq_info.split.avail_index) {
+			vq_info.split.avail_index =
+				log_reconnect->last_avail_idx;
+		}
 
 		ret = -EFAULT;
 		if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
-- 
2.34.3

Re: [RFC 1/4] vduse: Add the struct to save the vq reconnect info

[Jason Wang]

On Wed, Jun 28, 2023 at 2:59 PM Cindy Lu <lulu@redhat.com> wrote:
>
> From: Your Name <you@example.com>

It looks to me your git is not properly configured.

>
> this struct is to save the reconnect info struct, in this
> struct saved the page info that alloc to save the
> reconnect info
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 26b7e29cb900..f845dc46b1db 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -72,6 +72,12 @@ struct vduse_umem {
>         struct page **pages;
>         struct mm_struct *mm;
>  };
> +struct vdpa_reconnect_info {
> +       u32 index;
> +       phys_addr_t addr;
> +       unsigned long vaddr;
> +       phys_addr_t size;
> +};

Please add comments to explain each field. And I think this should be
a part of uAPI?

Thanks

>
>  struct vduse_dev {
>         struct vduse_vdpa *vdev;
> @@ -106,6 +112,7 @@ struct vduse_dev {
>         u32 vq_align;
>         struct vduse_umem *umem;
>         struct mutex mem_lock;
> +       struct vdpa_reconnect_info reconnect_info[64];
>  };
>
>  struct vduse_dev_msg {
> --
> 2.34.3
>

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

[Jason Wang]

On Wed, Jun 28, 2023 at 2:59 PM Cindy Lu <lulu@redhat.com> wrote:
>
> From: Your Name <you@example.com>
>
> Add the operation for mmap, The user space APP will
> use this function to map the pages to userspace

Please be specific in the log. E.g why and what the main goal for this mmap.

>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 49 ++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index f845dc46b1db..1b833bf0ae37 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1313,6 +1313,54 @@ 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 vdpa_reconnect_info *info;
> +       info = &dev->reconnect_info[index];
> +
> +       vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +       if (remap_pfn_range(vma, vmf->address & PAGE_MASK, PFN_DOWN(info->addr),
> +                           PAGE_SIZE, vma->vm_page_prot))

I'm not sure if this can work e.g do we want to use separate pages for
each virtqueue (I think the answer is yes).

> +               return VM_FAULT_SIGBUS;
> +       return VM_FAULT_NOPAGE;
> +}
> +
> +static const struct vm_operations_struct vduse_vm_ops = {
> +       .fault = vduse_vm_fault,
> +};
> +
> +static int vduse_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;
> +
> +       if (vma->vm_end - vma->vm_start != PAGE_SIZE)
> +               return -EINVAL;
> +       if ((vma->vm_flags & VM_SHARED) == 0)
> +               return -EINVAL;
> +
> +       if (index > 65535)
> +               return -EINVAL;
> +
> +       info = &dev->reconnect_info[index];
> +       if (info->addr & (PAGE_SIZE - 1))
> +               return -EINVAL;
> +       if (vma->vm_end - vma->vm_start != info->size) {
> +               return -ENOTSUPP;
> +       }

How can userspace know the correct size (info->size) here?

> +
> +       vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);

Why do you need VM_IO, VM_PFNMAP and VM_DONTDUMP here?

Thanks

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

Re: [RFC 3/4] vduse: Add the function for get/free the mapp pages

[Jason Wang]

On Wed, Jun 28, 2023 at 2:59 PM Cindy Lu <lulu@redhat.com> wrote:
>
> From: Your Name <you@example.com>
>
> Add the function for get/free pages, ad this info
> will saved in dev->reconnect_info

I think this should be squashed to patch 2 otherwise it fixes a bug
that is introduced in patch 2?

>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 35 ++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 1b833bf0ae37..3df1256eccb4 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1313,6 +1313,35 @@ static struct vduse_dev *vduse_dev_get_from_minor(int minor)
>         return dev;
>  }
>
> +int vduse_get_vq_reconnnect(struct vduse_dev *dev, u16 idx)
> +{
> +       struct vdpa_reconnect_info *area;
> +       void *addr = (void *)get_zeroed_page(GFP_KERNEL);
> +
> +       area = &dev->reconnect_info[idx];
> +
> +       area->addr = virt_to_phys(addr);
> +       area->vaddr = (unsigned long)addr;
> +       area->size = PAGE_SIZE;
> +       area->index = idx;
> +
> +       return 0;
> +}
> +
> +int vduse_free_vq_reconnnect(struct vduse_dev *dev, u16 idx)
> +{
> +       struct vdpa_reconnect_info *area;
> +
> +       area = &dev->reconnect_info[idx];
> +       if ((area->size == PAGE_SIZE) && (area->addr != NULL)) {
> +               free_page(area->vaddr);
> +               area->size = 0;
> +               area->addr = 0;
> +               area->vaddr = 0;
> +       }
> +
> +       return 0;
> +}
>
>  static vm_fault_t vduse_vm_fault(struct vm_fault *vmf)
>  {
> @@ -1446,6 +1475,10 @@ static int vduse_destroy_dev(char *name)
>                 mutex_unlock(&dev->lock);
>                 return -EBUSY;
>         }
> +       for (int i = 0; i < dev->vq_num; i++) {
> +
> +               vduse_free_vq_reconnnect(dev, i);
> +       }
>         dev->connected = true;
>         mutex_unlock(&dev->lock);
>
> @@ -1583,6 +1616,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
>                 INIT_WORK(&dev->vqs[i].kick, vduse_vq_kick_work);
>                 spin_lock_init(&dev->vqs[i].kick_lock);
>                 spin_lock_init(&dev->vqs[i].irq_lock);
> +
> +               vduse_get_vq_reconnnect(dev, i);

Can we delay the allocated until fault?

Thanks

>         }
>
>         ret = idr_alloc(&vduse_idr, dev, 1, VDUSE_DEV_MAX, GFP_KERNEL);
> --
> 2.34.3
>

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

[Jason Wang]

On Wed, Jun 28, 2023 at 3:00 PM Cindy Lu <lulu@redhat.com> wrote:
>
> From: Your Name <you@example.com>
>
> in VDUSE_VQ_GET_INFO, driver will sync the last_avail_idx
> with reconnect info, I have olny test the split mode, so

Typo, should be "only".

> only use this here, will add more information later
>
> 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 3df1256eccb4..b8e453eac0ce 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -141,6 +141,11 @@ static u32 allowed_device_id[] = {
>         VIRTIO_ID_NET,
>  };
>
> +struct vhost_reconnect_vring {
> +       uint16_t last_avail_idx;
> +       bool avail_wrap_counter;
> +};

Should this belong to uAPI?

> +
>  static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
>  {
>         struct vduse_vdpa *vdev = container_of(vdpa, struct vduse_vdpa, vdpa);
> @@ -1176,6 +1181,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                                 vq->state.split.avail_index;
>
>                 vq_info.ready = vq->ready;
> +               struct vdpa_reconnect_info *area;
> +
> +               area = &dev->reconnect_info[index];
> +               struct vhost_reconnect_vring *log_reconnect;
> +
> +               log_reconnect = (struct vhost_reconnect_vring *)area->vaddr;

What if userspace doesn't do mmap()?

Thanks

> +               if (log_reconnect->last_avail_idx !=
> +                   vq_info.split.avail_index) {
> +                       vq_info.split.avail_index =
> +                               log_reconnect->last_avail_idx;
> +               }
>
>                 ret = -EFAULT;
>                 if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
> --
> 2.34.3
>

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

[Cindy Lu]

On Wed, Jun 28, 2023 at 4:13 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Jun 28, 2023 at 3:00 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > From: Your Name <you@example.com>
> >
> > in VDUSE_VQ_GET_INFO, driver will sync the last_avail_idx
> > with reconnect info, I have olny test the split mode, so
>
> Typo, should be "only".
>
sure will change this
> > only use this here, will add more information later
> >
> > 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 3df1256eccb4..b8e453eac0ce 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -141,6 +141,11 @@ static u32 allowed_device_id[] = {
> >         VIRTIO_ID_NET,
> >  };
> >
> > +struct vhost_reconnect_vring {
> > +       uint16_t last_avail_idx;
> > +       bool avail_wrap_counter;
> > +};
>
> Should this belong to uAPI?
>
will change this
> > +
> >  static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
> >  {
> >         struct vduse_vdpa *vdev = container_of(vdpa, struct vduse_vdpa, vdpa);
> > @@ -1176,6 +1181,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> >                                 vq->state.split.avail_index;
> >
> >                 vq_info.ready = vq->ready;
> > +               struct vdpa_reconnect_info *area;
> > +
> > +               area = &dev->reconnect_info[index];
> > +               struct vhost_reconnect_vring *log_reconnect;
> > +
> > +               log_reconnect = (struct vhost_reconnect_vring *)area->vaddr;
>
> What if userspace doesn't do mmap()?
>
> Thanks
>
sure will add the check for this
Thanks
Cindy
> > +               if (log_reconnect->last_avail_idx !=
> > +                   vq_info.split.avail_index) {
> > +                       vq_info.split.avail_index =
> > +                               log_reconnect->last_avail_idx;
> > +               }
> >
> >                 ret = -EFAULT;
> >                 if (copy_to_user(argp, &vq_info, sizeof(vq_info)))
> > --
> > 2.34.3
> >
>

Re: [RFC 3/4] vduse: Add the function for get/free the mapp pages

[Cindy Lu]

On Wed, Jun 28, 2023 at 4:11 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Jun 28, 2023 at 2:59 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > From: Your Name <you@example.com>
> >
> > Add the function for get/free pages, ad this info
> > will saved in dev->reconnect_info
>
> I think this should be squashed to patch 2 otherwise it fixes a bug
> that is introduced in patch 2?
>
sure will do
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vdpa/vdpa_user/vduse_dev.c | 35 ++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index 1b833bf0ae37..3df1256eccb4 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -1313,6 +1313,35 @@ static struct vduse_dev *vduse_dev_get_from_minor(int minor)
> >         return dev;
> >  }
> >
> > +int vduse_get_vq_reconnnect(struct vduse_dev *dev, u16 idx)
> > +{
> > +       struct vdpa_reconnect_info *area;
> > +       void *addr = (void *)get_zeroed_page(GFP_KERNEL);
> > +
> > +       area = &dev->reconnect_info[idx];
> > +
> > +       area->addr = virt_to_phys(addr);
> > +       area->vaddr = (unsigned long)addr;
> > +       area->size = PAGE_SIZE;
> > +       area->index = idx;
> > +
> > +       return 0;
> > +}
> > +
> > +int vduse_free_vq_reconnnect(struct vduse_dev *dev, u16 idx)
> > +{
> > +       struct vdpa_reconnect_info *area;
> > +
> > +       area = &dev->reconnect_info[idx];
> > +       if ((area->size == PAGE_SIZE) && (area->addr != NULL)) {
> > +               free_page(area->vaddr);
> > +               area->size = 0;
> > +               area->addr = 0;
> > +               area->vaddr = 0;
> > +       }
> > +
> > +       return 0;
> > +}
> >
> >  static vm_fault_t vduse_vm_fault(struct vm_fault *vmf)
> >  {
> > @@ -1446,6 +1475,10 @@ static int vduse_destroy_dev(char *name)
> >                 mutex_unlock(&dev->lock);
> >                 return -EBUSY;
> >         }
> > +       for (int i = 0; i < dev->vq_num; i++) {
> > +
> > +               vduse_free_vq_reconnnect(dev, i);
> > +       }
> >         dev->connected = true;
> >         mutex_unlock(&dev->lock);
> >
> > @@ -1583,6 +1616,8 @@ static int vduse_create_dev(struct vduse_dev_config *config,
> >                 INIT_WORK(&dev->vqs[i].kick, vduse_vq_kick_work);
> >                 spin_lock_init(&dev->vqs[i].kick_lock);
> >                 spin_lock_init(&dev->vqs[i].irq_lock);
> > +
> > +               vduse_get_vq_reconnnect(dev, i);
>
> Can we delay the allocated until fault?
>
sure will do
> Thanks
>
> >         }
> >
> >         ret = idr_alloc(&vduse_idr, dev, 1, VDUSE_DEV_MAX, GFP_KERNEL);
> > --
> > 2.34.3
> >
>

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

[Cindy Lu]

On Wed, Jun 28, 2023 at 4:08 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Jun 28, 2023 at 2:59 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > From: Your Name <you@example.com>
> >
> > Add the operation for mmap, The user space APP will
> > use this function to map the pages to userspace
>
> Please be specific in the log. E.g why and what the main goal for this mmap.
>
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vdpa/vdpa_user/vduse_dev.c | 49 ++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index f845dc46b1db..1b833bf0ae37 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -1313,6 +1313,54 @@ 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 vdpa_reconnect_info *info;
> > +       info = &dev->reconnect_info[index];
> > +
> > +       vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> > +       if (remap_pfn_range(vma, vmf->address & PAGE_MASK, PFN_DOWN(info->addr),
> > +                           PAGE_SIZE, vma->vm_page_prot))
>
> I'm not sure if this can work e.g do we want to use separate pages for
> each virtqueue (I think the answer is yes).
>
yes, this map the separate pages per vq, beads on my test this works
> > +               return VM_FAULT_SIGBUS;
> > +       return VM_FAULT_NOPAGE;
> > +}
> > +
> > +static const struct vm_operations_struct vduse_vm_ops = {
> > +       .fault = vduse_vm_fault,
> > +};
> > +
> > +static int vduse_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;
> > +
> > +       if (vma->vm_end - vma->vm_start != PAGE_SIZE)
> > +               return -EINVAL;
> > +       if ((vma->vm_flags & VM_SHARED) == 0)
> > +               return -EINVAL;
> > +
> > +       if (index > 65535)
> > +               return -EINVAL;
> > +
> > +       info = &dev->reconnect_info[index];
> > +       if (info->addr & (PAGE_SIZE - 1))
> > +               return -EINVAL;
> > +       if (vma->vm_end - vma->vm_start != info->size) {
> > +               return -ENOTSUPP;
> > +       }
>
> How can userspace know the correct size (info->size) here?
>
I had hard code the size in userpace , I will add the new ioctl of get
the map size
Thanks
cindy
> > +
> > +       vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
>
> Why do you need VM_IO, VM_PFNMAP and VM_DONTDUMP here?
>
> Thanks
>
> > +       vma->vm_ops = &vduse_vm_ops;
> > +
> > +       return 0;
> > +}
> > +
> >  static int vduse_dev_open(struct inode *inode, struct file *file)
> >  {
> >         int ret;
> > @@ -1345,6 +1393,7 @@ static const struct file_operations vduse_dev_fops = {
> >         .unlocked_ioctl = vduse_dev_ioctl,
> >         .compat_ioctl   = compat_ptr_ioctl,
> >         .llseek         = noop_llseek,
> > +       .mmap           = vduse_mmap,
> >  };
> >
> >  static struct vduse_dev *vduse_dev_create(void)
> > --
> > 2.34.3
> >
>

Re: [RFC 1/4] vduse: Add the struct to save the vq reconnect info

[Cindy Lu]

On Wed, Jun 28, 2023 at 4:04 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Jun 28, 2023 at 2:59 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > From: Your Name <you@example.com>
>
> It looks to me your git is not properly configured.
>
> >
> > this struct is to save the reconnect info struct, in this
> > struct saved the page info that alloc to save the
> > reconnect info
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index 26b7e29cb900..f845dc46b1db 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -72,6 +72,12 @@ struct vduse_umem {
> >         struct page **pages;
> >         struct mm_struct *mm;
> >  };
> > +struct vdpa_reconnect_info {
> > +       u32 index;
> > +       phys_addr_t addr;
> > +       unsigned long vaddr;
> > +       phys_addr_t size;
> > +};
>
> Please add comments to explain each field. And I think this should be
> a part of uAPI?
>
> Thanks
>
Will add the new ioctl for this information
Thanks
Cindy
> >
> >  struct vduse_dev {
> >         struct vduse_vdpa *vdev;
> > @@ -106,6 +112,7 @@ struct vduse_dev {
> >         u32 vq_align;
> >         struct vduse_umem *umem;
> >         struct mutex mem_lock;
> > +       struct vdpa_reconnect_info reconnect_info[64];
> >  };
> >
> >  struct vduse_dev_msg {
> > --
> > 2.34.3
> >
>

Re: [RFC 1/4] vduse: Add the struct to save the vq reconnect info

[Jason Wang]

On Sat, Jul 1, 2023 at 5:25 PM Cindy Lu <lulu@redhat.com> wrote:
>
> On Wed, Jun 28, 2023 at 4:04 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Jun 28, 2023 at 2:59 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > From: Your Name <you@example.com>
> >
> > It looks to me your git is not properly configured.
> >
> > >
> > > this struct is to save the reconnect info struct, in this
> > > struct saved the page info that alloc to save the
> > > reconnect info
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > >  drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index 26b7e29cb900..f845dc46b1db 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -72,6 +72,12 @@ struct vduse_umem {
> > >         struct page **pages;
> > >         struct mm_struct *mm;
> > >  };
> > > +struct vdpa_reconnect_info {
> > > +       u32 index;
> > > +       phys_addr_t addr;
> > > +       unsigned long vaddr;
> > > +       phys_addr_t size;
> > > +};
> >
> > Please add comments to explain each field. And I think this should be
> > a part of uAPI?
> >
> > Thanks
> >
> Will add the new ioctl for this information

I may miss something but having this to be part of the uAPI seems more
than enough.

Or what would this new ioctl do?

Thanks

> Thanks
> Cindy
> > >
> > >  struct vduse_dev {
> > >         struct vduse_vdpa *vdev;
> > > @@ -106,6 +112,7 @@ struct vduse_dev {
> > >         u32 vq_align;
> > >         struct vduse_umem *umem;
> > >         struct mutex mem_lock;
> > > +       struct vdpa_reconnect_info reconnect_info[64];
> > >  };
> > >
> > >  struct vduse_dev_msg {
> > > --
> > > 2.34.3
> > >
> >
>

Re: [RFC 1/4] vduse: Add the struct to save the vq reconnect info

[Maxime Coquelin]

Hello Cindy,

On 6/28/23 08:59, Cindy Lu wrote:
> From: Your Name <you@example.com>
> 
> this struct is to save the reconnect info struct, in this
> struct saved the page info that alloc to save the
> reconnect info
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>   drivers/vdpa/vdpa_user/vduse_dev.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 26b7e29cb900..f845dc46b1db 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -72,6 +72,12 @@ struct vduse_umem {
>   	struct page **pages;
>   	struct mm_struct *mm;
>   };
> +struct vdpa_reconnect_info {
> +	u32 index;
> +	phys_addr_t addr;
> +	unsigned long vaddr;
> +	phys_addr_t size;
> +};
>   
>   struct vduse_dev {
>   	struct vduse_vdpa *vdev;
> @@ -106,6 +112,7 @@ struct vduse_dev {
>   	u32 vq_align;
>   	struct vduse_umem *umem;
>   	struct mutex mem_lock;
> +	struct vdpa_reconnect_info reconnect_info[64];

Why 64?
Shouldn't it be part of struct vduse_virtqueue instead?

>   };
>   
>   struct vduse_dev_msg {

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

[Maxime Coquelin]

On 6/28/23 08:59, Cindy Lu wrote:
> From: Your Name <you@example.com>
> 
> 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 | 49 ++++++++++++++++++++++++++++++
>   1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index f845dc46b1db..1b833bf0ae37 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1313,6 +1313,54 @@ 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 vdpa_reconnect_info *info;
> +	info = &dev->reconnect_info[index];
> +
> +	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +	if (remap_pfn_range(vma, vmf->address & PAGE_MASK, PFN_DOWN(info->addr),
> +			    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_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;
> +
> +	if (vma->vm_end - vma->vm_start != PAGE_SIZE)
> +		return -EINVAL;
> +	if ((vma->vm_flags & VM_SHARED) == 0)
> +		return -EINVAL;
> +
> +	if (index > 65535)
> +		return -EINVAL;

You declare an array of 64 entries in patch 1, so it can overflow.

> +
> +	info = &dev->reconnect_info[index];
> +	if (info->addr & (PAGE_SIZE - 1))
> +		return -EINVAL;
> +	if (vma->vm_end - vma->vm_start != info->size) {
> +		return -ENOTSUPP;
> +	}
> +
> +	vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
> +	vma->vm_ops = &vduse_vm_ops;
> +
> +	return 0;
> +}
> +
>   static int vduse_dev_open(struct inode *inode, struct file *file)
>   {
>   	int ret;
> @@ -1345,6 +1393,7 @@ static const struct file_operations vduse_dev_fops = {
>   	.unlocked_ioctl	= vduse_dev_ioctl,
>   	.compat_ioctl	= compat_ptr_ioctl,
>   	.llseek		= noop_llseek,
> +	.mmap		= vduse_mmap,
>   };
>   
>   static struct vduse_dev *vduse_dev_create(void)