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 <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.
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);
> > > > > > > }
> > > > > >
> > > >
> >
