Re: [PATCH v3 6/7] vduse: Update the vq_info in ioctl VDUSE_VQ_GET_INFO

[Jason Wang]

On Tue, Dec 5, 2023 at 4:35 PM Cindy Lu <lulu@xxxxxxxxxx> wrote:
>
> Once the reconnect memory pages are mapped to userspace, the userspace
> application will update the "reconnected" bit in the
> "struct vduse_dev_reconnect_data".
> The kernel will then check this bit. If it is not set to
> "VDUSE_NOT_RECONNECT", it means that the application has been
> reconnected, and the kernel will synchronize the vq information.

Can you explain why we need such a flag?

>
> Signed-off-by: Cindy Lu <lulu@xxxxxxxxxx>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index f55f415629de..422f1aedebac 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1193,6 +1193,9 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                 struct vduse_vq_info vq_info;
>                 struct vduse_virtqueue *vq;
>                 u32 index;
> +               unsigned long vaddr;
> +               struct vduse_vq_state *vq_reconnect;
> +               struct vduse_dev_reconnect_data *dev_reconnect;
>
>                 ret = -EFAULT;
>                 if (copy_from_user(&vq_info, argp, sizeof(vq_info)))
> @@ -1209,6 +1212,12 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                 vq_info.device_addr = vq->device_addr;
>                 vq_info.num = vq->num;
>
> +               vaddr = dev->vdpa_reconnect_vaddr;
> +               dev_reconnect = (struct vduse_dev_reconnect_data *)vaddr;
> +
> +               vaddr = vq->vdpa_reconnect_vaddr;
> +               vq_reconnect = (struct vduse_vq_state *)vaddr;
> +
>                 if (dev->driver_features & BIT_ULL(VIRTIO_F_RING_PACKED)) {
>                         vq_info.packed.last_avail_counter =
>                                 vq->state.packed.last_avail_counter;
> @@ -1218,9 +1227,22 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                                 vq->state.packed.last_used_counter;
>                         vq_info.packed.last_used_idx =
>                                 vq->state.packed.last_used_idx;
> -               } else
> +                       /*check if the vq is reconnect, if yes then update the info*/
> +                       if (dev_reconnect->reconnected != VDUSE_NOT_RECONNECT) {

So the dev_reconnect is shared, how to synchronize between them?

> +                               vq_info.packed.last_avail_idx =
> +                                       vq_reconnect->packed.last_avail_idx;
> +                               vq_info.packed.last_avail_counter =
> +                                       vq_reconnect->packed.last_avail_counter;
> +                       }
> +               } else {
>                         vq_info.split.avail_index =
>                                 vq->state.split.avail_index;
> +                       /*check if the vq is reconnect, if yes then update the info*/
> +                       if (dev_reconnect->reconnected != VDUSE_NOT_RECONNECT) {

If this only check is !=, I'd define it as VDUSE_RECONNECT to ease the
code logic.

> +                               vq_info.split.avail_index =
> +                                       vq_reconnect->split.avail_index;
> +                       }
> +               }

It looks like I miss something here

If I read the code correctly, vq_reconnect is written by userspace, so
userspace query it again via the get_vq_info()? If this is true,
userspace can just ignore the index via GET_VQ_INFO() or is it just
for consistency?

Thanks


>
>                 vq_info.ready = vq->ready;
>
> --
> 2.34.3
>