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

[Michael S. Tsirkin]

On Mon, Oct 31, 2022 at 05:05:42PM +0100, Eugenio Perez Martin wrote:
> On Mon, Oct 31, 2022 at 4:09 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > 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.
> >
> 
> The conversion I tried to put here was legacy guests communicating in
> big endian with qemu, and then qemu communicating in little endian
> with modern devices. For this to work SVQ should be enabled for all
> the queues all the time.

Yes I got that.  This won't work so easily just because e.g. network
header is slightly different, so it's more than just descriptor
translations even just on data path.

> Then the simplest conversion function here should be cpu_to_leNN,
> isn't it? The only device we support here is a modern, little endian,

At the moment vdpa only properly works with modern. But really
another way to support legacy is if a device has support, and
to fix vdpa to support legacy.

Will we ever do that? Will anyone bother? I don't know.


> But maybe my example just added more noise. My point is that this
> write and all the writes and loads added on these patches, have
> nothing to do with the guest's endianness.

Device might support programmable endian-ness. If it does,
then yes we could thinkably have yet another type of
endian-ness "device endian" but practically setting it to
anything except guest endian is just inviting pain.

> They are only from the SVQ
> vring to the device. And they are not forwarding the used_event of the
> guest, but another decoupled one that may or may not match. That's why
> the endianness we should take into account is not the vdev one, but
> only the CPU and little endian.
> 
> > But maybe given we add yet another variant of endian-ness
> > it is time to actually use sparse tags for this stuff.
> >
> 
> I agree with this. I can try to do a fast POC.
> 
> Thanks!
> 
> 
> > > >
> > > >
> > > > > > >       } else {
> > > > > > >           svq->vring.avail->flags &= 
> > > > > > > ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > > > >       }
> > > > > >
> > > >
> >