[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CACGkMEvycvCUjjdKvK50dmoQbDL79Xc9s4YDMVed9BUv+Gf0yg@mail.gmail.com>
Date: Thu, 7 Dec 2023 16:46:15 +0800
From: Jason Wang <jasowang@...hat.com>
To: Cindy Lu <lulu@...hat.com>
Cc: mst@...hat.com, xieyongji@...edance.com,
linux-kernel@...r.kernel.org, maxime.coquelin@...hat.com
Subject: Re: [PATCH v3 6/7] vduse: Update the vq_info in ioctl VDUSE_VQ_GET_INFO
On Tue, Dec 5, 2023 at 4:35 PM Cindy Lu <lulu@...hat.com> 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@...hat.com>
> ---
> 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
>
Powered by blists - more mailing lists