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

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

[Jason Wang]

On Tue, Sep 12, 2023 at 11:00 AM 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 | 13 +++++++++++++
>  include/uapi/linux/vduse.h         |  6 ++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 2c69f4004a6e..680b23dbdde2 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1221,6 +1221,8 @@ 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;
>
>                 ret = -EFAULT;
>                 if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
> @@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>
>                 vq_info.ready = vq->ready;
>
> +               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) &&
> +                   (vq_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;
> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> index 11bd48c72c6c..d585425803fd 100644
> --- a/include/uapi/linux/vduse.h
> +++ b/include/uapi/linux/vduse.h
> @@ -350,4 +350,10 @@ struct vduse_dev_response {
>         };
>  };
>
> +struct vhost_reconnect_vring {
> +       __u16 reconnect_time;
> +       __u16 last_avail_idx;
> +       _Bool avail_wrap_counter;

Please add a comment for each field.

And I never saw _Bool is used in uapi before, maybe it's better to
pack it with last_avail_idx into a __u32.

Btw, do we need to track inflight descriptors as well?

Thanks

> +};
> +
>  #endif /* _UAPI_VDUSE_H_ */
> --
> 2.34.3
>

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

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

[Jason Wang]

On Tue, Sep 12, 2023 at 11:00 AM 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.

Please explain why this is needed instead of only describing how it is
implemented. (Code can explain itself).

>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 86 ++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 26b7e29cb900..4c256fa31fc4 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,23 @@
>  #define VDUSE_IOVA_SIZE (128 * 1024 * 1024)
>  #define VDUSE_MSG_DEFAULT_TIMEOUT 30
>
> +/* struct vdpa_reconnect_info save the page information for reconnection
> + * kernel will init these information while alloc the pages
> + * and use these information to free the pages
> + */
> +struct vdpa_reconnect_info {
> +       /* Offset (within vm_file) in PAGE_SIZE,
> +        * this just for check, not using
> +        */
> +       u32 index;
> +       /* physical address for this page*/
> +       phys_addr_t addr;
> +       /* virtual address for this page*/
> +       unsigned long vaddr;

If it could be switched by virt_to_phys() why duplicate those fields?

> +       /* memory size, here always page_size*/
> +       phys_addr_t size;

If it's always PAGE_SIZE why would we have this?

> +};
> +
>  struct vduse_virtqueue {
>         u16 index;
>         u16 num_max;
> @@ -57,6 +78,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 +128,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 +1053,65 @@ static int vduse_dev_reg_umem(struct vduse_dev *dev,
>         return ret;
>  }
>
> +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;

-ENOMEM?

Thanks

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

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

[Jason Wang]

On Tue, Sep 12, 2023 at 11:00 AM 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 | 63 ++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 4c256fa31fc4..2c69f4004a6e 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1388,6 +1388,67 @@ 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;
> +
> +       if (index == 0) {
> +               info = &dev->reconnect_status;
> +       } else {
> +               vq = &dev->vqs[index - 1];
> +               info = &vq->reconnect_info;
> +       }
> +       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_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 > 65535)
> +               return -EINVAL;
> +
> +       if (index == 0) {
> +               info = &dev->reconnect_status;
> +       } else {
> +               vq = &dev->vqs[index - 1];
> +               info = &vq->reconnect_info;
> +       }
> +
> +       if (info->index != index)
> +               return -EINVAL;

Under which case could we meet this?

> +
> +       if (info->addr & (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_PFNMAP | VM_DONTDUMP);

Why do you use VM_IO, VM_PFNMAP and VM_DONTDUMP?

Thanks

> +       vma->vm_ops = &vduse_vm_ops;
> +
> +       return 0;
> +}
> +
>  static int vduse_dev_open(struct inode *inode, struct file *file)
>  {
>         int ret;
> @@ -1420,6 +1481,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
>

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

Re: [RFC v2 4/4] vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO

[Jason Wang]

On Tue, Sep 12, 2023 at 11:01 AM Cindy Lu <lulu@redhat.com> wrote:
>
> 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.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 15 +++++++++++++++
>  include/uapi/linux/vduse.h         | 15 +++++++++++++++
>  2 files changed, 30 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index 680b23dbdde2..c99f99892b5c 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1368,6 +1368,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 d585425803fd..ce55e34f63d7 100644
> --- a/include/uapi/linux/vduse.h
> +++ b/include/uapi/linux/vduse.h
> @@ -356,4 +356,19 @@ struct vhost_reconnect_vring {
>         _Bool avail_wrap_counter;
>  };
>
> +/**
> + * struct vduse_reconnect_mmap_info
> + * @size: mapping memory size, always page_size here
> + * @max_index: the number of pages allocated in kernel,just
> + * use for check
> + */
> +
> +struct vduse_reconnect_mmap_info {
> +       __u32 size;
> +       __u32 max_index;
> +};

One thing I didn't understand is that, aren't the things we used to
store connection info belong to uAPI? If not, how can we make sure the
connections work across different vendors/implementations. If yes,
where?

Thanks

> +
> +#define VDUSE_GET_RECONNECT_INFO \
> +       _IOWR(VDUSE_BASE, 0x1b, struct vduse_reconnect_mmap_info)
> +
>  #endif /* _UAPI_VDUSE_H_ */
> --
> 2.34.3
>

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

Re: [RFC v2 4/4] vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO

[Jason Wang]

On Thu, Sep 21, 2023 at 10:07 PM Cindy Lu <lulu@redhat.com> wrote:
>
> On Mon, Sep 18, 2023 at 4:49 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Tue, Sep 12, 2023 at 11:01 AM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > 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.
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > >  drivers/vdpa/vdpa_user/vduse_dev.c | 15 +++++++++++++++
> > >  include/uapi/linux/vduse.h         | 15 +++++++++++++++
> > >  2 files changed, 30 insertions(+)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index 680b23dbdde2..c99f99892b5c 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -1368,6 +1368,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 d585425803fd..ce55e34f63d7 100644
> > > --- a/include/uapi/linux/vduse.h
> > > +++ b/include/uapi/linux/vduse.h
> > > @@ -356,4 +356,19 @@ struct vhost_reconnect_vring {
> > >         _Bool avail_wrap_counter;
> > >  };
> > >
> > > +/**
> > > + * struct vduse_reconnect_mmap_info
> > > + * @size: mapping memory size, always page_size here
> > > + * @max_index: the number of pages allocated in kernel,just
> > > + * use for check
> > > + */
> > > +
> > > +struct vduse_reconnect_mmap_info {
> > > +       __u32 size;
> > > +       __u32 max_index;
> > > +};
> >
> > One thing I didn't understand is that, aren't the things we used to
> > store connection info belong to uAPI? If not, how can we make sure the
> > connections work across different vendors/implementations. If yes,
> > where?
> >
> > Thanks
> >
> The process for this reconnecttion  is
> A.The first-time connection
> 1> The userland app checks if the device exists
> 2>  use the ioctl to create the vduse device
> 3> Mapping the kernel page to userland and save the
> App-version/features/other information to this page
> 4>  if the Userland app needs to exit, then the Userland app will only
> unmap the page and then exit
>
> B, the re-connection
> 1> the userland app finds the device is existing
> 2> Mapping the kernel page to userland
> 3> check if the information in shared memory is satisfied to
> reconnect,if ok then continue to reconnect
> 4> continue working
>
>  For now these information are all from userland,So here the page will
> be maintained by the userland App
> in the previous code we only saved the api-version by uAPI .  if  we
> need to support reconnection maybe we need to add 2 new uAPI for this,
> one of the uAPI is to save the reconnect  information and another is
> to get the information
>
> maybe something like
>
> struct vhost_reconnect_data {
> uint32_t version;
> uint64_t features;
> uint8_t status;
> struct virtio_net_config config;
> uint32_t nr_vrings;
> };

Probably, then we can make sure the re-connection works across
different vduse-daemon implementations.

>
> #define VDUSE_GET_RECONNECT_INFO _IOR (VDUSE_BASE, 0x1c, struct
> vhost_reconnect_data)
>
> #define VDUSE_SET_RECONNECT_INFO  _IOWR(VDUSE_BASE, 0x1d, struct
> vhost_reconnect_data)

Not sure I get this, but the idea is to map those pages to user space,
any reason we need this uAPI?

Thanks

>
> Thanks
> Cindy
>
>
>
>
> > > +
> > > +#define VDUSE_GET_RECONNECT_INFO \
> > > +       _IOWR(VDUSE_BASE, 0x1b, struct vduse_reconnect_mmap_info)
> > > +
> > >  #endif /* _UAPI_VDUSE_H_ */
> > > --
> > > 2.34.3
> > >
> >
>

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

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

[Maxime Coquelin]

On 9/25/23 06:15, Cindy Lu wrote:
> On Tue, Sep 12, 2023 at 3:39 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On Tue, Sep 12, 2023 at 11:00 AM 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 | 13 +++++++++++++
>>>   include/uapi/linux/vduse.h         |  6 ++++++
>>>   2 files changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>>> index 2c69f4004a6e..680b23dbdde2 100644
>>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>>> @@ -1221,6 +1221,8 @@ 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;
>>>
>>>                  ret = -EFAULT;
>>>                  if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
>>> @@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>>>
>>>                  vq_info.ready = vq->ready;
>>>
>>> +               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) &&
>>> +                   (vq_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;
>>> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
>>> index 11bd48c72c6c..d585425803fd 100644
>>> --- a/include/uapi/linux/vduse.h
>>> +++ b/include/uapi/linux/vduse.h
>>> @@ -350,4 +350,10 @@ struct vduse_dev_response {
>>>          };
>>>   };
>>>
>>> +struct vhost_reconnect_vring {
>>> +       __u16 reconnect_time;
>>> +       __u16 last_avail_idx;
>>> +       _Bool avail_wrap_counter;
>>
>> Please add a comment for each field.
>>
> Sure will do
> 
>> And I never saw _Bool is used in uapi before, maybe it's better to
>> pack it with last_avail_idx into a __u32.
>>
> Thanks will fix this
>> Btw, do we need to track inflight descriptors as well?
>>
> I will check this

For existing networking implemenation, this is not necessary.
But it should be for block devices.

Maxime

> Thanks
> 
> cindy
>> Thanks
>>
>>> +};
>>> +
>>>   #endif /* _UAPI_VDUSE_H_ */
>>> --
>>> 2.34.3
>>>
>>
> 

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

Re: [RFC v2 4/4] vduse: Add new ioctl VDUSE_GET_RECONNECT_INFO

[Maxime Coquelin]

On 9/25/23 04:57, Jason Wang wrote:
> On Thu, Sep 21, 2023 at 10:07 PM Cindy Lu <lulu@redhat.com> wrote:
>>
>> On Mon, Sep 18, 2023 at 4:49 PM Jason Wang <jasowang@redhat.com> wrote:
>>>
>>> On Tue, Sep 12, 2023 at 11:01 AM Cindy Lu <lulu@redhat.com> wrote:
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Cindy Lu <lulu@redhat.com>
>>>> ---
>>>>   drivers/vdpa/vdpa_user/vduse_dev.c | 15 +++++++++++++++
>>>>   include/uapi/linux/vduse.h         | 15 +++++++++++++++
>>>>   2 files changed, 30 insertions(+)
>>>>
>>>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>>>> index 680b23dbdde2..c99f99892b5c 100644
>>>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>>>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>>>> @@ -1368,6 +1368,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 d585425803fd..ce55e34f63d7 100644
>>>> --- a/include/uapi/linux/vduse.h
>>>> +++ b/include/uapi/linux/vduse.h
>>>> @@ -356,4 +356,19 @@ struct vhost_reconnect_vring {
>>>>          _Bool avail_wrap_counter;
>>>>   };
>>>>
>>>> +/**
>>>> + * struct vduse_reconnect_mmap_info
>>>> + * @size: mapping memory size, always page_size here
>>>> + * @max_index: the number of pages allocated in kernel,just
>>>> + * use for check
>>>> + */
>>>> +
>>>> +struct vduse_reconnect_mmap_info {
>>>> +       __u32 size;
>>>> +       __u32 max_index;
>>>> +};
>>>
>>> One thing I didn't understand is that, aren't the things we used to
>>> store connection info belong to uAPI? If not, how can we make sure the
>>> connections work across different vendors/implementations. If yes,
>>> where?
>>>
>>> Thanks
>>>
>> The process for this reconnecttion  is
>> A.The first-time connection
>> 1> The userland app checks if the device exists
>> 2>  use the ioctl to create the vduse device
>> 3> Mapping the kernel page to userland and save the
>> App-version/features/other information to this page
>> 4>  if the Userland app needs to exit, then the Userland app will only
>> unmap the page and then exit
>>
>> B, the re-connection
>> 1> the userland app finds the device is existing
>> 2> Mapping the kernel page to userland
>> 3> check if the information in shared memory is satisfied to
>> reconnect,if ok then continue to reconnect
>> 4> continue working
>>
>>   For now these information are all from userland,So here the page will
>> be maintained by the userland App
>> in the previous code we only saved the api-version by uAPI .  if  we
>> need to support reconnection maybe we need to add 2 new uAPI for this,
>> one of the uAPI is to save the reconnect  information and another is
>> to get the information
>>
>> maybe something like
>>
>> struct vhost_reconnect_data {
>> uint32_t version;
>> uint64_t features;
>> uint8_t status;
>> struct virtio_net_config config;
>> uint32_t nr_vrings;
>> };
> 
> Probably, then we can make sure the re-connection works across
> different vduse-daemon implementations.

+1, we need to have this defined in the uAPI to support interoperability
across different VDUSE userspace implementations.

> 
>>
>> #define VDUSE_GET_RECONNECT_INFO _IOR (VDUSE_BASE, 0x1c, struct
>> vhost_reconnect_data)
>>
>> #define VDUSE_SET_RECONNECT_INFO  _IOWR(VDUSE_BASE, 0x1d, struct
>> vhost_reconnect_data)
> 
> Not sure I get this, but the idea is to map those pages to user space,
> any reason we need this uAPI?

It should not be necessary if the mmapped layout is properly defined.

Thanks,
Maxime

> Thanks
> 
>>
>> Thanks
>> Cindy
>>
>>
>>
>>
>>>> +
>>>> +#define VDUSE_GET_RECONNECT_INFO \
>>>> +       _IOWR(VDUSE_BASE, 0x1b, struct vduse_reconnect_mmap_info)
>>>> +
>>>>   #endif /* _UAPI_VDUSE_H_ */
>>>> --
>>>> 2.34.3
>>>>
>>>
>>
> 

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

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

[Maxime Coquelin]

On 9/12/23 09:39, Jason Wang wrote:
> On Tue, Sep 12, 2023 at 11:00 AM 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 | 13 +++++++++++++
>>   include/uapi/linux/vduse.h         |  6 ++++++
>>   2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>> index 2c69f4004a6e..680b23dbdde2 100644
>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>> @@ -1221,6 +1221,8 @@ 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;
>>
>>                  ret = -EFAULT;
>>                  if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
>> @@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>>
>>                  vq_info.ready = vq->ready;
>>
>> +               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) &&
>> +                   (vq_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;
>> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
>> index 11bd48c72c6c..d585425803fd 100644
>> --- a/include/uapi/linux/vduse.h
>> +++ b/include/uapi/linux/vduse.h
>> @@ -350,4 +350,10 @@ struct vduse_dev_response {
>>          };
>>   };
>>
>> +struct vhost_reconnect_vring {
>> +       __u16 reconnect_time;
>> +       __u16 last_avail_idx;
>> +       _Bool avail_wrap_counter;
> 
> Please add a comment for each field.
> 
> And I never saw _Bool is used in uapi before, maybe it's better to
> pack it with last_avail_idx into a __u32.

Better as two distincts __u16 IMHO.

Thanks,
Maxime

> 
> Btw, do we need to track inflight descriptors as well?
> 
> Thanks
> 
>> +};
>> +
>>   #endif /* _UAPI_VDUSE_H_ */
>> --
>> 2.34.3
>>
> 

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

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

[Jason Wang]

On Fri, Sep 29, 2023 at 5:12 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
>
>
> On 9/12/23 09:39, Jason Wang wrote:
> > On Tue, Sep 12, 2023 at 11:00 AM 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 | 13 +++++++++++++
> >>   include/uapi/linux/vduse.h         |  6 ++++++
> >>   2 files changed, 19 insertions(+)
> >>
> >> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> >> index 2c69f4004a6e..680b23dbdde2 100644
> >> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> >> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> >> @@ -1221,6 +1221,8 @@ 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;
> >>
> >>                  ret = -EFAULT;
> >>                  if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
> >> @@ -1252,6 +1254,17 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> >>
> >>                  vq_info.ready = vq->ready;
> >>
> >> +               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) &&
> >> +                   (vq_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;
> >> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> >> index 11bd48c72c6c..d585425803fd 100644
> >> --- a/include/uapi/linux/vduse.h
> >> +++ b/include/uapi/linux/vduse.h
> >> @@ -350,4 +350,10 @@ struct vduse_dev_response {
> >>          };
> >>   };
> >>
> >> +struct vhost_reconnect_vring {
> >> +       __u16 reconnect_time;
> >> +       __u16 last_avail_idx;
> >> +       _Bool avail_wrap_counter;
> >
> > Please add a comment for each field.
> >
> > And I never saw _Bool is used in uapi before, maybe it's better to
> > pack it with last_avail_idx into a __u32.
>
> Better as two distincts __u16 IMHO.

Fine with me.

Thanks

>
> Thanks,
> Maxime
>
> >
> > Btw, do we need to track inflight descriptors as well?
> >
> > Thanks
> >
> >> +};
> >> +
> >>   #endif /* _UAPI_VDUSE_H_ */
> >> --
> >> 2.34.3
> >>
> >
>

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