[PATCH 0/2] Search the split vq desc and avail in RO areas

[PATCH 0/2] Search the split vq desc and avail in RO areas

[Eugenio Pérez]

QEMU's shadow virtqueue and VDUSE exposes these areas as read-only. If
we don't change it, vhost_iova_to_vva do not consider them as valid and
returns that they're not found.

Eugenio Pérez (2):
  vhost: search the split vq desc and avail in RO areas
  vhost: search the packed vq driver area in RO areas

 lib/vhost/vhost.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.49.0

[PATCH 1/2] vhost: search the split vq desc and avail in RO areas

[Eugenio Pérez]

QEMU's shadow virtqueue and VDUSE exposes these areas as read-only. If
we don't change it, vhost_iova_to_vva do not consider them as valid and
returns that they're not found.

Fixes: eefac9536a90 ("vhost: postpone device creation until rings are mapped")
Cc: stable@dpdk.org

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 lib/vhost/vhost.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 0353a04dc8..95a99bace6 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -497,7 +497,7 @@ vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
 	size = req_size;
 	vq->desc = (struct vring_desc *)(uintptr_t)vhost_iova_to_vva(dev, vq,
 						vq->ring_addrs.desc_user_addr,
-						&size, VHOST_ACCESS_RW);
+						&size, VHOST_ACCESS_RO);
 	if (!vq->desc || size != req_size)
 		return -1;
 
@@ -508,7 +508,7 @@ vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
 	size = req_size;
 	vq->avail = (struct vring_avail *)(uintptr_t)vhost_iova_to_vva(dev, vq,
 						vq->ring_addrs.avail_user_addr,
-						&size, VHOST_ACCESS_RW);
+						&size, VHOST_ACCESS_RO);
 	if (!vq->avail || size != req_size)
 		return -1;
 
-- 
2.49.0

Re: [PATCH 1/2] vhost: search the split vq desc and avail in RO areas

[Maxime Coquelin]

On 6/5/25 1:35 PM, Eugenio Pérez wrote:
> QEMU's shadow virtqueue and VDUSE exposes these areas as read-only. If
> we don't change it, vhost_iova_to_vva do not consider them as valid and
> returns that they're not found.
> 
> Fixes: eefac9536a90 ("vhost: postpone device creation until rings are mapped")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   lib/vhost/vhost.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 0353a04dc8..95a99bace6 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -497,7 +497,7 @@ vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
>   	size = req_size;
>   	vq->desc = (struct vring_desc *)(uintptr_t)vhost_iova_to_vva(dev, vq,
>   						vq->ring_addrs.desc_user_addr,
> -						&size, VHOST_ACCESS_RW);
> +						&size, VHOST_ACCESS_RO);
>   	if (!vq->desc || size != req_size)
>   		return -1;
>   
> @@ -508,7 +508,7 @@ vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
>   	size = req_size;
>   	vq->avail = (struct vring_avail *)(uintptr_t)vhost_iova_to_vva(dev, vq,
>   						vq->ring_addrs.avail_user_addr,
> -						&size, VHOST_ACCESS_RW);
> +						&size, VHOST_ACCESS_RO);
>   	if (!vq->avail || size != req_size)
>   		return -1;
>   

I propose below text for the commit message:

"
The virtqueues driver areas are read-only for the device.
While they are exposed as Read/Write to Vhost-user and regular VDUSE
backends, they are only exposed as Read-only when the control Virtqueue
is shadowed by QEMU with VDUSE backend.

This patch makes the backend to request these areas as Read-only, so 
that it can be accessed in all the configurations.
"

Also, as requested by David, patch 2 will be squashed into this one.

Eugenio, is that Ok for you?

With above suggested changes:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [PATCH 1/2] vhost: search the split vq desc and avail in RO areas

[Eugenio Perez Martin]

On Thu, Jun 12, 2025 at 3:56 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
>
>
> On 6/5/25 1:35 PM, Eugenio Pérez wrote:
> > QEMU's shadow virtqueue and VDUSE exposes these areas as read-only. If
> > we don't change it, vhost_iova_to_vva do not consider them as valid and
> > returns that they're not found.
> >
> > Fixes: eefac9536a90 ("vhost: postpone device creation until rings are mapped")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   lib/vhost/vhost.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> > index 0353a04dc8..95a99bace6 100644
> > --- a/lib/vhost/vhost.c
> > +++ b/lib/vhost/vhost.c
> > @@ -497,7 +497,7 @@ vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
> >       size = req_size;
> >       vq->desc = (struct vring_desc *)(uintptr_t)vhost_iova_to_vva(dev, vq,
> >                                               vq->ring_addrs.desc_user_addr,
> > -                                             &size, VHOST_ACCESS_RW);
> > +                                             &size, VHOST_ACCESS_RO);
> >       if (!vq->desc || size != req_size)
> >               return -1;
> >
> > @@ -508,7 +508,7 @@ vring_translate_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
> >       size = req_size;
> >       vq->avail = (struct vring_avail *)(uintptr_t)vhost_iova_to_vva(dev, vq,
> >                                               vq->ring_addrs.avail_user_addr,
> > -                                             &size, VHOST_ACCESS_RW);
> > +                                             &size, VHOST_ACCESS_RO);
> >       if (!vq->avail || size != req_size)
> >               return -1;
> >
>
> I propose below text for the commit message:
>
> "
> The virtqueues driver areas are read-only for the device.
> While they are exposed as Read/Write to Vhost-user and regular VDUSE
> backends, they are only exposed as Read-only when the control Virtqueue
> is shadowed by QEMU with VDUSE backend.
>
> This patch makes the backend to request these areas as Read-only, so
> that it can be accessed in all the configurations.
> "
>
> Also, as requested by David, patch 2 will be squashed into this one.
>
> Eugenio, is that Ok for you?
>

Sure, I'm ok with that.

Thanks!

> With above suggested changes:
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> Thanks,
> Maxime
>

[PATCH 2/2] vhost: search the packed vq driver area in RO areas

[Eugenio Pérez]

QEMU's shadow virtqueue and VDUSE exposes this as read-only. If we
don't change it, vhost_iova_to_vva do not consider them as valid and
returns that they're not found.

Fixes: 2d1541e2b6b3 ("vhost: add vring address setup for packed queues")
Cc: stable@dpdk.org

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 lib/vhost/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 95a99bace6..a2e3e2635d 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -544,7 +544,7 @@ vring_translate_packed(struct virtio_net *dev, struct vhost_virtqueue *vq)
 	size = req_size;
 	vq->driver_event = (struct vring_packed_desc_event *)(uintptr_t)
 		vhost_iova_to_vva(dev, vq, vq->ring_addrs.avail_user_addr,
-				&size, VHOST_ACCESS_RW);
+				&size, VHOST_ACCESS_RO);
 	if (!vq->driver_event || size != req_size)
 		return -1;
 
-- 
2.49.0

Re: [PATCH 2/2] vhost: search the packed vq driver area in RO areas

[Maxime Coquelin]

On 6/5/25 1:35 PM, Eugenio Pérez wrote:
> QEMU's shadow virtqueue and VDUSE exposes this as read-only. If we
> don't change it, vhost_iova_to_vva do not consider them as valid and
> returns that they're not found.
> 
> Fixes: 2d1541e2b6b3 ("vhost: add vring address setup for packed queues")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   lib/vhost/vhost.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 95a99bace6..a2e3e2635d 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -544,7 +544,7 @@ vring_translate_packed(struct virtio_net *dev, struct vhost_virtqueue *vq)
>   	size = req_size;
>   	vq->driver_event = (struct vring_packed_desc_event *)(uintptr_t)
>   		vhost_iova_to_vva(dev, vq, vq->ring_addrs.avail_user_addr,
> -				&size, VHOST_ACCESS_RW);
> +				&size, VHOST_ACCESS_RO);
>   	if (!vq->driver_event || size != req_size)
>   		return -1;
>   

Squashed into patch 1.

Thanks,
Maxime

Re: [PATCH 0/2] Search the split vq desc and avail in RO areas

[David Marchand]

On Thu, Jun 5, 2025 at 1:36 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> QEMU's shadow virtqueue and VDUSE exposes these areas as read-only. If
> we don't change it, vhost_iova_to_vva do not consider them as valid and
> returns that they're not found.
>
> Eugenio Pérez (2):
>   vhost: search the split vq desc and avail in RO areas
>   vhost: search the packed vq driver area in RO areas
>
>  lib/vhost/vhost.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

At a quick glance, no need for two patches.

Sorry, the implication of this issue is not clear to me.
What is the impact from a user pov?


-- 
David Marchand

Re: [PATCH 0/2] Search the split vq desc and avail in RO areas

[Eugenio Perez Martin]

On Thu, Jun 5, 2025 at 1:50 PM David Marchand <david.marchand@redhat.com> wrote:
>
> On Thu, Jun 5, 2025 at 1:36 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > QEMU's shadow virtqueue and VDUSE exposes these areas as read-only. If
> > we don't change it, vhost_iova_to_vva do not consider them as valid and
> > returns that they're not found.
> >
> > Eugenio Pérez (2):
> >   vhost: search the split vq desc and avail in RO areas
> >   vhost: search the packed vq driver area in RO areas
> >
> >  lib/vhost/vhost.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
>
> At a quick glance, no need for two patches.
>

Sure I can respin with both squashed.

> Sorry, the implication of this issue is not clear to me.
> What is the impact from a user pov?
>

QEMU maps the CVQ descriptors and avail vring through as read only
maps in the case of vDPA. But DPDK is looking for them with RW
permissions, so the vhost_iova_to_vva function never selects the right
one as valid.

Looking for them with RO still picks the map if it is mapped as RW,
but the reverse is not true.

Let me know if you want me to respin the series with this comment too!

>
> --
> David Marchand
>

Re: [PATCH 0/2] Search the split vq desc and avail in RO areas

[David Marchand]

On Thu, Jun 5, 2025 at 4:55 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Thu, Jun 5, 2025 at 1:50 PM David Marchand <david.marchand@redhat.com> wrote:
> >
> > On Thu, Jun 5, 2025 at 1:36 PM Eugenio Pérez <eperezma@redhat.com> wrote:
> > >
> > > QEMU's shadow virtqueue and VDUSE exposes these areas as read-only. If
> > > we don't change it, vhost_iova_to_vva do not consider them as valid and
> > > returns that they're not found.
> > >
> > > Eugenio Pérez (2):
> > >   vhost: search the split vq desc and avail in RO areas
> > >   vhost: search the packed vq driver area in RO areas
> > >
> > >  lib/vhost/vhost.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > At a quick glance, no need for two patches.
> >
>
> Sure I can respin with both squashed.
>
> > Sorry, the implication of this issue is not clear to me.
> > What is the impact from a user pov?
> >
>
> QEMU maps the CVQ descriptors and avail vring through as read only
> maps in the case of vDPA. But DPDK is looking for them with RW
> permissions, so the vhost_iova_to_vva function never selects the right
> one as valid.
>
> Looking for them with RO still picks the map if it is mapped as RW,
> but the reverse is not true.

Ok, thanks.
Well, it's better, but still hard to tell how this impacts existing
usecases :-).
After discussing with Maxime, I understand that the shadow CVQ just
can't work => blocking multi queue support with vduse for example.
This is the type of high level impact I was looking for.

> Let me know if you want me to respin the series with this comment too!

Ideally yes, but maybe Maxime can do it when applying.


-- 
David Marchand

Re: [PATCH 0/2] Search the split vq desc and avail in RO areas

[Maxime Coquelin]

On 6/6/25 4:20 PM, David Marchand wrote:
> On Thu, Jun 5, 2025 at 4:55 PM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>>
>> On Thu, Jun 5, 2025 at 1:50 PM David Marchand <david.marchand@redhat.com> wrote:
>>>
>>> On Thu, Jun 5, 2025 at 1:36 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>>>>
>>>> QEMU's shadow virtqueue and VDUSE exposes these areas as read-only. If
>>>> we don't change it, vhost_iova_to_vva do not consider them as valid and
>>>> returns that they're not found.
>>>>
>>>> Eugenio Pérez (2):
>>>>    vhost: search the split vq desc and avail in RO areas
>>>>    vhost: search the packed vq driver area in RO areas
>>>>
>>>>   lib/vhost/vhost.c | 6 +++---
>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> At a quick glance, no need for two patches.
>>>
>>
>> Sure I can respin with both squashed.
>>
>>> Sorry, the implication of this issue is not clear to me.
>>> What is the impact from a user pov?
>>>
>>
>> QEMU maps the CVQ descriptors and avail vring through as read only
>> maps in the case of vDPA. But DPDK is looking for them with RW
>> permissions, so the vhost_iova_to_vva function never selects the right
>> one as valid.
>>
>> Looking for them with RO still picks the map if it is mapped as RW,
>> but the reverse is not true.
> 
> Ok, thanks.
> Well, it's better, but still hard to tell how this impacts existing
> usecases :-).
> After discussing with Maxime, I understand that the shadow CVQ just
> can't work => blocking multi queue support with vduse for example.
> This is the type of high level impact I was looking for.
> 
>> Let me know if you want me to respin the series with this comment too!
> 
> Ideally yes, but maybe Maxime can do it when applying.
> 
> 

Yes, I can rework the commit message while applying.

Thanks,
Maxime

Re: [PATCH 0/2] Search the split vq desc and avail in RO areas

[Maxime Coquelin]

On 6/5/25 1:35 PM, Eugenio Pérez wrote:
> QEMU's shadow virtqueue and VDUSE exposes these areas as read-only. If
> we don't change it, vhost_iova_to_vva do not consider them as valid and
> returns that they're not found.
> 
> Eugenio Pérez (2):
>    vhost: search the split vq desc and avail in RO areas
>    vhost: search the packed vq driver area in RO areas
> 
>   lib/vhost/vhost.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 

Applied to next-virtio/for-net-next with suggested changes.

Thomas, patch 2 has been squashed into patch 1.

Thanks,
Maxime