On Mon, Oct 31, 2022 at 1:33 PM Michael S. Tsirkin <m...@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é
> > <phi...@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 <epere...@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.
> 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!
>
>
> > > > } else {
> > > > svq->vring.avail->flags &=
> > > > ~cpu_to_le16(VRING_AVAIL_F_NO_INTERRUPT);
> > > > }
> > >
>
