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

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
>

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

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
>

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

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
>

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

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
>

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

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
> > >
> >
>

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

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 {

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

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)

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