| Message ID | 20221028160251.268607-5-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Endianess and coding style fixes for SVQ event idx support | expand |
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);
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); >
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); > >
在 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);
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);
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(-)