On Mon, Oct 31, 2022 at 4:09 PM Michael S. Tsirkin <m...@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 <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.
>
> 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.
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,
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. 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);
> > > > > > }
> > > > >
> > >
>
