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

[Jason Wang]

在 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);