Re: [PATCH 2/4] vhost: convert byte order on SVQ used event write

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Oct 31, 2022 at 02:02:16PM +0100, Eugenio Perez Martin wrote:
> On Mon, Oct 31, 2022 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Oct 31, 2022 at 09:54:34AM +0100, Eugenio Perez Martin wrote:
> > > On Sat, Oct 29, 2022 at 12:48 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
> > > > >
> > > > > Fixes: 01f8beacea2a ("vhost: toggle device callbacks using used event idx")
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > ---
> > > > >   hw/virtio/vhost-shadow-virtqueue.c | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> > > > > index 70766ea740..467099f5d9 100644
> > > > > --- a/hw/virtio/vhost-shadow-virtqueue.c
> > > > > +++ b/hw/virtio/vhost-shadow-virtqueue.c
> > > > > @@ -382,7 +382,7 @@ static bool vhost_svq_enable_notification(VhostShadowVirtqueue *svq)
> > > > >   {
> > > > >       if (virtio_vdev_has_feature(svq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > > > >           uint16_t *used_event = &svq->vring.avail->ring[svq->vring.num];
> > > > > -        *used_event = svq->shadow_used_idx;
> > > > > +        *used_event = cpu_to_le16(svq->shadow_used_idx);
> > > >
> > > > This looks correct, but what about:
> > > >
> > > >             virtio_stw_p(svq->vdev, used_event, svq->shadow_used_idx);
> > > >
> > >
> > > Hi Philippe,
> > >
> > > I think this has the same answer as [1], the endianness conversion
> > > from the guest to the host may not be the same as the one needed from
> > > qemu SVQ to the vdpa device. Please let me know if it is not the case.
> > >
> > > Thanks!
> > >
> > > [1] https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg06081.html
> >
> > So considering legacy, i do not belive you can make a legacy
> > device on top of modern one using SVQ alone.
> >
> 
> Right, more work is needed. For example, config r/w conversions. But
> it's a valid use case where SVQ helps too.

I am not sure why it's valid frankly.

> > So I'd say SVQ should follow virtio endian-ness, not LE.
> 
> At this moment both the device that the guest sees and the vdpa device
> must be modern ones to enable SVQ. So the event idx must be stored in
> the vring in LE. Similar access functions as virtio_ld* and virtio_st*
> are needed if SVQ supports legacy vdpa devices in the future.
> 
> The point is that svq->shadow_avail_idx is decoupled from the guest's
> avail ring, event idx, etc. It will always be in the host's CPU
> endianness, regardless of the guest's one. And, for the moment, the
> event idx write must be in LE.
> 
> There is a more fundamental problem about using virtio_{st,ld}* here:
> These read from and write to guest's memory, but neither
> svq->shadow_used_idx or shadow vring are in guest's memory but only in
> qemu's VA. To start the support of legacy vdpa devices would involve a
> deeper change here, since all shadow vring writes and reads are
> written this way.
> 
> Thanks!

Yea generally, I don't know how it can work given legacy
will never attach a PASID to a VQ.

But maybe given we add yet another variant of endian-ness
it is time to actually use sparse tags for this stuff.

> >
> >
> > > > >       } else {
> > > > >           svq->vring.avail->flags &= ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > >       }
> > > >
> >