Endianess and coding style fixes for SVQ event idx support

[PATCH 4/4] vhost: convert byte order on avail_event read

Posted by Eugenio Pérez 3 years, 3 months ago

This causes errors on virtio modern devices on big endian hosts

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index 18a49e1ecb..3131903edd 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -231,7 +231,8 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
         size_t num = svq->vring.num;
         uint16_t *avail_event = (uint16_t *)&svq->vring.used->ring[num];
 
-        needs_kick = vring_need_event(*avail_event, svq->shadow_avail_idx,
+        needs_kick = vring_need_event(le16_to_cpu(*avail_event),
+                                      svq->shadow_avail_idx,
                                       svq->shadow_avail_idx - 1);
     } else {
         needs_kick = !(svq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
-- 
2.31.1

Re: [PATCH 4/4] vhost: convert byte order on avail_event read

Posted by Philippe Mathieu-Daudé 3 years, 3 months ago

On 28/10/22 18:02, Eugenio Pérez wrote:
> This causes errors on virtio modern devices on big endian hosts
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-shadow-virtqueue.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index 18a49e1ecb..3131903edd 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -231,7 +231,8 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
>           size_t num = svq->vring.num;
>           uint16_t *avail_event = (uint16_t *)&svq->vring.used->ring[num];
>   

   uint16_t avail_event = virtio_lduw_p(svq->vdev,
                                        &svq->vring.used->ring[num]);
   needs_kick = vring_need_event(avail_event,
                                 svq->shadow_avail_idx,
                                 svq->shadow_avail_idx - 1);

> -        needs_kick = vring_need_event(*avail_event, svq->shadow_avail_idx,
> +        needs_kick = vring_need_event(le16_to_cpu(*avail_event),
> +                                      svq->shadow_avail_idx,
>                                         svq->shadow_avail_idx - 1);
>       } else {
>           needs_kick = !(svq->vring.used->flags & VRING_USED_F_NO_NOTIFY);

Re: [PATCH 4/4] vhost: convert byte order on avail_event read

Posted by Eugenio Perez Martin 3 years, 3 months ago

On Sat, Oct 29, 2022 at 12:53 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> On 28/10/22 18:02, Eugenio Pérez wrote:
> > This causes errors on virtio modern devices on big endian hosts
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-shadow-virtqueue.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > index 18a49e1ecb..3131903edd 100644
> > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > @@ -231,7 +231,8 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
> >           size_t num = svq->vring.num;
> >           uint16_t *avail_event = (uint16_t *)&svq->vring.used->ring[num];
> >
>
>    uint16_t avail_event = virtio_lduw_p(svq->vdev,
>                                         &svq->vring.used->ring[num]);
>    needs_kick = vring_need_event(avail_event,
>                                  svq->shadow_avail_idx,
>                                  svq->shadow_avail_idx - 1);
>

It would work, but just because all vrings must be little endian for
the moment. If we support legacy drivers on a big endian host and
guest in the future, it would not work.

virtio_ld and virtio_st handle the conversions between the guest and
the emulated device in qemu, but this conversion is between qemu
shadow vring and the vdpa device (assuming modern, little endian for
the moment).

Right now the feature set must be the same, but it could not be that
way in the future.

Thanks!

> > -        needs_kick = vring_need_event(*avail_event, svq->shadow_avail_idx,
> > +        needs_kick = vring_need_event(le16_to_cpu(*avail_event),
> > +                                      svq->shadow_avail_idx,
> >                                         svq->shadow_avail_idx - 1);
> >       } else {
> >           needs_kick = !(svq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
>

Re: [PATCH 4/4] vhost: convert byte order on avail_event read

Posted by Michael S. Tsirkin 3 years, 3 months ago

On Mon, Oct 31, 2022 at 09:29:53AM +0100, Eugenio Perez Martin wrote:
> On Sat, Oct 29, 2022 at 12:53 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
> >
> > On 28/10/22 18:02, Eugenio Pérez wrote:
> > > This causes errors on virtio modern devices on big endian hosts
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > >   hw/virtio/vhost-shadow-virtqueue.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > > index 18a49e1ecb..3131903edd 100644
> > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > @@ -231,7 +231,8 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
> > >           size_t num = svq->vring.num;
> > >           uint16_t *avail_event = (uint16_t *)&svq->vring.used->ring[num];
> > >
> >
> >    uint16_t avail_event = virtio_lduw_p(svq->vdev,
> >                                         &svq->vring.used->ring[num]);
> >    needs_kick = vring_need_event(avail_event,
> >                                  svq->shadow_avail_idx,
> >                                  svq->shadow_avail_idx - 1);
> >
> 
> It would work, but just because all vrings must be little endian for
> the moment. If we support legacy drivers on a big endian host and
> guest in the future, it would not work.
> 
> virtio_ld and virtio_st handle the conversions between the guest and
> the emulated device in qemu, but this conversion is between qemu
> shadow vring and the vdpa device (assuming modern, little endian for
> the moment).
> 
> Right now the feature set must be the same, but it could not be that
> way in the future.
> 
> Thanks!


I don't think this works  legacy and virtio data path are similar but
not similar enough to allow switches through svq alone.


> > > -        needs_kick = vring_need_event(*avail_event, svq->shadow_avail_idx,
> > > +        needs_kick = vring_need_event(le16_to_cpu(*avail_event),
> > > +                                      svq->shadow_avail_idx,
> > >                                         svq->shadow_avail_idx - 1);
> > >       } else {
> > >           needs_kick = !(svq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
> >

Re: [PATCH 4/4] vhost: convert byte order on avail_event read

Posted by Jason Wang 3 years, 3 months ago

在 2022/10/31 20:35, Michael S. Tsirkin 写道:
> On Mon, Oct 31, 2022 at 09:29:53AM +0100, Eugenio Perez Martin wrote:
>> On Sat, Oct 29, 2022 at 12:53 AM Philippe Mathieu-Daudé
>> <philmd@linaro.org> wrote:
>>> On 28/10/22 18:02, Eugenio Pérez wrote:
>>>> This causes errors on virtio modern devices on big endian hosts
>>>>
>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> ---
>>>>    hw/virtio/vhost-shadow-virtqueue.c | 3 ++-
>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
>>>> index 18a49e1ecb..3131903edd 100644
>>>> --- a/hw/virtio/vhost-shadow-virtqueue.c
>>>> +++ b/hw/virtio/vhost-shadow-virtqueue.c
>>>> @@ -231,7 +231,8 @@ static void vhost_svq_kick(VhostShadowVirtqueue *svq)
>>>>            size_t num = svq->vring.num;
>>>>            uint16_t *avail_event = (uint16_t *)&svq->vring.used->ring[num];
>>>>
>>>     uint16_t avail_event = virtio_lduw_p(svq->vdev,
>>>                                          &svq->vring.used->ring[num]);
>>>     needs_kick = vring_need_event(avail_event,
>>>                                   svq->shadow_avail_idx,
>>>                                   svq->shadow_avail_idx - 1);
>>>
>> It would work, but just because all vrings must be little endian for
>> the moment. If we support legacy drivers on a big endian host and
>> guest in the future, it would not work.
>>
>> virtio_ld and virtio_st handle the conversions between the guest and
>> the emulated device in qemu, but this conversion is between qemu
>> shadow vring and the vdpa device (assuming modern, little endian for
>> the moment).
>>
>> Right now the feature set must be the same, but it could not be that
>> way in the future.
>>
>> Thanks!
>
> I don't think this works  legacy and virtio data path are similar but
> not similar enough to allow switches through svq alone.


Then we need full copy in that case, looks more like a normal network 
backend instead of registering it to as vhost backend.

Thanks


>
>
>>>> -        needs_kick = vring_need_event(*avail_event, svq->shadow_avail_idx,
>>>> +        needs_kick = vring_need_event(le16_to_cpu(*avail_event),
>>>> +                                      svq->shadow_avail_idx,
>>>>                                          svq->shadow_avail_idx - 1);
>>>>        } else {
>>>>            needs_kick = !(svq->vring.used->flags & VRING_USED_F_NO_NOTIFY);